Repository navigation
feat(deep-scan): resume from accepted worker results - #907
mldangelo-oai wants to merge 294 commits into
Conversation
mldangelo-oai
changed the title
![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: 812abc28dc
ℹ️ 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".
| for path, digest in references: | ||
| if path is not None and digest is not None: | ||
| safe_path = deep_scan_path(scan, path, "Claimed reducer input", kind="file") | ||
| if hashlib.sha256(Path(safe_path).read_bytes()).hexdigest() != digest: |
There was a problem hiding this comment.
When a repository-induced background writer can modify the writable scan directory, this after-the-fact digest check does not prove which bytes the reducer and its artifact-validation tool consumed. The writer can temporarily replace a claimed checkpoint with a schema-valid discovery containing injected findings, leave it in place through reducer validation, and restore the accepted bytes before this line; the digest then matches and the injected aggregate can be selected. Validate reducer sources from host-bound accepted bytes, or otherwise make those inputs immutable before dispatch, rather than relying on a post-execution path hash.
AGENTS.md reference: AGENTS.md:L11-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9522bea563
ℹ️ 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".
| if (event["type"] === "compacted") { | ||
| // Later counter epochs cannot account for missing earlier scan receipts. | ||
| if ( | ||
| session.responseEpochUsageObserved && | ||
| session.expectedResponseTokens > session.responseTokens |
There was a problem hiding this comment.
When a token-counter-only rollout emits compacted, Codex can restart its cumulative counter from a smaller value, but this branch resets only response bookkeeping and leaves previousUsage intact. Subsequent counters are therefore rejected as regressions at readSessionEvent, so live cost tracking omits the new epoch until it exceeds the pre-compaction total and can let a scan continue past --max-cost; reset the counter baseline here while retaining the already accumulated usage. This is the separate SDK live-tracking path, so correcting the persisted Python usage collector does not fix it.
AGENTS.md reference: sdk/typescript/AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed the changes since the previous reviewed head in two passes, using exact source at 9522bea56333 and prior review evidence. Unchanged portions of the large branch were not exhaustively re-audited.
Approved with these considerations still open:
- P1: Selected publication records canonical digests before custom validation rewrites scope, findings and coverage. Completion then rejects the changed projection, even after successful validation. Bind the validated projection through the authorized publication flow; retain digest verification. sdk/typescript/src/api.ts:2395
- P2: CLI resume restores knowledge before artifact-only selected recovery, so a missing/corrupt snapshot blocks publishing already selected output. Defer restoration until model or validation work is actually required. sdk/typescript/src/cli.ts:2314
Validation: CI-helper TypeScript compilation and plugin bundling passed locally; native binaries came from CI with matching compiled source inputs. The full cross-platform suite was not run locally; Windows execution remains unverified here.
CI consideration: 5 latest observed checks are pending or have not passed, including windows-latest / node-22 / tests-2, windows-latest / node-22 / tests-5, windows-latest / node-22, windows-latest / node-24. See the checks; this approval does not resolve those results.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: 052c9b7a9b
ℹ️ 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".
| ]), | ||
| "Validated and indexed the completed Codex Security scan.", | ||
| result, | ||
| `Validated and indexed the completed Codex Security scan. Canonical coverage: ${coverage.completeness}; ${coverage.surfaceCount} surfaces; ${coverage.deferredCount} deferred items; ${coverage.explicitExclusionCount} explicit exclusions.`, |
There was a problem hiding this comment.
Every successful complete_codex_security_scan call now replaces the established fixed text response with coverage-dependent prose. This is an unrelated observable output-contract change for MCP clients that display or snapshot tool text; keep the original summary and expose the new coverage values only through structuredContent.
AGENTS.md reference: AGENTS.md:L41-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cc4842ad0
ℹ️ 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".
| const committed = await this.replayStoreMutation( | ||
| "dedup_commit_replay", | ||
| reducerId, | ||
| async () => await this.options.store.commitDedup(commit), |
There was a problem hiding this comment.
When a repository-controlled background writer can modify the worker output directory, it can replace result.json, checkpoint-head.json, and the referenced checkpoint after validateReducerArtifacts() returns but before this commit. commit_deep_scan_dedup then snapshots the replacement as the accepted result, while acceptedResult still refers to the earlier validated document; finalization later publishes the replacement through acceptedPath without applying the reducer conservation checks to it. Pass the validated digest or bytes into the commit and require the persisted checkpoint to match them.
AGENTS.md reference: AGENTS.md:L6-L8
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the full PR diff and touched production and test files at 9c0f060 against 51d3a4d. No blocking issue identified in this revision.
The selected-result recovery path now handles an unavailable original rollout. I am not retaining the artifact-swap race comments: the worker permission profile and awaited artifact writes do not establish a permitted concurrent writer for the claimed race.
Validation was static, including the material tests and recovery paths. I did not run the test suites.
Allow usage-attribution reads in the failure-recording test while preserving its error and saved-state assertions.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: c27c3c57e5
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| digest = hashlib.sha256(contents).hexdigest() | ||
| destination = directory / f"{digest}.json" | ||
| if destination.exists(): | ||
| raise SystemExit("An existing worker checkpoint does not match its accepted content.") | ||
| write_scan_local_bytes(scan_dir, destination.relative_to(scan_dir).as_posix(), contents) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Required conditions: an SDK Deep scan of a malicious repository must induce the parent scan turn, which can write its scan workspace, to leave a writer that replaces a reducer result after TypeScript validation. The new fallback then hashes and copies those replacement bytes into the accepted checkpoint, so the stored digest authenticates attacker-controlled content rather than the validated reduction; selected finalization later parses and publishes it. Fresh evidence beyond the prior symlink report is that an ordinary-file replacement still passes the new no-follow read. Pass the validator’s digest/bytes into the commit and reject any mismatch.
SECURITY.md reference: SECURITY.md:L11-L18
Dismiss this finding: Reply with @codex security dismiss <reason> [context]. Codex will resolve this conversation automatically; GitHub may require a page refresh to show the result.
Valid reasons: false-positive, duplicate, out-of-scope, compensating-control, risk-accepted, or other. Example: @codex security dismiss duplicate Already flagged by another review
What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context required
Useful? React with 👍 / 👎.
This branch has not been deployed
4 participants
Summary
Resume interrupted Deep Scans from accepted worker results without losing their settings, evidence, or attributable costs. Keep incomplete drafts recoverable while final writeups or receipts are still pending.
Changes
scan-filesmodule, and retain isolated native-policy build coverage.Testing
All five portable plugin checks and the bundled plugin build passed at
c27c3c57e5ca, along with the helper-help and legacy-context upgrade cases. The preceding runtime follow-up passed SDK/MCP type and format checks and focused Python and real SDK cases for repeated preparation after concurrent completion, unchanged sealed artifacts, tampered accepted sources, and strict admission without accepted-source authority.Earlier integration checks covered historical migrations and receipts, usage ownership and completion turns, durable recovery, accepted discovery normalization, both legacy workflow names, worker settings, and hosted cancellation receipts. Full SDK qualification with seed
12345passed on the immutable98ca2a34ec84baseline; the default-order SDK run has a compressed-session-log failure under investigation, and the remaining SDK/MCP qualification is not yet complete. The subsequent follow-ups change three Python runtime files and regression tests. Their focused checks and independent source, dependency, and bundle reviews supplement the baseline results; these results do not claim a full suite ran against the final bundle.Risk and rollout
Existing commands, settings, diagnostics, credential/configuration protections, and artifact-integrity checks remain in place. Existing feature-review threads still require separate disposition before merge. In particular, replay of a legacy accepted reducer can still reject an absent optional writeup; source comparison established that this behavior predates the main integration.
Public disclosure review
The repair commits and their fixtures have been reviewed. Maintainers should still confirm the provenance of previously imported example and rendering-fixture material and review historical discussion artifacts. The corresponding attestations remain unchecked; this update does not establish historical erasure or approved provenance.