Address review feedback: Plan: MVP Vertical Slice: Client-Server Wiring #28

2026-08-19T14:31:34Z

planner-bot commented

2026-08-19 14:31:34 +00:00

Auto-opened by planner for task 76aa20cd2e45.

Dashboard: http://cloudbison:7070/task/76aa20cd2e45

Reviewer requested changes on PR #26 for this task.

Feedback:
The planning doc itself is excellent — I verified every backend-contract claim against the code and it's all correct: register (JSON, YYYY-MM-DD DOB, 201 token, under-18→400), login (OAuth2 form-encoded), GET /api/worlds (requires CurrentUser, returns id/name/description/visibility/owner_id/created_at), and GET /api/health (unauthenticated). Scope is correctly narrow, the architecture/implementation task-type split is right, and the captured design decisions (in-memory token, discriminated ok/error result, configurable base URL, dual content types) are sound. I also confirmed the premise: zero HTTP usage in the client today, and main_menu.gd has no auth UI yet.

However, the actual PM task breakdown doesn't match the doc, and that gap matters:

The doc's task 5 — "End-to-end headless smoke test + manual verify" — was never created in the PM system. Only 3 tasks exist (ApiClient, Login/Register UI, Browse worlds). The whole point of this slice is proving the client and server actually talk; without a testing task there's no gate to verify that before it's called done. Please create it.
Doc ↔ PM mismatch: the doc lists login (task 2) and register (task 3) as separate tasks, but PM has them merged into one "Login/Register UI wired to /api/auth". Merging is arguably fine (same screen), but pick one and sync the doc to match reality — downstream agents will read both and get confused.
Please confirm depends_on is actually set on the PM tasks (world-list and UI tasks → ApiClient). The doc specifies these chains but I couldn't verify them from the task list.
Minor: "wire the main menu's login/register UI" is slightly misleading — main_menu.gd currently has only Play/Quit stubs, so the UI needs to be built, not just wired. The PM task title ("Login/Register UI wired") does cover this, but the doc's phrasing should say "build + wire" so nobody assumes the UI already exists.

Your own branch already contains this PR's code (it was branched from agent/21694e349045, not from scratch) — no need to fetch, diff against, or go looking for that branch separately. Just read the files directly and apply the fixes below.
PR (for reference only): daddy/agp-game#26

Original task description:
Break this feature down into well-defined user stories and right-sized tasks (pm_create_story / pm_create_task).

Feature: MVP Vertical Slice: Client-Server Wiring
The Godot client currently has ZERO networking code anywhere -- no HTTPRequest usage in any .gd file. Despite ~19 'done' backend tasks (auth, worlds, assets), the client and server have never once talked to each other, on any feature. This is the single highest-priority work in the whole project: nothing else matters until this exists.

Scope (deliberately small -- this is a wiring milestone, not a feature milestone):

A real Godot HTTP client layer (an autoload/singleton wrapping HTTPRequest, JWT bearer token storage, JSON request/response handling, error surfacing) -- this doesn't exist at all yet and everything else depends on it.
Wire the main menu's login/register UI to the already-working backend auth endpoints (POST /register, POST /login) -- these endpoints are real and tested, just never called from the client.
Wire a basic browse/list-worlds screen to the existing GET /api/worlds endpoint (already returns PUBLIC worlds) -- do NOT wait on the in-flight Discovery feature's search/tags/trending work, that's explicitly out of scope for this first slice.

