[architecture] Define end-to-end MCP admission regression for #181 - #200
Conversation
Architecture review updateCompleted two orthogonal architecture passes for #181. Round 1 findings addressed
Architecture updates are in Round 2 resultNo further architecture findings identified in the reviewed scope. #181 remains test-only and must not introduce test-only production bypasses or a second admission implementation. This is an architecture-only PR and does not implement #181. |
Integrated architecture review — round 3 findings (before correction)Verdict: Needs architecture changes. Implementation must not proceed from this revision. High — the required failure/recovery matrix is absent
High — migration/mixed-version/rollback proof is unownedCurrent schema lacks nonce/claim fields and artifact uniqueness. S6 must exercise additive expand, dual read/write, legacy Medium — CI partitioning has no executable commands or numeric budgetsThe amendment asks for partitions but Medium — lease tests cannot use a mocked worker clockLines 191-199 say “fixed clocks.” Claim/expiry comparisons must use PostgreSQL time. Use database timestamps, relative expired rows, and deterministic barriers; test both lock acquisition orders and ownership compare-and-set failure. Coverage gaps
Inspected scope: all slice/ADR documents, current route/handoff/executor/schema/migration, package scripts/CI shape, real-PostgreSQL concurrency spec, and UI test seams. Confidence: high. Provider-specific ACP cancellation and rendered UI remain unchecked; this is not proof of correctness. |
Integrated architecture review — round 7 findings before correctionThe fresh integrated pass found the following S6 evidence gaps:
These are architecture/test-contract corrections only. The PR remains draft and no production feature or merge is part of this pass. |
Integrated architecture review — Round 8 findings (before correction)Verdict for this round: blocked because S6 does not yet prove four newly exposed cross-slice boundaries.
Release proof must also exercise the actual checked-in epoch-activation command/runbook under both bridge-trigger orderings and a genuine pre-trigger worker fixture. Inspected stack head: |
Round 8 addendum — additional required racesS6 must also cover:
|
Integrated architecture review — Round 9 downstream findings (before correction)S6 must add executable proof for the post-submission quiescence contract:
Posted before correction. |
Round 9 addendum — final state/order testsAdd these S6 proofs before readiness:
Posted before correction. |
Integrated architecture review — Round 10 downstream finding (before correction)S6 must include host-ledger and integrity alert/resolution rows in the declared complete lock tail and race per-file intent/outcome, quiescence alert insertion, privileged repair, recovery, and finalization in both relevant orderings. No transaction may wait for the host fence while holding a database row lock. |
Integrated architecture review — Round 10 additional blocking findingSeverity: High Project management can bypass the host-effect exclusion boundaryThe S6 matrix does not race host apply/recovery against project-root repoint, project deletion, path swaps, or reuse of the same canonical host path by another project. A project-ID-only worker/recovery fence cannot exclude those current management-route filesystem operations. Required S6 additions:
The same pass also found two stale shorthand lock-tail summaries in this PR/ADR that still say artifacts → actions → gates. They must include host ledgers/entries and integrity alerts/resolutions. The upstream design correction belongs in #198; this PR must consume and prove it. |
Integrated architecture review — Round 10 test-contract addendumPR #200 must consume the full Round 10 corrections and prove the following missing cases:
The two stale shorthand summaries in this PR/ADR that say artifacts → actions → gates must also be expanded to the complete tail. CI manifest, budget, no-skip/no-retry, and forward-schema rollback design were otherwise coherent in this pass. Runtime/PostgreSQL/process-tree/browser proof remains an implementation prerequisite. |
Integrated architecture review — Round 11 test findingsS6 must add two exact cases from the fresh state-table pass:
The expected effect/ledger table and PostgreSQL/finalizer/parser fixtures must consume these same outcomes. |
Integrated architecture review — Round 11 lifecycle test addendumS6 must add exact executable barriers for:
The canonical lock-order assertions must include worker-instance rows immediately after the protocol epoch. |
Integrated architecture review — Round 11 evidence-bypass test addendumS6 must add exact executable cases for both new S4 blockers:
Run each lifecycle in both transaction orderings and prove no database lock is held while waiting for the namespace, resource, or containment fence. |
Integrated architecture review — Round 12 test findings (before correction)S6 must add two exact downstream assertions:
|
Integrated architecture review — Round 12 additional test findings (before correction)S6 must add exact failure-injection coverage for two corrected lifecycle contracts:
|
Integrated architecture review — Round 12 state-machine test finding (before correction)S6 must exhaust the corrected disjoint success branches:
Database constraints, finalizer, repair, parser, API, and S5 fixtures must reject success in the generic pre-stage row, a fabricated |
Integrated architecture review — Round 12 recovery and presentation test findings (before correction)S6 must add exact coverage for three corrected contracts:
|
Integrated architecture review — Round 12 containment test finding (before correction)S6 must prove that normal success empties the per-run execution group and releases the resource fence without terminating the long-lived queue/control worker. It must also prove authenticated child handoff, descendants unable to escape the run group, queue-worker crash with child survival becoming orphaned, child/control crash ordering, and release only after the trusted adapter proves the complete per-run group empty. |
Integrated architecture review — Round 12 root-exclusion test finding (before correction)S6 must cover existing and nonexistent parent/child creates, repoints, cleanup, tombstone, and root reuse in both acquisition orderings. Include crash after parent or child materialization, alias/case normalization, concurrent reservation-to-binding conversion, and recursive cleanup. No parent operation may delete or absorb a live/reserved descendant, and no child may bind beneath a live/reserved parent. |
Integrated architecture review — Round 12 sibling-claim test finding (before correction)S6 must create terminal package A with host-apply review required, repository-change review required, and each independently, then race independent ready sibling B in packet, packet-free, and handoff-only modes. B must create zero claim, lease, run, repository read, or write until the exact A review/quarantine barrier is resolved. Task reconciliation, periodic sweeps, direct progression, Redis replay, and review-decision paths must share the same barrier and lock order. |
Integrated architecture review — Round 12 mixed-version test finding (before correction)S6 must exercise a genuine old project create/repoint/delete at each rollout boundary. It must either be safely completed before the maintenance barrier or fail before path read/filesystem work after v1 ingress/credentials are revoked. Race cutover reconciliation with v2 grant/claim and activation in both lock orderings; assert no project -> epoch -> task/package lock path, no stale issuable decision, and no old service restart. |
Integrated architecture review — Round 12 recovery-action test finding (before correction)S6 must cover both grant modes with |
Integrated architecture review — Round 12 evidence-scanner test finding (before correction)S6 must exercise FIFO, socket/device/special entries, symlink loops and out-of-root links, huge files/trees, ignored and untracked secrets, concurrent mutation, and Forge runtime-directory churn. The versioned scanner must finish within hard bounds, never follow a link or read a special file, fail preflight before exposure when a baseline cannot be proved, and return post-call |
Integrated architecture review — Round 12 fence-service trust test finding (before correction)S6 must adversarially test unauthorized socket/API calls, state-file mutation/deletion, service |
Integrated architecture review — Round 12 tombstone-state test finding (before correction)S6 must seed queued |
Integrated architecture review — Round 12 rootless-project test finding (before correction)S6 must create a rootless GitHub/remote project after epoch 2 with every local binding field null and prove it has no filesystem authority. Reject partial root/binding sets. Then attach a local root only through the full namespace reservation, exact writer-instance, hierarchical exclusion, binding revision, and grant-reconciliation protocol. |
Integrated architecture review — Round 12 rollout-sequence test finding (before correction)S6's rollout rehearsal and runbook assertions must distinguish |
Integrated architecture review — Round 12 root-revision test finding (before correction)S6 must seed unbound legacy projects, perform zero/one/multiple pre-bind path changes, then bind and repoint away/back. The revision starts in the single explicit unbound state, every bind/repoint compare-and-set strictly increases it, no command forces revision 1 after a prior increment, and no old decision becomes issuable again. |
0bf42e2 to
e3ed7e4
Compare
Integrated architecture review — Round 12 correctionsCorrected in S6 now requires exact SQL/finalizer/repair/parser/API/S5 and race/failure-injection coverage for the corrected success tuples, review-first action flow, repository-evidence joins, audit-versus-quarantine abandonment, sibling local-change barriers, authenticated W2 recovery, protected per-run containment/service attacks, bounded scanning, hierarchical root exclusion, writer-pinned reservations, archived-project cancellation, rootless projects, monotonic root revisions, two-phase key rotation, and the post-drain root-trigger/activation sequence. The failure matrix and rollout rehearsal use the exact binding and activation commands, preserve immutable delivery, and keep the four PRs draft-only. Validation: documentation-only diff; |
…172S6AtomicTransition
# Conflicts: # .github/workflows/web-ci.yml # web/__tests__/epic-172-s3-release.test.ts
# Conflicts: # .github/workflows/web-ci.yml
![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: b3aeeef378
ℹ️ 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".
…d 25 review resolution - mcp-host-boundary-trusted.yml now rejects any reviewed_sha that is not an exact 40-char lowercase hex commit SHA, closing the moving-ref checkout gap flagged by chatgpt-codex-connector (P1). - issue-181-review-amendments.md records Round 25: the five earlier Joncallim P0/blocker findings are verified resolved at current head (rechecked tsc/lint/vitest/playwright --list), the codex P1 SHA fix is noted, and the codex P2 S6-transition SQL mismatch is documented as a known, deferred limitation (dead code path, controller disabled by default) acceptable for this beta-scoped architecture PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both manifest wrappers quarantine child stdout/stderr, so a CI failure previously printed only MCP_*_CONTRACT_REJECTED with no indication of what went wrong -- and with no raw report upload there was nothing else to read. Emit a fixed reason code from a closed enum, plus the canonical scenario IDs involved in an identity mismatch. The output-quarantine contract explicitly permits fixed schema-free status codes and canonical IDs on the live runner channel; it forbids child bytes, which are still never emitted. Reported identifiers are filtered to the canonical execution-key shape and anything else is reduced to a suppressed count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…partition The S6 issuance suite drove forge.insert_packet_authorization_snapshot_v2 directly as forge_packet_issuer. Three deliberate changes in the #198->#199 stack make that invalid: - 7876621 dropped epic_172_s4_protocol_state; activation is now derived from Step 0's forge_epic_172_enablement_state singleton. - The routine gained p_local_claim_token, splitting local from packet claims. - The routine became an internal helper the packet issuer is forbidden to call. epic-172-s4-context.test.ts asserts the GRANT is absent, and a live has_function_privilege check confirms it. epic-172-s4-postgres.test.ts already implements the same three scenarios under the same names and passes against real PostgreSQL 16, so porting would duplicate lower-slice coverage that the architecture's Coverage ownership section explicitly reserves for #179. Remove the issuance partition from S6 end to end: spec file, Playwright project and tag, manifest partition, bridge inventory, wrapper and suite-contract maps, controller budget, and the ordinary-CI step. Neither definition of test:mcp:issuance survives -- the base's alias is unused and its three files are already covered by test:unit:zero-skip and test:mcp:s4-postgres. The S6 manifest is now five partitions driven by four suite commands; docs and sentinels updated to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing the issuance partition left a stale `partitions.length !== 6` literal in run-playwright-contract.mjs. The manifest check runs before anything is spawned, so every Playwright partition failed at startup in under a second -- CI showed only MCP_PLAYWRIGHT_CONTRACT_REJECTED with reason=wrapper_error. Derive the expected count from MANIFEST_PARTITIONS, and give the manifest and partition-contract failures an `invalid_manifest` reason code so this class of error names itself instead of collapsing into wrapper_error. Add a regression test asserting the wrapper's partition map matches the checked-in manifest and that the count stays derived. Verified it fails when the old literal is put back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # .github/workflows/web-ci.yml # web/package.json
a0f4861
into
architecture/issue-180-mcp-operator-copy
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e634bcf9b
ℹ️ 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".
| steps: | ||
| - name: Require an immutable full-length commit SHA | ||
| run: | | ||
| if [[ ! "${{ inputs.reviewed_sha }}" =~ ^[0-9a-f]{40}$ ]]; then |
There was a problem hiding this comment.
When a dispatcher supplies a value such as $(malicious-command), GitHub substitutes it directly into this double-quoted Bash expression and command substitution runs before the regex can reject it, allowing checkout-independent code execution on the trusted self-hosted runner after environment approval. Pass the input through env: and validate the quoted environment variable instead. Fresh evidence beyond the prior immutable-ref comment is that the newly added guard is itself the pre-checkout injection sink.
AGENTS.md reference: AGENTS.md:L141-L143
Useful? React with 👍 / 👎.
| expect(result).toEqual({ code: 124, stderr: 'DEADLINE_EXCEEDED\n' }) | ||
| descendantPid = Number(await readFile(descendantPidFile, 'utf8')) | ||
| expect(Number.isSafeInteger(descendantPid) && descendantPid > 0).toBe(true) | ||
| expect(() => process.kill(descendantPid!, 0)).toThrow() |
There was a problem hiding this comment.
On Linux runners where PID 1 does not immediately reap orphaned children, the killed descendant remains a zombie, so process.kill(pid, 0) succeeds even though it can no longer execute. In the inspected environment this made the newly mandatory npm run test:mcp:contract fail solely at this assertion, with /proc/<pid>/stat reporting state Z and PPID 1; affected CI environments therefore remain red. Check for a non-running state or use a reaping harness rather than requiring immediate PID disappearance.
Useful? React with 👍 / 👎.
| controller budget described above. Workflow/process ceilings remain defensive inner | ||
| bounds and cannot authorize a retry or extend that controller deadline. | ||
|
|
||
| The release-blocking host-boundary job in `.github/workflows/web-ci.yml` targets a |
There was a problem hiding this comment.
Operators following this architecture are directed to a release-blocking host-boundary job in web-ci.yml, but that workflow contains only the contract, PostgreSQL, and operator partitions. The self-hosted job is actually in mcp-host-boundary-trusted.yml, and that workflow explicitly states that its completion is advisory while the external controller owns the required Check Run. Correcting both the path and authority description is necessary so release verification does not inspect the wrong workflow or treat runner success as the gate.
Useful? React with 👍 / 👎.
2 participants
Source Issue
Refs #181
Refs #172
Status
Round 26 (orthogonal, merge-aware) is complete and this PR is now
MERGEABLE.The key correction from Round 25: that pass reviewed the S6 head in isolation and wrongly called two P0 findings stale. Re-running the review against the merged result of the current #198/#199 stack showed both were real and recur on merge, because the same files are edited on both sides. The branch was 66 commits behind its base with five conflicting files, all at the S6↔S4/S5 seam.
All five conflicts are resolved, and the merged tree is verified green:
tsc --noEmit,eslint --max-warnings=0,test:unit:zero-skip(1741 passed / 0 failed),test:mcp:contract,next build, andgit diff --checkall clean, with every manifest partition collecting exactly its declared scenario IDs. Seedocs/architecture/issue-181-review-amendments.md("Integrated review round 25") for the per-conflict resolution table and the beta-scope note.Round 26 also found, by running the merged suite against a real PostgreSQL 16, that the S6 packet-issuance partition was duplicating coverage #179/S4 already owns — through a routine the packet issuer is now deliberately forbidden to call. That partition has been removed from S6 end to end and handed back to S4; see "Packet issuance belongs to #179/S4" in the amendments doc. The S6 manifest is now five partitions driven by four suite commands.
Not yet covered, and stated plainly: the Playwright
mcp-postgresandmcp-operator-*partitions were collected but not executed here, and the host-boundary partition still requires the self-hosted trusted runner.This PR contains architecture and test scaffolding only. The entire S6 controller surface is imported solely by its own unit tests — no route, worker, CLI, or component references it — so it cannot change application behaviour in this beta.
Summary
Defines the release-critical S6 proof system for Epic #172: exact contract fixtures, real PostgreSQL/routes/workers, thin Playwright operator flows, a separately trusted supported-host controller, signed evidence, output quarantine, teardown/destruction proof, and the ten-node activation gate.
Scope
Integrated Review Rounds
web-ci.ymlconflict, merged the base, and re-verified the integrated tree against a real PostgreSQL 16. The two mis-resolved threads are re-opened with corrections.Findings Corrected
not_started; only exact durabledefinitive_not_startedmay authorize it.invokingrecover touncertain; only the still-live owner may commitreturned.session_userreader, historical task-log scrub, append-only reapproval/index migration, branded S5 join, and eight-head attack matrices.Round 24 Corrections
public.sessionsdigest/expiry/revocation/rekey migration and its valid, expiry, cache-failure, crash/resume, concurrency, and raw-key-removal regressions.s3_issue_178.Cross-Slice Contracts
s5_s6_release_readyonly after ingress/issuance enablement evidence; legacy-root scrub is forbidden before that exact receipt.Remaining Implementation Risks
Architecture readiness is not release proof. Implementation must still execute real PostgreSQL interleavings, supported Ubuntu containment, distinct principals, copied-token attacks, controller signature/App checks, zero-egress and output-leak sentinels, teardown/destruction, exact manifest counts, and every stop condition.
Exact Implementation Order
ingress_and_issuance_enablednode as the non-extendable 1,560-second provisional operation; every boundary also requires the direct-controller 10-second heartbeat and at-most-45-second livelease_expires_at.enabled_build_tests_green, appends5_s6_release_ready, and promote only that exact live operation.Validation
c86e6d3(merged with basec7d2dcd; PR reportsMERGEABLE/CLEAN)architecture/issue-180-mcp-operator-copyat277f5d5a757b1e50ab303956d73b28fd8cdc46d4git diff --checkclean.test:mcp:contract→MCP_VITEST_CONTRACT_PASSED;test:mcp:postgresande2e:mcp-operator→MCP_PLAYWRIGHT_CONTRACT_PASSED(the wrapper runs with--forbid-skips --forbid-retriesand proves collected IDs = executed IDs = manifest); mandatory S4 PostgreSQL zero-skip proof, mandatory S3 PostgreSQL concurrency proof (14 passed), Step 0 disabled-ingress (1 passed), and the fail-closed Step 0 E2E bridge suite (23 passed) all ran.mcp-postgresprojects matches the mergedfilesystem-grant-lifecycle-concurrency.spec.tsexactly (16 tests, 2 tagged@mcp-postgres), independently confirming that conflict resolution.