docs(retro): #228 design-packet retro, and the eight consistency gaps found closing it - #248
docs(retro): #228 design-packet retro, and the eight consistency gaps found closing it#248flyingrobots wants to merge 9 commits into
Conversation
Round four, self-audited rather than review-driven. Every one is the same class Codex caught twice on #245: sections I edited in rounds 1-3 left the sections I did not edit contradicting or under-scoping them. Merging #245 without a fourth review round is what let them survive. CONTRADICTIONS WITH THE PROJECTION DECISION: - Human playback Q5 still asked for "the same structured file_outline result". - Test strategy step 4 still said "compares structured results". Both now name replayProjection, matching the Hill and the acceptance list. OMISSIONS OF PROCESS AUTHORITY: - Agent playback Q4 asked whether replay can reach LiveWorkspaceReadSource, Git, git-warp or the test-only constructor, but not an external process -- the exact hole invariant 10 was extended to close, since the prose projector shells out. TEST STRATEGY AUTHORIZED NONE OF THE NEW INVARIANTS: Acceptance requires invariants 13, 14 and 15; the strategy had no step for any of them. Added steps 9-11: a denied-path test asserting no observation, no retention and no fabricated `actual`; a multi-settlement test proving the replay key selects deterministically; and an entry-kind test proving the decoder refuses a symlink-reached path on schema-bound evidence. Also amended step 1, which specified only a code fixture -- a code-only fixture satisfies the zero-process counter without exercising the projector branch that counter exists to constrain. IMPLEMENTATION BOUNDARY UNDER-SCOPED THE WORK: - Item 6 named only causal-order, filesystem-read and git-warp-open evidence, omitting the four counters added since. An acceptance criterion whose instrumentation sits outside the implementation boundary cannot be met by the cycle that owns it. - Added item 7 for the contract changes acceptance now depends on: entryKind, the two request identities, the path-only refusal variant, and the recovery-state result. AUTHORITY MODEL DESCRIBED ONLY THE BLIND SPOT: The ordering proof was described as instrumenting the observation authority, which is precisely what Codex finding 3791341263 showed to be insufficient: repo-paths.ts calls realpathSync.native and lstatSync before RepoWorkspace exists, so an assertion scoped to that authority reads green against a workspace already touched. Stated inline where the proof is defined, not only in the acceptance list. Design packet only; no runtime code changed. Refs #228.
METHOD.md requires a cycle's local retro to exist, be committed, and be validated before its PR. For PR #245 that did not happen -- the PR went first. This writes the retro that was owed, records the violation as drift rather than smoothing it over, and files the debt the cycle exposed. Artifacts: - docs/method/retro/CORE_first-retained-workspace-observation/ CORE_first-retained-workspace-observation.md witness/verification.md The witness records what a documentation cycle can actually prove: every one of the packet's eight src/** citations resolves and its claim about that file is true; the packet does not contradict itself after the round-four repair; and no runtime or test file changed (git diff --stat over src bin test tests is empty). outcome: not-met, which is accurate. A design packet cannot meet an implementation hill, and the packet's own status line still reads "Implementation has not started". `design-recorded` was drafted and rejected -- inventing a third enum value across a 79-retro corpus is precisely the drift this cycle is about. Debt filed: - CLEAN_design-packets-receive-no-automated-review.md -- .coderabbit.yaml excludes **/*.md, so CodeRabbit read zero lines of the artifact that defines what the next cycle builds. #245 was reviewed by Codex alone, which found 16 defects, 10 of them P1. CI cannot catch these; it passes on docs by construction. - CORE_design-packet-consistency-lint.md -- across four rounds the most common defect was one section of the packet contradicting another after an edit landed in one place and not the others. Every instance was mechanical: invariants with no test step, counters named in one list and not the other, raw-equality language surviving next to a named projection, stale src paths. A structural lint would catch all of them, and none of its rules asserts what the packet should say. Refs #228.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (7)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Self-check against the PR's actual badges: 16 Codex findings on #245 are 11xP1, 4xP2, 1xP3. The witness round table was already right (3+4+4 P1); the bad-code card's summary line said ten. Added the explicit totals line to the witness so the two cannot drift apart again -- which is the same defect class this retro is about.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b09aca03a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CI FAILURE, AND THE CLAIM IT DISPROVED: test (22) failed on c93bea5. Adding this retro's two backlog cards invalidated docs/method/backlog/dependency-dag.dot, a checked-in artifact generated from backlog frontmatter and pinned by test/unit/method/backlog-dependency-dag.test.ts. I had applied the reduced docs gate -- git diff --check plus lint -- reasoning that no runtime file changed, and the witness asserted the branch touched nothing the suite loads. That was false. In this repository backlog frontmatter is a suite input, so "documentation-only" does not imply "cannot break the suite". Regenerated through scripts/generate-backlog-dependency-dag.ts (143 cards, 15 edges); no generated byte hand-resolved. The two unresolved refs it reports are pre-existing and belong to other cards. Both the retro Drift section and the witness now record this rather than carrying the disproved claim. Full suite reverified: 258 files, 2056 tests. CODEX FINDINGS ON #248: - Totality was specified over FileOutlineResult, but the operation actually returns RepoWorkspaceFileOutlineResult = FileOutlineResult | RepoWorkspaceRefusedResult (src/operations/repo-workspace.ts:68), and item 7 adds two more variants. Totality over the success type alone would leave every refusal shape unprojected -- the same under-enumeration defect one level further out. Now specified over the whole operation result union. - "Move the resolution inside the claimed authority OR count it with preRequestWorkspaceMetadataReads" was incoherent: the evidence block requires that counter to be 0, so instrumenting a surviving pre-request read reports the violation instead of satisfying the criterion. Counting is now stated as the detector, not a second way to pass, in both places the alternative appeared. Refs #228.
|
@codex review please Head is Finding 1 is the third instance of the same defect class on this packet, one level further out each time — the projection's field list was corrected at the operation layer, then at the MCP wrapper, and this time it was the totality scope that named only the success type. Worth noting because it is now recorded in the retro's "what would you do differently" as the reason the enumeration was moved into code with a totality test in the first place. Finding 2 was a straight logical contradiction of my own making: the evidence block requires Separately, CI caught something the review could not: this PR's two backlog cards invalidated the generated |
Applying this cycle's own lesson to itself. Having just shipped one false witness claim that CI disproved, I re-checked the rest instead of assuming: - packet length said 769; the Codex repairs grew it to 786 - the scope block listed one changed file; there are seven, including the two regenerated dependency-dag artifacts - "verified from branch commit c61023f" predated two later gate runs Invariant count (15), acceptance count (29), the empty src/bin/test/tests diff, and all six cited commit SHAs re-verified and unchanged. A witness whose numbers drift is worth less than no witness, because it reads as evidence. That failure mode is the same one the packet's replayProjection work is about, and the same one the new consistency-lint card proposes to catch mechanically.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e187963f84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…have found DUPLICATE BACKLOG CARD -- my process failure, not a wording problem: CLEAN_coderabbit-path-filters-skip-method-docs.md has tracked this exact concern since 2026-06-01 against issue #69, and already names docs/design/**/*.md in its acceptance criteria and already requires generated artifacts to stay excluded. I filed a second card without searching the backlog first, which would have given one defect two identities, two priorities, and two nodes in the generated DAG. Deleted my duplicate; added the PR #245 recurrence to the existing card as cost evidence, explicitly noting it does not change that card's remedy. The retro's New Debt section now reports "none new" and links the existing card, and the Drift section records the duplicate rather than leaving the correction in a review thread. DAG regenerated: 143 -> 142 cards. INVARIANT 1 WAS SCOPED TO THE LOOPHOLE: It defined request-before-effect as ordering reads "through the observation authority", so an implementation could satisfy the named invariant while making exactly the ambient pre-request read the surrounding paragraph identifies -- repo-paths.ts calls realpathSync.native and lstatSync outside that authority. Broadened to every workspace metadata or content read by any component, with resolver reads named. TOTALITY SCOPE, THIRD INSTANCE: I corrected the projection's totality to the whole operation union in the projection section and left implementation boundary item 5 still saying FileOutlineResult. Implementation starts from the boundary list, so the uncorrected copy is the one that would have been followed. Both now name RepoWorkspaceFileOutlineResult and item 7's two added variants. MY OWN LINT PROPOSAL CONTRADICTED ITS OWN NON-GOALS: Rule 3 banned equality words wherever a named comparison exists, which fires on this packet's own correct sentences -- the Hill saying byte identity is deliberately not the bar, and the acceptance criterion qualifying "identical" with the projection. The card's non-goals section says no rule may assert what a packet should say; rule 3 did. Rescoped to unqualified equality claims, with a structural model preferred and an explicit instruction to drop the rule rather than ship a word list. WITNESS ACCURACY, AGAIN: Scope block refreshed for the real seven paths, dangling reference to the deleted card removed. Packet line count dropped entirely: it drifted three times during this cycle and carries nothing the invariant and criteria counts do not carry better. A witness number that must be re-checked every commit is a liability, not evidence. Gate: 258 files, 2056 tests, lint clean, diff --check clean, all retro links resolve. Refs #228.
|
@codex review please Head is Two were mine in a way the earlier ones weren't, and both are now recorded in the retro rather than only here: I filed a backlog card without searching the backlog (the concern has been tracked since 2026-06-01 under issue #69), and my own proposed lint contradicted its own non-goals section by banning equality words that this packet uses correctly. The totality-scope finding is the third instance of that class on this packet — corrected in the projection section, left stale in the implementation boundary, which is the copy implementation would actually follow. Gate on this head: 258 files / 2056 tests, lint clean, diff --check clean, all retro links resolve, and the witness scope block verified by diffing it against the actual |
Self-audit, pre-empting the class that produced three of the last five findings: a fix applied in one place and not its twin. Codex's last round broadened invariant 1 to order workspace reads by any component, not only reads through the observation authority. Test-strategy step 2 still described an authority-scoped spy as the proof -- which is exactly the instrument that reports green on a workspace repo-paths.ts already touched. Step 2 now states that the spy alone cannot prove invariant 1, and that the proof also instruments the filesystem below the resolver and asserts preRequestWorkspaceMetadataReads == 0. Also swept the two other recurring fixes for twin copies and found none: no 'or counted' escape hatch survives, and every totality reference names the whole RepoWorkspaceFileOutlineResult union.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 302f8c42e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
I OVER-CORRECTED INVARIANT 1: Last round broadened it from "reads through the observation authority" to "reads by any component". That is unsatisfiable, not merely broad: routing loads .graftignore and constructs the path resolver before an operation exists at all (src/mcp/workspace-router-runtime.ts:91-92), so the first read by some component has always already happened. The correct line is causal, not positional: every read causally initiated for this observation is ordered, whatever component makes it; reads establishing the prerequisite routing and policy context are outside the claim. Both failure modes are now named in the invariant so the next editor cannot re-narrow or re-broaden it by accident. Twin copies moved in the same commit rather than the next round: the counter definition now excludes prerequisite routing reads, and a new acceptance criterion requires the exemption list to be enumerated and closed -- an unbounded "prerequisite" category is the authority-scoped loophole with a better name. TWO LINT RULES WITHDRAWN, NOT PATCHED: - The equality-vocabulary rule failed twice. The narrowed version still fires on the Hill's "Byte-identical is deliberately not the bar", because that sentence NEGATES raw equality and so never names the projection. Catching it needs claim polarity modelled structurally, at which point the word list does no work. - The numbered-list rule asserts Markdown formatting, which AGENTS.md prohibits outright. That a numbering hole really did occur does not license encoding document structure in CI. Both are recorded as withdrawn with the reasoning, because the instinct that produced them recurs: a real defect observed, and the nearest predicate is a text pattern. Three structural rules survive. COST CLAIM NARROWED: The CodeRabbit card said the b9caa72 projection "would have rejected every valid result". It kept path, outline, jumpTable and reason, so an ordinary success was accepted; it rejected variants carrying partial, error, or refusal-only fields. The narrower claim is the true one and the only one worth recording in a debt card. WITNESS PROVENANCE DESCRIBED BY RULE, NOT SHA: The line pinning the gate to e187963 and claiming later commits touched only witness and retro was false. A witness pinned to a SHA that later commits move past decays on the next push, so the gate is now described by rule -- re-run whenever a packet, card, or generated artifact changes -- with the SHA recorded in the commit that ran it. Gate: 258 files, 2056 tests, lint clean, diff --check clean, witness scope block verified against git output. Refs #228.
It is a cool-idea card and was written as a five-rule specification, which invited rule-by-rule review of something nobody has committed to building. It generated three review findings, including the only P1 on the PR, none of which concerned the #228 design packet. Reduced to the observation, the three structural checks this cycle actually evidenced (invariant coverage, counter closure, reference resolution), and a one-line note that the two rejected rules asserted document wording and formatting so they are not re-proposed. No new invariants, acceptance criteria, counters, or design requirements. The retro's Cool Ideas link is preserved, so the retro is unchanged.
|
@codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 211d7b0f81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 3. **Reference resolution** — "see below" resolves in the stated direction, and | ||
| every `src/...` path cited exists in the tree. The path check alone catches a | ||
| packet that has gone stale against the tree. |
There was a problem hiding this comment.
When this idea is implemented, checking whether the phrase “see below” resolves in a particular direction will make CI depend on incidental prose wording rather than a Graft software invariant; a correct packet can rephrase or remove that cross-reference without changing its contract. Keep the source-path existence check, but drop the wording-dependent half of this rule.
AGENTS.md reference: AGENTS.md:L21-L26
Useful? React with 👍 / 👎.
| `dc619514` is the `main` baseline. The full gate — lint, `diff --check`, and | ||
| the complete 258-file suite — is re-run on every commit of this branch that | ||
| touches the design packet, a backlog card, or a generated artifact, because | ||
| this cycle established that those are suite inputs. The last such run is | ||
| recorded in that commit's message. |
There was a problem hiding this comment.
Fresh evidence in the reviewed squashed commit makes this provenance rule false: 3c0dde6's message says the complete suite was last green on the dc619514 baseline and incorrectly claims the branch changes no suite input, while this witness later acknowledges that the added backlog cards are suite inputs. Consequently there is no final-head commit-message record supporting the claimed 258-file run; record validation for the squashed tree or stop claiming that the last relevant commit contains it.
AGENTS.md reference: AGENTS.md:L130-L130
Useful? React with 👍 / 👎.
| - [ ] **The prerequisite-read exemption is enumerated and bounded.** Invariant 1 | ||
| orders reads causally initiated for this observation, so the routing and | ||
| policy reads that establish the authority context — loading | ||
| `.graftignore`, constructing the path resolver | ||
| (`src/mcp/workspace-router-runtime.ts`) — sit outside the claim. The | ||
| cycle lists exactly which reads it exempts and asserts the list is | ||
| closed. An unenumerated "prerequisite" category would let any read be |
There was a problem hiding this comment.
Fresh evidence in the normal routed flow shows that this supposedly closed list is already incomplete: before createBoundWorkspace, resolveWorkspaceRequest runs fs.realpathSync while canonicalizing the worktree and Git-common roots (src/mcp/workspace-router-resolution.ts:47-64). That prerequisite metadata read is neither .graftignore loading nor resolver construction, so instrumentation must either count it and fail every routed call or silently exempt an unlisted read; enumerate and bound the route-resolution reads as well.
AGENTS.md reference: AGENTS.md:L91-L94
Useful? React with 👍 / 👎.
| 7. The contract additions the acceptance list depends on: `entryKind` per | ||
| admitted entry, the two identity fields on the request, a path-only refusal | ||
| variant carrying no `actual`, and an explicit recovery-state result in both | ||
| the operation and MCP unions. |
There was a problem hiding this comment.
If implementation chooses acceptance option (a), which retains and versions the Colorful projection as settlement content, this supposedly complete contract-addition list provides no field or result variant in which to retain that projection; the canonical settlement currently contains only the source bytes and their evidence. The cycle would therefore have to expand its contract outside the recorded boundary or could not replay the prose answer, so include this conditional contract work here or remove option (a).
AGENTS.md reference: AGENTS.md:L91-L94
Useful? React with 👍 / 👎.
| Every claim the packet makes about the current tree was checked against the | ||
| tree, not against memory. All eight cited paths resolve: |
There was a problem hiding this comment.
The final packet cites nine unique src/** paths, because the new prerequisite-read text adds src/mcp/workspace-router-runtime.ts, but this witness still claims that all eight cited paths were checked. Although the router path is mentioned separately below, the stated 8/8 source-truth count no longer describes the packet being witnessed; update the count and enumeration.
AGENTS.md reference: AGENTS.md:L130-L130
Useful? React with 👍 / 👎.
1 participant
Closes the retro gate that PR #245 went around, and clears the outstanding issues first — the retro is committed before this PR exists, which is what
METHOD.mdactually requires.Two commits, in order
c61023f8— the outstanding issues. A fourth review round was never run before #245 merged. I ran it as a self-audit and it found eight defects, every one the same class Codex caught twice: sections edited in rounds 1–3 left sections that were not edited contradicting or under-scoping them.replayProjectionreplayProjectionentryKind, 2 identities, refusal variant, recovery-stateThe last one is the sharpest.
repo-paths.tscallsrealpathSync.nativeandlstatSyncbeforeRepoWorkspaceexists, so an ordering assertion scoped to the observation authority would report clean precisely when it is wrong. That was stated only in an acceptance criterion; it now sits where the proof is defined.7b09aca0— the retro.The retro records the violation rather than smoothing it
AGENTS.mdandMETHOD.mdboth say a PR must not precede the cycle's local retro. #245 opened first. That is in the Drift section verbatim, including the rule's own line — "the PR may review the retro, but it must not be the first place the retro exists" — which describes exactly what happened.Other drift recorded: review found 16 defects across 3 rounds and never converged (3 → 7 → 6); five of the sixteen were caused by the previous round's repair; and the same defect class recurred one layer up, which is why the fix stopped being "enumerate again" and became "put the enumeration in code and assert totality".
outcome: not-met— accurate, and within the existing vocabulary. I drafteddesign-recordedand rejected it: inventing a third enum value across a 79-retro corpus is the exact drift this cycle is about.Debt filed
CLEAN_design-packets-receive-no-automated-review.md—.coderabbit.yamlexcludes**/*.md, so CodeRabbit read zero lines of docs(design): first retained workspace observation packet (#228) #245. The artifact that defines what the next cycle builds is the one artifact the review gate does not read, and CI can't cover it — it passes on docs by construction.CORE_design-packet-consistency-lint.md— every round-four defect was mechanical: an invariant with no test step, a counter in one list and not the other, raw-equality language surviving beside a named projection, a stalesrc/path. A structural lint catches all of them, and no rule of it asserts what the packet should say.Verification
Full suite last green on the
dc619514baseline (258 files, 2056 tests); this branch changes no file that suite loads.Refs #228 — does not close it. The hill is untouched; implementation has not started.