fix(multiscan): authenticate resumed scans and preserve outcomes - #466
fix(multiscan): authenticate resumed scans and preserve outcomes#466mldangelo-oai wants to merge 9 commits into
Conversation
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 0afea57ee1
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ 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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review Please review the current head, |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61658ebce
ℹ️ 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 { manifest, coverage } = await loadContract(path, { | ||
| pluginRoot, | ||
| signal, | ||
| }); |
There was a problem hiding this comment.
When an imported or replaced bundle changes only report.md, hasArtifacts still accepts it and this call validates only the canonical JSON documents and their sealed artifacts; report.md is merely checked for existence and is not part of the manifest seal. The campaign therefore skips the scan while preserving an attacker-controlled or stale human-facing report even though the plugin defines that report as a deterministic projection of the validated documents; regenerate the projection or compare it before accepting the resume.
AGENTS.md reference: sdk/typescript/AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
|
@codex review Please review the current head, |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df25b47d26
ℹ️ 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".
| receipt.warning ?? | ||
| `Scan coverage is ${coverage}; results may be incomplete.`, | ||
| }); | ||
| await restoreReport(canonicalArtifactOutput, schemaPluginRoot); |
There was a problem hiding this comment.
When a resumed manifest includes report.md among its sealed artifacts, loadContract has already authenticated that report and the Python helper will immediately return without writing anything. Calling restoreReport unconditionally still resolves and probes a Python interpreter first, so a valid completed campaign becomes impossible to resume if Python was removed or is temporarily misconfigured; inspect the authenticated artifact list and invoke recovery only for an unsealed report.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
|
@codex review Please review the current head, |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b270e7d3e
ℹ️ 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 sameFile = await sameCheckedFileDevice( | ||
| file, | ||
| report, | ||
| await file.stat(), | ||
| ); | ||
| throwIfAborted(signal); | ||
| if (sameFile) return true; |
There was a problem hiding this comment.
When an unsealed report.md is a hard link to any sealed artifact, such as findings.json, inode identity makes this return true; runCampaign therefore skips report recovery and resumes with JSON or another artifact masquerading as the human-facing report. The Python --report-only path has the same hard-link exemption, even though its atomic os.replace writer can safely replace the alias without modifying the sealed inode. The newly added hard-link acceptance is fresh evidence for the previously raised report-validation issue; only treat an actual sealed report.md artifact as reusable and regenerate all aliases.
AGENTS.md reference: sdk/typescript/AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ 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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Draft feedback: narrow the authenticated-resume change before adding unrelated result and warning behavior.
| cost?: ScanCost; | ||
| error?: string; | ||
| warning?: string; | ||
| warnings?: string[]; |
There was a problem hiding this comment.
[P2] Split warning aggregation from authenticated scan recovery
Can we keep this PR focused on verifying resumed scan artifacts and move warning persistence, checkout-cleanup reporting, and the new warned result field into a separate change? The current patch combines resume authentication, report regeneration, Python/runtime changes, and a new result/receipt shape across roughly 1,700 added lines. Splitting the independent warning behavior would make the security-sensitive recovery path substantially easier to review and test.
2 participants
Summary
Authenticate saved bulk-scan artifacts before reusing them, recover derived reports from validated canonical documents, and preserve complete or incomplete scan outcomes when checkout cleanup fails.
Fixes #30. Fixes #211. Fixes #248. Related proposals: #97, #218, and #255.
Changes
0.1.21for the report-only helper.Testing
--no-env-file, and the original 30-second timeout — 7 passed, 46 assertions, 0 failures. New coverage uses isolated metadata, reader, and writer mocks only.pnpm run types— passed.git diff HEAD^ HEAD --check— passed.7ac503f8: complete fixed-seed SDK suite with Bun 1.3.14,--no-env-file, seed 12345, and the original 30-second timeout — 1,311 passed, 11 skipped, 0 failed; 9,259 assertions across 63 files. JUnit agrees. Live integration and machine-policy opt-ins were disabled.plugin-files.jsonpackage inspection, and the unchanged installed real-worker smoke — passed once, sequentially. The packed helper and paired plugin version match the frozen source.Risk and rollout
Valid historical plugin output remains resumable, including configured archives that contain schemas without current helper scripts and permitted unknown v1 fingerprint fields. Authenticated report entries retain their historical formatting and do not require Python selection. Distinct unsealed entries are regenerated atomically without changing canonical JSON, seals, SARIF, HTML, receipts, costs, or attempts; ambiguous sealed-entry relationships require recovery before any write. Malformed complete ledger records and projection failures stop with recovery-required errors; cancellation is preserved. Historical dirty-worktree or symlink-scoped receipts without authoritative identity metadata may require one new attempt. The warning count and optional receipt warnings are additive; a cleanup warning does not change the scan's exit outcome. The plugin version advances to
0.1.21; no contract schema or dependency changes are included.Public disclosure review
Newly authored material was reviewed for public disclosure. Existing automated review comments contain access-restricted report references; maintainer cleanup remains outstanding.