Repository navigation
fix: Windows correctness in checkpoints, config firewall, sandbox and agentEnv - #25
Conversation
… agentEnv
Fixes the 12 Windows-only test failures (backlog M8-6). Each is a product
bug on Windows, not a test assumption:
checkpoints.ts (4 tests, m6-checkpoints / m6-restore)
- Snapshot/restore went through git with the user's core.autocrlf. With
autocrlf=true (the Windows default) an LF file was restored as CRLF, breaking
the M6.6 byte-identical restore guarantee. Checkpoint git calls now pin
core.autocrlf=false via GIT_CONFIG_COUNT/KEY/VALUE; the user's git config is
untouched and the repo's own .gitattributes still apply. Reproduced on Linux
with autocrlf=true (4 failures) and verified fixed.
configFirewall.ts (3 tests, m3-security / m8-hostile-config)
- Drift/baseline keys came from path.relative(), so Windows recorded
`.claude\settings.json` while every other platform (and the guarded list)
uses `/`. Keys are now POSIX-normalised; baselines approved before this fix
on Windows are normalised on read so they don't resurface as drift.
runner.ts agentEnv (4 tests, m5-diff-capture)
- PATH was re-joined with a hard-coded ":" instead of path.delimiter, mangling
a Windows PATH (";"-separated) so git, and any agent CLI, could not be
found. No change on POSIX, where path.delimiter is ":".
sandbox.ts (1 test, m3-security M3.10)
- When `sh` itself cannot be spawned, spawnSync().stdout is null and .trim()
threw a TypeError. Now treated as "no srt", so selectSandbox fails closed
with the intended "sandbox unavailable" error.
Verified: typecheck, lint (0 errors), 406 tests passed, eval 174/174; full
suite also passes under core.autocrlf=true.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KddPyDgHXVotgYiCUfYS3e
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ECC Tools / Security EvidenceCommit: Security evidence gate passed (success) No security-sensitive scanner-evidence gap detected. Mode: enforce Scanned 4 changed file(s). No missing scanner-evidence signal was detected. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Risk TaxonomyCommit: PR taxonomy review recommended (neutral) Detected 1 PR taxonomy bucket(s): CI/CD Recommendation. Scanned 4 changed file(s). Roadmap taxonomy buckets: CI/CD RecommendationCI, dependency, coverage, and contract signals should be routed into follow-up checks or verification work. Signals:
Paths:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Reference Set ReadinessCommit: Reference set readiness gaps detected (neutral) Reference evidence present for 0/7 areas (0%) across 4 changed file(s). This check is based on files changed in this PR. Repository-level readiness is still reported by
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (7)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ECC Tools / Hosted Promotion ReadinessCommit: Hosted promotion readiness passed (success) No hosted promotion evidence gaps detected across 4 changed file(s); 0 corpus scenarios had matching evidence. This check compares PR file changes against the evaluator/RAG promotion corpus in No evaluator corpus scenarios matched this PR. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL js/file-system-race on configFirewall.ts: a file was lstat'ed and then read by path, so a hostile repo (the M8.2 threat model) could swap it for a symlink in between and have content from outside the workspace hashed into the baseline and shown in review. Guarded files are now opened once with O_NOFOLLOW, re-checked with fstat, and read through that fd. A file that vanishes or turns into a symlink mid-scan is skipped; the next scan picks up the new state (symlinks are already recorded as `SYMLINK -> target`). Verified: typecheck, lint, 406 tests passed (incl. m3-security and m8-hostile-config), eval 174/174. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KddPyDgHXVotgYiCUfYS3e
ECC Tools / Security EvidenceCommit: Security evidence gate passed (success) No security-sensitive scanner-evidence gap detected. Mode: enforce Scanned 4 changed file(s). No missing scanner-evidence signal was detected. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Risk TaxonomyCommit: PR taxonomy review recommended (neutral) Detected 1 PR taxonomy bucket(s): CI/CD Recommendation. Scanned 4 changed file(s). Roadmap taxonomy buckets: CI/CD RecommendationCI, dependency, coverage, and contract signals should be routed into follow-up checks or verification work. Signals:
Paths:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Reference Set ReadinessCommit: Reference set readiness gaps detected (neutral) Reference evidence present for 0/7 areas (0%) across 4 changed file(s). This check is based on files changed in this PR. Repository-level readiness is still reported by
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Hosted Promotion ReadinessCommit: Hosted promotion readiness passed (success) No hosted promotion evidence gaps detected across 4 changed file(s); 0 corpus scenarios had matching evidence. This check compares PR file changes against the evaluator/RAG promotion corpus in No evaluator corpus scenarios matched this PR. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
With the native-Windows fixes in this PR the full suite passes on windows-latest (406 passed, 2 skipped; eval 174/174), so drop continue-on-error and treat Windows like Linux/macOS. DISTRIBUTION.md and backlog M8-6 updated; WSL2 stays the recommended Windows path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KddPyDgHXVotgYiCUfYS3e
ECC Tools / Security EvidenceCommit: Security evidence gate passed (success) No security-sensitive scanner-evidence gap detected. Mode: enforce Scanned 7 changed file(s). No missing scanner-evidence signal was detected. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KddPyDgHXVotgYiCUfYS3e
ECC Tools / PR Risk TaxonomyCommit: PR taxonomy review recommended (neutral) Detected 2 PR taxonomy bucket(s): Security Evidence, CI/CD Recommendation. Scanned 7 changed file(s). Roadmap taxonomy buckets: Security EvidenceSecurity-sensitive changes should carry explicit scanner, code-scanning, or focused regression evidence. Signals:
Paths:
CI/CD RecommendationCI, dependency, coverage, and contract signals should be routed into follow-up checks or verification work. Signals:
Paths:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Reference Set ReadinessCommit: Reference set readiness gaps detected (neutral) Reference evidence present for 0/7 areas (0%) across 7 changed file(s). This check is based on files changed in this PR. Repository-level readiness is still reported by
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Hosted Promotion ReadinessCommit: Hosted promotion readiness passed (success) No hosted promotion evidence gaps detected across 7 changed file(s); 0 corpus scenarios had matching evidence. This check compares PR file changes against the evaluator/RAG promotion corpus in No evaluator corpus scenarios matched this PR. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Config AuditCommit: No changed-config issues detected (success) Scanned 1 config file(s) present at this commit across 1 changed config path(s) and found no issues in the supported security rules. Changed config files:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Harness AuditCommit: No harness issues detected (success) Scanned 1 changed config file(s) and found no harness issues. Changed config files:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Security EvidenceCommit: Security evidence gate passed (success) No security-sensitive scanner-evidence gap detected. Mode: enforce Scanned 7 changed file(s). No missing scanner-evidence signal was detected. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Risk TaxonomyCommit: PR taxonomy review recommended (neutral) Detected 2 PR taxonomy bucket(s): Security Evidence, CI/CD Recommendation. Scanned 7 changed file(s). Roadmap taxonomy buckets: Security EvidenceSecurity-sensitive changes should carry explicit scanner, code-scanning, or focused regression evidence. Signals:
Paths:
CI/CD RecommendationCI, dependency, coverage, and contract signals should be routed into follow-up checks or verification work. Signals:
Paths:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Reference Set ReadinessCommit: Reference set readiness gaps detected (neutral) Reference evidence present for 0/7 areas (0%) across 7 changed file(s). This check is based on files changed in this PR. Repository-level readiness is still reported by
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Hosted Promotion ReadinessCommit: Hosted promotion readiness passed (success) No hosted promotion evidence gaps detected across 7 changed file(s); 0 corpus scenarios had matching evidence. This check compares PR file changes against the evaluator/RAG promotion corpus in No evaluator corpus scenarios matched this PR. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Config AuditCommit: No changed-config issues detected (success) Scanned 1 config file(s) present at this commit across 1 changed config path(s) and found no issues in the supported security rules. Changed config files:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Harness AuditCommit: No harness issues detected (success) Scanned 1 changed config file(s) and found no harness issues. Changed config files:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
|
The Generated by Claude Code |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Graphify review — findings
Makes native Windows a blocking CI leg by fixing the product bugs behind the earlier failures. Checkpoint git now pins core.autocrlf=false through GIT_CONFIG_* so restores are byte-faithful, and agentEnv joins PATH with path.delimiter. selectSandbox treats an unspawnable sh as "no srt", and config-firewall keys are normalized to / (with legacy \ baselines rewritten on Windows), with files read through an O_NOFOLLOW fd re-checked by fstat and skipped if they vanish or turn into a symlink mid-scan.
Worth a look
- Config firewall now skips files it fails to read instead of failing closed —
source/src/lib/configFirewall.ts:43· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Pinning core.autocrlf=false does not make checkpoints byte-faithful when the workspace has .gitattributes —
source/src/lib/checkpoints.ts:30· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 243 functions depend on the 118 functions this change touches.
Health — this change adds coupling hotspots:
- new:
run()— 64 callers, 11 callees - new:
spawnStream()— 20 callers, 5 callees - new:
createCheckpoint()— 8 callers, 7 callees - new:
restoreCheckpoint()— 5 callers, 11 callees - new:
prepareRun()— 2 callers, 26 callees - new:
POST()— 5 callers, 10 callees - new:
forkFromCheckpoint()— 4 callers, 11 callees - new:
retryFromCheckpoint()— 3 callers, 13 callees - …and 14 more — each is listed as a finding
Verification — 243 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 226 function(s) in the blast radius were not formally verified this run
· 22 more finding(s) on lines outside this diff (see the check run).
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous Review SummaryCurrent summary above is authoritative. Previous snapshots are kept for context only. Previous reviewStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Reviewed by free · Input: 694.6K · Output: 11.7K · Cached: 199.6K |
…e CRLF override Addresses Greptile review on #25. configFirewall.ts (P1 security) - The O_NOFOLLOW read added in 773d44f swallowed every open error, so an unreadable, unapproved guarded file dropped out of the scan, produced no drift, and the run launched unquarantined; before that change the read error aborted the scan (and prepareRun) instead. Now: ENOENT (vanished mid-scan) is skipped and compared against the baseline as absence; ELOOP/EMLINK (swapped for a symlink) is recorded as `SYMLINK -> target` drift; anything else, or a type change seen by fstat, throws so the scan fails closed. checkpoints.ts (P1) - core.autocrlf=false applied to every checkpoint git call, including the `git status` behind isWorkingTreeDirty, so a clean autocrlf=true tree (CRLF working copy, LF blobs) read as dirty and in-place restore refused without force. The override (RAW_EOL) now applies only to calls that move file content: snapshot `add` via the temporary index, `read-tree -u` and `worktree add`. Explicit .gitattributes eol/filter rules remain a documented limitation. Tests: new windows-portability.test.ts covers both (both fail on 99e772a, pass now). typecheck, lint 0 errors, 408 passed, eval 174/174; full suite also passes under core.autocrlf=true. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KddPyDgHXVotgYiCUfYS3e
ECC Tools / Security EvidenceCommit: Security evidence gate passed (success) No security-sensitive scanner-evidence gap detected. Mode: enforce Scanned 8 changed file(s). No missing scanner-evidence signal was detected. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Risk TaxonomyCommit: PR taxonomy review recommended (neutral) Detected 2 PR taxonomy bucket(s): Security Evidence, CI/CD Recommendation. Scanned 8 changed file(s). Roadmap taxonomy buckets: Security EvidenceSecurity-sensitive changes should carry explicit scanner, code-scanning, or focused regression evidence. Signals:
Paths:
CI/CD RecommendationCI, dependency, coverage, and contract signals should be routed into follow-up checks or verification work. Signals:
Paths:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Reference Set ReadinessCommit: Reference set readiness gaps detected (neutral) Reference evidence present for 0/7 areas (0%) across 8 changed file(s). This check is based on files changed in this PR. Repository-level readiness is still reported by
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Hosted Promotion ReadinessCommit: Hosted promotion readiness passed (success) No hosted promotion evidence gaps detected across 8 changed file(s); 0 corpus scenarios had matching evidence. This check compares PR file changes against the evaluator/RAG promotion corpus in No evaluator corpus scenarios matched this PR. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Config AuditCommit: No changed-config issues detected (success) Scanned 1 config file(s) present at this commit across 1 changed config path(s) and found no issues in the supported security rules. Changed config files:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Harness AuditCommit: No harness issues detected (success) Scanned 1 changed config file(s) and found no harness issues. Changed config files:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
## Summary Follow-up to #25, which fixed this bug in `agentEnv`. Four more places built `PATH` with a hard-coded `":"`. On Windows, `PATH` uses `;` as its separator, so the joined value is mangled and the programs they launch can't be found. ## Changes - `src/lib/nlm.ts`: `BIN_PATH`, used to launch `nlm` - `src/app/api/glm-code/build/route.ts`: `BIN_PATH`, used to launch `claude` - `src/app/api/seo/deploy/route.ts`: `DEPLOY_PATH`, used for the deploy tools - `src/app/api/opendesign/control/route.ts`: `PATH_EXTRA` **and** the template string that prepends it to `process.env.PATH` All of them now use `path.delimiter`. Nothing changes on Linux or macOS, where `path.delimiter` is `":"`. ## Testing - [x] `npm run typecheck` - [x] `npm run lint`: 0 errors - [x] `npm test`: 408 passed, 2 skipped - [x] `npm run eval`: 174/174 - [x] `npm run build` - [ ] Manually exercised in the dashboard. These routes launch external CLIs (nlm, claude, deploy tools) that aren't installed in CI. The change only swaps the separator character. ## Security checklist - [x] No direct `child_process` imports in `src/app/**` or `src/features/**` - [x] No secrets, API keys, `.env` files or personal config committed - [x] No new dependencies 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01KddPyDgHXVotgYiCUfYS3e --- _Generated by [Claude Code](https://claude.ai/code/session_01KddPyDgHXVotgYiCUfYS3e)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed executable path handling across build, design, deployment, and language-model features so it works with platform-specific path separators. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- greptile_comment --> <!-- greptile_summary --> <h2><a href="https://app.greptile.com/api/retrigger?id=74245275"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/RetriggerDark.svg?v=2"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=2"><img alt="Retrigger" src="https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=2" align="right"></picture></a>Confidence Score: 5/5</h2> <!-- greptile-risk --> The PR appears safe to merge. <details><summary>Summary</summary> This PR replaces hard-coded PATH separators with `path.delimiter` in four CLI-launch paths. - Linux, macOS, and WSL2 retain the same separator. - Native Windows receives a consistently semicolon-separated PATH. </details> <!-- greptile_confidence_score:5 --> <sub>Reviews (1) · Last reviewed commit: ["fix: join PATH with path.delimiter in nl..."](https://github.com/nilhemdot/agent-os/commit/6cac1d5f757bdc7fbd62397e56ddb46729abb6a6)</sub> <!-- /greptile_comment --> Co-authored-by: Claude <noreply@anthropic.com>
![https://greptile-static-assets.s3.amazonaws.com/badges/RetriggerDark.svg?v=2"><source](https://greptile-static-assets.s3.amazonaws.com/badges/RetriggerDark.svg?v=2"><source){kind=link}
![https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=2"><img](https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=2"><img){kind=link}
This branch was successfully deployed
3 participants
Summary
Fixes the 12 tests that failed only on Windows (backlog M8-6). Each failure traced back to a real bug on Windows, not a wrong test assumption. The full suite now passes on
windows-latest(406 passed, 2 skipped; eval 174/174 one8715fa), so the Windows CI job now blocks merges. No test was changed or skipped.Changes
src/lib/checkpoints.tscore.autocrlf. Withautocrlf=true(the Windows default), a file saved with LF line endings came back with CRLF, breaking the M6.6 "byte-identical restore" guarantee.core.autocrlf=falsethroughGIT_CONFIG_COUNT/KEY/VALUEenv vars. The user's git config isn't touched, and the repo's own.gitattributesstill applies.src/lib/configFirewall.tspath.relative(), which gives.claude\settings.jsonon Windows and.claude/settings.jsoneverywhere else./. Approvals saved on Windows before this fix are converted when read, so they don't reappear as drift.src/lib/runner.ts(agentEnv)PATHwas joined with a hard-coded":"instead ofpath.delimiter. That mangles a WindowsPATH(which uses;), sogitand agent CLIs launched by the runner can't be found.join(path.delimiter). No change on Linux or macOS, where the delimiter is":".src/lib/sandbox.tsshcan't be started at all,spawnSync().stdoutisnull, so.trim()threw aTypeErrorinstead of the intended error.srtfound", soselectSandboxrefuses to launch with the intended "sandbox unavailable" error.Security fix (CodeQL
js/file-system-race,773d44f): the config firewall checked each guarded file withlstatand then read it by path. A hostile repo could swap the file for a symlink in between and get a file from outside the workspace hashed and shown for review. Files are now opened once withO_NOFOLLOW, re-checked withfstat, and read through that same open handle. CodeQL's two "uncontrolled data in path expression" alerts are by design: the path is the workspace folder the local user chose to scan. The reasons are in the threads.CI and docs (
fb3b225): removedcontinue-on-errorfrom the Windows job. Updated DISTRIBUTION.md and backlog M8-6. WSL2 remains the recommended way to run on Windows.Not in this PR:
nlm.ts, plus theglm-code/build,seo/deployandopendesign/controlroutes, also joinPATHwith":". They only add macOS/Linux directories, for features the tests don't cover.Testing
npm run typechecknpm run lint: 0 errorsnpm test: 406 passed, 2 skipped (Linux, and onwindows-latestin CI)npm run eval: 174/174core.autocrlf=true, and the full suite passes under it with the fixSecurity checklist
child_processimports insrc/app/**orsrc/features/**.envfiles or personal config committedselectSandboxstill refuses to launch whenever no sandbox is available. The config-firewall race is fixed.🤖 Generated with Claude Code
https://claude.ai/code/session_01KddPyDgHXVotgYiCUfYS3e
Summary by CodeRabbit
The PR appears safe to merge; no new blocking issue was established.
Summary
The PR fixes native-Windows checkpoint, config-firewall, subprocess PATH, and sandbox behavior, adds regression tests, and makes Windows CI blocking.
Reviews (2) · Last reviewed commit: "fix(security): config firewall fails clo..."