fix(#6358): load sandbox hook settings via --settings flag - #6405
fix(#6358): load sandbox hook settings via --settings flag#6405fullsend-ai-coder[bot] wants to merge 5 commits into
Conversation
Site previewPreview: https://1d442f0d-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 10:57 AM UTC · Completed 11:15 AM UTC Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Labels: PR modifies sandbox security hook installation (internal/security/hooks.go) and runner CLI/runtime (internal/cli/run.go, internal/runtime/) Previous run (6)ReviewFindingsMedium
Low
|
|
[High] The issue acceptance criteria require "a behaviour/e2e or functional check exercises one blocking hook end-to-end under the pinned CLI." The golden tests added here verify the Hook scripts and [Low] |
|
/fs-fix Address the review comments: add a Gherkin behaviour test, fix the stale comments in run.go:1773, hooks.go:46, and hooks.go:61. |
|
🤖 Finished Fix · ✅ Success · Started 11:53 AM UTC · Completed 12:05 PM UTC Commit: |
- run.go:1773: update comment to reference claude-config/hooks/ instead of the old workspace/.claude/ path - hooks.go:46: update claudeSettings comment to reference hooks.json instead of .claude/settings.json - hooks.go:61: update GenerateClaudeSettings docstring to reference hooks.json instead of .claude/settings.json - Add hooks-loaded.feature: Gherkin behaviour test that dispatches a custom harness, attempts to fetch a cloud metadata endpoint (169.254.169.254), and asserts the SSRF PreToolUse hook blocks it. Catches the "silently not loaded" regression class. Addresses review feedback on #6405
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 4 review findings: fixed 3 stale comments referencing old hook paths (.claude/settings.json, workspace/.claude/) and added a Gherkin behaviour test (hooks-loaded.feature) that verifies the SSRF PreToolUse hook fires end-to-end when hooks are loaded via --settings. Fixed (4):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 12:07 PM UTC · Completed 12:25 PM UTC Commit: |
|
/fs-fix Rebase onto main and fix any coverage gaps. |
|
🤖 Finished Fix · ✅ Success · Started 1:48 PM UTC · Completed 1:59 PM UTC Commit: |
🔧 Fix agent — iteration 2 (human-triggered)Rebased branch onto origin/main (clean, no conflicts). Added 2 tests for installClaudeHooks to close the patch coverage gap — function went from 0% to 59.6% coverage. All changed lines in the diff (hooksDir, mkdirCmd format, remotePath format, Upload target) are now exercised. Pre-existing CLI test failures (TestStartFetchService, TestPostScriptEnv) confirmed present on main, not caused by this PR. internal/cli/run.go is in Codecov's ignore list. Fixed (2):
Tests: passed Decision points
Next steps:
|
- run.go:1773: update comment to reference claude-config/hooks/ instead of the old workspace/.claude/ path - hooks.go:46: update claudeSettings comment to reference hooks.json instead of .claude/settings.json - hooks.go:61: update GenerateClaudeSettings docstring to reference hooks.json instead of .claude/settings.json - Add hooks-loaded.feature: Gherkin behaviour test that dispatches a custom harness, attempts to fetch a cloud metadata endpoint (169.254.169.254), and asserts the SSRF PreToolUse hook blocks it. Catches the "silently not loaded" regression class. Addresses review feedback on #6405
The installClaudeHooks function had 0% test coverage. Two new tests exercise the happy path (stub openshell) and the error path (missing openshell), bringing the function to ~60% coverage and ensuring the changed hook-path and settings-path lines are covered. Addresses review feedback on #6405
45ccde0 to
0a16c18
Compare
|
🤖 Finished Review · ✅ Success · Started 2:01 PM UTC · Completed 2:19 PM UTC Commit: |
|
🤖 Finished Review · ✅ Success · Started 10:36 AM UTC · Completed 10:55 AM UTC Commit: |
- run.go:1773: update comment to reference claude-config/hooks/ instead of the old workspace/.claude/ path - hooks.go:46: update claudeSettings comment to reference hooks.json instead of .claude/settings.json - hooks.go:61: update GenerateClaudeSettings docstring to reference hooks.json instead of .claude/settings.json - Add hooks-loaded.feature: Gherkin behaviour test that dispatches a custom harness, attempts to fetch a cloud metadata endpoint (169.254.169.254), and asserts the SSRF PreToolUse hook blocks it. Catches the "silently not loaded" regression class. Addresses review feedback on #6405
The installClaudeHooks function had 0% test coverage. Two new tests exercise the happy path (stub openshell) and the error path (missing openshell), bringing the function to ~60% coverage and ensuring the changed hook-path and settings-path lines are covered. Addresses review feedback on #6405
f3153a0 to
b924d2e
Compare
…rateHooksConfig The struct and function names still referenced "claude settings" after the output file was renamed to hooks.json. Rename to hooksConfig and GenerateHooksConfig to match the actual artifact, and add a test covering the hooks.json upload error path to close the codecov/patch coverage gap. Addresses review feedback on #6405
- docs/architecture.md: update parenthetical from ".claude/settings.json" to "hooks.json loaded via --settings" - docs/guides/dev/cli-internals.md: fix function name from buildClaudeCommand() to buildRunCommand() and add conditional --settings flag to the command diagram - internal/runtime/claude.go: rename temp file pattern from fullsend-settings-*.json to fullsend-hooks-*.json for consistency with the hooksConfig/GenerateHooksConfig rename Addresses review feedback on #6405
|
🤖 Finished Review · ✅ Success · Started 11:00 AM UTC · Completed 11:13 AM UTC Commit: |
- run.go:1773: update comment to reference claude-config/hooks/ instead of the old workspace/.claude/ path - hooks.go:46: update claudeSettings comment to reference hooks.json instead of .claude/settings.json - hooks.go:61: update GenerateClaudeSettings docstring to reference hooks.json instead of .claude/settings.json - Add hooks-loaded.feature: Gherkin behaviour test that dispatches a custom harness, attempts to fetch a cloud metadata endpoint (169.254.169.254), and asserts the SSRF PreToolUse hook blocks it. Catches the "silently not loaded" regression class. Addresses review feedback on #6405
The installClaudeHooks function had 0% test coverage. Two new tests exercise the happy path (stub openshell) and the error path (missing openshell), bringing the function to ~60% coverage and ensuring the changed hook-path and settings-path lines are covered. Addresses review feedback on #6405
…rateHooksConfig The struct and function names still referenced "claude settings" after the output file was renamed to hooks.json. Rename to hooksConfig and GenerateHooksConfig to match the actual artifact, and add a test covering the hooks.json upload error path to close the codecov/patch coverage gap. Addresses review feedback on #6405
- docs/architecture.md: update parenthetical from ".claude/settings.json" to "hooks.json loaded via --settings" - docs/guides/dev/cli-internals.md: fix function name from buildClaudeCommand() to buildRunCommand() and add conditional --settings flag to the command diagram - internal/runtime/claude.go: rename temp file pattern from fullsend-settings-*.json to fullsend-hooks-*.json for consistency with the hooksConfig/GenerateHooksConfig rename Addresses review feedback on #6405
b924d2e to
c5ad996
Compare
|
🤖 Review · Commit: |
installClaudeHooks wrote hook wiring to /sandbox/workspace/.claude/settings.json, but buildRunCommand starts Claude Code from /sandbox/workspace/<repo>. Claude Code only reads project settings from <cwd>/.claude/settings.json, so the security hooks were silently never loaded. Move hook scripts and settings to /sandbox/claude-config/ (the runner-owned config directory, outside the agent-writable workspace tree) and pass the settings file via --settings in buildRunCommand. --settings takes precedence over project/local settings, ensuring the runner's security hooks are always active. Changes: - security.SandboxHooksDir → /sandbox/claude-config/hooks - Add security.SandboxHooksSettings → /sandbox/claude-config/hooks.json - installClaudeHooks writes to the new paths - buildRunCommand appends --settings when HooksSettingsPath is set - run.go populates HooksSettingsPath when security is enabled - Golden tests for --settings presence/absence/quoting - docs/runtimes.md workspace layout updated Note: pre-commit could not run (sandbox network policy blocks git fetch for hook environment setup). go vet and go test passed. The post-script runs authoritative pre-commit on the runner. Closes #6358
- run.go:1773: update comment to reference claude-config/hooks/ instead of the old workspace/.claude/ path - hooks.go:46: update claudeSettings comment to reference hooks.json instead of .claude/settings.json - hooks.go:61: update GenerateClaudeSettings docstring to reference hooks.json instead of .claude/settings.json - Add hooks-loaded.feature: Gherkin behaviour test that dispatches a custom harness, attempts to fetch a cloud metadata endpoint (169.254.169.254), and asserts the SSRF PreToolUse hook blocks it. Catches the "silently not loaded" regression class. Addresses review feedback on #6405
The installClaudeHooks function had 0% test coverage. Two new tests exercise the happy path (stub openshell) and the error path (missing openshell), bringing the function to ~60% coverage and ensuring the changed hook-path and settings-path lines are covered. Addresses review feedback on #6405
…rateHooksConfig The struct and function names still referenced "claude settings" after the output file was renamed to hooks.json. Rename to hooksConfig and GenerateHooksConfig to match the actual artifact, and add a test covering the hooks.json upload error path to close the codecov/patch coverage gap. Addresses review feedback on #6405
- docs/architecture.md: update parenthetical from ".claude/settings.json" to "hooks.json loaded via --settings" - docs/guides/dev/cli-internals.md: fix function name from buildClaudeCommand() to buildRunCommand() and add conditional --settings flag to the command diagram - internal/runtime/claude.go: rename temp file pattern from fullsend-settings-*.json to fullsend-hooks-*.json for consistency with the hooksConfig/GenerateHooksConfig rename Addresses review feedback on #6405
c5ad996 to
b07a630
Compare
|
🤖 Finished Review · ✅ Success · Started 11:33 AM UTC · Completed 11:54 AM UTC Commit: |
Summary
Sandbox tool hooks (Tirith, SSRF, canary, secret redaction, unicode normalization, context suppression, tool allowlist) were silently never loaded by Claude Code in production runs.
installClaudeHookswrote hook wiring to/sandbox/workspace/.claude/settings.json, butbuildRunCommandstarts the CLI from/sandbox/workspace/<repo>. Claude Code only reads project settings from<cwd>/.claude/settings.json— the hooks directory was a sibling, not a parent, so the settings were never found.Changes
/sandbox/claude-config/hooks/and settings at/sandbox/claude-config/hooks.json— co-located under the runner-owned directory, outside the agent-writable workspace tree--settingsflag:buildRunCommandappends--settings /sandbox/claude-config/hooks.jsonwhenHooksSettingsPathis set inRunParams, whichrun.gopopulates when security is enabled.--settingstakes precedence over project/local settings--settingspresence, absence, and quote escaping. The no-double-spaces table test updated to cover the new flagdocs/runtimes.mdworkspace layout diagram updated to reflect the new pathsTesting
go test ./internal/runtime/...— all tests pass including new golden testsgo test ./internal/security/...— all tests passgo vet ./...— cleango build ./...— cleanbuildRunCommandat 100% coverageFollow-up
Repo-owned
<repo>/.claude/settings.jsonhooks are still loaded from<cwd>by Claude Code. The--settingsflag ensures runner hooks take precedence, but a malicious repo could add its own hooks. A follow-up issue should assess whether to scan or disable repo-owned hooks.Closes #6358
Post-script verification
agent/6358-hooks-settings-path)2e78d663bb7aa15db58e97e83dcf9d27eacdc0ac..HEAD)