feat: add Epic 172 deterministic filesystem grant recovery#197
Conversation
Architecture review updateCompleted two orthogonal architecture passes for #178. Round 1 findings addressed
Architecture updates are in Round 2 resultNo further architecture findings identified in the reviewed scope. Remaining uncertainty is implementation-level PostgreSQL concurrency and legacy-data behavior; the PR requires the listed real database tests before implementation can be considered correct. This is an architecture-only PR and does not implement #178. |
Integrated architecture review — round 3 findings (before correction)Verdict: Needs architecture changes. Implementation must not proceed from this revision. Blocker — the proposed hold is interpreted as terminal task failure
High — grant precedence has no durable ordering identity
High — revocation/narrowing lacks a negative reconciliation transition
Medium — project-wide ownership still conflicts with the retained amendmentThe primary service trigger includes package-local Cross-PR and rollout requirements
Inspected scope: PR/issue/ADR documents; current admission reader, handoff/orchestrator, both grant routes, schema/migration, and PostgreSQL test shape. Confidence: high. This is not proof of implementation correctness; executable PostgreSQL interleavings remain mandatory. |
Integrated architecture review — round 7 findings before correctionThe fresh integrated pass found one S3 contract precision issue:
Required correction: align the marker type and ADR with the canonical reader and add exhaustive phase/consumed compatibility tests. This is an architecture finding only; no production code is being changed in this review pass. |
Integrated architecture review — Round 9 finding (before correction)Medium / blocking-by-consistency: S3 restates the cross-slice “full” lock order as ending at packet artifact. Round 8 correctly added review-gate rows as the S4 tail because gate decisions must not lock backward into package/finalizer state. Leaving #178's authoritative lock-order text unchanged gives implementers two different “complete” orders. Required correction: append review-gate rows after packet artifact in #178 and the S3 ADR summary, while keeping the explicit statement that ordinary S3 transactions stop at approval/decision rows. This changes no S3 mutation ownership; it only restores one canonical order. Inspected final-stack head: |
Round 9 addendum — S3 to S4 resolver suffixThe S3 one-time-reapproval handoff repeats the S4 resolver suffix as prior run → audit, but Round 8's complete-tuple rule also requires the exact packet artifact. Align this reference to prior run → audit → exact artifact and require canonical typed equality before the S4 marker can be cleared. S3 still owns only nonce rotation; S4 owns the downstream validation and mutation. This is the #197 side of the consistency finding already recorded on #198, posted before correction here. |
Round 9 addendum — complete shared lock tailThe final concurrency pass found that the order called “complete” still names one packet artifact but omits other run artifacts and issuance-recovery action rows that S4 locks/inserts. Before correction, #197's cross-slice reference must be aligned to the complete S4 tail:
S3 still stops at approval in ordinary mutations. This is only the shared-order reference; S4 owns the suffix implementation. |
Integrated architecture review — Round 10 finding (before correction)Medium / blocking-by-consistency: the S4 host-effect correction added run-scoped apply ledger entries plus integrity alert/resolution rows, but the cross-slice order repeated by S3 does not yet place them. The complete tail must be:
Ordinary S3 mutations still stop at the grant decision. This updates only the shared suffix reference. |
Integrated architecture review — Round 10 addendumDisposition: Changes requested before architecture readiness High — S3 can clear the mandatory-review task barrierS3 currently moves a task from Required correction:
Medium — S3's one-time handoff still names the old lock tailThe one-time reapproval handoff says S4 continues only through the prior run, audit, and exact packet artifact. Any mutating resolver must state the full applicable suffix: host ledger/entries → all artifacts → existing/new recovery action → integrity alerts/resolutions → review gates. Its task-wide review check must lock sibling packages in the canonical package order; “package-local” limits grant evaluation, not the mandatory task barrier. The canonical top-level order is now correct; these individual paths must consume it without shorthand that lets an implementation skip rows. |
Integrated architecture review — Round 11 addendumSeverity: High Legacy approval authority cannot be proven retrospectivelyThe current database has no immutable canonical-root fingerprint or root-binding revision captured when a pre-v2 approval was made. A host-binding command that validates today's configured path cannot prove that the approval originally authorized the same repository. Binding such an approval to revision 1 would manufacture authority. Required correction: every legacy filesystem approval without a stored root-binding revision remains non-issuable after project-root binding and requires explicit operator reapproval. The binding command may bind the project, never upgrade approval authority. Remove every “proven unchanged root may bind legacy approval” exception from S3, ADR 0009, rollout, and S6. The same pass also requires the S3 copy of the complete suffix to include the worker-instance row introduced by S4 after the protocol epoch. |
Integrated architecture review — Round 11 consistency findingSeverity: Low The S3 reconciliation input lists only Required correction: add a closed |
Integrated architecture review — Round 12 mixed-version finding (before correction)Medium — The epoch-1 root trigger cannot invoke S3's sole TypeScript reconcilerS4 says the PostgreSQL root-mutation trigger handles a legacy epoch-1 repoint by invoking S3's negative reconciler. S3 defines that reconciler as the sole server-side TypeScript service operating after the route has locked project -> tasks -> packages -> approvals. PostgreSQL cannot invoke it; acquiring those rows after the trigger has reached project -> epoch would reverse the global order. Required correction: preserve S3 as the sole reconciliation policy and define an executable cutover bridge. A safe route is to keep the root trigger out of the live legacy expand window, disable project ingress, revoke/terminate v1 web credentials/sessions, drain old services, run the canonical S3 reconciliation/backfill under its normal order, then install/enable the root trigger and activate epoch 2 before v2 ingress. A trigger never implements or locks S3 rows. If mixed-version root writes are retained instead, every writer must call an application bridge and the trigger may validate only an exact transaction-local proof. This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 rollout wording finding (before correction)Low — The cutover sequence does not name the epoch activation step unambiguouslyS3's rollout enables S3 writers after root binding without explicitly placing epoch activation between binding and enablement. Downstream prose says “that command” after naming the binding command, even though a different command owns epoch activation. Required correction: use one repeated exact sequence and command ownership: disable ingress/issuance -> revoke v1 credentials and drain -> reconcile/backfill and bind roots -> run This is architecture/rollout wording, not an implementation finding. |
Integrated architecture review — Round 12 root-revision finding (before correction)Medium — “Initial revision 1” is incompatible with pre-bind root changesThe rollout says every project binds at initial root-binding revision 1, while downstream schema/compatibility text lets an unbound legacy repoint increment the same counter. A project can reach revision 2 before its first binding; resetting it to 1 breaks monotonicity, while requiring exactly 1 makes it unbindable. Required correction: represent unbound state explicitly as revision This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 correctionsCorrected in
Validation: documentation-only diff; |
Integrated architecture review — Round 13 finding (before correction)High — An S3-only hold can remain non-actionable foreverS3 deliberately leaves a task Required correction: define one shared operator-hold task-convergence service over the recognized marker union, including This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 13 correctionCorrected in
Validation: docs-only; |
|
Round 14 finding — High / blocking — cross-PR lock/state contradiction (PR #197 primary; ADR/S4 and S6 downstream) The S3 one-time reapproval resolver still uses the pre-generic-evidence lock suffix: epoch → worker instance → run → runtime audit → host ledger → artifacts → issuance actions → integrity → gates. It validates only packet audit/artifact/ledger state. The corrected S4 contract now requires authenticated instance → binding generation/rotation → hierarchy guard → run → generic local-run evidence → optional runtime audit → ledgers/artifacts/actions, and packet/local-effect markers may coexist without either owner clearing the other. Evidence:
Concrete failure: one-time reapproval can lock the runtime audit before the generic local record (the reverse of S4 recovery/finalization), or omit the generic record and clear Required correction: use one canonical database lock order. S3 must prelock every sibling and the resolver must lock/validate binding generation, generic local-run evidence, working-tree and Git-control review fingerprints, local-effect marker/task projection, then the optional packet audit/artifact/actions. It may clear only the packet marker. If local-effect review or marker remains, the package stays blocked and no wake/new run occurs. Required proof: race one-time reapproval against generic local recovery/finalization in both orderings; prove observed waits with no deadlock; cover coexisting packet/local markers; prove unresolved or mismatched generic evidence returns conflict without consuming the nonce or clearing markers; and prove exact local review preserves the task projection while only eligible packet state clears. This finding was published before correction. Architecture only; no implementation or merge is authorized. |
|
Round 14 finding — Medium / blocking — authoritative S3 rollout omits the mandatory expansion-window journal drain (PR #197 primary; #198/#200 activation impact) S3 tells operators to add only revision/marker fields, disable and drain legacy services, reconcile projects that “may have changed,” and then run Evidence:
Cross-PR impact: an implementer following S3’s authoritative primary can skip the durable completeness proof assumed by S4 and S6 activation, leaving a legacy root mutation outside the binding scan. Required correction: synchronize S3 schema, rollout, implementation order, stop conditions, and tests with the journal trigger, post-session watermark capture, exact Required proof: legacy insert, repoint, repoint-back, archive, and delete around the drain; an in-flight commit crossing the drain boundary; crash/resume reconciliation; binding and activation reject every missing generation/outcome. This finding was published before correction. Architecture only; no implementation or merge is authorized. |
|
Round 14 correction completed at fd32217. Resolved the stale recovery suffix by carrying the canonical all-mode lock order through authenticated instances, binding generation and rotation, hierarchy, generic local evidence and task projection, optional audit, ledgers, artifacts, actions, integrity rows, and gates. The one-time resolver now validates working-tree, Git-control, and Git-storage fingerprints, clears only the packet marker, and cannot wake work while a local barrier remains. The expansion-window journal, post-session watermark, and exact restartable reconciliation command are now mandatory cutover evidence. Validation: git diff --check, clean worktree, exact stacked ancestry, and integrated stale-contract scan. |
|
Round 15 finding 15 — LOW / ADVISORY. #178 implementation order at docs/architecture/issue-178-filesystem-grant-recovery.md:582-585 tells the S3 slice to add #179 expansion-window root-change journal/trigger, while #181 lines 956-964 assigns the change journal and mixed-version schema to #179 and #178 otherwise calls it #179-owned. The rollout dependency is clear, but the per-PR migration owner is not; two implementers could create duplicate migrations or a dependency cycle. Required correction: name one owner. Prefer #179 owning the journal/trigger and command while #178 defines/consumes the canonical S3 reconciliation callback and remains compatible until the downstream schema lands, unless the journal is intentionally pulled forward and #179 explicitly consumes it without redefining it. |
fd32217 to
c33a9b1
Compare
|
Round 15 corrections are published at Resolved the #178 ownership ambiguity: S3 owns only the decision schema and dual reader in this slice; #179 owns the expansion journal/trigger, reconciler command, and cutover checkpoint. This keeps the implementation order from assigning the same migration authority to two slices. Validation: clean worktree, |
Round 16 finding — High / blocking cross-PR contradiction
That now contradicts the corrected canonical S4/S6 rollout: the legacy filesystem-first delete route must be replaced and drained before retained-evidence expansion; evidence foreign keys must be Concrete failure mode: an implementation following #178 can deploy the journal while the current delete route still removes the repository before SQL and while cascade foreign keys erase retained task/run/audit/artifact evidence. The later journal outcome cannot restore either the filesystem or deleted evidence. Affected invariant: retained immutable evidence and bridge-first migration safety. Downstream: #179 migration/cutover, ADR 0009 S3/S4, and #181 mixed-version regression. Smallest correction: make #178 explicitly depend on the pre-expansion bridge/drain/FK/guard checkpoint; journal only insert/root-update/archive generations; remove deleted-row test/command expectations; keep #179 as the sole journal implementation owner. Implementation test: kill/race the old delete route around the filesystem/SQL boundary, prove the bridge conflicts/archives before This finding is published before edits. The PR remains draft and unmerged. |
c33a9b1 to
611b248
Compare
|
Round 16 correction published in 611b248. The S3 rollout now requires #179’s bridge removal route, pre-bridge process/session drain, retention-safe foreign keys, and database hard-delete guard before the expansion journal opens. The journal and reconciliation watermark now truthfully cover insert/root-update/archive generations only; hard delete is already impossible. Migration tests separately prove the bridge/guard boundary and no longer claim a later journal can reconstruct deleted rows. Validation: git diff --check; architecture-only diff; no production implementation added. A fresh integrated orthogonal review will run after all four corrected slice heads are restacked. |
1 similar comment
|
Round 16 correction published in 611b248. The S3 rollout now requires #179’s bridge removal route, pre-bridge process/session drain, retention-safe foreign keys, and database hard-delete guard before the expansion journal opens. The journal and reconciliation watermark now truthfully cover insert/root-update/archive generations only; hard delete is already impossible. Migration tests separately prove the bridge/guard boundary and no longer claim a later journal can reconstruct deleted rows. Validation: git diff --check; architecture-only diff; no production implementation added. A fresh integrated orthogonal review will run after all four corrected slice heads are restacked. |
|
Round 17 orthogonal review finding — MEDIUM / blocking (global lock-order parity): #179’s canonical order names worker/root-writer instance rows and the combined local-run-evidence/task-projection source rows. #178’s normative restatements shorten those families, and ADR 0009 also omits explicit root-writer/task-projection members. An implementer following a shortened list can acquire a different suffix and invalidate the deadlock proof. Required correction: make #178 and ADR import/reference the one canonical #179 ordered list or enumerate the identical row families byte-for-byte. Add a static sequence-parity sentinel plus opposing-order PostgreSQL races across root-writer/generation and local-evidence/projection locks. Published before editing. |
|
Round 17 orthogonal review finding — HIGH / blocking (cross-slice ownership and rollout): #178 implementation step 1 requires #179’s bridge/drain/retention-safe-FK/hard-delete-guard checkpoint before S3 schema, while #179 implementation step 1 requires #178 decision/operator-hold contracts to land before #179 bridge work. Neither slice can legally land first. Required correction: define a separately landable #179 prerequisite Step 0 containing only bridge route, drain, FK replacement, and database hard-delete guard; land it before #178, then land #178 S3, then the remaining #179 expansion/producers. Add a release-order manifest/fixture proving the acyclic order and retention seam. Published before editing. |
98c5021 to
833e4a2
Compare
Joncallim
changed the title
…TP fixtures, legacy adapter, and logged 500 - Add 8-kind local projection head table with package preallocation (256 package limit, 2048 heads) - Enforce head identity immutability, reject reassignment, deletion, and missing heads - Persist full canonical requestedCapabilities in grant block metadata, not just missing subset - Add time-bounded legacy adapter contract bridging old filesystem-grants to S3 types - Add generic logged 500 error response utility with deduplication - Add authenticated HTTP grant fixture tests with mutation schema validation - Add S3 mutation-vs-claim contention E2E test from lower sibling - Update Step 0 E2E inventory with new concurrency test entry
Joncallim
left a comment
Joncallim
left a comment
There was a problem hiding this comment.
Ultra-deep orthogonal review of DeepSeek's local S3 head ce9ae5a922073f530c00aa685258b253703c5b5f against this PR's contract head.
Verdict: blocked — 3 blocker clusters and 2 high-risk validation gaps. The local implementation commits are not on the remote PR, so the inline findings are anchored to the exact contract clauses they violate. Remote green checks do not validate this local head. No implementation files were changed.
Axes inspected: contract/architecture, PostgreSQL ownership/ACLs, concurrency/atomicity, runtime wiring, compatibility/error paths, and regression evidence. Confidence: high for the cited scope. This review is not proof of correctness; fresh and upgrade PG16 migrations plus the real authenticated HTTP/race suite remain required.
- Add task_id column to work_package_local_projection_heads with FK to tasks
- Update preallocation trigger to populate task_id from work_packages.task_id
- Update backfill loop for task-scoped heads
- Add task_id index for per-task head queries
- Update fingerprint format: head:v1:{taskId}:{packageId}:{kind}:{index}
- Update TypeScript types and tests for task-level head identity
…ity marker tests, HTTP auth gate tests - Fix projection head fingerprint CHECK for two-UUID task_id:package_id format - Add PUBLIC SELECT grant for application role head reads; revoke ALL then grant SELECT - Add full-capability marker unit tests verifying canonical three-capability set - Add HTTP authentication gate tests for filesystem-grant and filesystem-grants routes
…4 protocol surface
…pter Resolves the remaining P1 compatibility/error-hardening thread on #197. Error boundary: - Add web/lib/http/route-error.ts with a branded PublicHttpError. Only 4xx errors constructed via publicHttpError() are trusted as public; an object that merely forges a numeric `status` can no longer impersonate one. - Route both the project and task filesystem-grant PUT/GET handlers through respondToRouteError(). Typed 4xx surface their approved message; every unknown/untyped 5xx returns fixed generic text plus a fresh correlation id, with the detail retained only in the internal server log. No exception message, path, SQL, prompt, nonce, or token reaches the HTTP response. - Reconciliation's httpError() now delegates to publicHttpError() so its approved 4xx operator messages stay public through the new boundary. Legacy compatibility adapter: - Introduce one canonical readFilesystemGrantBlockFromMetadata() reader and route the reconciliation and predicate call sites through it (removing the duplicate inline readers). - legacyFilesystemGrantBlock() now preserves the real work-package id, reads the block from its canonical metadata.mcpGrantBlock location, reconstructs the canonical compatibility record and verifies its fingerprint from the canonical fields, and accepts an injected clock for deadline behavior. Tests: - epic-172-s3-route-error-boundary.test.ts: hostile nonce/path/prompt/SQL message is suppressed from both PUT routes and the boundary; typed 4xx and forged-status cases; correlation-id contract. - epic-172-s3-legacy-adapter-block.test.ts: work-package-id preservation, canonical-location read, fixture-equivalent reconstruction/fingerprint, and injected-clock deadline. Validation (web/): tsc --noEmit clean, eslint --max-warnings=0 clean on changed files, git diff --check clean, full vitest 1458 passed / 7 pre-existing PostgreSQL skips (+10 new tests, 0 new failures/skips), next build succeeds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHZRmoaX4WLKt785UM9og3
- Use BigInt(7) instead of the 7n literal (target is below ES2020). - Annotate the console.error mock-calls map parameter to satisfy noImplicitAny. npx tsc --noEmit is now clean; both suites still pass (10 tests) and lint is clean. Vitest/esbuild and next build did not surface these because only tsc type-checks the __tests__ tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHZRmoaX4WLKt785UM9og3
![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: 10215ca3b8
ℹ️ 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".
2 participants
Closes #178.
Part of #172.
Requires the merged Step 0 bridge from #204.
What this completes
Forge now stores filesystem approvals as immutable history and changes authority only through preallocated current-decision pointers. Task and project decisions share one project-serialized revision, one root-binding revision, exact capability coverage, and a canonical project-to-package lock order. Denials, revocations, consumed one-time approvals, root repoints, and stale pointers converge to typed operator holds without misreporting the task as failed.
The branch adds the S3 migration, project- and task-level mutation services, approval/handoff reconciliation, explicit reapproval in the task UI, release-evidence completion, and real PostgreSQL race coverage. It does not enable protocol-v2 packet production or reopen Epic 172 ingress.
Safety and compatibility
TRUNCATErights.enabled_build_tests_greenbinds the exact enabled S3, S4, S5, and S6 build tuple; graph-node build tuples remain phase-specific.Validation
git diff --checkpassed.Stack
PR #198 remains stacked on this branch until #197 merges. Afterward it will be restacked onto
mainand validated independently.