fix(#6358): load sandbox hook settings via --settings flag - #6405
Conversation
Site previewPreview: https://812182e7-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
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)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 (10)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: |
Rename the local variable `settings` to `cfg` in GenerateHooksConfig to avoid shadowing the type name and align with Go naming conventions for config structs. Addresses review feedback on #6405
🔧 Fix agent — iteration 6 (human-triggered)Renamed local variable Fixed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 5:20 PM UTC · Completed 5:37 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
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
Rename two leftover error message strings in installClaudeHooks that still referenced "settings" after the hooks rename: "creating temp settings file" → "creating temp hooks file" and "writing settings" → "writing hooks config". Addresses review feedback on #6405
Rename the local variable `settings` to `cfg` in GenerateHooksConfig to avoid shadowing the type name and align with Go naming conventions for config structs. Addresses review feedback on #6405
5f7adc4 to
94b5251
Compare
|
🤖 Review · Commit: |
94b5251 to
3af4958
Compare
waynesun09
left a comment
There was a problem hiding this comment.
Reviewed and validated the rebase over #6355:
- Re-applied the fix on top of the runtime-neutral hooks refactor (SandboxHookConfig / installHookScripts / HookPlan): GenerateHooksConfig renders from HookPlan, scripts land in claude-config/hooks/, wiring in claude-config/hooks.json via --settings. Full internal test suite + make lint green.
- Empirical verification with a branch-built CLI (
fullsend run triage --debug, real OpenShell sandbox):hook_registeredtelemetry + 108 hook executions in claude-debug.log, vsRegistered 0 hookson pre-fix main — the fix demonstrably loads the hooks. - Docs reconciled with the post-#6355 matrix/contract: PreToolUse rows back to ✓, PostToolUse rows still carry #6357, ADR 0090 consequence annotated (minor cross-ref note only).
- The earlier functional-tests failure is a flaky max_turns judge (flip-flopped across rename-only commits), not PR-caused.
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
Sandbox tool hooks (Tirith, SSRF, canary, secret redaction, unicode
normalization, context suppression, tool allowlist) were silently never
loaded: the hook wiring was written to /sandbox/workspace/.claude/settings.json
but Claude Code, started from /sandbox/workspace/<repo>, only reads project
settings from <cwd>/.claude/.
- Move hook scripts to {CLAUDE_CONFIG_DIR}/hooks/ and the wiring to
{CLAUDE_CONFIG_DIR}/hooks.json — runner-owned, outside the agent-writable
workspace tree (security.SandboxHooksDir / SandboxHooksSettings).
- buildRunCommand appends --settings {hooks.json} when RunParams.
HooksSettingsPath is set; run.go sets it when harness security is enabled.
--settings merges with (and takes precedence over) project/local settings,
so plugin state in {CLAUDE_CONFIG_DIR}/settings.json is unaffected.
- Rename GenerateClaudeSettings -> GenerateHooksConfig (still rendered from
HookPlan) and claudeSettings -> hooksConfig to match the new artifact.
- Add e2e behaviour scenario hooks-loaded.feature: SSRF PreToolUse hook
blocks a metadata-endpoint fetch end-to-end, guarding the "wired but
silently not loaded" regression class.
- Golden tests for --settings presence/absence/quoting; docs (runtimes.md
layout + matrix + contract, cli-internals.md, architecture.md) updated;
ADR 0090 consequence annotated as fixed.
Rebased over #6355 (runtime-neutral sandbox hooks contract) — reapplied on
top of SandboxHookConfig/installHookScripts/HookPlan. Original implementation
by the fullsend code agent in this PR's earlier history.
Verified locally with a branch-built CLI (fullsend run triage --debug):
hook_registered telemetry + 108 hook executions in claude-debug.log, versus
"Registered 0 hooks" on main before this fix.
Closes #6358
Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:59 PM UTC · Completed 9:14 PM UTC Commit: |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
docs/ADRs/0090-runtime-neutral-sandbox-hooks-contract.md(file-level): Line 58 · [medium] stale-reference
Stale reference to renamed GenerateClaudeSettings — the function was renamed to GenerateHooksConfig in this PR but line 58 in the ADR Decision section was not updated. The reference points to a function name that no longer exists.
Suggested fix: Change GenerateClaudeSettings to GenerateHooksConfig on line 58.
docs/ADRs/0090-runtime-neutral-sandbox-hooks-contract.md(file-level): Line 65 · [low] stale-reference
Stale reference to settings.json as Claude Code's hook wiring mechanism. After this PR, hooks are wired via hooks.json loaded through --settings.
Suggested fix: Update 'Claude Code: settings.json' to 'Claude Code: hooks.json (via --settings)' on line 65.
internal/security/hooks.go:54: [low] sandbox-escape
Hook scripts and hooks.json at /sandbox/claude-config/hooks/ are writable by the same sandbox user that runs the agent. A prompt-injected agent could theoretically overwrite its own security hooks. Hooks are defense-in-depth; the primary boundary is the OpenShell sandbox + L7 egress policy.
Suggested fix: Consider making hook files and directory read-only (chmod 444/chmod 555) after bootstrap.
docs/runtimes.md:60: [low] sandbox-escape
Pre-existing exposure: Claude Code still auto-loads a target repo's own .claude/settings.json hooks from cwd. The PR explicitly documents this as a separate concern. Not introduced by this PR.
Suggested fix: File a follow-up to assess whether repo-owned hooks should be removed or overwritten during bootstrap.
|
🤖 Finished Retro · ✅ Success · Started 9:31 PM UTC · Completed 9:55 PM UTC Commit: |
Retro: PR #6405 —
|
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)