feat(#608): diagnose un-normalized tool names in allowlist hook - #6520
Conversation
Site previewPreview: https://8fccba3e-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 12:53 PM UTC · Completed 1:07 PM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Looks good to me Previous runReviewFindingsLow
|
6b68a6e to
3cbd90d
Compare
|
🤖 Review · Commit: |
3cbd90d to
7688769
Compare
|
🤖 Review · Commit: |
Runtime adapters must translate their tool names to Claude vocabulary before the sandbox hooks run (ADR 0090); the pi adapter does. When one does not — an extension tool, a future adapter without a map, a pi version that adds a tool — the call was blocked with the same "NOT in the allowlist" reason as a genuinely forbidden tool, hiding the adapter gap behind a plausible allowlist denial. - tool_allowlist_pretool.py stays exact-match and fail-closed (no case-insensitive allowing). A blocked name that equals an allowlisted entry case-insensitively is now reported as a normalization gap with an ALLOWLIST_HOOK_ERROR reason that says which side is wrong: the runtime adapter (tool_name_unnormalized finding, severity high) when the allowlist entry is a canonical or legacy Claude name, the allowlist (allowlist_entry_unnormalized) when the tool name is the Claude one, and tool_name_case_collision when neither is. MCP names (mcp__*) are matched verbatim and keep the plain tool_blocked path. A non-string tool_name, or a JSON payload that is not an object, blocks with the JSON contract instead of a traceback. - security.CanonicalClaudeTools records the Claude Code tool names (verified 2026-08-23 against the live tools reference — latest release 2.1.241; no tool changes in the CHANGELOG since the pinned 2.1.234) and security.LegacyClaudeTools the names agents and adapters still use (LS, MultiEdit, Task, ...). The hook carries a mirror of both sets; TestToolAllowlistHook_VocabularyMatchesGo keeps them identical, TestHookPlan_ToolsAreCanonical pins every HookPlan tool (defaults and all hooks enabled), and the pi map tests check both directions and that every name is canonical-or-legacy Claude vocabulary (ls -> LS is a deliberate relaxation of "canonical only"). The pi maps themselves are untouched. - Tests cover every diagnostic branch end-to-end and, in-process, the findings.jsonl records (name/severity/action) for each path. - docs/runtimes.md and ADR 0090 describe the vocabulary as a test-checked reference (not runtime validation), the MCP caveat, and the renaming gap (MultiEdit vs Edit under pi) the case-variant diagnostic cannot see. Closes #608 Assisted-by: Claude (code, fix), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
7688769 to
5d908d6
Compare
|
🤖 Finished Review · ✅ Success · Started 1:38 PM UTC · Completed 1:55 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Verified: internal/security tests pass; the hook this PR modifies (tool_allowlist_pretool.py) is opt-in and disabled fleet-wide today (grepped all fullsend-ai/agents harnesses — zero enable it), so this is currently pure diagnostics/test-coverage with no live runtime effect. Force-enabled it locally (security.sandbox_hooks.tool_allowlist_pretool.enabled + FULLSEND_TOOL_ALLOWLIST) and re-ran the triage agent under the pi runtime end-to-end: hook roster confirmed active, pi's bash/read tool calls passed cleanly through the new normalization check with zero tool_name_unnormalized/allowlist_entry_unnormalized/tool_name_case_collision findings. CI green, review thread resolved.
|
🤖 Finished Retro · ✅ Success · Started 2:40 PM UTC · Completed 2:52 PM UTC Commit: |
Retro: PR #6520 — Diagnose Un-normalized Tool Names in Allowlist HookWorkflow Timeline
What Went Well
Observed Inefficiencies (Covered by Existing Issues)
Agents RepoAll agents resolved from Proposals filed
|
Summary
Runtime adapters must translate their tool names to Claude vocabulary before the sandbox hooks run (ADR 0090); the pi adapter does. When one does not — an extension tool, a future adapter without a map, a pi version that adds a tool —
tool_allowlist_pretool.pyblocked the call with the sameNOT in the allowlistreason as a genuinely forbidden tool, hiding the adapter gap behind a plausible allowlist denial. This PR makes that gap diagnosable without weakening the fail-closed, exact-match allowlist — no case-insensitive allowing.Closes #608
Changes
internal/security/hooks/tool_allowlist_pretool.py— a blocked name that equals an allowlisted entry case-insensitively is reported as a normalization gap with anALLOWLIST_HOOK_ERRORreason that says which side is wrong:bashvsBash,lsvsLS) → "the runtime adapter must translate it", findingtool_name_unnormalized(high, block);Bashvsbash) → "FULLSEND_TOOL_ALLOWLIST entry 'bash' is not canonical … fix the allowlist", findingallowlist_entry_unnormalized(high, block);tool_name_case_collision.MCP names (
mcp__*) are matched verbatim and keep the plaintool_blocked(critical) path. A non-stringtool_name, or a JSON payload that is not an object ([]/null/123— pre-existing gap), now blocks with the JSON contract instead of a Python traceback. The three new findings arehigh, notcritical, so an adapter gap does not trip critical-keyed escalation the way a forbidden tool does. Usescasefold(); when several entries fold together a known Claude name wins.internal/security/canonical_tools.go—CanonicalClaudeToolslists the tool names Claude Code exposes, verified 2026-08-23 against the live Claude Code tools reference (latest release, 2.1.241 at the time; the CHANGELOG records no tool additions/removals since the 2.1.234 pinned in the sandbox image — re-check on every pin bump);LegacyClaudeToolslists names agents and adapters still use (LS,MultiEdit→Edit,Task→Agent,TodoRead,NotebookRead→Read);KnownClaudeTool(). The hook carries a mirror of both sets (scripts cannot reach Go).TestToolAllowlistHook_VocabularyMatchesGoparses the embedded script and fails on any drift between the Go and Python sets;TestHookPlan_ToolsAreCanonicalpins everyHookPlantool (defaults and every hook enabled);pythonNameSetignores commented-out entries (own fixture test);TestCanonicalClaudeTools_WellFormed(no(/,, nomcp__, canonical ∩ legacy = ∅, legacy replacements canonical);internal/runtime/pi_agent_test.gochecks the pi maps in both directions and that every name is canonical-or-legacy Claude vocabulary — a deliberate relaxation of the issue's "canonical only" wording, because pi'slsmaps toLS, which Claude Code no longer sends (theMultiEditskip is now documented as a renaming gap the case-variant diagnostic cannot see). Python: 26 cases covering every branch via subprocess plus in-process assertions on thefindings.jsonlrecords (name/severity/action) for each path.docs/runtimes.mdhook-contract bullet and the ADR 0090 note describe the vocabulary as a test-checked reference (not run-time validation), the MCP caveat, and the renaming gap.Not changed:
FULLSEND_TOOL_ALLOWLISTparsing, fail-closed-when-unset, the pi alias maps inpi_agent.go.Testing
pytest internal/security/hooks/tool_allowlist_pretool_test.py— 26 passgo test ./internal/security/ ./internal/runtime/— pass; mutation check: adding a bogus name to the Python set failsTestToolAllowlistHook_VocabularyMatchesGogo vet,gofmt,ruff check/format,ty, bandit, ADR + docs-link lints (pre-commit on changed files) — cleanPost-script verification
agent/608-tool-allowlist-diagnostic)997021c226272037a8cb69b32c73e14f26073b5e..HEAD)