Implement bounded context issuance and evidence for #179 - #198
Conversation
Architecture review updateCompleted two orthogonal architecture passes for #179. Round 1 findings addressed
Architecture updates are in Round 2 resultNo further architecture findings identified in the reviewed scope. Implementation must still prove one-winning-claim, stale-token fencing, truthful recovery artifacts, and no deadlock using real PostgreSQL race tests. This is an architecture-only PR and does not implement #179. |
Integrated architecture review — round 3 findings (before correction)Verdict: Needs architecture changes. Implementation must not proceed from this revision. Blocker — packet fencing covers
|
Integrated architecture review — round 7 findings before correctionThe fresh 12-axis pass reverified the prior corrections, then found these remaining S4 blockers:
Required tests include both epoch lock orderings, direct/sibling handoff bypass, every claim mode after epoch 2, terminal-known-audit repair, full valid/invalid evidence tuples, and path-bearing exception leakage. This is architecture correction only; no production implementation is included. |
Integrated architecture review — Round 8 findings (before correction)Verdict for this round: blocked. The earlier findings are materially resolved, but the fresh orthogonal passes found four remaining contract gaps in S4.
Required correction: add a closed post-submission failure stage contract and no-auto-resubmit semantics; branch invariant repair by exact terminal outcome; make every all-mode claim and every reconciler contend on task/all siblings in global order; and assign the cutover command/runbook plus executable proofs. Inspected stack head: |
Round 8 addendum — additional findings before correctionThe final independent state-machine pass found one additional High / blocking issue and two consistency advisories. No correction has been applied yet.
The all-mode claim correction will also lock all sibling packages, recompute eligibility under lock, and prove no sibling is running or leased. This preserves Forge's current sequential specialist model, not just task-status consistency. |
Round 8 addendum — ADR 0008 canonical vocabulary driftHigh / blocking: ADR 0008 still says every packet decision audits Required correction: make ADR 0008's evidence summary explicitly defer detailed lifecycle/schema to ADR 0009 and state only the privacy-safe vocabulary: opaque |
Integrated architecture review — Round 9 finding (before correction)Medium / blocking-by-consistency: Round 8 correctly made every recovery reader/action join and validate the exact prior packet artifact, but the route-specific locking prose still stops at the prior runtime audit. The S3 one-time resolver prose has the same omission. An implementation following those numbered locks could validate artifact state outside the transaction even though the normative marker contract says artifact equality is authoritative. Required correction: lock/read the exact Inspected final-stack head before restack: |
Integrated architecture review — Round 9 findings (before correction)Verdict remains blocked on the following fresh post-submission/operability findings.
Required tests: expiry before/between/after atomic file replacements; crash between rename and ledger outcome; fence acquisition in both orders; pre-transaction completion-preparation versus finalizer rollback; bounded alert deduplication and unauthorized repair rejection. Inspected stack head: |
Round 9 addendum — sibling review barrier, complete order, and hold authorityThree further blocking-by-contract findings were found before correction:
Required races: claim/recovery versus an |
Integrated architecture review — Round 10 finding (before correction)Medium / blocking: the canonical order omits rows newly introduced by Round 9: host-apply ledger/entries and integrity alert/resolution rows. Place them explicitly between runtime audits and review gates. Every per-stage/per-file ledger mutation and every alert/repair mutation uses the applicable prefix/tail; unlocked discovery is allowed, but mutation reacquires the order. A quiescence-timeout alert may be inserted without owning the host fence because it changes no execution state, but it must use a short top-down transaction and never wait for the fence while holding database locks. Add observed-lock races for per-file ledger updates/finalization/recovery and alert/repair/finalizer paths. |
Integrated architecture review — Round 10 additional blocking findingSeverity: High Host-tree exclusion is not closed over project management routesThe proposed post-submission fence is keyed only by project ID and is shared by worker/recovery. Current project management routes can repoint Evidence inspected:
Required correction before the next review:
This finding is based on the current implementation's project PUT/DELETE behavior; it is not proof that other filesystem-management paths do not exist. |
Integrated architecture review — Round 10 consolidated addendumThe independent security, QA, and state-machine passes found five additional blocking contract gaps after the top-level order correction. 1. High — Same-root, cross-host, and descendant quiescence are not fenced
2. High — S3/S4 review aggregation is contradictoryThe S4 resolver called from S3 must participate in the task-wide 3. Medium — Individual mutating paths still omit the complete tailThe acknowledgement/retry route, one-time resolver, invariant repair, success repair, integrity resolution, and gate decision must each explicitly acquire applicable rows in the canonical suffix: host ledger/entries → all artifacts → recovery actions → integrity alerts/resolutions → review gates. The route cannot validate host-review or integrity state from unlocked rows. 4. Medium — Activation cannot prove host capability/routingThe planned activation command promises to report every worker/fence/same-host blocker, but no durable capability evidence exists. Keep the initial v2 local-effect rollout explicitly single-active-host. Add a durable worker-host capability registration/heartbeat with stable opaque host ID, supported protocol version, fence-supervisor capability, last-seen time, and drain state. Activation fails unless exactly one fresh active host is registered and every legacy/incompatible worker is drained; its audit snapshots the registrations. Multi-host local effects remain disabled until a later host-affine routing design. 5. Medium — Terminal/effect/ledger and mismatch outcomes are underspecified
These are architecture gaps, not claims that the future implementation or host behavior has been proven. |
Integrated architecture review — Round 11 findingsDisposition: Changes requested before architecture readiness Medium — Wrong-host predicate references a field absent from
|
Integrated architecture review — Round 11 consolidated lifecycle findingsDisposition: Changes requested; all findings block architecture readiness 1. High — Hard project deletion destroys immutable evidenceCurrent foreign keys cascade project deletion through tasks, packages, runs, and artifacts. The proposed root cleanup still ends by deleting the project, contradicting permanent packet evidence, integrity alerts, and quarantine resolution retention. Required correction: protocol-v2 projects use a soft-delete tombstone. Final deletion clears the path and releases the live root binding but retains project/task/package/run/audit/artifact/action/alert/resolution rows. The unique root constraint applies only to non-deleted projects. Normal queries hide tombstones; hard purge is forbidden until a separate retention/export architecture exists. 2. High — A nonexistent clone destination has no physical identity to fenceGitHub/local creation can clone or create a directory before a project row or physical root exists. Two requests can pass Required correction: add a typed pre-create reservation keyed by authoritative host + canonical existing parent identity + platform-normalized missing suffix. Acquire its namespace fence before mkdir/clone, persist 3. High — Worker registration is checked only at activationAn epoch-2 claimant can currently supply protocol/host/supervisor settings without proving a fresh active 4. High — Inherited file descriptors do not prove ACP descendant quiescenceA child can close extra descriptors or daemonize, and supervisor-first death can release the lock while the main worker continues. Replace the inherited-descriptor claim with a host fence service plus an OS-enforced containment adapter that includes the Forge worker, ACP transport, validation children, and every descendant. The service owns the resource lock and durable local lease; worker/service/control loss makes it orphaned and actionless. Automated release requires the containment adapter to prove the group empty. Unsupported or unverifiable hosts fail closed for protocol-v2 local-root execution; no process-memory/process-tree guess is accepted. 5. High — Binding-key and old root writers are not fenced by cutoverEvery worker/epoch/root binding must carry one stable host-binding-key fingerprint; divergent key material on the same host blocks activation/claim. Define backup and rotation as drain → disable → rebind, never silent key replacement. Add an expand-phase project-root mutation trigger serialized with the epoch. At epoch 1, legacy create/repoint clears/invalidate nullable bindings and forces explicit rebind; at epoch 2 it rejects missing/malformed host ID, resource ref, revision, maintenance token, or authorized writer settings. Activation's fresh snapshot plus exclusive epoch lock then durably excludes stale web/root-management writers, not only workers. These corrections must preserve the rule that no database lock is held while waiting for a namespace/resource/containment fence. |
Integrated architecture review — Round 11 evidence-bypass findingSeverity: High Unconfined ACP work can bypass the host-review gateThe architecture correctly says prompt text cannot stop an Agent Communication Protocol (ACP) runtime from making equivalent shell or filesystem changes. However, the normative table treats Required correction: capture a repository baseline before ACP submission and a post-quiescence change fingerprint under the same resource fence. Detected or unverifiable external-runtime changes require fingerprint-bound working-tree review before acknowledgement, retry, reapproval, root management, or a new run. Apply this rule to provider success as well as failure. If that evidence cannot be produced, protocol-v2 packet guarantees must exclude unconfined ACP execution. Privileged quarantine can bypass a sibling's mandatory host review
Required correction: permanent quarantine must either bind and acknowledge every affected sibling's exact repository-change and host-ledger fingerprints, or append a separate privileged repository-abandonment acknowledgement. An unresolved review/marker remains a project-root management barrier even after task terminalization until that explicit evidence-bound action exists. Normal quarantine must never turn unknown physical work into permission to repoint, delete, or reuse the root. These are evidence-state findings. They do not assert that a future containment or fingerprint implementation is correct. |
Integrated architecture review — Round 12 findings (before correction)High — Terminal success still permits unresolved external repository changesThe new lifecycle correctly says a changed or unverifiable post-ACP repository comparison stops all Forge local stages and terminalizes with That tuple is contradictory and actionless: success creates no recovery marker, so an unreviewed successful row has no valid acknowledgement path; a later review also cannot retroactively turn the stopped failed run into an original success. Required correction: terminal success requires baseline comparison Low — The tombstone duplicates Forge's existing project lifecycle fieldThe proposal adds Required correction: use existing Both findings are architecture-contract issues, not implementation findings. |
Integrated architecture review — Round 12 additional findings (before correction)High — Binding-key rotation is circular under the epoch and root triggersThe current sequence installs K2, recomputes/rebinds every root, then reactivates. Epoch-2 root mutation accepts only the exact active epoch/instance key, while activation requires every live project already to be bound to that active key. Starting from epoch K1/projects K1, a K2 writer cannot rebind. Promoting the epoch to K2 first also fails activation because the projects still carry K1. Required correction: define a privileged two-phase rotation state with High — Pre-create reservations bypass the exact root-writer instance contractReservation rows carry host/key/token/state data but no exact root-writer instance or credential-generation pin. The root-mutation trigger covers project rows, not reservation planning/materialization/cleanup. The stated transaction order locks only the reservation row, so it cannot prove the epoch and exact fresh writer instance that authorizes Required correction: pin each reservation transition to the exact root-writer instance and credential generation. After acquiring the namespace fence with zero database locks, every reservation planning/materialization/cleanup/bind transaction must lock in the canonical order These are architecture-contract findings, not implementation findings. |
Integrated architecture review — Round 12 state-machine finding (before correction)High — Successful runs have two contradictory effect-intent tuplesThe normative compatibility table permits Required correction: split success into two disjoint exhaustive tuples. A successful run with no response-driven local stage remains This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 additional findings (before correction)High — Same-host recovery has no authenticated current worker identityThe durable pins name the historical claiming worker instance, and stale recovery locks only that exact row. The same section says a missing or stale registration is alert-only. After worker W1 crashes and its row becomes stale, a fresh same-host worker W2 is therefore neither named, locked, freshness-validated, nor recorded: requiring W1 freshness makes recovery impossible, while allowing W2 without a durable pin makes it unauthenticated. Required correction: retain W1 as immutable claim history and add an exact Medium — Repository-change abandonment has two incompatible state models
Required correction: keep Low — The repository-review barrier blocks its own acknowledgementThe lifecycle says an acknowledgement cannot proceed until repository review is already reviewed/abandoned, while the recovery-action section says that same exact acknowledgement changes Required correction: all later authority, retry, reapproval, root management, and unrelated actions remain blocked, but the one fingerprint-bound acknowledgement that atomically completes the review is the explicit exception. These are architecture-contract findings, not implementation findings. |
Integrated architecture review — Round 12 containment finding (before correction)High — The normal containment lifecycle cannot become emptyThe current contract places the Forge worker itself plus ACP, validation, and response-driven descendants in one non-escapable lease group, then requires a normal owner to wait until that complete group is empty before releasing the resource fence. Forge's worker is a long-lived queue loop. While that durable worker remains a member, the group cannot become empty, so a successful run cannot release its fence without terminating the queue worker. Required correction: keep the durable queue/control worker outside the containment group. It asks the trusted fence service to create a per-run execution child; that child and every ACP/validation/local-effect descendant enter the non-escapable run group before repository access. Specify the authenticated handoff, owner token, normal child exit/group-empty proof, durable lease release, and crash/orphan takeover protocol. The queue worker may observe/control the run but is not evidence of group emptiness. This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 sibling-claim finding (before correction)High — Unresolved local-change review is not enforced at the all-mode claim boundaryThe lifecycle says a changed/unverifiable repository or uncertain host write blocks every later run. The shared all-mode sibling claim currently rejects only Required correction: materialize one task/project unresolved-local-change barrier derived from the exact sibling host/repository review fingerprints, or make every all-mode claim lock and validate all sibling audits/ledgers/reviews in canonical order. Task status must not become claimable while any exact review is unresolved. Acknowledgement/quarantine clears only the matching fingerprint-bound barrier; it never rewrites immutable evidence. This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 root-exclusion finding (before correction)High — Exact root identities do not exclude ancestor/descendant repositoriesLive-root uniqueness and missing-root reservation keys reject only equal physical roots. Required correction: define a prefix-aware hierarchical namespace reservation/fence protocol and a durable no-ancestor/no-descendant constraint for every live root and reservation on one host. Acquisition uses a canonical hierarchy order without database-lock/fence inversion. Create, repoint, cleanup, tombstone, and reuse prove that no conflicting ancestor or descendant binding/reservation exists; recursive cleanup additionally proves the protected subtree contains none. This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 evidence-scanner finding (before correction)Medium — Repository comparison has no bounded hostile-filesystem contractThe design requires a complete content/metadata baseline but does not define Required correction: define a versioned bounded scanner. It never follows symlinks, reads content only from regular files, records bounded metadata for links/special entries, excludes This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 mixed-version finding (before correction)Medium — The epoch-1 root trigger cannot invoke S3's sole TypeScript reconcilerThe proposed database trigger cannot call the TypeScript S3 reconciler, and locking task/package/approval rows after it has acquired project -> epoch reverses the documented order. Required correction: make the rollout executable. Prefer no live old-web/root-writer window with the root trigger: disable project ingress, revoke and terminate v1 database access, drain old services, run S3 reconciliation/root backfill under the canonical application lock order, then enable the root trigger and epoch 2 before v2 ingress. The trigger validates protocol-v2 root mutation only and never reimplements S3. If a mixed writer bridge is chosen, the application must reconcile before project mutation and the trigger validates an exact proof without acquiring downstream rows. This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 recovery-action finding (before correction)High —
|
Integrated architecture review — Round 12 fence-service trust finding (before correction)High — The fence service is not protected from the explicitly unconfined ACP runtimeThe host fence service owns the only durable lease/release authority, while ACP retains shell, network, credential, and filesystem access. The contract does not give the service a separate operating-system principal, protected state/socket, peer authentication, or a run-bound lease capability. An ACP process with the worker's host authority could call or tamper with the service, replay another run's release, delete state, or combine state damage with service termination. Required correction: run the service under a protected principal with a protected state directory and socket/API. Validate kernel peer credentials and an unguessable capability bound to run, current worker instance, root identity, and containment group. Independently prove kernel group emptiness on every release; never trust a caller's declaration. Tamper, corruption, replay, service death, or unverifiable peer/group state becomes durable orphaned/disabled state and blocks root reuse. This is an architecture-contract finding, not an implementation finding. |
Integrated architecture review — Round 12 tombstone-state finding (before correction)High — Tombstoning retains nonterminal work that can execute after project removalThe tombstone retains tasks/packages but does not close ordinary Required correction: the tombstone transaction atomically closes/cancels every nonterminal task and package with one bounded project-removed audit reason, while retaining all immutable history/evidence. Queue discovery, direct progression, sibling reconciliation, and every all-mode claim also reject This is an architecture-contract finding, not an implementation finding. |
docs: document combined cross-sink production proof
…ion-proof test: prove production cross-sink sentinel closure
![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: 90763b2714
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90763b2714
ℹ️ 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".
…covery fix: recover timestamp-only legacy claims
…isher-reuse fix: reuse the production task event publisher
…d-answers fix: make protected answer batches atomic
…n-inventory fix: certify protected clarification S4 objects
…ation fix: preserve legacy Architect clarification storage
06a1442
into
architecture/issue-178-filesystem-grant-recovery
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1c99d6cd5
ℹ️ 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".
| // supplied runtime environment. The request flags are intentionally unable | ||
| // to open this boundary until an OS-enforced confined writer exists. | ||
| void env | ||
| return false |
There was a problem hiding this comment.
For every approved package, this now returns false regardless of FORGE_WORK_PACKAGE_EXECUTION, so the branch at handoffApprovedWorkPackages can never call executeReadyWorkPackage; Forge records a no-op handoff and may complete the task without running a specialist or applying its files. This removes the repository's default execution path and also makes an explicit FORGE_WORK_PACKAGE_EXECUTION=1 ineffective; preserve the existing flag semantics rather than hard-disabling the path.
AGENTS.md reference: AGENTS.md:L30-L35
Useful? React with 👍 / 👎.
| const response = await fetch(`/api/tasks/${taskId}/architect-plan-history/${planVersion}`) | ||
| if (!response.ok) throw new Error('Protected clarification history is unavailable') | ||
| const body = await response.json() as { entries?: Array<{ entryId: string; entryKind: string; content: string }> } | ||
| setClarificationQuestions(clarificationQuestionsFromHistory(body.entries ?? [], summaries)) |
There was a problem hiding this comment.
For protected plans, this is the only browser request that receives the authorized Architect entries, but it retains only clarification entries. The Implementation Plan and MCP review panel still read the generic artifact projection, whose Architect content is the fixed placeholder and whose metadata omits mcpExecutionDesign; operators therefore cannot inspect the generated plan, and a plan with MCP requirements cannot render or save its mandatory review even though the approval API rejects approval without it. Store and project the plan body and MCP requirement entries returned here into those views.
AGENTS.md reference: AGENTS.md:L30-L33
Useful? React with 👍 / 👎.
| v_next_marker := NULL; | ||
| IF p_action = 'acknowledge_possible_submission' THEN | ||
| v_next_marker := v_marker || pg_catalog.jsonb_ |
There was a problem hiding this comment.
On an upgrade with a task already in awaiting_answers, this changes every pre-S4 question to legacy_unavailable without changing or requeueing the task. The questions API accepts only protected rows whose status is open, while ordinary task retry rejects awaiting_answers, leaving the page requesting answers that cannot be displayed or submitted; the operator's only workaround is to stop and restart the task. Transition affected tasks to a recoverable state during migration or preserve a usable protected clarification source.
AGENTS.md reference: AGENTS.md:L30-L33
Useful? React with 👍 / 👎.
| SET provider_type_used = provider.provider_type, | ||
| provider_is_local_used = provider.is_local, | ||
| provider_config_updated_at_used = provider.updated_at, | ||
| acp_execution_mode = CASE WHEN provider.provider_type = 'acp' | ||
| THEN 'unconfined_host_process' ELSE 'not_applicable' END |
There was a problem hiding this comment.
When a provider configuration has been edited since an old run, this backfill records the configuration's current type, locality, update time, and derived ACP mode as values used by that historical run. Because the provider API permits changing both providerType and isLocal, an old cloud run can consequently be exposed as a local ACP unconfined-host run, or vice versa, corrupting the new execution evidence. Preserve an explicit unknown legacy state or populate these fields only from genuine run-time history.
Useful? React with 👍 / 👎.
1 participant
Status
PR #198 remains OPEN and DRAFT at exact head
b1c99d6cd5cc3906dc453fd7f6e1df8d58036534, with tree1eb934ef85754a1408028277d9f3433d57d16d57. Its base isarchitecture/issue-178-filesystem-grant-recoveryat7d0325334709dc11a3d8fcb6fbcf59c91e12be30.The five final clarification/recovery children are merged into this integrated head:
Issues/PRs #300, #301, and #302, plus the earlier stacked remediation children, remain completed as already documented. The final Reviewer verdict is APPROVED for the exact tree, with no blockers or advisories in the inspected scope. This verdict and hosted disposable-service evidence are not proof of correctness or a production deployment.
Final exact-head evidence is green: Web
30564270595/ job90944729950(1,754/1,754 units, PostgreSQL 16/16, Redis 3/3, Redis ACL 3/3, combined proof 1/1, S3 16/16, disabled ingress 1/1, build, and E2E 17 passed/59 expected skips); Contract30564270498/ job90944730134; GitGuardian green. All13/13review threads are resolved;0remain outstanding.Implemented scope and evidence
This stack protects Architect plan history, MCP review history, package and capability evidence, Redis/SSE projections, session-cache writes, clarification projections, recovery and archive paths, root-reference migration proofs, and staged PostgreSQL/Redis/ACL evidence.
Specialist/ACP execution and host-repository writes are disabled by default behind reversible, explicit, auditable controls. Current unavailable paths fail closed where the required confinement boundary is unavailable; the interfaces and capability controls remain extensible for future confined workers and trusted deployment modes.
The exact integrated head has green Web/Contract/GitGuardian evidence for the reviewed stack and combined proof. This body records scoped disposable-service release evidence and does not claim proof of correctness, a production deployment, or arbitrary future sinks.
Closure ledger
Must fix before ready
Accepted residual risks
Follow-up capability/hardening
S4_CROSS_SINK_PRODUCTION_SENTINEL_OK.Scope freeze
No new adjacent hardening becomes a merge blocker unless it violates an existing acceptance criterion or is a credible high-impact failure in the current supported deployment. Security hardening should enable future capability, not permanently remove it.