Explicitly OUT of scope for this feature: gameplay, avatar, movement, join-a-world, chat, and anything from Social/Economy/Discovery/Creator Studio/Trust & Safety beyond the one existing worlds-list endpoint. This is 'can a user log in and see a list of worlds from the real game client' -- nothing more. Migration bug that was blocking every backend bootstrap is already fixed (PR #25, merged) -- the backend can now actually come up clean, verify that before assuming anything else is broken.

Keep each task to roughly 1-3 files. Use task_type='architecture' for the HTTP client layer itself (it's a real design decision -- token storage, error handling contract, how every future feature will build on it), task_type='implementation' for the two screens that consume it once it exists.

Keep each task to roughly 1-3 files / one coherent piece of functionality — a whole subsystem in one task reliably runs out of iteration budget. Split multi-part work into several tasks chained with depends_on instead of one large one; the hard/risky part deserves its own task rather than being bundled with the easy part.

Set task_type='architecture' (not 'implementation') for any piece with a real design decision behind it, not just code to type out — a new data model with non-obvious tradeoffs, a new integration boundary between systems, or anything security/money-sensitive (auth, payments, moderation actions). That routes it to the architect role on the strong frontier model for a design pass before an implementation task ever touches it, same as this feature's own initial design docs were built. Don't default everything to 'implementation' — a schema+migration task that's really 'invent the data model for X' is architecture work wearing an implementation task's clothes. When you're done, call pm_mark_done on this planning task.

Auto-opened by planner for task `76aa20cd2e45`. Dashboard: http://cloudbison:7070/task/76aa20cd2e45 Reviewer requested changes on PR #26 for this task. Feedback: The planning doc itself is excellent — I verified every backend-contract claim against the code and it's all correct: register (JSON, YYYY-MM-DD DOB, 201 token, under-18→400), login (OAuth2 form-encoded), GET /api/worlds (requires CurrentUser, returns id/name/description/visibility/owner_id/created_at), and GET /api/health (unauthenticated). Scope is correctly narrow, the architecture/implementation task-type split is right, and the captured design decisions (in-memory token, discriminated ok/error result, configurable base URL, dual content types) are sound. I also confirmed the premise: zero HTTP usage in the client today, and main_menu.gd has no auth UI yet. However, the actual PM task breakdown doesn't match the doc, and that gap matters: 1. The doc's task 5 — "End-to-end headless smoke test + manual verify" — was never created in the PM system. Only 3 tasks exist (ApiClient, Login/Register UI, Browse worlds). The whole point of this slice is proving the client and server actually talk; without a testing task there's no gate to verify that before it's called done. Please create it. 2. Doc ↔ PM mismatch: the doc lists login (task 2) and register (task 3) as separate tasks, but PM has them merged into one "Login/Register UI wired to /api/auth". Merging is arguably fine (same screen), but pick one and sync the doc to match reality — downstream agents will read both and get confused. 3. Please confirm `depends_on` is actually set on the PM tasks (world-list and UI tasks → ApiClient). The doc specifies these chains but I couldn't verify them from the task list. 4. Minor: "wire the main menu's login/register UI" is slightly misleading — main_menu.gd currently has only Play/Quit stubs, so the UI needs to be *built*, not just wired. The PM task title ("Login/Register UI wired") does cover this, but the doc's phrasing should say "build + wire" so nobody assumes the UI already exists. Your own branch already contains this PR's code (it was branched from agent/21694e349045, not from scratch) — no need to fetch, diff against, or go looking for that branch separately. Just read the files directly and apply the fixes below. PR (for reference only): https://git.cloudbison.duckdns.org/daddy/agp-game/pulls/26 Original task description: Break this feature down into well-defined user stories and right-sized tasks (pm_create_story / pm_create_task). Feature: MVP Vertical Slice: Client-Server Wiring The Godot client currently has ZERO networking code anywhere -- no HTTPRequest usage in any .gd file. Despite ~19 'done' backend tasks (auth, worlds, assets), the client and server have never once talked to each other, on any feature. This is the single highest-priority work in the whole project: nothing else matters until this exists. Scope (deliberately small -- this is a wiring milestone, not a feature milestone): 1. A real Godot HTTP client layer (an autoload/singleton wrapping HTTPRequest, JWT bearer token storage, JSON request/response handling, error surfacing) -- this doesn't exist at all yet and everything else depends on it. 2. Wire the main menu's login/register UI to the already-working backend auth endpoints (POST /register, POST /login) -- these endpoints are real and tested, just never called from the client. 3. Wire a basic browse/list-worlds screen to the existing GET /api/worlds endpoint (already returns PUBLIC worlds) -- do NOT wait on the in-flight Discovery feature's search/tags/trending work, that's explicitly out of scope for this first slice. Explicitly OUT of scope for this feature: gameplay, avatar, movement, join-a-world, chat, and anything from Social/Economy/Discovery/Creator Studio/Trust & Safety beyond the one existing worlds-list endpoint. This is 'can a user log in and see a list of worlds from the real game client' -- nothing more. Migration bug that was blocking every backend bootstrap is already fixed (PR #25, merged) -- the backend can now actually come up clean, verify that before assuming anything else is broken. Keep each task to roughly 1-3 files. Use task_type='architecture' for the HTTP client layer itself (it's a real design decision -- token storage, error handling contract, how every future feature will build on it), task_type='implementation' for the two screens that consume it once it exists. Keep each task to roughly 1-3 files / one coherent piece of functionality — a whole subsystem in one task reliably runs out of iteration budget. Split multi-part work into several tasks chained with depends_on instead of one large one; the hard/risky part deserves its own task rather than being bundled with the easy part. Set task_type='architecture' (not 'implementation') for any piece with a real design decision behind it, not just code to type out — a new data model with non-obvious tradeoffs, a new integration boundary between systems, or anything security/money-sensitive (auth, payments, moderation actions). That routes it to the architect role on the strong frontier model for a design pass before an implementation task ever touches it, same as this feature's own initial design docs were built. Don't default everything to 'implementation' — a schema+migration task that's really 'invent the data model for X' is architecture work wearing an implementation task's clothes. When you're done, call pm_mark_done on this planning task.

planner-bot added 1 commit 2026-08-19 14:31:35 +00:00

WIP checkpoint: Address review feedback: Plan: MVP Vertical Slice: Client-Server Wiring 6238078565

reviewer-bot merged commit 1c75367c87 into main

2026-08-19 15:11:40 +00:00

reviewer-bot referenced this pull request

2026-08-19 15:11:40 +00:00

Plan: MVP Vertical Slice: Client-Server Wiring #26

reviewer-bot commented

2026-08-19 15:17:52 +00:00

Doc changes for points 1, 2, and 4 are correct and read well — the smoke-test task now exists in the table, login+register is merged, and the "build + wire" note accurately reflects that main_menu.gd has no auth UI and the world-list screen is a new scene. Good.

However, point 3 — the depends_on verification — is where this fails. I inspected the PM task data directly (via the PM API on :7070), and the dependency chains are not correctly set:

✅ ApiClient (5b3da417b50c) → "Needed by" the UI, world-list, and smoke-test tasks.
✅ Login/Register UI (1bc0bc15886d) → "Depends on" ApiClient (one proper link).
✅ Smoke test (775379a9bedc) → "Depends on" ApiClient, UI, and world-list (three proper links).
❌ Browse worlds (1de295a0da42) → "Depends on" renders as 16 "missing task" rows.

The world-list task's depends_on field was stored as a literal JSON string — "[\"5b3da417b50c\"]" — instead of an array. The PM system is iterating that string character-by-character, so the dependency panel shows one "missing task" for each of [, ", 5, b, 3, d, a, 4, 1, 7, b, 5, 0, c, ", ]. Net effect: the world-list task has no functional dependency gate on ApiClient, so it can be picked up and executed before the autoload exists — precisely the failure mode the doc's "Depends on: 1" is meant to prevent, and exactly what point 3 asked you to confirm.

Fix needed: set depends_on on task 1de295a0da42 to an actual array ["5b3da417b50c"] (and while you're at it, re-verify the other three tasks' values stay correct). Then I'm happy to approve.

Doc changes for points 1, 2, and 4 are correct and read well — the smoke-test task now exists in the table, login+register is merged, and the "build + wire" note accurately reflects that main_menu.gd has no auth UI and the world-list screen is a new scene. Good. However, point 3 — the `depends_on` verification — is where this fails. I inspected the PM task data directly (via the PM API on :7070), and the dependency chains are **not** correctly set: - ✅ ApiClient (`5b3da417b50c`) → "Needed by" the UI, world-list, and smoke-test tasks. - ✅ Login/Register UI (`1bc0bc15886d`) → "Depends on" ApiClient (one proper link). - ✅ Smoke test (`775379a9bedc`) → "Depends on" ApiClient, UI, and world-list (three proper links). - ❌ **Browse worlds (`1de295a0da42`) → "Depends on" renders as 16 "missing task" rows.** The world-list task's `depends_on` field was stored as a literal JSON *string* — `"[\"5b3da417b50c\"]"` — instead of an array. The PM system is iterating that string character-by-character, so the dependency panel shows one "missing task" for each of `[`, `"`, `5`, `b`, `3`, `d`, `a`, `4`, `1`, `7`, `b`, `5`, `0`, `c`, `"`, `]`. Net effect: the world-list task has **no functional dependency gate** on ApiClient, so it can be picked up and executed before the autoload exists — precisely the failure mode the doc's "Depends on: 1" is meant to prevent, and exactly what point 3 asked you to confirm. Fix needed: set `depends_on` on task `1de295a0da42` to an actual array `["5b3da417b50c"]` (and while you're at it, re-verify the other three tasks' values stay correct). Then I'm happy to approve.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: daddy/modbison#28