fix(windows): keep credential state reusable across scans - #635
fix(windows): keep credential state reusable across scans#635faizan-oai wants to merge 2 commits into
Conversation
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. |
|
@mldangelo-oai — could you review this credential-lifecycle fix? It narrows the default sandbox write roots and removes the cross-process ACL-count race while retaining the existing credential protections. Native boundary and repeated-start controls pass; the description records the two broader-suite failures and the recovery limits. @codex 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". |
mldangelo-oai
left a comment
There was a problem hiding this comment.
I think you are adding too much to the readme and this is hard for me to test, but I trust it solves an issue
|
Condensed the recovery guidance to one short paragraph beside the existing state-directory instructions in For a focused reproduction check on Windows, from bun test --timeout 30000 --seed 12345 tests-ts/runtime.test.ts tests-ts/config.test.ts --test-name-pattern 'Windows credential|Windows credentials|scan helpers to write'This passed 19 tests locally, including the actual sandboxed helper, denied state-directory writes, subsequent credential reuse, and live/broken junction rejection. It uses synthetic fixtures and does not need a model-backed scan. The initial Windows verification failure was the package smoke test's npm dependency installation timing out before the installed CLI was run. The new head's CI is rechecking installation as well as the tests. |
|
@codex review the current head |
|
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". |
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed 6a58daff3724aa7bc4961ebdfea41da3280f7242. The lifecycle fixes are justified, and the changes reduce unnecessary machinery. I found no blocking correctness issue in the reviewed diff.
Necessity and correctness
The two changes address different real causes: the broad state-directory write grant can affect credential ancestry, while the old descendant verifier compares two directory enumerations taken at different times. Normal cache/lock churn makes equality of those counts an unreliable success condition.
Removing the state-wide write root is more appropriate than adding exceptions to the credential-owner checks. I traced the SDK flow: the model's working directory is the scan workspace, while registration, thread updates, and final persistence are driven by the host-side workbench calls. This distinction is essential; the review is not assuming that a sandboxed process can still write arbitrary state because it runs under the same account.
The single native inspection preserves separate ancestor/home/descendant checks and requires both successful subprocess completion and a matching completion record. It rejects missing/truncated completion and output after completion. It therefore removes the cross-process counting race without simply treating a partial traversal as success. This is an inspection of entries encountered during traversal, not an atomic snapshot of a concurrently changing directory; the code and documentation should not promise the latter.
Simplification and rollout
Keep the deletion of the Node-side walk and retries. Reintroducing a second enumeration would restore the race and duplicate link/type checks. The completion marker is a small internal protocol; it does not need a general stream-protocol abstraction or another lock redesign.
The default-profile change applies on every platform, not only Windows. Keeping the explicit credential-home read protection and tests for workspace writes/state writes is therefore important. The recovery paragraph also correctly avoids silently rewriting pre-existing ancestor ACLs or pretending a fresh state directory retains the same history. I would keep that explanation beside the state-directory setting, not expand it into a second authentication setup guide.
Verification and limits
Ran api-credentials.test.ts, api-preflight-config.test.ts, config.test.ts, and runtime.test.ts with seed 12345: 170 passed, 13 platform skips, 0 failed, Bun 1.3.14/Linux with locally available dependencies. Initial runs exposed sandbox ownership mapping and group-writable fixture directories; rerunning outside the sandbox with a normal 022 umask passed without source changes.
This validates the cross-platform code and synthetic ACL-stream controls available on Linux. It does not independently validate native Windows ACL inheritance, ConstrainedLanguage behavior, or repeated real Windows scan startup; those remain platform-specific evidence to preserve from the PR's native controls and CI.
Summary
Keep the default Windows sandbox profile from invalidating credential-home ancestry for the next command, and allow credential ACL inspection to complete while ordinary private cache and lock files change.
Changes
Testing
git diff --check: passed.12345: 1,630 passed, 59 skipped, 2 failed, 1 follow-on error. An MCP test exceeded the package's 30-second timeout and passed an isolated 120-second-budget rerun in 33.27 seconds; its shutdown produced the follow-on error. A separate scan-root alias test returned null Python subprocess status and passed its isolated rerun. These failures remain reported; final cross-platform CI is required.Risk and rollout
The default profile no longer grants write access to the entire state directory on any platform. Approved requests can still grant additional permissions. Native tests cover the helpers' workspace writes and current credential reuse; CI must cover the remaining platforms. No new CLI flags or dependencies, automatic ACL migration, sandbox-identity allowlist, or lock redesign is introduced. Users with an already-invalid state ancestor must follow the documented fresh-state recovery; old reports and scan history are preserved separately.
Public disclosure review