githubnext/chopinPublic

Let's compose rich plans together

Keep /research cards in the document after submit #338

MergedMaggieAppletonmerged 2 commits intomainfromrefs/pull/338/head
  1. MA

    ## Why

    Typing `/research` in a document and submitting a brief started the request (201), but the card disappeared about 80 ms later and the document jumped to the top. The request kept running with nothing on screen.

    Root cause: `ResearchWorkspaceService` mints **UUID** request ids, but the room's research-projection authorizer in `apps/server/src/plan/service.ts` returned `false` for any id that was not a **ULID**, before it checked storage. Every browser-placed `<Research>` reference was rejected as `protected-projection`, the room rebuilt (`plan:reset`), and the reference was lost. This was reproduced on the 8795 reference server, which runs current main.

    Tests missed it because the unit fixture (`pendingInlineResearch`) and the e2e seed (`seedPendingInlineResearchRequest`) both minted ULIDs, which production never does.

    ## What changed

    - Removed the ULID shape gate from the authorizer. Authority is unchanged: the stored workspace, its initial turn, and its linked job, checked by `researchProjectionAllowed`. The browser still cannot place a reference for a request that does not exist, belongs to another channel, or has no active job. - The unit fixture and the e2e seed now mint UUID ids, the same shape production uses. - Regression tests: - Unit: `accepts a browser-placed Research reference for a request with active work` in `research-projection-authority.test.ts`. It failed before the fix. - E2E: `a submitted /research card stays in the document through reload`. It checks the card is durably placed, no `plan:reset` is sent, and the card is still there after reload.

    ## Screenshots

    Before: after submit, no card, and the document has reset to the top. ![Before](https://github.com/user-attachments/assets/15596eda-dfe5-4169-b918-56bc5a7d9733)

    After: the card stays in place. This environment has no web search, so the request fails, but the failure now shows on the card with Retry. ![After submit](https://github.com/user-attachments/assets/c69e1445-dded-429d-84b2-c8d76b70a8e9)

    After reload, the card is still present. ![After reload](https://github.com/user-attachments/assets/4d2a7e3b-3005-4719-ba47-f81e4d842a07)

    ## Testing

    - `bun test apps/server/src/plan/research-projection-*`: 9 pass. Before the fix, the new test and the lifecycle tests failed with `protected-projection`. - `bun test src/plan/research src/research` (apps/server): 89 pass, 1 fail. The failure is `placement-close.test.ts`, "defers placement while document close drains Chat", which times out at 10 s and fails identically on unmodified main. - `bun run ci` and `bun run types`: pass. - Manual: real browser on a fake-GitHub server, /research submit. No `plan:reset`, the card is visible, and it persists through reload. - E2E: CI's browser-integration job passes, including the new e2e. Local e2e is suspended for this audit run.

    🤖 Generated with [Claude Code](https://claude.com/claude-code)

  2. MaggieAppletonhead-ref-force-pushed c0cbf98d95c3last month on GitHub

  3. MA
    Author

    **Review:** Looks good

    Removing the gate is safe. The ULID check wasn't what provided authority.

    **Authority (verified)** - `researchProjectionAllowed` (`apps/server/src/storage/model.ts:556`) still requires: a stored workspace whose `channelId` is this document, an `initial` turn for that workspace, `origin === "inline"` for an add, and a linked job of the right type/targetKey in this channel in `pending|paused|running|completed` with no published child. `research.get(plan.id, id)` is channel-scoped too. - I tried these against the branch with crafted Yjs updates inserting `<Research>` (scratch test, not committed): - Foreign-channel request **with an active linked job**: rejected (`protected-projection`, `plan:reset`, source unchanged) - Nonexistent UUID: rejected. Nonexistent ULID: rejected. - Malformed ids (`../etc/passwd`, 200 chars, `' OR 1=1 --`, `x y`, `-lead`): rejected with `bad-reference` - Without the fix, the new unit test fails with `protected-projection`. With the fix it passes. `bun test src/plan/research-projection src/plan/research-placement`: 13 pass.

    **Defence in depth (UUID|ULID gate?)** Not needed. The dialect already bounds the id before the authorizer runs. `Research.id` is `type: "reference", max: 96` and must match `^[A-Za-z0-9][A-Za-z0-9._:-]*$` (`packages/dialect/src/validate.ts:337`), and `parse()` runs in `room.apply` before `authorizeResearch`. The storage column is `text`, so odd shapes can't make the query throw. The old gate didn't limit lookups per batch anyway, because an attacker can mint valid ULIDs just as easily. If you want an id-shape assertion, put it where the id is minted (`ResearchWorkspaceService`), not in the room.

    **Why the gate existed** It came in with the 175-file `b2d41b79` integration commit and no stated reason. The fixtures minted ULIDs, so it looks like an assumption about the id shape rather than a security boundary. The dialect's own choice of `reference` over `id` for `Research.id` supports that.

    **Non-blocking** 1. `research-projection-authority.test.ts` "rejects a Research projection backed by a request from another channel" uses an *unlinked* request. The no-job rule rejects it before the channel check matters. Linking an active job in the foreign channel would make the test actually prove channel scoping. I checked, and that case is rejected today. 2. The authorizer doesn't check `createdBy`. Any writer on the channel can place a reference to any active inline request in the same channel. That's unchanged and fine, since writers share the document and the record stays authoritative. Noting it only because the question came up. 3. The PR body still has broken local image links (`.//Users/...`) above the uploaded ones.

    CI: container, e2e, and format/lint/types/tests all pass.

  4. MaggieAppletonhead-ref-force-pushed 9d5c00a05e01last month on GitHub

  5. MaggieAppletonmerged fcfefb738aa7last month on GitHub

  6. MaggieAppletonclosedlast month on GitHub

  7. MaggieAppletonhead-ref-deletedlast month on GitHub

  8. MaggieAppletonreferenced this in Keep the research composer visible as one surface #355last month on GitHub

  9. MaggieAppletonreferenced 8e2c02cbad11last month on GitHub