fix(sdk): preserve sealed artifacts after post-scan failure - #626
Conversation
|
@codex review |
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. |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed head 3e8985ff89d85e1550bd76dfa4b61825a676835c against its declared base, #623, and compared the overlapping #468 implementation.
Recommendation: use this as the single artifact-restoration implementation and retire/supersede #468. No new blocking defect found in this diff. Preserving a completed scan after optional follow-up failure is necessary. Restoring through a checked pathname alone is insufficient; the actual write must remain bound to the validated directory.
Correctness
The implementation reuses the existing cross-platform safe writer instead of adding another TypeScript pathname-check/write sequence. prepareScanArtifactRestorer captures canonical root identity before follow-up and uses the SDK-owned helper, independent of a selected custom plugin. Held parent descriptors on POSIX and the Windows ancestor-handle path enforce the write boundary. Setup failure skips optional work; restoration failure is not silently presented as a successfully recovered scan.
The differences from #468 matter: equality checking is restoration-only, uses bounded chunks and a size check, and does not change ordinary atomic/private replacement semantics. In a paired actual-helper fixture, an ordinary identical write on #468 retained mode 0644 and the original inode; this head replaced it with mode 0600. The plugin version bump and predecessor-cache test also cover the executable payload, not just source files.
Simplification
Do not ship both proposals or keep two subtly different restoration paths. This version is the more complete replacement. The one naming/ownership cleanup I would consider is moving the platform-neutral stream_matches_payload routine out of the Windows-named module if a natural existing shared helper fits; do not create a new abstraction layer solely for that name. Preserve the distinction between normal writes and no-op restoration writes.
Verification
Ran focused post-scan/restoration and predecessor-bundle tests: 11 passed, 0 failed; also executed the paired ordinary-write fixture against both heads. Linux, Bun 1.3.14 / Node 22.13.1, cached dependencies. Source review covered the held Windows-handle path, but I did not run native Windows tests or a live model follow-up. This review does not substitute for reviewing the much larger #623 base.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
faizan-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 429ecd18a88866f8d16b736187066b19776789c9 against f5ca13647d427b1e4230674978c59dba2d79cf76. The update synchronizes bundle/cache versions; restoration implementation is unchanged. Focused checks pass (66 tests, one platform skip), as does typecheck. No new blocking code finding in this delta; this does not review or approve #623.
Holding approval for the current Windows CI failures:
- Node 22, shard 3: parallel credential imports encounter a PowerShell
Get-ChildItem/FileNotFoundExceptionfor an.auth-*.tmpfile. - Node 24, shard 2: the API-login cancellation test's ready file never appears before its deadline.
Those credential functions and test bodies are unchanged here, so these logs do not demonstrate a new restoration regression. Please get the full Windows checks passing; the local macOS passes do not establish Windows success.
faizan-oai
left a comment
There was a problem hiding this comment.
The CI-only hold is cleared: node-ci attempt 2 passes all 25 jobs, including Windows.
Head 429ecd18a88866f8d16b736187066b19776789c9 and declared base f5ca13647d427b1e4230674978c59dba2d79cf76 are unchanged from the code review. The earlier 66 passing focused tests (one platform skip) and typecheck apply to this exact delta; no new discussion concern appeared. No blocking findings in #626's own changes. This approval does not review or approve parent #623.
Summary
Preserve completed scan artifacts when optional post-scan instructions fail by restoring the sealed bytes with SDK-owned, identity-bound logic and bounded comparisons.
Changes
0.1.23to0.1.24and cover a real predecessor-cache upgrade with safety-key retention.Testing
pnpm run types,pnpm run format, andpnpm run build.Risk and rollout
Internal artifact-restoration behavior only; no CLI, configuration, or schema changes. The Windows implementation changed but was not executed locally and remains gated on hosted Windows CI.
Public disclosure review