refactor(runtime): make sandbox tool hooks and runner capabilities runtime-neutral - #6355
Merged
Merged
Conversation
PR Summary by QodoMake sandbox hooks and runner capabilities runtime-neutral
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Site previewPreview: https://44bf2a18-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 12:44 AM UTC · Completed 12:59 AM UTC Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Decision bundles runner contracts
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsLow
Previous runReviewFindingsLow
Labels: PR refactors sandbox hook infrastructure in the runner component and touches security hook wiring code |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
5 tasks
waynesun09
force-pushed
the
runtime-neutral-sandbox-hooks
branch
from
August 19, 2026 01:19
feaa4ec to
d448661
Compare
|
🤖 Review · Commit: |
5 tasks
…ntime-neutral Every new agent runtime (opencode #1260, cursor #6319, pi) tripped over the same Claude-specific seams: sandbox tool hooks were only installable through a ClaudeHooksBootstrap type assertion with the wiring living solely in the generated Claude settings.json, and the runner branched on rt.Name() == "claude" and hardcoded Claude artifact paths. - security: ClaudeSandboxHooks -> SandboxHookConfig; add HookPlan() as the single runtime-neutral wiring source (phase, tools, ordered scripts) and render GenerateClaudeSettings from it so they cannot diverge. - runtime: ClaudeHooksBootstrap -> SandboxHooksBootstrap; factor installHookScripts (any directory) and appendHookEnv out of installClaudeHooks; add optional capability interfaces DebugLogNamer and ContextBridger with helpers, implemented by ClaudeRuntime. - runner: resolve the runtime before starting the fetch service so the skill destination comes from rt.ConfigDir(); gate the CLAUDE.md bridge on ContextBridger; name the debug-log artifact via DebugLogNameFor; runtime-neutral --debug help text. - docs: sandbox hook contract (files, wiring plan, wire protocol), runtime-specific config-key support matrix, updated interface table in docs/runtimes.md; ADR 0090; architecture.md pointer. No behaviour change for the Claude runtime: same scripts, same settings.json, same claude-debug.log and CLAUDE.md bridge (golden tests unchanged). Docs record two pre-existing Claude Code gaps found while verifying the contract (settings.json not loaded from /sandbox/workspace/.claude — #6358; PostToolUse tool_response vs tool_result + parallel hooks — #6357) instead of a blanket ✓. Refs #6354 Assisted-by: Claude (fix), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
runtime-neutral-sandbox-hooks
branch
from
August 19, 2026 01:31
d448661 to
6cf88b1
Compare
|
🤖 Finished Review · ✅ Success · Started 1:33 AM UTC · Completed 1:48 AM UTC Commit: |
9 tasks
Contributor
|
The contract and HookPlan abstraction look solid. I dug into the OpenCode repo to verify the assumed wiring mechanisms, and wanted to share what I found for future implementers: OpenCode V1 has
Two gaps worth noting:
OpenCode V2 ( This matters for the security model: the hooks are defense-in-depth that should be enforced by the runtime, not opted into by the agent. A runtime without enforceable tool interception can't meet the contract. Consider making this explicit in ADR 0090 or runtimes.md — not just "a runtime that cannot MUST record the absence" but that a runtime without enforceable interception should not run in production until it does. None of this blocks this PR — the contract is correct regardless of wiring mechanism, and V1 is a viable starting point. Just flagging for the OpenCode integration work. |
maruiz93
approved these changes
Aug 20, 2026
9 tasks
|
🤖 Finished Retro · ✅ Success · Started 8:33 PM UTC · Completed 8:54 PM UTC Commit: |
waynesun09
added a commit
that referenced
this pull request
Aug 21, 2026
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>
waynesun09
added a commit
that referenced
this pull request
Aug 21, 2026
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>
waynesun09
added a commit
that referenced
this pull request
Aug 21, 2026
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>
Retro: PR #6355 — runtime-neutral sandbox hooksWorkflow shape: Issue #6354 → triage (run 32201426478) → human-authored PR #6355 → two review runs (32202300496, 32205257411) → two human approvals → merge. What went well:
Review quality gap — evidence for existing issues:
Post-merge functional test failure is a flaky infrastructure issue — the same SHA passed in the merge queue but failed on the main push, and the same 4-second crash pattern reproduces on unrelated branches. See proposal below. Proposals filed
Proposals skipped (target repo not allowed)File manually or update
|
waynesun09
added a commit
that referenced
this pull request
Aug 21, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the sandbox tool hooks and the runner's runtime-specific behaviour runtime-neutral so that OpenCode (#1260), Cursor CLI (#6319), pi and any future runtime can reuse the existing Tirith/SSRF/canary/redaction hooks instead of re-implementing or silently dropping them. No behaviour change for the Claude runtime.
Related Issue
Fixes #6354
Changes
ClaudeSandboxHooks→SandboxHookConfig(SandboxHookConfigFromHarness). NewHookPlan()returns the runtime-neutral wiring — orderedHookGroup{Phase, Tools, Scripts}— andGenerateClaudeSettingsis now rendered from it, so Claude'ssettings.jsonand any other runtime's adapter cannot diverge. Generated JSON is byte-identical (existing golden tests unchanged).ClaudeHooksBootstrap→SandboxHooksBootstrap;installHookScripts(sandbox, dir, cfg)(creates the dir, uploads + chmods the scripts anywhere) andappendHookEnvfactored out ofinstallClaudeHooks. New optional capability interfacesDebugLogNamer(+DebugLogNameFor, defaultagent-debug.log) andContextBridger(+NeedsClaudeMDBridge), implemented byClaudeRuntime.run.go): runtime is resolved before the fetch service starts soSkillDestDirisrt.ConfigDir()+"/skills"instead of the Claude constant; CLAUDE.md→AGENTS.md bridge is gated onContextBridgerinstead ofrt.Name() == "claude"; debug-log artifact name comes fromDebugLogNameFor(tx);--debughelp text is runtime-neutral.backendFromConfigFileis a pure config read, so the reorder only makes runtime-resolution failures surface before the fetch service is started.docs/runtimes.md— sandbox hook contract v1 (files, wiring plan, stdin/stdout/exit wire protocol as consumed by the scripts, fail modes split blocking-vs-sanitizing, env incl.FULLSEND_CANARY_TOKEN), runtime-specific config-key support matrix (also Track Cursor CLI as a supported agent runtime #6319's last AC), updated interface table and matrix rows; ADR 0090 (single decision: the hooks contract);architecture.mdpointer + open-question annotation;cli-internals.mdhelp text.--debuglog of the local run shows Claude Code registering 0 hooks — it probes$CLAUDE_CONFIG_DIR/settings.json, managed settings and<repo>/.claude/settings.json, never/sandbox/workspace/.claude/settings.jsonwhere the runner writes the hook wiring (CLI cwd is/sandbox/workspace/<repo>). Reproduced locally on 2.1.235: parent-dir settings → hook not invoked; cwd settings → PreToolUse blocks (exit 1 +{"decision":"block"}works). One-flag fix (--settings <file>) proposed in Sandbox tool hooks are never loaded: settings.json is written to /sandbox/workspace/.claude but Claude Code runs from /sandbox/workspace/<repo> #6358; kept out of this refactor PR. The security matrix Claude column now says "wired; not loaded (Sandbox tool hooks are never loaded: settings.json is written to /sandbox/workspace/.claude but Claude Code runs from /sandbox/workspace/<repo> #6358)" for every hook row instead of ✓, and the contract has a "Claude Code caveats" bullet.tool_resultbut Claude Code sendstool_response— sanitizers are inert under Claude Code #6357): while verifying the wire protocol against the Claude Code hooks reference we found a pre-existing mismatch — Claude Code sends PostToolUse output astool_response(scripts readtool_result), replaces output viahookSpecificOutput.updatedToolOutput, and runs matching hooks in parallel — so the PostToolUse sanitizers are wired but not effective under Claude Code today. Not this PR's regression and not fixed here (behaviour change, separate PR); the contract section carries an explicit caveat and the security matrix marks those Claude cells "wired, effectiveness unverified (PostToolUse sandbox hooks readtool_resultbut Claude Code sendstool_response— sanitizers are inert under Claude Code #6357)". Also from review:installHookScriptsshell-quotes paths and usessecurity.SandboxHooksDir,DebugLogNameFor(rt, tx)checks both backend components,WantsClaudeMDBridgenaming, HookPlan↔HookFiles all-enabled invariant test.Deliberately not in this PR:
ValidRuntimes()(stillclaude,dummy), any pi/opencode runtime code, and relaxingvalidModelName(/-less) — each noted in the docs as follow-ups.Only intentional non-Claude behaviour delta: a runtime without
DebugLogNamernow getsagent-debug.logas its local--debugartifact name (previously alwaysclaude-debug.log); Claude keepsclaude-debug.log.Testing
make lintpasses (staged, pre-commit + ADR linters + markdown links)HookPlanorder/consistency + all-disabled,SandboxHookConfigTirith defaults, hooks bootstrap with fakeopenshell(scripts uploaded/chmod'ed, settings.json still installed, hook env appended; no hooks without the extension; custom dir; upload/exec/chmod failure paths),DebugLogNameFor/NeedsClaudeMDBridge,newHarnessBootstrapround-tripgo test ./internal/{runtime,security,cli,fetchsvc,harness,config}/...green;go vet ./...cleango tool cover -func): new/changed functions 80–100% (installHookScripts81%,appendHookEnv90%,HookPlan/GenerateClaudeSettings/capability helpers 100%);installClaudeHooks65% only because of pre-existing, unchanged temp-file error branchesbehaviour,e2e,functional-tests,codecov/patchall green on the first pushfullsend run triage --forge github(fresh clone of fullsend-ai/agents, throwaway issue on a personal repo, Vertex SA creds,--debug --keep-sandbox --no-post-script) — sandbox bootstrapped withfullsendcross-compiled from this branch, all 7 hook scripts under/sandbox/workspace/.claude/hooks/(-rwx),settings.jsonrendered fromHookPlan(same matchers/order as before — and, as it turns out, not loaded by the CLI: Sandbox tool hooks are never loaded: settings.json is written to /sandbox/workspace/.claude but Claude Code runs from /sandbox/workspace/<repo> #6358), CLAUDE.md bridge injected, GitHub pre-flight OK, 17-turn Claude run produced a schema-validagent-result.json, transcript +claude-debug.logextracted, output scan ran. Sandbox deleted afterwards.make e2e-test(e2e/admin) not run locally — it exercisesadmin install/uninstall, not thefullsend runpath; CI'se2ejob passedTestDummyRuntime_Bootstrap/TestDummyRuntime_ClearIterationArtifactsfail on my machine identically on pristineorigin/main(a local OpenShell gateway is running, soexecon a nonexistent sandbox does not error) — environmental, unrelatedChecklist
refactor(runtime), no!— internal rename, no user-visible change)