fix(sdk): bind post-scan artifact restoration - #468
Conversation
|
@codex review |
|
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". |
|
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, |
|
Codex Review: Didn't find any major issues. 🚀 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 commit |
|
Codex Review: Didn't find any major issues. Keep it up! 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. |
![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: e8fc845591
ℹ️ 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".
mldangelo-oai
changed the title
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. Hooray! 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
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed head 1664172ca719ae1f4dc07ed23e6e228aab900324 and compared the overlapping newer #626 implementation.
Recommendation: supersede this PR with #626 rather than merging both. The underlying restoration fix is necessary, but this older version changes an unrelated write contract and ties recovery to the selected plugin's helper API.
[P2] Restrict unchanged-content short-circuiting to restoration
The new comparison in write_scan_local_bytes returns early for all identical writes, including ordinary callers that previously received a new private file. I executed the actual helper against an existing 0644 artifact with identical contents: this head left both its mode and inode unchanged. The corresponding #626 helper replaced it and produced mode 0600. Gate the optimization on restoration identity being supplied, as #626 does. Content equality is not equivalent to preserving the original writer's metadata/replacement contract.
Custom-plugin and bundle compatibility
prepareScanArtifactRestorer invokes options.pluginRoot and assumes that plugin contains the newly introduced scan_root_identity API. An otherwise compatible existing custom plugin will fail restoration setup and therefore skip requested post-scan instructions. Recovery is an SDK guarantee, so use the SDK-owned helper independently of the selected scan plugin. This PR also changes bundled Python code without advancing the bundled version markers; the version-keyed install/cache path needs an upgrade plan. #626 addresses both points and avoids the unbounded whole-file equality read in this version.
Existing feedback and verification
The earlier checked-path/rename race is addressed in the current implementation by held descriptors/handles; the unrelated credential-lock edit was removed. I am not re-reporting those old comments as current defects.
Ran the checked-in post-scan suite: 13 passed, 0 failed, plus the paired ordinary-write reproduction above. Linux with Bun 1.3.14 / Node 22.13.1 and cached dependencies; no native Windows or live model run. Keep the regression fixtures, but consolidate the production fix into #626.
2 participants
Summary
Keep completed scan artifacts inside the validated output tree when a post-scan
follow-up fails.
Changes
canonical scan-root identity before starting the follow-up.
preventing replacement roots or linked/swapped parents from redirecting writes.
nested, and binary artifacts.
without starting the follow-up.
Keep this PR's restoration guards unchanged and simplify test callbacks and
metadata assertions.
Testing
Local first-pass verification:
11 skipped, no failures.
After merging main
01bd062, 204 restoration/contract/concurrency/runtime testspassed with 12 platform skips. Types/models, formatting, Ruff, build, static
package checks and full installed smoke passed again. Real upgrades from the
original PR's 0.1.22, previous push's 0.1.59 and main's 0.1.60 each matched all
118 installed plugin files while preserving credentials.
The old Windows failure occurred in concurrent credential-lock setup; its
implementation has since changed on main. Native Windows and the full suite
were not rerun. CI is not awaited before advancing to the next PR.
fd98a90): package 0.1.21 includes the MCP launcher-permission fix; feature source and bundled payload are unchanged. Types/model generation, formatting, build, 28 focused package/report/launcher tests, static artifact verification and full installed-package smoke passed, including MCP initialization. CI was not awaited.Risk and rollout
No documented SDK, CLI, flag, environment, dependency, or state-format changes.
Failed follow-ups can atomically replace damaged sealed artifacts, then the
completed contract is revalidated. Unchanged artifacts retain their file
identity and timestamps.
If the root or an artifact parent changes identity, restoration fails closed.
If setup is unavailable, the follow-up is skipped and the completed scan is
returned with a warning. This remains a draft PR; no release is published.
Public disclosure review
The current update and public PR surface have been reviewed. Existing automated
comments contain access-restricted report links, so the second attestation
remains unchecked.