Repository navigation
fix(risk-fork): bind historical cleanup recovery - #491
Conversation
jmborden1112-beep
left a comment
jmborden1112-beep
left a comment
There was a problem hiding this comment.
LGTM
Needs Fixing
None found for refreshed head 672356633d8dfafeee36ff70d2fb020b6d16330a, tree a0b7c0cf8a2fa53968c5bb1530a5031449db244d.
Requires Human Review
None for authorized source merging. Independent operational provenance and production gates remain separate.
Recommended Optional
None.
Create Follow-up Issue
Existing #375/#301/#302/#303 retain production requirements.
Historical cleanup provenance remains separate from current qualification; private expiry latches and exact durable bindings prevent rollback and substitution. Journals precede bound cleanup; absence ambiguity never becomes success or new-effect authority. Full changed source/callers and regression coverage were reviewed. Refreshed regressions41/42 pass with one Windows POSIX skip, source integrity56637e76 passes, diff is clean. Every applicable exact-head CI check is required before merge.
Workflow Trace: real inherited Codex source/persistence and operations/test review, root final signed tree verification and canonical regeneration. Authorized second owner-controlled account approval, not independent operational certification. No provider call, spend, publication, deployment or activation; no protection bypass.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| test('source-disabled provider obligations stay unresolved while local-only recovery needs no SDK', async (t) => { | ||
| const dirs = await directories(t); | ||
| const { journal, record } = await intent(dirs); |
![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: 672356633d
ℹ️ 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".
| CLEANUP_OBSERVATION_VERIFIERS.set(verifier, (value, observation) => ( | ||
| verifyObservation(value, observation, assertExternalObservationHistorical) | ||
| )); |
There was a problem hiding this comment.
The cleanup verifier still calls assertExternalObservationHistorical, which compares the signed receipt's issued_at with the current wall clock. If an adapter accepted the receipt while it was current, created a journaled resource, and the clock subsequently moves to before issued_at, destroySavepoint, destroyFork, and reconcilePendingCleanup all fail with future-issued instead of releasing the resource. Preserve the construction-time temporal validation for cleanup or otherwise avoid reapplying this current-time check to already accepted provenance, and add a regression that rolls the clock back to before issuance before cleanup.
Useful? React with 👍 / 👎.
| The E2B adapter can recover already-journaled local export obligations after an | ||
| authentic observation receipt expires. Its cleanup-only provenance helper | ||
| (`verifyE2BCleanupQualificationProvenance`, reachable through the existing | ||
| `e2b-qualification` module subpath) preserves canonical evidence reconstruction, |
There was a problem hiding this comment.
This adds a public focused-subpath helper and a new cleanup-only adapter state, but the package-facing risk-fork/README.md still presents its qualification import at lines 289-309 without this helper and still says at line 378 that the adapter has only four states. Keeping the new behavior solely in the managed-service operational document leaves package consumers with a stale API/state contract; update the Risk Fork README's import and state sections alongside this integration change.
AGENTS.md reference: AGENTS.md:L62-L62
Useful? React with 👍 / 👎.
| assertTarget(await this.cleanupJournal.get(recordId)); | ||
| // Persist the exact cleanup obligation before the destructive effect. | ||
| await this.cleanupJournal.markSandboxCleanupRequested(recordId, sandboxId); | ||
| await Sandbox.kill(sandboxId); |
There was a problem hiding this comment.
The pinned E2B 2.39.0 contract returns false from Sandbox.kill(id) when the sandbox was not found, but this result is discarded and #killBoundSandbox unconditionally returns observed. If the sandbox disappears after the preceding getInfo succeeds but before this call, destroyFork records kill_observed and emits destroy_requested_observed even though no kill occurred. Check the boolean result and route false through #verifySandboxAbsent, as the earlier not-found branch does, with a regression covering this race.
Useful? React with 👍 / 👎.
jmborden1112-beep
left a comment
jmborden1112-beep
left a comment
There was a problem hiding this comment.
LGTM
Needs Fixing
None found at head a19f68d, tree1128af559075c55297d64777c3f5be877ee12ecc.
Requires Human Review
No additional source-merge approval is needed. Independent operational qualification remains separate.
Recommended Optional
None.
Create Follow-up Issue
Existing #375/#301/#302/#303 retain operational requirements.
Independent inherited Codex review fully inspected changed source/callers and confirmed constructor capture, mock-only provider override, qualified override rejection and immutable cleanup binding. Permanent constructor/qualification tests27/27 passed; cleanup24 passed with one Windows POSIX skip. The fake identity fix retains pre-kill checks and rejects uncertain destructive authority. Canonical source integrity d46d7f80 and final staged diff passed. All applicable exact-head hosted checks must pass before merge.
Workflow Trace: actual inherited Codex agents and root verification; fallback from requested review profiles was disclosed. This is an owner-controlled second-account source approval, not independent third-party operational certification. No provider calls, spend, publication, deployment or activation; no protection bypass.
Existing cleanup obligations remain recoverable after signed E2B qualification expires. Historical provenance is captured privately, expiry latches against clock rollback, and recovery checks persisted provider/template/export/sandbox bindings. Cleanup intent precedes a bound kill; uncertain or mismatched absence remains unresolved.
The full demo exposed a constructor identity mismatch: the fake adapter changed its public ID after immutable cleanup-policy capture. Its fixture ID now enters the constructor. Overrides are restricted to injected offline conformance fixtures; normal and qualified adapters reject them. Permanent regressions cover this restriction and qualification-evidence rejection. The uncertain pre-kill lookup test now correctly requires no destructive call. Removed CodeQL #953's unused import.
Refreshed against main da10cf9. Candidate a19f68d, tree1128af559075c55297d64777c3f5be877ee12ecc,13 intended paths. Canonical source integrity d46d7f80e892d8fd7e3c1670f0561c70e466d84d3a60829993768dc8d6bb3d1f.
Validation:52 constructor/qualification/historical-cleanup regressions,51 passed,one expected Windows POSIX skip; fake-E2B stale/cross-job/ambiguity8/8; offline kit/runtime verifier passed in keyless network-disabled Node24 Docker; canonical bundle/source verification and clean diff. Fresh exact-head Linux demo, PostgreSQL and artifact CI are required before merge.
Source only. Historical provenance grants no new effects. Provider calls remain source-disabled. Issues #375,#301,#302,#303 retain independent operational, production qualification, publication, deployment and activation requirements.