2 Commits

Include renames

Author	SHA1	Message	Date
3 people	30f6d8f591	fix(specialist): make overwrites atomic (#757) ... * fix(specialist): make overwrites atomic * fix(specialist): harden atomic replacement * fix(specialist): replace through a Windows-atomic, DACL-preserving primitive Addresses both P1 findings on #757. os.Rename — which fsutil.RenameWithRetry wraps — is documented non-atomic outside Unix, and the retry loop only handles sharing violations. Worse, the replacement is a freshly created temporary file, so renaming it over the destination also publishes the directory's inherited DACL in place of the destination's own: overwriting a specialist that had been explicitly restricted inside a group-readable directory would expose its system prompt to that group. temp.Chmod(0o600) cannot prevent that on Windows, where Go maps only the owner-write bit. New fsutil.ReplaceWithRetry publishes through a platform primitive: - Windows uses ReplaceFileW with REPLACEFILE_WRITE_THROUGH, one operation that carries the replaced file's security descriptor, attributes, and streams over to the replacement. REPLACEFILE_IGNORE_ACL_ERRORS is deliberately not passed — silently losing the descriptor is the failure being fixed — so a merge failure surfaces and leaves the destination untouched. A missing destination falls back to a rename (nothing to replace or preserve), as do volumes that cannot perform a replace at all (FAT/exFAT, some network redirectors), which carry no ACL either. The transient-lock retry is unchanged. - Unix keeps rename(2), which is already atomic within one filesystem. Tests: Windows coverage asserts the destination's creation time survives (a rename would not, proving the call goes through ReplaceFileW) and that a protected owner-only DACL survives both at the fsutil layer and through the real `Create --force` path; a plain rename was verified to replace that DACL with the directory's inherited ACEs, so the assertion has teeth. POSIX covers both destination shapes. * fix(fsutil): stop ignoring ACL merge errors, recover partial ReplaceFileW Addresses jatmn's two #757 review findings: - The call passed REPLACEFILE_IGNORE_MERGE_ERRORS (0x2) while the comment claimed ACL failures were fail-closed. Microsoft documents 0x2 and REPLACEFILE_IGNORE_ACL_ERRORS (0x4) identically: with either set, a call lacking WRITE_DAC "succeeds but the ACLs are not preserved". Passing 0x2 therefore let a --force overwrite silently publish the temp file's inherited directory DACL over a restricted specialist and expose its system prompt. Pass no flags at all. - With no backup file, ERROR_UNABLE_TO_MOVE_REPLACEMENT (1176) and ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 (1177) leave the destination deleted (1176) or renamed to an unreachable name (1177) while the replacement survives only under its temporary name. Returning those unrepaired let the caller's deferred temp-file cleanup remove the sole surviving copy, so one failed 'specialist create --force' destroyed both the old manifest and the new one. recoverPartialReplace now moves the replacement into place and still reports the error, since the destination's descriptor died with the destination. Also drops the doc claim that REPLACEFILE_WRITE_THROUGH (0x1) flushes before returning: Microsoft documents that value as "not supported". The two fsutil regression tests are verified to fail against the pre-fix code. The specialist-side test guards the caller half of the recovery contract and does not fail pre-fix; its comment says so. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(fsutil): preserve Windows replacement backups Use a managed ReplaceFileW backup so partial failures can restore the original file and DACL. Fail closed when atomic replacement is unsupported, retry rollback and backup cleanup, and retain a known backup if rollback cannot complete.\n\nCover unsupported volumes, 1176/1177 semantics, cleanup, rollback retries, and symlink rejection. Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-be8e-75eb-b274-b7e22af373a0 Co-authored-by: Pierre Bruno <[email protected]> * fix(specialist): report committed cleanup warnings Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * docs(specialist): describe what a failed overwrite actually leaves behind The Windows partial-replace recovery errors told an operator the replacement "remains at" the caller's temporary path, but the only caller removes that file in a deferred cleanup before the error ever surfaces — it named a file that was already gone. Recovery from a failed overwrite is always about the original, so the errors now describe only what the replace itself left on disk: the state of the destination and of the managed backup. The replacement path is no longer a parameter, since nothing reports it. The one terminal state that needs a human — 1177 whose rollback exhausted its retries — now says so outright. The original then exists only under an opaque .zero-replace-*.backup name, and the loader reads *.md, so the specialist stays invisible until somebody moves it back. The error names both paths and the action; docs/SPECIALISTS.md walks through it; and loadDirectory records why the non-.md siblings an interrupted overwrite can leave are skipped rather than guessed at. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(specialist): serialize managed loads during replacement Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <[email protected]> --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> Co-authored-by: Amp <[email protected]>	2026-08-10 09:49:32 +08:00
euxaristia	6fc1220f6a	fix(sandbox): scrub sensitive credentials from sandbox environment (#660) ... * fix(windows): retry atomic file renames on Access is denied / Sharing violation * test(swarm): add unit tests for rename retry and non-retryable errors * test(swarm): skip TestMailboxRenameRetry on non-Windows platforms * fix(sandbox): scrub sensitive provider credentials from sandbox env * refactor(sandbox): unify environment creation and reduce lines of code * fix(sandbox): forward workspaceRoot through sandboxEnvironment, gate Windows env on backend * style(sandbox,swarm): remove stray blank lines flagged by gofmt * refactor(fsutil): extract shared rename retry helper, derive scrub list from provider catalog Address CodeRabbit review feedback: - Add internal/fsutil.RenameWithRetry with an injectable rename override and move the four duplicated renameWithRetry/isWindowsSharingViolation copies (cron, sessions, sandbox, swarm) onto it. Mailbox keeps its rename test seam by passing its field through. Add fsutil tests for the Windows retry and non-retryable paths. - Remove the orphaned metadata.json.tmp in cron writeJob when the rename permanently fails, matching the sibling writers. - Derive scrubSensitiveEnv keys from providercatalog AuthEnvVars plus a static list of non-catalog secrets, so new providers are scrubbed automatically. AWS_PROFILE and GOOGLE_APPLICATION_CREDENTIALS stay because they are pointers, not secrets. * fix(sandbox): deny sandboxed reads of cloud credential stores by default Address CodeRabbit finding that scrubbed env pointers still left the underlying credential files readable under the read-all workspace posture: - Add credentialDenyReadPaths: default profile-level deny-read entries for ~/.aws, ~/.config/gcloud, ~/.azure, and the GOOGLE_APPLICATION_CREDENTIALS target file, wired into both permission profile builders so the seatbelt and Linux helper backends enforce them. Entries are stat-filtered and a store nested under a user AllowRead entry is dropped, keeping an explicit opt-out. - Keep the defaults out of Policy.DenyRead, whose emptiness gates escalated execution, and skip Windows so the profile deny list does not flip the runner off the WRITE_RESTRICTED token path. - Scrub GOOGLE_APPLICATION_CREDENTIALS from the sandbox env; keep AWS_PROFILE since the deny-read rule on ~/.aws is the real protection and the SDKs fall back to the default profile regardless. - Add credential deny path tests, extend the scrub test, relax the profile test's exact deny count to containment. * test(cli): pin HOME in sandbox policy golden test The default credential-store deny-read entries depend on what exists in the real home directory (the macOS CI image ships ~/.aws), which leaked host paths into the policy JSON golden comparison. Point HOME at an empty temp directory so the golden stays hermetic. * fix(sandbox): protect GOOGLE_APPLICATION_CREDENTIALS without a home, retry lock violations Address CodeRabbit review feedback: - credentialDenyReadPaths no longer returns early when the home lookup fails; home-based candidates become conditional and the GOOGLE_APPLICATION_CREDENTIALS target is always considered. - fsutil.RenameWithRetry also retries ERROR_LOCK_VIOLATION (errno 33), matching the mcp lock handling; helper renamed to isWindowsSharingOrLockViolation. - Extend tests: homeless GOOGLE_APPLICATION_CREDENTIALS case and a lock violation step in the retry sequence. * fix(sandbox): tighten env-scrub test assertion and add missing secret keys TestScrubSensitiveEnv only checked an allowlist plus a length count, so a duplicate allowed key silently replacing a required one would still pass. Compare the exact expected key/value map instead. Also add ZERO_WEBSEARCH_API_KEY and ZERO_DAEMON_REMOTE_TOKEN to scrubSensitiveEnv's sensitiveKeys list: both are real bearer secrets read from the environment but were missing from the scrub list, leaving them readable by sandboxed commands when set in the parent shell.	2026-07-14 21:46:10 +08:00