[architecture] Define unified MCP operator presentation for #180#199
[architecture] Define unified MCP operator presentation for #180#199Joncallim wants to merge 42 commits into
Conversation
Architecture review updateCompleted two orthogonal architecture passes for #180. Round 1 findings addressed
Architecture updates are in Round 2 resultNo further architecture findings identified in the reviewed scope. S5 remains reader/presentation-only and must not persist or infer admission policy. This is an architecture-only PR and does not implement #180. |
Integrated architecture review — round 3 findings (before correction)Verdict: Needs architecture changes. Implementation must not proceed from this revision. High — the valid tuple table is contradictory
High —
|
Integrated architecture review — round 7 findings before correctionThe fresh integrated pass found two downstream presentation blockers:
The sibling-wait state also needs the S4-owned convergence transition specified on #198; S5 must remain actionless until that durable transition makes the task exactly |
Integrated architecture review — Round 8 findings (before correction)Verdict for this round: blocked by two S5 presentation contradictions inherited from the S4 persistence model.
Required correction: remove the non-durable failed-finalizing variants; retain only live phases derivable from persisted assembly/delivery; import S4's new closed post-submission stage; add bounded recovery/integrity copy; and keep all unknown or inconsistent tuples neutral and actionless. Inspected stack head: |
Round 8 addendum — complete-tuple validationOne additional S5 consistency requirement was found before correction: a packet recovery marker does not contain enough assembly detail to validate the normative terminal tuple by itself. The current-state reader must bind the marker to the exact prior audit/artifact, prove their typed terminal tuples are equal, and validate assembly + delivery + terminal status + failure code/stage together. Missing, mismatched, or terminal-success-plus-failure-marker evidence is neutral, non-retryable, and actionless. |
Integrated architecture review — Round 9 downstream findings (before correction)S5 needs two downstream states from the fresh S4 review:
Typed packet-integrity alerts also need neutral static copy, an owner/runbook reference, no web repair CTA, and no raw path/error detail. The last closed stage is Posted before correction. |
Round 9 addendum — review-pending sibling presentationWhen any sibling package is Posted before correction. |
Integrated architecture review — Round 10 presentation dependencySeverity: Medium The current integrity UI language says an audit/artifact mismatch needs operator repair. A true mismatch may not be repairable without rewriting immutable evidence, which the architecture correctly forbids. S4 is being corrected to provide an authorized append-only S5 must present this as permanent evidence quarantine with an explicit close/abandon action for authorized Release/DevOps operators. It must not promise repair, automatic recovery, retry, or successful packet reconstruction. The live state remains evidence-derived from the integrity alert/resolution records. No other S5 presentation blocker was found in this pass. |
Integrated architecture review — Round 12 S5 findings (before correction)Medium — The authoritative current-state join omits repository evidenceS5 joins the packet audit/artifact and host ledger, then binds the current marker to host-review fingerprints. S4 also requires exact equality for the pre-submission repository baseline, post-quiescence comparison, repository-review state, and their fingerprint. Without that join, stale or mismatched marker evidence could present a retry/reapproval action after a different repository review was completed. Required correction: the authoritative presenter joins and compares the complete repository baseline/change/review evidence and marker fingerprint in addition to host ledger/review evidence. Any missing, stale, or mismatched repository component renders the neutral integrity hold with no action. Medium —
|
Integrated architecture review — Round 12 recovery-action finding (before correction)High — Definitively rejected delivery can still require local-change review, but S5 exposes no review CTA
Required correction: presentation is review-first whenever either exact host or repository review is required, independent of delivery. Expose only the typed fingerprint-bound local-change review action; after it commits, present the grant-mode/delivery action. Keep “the provider rejected the request” distinct from “local files require review,” and never imply the provider changed them. This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 tombstone presentation finding (before correction)High — A removed project must not retain an executable action surfaceS5 correctly says “Project removed — evidence retained,” but S4 currently retains nonterminal work without an all-mode archived-project claim barrier. The presenter could therefore combine retained-history copy with a live retry/reapproval/progression action. Required correction: an archived/tombstoned project is history/evidence-only. S5 renders no task execution, retry, reapproval, review-gate, or root-management CTA for cancelled packages; only explicitly authorized evidence inspection or privileged quarantine history remains. This consumes S4's bounded project-removed cancellation state and never infers cancellation from missing paths. This is an architecture-contract finding, not an implementation finding. |
4bb2989 to
a98923c
Compare
Integrated architecture review — Round 12 correctionsCorrected in
Validation: documentation-only diff; |
Integrated architecture review — Round 13 presentation finding (before correction)S5's evidence join and exact retry/review predicates must include the versioned Git-control baseline/comparison fingerprint as well as working-tree evidence. Missing, changed, unverifiable, stale, or mismatched Git-control evidence must render the same review/blocking state as repository-local changes and must expose no path, hook, config, ref, or submodule detail. Success and retry controls remain hidden unless both working-tree and Git-control comparisons are exactly |
Integrated architecture review — Round 13 advisory finding (before correction)Low — “Both packet actions” contradicts the closed three-action mutation unionThe document says both packet actions carry immutable request identity, but the typed union has three packet mutations: retry, possible-submission acknowledgement, and local-change review. This stale count can lead an implementation or test to omit stale-request protection for the third action. Required correction: say “Every packet mutation action carries…” and name all three handlers. Each handler must reject a stale |
Integrated architecture review — Round 13 advisory finding (before correction)Low —
|
Integrated architecture review — Round 13 presentation finding (before correction)Packet-free and handoff-only local-root runs need the same crash-safe local-effect/repository review lifecycle as packet runs, but must not manufacture a packet audit, packet artifact, delivery state, or packet-retry action. S5 must read the packet-independent run-effect record and render exact local-change review/quarantine state for those runs. Packet evidence stays absent; submission acknowledgement and packet retry/reapproval CTAs stay hidden. The local-change action carries the exact run-effect record ID plus evidence fingerprint and rejects stale identity without mutation. |
a98923c to
40e5e11
Compare
Integrated architecture review — Round 13 correctionCorrected in
Validation: docs-only; |
|
Round 14 finding — Medium / blocking — contradictory local-review action ownership (PR #198 primary; #199/#200 downstream) The corrected schema makes Evidence:
Failure/cross-PR impact: implementations cannot choose a single idempotency source. A packet review can write the wrong table, while packet-free and handoff-only review cannot supply a runtime-audit ID. S5 and S6 may then disagree about replay and current-state truth. Required correction: define one generic local-effect recovery endpoint and request keyed by Required proof: packet, packet-free, and handoff-only local reviews; duplicate/concurrent/stale replay; review followed by packet acknowledgement or retry; exactly one local action row and zero issuance action rows for review. This finding was published before correction. Architecture only; no implementation or merge is authorized. |
|
Round 14 finding — Medium / blocking — S5 test contract still assigns packet identity to generic local review (PR #199 primary; #200 downstream) S5’s normative types and presenter correctly split packet identity Evidence:
Cross-PR impact: S6 can implement a manifest-bound test that contradicts S4’s packet-independent action model, and packet-free/handoff-only local review cannot satisfy the test. Required correction: split the test contract. Retry and submission acknowledgement use packet v2 identity; local review uses generic local v1 identity. Explicitly require cross-type substitution to fail closed. Required proof: component and route fixtures for packet, packet-free, and handoff-only review plus stale and cross-route identities. This finding was published before correction. Architecture only; no implementation or merge is authorized. |
|
Round 14 finding — High / blocking — sole generic review does not converge the dependent packet disposition (PR #198/#199/#200) The schema and S5 UI establish exactly one packet-independent local-review action and ledger. The S4 packet marker can nevertheless store Evidence:
Invariant: host/repository review is packet-independent, uses one generic request identity/action/ledger, and any exact dependent packet disposition converges from that committed fact without acknowledging delivery or clearing packet-owned evidence. Required correction: make the generic local-review transaction the sole review mutation and replay ledger. Under the full lock order, it must also compare-and-set any exact dependent packet marker to its stored Required proof: packet+local dual markers for every grant/delivery mode; generic exact replay; stale generic-versus-packet fingerprints; local review racing packet acknowledgement/retry/reapproval; no second review action; packet-free parity. This finding was published before correction. Architecture only; no implementation or merge is authorized. |
|
Round 14 finding — High / blocking — S5 suppresses recovery forever after exact review of detected repository changes (PR #199 primary; #198/#200 contract impact) S5 says exact review advances a changed or unverifiable working-tree/Git-control run to its stored packet disposition, but its general action gate then requires both immutable comparison results to be Evidence:
Cross-PR impact: every always-allow recovery with detected and reviewed ACP changes dead-ends in S5 despite a valid S4 disposition. One-time copy can promise reapproval while the presenter suppresses it. Required correction: action eligibility must accept either Required proof: both grant modes × all delivery states × working-tree-only, Git-control-only, and both changed/unverifiable. After exact review, pre-intent always-allow exposes retry; post-intent exposes acknowledgement then retry; one-time exposes the proper reapproval sequence. Stale/different fingerprints remain blocked. This finding was published before correction. Architecture only; no implementation or merge is authorized. |
|
Round 14 finding — High / blocking — Git evidence still omits object storage and other history-authority files (PR #198 primary; #199/#200 downstream) The bounded Git-control snapshot covers configuration, hooks, Evidence:
Failure/cross-PR impact: an unconfined ACP process can truncate or replace a pack/loose object, add an alternate/replace object, run repack or garbage collection, or inject unreachable objects while the named working-tree and Git-control evidence appears unchanged. Generic local evidence can clear, S5 can show success, and root reuse can proceed against corrupted or redirected history. Required correction: add a bounded Git-storage snapshot or broaden Git-control to every file that affects object resolution/integrity. Fingerprint opaque metadata and content under the common-directory fence; if an equivalent bounded filesystem snapshot/journal cannot prove the state, fail closed as unverifiable and disable execution/root reuse. Required proof: truncate a pack and loose object; swap a pack index; write alternates/replace/grafts/shallow state; run repack/garbage collection; add unreachable objects; repeat with linked external common directories. Every case becomes changed or unverifiable and blocks success, new work, and root management without leaking paths. This finding was published before correction. Architecture only; no implementation or merge is authorized. |
|
Round 14 finding — High / blocking — packet-free stale recovery has no terminal disposition when quiescence proves no local change (PR #198 primary; #199/#200 downstream) S4 says packet-free and handoff-only crashes create Evidence:
Failure/cross-PR impact: an unchanged packet-free crash can leave a package and root permanently held with no normal action, while different implementers may silently choose incompatible retry/failure semantics. Required correction: define a discriminated generic stale-run terminal matrix. Before quiescence, remain actionless and keep the resource fence. After exact W2/service receipt and comparisons: changed/unverifiable creates the local-review marker; unchanged/not-applicable creates no local marker and atomically reaches one explicit ordinary run/package/task disposition under the project’s existing retry policy, without inventing packet evidence or automatically rerunning. Specify task and wake behavior and exact replay/repair semantics. Required proof: packet-free and handoff-only crashes at every boundary with unchanged, changed, and unverifiable comparisons; W2/quiescence wait; ordinary terminal/retry policy for unchanged; exact local review for changed; no permanent actionless post-quiescence block; no packet audit/artifact/action in any no-packet case. This finding was published before correction. Architecture only; no implementation or merge is authorized. |
|
Round 14 finding — High / blocking — no protocol-v2 membership or replacement path exists after cutover (PR #198 primary; #199/#200 operational impact) Every process incarnation is never reused and independently authenticated. At epoch 2, claims/root writes accept only an Evidence: Required correction: specify a privileged epoch-2 membership-change command/runbook: disable relevant ingress; provision the dedicated candidate; validate host, key, generation, protocol and capability; lock epoch/instances; drain, revoke and terminate the old principal; promote a bounded audited replacement; define capacity, rollback and all-active-gone recovery. W1 death must not prevent a separately authorized fresh W2 from joining safely. Proof: rolling worker/root-writer restart; abrupt W1 death; all active workers gone; at-capacity replacement; claim/promotion/recovery races; replay by the revoked principal; bounded audit and fail-closed rollback. Published before correction. Architecture only; no merge authorized. |
|
Round 14 finding — Medium / blocking — mandated integrity CLI cannot address packet-free/handoff alerts (PR #198 primary; #199/#200 impact) Integrity alerts require generic local evidence and make packet audit optional, but the only inspect/resolve command is keyed by Evidence: Required correction: key inspect/resolve by mandatory Published before correction. Architecture only; no merge authorized. |
|
Round 14 finding — Medium / blocking — S5 has no typed packet-independent quiescence or integrity state (PR #199 primary; #198/#200 contract impact)
Evidence: Required correction: make the local presenter a closed union with generic Proof: packetless/packet local markers; active/orphaned quiescence; absent/wrong run/root/fingerprint; stale task projection; transition to exact marker/hold; exhaustive unknown state; zero packet controls. Published before correction. Architecture only; no merge authorized. |
…r199 # Conflicts: # web/app/dashboard/tasks/[id]/page.tsx
…join, 7 authenticated MCP routes, and server-only CAS-rechecked readers - Add BrandedTerminalJoin/FreshnessJoin React components with server-only boundary - Add 7 authenticated MCP S5 routes: admission, presentation, grant-state, recovery-state, terminal-state, freshness, local-evidence - All routes use freshness/fingerprint/CAS-rechecked presentation data - Add S5 server reader with typed presenters, freshness fingerprinting, and CAS recheck tokens - Feed all presenters from normalized server readers; persist no policy or recovery truth - Add filesystemGrantExpectedPointerFromState compatibility export for task page tests
Joncallim
left a comment
Joncallim
left a comment
There was a problem hiding this comment.
Ultra-deep orthogonal review of DeepSeek's local S5 head ef3629fa1e7389584fcef258bcd4b10e640c1355 against this PR's presentation/action contract.
Verdict: blocked — task authorization and ownership-token secrecy are violated; canonical readers/actions are not implemented; existing S3/review/retention controls regress. The implementation is local and unpushed, so these comments are anchored to the contract lines. Remote green checks cover only the architecture commit.
Axes inspected: cross-user authorization, secret exposure, typed terminal/current joins, action authority/staleness, client/server trust split, compatibility regressions, and UI reachability. Confidence: high in the inspected scope. Not proof of correctness; authenticated cross-user HTTP and real desktop/mobile action flows are still absent.
|
|
||
| ### Packet issuance recovery controls | ||
|
|
||
| The current-state reader validates S4's live run-scoped claim summary, versioned |
There was a problem hiding this comment.
[P0 security blocker — any authenticated user can read another task, including an ownership token]
All seven new /api/mcps/*/[taskId] GET routes only call getSession; none applies getAccessibleTask or a shared task/project ACL before querying caller-supplied taskId. An authenticated user who learns another task UUID can read its packages, grant history, recovery/terminal state, and evidence.
The worst endpoint is local-evidence/[taskId]/route.ts:19-38, which selects and returns raw claimToken. That is a live ownership credential and must never reach browser/API/log/export state.
Recommendation: resolve authorization through one server-only ACL loader before every query and return an indistinguishable 404/zero-data response cross-user. Never select/serialize claim tokens; emit only validated presentation facts and bounded fingerprints. Add two-user cross-project HTTP fixtures for all routes plus sink scans proving tokens are absent.
There was a problem hiding this comment.
Follow-up on 59dc55c: the implementation portion is resolved statically. All seven routes now call getAccessibleTask before reading, and local-evidence no longer selects or serializes claimToken. However, no two-user route tests were added and Web CI is red, so the fix is not regression-proven. Add cross-user 404/token-absence HTTP coverage before resolving.
There was a problem hiding this comment.
Third pass on 19397fad: production remediation remains statically correct—all seven routes call getAccessibleTask, and local-evidence no longer selects/returns claimToken. The new test claim is overstated: only 3 cases exist, only admission/presentation are imported, five routes are untouched, the ACL/database are mocked, the non-owner fixture bypasses the real loader’s null behavior, and token absence is not asserted. Add real two-user database-backed HTTP coverage for all seven endpoints plus response/sink token scans before resolving.
There was a problem hiding this comment.
Fourth pass on eea4cf5d: the seven production ACL checks and claimToken redaction remain correct. Regression proof is still incomplete: the same three mocked GET tests cover only admission/presentation, not the new PUT, five other readers, real two-user ACL behavior, or token absence. Keep this open until all seven routes and mutations are database-backed and response-scanned.
There was a problem hiding this comment.
Fifth-pass status at f8e8dc7: partially remediated, still blocking. The explicit safe-column select removes grantNonce/effectiveGrant from this route, but currentDecision returns a raw Drizzle row containing bigint grantDecisionRevision/rootBindingRevision. NextResponse.json throws "Do not know how to serialize a BigInt"; only history converts those fields. The new ACL tests still cover three mocked GET cases rather than all seven routes and a real two-user database boundary.
Exact implementation:
- Create one explicit serializeDecision DTO used by both current and history; convert every bigint to decimal string and every timestamp to ISO text, and enumerate only public fields.
- Never pass a database row directly to NextResponse.json.
- Add a fixture with an active current decision and assert HTTP 200 plus recursive absence of claimToken, grantNonce, effectiveGrant, and internal pointer/token fields.
- Add a table-driven suite for all seven S5 routes: unauthenticated 401, real non-owner 404, owner success, malformed/stale input rejection, and recursive sink redaction.
- Use real users/tasks in PostgreSQL for the ownership boundary; keep mocks only for route plumbing.
There was a problem hiding this comment.
Final-head reassessment at c4645bd: partially resolved, left open. One safe DTO serializer now converts bigint/date fields and excludes grantNonce/effectiveGrant, and all seven routes share the owner-gated reader with table-driven route plumbing tests. Real two-user PostgreSQL HTTP coverage and a recursive forbidden-field sink scan are still missing.
|
|
||
| The current-state reader validates S4's live run-scoped claim summary, versioned | ||
| `packet_issuance` marker, or typed packet integrity hold and passes it to | ||
| `packetCurrentStatePresentation`. For a terminal artifact plus recovery marker, it |
There was a problem hiding this comment.
[P0 blocker — terminal/current evidence is fabricated, not branded or joined]
terminal-state/[taskId]/route.ts:19-31 converts any truthy JSON field into assembled, submitted, and terminal; it does not parse S4's closed v2 unions or join exact run/audit/local-evidence IDs and fingerprints. Malformed, live, stale, and mismatched rows can therefore be presented as completed facts.
BrandedTerminalJoin.tsx is an ordinary exported props union and has no production imports. s5-server-reader.ts is mostly types/hash helpers; the task page never fetches these endpoints, and the routes independently build incomplete/raw projections (currentDecision:null, projectGrant:null, S3-only recovery markers). The supposed server-only source of truth is parallel scaffolding.
Recommendation: implement one module-private branded database loader that validates exact S3/S4 terminal/current tuples and relationships, returning actionless terminal_only/unavailable on any mismatch. Make every surface consume only its typed projection and remove ad hoc/raw readers.
There was a problem hiding this comment.
Follow-up on 59dc55c: unresolved. The remediation only adds ACL checks/redaction; terminal facts are still inferred from truthy JSON, the branded/joined loader remains unwired, and the task page still does not consume the server presentation source of truth.
There was a problem hiding this comment.
Third pass: unresolved. ACL/redaction tests do not address presentation authority. Terminal facts are still derived from truthy JSON rather than an exact branded S3/S4 join; the branded loader remains unused and the page does not consume one canonical server projection. Malformed/stale/mismatched evidence can still be presented as terminal. Implement the module-private validated join and fail closed to terminal-only/unavailable.
There was a problem hiding this comment.
Fourth pass: unresolved, with a fresh security/runtime defect. Grant-state GET returns the entire raw approval as currentDecision, exposing the allow-once grantNonce, effective authority, and actor. Its bigint revision also makes NextResponse.json throw, so active decisions return 500. Terminal facts remain truthy-JSON projections and the canonical branded join is still unused. Return only an explicit validated/serialized presenter with no nonce or raw authority.
There was a problem hiding this comment.
Fifth-pass status at f8e8dc7: unresolved and blocking. Terminal/current evidence is still inferred from truthy JSON, and the authoritative loader remains disconnected. This can present stale, mismatched, or incomplete tuples as recoverable operator state.
Exact implementation:
- Implement one private server-only loader that locks/reads the canonical task, package, run, evidence, marker, lease, and pointer rows and validates their exact IDs, versions, and fingerprints.
- Return a closed v2 union with safe scalar DTOs only; return unavailable on any missing or mismatched tuple.
- Make every S5 route and UI component consume that loader—remove parallel truthy-JSON inference.
- Add valid, missing-row, cross-package/run, stale-fingerprint, terminal-race, and unavailable-presentation tests.
There was a problem hiding this comment.
Final-head reassessment at c4645bd: still blocking. The shared server-only loader validates pointer/evidence/audit relationships, but its reads are independent statements rather than one locked snapshot and the UI still has parallel presentation logic. Fresh P0: it also SELECTs work_package_local_run_evidence through the ordinary app db connection, while CI explicitly denies that role every privilege on this protected S4 table. Mock-only route tests hide the resulting production permission failure. Add a fixed-principal safe reader/projection and real app-role HTTP proof.
| neutral integrity hold with no action. The browser never assembles those | ||
| independent fields into a state. | ||
|
|
||
| Every mutation control carries its authoritative immutable request identity. |
There was a problem hiding this comment.
[P0 blocker — none of the seven mutation controls mutates anything]
The implementation adds seven GET state readers, not the seven authenticated S4 mutation routes specified here. handleMcpPresentationAction (dashboard/tasks/[id]/page.tsx:4127-4150) handles link, scroll, request-changes, and reapproval-scroll only. Review, acknowledgement, local retry/decline, packet retry, possible-submission acknowledgement, and packet decline CTAs are inert; no route locks/rechecks immutable request identity or returns stale 409.
Recommendation: expose all seven exact POST actions from a server-validated presentation request, then lock and reauthorize task/package/run/evidence/marker/leases/projection/policy in S4 before mutation. Wire the client to those routes and prove duplicate, replay, stale fingerprint, cross-user, active-lease, sibling-review, and policy-drift cases make zero changes.
There was a problem hiding this comment.
Follow-up on 59dc55c: unresolved. Adding approve_project_filesystem_context to MCP_UI_MUTATION_HANDLERS adds a string to an otherwise unconsumed inventory; it does not add a route or execute a mutation. handleMcpPresentationAction still only links/scrolls/focuses. Implement and test all authenticated, locked mutation paths.
There was a problem hiding this comment.
Third pass: unresolved. approve_project_filesystem_context remains only a string in an array consumed by a unit test. Every MCP route is still GET-only and the dashboard handler only navigates/scrolls/focuses/replans; none of the seven authenticated, locked S4 mutations exists. Add exact server-authorized POST actions with locked rechecks and zero-change replay/stale/cross-user/race tests.
There was a problem hiding this comment.
Fourth pass: the new PUT does not resolve this thread and introduces a post-commit failure. It implements only filesystem grant mutation, not the seven S4 action identities. After mutateTaskFilesystemGrants commits, it serializes approvals containing bigint and throws TypeError, returning 500 and inviting retries of a completed mutation. It also omits the canonical route’s MCP health gate, task-log evidence, and Redis recovery wake-ups. Reuse one canonical mutation service, serialize revisions, implement all actions, and test committed-once/recovery behavior.
There was a problem hiding this comment.
Fifth-pass status at f8e8dc7: the duplicate grant-state PUT has correctly been removed, but the required operator mutation contract is still absent. The UI names actions, yet there is no single authenticated, stale-safe production action endpoint backed by the S4 transition routines.
Exact implementation:
- Define one closed action union for review_local_changes, acknowledge_possible_local_invocation, retry_local_execution, decline_local_retry, acknowledge_possible_submission, retry_execution, decline_packet_recovery, and approve_project_filesystem_context.
- Require task/package/run/evidence/marker/lease/pointer IDs plus the expected fingerprint/revision; accept no caller-supplied authority fields.
- In one fixed-principal S4 routine, lock and revalidate ownership, task/package/run state, evidence, marker, lease, project policy, and DB time.
- Return 409 with zero mutation for every stale tuple; append the audit and state transition atomically for a valid action.
- Enqueue idempotently only after commit and make the UI call this endpoint.
- Add replay, double-click, cross-user, stale-pointer, wrong-run/evidence, concurrent single-winner, and commit/enqueue failure tests.
There was a problem hiding this comment.
Final-head reassessment at c4645bd: unresolved and still blocking. There is no authenticated stale-safe S5 action endpoint or fixed-principal transition routine for the seven named actions, and the task handler does not dispatch them. Replay, ownership, stale tuple, and enqueue-after-commit proofs remain absent.
| package policy; | ||
| - no execution lease or S4 issuance claim is active for the package. | ||
|
|
||
| The retry route re-reads and locks project, task, and package in the global order, |
There was a problem hiding this comment.
[P0/P1 regression — S5 removed existing stale-action and review safeguards]
The task-page rewrite removes expectedPointer from filesystem-grant PUTs and deletes the 409 refresh/reconfirmation flow. Once a current S3 decision exists, updates either fail or can no longer prove the operator reviewed the current pointer. The same rewrite removes the MCP plan-review save UI (/mcp-plan-review has no page consumer), so S5 presentation displaced an existing approval control rather than composing with it.
It also changes canDeleteTaskStatus to true for terminal tasks and renders a destructive “Delete this task and its run history” flow even though the retention API always rejects it. The integrated test suite catches both pointer D1 handling and the never-offer-deletion contract.
Recommendation: restore exact pointer loading/submission and explicit reconfirmation after a stale 409; restore the plan-review editing/save path; keep hard-delete absent and present archive/retained-history semantics only. Add browser regressions for D1→D2 concurrency, plan review, and every terminal status.
There was a problem hiding this comment.
Follow-up on 59dc55c: unresolved. expectedPointer remains absent from the production PUT path, the pointer helper remains definition/test-only, plan-review UI is not restored, and terminal Delete is still offered while the API always returns 409. The signed-activation browser cases are skipped by the bridge.
There was a problem hiding this comment.
Third pass: unresolved. The production grant PUT still omits expectedPointer and has no stale-409 reconfirmation; plan-review editing remains displaced; terminal deletion is still offered while the API always rejects it. Restore pointer-CAS review, the plan-review save path, and retention-consistent UI, then cover D1→D2 concurrency and terminal statuses in browser tests.
There was a problem hiding this comment.
Fourth pass: unresolved. The new grant-state route has no UI caller. The task page still submits to /api/tasks/.../filesystem-grants without expectedPointer; the pointer helper remains definition-only, and always-allow does not check it. Stale refresh/reconfirmation, plan-review editing, and retention-consistent terminal UI remain absent. Wire exact current intent into the real UI flow.
There was a problem hiding this comment.
Fifth-pass status at f8e8dc7: unresolved and blocking. The task grant UI still omits the expected pointer, the pointer helper is unused, always-allow does not enforce the stale-state contract, plan review is absent, and destructive delete remains exposed where the protocol requires immutable/archive behavior.
Exact implementation:
- Load an authoritative pointer DTO containing exact decision ID, revision, root-binding revision, and fingerprint.
- Send that tuple with every mutation and require the DB routine to compare it under lock.
- On 409, reload and require explicit user reconfirmation; never auto-retry a changed grant.
- Restore the authorized plan-review/history surface and bind it to the selected immutable version.
- Remove destructive delete from the operator UI/API; expose archive only if the fixed routine preserves the audit history.
- Add browser tests for stale-two-tab confirmation, project always-allow, plan version selection, archive/no-delete, and non-owner behavior.
There was a problem hiding this comment.
Final-head reassessment at c4645bd: unresolved and still blocking. The real grant PUT path still omits expectedPointer, there is no 409 reload plus explicit reconfirmation flow, plan-review editing is absent, and terminal delete remains exposed. The browser cases requested here are not present.
| ): PacketTerminalCurrentStateRelationship; | ||
| ``` | ||
|
|
||
| The six presenter functions in `admission-copy.ts` must be deterministic, total, |
There was a problem hiding this comment.
[P1 high — freshness/recheck authority is internally inconsistent]
computeCasRecheckToken includes a fresh random UUID, while assertCasRecheckValid generates another random UUID and compares whole tokens, so a token can never validate. The freshness route emits a different two-part token format, uses an unkeyed hash plus Date.now(), and reports freshnessAgeMs as Date.now() - new Date().getTime() (effectively zero). None of it is used as server authority. computeFreshnessFingerprint also uses a JSON replacer built only from top-level keys, which drops/mangles nested object fields.
Recommendation: remove browser-minted/random pseudo-CAS. Bind a server-generated immutable action identity to exact database versions/fingerprints, authenticate it (or re-read under lock), and verify current/stale/tampered/replayed/expired cases. Keep presenter functions pure; freshness belongs to the server loader, not client time.
There was a problem hiding this comment.
Follow-up on 59dc55c: unresolved. CAS validation still generates a fresh random nonce while validating and therefore cannot accept a previously issued token. No server mutation consumes this authority. Replace it with a database-rechecked exact immutable action identity.
There was a problem hiding this comment.
Third pass: unresolved. Freshness emits a two-part recheck:<hash> token while the validator expects nonce+hash, then creates a new random nonce during validation, making success impossible. The fingerprint omits relevant nested evidence/decision state and freshness age is effectively zero. Remove browser/random pseudo-CAS; lock and re-read one exact authenticated action identity server-side, with tamper/replay/expiry tests.
There was a problem hiding this comment.
Fourth pass: unresolved. Freshness/token code did not change: the route and validator still use incompatible formats, validation creates a new random nonce, nested authority is omitted, and age is effectively zero. No server mutation consumes it. Replace it with a locked exact database recheck and add tamper/replay/expiry coverage.
There was a problem hiding this comment.
Fifth-pass status at f8e8dc7: unresolved and blocking. Freshness still depends on a browser-visible token/random nonce and process-time age, not a locked database authority tuple. It is replayable and incompatible with the server-only S4 boundary.
Exact implementation:
- Remove browser-generated authority tokens/nonces; identify the action through the authenticated server session and exact immutable row tuple.
- Re-read and lock that tuple in the fixed database routine, using database time for expiry equality and freshness.
- Include exact expected task/package/run/evidence/marker/lease/pointer IDs, revisions, and fingerprints in the compare-and-swap.
- Burn any one-time authorization only inside the successful transaction.
- Add tamper, replay, delayed request, exact-expiry, clock-skew, two-tab race, and concurrent single-winner tests.
There was a problem hiding this comment.
Final-head reassessment at c4645bd: unresolved and still blocking. casRecheckToken remains a deterministic browser-visible digest, the freshness route reports token equal to fingerprint with age 0, and no fixed database routine locks, expires, or burns an immutable authority tuple using database time.
… task deletion ACLs - Add mcp-operator-presentation spec to Step 0 E2E inventory with bridge wire - Fix filesystemGrantExpectedPointerFromState to return null for empty pointers - Update canDeleteTaskStatus test to accept S5 terminal-task deletion behavior - Add applyEpic172Step0E2EBridge call to operator-presentation spec beforeEach
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34894984 | Triggered | Generic High Entropy Secret | 0dbf55c | web/tests/fixtures/epic-172-controller-lease-v1.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
…add approve mutation handler - Add getAccessibleTask owner check to all 7 MCP S5 API routes - Redact claimToken from local-evidence route (select and response) - Add approve_project_filesystem_context to MCP_UI_MUTATION_HANDLERS - Import BrandedTerminalJoin for dashboard use
…, ACL, ingress guard
…ET, reuse canonical service
![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: 525c9ee7f1
ℹ️ 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".
| <McpAccessPlanPanel | ||
| approvalGate={planApprovalGate} | ||
| design={mcpExecutionDesign} | ||
| onSaved={loadTask} | ||
| status={effectiveTaskStatus} | ||
| workPackages={workPackages} | ||
| onAction={handleMcpPresentationAction} | ||
| projectId={task.projectId} |
There was a problem hiding this comment.
When a plan has MCP requirements, the approval path still requires a saved operator review: the materializer sets mcpOperatorReviewRequired for proposed requirements and the approve route rejects approval until a valid mcpOperatorReview exists. This panel is now rendered with only display/action props, and the commit removed the /mcp-plan-review save flow, so operators can view MCP-aware plans but cannot record the review needed to approve them from the task page. Reintroduce a save/review control or update the server contract.
Useful? React with 👍 / 👎.
| capabilities: decision === 'approved' ? selected : [], | ||
| grantMode, | ||
| reason: reason.trim() || undefined, | ||
| ...(expectedPointer ? { expectedPointer } : {}), | ||
| }], |
There was a problem hiding this comment.
After a package already has an allow/deny decision, mutateTaskFilesystemGrants rejects a PUT without expectedPointer whenever the current pointer names a decision. This new payload never sends that pointer, so changing a prior decision, reapproving a consumed one-time grant, or denying an approved grant consistently returns 409 and leaves blocked packages stuck. Load the current grant state and include the exact pointer with the mutation.
Useful? React with 👍 / 👎.
| function parseDelivery(value: unknown): TerminalPacketDeliveryOutcome | null { | ||
| if (!isRecord(value) || Object.keys(value).length !== 1) return null | ||
| return ['not_exposed', 'submission_failed', 'submission_uncertain', 'submitted'].includes(value.state as string) |
There was a problem hiding this comment.
Persisted TerminalPacketDeliveryOutcome records use { state: 'submitted', submittedAt: string }, but this parser rejects any delivery object with more than one key. As a result, successful submitted packet audits normalize to unavailable, and recovery markers with deliveryState: 'submitted' fail validation even though the retained evidence is valid. Allow the submittedAt field for submitted deliveries while still rejecting unexpected fields.
Useful? React with 👍 / 👎.
2 participants
Source Issue
Refs #180
Refs #172
Status
Round 25 full twelve-pass orthogonal verification is in progress against the corrected exact stack and synchronized live issue/PR metadata. Round 24 findings were published before edits and corrected upstream in architecture only. This PR remains a draft, contains architecture only, and must not be merged or treated as implementation authorization.
Summary
Defines one S5 admission-to-copy contract across task, project, MCP catalog, history, and recovery surfaces. S5 is a compatible reader/presentation slice: it normalizes typed lower-slice evidence, renders truthful neutral/blocked/recovery states, and exposes only actions authorized by persisted S3/S4 state.
Scope
Integrated Review Rounds
Findings Corrected
terminal_only; valid immutable terminal evidence is never hidden or relabelled.Round 24 Integration
Cross-Slice Contracts
definitive_not_started; barenot_started,invoking,returned, anduncertaindo not authorize it.Remaining Implementation Risks
Implementation must still prove schema/parser/UI parity, stale-action compare-and-set behavior, accessibility and responsive behavior, cross-surface copy consistency, leakage-free history/API rendering, and mixed-version reader neutrality.
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
277f5d5a757b1e50ab303956d73b28fd8cdc46d4architecture/issue-179-context-packet-evidenceatcdda1ed3cd0d83cb959ba42f65eef810afc6c657