[Spec 250] t3code is the front end: nested Workspace > Architect > Builders, porch gates, and approval from t3code

apps/client/__tests__/derive.test.ts

@@ -211,7 +211,27 @@ describe('deriveRowStatus', () => {
       ),
     ) as { $defs: Record<string, { properties?: { status?: { enum?: string[] } } }> };
 
-    const session = schema.$defs.subscribeThreadOutput__Objects_6;
+    /*
+     * `_7`, not `_6`, and the number is generated rather than chosen.
+     *
+     * Spec 250 phase 5 regenerated the vendored contract FROM THE FORK, and the
+     * fork's `codevGate` object lands ahead of the session object in the
+     * generator's numbering — so the enum this test reads moved one along. The
+     * mapping below is unaffected; only the path was stale.
+     *
+     * The assertion message under this is what made that diagnosable, and it is
+     * why the message says what it says: "this test needing a new path, not a
+     * mapping change" is the difference between a one-character fix and an hour
+     * spent looking at `deriveRowStatus`.
+     *
+     * A positional key like this WILL move again whenever the contract is
+     * regenerated after a change ahead of it. That is the cost of reading a
+     * generated artifact positionally, and it is accepted here rather than
+     * hidden: the alternative — searching every `$def` for one carrying a status
+     * enum — would silently find a DIFFERENT object if the session one ever lost
+     * its enum, which is the failure this test exists to catch.
+     */
+    const session = schema.$defs.subscribeThreadOutput__Objects_7;
     const declared = session?.properties?.status?.enum;
     expect(
       declared,

codev-skeleton/resources/lessons-critical.md

@@ -10,6 +10,8 @@ STARTER: a few universal lessons are seeded; add your project's as you learn the
 - Check for existing work (PRs, git history) before building from scratch.
 - "It compiled" / "tests pass" is not "it works" — verify the real user path before calling it done.
 - When stuck (2 failed hypotheses or ~30 min), get an outside perspective instead of guessing.
+- A test that cannot fail is not a test — revert the fix and confirm the test fails before trusting it.
+- A test that supplies the boundary itself cannot tell you the boundary exists — test the seam, not the two ends.
 - <Add your project's hard-won, cross-cutting lessons; keep <=10, one line each.>
 
 ## Map of lessons-learned.md (consult when…)

codev/projects/250-t3code-is-the-front-end-privat/250-phase_1-iter1-rebuttals.md

@@ -0,0 +1,110 @@
+# Spec 250, phase_1, iteration 1 — review responses
+
+Two lanes: **claude** (APPROVE) and **opencode / grok-4.6** (REQUEST_CHANGES).
+
+The opencode lane timed out at 360s on its first attempt and produced no verdict. `consult` has
+no timeout flag — `OPENCODE_TIMEOUT_MS` is a hard-coded 6 minutes in
+`packages/codev/src/commands/consult/index.ts:1561` — so the run was retried and completed. The
+first failure was loud and exited 1, which is the lane behaving correctly; it is recorded here
+rather than left as an unexplained gap.
+
+---
+
+## opencode #1 — `ready()` still runs full `verify()` — ACCEPTED
+
+> `start()` only calls `verifyUpstream()` so a spec 146 run does not need the fork. `ready()`
+> then calls `verify('CHECKOUT_MOVED_DURING_RUN')`, which requires fork HEAD == `pin.commit`.
+
+Correct, and it made `start()`'s upstream-only exemption buy nothing: `smoke.mjs` calls
+`acquire`, `verify`, `start`, `ready` in sequence, so the fork requirement came back one call
+later. `packages/t3-client/live/integration.mjs:202` had the same shape.
+
+The consequence is not hypothetical. Phases 2 through 4 commit to the fork while `pin.commit`
+stays at `upstreamBase` until phase 5. On the first fork commit, a correct upstream server would
+have failed `ready` with `CHECKOUT_MOVED_DURING_RUN` — a signal about the checkout the *server*
+runs from, reported for a checkout the server never touches.
+
+**Fixed.** `ready()` now calls `verifyUpstream('CHECKOUT_MOVED_DURING_RUN')`. Two new subcommands,
+`verify-upstream` and `verify-fork`, assert one identity each; `smoke.mjs` and
+`live/integration.mjs` use `verify-upstream`. Bare `verify` still asserts both, which is what the
+phase's acceptance criterion requires.
+
+Four tests: `verify-upstream` passes with no fork checkout at all, `verify-fork` passes with no
+upstream root, bare `verify` still stops at `3` on a missing fork, and the three callers are
+asserted to use the upstream-only verb.
+
+The cold-start evidence was re-collected after the `smoke.mjs` change.
+
+## opencode #2 — `verifyCheckout` treats a failed `git status` as clean — ACCEPTED
+
+> The catch comment says undetermined; the code returns success. Same "could not tell" as pass.
+
+Correct. The catch fell through to `dirty = ''`, and an empty string is how "clean" is spelled.
+The comment claiming otherwise was inherited from the spec 146 version, which had the same bug —
+the reviewer found it in the new file, and it was there before.
+
+**Fixed.** The catch now `die(UNDETERMINED, 'NO_<IDENTITY>_STATUS: could not check: ...')`.
+
+The test triggers it for real rather than asserting on source: `chmod 000` on `.git/index` leaves
+`rev-parse HEAD` working (it reads only the ref) and makes `git status` exit 128, which lands the
+failure exactly between the two checks. The test refuses to pass vacuously — if the platform
+ignores the mode, it fails with a message saying so rather than skipping quietly.
+
+## claude #1 — `FORK.md` overstates "nothing re-derives it" — ACCEPTED
+
+`packages/t3-client/live/integration.mjs` deliberately reads `process.env.T3CODE_ROOT` directly
+and keeps it **required** (#214). The sentence was stronger than the code.
+
+**Fixed.** `FORK.md` names the exception and why it is one.
+
+## claude #2 — test heading says "the seventh readers" over six — ACCEPTED
+
+**Fixed.** Renamed to "the root readers".
+
+## claude — items it could not verify from its session
+
+The lane had no shell. All three are now checked:
+
+| Claim | Result |
+|---|---|
+| `pnpm -w test` | 7263 passed, 54 skipped, 0 failed, with both live suites executing |
+| `gh repo view pseudoseed/t3code` | `visibility: PRIVATE`, `isFork: false`, default branch `codev` |
+| MIT `LICENSE` unmodified in the fork | `diff` against the upstream clone reports identical |
+
+## Not changed
+
+**When `pin.commit` moves to the fork head.** Both lanes brushed against it; neither asked for a
+change. The plan puts it at phase 5, so phases 2 through 4 will run with a fork checkout ahead of
+`pin.commit` and bare `verify` will report `FORK_CHECKOUT_MISMATCH` in that window. That is the
+plan's sequencing, not a phase 1 defect, and the per-identity verbs above mean it no longer blocks
+an upstream server. Flagged to the architect rather than resolved here.
+
+---
+
+# Iteration 2 review responses
+
+Both lanes APPROVE. claude raised two non-blocking notes; opencode raised none.
+
+## claude — `--since` bypassed the ref-resolution guard — ACCEPTED
+
+The guard ran over `range.from` before `--since` replaced it, so an unresolvable `--since` ref
+slipped past and surfaced as a raw git error: exit 1 doing exit 3's job.
+
+**Fixed.** The guard now runs after `from` is computed, over the refs actually used. Tested with a
+throwaway checkout and a `--since` naming a sha that does not exist.
+
+## claude — no direct test for the `NO_UPSTREAM_MOVEMENT` named zero — ACCEPTED
+
+Its `NO_FORK_DRIFT` twin was tested; the upstream one was not, because upstream has genuinely
+moved on this machine (3 closure commits between `upstreamBase` and `origin/main`), so the real
+pair cannot produce the zero.
+
+**Fixed.** The test builds a throwaway checkout whose `refs/remotes/origin/main` sits exactly where
+the range starts. A real empty range, not a mocked one.
+
+## claude — items it could not verify without a shell
+
+`gh repo view pseudoseed/t3code --json visibility,isFork,nameWithOwner,defaultBranchRef` returns
+`{"defaultBranchRef":{"name":"codev"},"isFork":false,"nameWithOwner":"pseudoseed/t3code","visibility":"PRIVATE"}`.
+`pnpm -w test`: 7263 passed, 54 skipped, 0 failed, plus 180 in the v2 suite. Both re-run after
+these two fixes.

codev/projects/250-t3code-is-the-front-end-privat/250-phase_1-iter2-context.md

@@ -0,0 +1,94 @@
+### Iteration 1 Reviews
+- claude: APPROVE — Phase 1 two-identity harness is complete and correct; the destructive acquire path, merge-base check, and three-code exit semantics are all implemented and tested.
+- opencode: REQUEST_CHANGES — ready/smoke/integration still run both-identity verify, so an upstream server start dies once the fork HEAD leaves pin.commit.
+
+### Builder Response to Iteration 1
+# Spec 250, phase_1, iteration 1 — review responses
+
+Two lanes: **claude** (APPROVE) and **opencode / grok-4.6** (REQUEST_CHANGES).
+
+The opencode lane timed out at 360s on its first attempt and produced no verdict. `consult` has
+no timeout flag — `OPENCODE_TIMEOUT_MS` is a hard-coded 6 minutes in
+`packages/codev/src/commands/consult/index.ts:1561` — so the run was retried and completed. The
+first failure was loud and exited 1, which is the lane behaving correctly; it is recorded here
+rather than left as an unexplained gap.
+
+---
+
+## opencode #1 — `ready()` still runs full `verify()` — ACCEPTED
+
+> `start()` only calls `verifyUpstream()` so a spec 146 run does not need the fork. `ready()`
+> then calls `verify('CHECKOUT_MOVED_DURING_RUN')`, which requires fork HEAD == `pin.commit`.
+
+Correct, and it made `start()`'s upstream-only exemption buy nothing: `smoke.mjs` calls
+`acquire`, `verify`, `start`, `ready` in sequence, so the fork requirement came back one call
+later. `packages/t3-client/live/integration.mjs:202` had the same shape.
+
+The consequence is not hypothetical. Phases 2 through 4 commit to the fork while `pin.commit`
+stays at `upstreamBase` until phase 5. On the first fork commit, a correct upstream server would
+have failed `ready` with `CHECKOUT_MOVED_DURING_RUN` — a signal about the checkout the *server*
+runs from, reported for a checkout the server never touches.
+
+**Fixed.** `ready()` now calls `verifyUpstream('CHECKOUT_MOVED_DURING_RUN')`. Two new subcommands,
+`verify-upstream` and `verify-fork`, assert one identity each; `smoke.mjs` and
+`live/integration.mjs` use `verify-upstream`. Bare `verify` still asserts both, which is what the
+phase's acceptance criterion requires.
+
+Four tests: `verify-upstream` passes with no fork checkout at all, `verify-fork` passes with no
+upstream root, bare `verify` still stops at `3` on a missing fork, and the three callers are
+asserted to use the upstream-only verb.
+
+The cold-start evidence was re-collected after the `smoke.mjs` change.
+
+## opencode #2 — `verifyCheckout` treats a failed `git status` as clean — ACCEPTED
+
+> The catch comment says undetermined; the code returns success. Same "could not tell" as pass.
+
+Correct. The catch fell through to `dirty = ''`, and an empty string is how "clean" is spelled.
+The comment claiming otherwise was inherited from the spec 146 version, which had the same bug —
+the reviewer found it in the new file, and it was there before.
+
+**Fixed.** The catch now `die(UNDETERMINED, 'NO_<IDENTITY>_STATUS: could not check: ...')`.
+
+The test triggers it for real rather than asserting on source: `chmod 000` on `.git/index` leaves
+`rev-parse HEAD` working (it reads only the ref) and makes `git status` exit 128, which lands the
+failure exactly between the two checks. The test refuses to pass vacuously — if the platform
+ignores the mode, it fails with a message saying so rather than skipping quietly.
+
+## claude #1 — `FORK.md` overstates "nothing re-derives it" — ACCEPTED
+
+`packages/t3-client/live/integration.mjs` deliberately reads `process.env.T3CODE_ROOT` directly
+and keeps it **required** (#214). The sentence was stronger than the code.
+
+**Fixed.** `FORK.md` names the exception and why it is one.
+
+## claude #2 — test heading says "the seventh readers" over six — ACCEPTED
+
+**Fixed.** Renamed to "the root readers".
+
+## claude — items it could not verify from its session
+
+The lane had no shell. All three are now checked:
+
+| Claim | Result |
+|---|---|
+| `pnpm -w test` | 7263 passed, 54 skipped, 0 failed, with both live suites executing |
+| `gh repo view pseudoseed/t3code` | `visibility: PRIVATE`, `isFork: false`, default branch `codev` |
+| MIT `LICENSE` unmodified in the fork | `diff` against the upstream clone reports identical |
+
+## Not changed
+
+**When `pin.commit` moves to the fork head.** Both lanes brushed against it; neither asked for a
+change. The plan puts it at phase 5, so phases 2 through 4 will run with a fork checkout ahead of
+`pin.commit` and bare `verify` will report `FORK_CHECKOUT_MISMATCH` in that window. That is the
+plan's sequencing, not a phase 1 defect, and the per-identity verbs above mean it no longer blocks
+an upstream server. Flagged to the architect rather than resolved here.
+
+
+### IMPORTANT: Stateful Review Context
+This is NOT the first review iteration. Previous reviewers raised concerns and the builder has responded.
+Before re-raising a previous concern:
+1. Check if the builder has already addressed it in code
+2. If the builder disputes a concern with evidence, verify the claim against actual project files before insisting
+3. Do not re-raise concerns that have been explained as false positives with valid justification
+4. Check package.json and config files for version numbers before flagging missing configuration

codev/projects/250-t3code-is-the-front-end-privat/250-phase_10-iter1-rebuttals.md

@@ -0,0 +1,116 @@
+# Phase 10 — 3-way review, iteration 1
+
+Two lanes. **Claude APPROVE / HIGH** (after a second pass that closed its own stated
+coverage gap). **opencode REQUEST_CHANGES / HIGH.** The stricter lane is binding, and it
+found the one defect that mattered.
+
+Every finding was accepted. Nothing is in a disagree column.
+
+---
+
+## 1. The vitest e2e reported a PASS on a run that never happened — opencode, blocking
+
+> `spec-250-t3code-approval.e2e.test.ts` returns from `it()` when the fork is unavailable, so
+> criterion 4 / SSRF at the wired handler go green without running. The file's own header says
+> "skips, never passes".
+
+**Accepted, and it is the worst defect in the phase.** The guard was:
+
+```ts
+function skipIfUnavailable(): boolean {
+  if (unavailable === null) return false;
+  console.warn(`SKIP spec-250 t3code approval: ${unavailable}`);
+  return true;      // <- vitest records this as a PASS
+}
+```
+
+So a run where the fork server never started reported **8 passed** with not one assertion
+executed — on the phase's own acceptance criterion. That is this project's recurring defect
+inverted: not "I could not tell" spelled as "no", but spelled as **"yes"**, which is strictly
+worse. The file's header had the rule written in it and the code broke it; a header is not a
+mechanism.
+
+Worse, it was invisible in every run I did, because the fork was always up. It would have
+surfaced the first time someone ran the suite without `T3_NODE` — and it would have surfaced
+as a green tick.
+
+**Fixed** with `ctx.skip(...)`, which marks the test skipped and does not return, so the body
+is unreachable rather than merely unexecuted. The Playwright spec beside it already did this
+with `test.skip`; the two now agree.
+
+**Demonstrated, not asserted.** Same file, same command, `T3_NODE` unset:
+
+```
+before:  Tests  8 passed (8)
+after:   Tests  8 skipped (8)
+```
+
+and with the fork available, `Tests 8 passed (8)`.
+
+## 2. `UPSTREAM_TIMEOUT_MS` claimed more than the mechanism gives — Claude, non-blocking
+
+> applied via `upstream.setTimeout`, a Node idle-socket timeout, while the comment describes it
+> as bounding "the whole exchange".
+
+**Accepted.** `ClientRequest.setTimeout` restarts its clock on socket activity, so it bounds
+SILENCE, not elapsed time. The comment said otherwise, and overstating a bound is the same class
+of error as the `connect-src 'self'` claim this phase existed to correct — it reads as protection
+that is not there.
+
+The comment now says what the mechanism gives and states the residual explicitly: a trickling
+upstream is not bounded by it. That upstream is one the operator named in
+`T3CODE_CODEV_AGENT_ORIGINS`, so it is not a stranger, and a total-duration bound would have to
+be large enough for the slowest legitimate answer — a worse trade for a threat the allowlist
+already narrows to the operator's own hosts. Recorded rather than quietly accepted.
+
+## 3. `data-codev-approval-state` was coarser than its own words — both lanes
+
+> a session-ended outcome tags as `refused` in the machine-readable attribute while the visible
+> text and testid distinguish it correctly.
+
+**Accepted**, and both lanes finding it independently is the signal. The attribute computed three
+values over four outcomes. Nothing asserts on it today, which is exactly why it was worth fixing
+now rather than later: **the first test written against it would have inherited the conflation
+the file's own header exists to prevent** — "the session idled out" spelled the same as "your
+approval was refused", one layer below where a human reads it.
+
+Four outcomes, four words, in an exported pure function (`approvalStateAttribute`) so the
+attribute and the rendering cannot drift. Three tests, including one that pins the precedence
+when an outcome carries both flags. Removing the `session-ended` branch fails two of them.
+
+## 4. Claude's own coverage gap, stated and then closed
+
+Claude's first pass said plainly which files it had not read — `agentState.ts`,
+`useCodevAgent.ts`, `GateApproval.tsx`, `PairingPanel.tsx`, patches 0029-0031, the harness — and
+rested its verdict on what it had read in full. Its second pass read them and raised confidence
+from MEDIUM to HIGH.
+
+Worth recording because the honest declaration is what made the second pass targeted. A lane that
+had said nothing would have produced the same verdict with no way to tell what it covered.
+
+## 5. A finding of my own, confirmed by the review
+
+Between the two lanes I found that the proxy buffered request bodies with **no bound** —
+Effect's `MaxBodySize` defaults to unbounded, and this route reads the whole body before
+forwarding. One authenticated caller could pin arbitrary memory on the route whose whole purpose
+is to be reachable from a phone.
+
+Capped at 64 KiB; a declared oversize `content-length` is refused **before** the read, because
+refusing after reading would already have done the thing the cap exists to prevent; a chunked
+body declares no length, so the cap on the read catches that one. Too-large and could-not-read
+get separate signals.
+
+Claude's second pass called it "a real availability fix". Verified by running the same test
+against the fork commit before it (`e0476d49aec1`): it fails there and passes at `24aeeebb3ded`,
+with no fork history touched.
+
+## What both lanes verified as holding
+
+Server-held origin allowlist with selection by id; `CODEV_AGENT_PATH_ABSOLUTE` for a URL in the
+path; redirects refused rather than followed; unreachable and silent as distinct signals;
+`Connection`'s own tokens subtracted from the header allowlist; `authorization` and `cookie`
+never forwarded; the machine credential and the `client-session` token both required and refused
+differently; four approval outcomes with the record server-sourced and an empty 200 rendering as
+`unconfirmed` rather than a manufactured yes; pane content from one workspace-state poll rather
+than six transcripts; and the Playwright spec recording every request and asserting same-origin,
+with a positive assertion that the proxy was reached at all so the negative cannot pass vacuously.