feat(413): tool registry, policy engine, and approvals strip - #418
Conversation
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
Documentation Review — PR #418 (Plan 413: Tools, Policy, Approvals)
Must-Fix
D1. Privacy section omits tool-investigation data flow
docs/ai-agents.md:247-259, docs/llm-providers.md:333-344
The privacy sections list incident titles, service names, alert names, and cluster IDs as the data sent to remote providers. With tool investigation, the model now autonomously fetches and sends back substantially more data: full incident payloads, alert details, notes, cluster metadata, service logs (up to 8 KB each per MaxResponseBytes), and limited-support reasons — all flowing through the Anthropic API as tool-use turns (pkg/tui/investigation.go:92-126). Neither privacy section mentions this.
Failure scenario: A user reads the privacy section, concludes only incident titles/IDs leave the machine, and enables an Anthropic-family provider in a restricted environment. The watcher autonomously pulls and transmits service logs and cluster metadata they did not intend to share.
Verified by reading: code in investigation.go and registry.go:97-106.
D2. Approvals strip says "press A" but no key handler exists
pkg/tui/approvals.go:85, pkg/tui/tui.go (missing handler)
Render() displays " ⚑ %d asks — press A " and RenderExpanded() shows [Enter] Accept [d] Dismiss [Esc] Close. No corresponding key handlers exist in tui.go for A, Enter, d, or Esc in an approvals context. The strip renders but cannot be interacted with. This is a functional gap surfaced as a docs issue — the UI text documents interaction that doesn't work.
Note: Currently all 7 tools are ClassRead (always Allow), so Ask decisions never fire and the strip never appears from policy. But tui.go:729 adds asks from actionable verdicts, so the strip can appear. If it does, the user sees "press A" and nothing happens.
Failure scenario: An actionable investigation verdict adds an ask to the strip. The user sees "press A" and presses it. Nothing happens.
Verified by: grep -rn for key handlers matching A/Enter/d/Esc in an approvals context — none found.
D3. Five new config keys missing from docs/configuration.md and README.md
docs/configuration.md:62-68, README.md:85-99
docs/configuration.md is the central config reference (README links to it as "the full reference"). Its AI Agents section only lists agent_cli_command, agent_system_prompt, and watcher_system_prompt. None of the five new keys appear: watcher_max_tool_turns, watcher_investigation_timeout, ai_permission_mode, ai_auto_allow_tools, ai_allowed_command_prefixes.
README.md has a comparable config table (lines 85-99) that also omits all five keys.
The keys ARE documented in docs/ai-agents.md:191-198 with correct defaults, so the information exists — but not in the two reference files users consult first.
Failure scenario: A user reads docs/configuration.md or the README to find all available config keys. They don't discover ai_permission_mode and unknowingly run with the default interactive mode when they intended plan.
Verified by: grep for all five key names in configuration.md and README.md — zero hits.
D4. PR body cites 5 test names that don't exist
PR body, Criterion → Test traceability table
| PR body claims | Actual test name | File |
|---|---|---|
TestGetIncident_ErrorResponse |
TestHandler_ErrorDoesNotLeakInternals |
handlers_test.go:213 |
TestGetIncident_Truncation |
TestGetServiceLogs_Truncation + TestHandler_TruncationMarker |
handlers_test.go:143,201 |
TestGetIncident_InvalidJSON |
TestHandler_InvalidJSON |
handlers_test.go:190 |
TestDecide (21 cases) |
TestDecide_Exhaustive (19 cases) |
policy_test.go:10 |
TestParseWatcherVerdict table |
6 separate TestParseWatcherVerdict_* functions |
verdict_test.go:10-63 |
Failure scenario: A reviewer runs go test -run TestGetIncident_ErrorResponse to verify the token-leak property, gets 0 tests matched, and has no way to trace the claim. A future contributor reads "21 cases" and wonders why they count 19.
Verified by running: grep -rn "func TestGetIncident_ErrorResponse" → 0 hits, etc.
D5. PR body missing required sections
PR body
| Required section | Present? |
|---|---|
| Problem | No (uses ## Summary instead) |
| Approach | No (merged into Summary) |
## Testing this live (must mention --dev) |
No |
## Visual validation |
No |
| UNVERIFIED resolved | No |
The PR has a detailed traceability table and test plan, but omits the live-testing and visual-validation sections that would confirm the approvals strip and investigation loop were exercised in the real TUI.
D6. auto and custom modes are code-identical but documented differently
pkg/ai/policy/policy.go:61-77, docs/ai-agents.md:128-129
ModeAuto (lines 61-68) and ModeCustom (lines 70-77) have character-for-character identical logic. The docs describe auto as "Tools on the allowlist execute without prompting" and custom as "Fully user-defined allowlist" — but both do the same thing: ClassRead → Allow, allowlisted → Allow, else Ask. A user choosing between them has no way to know they're equivalent.
Failure scenario: A user configures custom expecting different behavior from auto, discovers it's identical, and files a bug.
Verified by: diff of the two switch branches — only the case label differs.
Nice-to-Have
D7. Plan 413 has no post-mortem section
docs/plans/413-tools-policy-approvals.md
Other recent plans (411, 412) include ## Post-mortem / Lessons learned. Plan 413 has deviations (e.g., get_recent_events omitted because pkg/delta never shipped) but no post-mortem. The deviation about plan 412 not delivering pkg/delta would be a natural lesson to capture.
D8. "(unlimited)" warning in config description is misleading
pkg/config/config.go:68
The description says "Never set to 0 (unlimited)." — the parenthetical implies 0 means unlimited. But both pkg/tui/model.go:357 (if v > 0) and investigation.go:67 (if cfg.maxToolTurns <= 0 { cfg.maxToolTurns = 6 }) clamp 0 to 6. Setting it to 0 doesn't produce unlimited behavior — the description says a bad thing happens that the code prevents.
D9. onAsk callback is nil in watcher invocation
pkg/tui/watcher.go:250
watcherInvestigateCmd is called with nil for the onAsk parameter. If a tool ever triggers an Ask policy decision, the approval request is silently swallowed — the approvals strip won't receive it. Currently safe because all 7 tools are ClassRead (always Allow), but this is a landmine for phase 4 when write-class tools are added.
Not strictly a docs issue, but the docs describe a functioning approvals flow for Ask decisions (docs/ai-agents.md:122-130) when the wiring to surface them is incomplete.
Clean
- Config defaults: All five new keys have defaults matching between
pkg/config/config.goanddocs/ai-agents.md. ✓ - Tool-support-per-provider table:
docs/llm-providers.md:318-330exists and matchesisAnthropicFamily()atinvestigation.go:153-159. ✓ make quickstart-check: Passes correctly — no keymap/chords files were modified. ✓- Plan doc: Present at
docs/plans/413-tools-policy-approvals.mdwith problem, solution, testing, files, and deviations. No UNVERIFIED markers remain. ✓ - Non-Anthropic degradation: Documented in
ai-agents.md:131andllm-providers.md:320, matches code. ✓
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
PR #418 Review — Plan 413: Tool Registry, Policy Engine, Approvals Strip
Verdict: MUST-FIX items block merge
MUST-FIX
1. Approvals strip Accept is a no-op — Action callback never wired
Severity: high | pkg/tui/tui.go:729-733
When an actionable investigation verdict fires, the Ask struct is created with Kind, Title, and Body but the Action field (a func() tea.Cmd) is never set:
m.approvals.Add(Ask{
Kind: AskDraftNote,
Title: msg.verdict.Summary,
Body: msg.verdict.Action, // this is the string body, NOT the func
})approvals.go:56-65 — Accept() calls ask.Action() only when non-nil, so Accept silently removes the item and does nothing. The PR claims "Accept on DraftNote actually invokes the existing PD add-note path" but no such wiring exists.
Failure scenario: User sees an actionable AI recommendation, presses Accept → item disappears, no note is added to PagerDuty. The feature is decorative.
Verified by reading: production code only. No test exercises the Accept→PD path because the path does not exist.
2. No key handler for approvals strip — "press A" is a lie
Severity: high | pkg/tui/approvals.go:85, no corresponding handler anywhere
Render() displays " ⚑ N asks — press A " but there is zero key handling for the approvals strip in the Update loop. Confirmed by:
grep -rn 'approvals\.\(Accept\|Dismiss\|RenderExpanded\)' --include='*.go' | grep -v _test.go
# (no results)
Accept(), Dismiss(), and RenderExpanded() are never called from production code. The only production callers are Add() at tui.go:729, Count() and Render() at views.go:173-174.
Failure scenario: User sees the approvals bar, presses A → nothing happens. No way to interact with approval items.
3. onAsk is nil at the only call site — Ask decisions create no approval items
Severity: medium | pkg/tui/watcher.go:250
The investigation is launched with onAsk as nil:
cmds = append(cmds, watcherInvestigateCmd(
m.toolRunnerFactory,
m.toolRegistry,
m.investigationCfg,
m.watcherSystemPrompt,
obs.Summary,
contextStr,
"",
nil, // ← onAsk
))When the policy engine returns Ask for a tool call, gatedTool.Execute at registry.go:144 checks if g.onAsk != nil — it's nil, so no approval item is created. The tool returns "Awaiting user approval" text to the LLM and the investigation continues, but the user never sees the request.
This is less severe than #1-#2 because in the current phase all 7 tools are ClassRead, and ClassRead always returns Allow in every mode (confirmed by TestDecide_ClassReadAllowedInEveryMode). So Ask cannot fire for any registered tool in this phase. But the infrastructure is wired for future phases and will silently fail to surface Ask items.
4. PR traceability table cites 3 fabricated test names
Severity: medium — repeat of the pattern from the prior PR
| Cited Name (fabricated) | Actual Test Name |
|---|---|
TestGetIncident_ErrorResponse |
TestHandler_ErrorDoesNotLeakInternals (handlers_test.go:213) |
TestGetIncident_Truncation |
TestHandler_TruncationMarker (handlers_test.go:201) or TestGetServiceLogs_Truncation (handlers_test.go:143) |
TestGetIncident_InvalidJSON |
TestHandler_InvalidJSON (handlers_test.go:190) |
All three follow the same hallucination pattern: TestGetIncident_ prefix combined with a suffix from an unrelated test function.
Additionally, TestDecide should be TestDecide_Exhaustive with 19 cases (not 21 as claimed), and TestParseWatcherVerdict is not a single table test but 6 separate functions (_ValidTiers, _MissingBlock, _MalformedJSON, _ExtraKeysIgnored, _EmptyInput, _UnknownTier).
5. No test exercises watcherInvestigateCmd or verifies that policy.Decide() is wired in production
Severity: medium | pkg/tui/investigation.go:71-73
The headline test (TestHeadline_DenyEverything_ZeroHandlerInvocations) uses its own denyAll closure passed directly to GatedBetaTools(). It does NOT call policy.Decide(). The production code at investigation.go:71-73 wraps policy.Decide(cfg.policyConfig, ...) in a closure — that wiring is untested.
If someone deleted lines 71-73 and replaced the decide closure with func(...) policy.Decision { return policy.Allow }, no test would fail. Confirmed by:
- No test file calls
policy.Decide()(onlypolicy_test.gotests it in isolation) - No test in
pkg/tui/exerciseswatcherInvestigateCmd - The headline test constructs its own registry, tools, and decide function independently of the production path
The test proves the gatedTool.Execute() mechanism works (which is real code at registry.go:136-160), so the enforcement layer itself is sound. But the production wiring to that layer is an untested seam — exactly the "unwired code" failure shape documented in this project's history.
NICE-TO-HAVE
6. RenderExpanded() is dead code
Severity: low | pkg/tui/approvals.go:94-125
RenderExpanded() is defined but never called from any production code. It renders a selectable list with [Enter] Accept [d] Dismiss [Esc] Close instructions, but since no key handler exists (finding #2), this code path is unreachable. Should either be wired or removed.
CLEAN (verified)
-
Tool registry: All 7 read-class tools registered (
get_incident,get_alerts,get_notes,list_queue,get_cluster_info,get_service_logs,get_limited_support). Each handler calls a real PD/OCM interface method, not stubs. Duplicate-name rejection works (registry.go:44). 8192-byte truncation with\n[truncated]marker applied in both gated and ungated paths. -
Policy engine:
Decide()is pure — no I/O (policy.go:43). Default mode isModeInteractive.ClassRead→Allowin every mode. 19 exhaustive test cases cover all Mode×Class combinations plus edge cases (empty tool name → Deny, unknown mode → Deny). Allowlist logic tested separately. -
Error sanitization:
formatErrorathandlers.go:215-217uses blank identifier_to drop raw error text entirely. Tests confirm no internal text leaks. -
ParseWatcherVerdict: Pure function, no I/O (
verdict.go:36-53). Three tiers: Silent, Noteworthy, Actionable. Malformed JSON → Noteworthy (tested atverdict_test.go:42-47). Missing block → Noteworthy. Unknown tier → Noteworthy. All paths return nil error. -
Investigation loop guards:
maxToolTurnsdouble-guarded against 0 (config load atmodel.go:357rejectsv <= 0; runtime guard atinvestigation.go:67-69snaps to 6). Timeout enforced viacontext.WithTimeoutatinvestigation.go:104-105, default 90s. One-at-a-time viawatcherAnalyzingflag (safe because Bubble Tea Update is single-threaded). -
Non-Anthropic degradation:
isAnthropicFamily()atinvestigation.go:153-159gates tool registration. Non-Anthropic providers get no tools, no errors, one log line (guarded bytoolsLoggedOnceatmodel.go:959). Falls through to synthesis path atwatcher.go:253. -
Config keys: All 5 keys (
watcher_max_tool_turns,watcher_investigation_timeout,ai_permission_mode,ai_auto_allow_tools,ai_allowed_command_prefixes) registered inDefaultOptionalKeyswith sensible defaults, consumed inresolveInvestigationConfig()/resolveAIPermissionConfig()atmodel.go:355-383, documented indocs/ai-agents.mdandconfig.go. -
get_recent_eventsomission:pkg/deltadoes not exist. Omission documented in PR body's Deviations section. Noget_recent_eventstool implementation exists in the codebase. -
TestRevertCheck_PolicyGate: Proves the ungated path fires handlers, confirming the headline test has teeth for the gating mechanism (though not for the production wiring — see finding #5).
Summary
| # | Finding | Severity | Verified by |
|---|---|---|---|
| 1 | Accept is a no-op — Action never wired | must-fix | reading production code |
| 2 | No key handler for approvals — "press A" inert | must-fix | grep for Accept/Dismiss/RenderExpanded callers |
| 3 | onAsk nil at call site — Ask items never surface |
must-fix | reading watcher.go:250 |
| 4 | 3 fabricated + 2 inaccurate test names in PR body | must-fix | grep for each test name |
| 5 | Production Decide() wiring untested |
must-fix | no test calls policy.Decide() or watcherInvestigateCmd |
| 6 | RenderExpanded() is dead code |
nice-to-have | grep for callers |
The core infrastructure (registry, gating mechanism, policy engine, verdict parsing, investigation loop bounds) is solid. The gap is in the last mile: the approvals strip renders but cannot be interacted with, and the production wiring of the policy engine to the investigation loop has no test coverage.
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
V5: Dead Code and Unused Surface — PR #418
deadcode tool output (full)
pkg/ai/mock.go:18:24: unreachable func: MockProvider.SupportsStreaming
pkg/ai/mock.go:23:6: unreachable func: NewMockProvider
... (6 mock funcs — pre-existing)
pkg/config/config.go:478:6: unreachable func: BuildFullConfig
pkg/config/config.go:705:6: unreachable func: WriteConfigTeams
pkg/config/config.go:730:6: unreachable func: WriteConfigKey
pkg/config/config.go:755:6: unreachable func: WriteConfigMap
pkg/ocm/client.go:174:6: unreachable func: NewClient
pkg/pd/mock.go:96-346: (16 mock funcs — pre-existing)
pkg/tui/chords.go:108:6: unreachable func: chordHelpText
pkg/tui/commands.go:69:20: unreachable func: execErr.Error
pkg/tui/commands.go:80:20: unreachable func: execErr.Code
pkg/tui/commands.go:544:6: unreachable func: AssignedToAnyUsers
pkg/tui/commands.go:557:6: unreachable func: ShouldBeAcknowledged
All 32 unreachable functions are pre-existing on origin/main. This PR adds zero new items to the deadcode list. ✓
New findings from this PR
MUST-FIX
1. Registry.BetaTools() — exported, zero production callers
pkg/ai/tools/registry.go:62 — BetaTools() returns []anthropic.BetaToolUnionParam and has no production callers. It is only referenced in registry_test.go:53. Production uses GatedBetaTools() (in investigation.go:75) and AsBetaTools() (test-only revert check). This is the exact founding-failure pattern: fully implemented, fully tested, zero callers.
Failure scenario: A developer reads BetaTools() as the primary API and builds code against it, bypassing the policy gate entirely — the very thing this PR exists to prevent.
Verified by: grep -rn '\.BetaTools()' --include='*.go' | grep -v _test.go → empty output.
2. Registry.AsBetaTools() — exported, zero production callers
pkg/ai/tools/registry.go:110 — AsBetaTools() returns ungated tools. Its only caller is the integration revert-check test (integration_test.go:90). No production code calls it.
Failure scenario: Same as above — an ungated API surface exists with no policy enforcement, inviting accidental bypass.
Verified by: grep -rn '\.AsBetaTools()' --include='*.go' | grep -v _test.go → empty output.
3. Approvals strip: Accept(), Dismiss(), RenderExpanded() — zero production callers, no key handler
pkg/tui/approvals.go:56(Accept) — test-onlypkg/tui/approvals.go:68(Dismiss) — test-onlypkg/tui/approvals.go:94(RenderExpanded) — never called anywhere
The strip renders via Render() in views.go:174, showing "⚑ N asks — press A", but no key handler in tui.go or msgHandlers.go processes the 'A' key for approvals. Asks accumulate but the user can never accept or dismiss them. The Accept method would also be a no-op because the production Ask struct (tui.go:729-733) never sets the Action callback field.
Failure scenario: User sees "⚑ 3 asks — press A" in the TUI, presses A, nothing happens. Asks pile up with no way to clear them.
Verified by: grep -rn '\.Accept\|\.Dismiss\|RenderExpanded' --include='*.go' | grep -v _test.go → zero hits. Searched all key handlers for 'A' or approval dispatch — none found.
NICE-TO-HAVE
4. Config.AllowedCommandPrefixes — struct field populated but never read by Decide()
pkg/ai/policy/policy.go:38 — The field is populated from viper.GetStringSlice("ai_allowed_command_prefixes") at model.go:382, but Decide() never inspects it. The config comment says "unused until phase 415, defined for schema stability." This is fine as documented forward-planning, but worth noting: the field is dead weight today and go vet won't catch it since it's in a public struct.
Verified by: grep -n 'AllowedCommandPrefixes' pkg/ai/policy/policy.go → only the field declaration at line 38, never referenced in Decide().
5. ModeAuto and ModeCustom — identical logic
pkg/ai/policy/policy.go:61-78 — The ModeAuto and ModeCustom branches in Decide() are textually identical (confirmed via diff). There's currently no behavioral distinction between them.
Verified by: diff of the two case branches → identical except the case label.
6. investigationMsg.fullText — struct field set but never read
pkg/tui/investigation.go:27 — The field is populated at investigation.go:134 but the handler at tui.go:708-738 never reads it. The full model response text is computed, stored in the message, and discarded.
Verified by: grep -n 'fullText' pkg/tui/tui.go → zero hits.
7. formatError silently drops the error argument
pkg/ai/tools/handlers.go:215 — func formatError(msg string, _ error) string discards the error. Every handler call site passes a real err that gets swallowed. The LLM sees "incident not found" with no detail about why.
Read from code, not verified at runtime.
8. AskSuggestedCommand, AskEscalationSuggestion, AskToolPermission — enum values with zero production usage
pkg/tui/approvals.go:17-19 — Three of four AskKind constants exist only in the definition and askKindLabel() switch. Only AskDraftNote is used in production (tui.go:730).
Verified by: grep -rn 'AskSuggestedCommand\|AskEscalationSuggestion\|AskToolPermission' --include='*.go' | grep -v _test.go → only approvals.go definition/switch.
9. ClassWriteLocal, ClassExec, ClassExternal — policy classes with zero production usage
pkg/ai/policy/policy.go:11-13 — All registered tools use ClassRead. The entire non-trivial logic in Decide() (the ModeInteractive/ModeAuto/ModeCustom branches for non-Read classes) is unreachable in the current codebase.
Verified by: grep -rn 'ClassWriteLocal\|ClassExec\|ClassExternal' --include='*.go' | grep -v _test.go | grep -v policy.go → empty.
10. onAsk callback always nil in production
pkg/tui/watcher.go:250 — The only production call to watcherInvestigateCmd passes nil for onAsk. When the gated tool returns Ask from the policy engine, it returns a "waiting for approval" text response to the LLM but never surfaces the ask to the user via the approvals strip. This makes the ModeInteractive Ask path a silent dead end for tool calls.
Verified by: grep -A10 'watcherInvestigateCmd(' pkg/tui/watcher.go → last param is nil.
Summary
| Category | Count | Items |
|---|---|---|
| Deadcode tool (pre-existing) | ~32 | All on origin/main, none added by PR |
| Exported with zero prod callers (new) | 2 | BetaTools(), AsBetaTools() |
| Methods with zero prod callers (new) | 3 | Accept(), Dismiss(), RenderExpanded() |
| Struct field set, never read (new) | 2 | AllowedCommandPrefixes, investigationMsg.fullText |
| Identical code branches (new) | 1 | ModeAuto ≡ ModeCustom |
| Unreachable by construction (new) | 2 | Non-Read policy classes, onAsk nil |
| Enum values never used (new) | 3 | AskSuggestedCommand, AskEscalationSuggestion, AskToolPermission |
Items 4-10 are defensible as forward-planning scaffolding (the plan doc references later phases). Items 1-3 are the classic "unwired code" anti-pattern this project has been burned by before — especially the approvals strip, which renders a UI affordance the user cannot interact with.
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
PR #418 Review — Tools Registry, Policy Engine, Approvals Strip
Headline criterion
PASS. A Deny-everything policy produces zero handler invocations. Verified by running TestHeadline_DenyEverything_ZeroHandlerInvocations and inspecting the gated-tool Execute() path at pkg/ai/tools/registry.go:136-159. The revert-check test (TestRevertCheck_PolicyGate) also proves the gate is load-bearing.
Build: PASS (go build ./... succeeds).
Must-fix
B1. Approvals strip is unwired — asks accumulate without limit, no user action possible
File: pkg/tui/approvals.go:42-49 (Add), pkg/tui/views.go:173 (Render)
Severity: Feature is broken; UX lie; unbounded memory growth
approvalsStrip.Add() is called from the investigationMsg handler (tui.go:729) when an investigation returns an actionable verdict. The strip renders "press A" (approvals.go:85). But:
- No key handler is wired for 'A' (or any key) to open the expanded approvals view. The lowercase
'a'is "acknowledge incident" (keymap.go:199). Accept()andDismiss()are never called from any production code path — only from tests.RenderExpanded()is never called from production code.- The
asksslice grows without bound. Every actionable investigation appends an Ask. There is no cap, no eviction, and no way for the user to dismiss them.
Failure scenario: User runs srepd with an Anthropic provider for a multi-hour shift. The watcher detects 20 actionable patterns. 20 Asks accumulate in memory with no way to act on them. The strip shows "20 asks — press A" but pressing A acknowledges the highlighted incident instead. The Asks hold closures (Action func() tea.Cmd) that may reference stale model state.
The tests pass because they test the data structure in isolation, not its integration with the TUI. This is exactly the "unwired code" pattern from this project's history.
B2. Investigation goroutine outlives session — context.Background() cannot be cancelled
File: pkg/ai/tools/investigation.go:104
Severity: Goroutine leak on exit
ctx, cancel := context.WithTimeout(context.Background(), cfg.timeout)
defer cancel()The investigation runs inside a tea.Cmd closure. Bubble Tea's Program.Quit() does NOT cancel outstanding tea.Cmd goroutines — they run until they return. With context.Background(), the only backstop is the 90-second timeout.
Failure scenario: User presses ctrl+c to quit srepd while a watcher investigation is in-flight (making HTTP calls to the Anthropic API). The process appears to hang for up to 90 seconds because the goroutine is blocked inside toolRunner.NextMessage(ctx) waiting for the API. The cancel() is only called when the goroutine finishes — it can't be triggered externally.
This matches the documented bug pattern: "A goroutine still pinned after its caller returned."
Fix: Derive the context from a model-scoped or program-scoped context that is cancelled on quit. The model already has watcherStreamCancel for stream queries — investigation needs the same pattern.
B3. onAsk is nil in runDetectors path — Ask decisions produce a dead-end message with no follow-up
File: pkg/tui/watcher.go:250, pkg/ai/tools/registry.go:143-149
Severity: Silent failure for non-Read tools in future phases
When runDetectors calls watcherInvestigateCmd, it passes nil for onAsk:
cmds = append(cmds, watcherInvestigateCmd(
...
nil, // onAsk
))When the gated tool's Execute() receives an Ask decision (registry.go:143), it:
- Skips the nil
onAskcallback (no approval request created) - Returns
"Awaiting user approval — this action requires confirmation"as the tool result
The SDK tool runner receives this string as the tool's output and may continue the conversation loop. The model sees "awaiting approval" but no approval mechanism exists in the investigation — the tool will never be re-tried with approval. The investigation burns its remaining tool turns with the model potentially re-requesting the same tool.
Impact for phase 3: None — all 7 registered tools are ClassRead, which gets Allow in every mode except default (unknown mode). This becomes a must-fix when phase 4/5 adds ClassWriteLocal or ClassExec tools.
Downgrading to nice-to-have for this PR since all current tools are read-class. But document it.
Nice-to-have
N1. ModeAuto and ModeCustom are identical in Decide()
File: pkg/ai/policy/policy.go:61-77
Verified by reading: The switch branches for ModeAuto and ModeCustom contain the exact same logic. AllowedCommandPrefixes is declared in the Config struct, populated from viper (model.go:382), documented in config.go, but never read in Decide().
The comment at config.go:70 says "unused until phase 415, defined for schema stability." This is intentional scaffolding — not a bug. But having two enum values with identical behavior is confusing. Consider either:
- Removing
ModeCustomuntil it diverges, or - Adding a comment to the
ModeCustombranch explaining what will differ
N2. formatError discards the actual error
File: pkg/ai/tools/handlers.go:215
func formatError(msg string, _ error) string {
return msg
}This is intentional (test TestHandler_ErrorDoesNotLeakInternals validates it), but it makes debugging harder. The error is already logged at the call site via log.Warn in some paths but not in the handler. Consider logging the error inside formatError before discarding it.
N3. Truncate() panics on maxBytes < len(marker)
File: pkg/ai/tools/registry.go:184-190
Verified by running: Truncate("hello world", 5) panics with slice bounds out of range [:-7]. Production always passes MaxResponseBytes = 8192, so this can't trigger today. But the function is exported and accepts int — a defensive if maxBytes < len(marker)+1 { return s[:maxBytes] } guard prevents a future surprise.
N4. investigation_test.go only tests helpers, not the investigation loop
File: pkg/tui/investigation_test.go
The test file covers isAnthropicFamily and defaultInvestigationConfig but NOT watcherInvestigateCmd. The core investigation loop — which does API calls, tool execution, verdict parsing, and error handling — has zero test coverage. A mock ToolRunnerFactory would be straightforward.
This is the "unwired code" pattern: the test proves the helpers work, but a complete stub of watcherInvestigateCmd would not break any test.
N5. AsBetaTools() exports an ungated path
File: pkg/ai/tools/registry.go:110
AsBetaTools() returns tools that execute without any policy check. It's not called from production code today (only from TestRevertCheck_PolicyGate), but it's exported. Any future caller gets policy bypass. Consider making it unexported (asBetaTools) or adding a build-tag restriction.
Clean
- Concurrency in the Update loop: The
watcherAnalyzingguard is correctly single-threaded (all reads/writes in the Update loop). No data race. - Registry mutex:
sync.RWMutexcorrectly protects concurrent access to the tools slice.GatedBetaToolssnapshots by value under RLock. - Policy engine purity:
Decide()is pure — no I/O, no side effects, no global state. Exhaustive test coverage across all modes and classes. - Handler error paths: All handlers return
(string, nil)on API errors viaformatError, preventing the SDK tool runner from seeing Go errors for expected conditions (not-found, invalid input).marshalResultcan return a real error on marshal failure, which correctly propagates. - Tool registration: Duplicate names are rejected. Insertion order is preserved.
Tools()returns a copy. - Verdict parser: Gracefully defaults to
TierNoteworthyon malformed/missing JSON blocks. - Race detector: All tests pass with
-race— no data races detected (pkg/ai: 1.3s,pkg/ai/policy: 1.0s,pkg/ai/tools: 1.0s,pkg/tui: 93.0s).
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
PR #418 Code Review — Tools Registry, Policy Engine, Approvals Strip
Summary
This is well-structured, idiomatic Go. The layering is clean (pure policy engine → registry → gated tools → TUI wiring), the headline test has genuine teeth (verified by the revert-check test), and the exported surface is minimal. Seven read-class tools, a pure Decide() function, and a TUI approvals strip — nothing over-engineered.
Below are findings ranked by severity. Where I ran something to verify, I say so.
Must-fix
1. formatError discards the error — LLM gets no diagnostic signal
pkg/ai/tools/handlers.go:215
func formatError(msg string, _ error) string {
return msg
}Every handler calls formatError("incident not found", err) but the error is silently dropped. The LLM sees only "incident not found" — no hint whether the cause was a network timeout, auth failure, or a genuinely missing ID. The plan doc (line 48-51) says this is deliberate to prevent token leaks, which is a valid posture for detailed stack traces, but the err itself is usually just "not found" or "context deadline exceeded" — one-line strings the LLM could use to decide whether to retry.
Failure scenario: OCM returns a transient auth error. The LLM sees "cluster not found" and tells the user the cluster doesn't exist, when in fact it's an auth problem that would succeed on retry. The SRE acts on wrong information.
Recommendation: Include the error class (timeout, auth, not-found) without the full message: return fmt.Sprintf("%s: %s", msg, classifyErr(err)). Or at minimum, log the full error (it's currently lost entirely).
Verified by reading: confirmed _ error is unused and no log call captures it.
2. ModeAuto and ModeCustom are identical — dead code path
pkg/ai/policy/policy.go:61-78
The case ModeAuto and case ModeCustom branches in Decide() have byte-identical logic. AllowedCommandPrefixes is declared in Config (line 38) and populated from config (model.go:382) but never read by Decide(). If ModeCustom exists to eventually use command prefixes, it should be documented as a placeholder or removed.
Failure scenario: A user sets ai_permission_mode: custom expecting differentiated behavior from auto. They get identical behavior — no indication anything is different. When command-prefix enforcement lands later, its absence now is invisible.
Verified by grep: AllowedCommandPrefixes has exactly 2 references — the struct field and the viper read. Zero reads in Decide().
3. Truncate panics on small maxBytes
pkg/ai/tools/registry.go:184-190
func Truncate(s string, maxBytes int) string {
if len(s) <= maxBytes {
return s
}
const marker = "\n[truncated]"
return s[:maxBytes-len(marker)] + marker // panics if maxBytes < 12
}Truncate is exported. If called with maxBytes < len(marker) (12), the slice expression goes negative and panics. With MaxResponseBytes = 8192 this can't happen today, but the function is public API. Per CONVENTIONS.md: "never panic in library code."
Failure scenario: A future caller passes a small max (e.g., for a summary field). Runtime panic in the TUI.
Verified by reading: no bounds guard exists.
Nice-to-have
4. RenderExpanded has zero callers — dead code
pkg/tui/approvals.go:94
RenderExpanded is defined but never called anywhere in the codebase. View() only calls Render() (the collapsed strip). If the expanded panel is planned for a future PR, a comment or TODO would help; otherwise it's dead weight.
Verified by grep: exactly 1 hit (the definition itself).
5. watcherInvestigateCmd and initToolRegistryForModel have no tests
The investigation loop (investigation.go:49-137) is the most complex new function: it builds gated tools, constructs SDK params, runs NextMessage in a loop, and parses the verdict. It has zero test coverage — only isAnthropicFamily and defaultInvestigationConfig are tested. The initToolRegistryForModel wiring function is also untested.
This falls into the repo's "unwired code" failure pattern: watcherInvestigateCmd compiles and is called from watcher.go:242, but no test exercises the path from observation → investigation → verdict → TUI state. A bug in the NextMessage loop or context wiring would be invisible.
Failure scenario: A future SDK upgrade changes BetaToolRunner behavior. All tool/policy unit tests pass. The investigation loop silently breaks.
Verified by grep: zero matches for watcherInvestigateCmd or investigationMsg in any *_test.go file.
6. extractToolRunnerFactory uses untyped interface assertion
pkg/tui/investigation.go:141-149
func extractToolRunnerFactory(provider interface{}) ToolRunnerFactory {
type betaMessagesProvider interface {
BetaMessages() *anthropic.BetaMessageService
}
...
}The provider interface{} parameter loses type information. If ai.Provider grew a BetaMessages() method on the interface, this local type assertion would become unnecessary. As-is, it works correctly via structural typing, but using any for a parameter that's always ai.Provider is a mild idiom deviation.
Cosmetic — no runtime impact.
7. Decide takes json.RawMessage it never uses
pkg/ai/policy/policy.go:43
func Decide(cfg Config, toolName string, class Class, _ json.RawMessage) Decision {The 4th parameter is explicitly ignored (_). If it's reserved for future input-based policy (e.g., argument validation), a comment would help. Otherwise, it widens the API surface for no reason.
Cosmetic — the _ name prevents accidental use.
Clean
- gofmt: clean. Zero files flagged by
gofmt -s -l cmd pkg. - go vet: clean. Zero issues.
- golangci-lint: not available in this environment; recommend running locally.
- No new
//nolintdirectives. - No panics in library code (except the
Truncateedge case above). - No new dependencies: the only go.mod change is bumping
anthropic-sdk-gofrom v1.57.0 → v1.61.0. Same module path, same owner — no supply-chain concern. - Error wrapping with
%w: used correctly inmarshalResult(handlers.go:210),investigation.go:62. context.Contextfirst param: respected in all handlers andExecutemethods.- Comma-ok type assertions: used correctly in
extractToolRunnerFactory(line 146) and throughout. - Receiver naming: consistent single-letter receivers (
r,g,u,a) matching type initials. - Interface placement:
ToolRunnerFactoryis in the consumer package (pkg/tui), not the provider — correct Go convention. - Zero-value usefulness:
RegistryrequiresNewRegistry()(needs initialized map), which is appropriate. - Headline criterion verified:
TestHeadline_DenyEverything_ZeroHandlerInvocationspasses, andTestRevertCheck_PolicyGateproves the test has teeth by verifying the ungated path does fire handlers. - Test quality: policy tests are exhaustive (every mode × class combination). Registry tests cover duplication, copy safety, and SDK shape. Handler tests cover happy path, error path, and truncation.
- Plan document: present at
docs/plans/413-tools-policy-approvals.md, thorough.
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
Security Review: PR #418 — Tool Registry, Policy Engine, Approvals Strip
Reviewer: automated security analysis
Branch: srepd/ai-p3-tools-policy
Scope: policy gate integrity, data leakage, approvals consent, injection, resource exhaustion, test faithfulness, fixtures
MUST-FIX
1. Fail-open default case in policy gate — unknown Decision values execute the handler
pkg/ai/tools/registry.go:138-150
The gatedTool.Execute switch matches Deny and Ask explicitly, then falls through to default which executes the handler. Allow is the only other value today, so default acts as case Allow. But Decision is an int — if a fourth value is ever added (e.g., Audit, RateLimit), it will silently execute the handler because the new value won't match Deny or Ask.
Failure scenario: A future PR adds policy.Log = 3. Any tool call that returns Log hits default, the handler fires, and the new policy intent is silently bypassed.
Verified by running: Passed Decision(42) to a gated tool — handler executed, zero errors returned.
Fix: Switch to explicit case policy.Allow: with default: Deny.
2. Truncate panics on small maxBytes values
pkg/ai/tools/registry.go:189
Truncate(s, maxBytes) computes s[:maxBytes-len(marker)]. When maxBytes < len(marker) (12 bytes), the slice index goes negative and panics with slice bounds out of range.
Failure scenario: Truncate("anything", 5) → panic, process crash.
Verified by running: Called Truncate("very long string", 5) — got panic: runtime error: slice bounds out of range [:-7].
Truncate is exported and maxBytes is caller-controlled. While the production constant is 8192, any future caller or test using a smaller value triggers a crash.
3. nil decide function panics in gatedTool.Execute
pkg/ai/tools/registry.go:137
GatedBetaTools(decide, onAsk) stores decide without nil-checking. Every Execute call dereferences it: g.decide(g.tool.Name, ...). A nil decide panics.
Failure scenario: Caller passes nil as the decide function (e.g., a test or a wiring mistake) → nil pointer dereference → process crash.
Verified by running: Called GatedBetaTools(nil, nil) then Execute — got PANIC: runtime error: invalid memory address or nil pointer dereference.
Contrast: onAsk has a nil guard at line 144 (if g.onAsk != nil). decide has no such guard.
Fix: Either validate at construction time (GatedBetaTools returns error if decide is nil) or add a nil guard that defaults to Deny.
4. Investigation loop logs raw SDK errors at Warn level — breaks plan-101 token-leak property
pkg/tui/investigation.go:111 and pkg/tui/tui.go:711
The investigation error path logs err at log.Warn twice. The Anthropic SDK's Error.Error() includes the full API JSON response body (r.JSON.raw). This appears in standard log output.
The existing watcher query path intentionally keeps raw errors at log.Debug (see pkg/tui/commands.go:289-293, which has a comment explaining why) and uses ClassifyProviderError for the Warn-level message. The investigation path does not follow this pattern.
Failure scenario: An API error response body containing internal details (error messages, request IDs, model identifiers) appears in standard Warn-level logs. Not user-visible in the TUI, but present in log files without debug mode.
Verified by reading: Confirmed ClassifyProviderError is never called in the investigation error path.
5. investigation_test.go tests only two trivial helpers — plan claims four substantial test scenarios that don't exist
pkg/tui/investigation_test.go (33 lines total)
The plan document (docs/plans/413-tools-policy-approvals.md, lines 288-294) claims:
- "Bounded by max tool turns" — no test
- "Bounded by timeout" — no test
- "Policy deny stops tool execution" — no test
- "Verdict extracted from model response" — no test
The actual file tests only isAnthropicFamily() (a 7-case string comparison) and defaultInvestigationConfig() (checks that defaults are non-zero). Neither exercises the investigation loop, the SDK integration, the timeout, or the policy enforcement during investigation.
This is the "unwired code" pattern from the project's recurring failure table. The investigation loop (watcherInvestigateCmd) has no test coverage at all. A test that stubs ToolRunnerFactory and verifies that a deny-all policy produces zero handler invocations during investigation does not exist — the headline test only covers the registry-level gate, not the investigation-loop-level integration.
Verified by reading: grep -rn 'watcherInvestigateCmd' pkg/tui/*_test.go returns zero results.
NICE-TO-HAVE
6. onAsk passed as nil in production — Ask decisions silently swallowed
pkg/tui/watcher.go:250
runDetectors calls watcherInvestigateCmd(..., nil) for the onAsk parameter. When the policy engine returns Ask, the gated tool checks if g.onAsk != nil (it's nil), skips the callback, and returns "Awaiting user approval" to the LLM. The TUI is never notified. No Ask appears in the approvals strip.
Currently harmless because all registered tools are ClassRead (which always returns Allow), so the Ask path is unreachable. But if a write-class tool is added without fixing this wiring, the user won't see the approval request.
Verified by reading: All 7 tools in handlers.go have Class: policy.ClassRead.
7. Actionable verdict creates an Ask with nil Action — Accept is a no-op
pkg/tui/tui.go:729-733
The TierActionable handler creates an Ask with Title and Body but no Action callback. Accept(idx) at approvals.go:62 checks ask.Action != nil before calling, so Accept simply removes the Ask silently. The user sees the approval strip and the "[Enter] Accept" hint, but pressing it does nothing.
Combined with finding 6 (onAsk nil), the entire approvals infrastructure is display-only scaffolding: Asks from verdicts display in the collapsed strip but can never be accepted or dismissed (no key handlers are wired in msgHandlers.go).
Security-positive (no LLM-authored action can cause a write), but functionally incomplete.
8. list_queue has no result-count cap — transient memory spike before truncation
pkg/ai/tools/handlers.go:119-142
get_service_logs caps results at maxServiceLogs = 5 before marshalling. list_queue, get_alerts, get_notes, and get_limited_support marshal the full API response into a string. The Truncate call afterwards caps the output at 8192 bytes, but the full string is allocated first.
Worst case for list_queue: PagerDuty returns up to 100 incidents per page, each several KB → ~100-400 KB transient allocation. Not a practical DoS but inconsistent with the get_service_logs pattern.
9. LLM-produced verdict strings rendered without sanitization
pkg/tui/tui.go:725,735
verdict.Summary is passed to startTypewriter and rendered into the watcher viewport. verdict.Action becomes Ask.Body (currently unreachable via RenderExpanded). Neither is filtered for ANSI escape sequences.
A compromised or jailbroken LLM could produce ANSI sequences to manipulate the terminal display (clear screen, change title, overwrite text). Bubble Tea's viewport does not strip escape sequences. Low practical risk since the LLM is the project's own watcher, not external input.
CLEAN CATEGORIES
- Injection (shell/URL/file path): Clean. No tool input reaches
os/exec,os.Open,filepath, ornet/http. All tools delegate to existing PD/OCM client interfaces. - SSRF / path traversal: Clean. Tool inputs are PagerDuty IDs and OCM cluster IDs passed to typed client methods, not constructed URLs or file paths.
- Fixtures: Clean. No real cluster IDs, domains, tokens, or unmarked UUIDs in the new files. Existing
openshiftapps.com/devshift.netreferences in pre-existing test fixtures are not part of this PR. - formatError: Clean. The
_ errorparameter is discarded at all 9 call sites. No handler path returns raw errors to the model. - Concurrent investigations: Clean.
watcherAnalyzingboolean in the single-threaded Bubble Tea Update loop prevents parallel investigations. ClassifyProviderErrorproperty for non-investigation paths: Preserved. Existing watcher query path correctly usesClassifyProviderError.- Case-sensitivity / unregistered tool names: Clean. The SDK uses exact-match map lookup; unregistered names return "Tool not found" without reaching the gate.
- Post-snapshot tool registration: Clean.
GatedBetaToolscopies tools by value; later registrations are not reachable through the gated slice.
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
PR #418 — V6: Unit Test Adequacy Review
Mutation Testing Matrix
Every mutation below was applied to production code, compiled, tested, and reverted.
| # | Mutation | Result | Catching Test |
|---|---|---|---|
| 1 | Bypass policy.Deny branch in gatedTool.Execute — Deny case calls handler instead of returning denial message |
CAUGHT | TestHeadline_DenyEverything_ZeroHandlerInvocations (integration_test.go:19) |
| 2 | Make Decide always return Allow (early return before switch) |
CAUGHT | TestDecide_Exhaustive — 11 sub-cases fail (policy_test.go:10) |
| 3 | Remove truncation cap — Truncate returns input unchanged |
CAUGHT | TestHandler_TruncationMarker (handlers_test.go:201) |
| 4 | Make ParseWatcherVerdict always return TierNoteworthy — skip JSON parsing |
CAUGHT | TestParseWatcherVerdict_ValidTiers — all 5 sub-cases fail (verdict_test.go:10) |
| 5 | Make registry accept duplicate names — skip duplicate check | CAUGHT | TestRegistry_DuplicateRejection (registry_test.go:14) |
| 6 | Break non-Anthropic degradation — isAnthropicFamily always returns true |
CAUGHT | TestIsAnthropicFamily — 4 sub-cases fail (investigation_test.go:9) |
| 7 | Make Accept a no-op — remove Action invocation |
CAUGHT | TestApprovalsStrip_AcceptInvokesAction (approvals_test.go:23) |
| 8 | Break gatedTool.Execute Allow path — make default case return denial instead of calling handler |
NOT CAUGHT | No test exercises GatedBetaTools with an Allow decision |
Must-Fix
1. gatedTool.Execute Allow path is untested (registry.go:150-158)
Verified by running. Replacing the entire default (Allow) branch of gatedTool.Execute with a denial message — all 34 tests pass. The test suite exercises Deny (TestHeadline) and Ask (TestGatedBetaTools_AskCallsCallback) but never exercises GatedBetaTools with a decide function that returns Allow. TestRevertCheck_PolicyGate uses AsBetaTools() (the ungated path), not GatedBetaTools(), so it doesn't cover this.
Failure scenario: A refactor could break the Allow path (e.g., forget to call the handler, double-truncate, swallow the error) and every test would still pass. This is the path that runs in production when the policy engine approves a tool call.
Fix: Add a test like TestGatedBetaTools_AllowExecutesHandler that registers a tool, wraps it with GatedBetaTools using a decide function that returns policy.Allow, calls Execute, and asserts the handler output is returned (not a denial message) and that the handler was invoked exactly once.
Nice-to-Have
2. TestApprovalsStrip_RenderShowsCount doesn't verify the count (approvals_test.go:65)
Verified by running. Hardcoding 999 in the Render format string — test still passes. The test only checks NotEmpty and Width > 0, not that the count matches. This is cosmetic (the feature is "render something"), not a behavioral gap.
Fix: Add assert.Contains(t, rendered, "2 asks").
3. ModeCustom missing ClassExec and ClassExternal in TestDecide_Exhaustive (policy_test.go)
Suspected from reading. The exhaustive table covers ModeCustom with ClassRead, ClassWriteLocal/NotListed, and ClassWriteLocal/Listed, but skips ClassExec and ClassExternal. Currently ModeCustom and ModeAuto share the same switch body so these are effectively tested via ModeAuto, but if they diverge later this gap will matter.
4. extractToolRunnerFactory and watcherInvestigateCmd have no unit tests (investigation.go)
Suspected from reading. These are the functions that wire GatedBetaTools into the real investigation loop. extractToolRunnerFactory does an interface type assertion; watcherInvestigateCmd orchestrates the SDK BetaToolRunner. Both have production callers (model.go:966, watcher.go:242). Testing them requires a mock ToolRunnerFactory, which is significant integration work — acceptable for phase 3 but should be tracked.
Coverage
| Package | Coverage |
|---|---|
pkg/ai/policy |
100.0% |
pkg/ai/tools |
80.3% |
| Combined | 83.0% |
The 80.3% in pkg/ai/tools is partially the gatedTool Allow path (finding #1 above) and partially accessor methods (Name/Description/InputSchema on ungatedTool and gatedTool) which are trivial.
Other Checks
make test-race: All new package tests pass with-race. No data races detected.- Flakiness: No
time.Sleep, wall-clock assertions, or real network calls in any test. ThewatcherDedupusestime.Now()in production but is not exercised in the tests under review. - Table-driven completeness for
Decide: 20 cases covering all 4 modes x 4 classes (with allowlist variants for Auto/Custom), plus 2 edge cases (empty tool name, unknown mode). The only gap isModeCustommissingClassExec/ClassExternal(nice-to-have #3 above). - Confounded assertions:
TestGetAlerts_HappyPath,TestGetNotes_HappyPath, andTestListQueue_HappyPathassert onlyNotEmpty— they'd pass if the handler returned any non-empty string. However, the mock returns real-shaped data so this is low risk. TestRevertCheck_PolicyGateis well-designed: It explicitly proves the headline test has teeth by showing the ungated path fires the handler. Good anti-tautology test.
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
V7 — Documentation Accuracy & Completeness Review
Must-fix
1. Privacy sections omit new data types sent to LLM via tool results
Files: docs/ai-agents.md:249, docs/llm-providers.md:335-341
What's wrong: Both privacy sections list only the data sent in the pre-413 watcher context (incident title/ID/status, service name, alert names, cluster IDs). With tool-assisted investigation, the tool handlers now marshalResult() full objects back to the Anthropic API, including:
- Note content (actual text of PagerDuty notes, not just "notes exist")
- Service log entries (with severity and full summary text)
- Limited support reasons (summaries of why a cluster is in limited support)
- Full incident queue (up to 25 incidents, marshalled as JSON)
These are sent as tool results to the LLM provider over the network. The privacy note doesn't mention this.
Failure scenario: A user reads the privacy section, sees only "titles, service names, alert names, cluster IDs," and concludes that note content and service log bodies stay on their machine. They configure an Anthropic provider for a cluster handling sensitive workloads. The tool investigation loop sends full note text and service logs to the Anthropic API — data the user didn't consent to sending based on the documented scope.
Verified by: reading pkg/ai/tools/handlers.go — every handler calls marshalResult() on the full API response object, which is returned as a tool result to the SDK's BetaToolRunner, which sends it to the Anthropic Messages API.
2. Plan doc "Solution" section describes AsBetaTools() as if it still exists
File: docs/plans/413-tools-policy-approvals.md:52-54, 58, 158
What's wrong: The plan's Solution section says:
"Two exposure modes.
AsBetaTools()returns all tools ungated ...GatedBetaTools()checks policy before executing each handler."
And line 58:
"
Registrystruct withRegister,AsBetaTools,GatedBetaTools, andExecutemethods."
And Testing section line 158:
"
AsBetaToolsreturns correctanthropic.BetaToolvalues."
But AsBetaTools() was deleted in this PR (confirmed: grep -rn AsBetaTools pkg/ai/tools/ returns nothing). The post-mortem at line 276 correctly describes the deletion, but the design and testing sections still describe the method as shipped code.
Failure scenario: A future developer reads the plan to understand the tool registry's API, calls AsBetaTools(), gets a compile error, and is confused about the actual API surface. Or worse, re-implements it without understanding why it was removed (policy bypass risk documented in the post-mortem).
Verified by: grep -rn AsBetaTools pkg/ai/tools/ — zero results.
3. PR body cites non-existent test name TestGetIncident_ErrorResponse
File: PR body, "Criterion -> Test traceability" table
What's wrong: The traceability table says:
Error sanitization (token-leak) |
TestGetIncident_ErrorResponse|pkg/ai/tools/handlers_test.go
The actual test function is TestGetIncident_NotFound. There is no function named TestGetIncident_ErrorResponse anywhere in the codebase.
Failure scenario: A reviewer tries to run go test -run TestGetIncident_ErrorResponse to verify the token-leak property and gets zero test matches, undermining confidence in the claimed test coverage.
Verified by: grep -rn "func TestGetIncident_ErrorResponse" pkg/ — zero results. Actual test: TestGetIncident_NotFound at pkg/ai/tools/handlers_test.go:27.
4. PR body missing ## Testing this live section
File: PR body
What's wrong: The PR template requires a ## Testing this live section that must mention --dev. The PR body has ## Test plan and ## Visual validation but no ## Testing this live section.
Failure scenario: Reviewers cannot determine how to manually validate the feature in a running srepd instance. The --dev flag (which uses mock PD data) is the standard way to test without a live PagerDuty account.
Verified by: reading the full PR body — the section is absent.
Nice-to-have
5. auto and custom mode descriptions in ai-agents.md are incomplete
File: docs/ai-agents.md:128-129
What's wrong: The mode table says:
auto: "Tools on theai_auto_allow_toolsallowlist execute without prompting"custom: "Fully user-defined allowlist viaai_auto_allow_tools"
Both omit that Read-class tools are always auto-allowed regardless of the allowlist (same as every other mode), and that non-allowlisted non-Read tools get Ask (not Deny). A reader could interpret auto as "only allowlisted tools work" and custom as "only what's on the list runs."
In code (pkg/ai/policy/policy.go:61-78), both ModeAuto and ModeCustom do: ClassRead→Allow, in-allowlist→Allow, else→Ask.
The description for plan correctly says "Read-only tools only" (implying reads are special), but auto/custom don't carry this qualifier forward.
Failure scenario: A user sets ai_permission_mode: custom with an empty allowlist expecting all tools to be blocked. Instead, all 7 read-class tools execute without prompting because ClassRead is always allowed. The current phase has only read tools, so the empty-allowlist user gets the same behavior as plan/interactive/auto — no actual restriction.
Verified by: reading pkg/ai/policy/policy.go:61-78.
6. Plan doc Testing section claims AsBetaTools test coverage that no longer exists
File: docs/plans/413-tools-policy-approvals.md:157-158
What's wrong: The Testing section's "Tool registry" subsection claims:
"
AsBetaToolsreturns correctanthropic.BetaToolvalues."
This test was presumably renamed or removed when AsBetaTools() was deleted. A plan doc that claims test coverage for deleted code misleads anyone doing a test-coverage audit.
Verified by: grep -rn "AsBetaTools" pkg/ai/tools/registry_test.go — zero results.
Clean categories
- README.md:
Akeybinding added to the key table (line 235). Matcheskeymap.go:260-261. Clean. docs/quickstart.md:make quickstart-checkandmake quickstart-verifyboth pass. Clean.- Config defaults: All 5 new config keys documented with correct defaults matching
pkg/config/config.go:43-46. Clean. docs/llm-providers.mdtool support table: Present with correct per-provider breakdown matchingisAnthropicFamily()atpkg/tui/investigation.go:177-183. Clean.- Plan doc: Present at
docs/plans/413-tools-policy-approvals.md, no UNVERIFIED items, post-mortem section included. Clean (aside from staleAsBetaToolsreferences noted above). - All cited test names exist except
TestGetIncident_ErrorResponse(finding #3 above). 27 of 28 verified. - Timeouts table in
ai-agents.md:237-243: Watcher tool investigation correctly documented as 90s configurable. MatchesdefaultInvestigationConfig()atinvestigation.go:49. Clean. - Health check table: Anthropic correctly listed as "(no health check)". Clean.
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
PR #418 — Test Adequacy Review (V6: Mutation Testing)
Mutation Testing Matrix
Every mutation was compiled (no build-break cheats), run against the test suite, then reverted.
| # | Mutation | File:Line | Test(s) That Caught It | Result |
|---|---|---|---|---|
| 1 | Bypass policy.Deny branch — make Deny run handler |
registry.go:102 |
TestHeadline_DenyEverything_ZeroHandlerInvocations |
CAUGHT |
| 2 | Decide always returns Allow |
policy.go:43 |
TestDecide_Exhaustive (12 subtests failed) |
CAUGHT |
| 3 | Remove truncation cap (Truncate returns input unchanged) |
registry.go:147 |
TestHandler_TruncationMarker, TestTruncate_SmallMaxBytes_NoPanic, TestGatedBetaTools_AllowPath_Truncation |
CAUGHT |
| 4 | ParseWatcherVerdict always returns TierNoteworthy |
verdict.go:36 |
TestParseWatcherVerdict_ValidTiers (silent, actionable subtests) |
CAUGHT |
| 5 | Registry accepts duplicate names | registry.go:44 |
TestRegistry_DuplicateRejection |
CAUGHT |
| 6 | isAnthropicFamily always returns true |
investigation.go:177 |
TestIsAnthropicFamily (ollama, openai, ramalama, empty) |
CAUGHT |
| 7 | Accept skips Action invocation (DraftNote becomes no-op) |
approvals.go:57 |
TestApprovalsStrip_AcceptInvokesAction, TestApprovalsStrip_DraftNoteAcceptInvokesPDMock, TestApprovalsStrip_AcceptSelected |
CAUGHT |
| (bonus) | Remove service-log maxServiceLogs cap |
handlers.go:186 |
TestGetServiceLogs_Truncation |
CAUGHT |
All 8 mutations caught. No gaps in the critical-path coverage.
Coverage
| Package | Coverage |
|---|---|
pkg/ai/policy |
100% |
pkg/ai/tools |
83.8% |
pkg/tui (new files only) |
~77.6% (overall); approvals.go ~95%, watcher.go ~97%, investigation.go ~73% |
Uncovered in pkg/ai/tools: gatedTool.Name(), .Description(), .InputSchema() (0% each) — these are SDK interface methods exercised only by the real Anthropic runner, not unit-testable in isolation. Not a concern.
Race Detection
go test -race ./pkg/ai/... ./pkg/tui/... — clean, no data races detected.
Findings
Nice-to-have (not must-fix)
1. Decide table: ModeCustom missing ClassExec and ClassExternal cases
policy_test.go:41-43: ModeCustom is tested only for ClassRead and ClassWriteLocal. ClassExec and ClassExternal are not explicitly tested. Since ModeCustom is currently identical to ModeAuto (documented, and plan 415 will differentiate), the mutation was still caught via other mode tests. However, when plan 415 differentiates ModeCustom, missing rows could let a regression slip.
Verified by reading the test table; not a correctness bug today.
2. watcherSynthesizeCmd at 0% coverage
commands.go:331: The non-Anthropic synthesis fallback path is never exercised by any test. The watcherSynthesisMsg handler is tested (confirmed by mutation 6's catch via isAnthropicFamily), but the command that produces that message is not — the mock watcher tests route through the tool-using investigation path or direct message injection.
Verified by reading go tool cover output. Not a correctness bug because the message handler IS tested, but the command itself could silently break.
3. extractToolRunnerFactory at 0% coverage
investigation.go:165: This glue function (type-asserts a provider to get BetaMessages()) has no test. It's a thin adapter and unlikely to break, but a nil-return path could mask a misconfigured provider at runtime.
Verified by reading coverage output.
4. approvalsStrip.Selected() at 0% coverage
approvals.go:94: The Selected() getter is never called in tests — tests access strip.selected directly (internal field). Not a correctness risk since the field IS tested, but the public API surface has a gap.
Verified by reading coverage output.
Clean areas (no findings)
- Headline criterion: The
TestHeadline_DenyEverything_ZeroHandlerInvocations+TestRevertCheck_PolicyGatepair is exemplary — one proves the gate blocks, the other proves the gate is load-bearing. Mutation 1 confirmed this. - Flakiness: No
time.Sleep, no real network calls, no wall-clock assertions. Thetime.Now()uses in watcher tests set model fields for render assertions — deterministic. - Verdict parsing: Full tier coverage (silent, noteworthy, actionable, unknown, malformed, empty, extra keys). Mutation-proof.
- Approvals strip: Thorough — add/remove/accept/dismiss/bounds/selection/rendering all tested with action-invocation verification. Mutation 7 confirmed this.
- Policy engine: 100% statement coverage with exhaustive Mode x Class matrix (minor ModeCustom gap noted above). Mutation-proof.
- Registry: Duplicate rejection, copy semantics, BetaTools shape, gated tool paths (Deny/Ask/Allow) all tested through both unit and integration tests.
Summary
The tests are strong. Every critical mutation was caught, coverage is good, and there's no flakiness. The four nice-to-haves are all about completeness of secondary paths — none is a correctness gap today.
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
PR #418 — Adversarial Re-Review After Fix Rounds
Reviewer: Claude Code (automated adversarial review)
Branch: srepd/ai-p3-tools-policy
Scope: a381d12..HEAD (10 fix-round commits) + full feature coherence
Date: 2026-08-01
Verification Summary
| Check | Result |
|---|---|
go build ./... |
PASS |
go test -race ./pkg/ai/... ./pkg/tui/... |
PASS |
deadcode ./... |
No new entries (all pre-existing: mocks, config helpers) |
make test-all |
1 pre-existing failure in cmd/ (unrelated: /var/log read-only in sandbox) — no regressions from this PR |
| All test names from PR body exist in source | PASS |
AsBetaTools removed |
Confirmed (zero grep hits) |
Fix Verification — All Seven Fixes Hold
- Approvals strip wired end to end —
Akey binding inkeymap.go:259-262, handler inmsgHandlers.go:212-222, expanded/collapsed rendering inviews.go:173-180,switchApprovalsFocusModewith Accept/Dismiss/navigation. Holds. - Headline test runs production path —
TestWatcherInvestigateCmd_DenyEverything_ProductionPathuses realpolicy.DecideviainvestigationConfig, not a hand-rolled closure. Holds. - Allow path tested —
TestGatedBetaTools_AllowPath_HandlerRunsandTestWatcherInvestigateCmd_AllowPath_HandlerRunsboth assert handler invocation count > 0.TestRevertCheck_PolicyGateproves the deny test has teeth. Holds. - Raw SDK errors moved to Debug —
investigation.go:130useslog.Debugfor raw error,log.Warnfor classified error.tui.go:711-712same pattern. User-facing paths (errMsg) useClassifyProviderErrorwhich extracts only themessagefield. Holds. - Truncate panic guard —
registry.go:152-156handlesmaxBytes <= 0andmaxBytes <= len(marker).TestTruncate_SmallMaxBytes_NoPaniccovers 0, 1, and 5. Holds. AsBetaToolsremoved — zero grep hits across the repo. Holds.formatErrorconveys error class —handlers.go:222-228usesclassifyToolErrorto return class labels ("timeout", "auth error", "not found", "network error", "request failed") instead of raw error strings.TestHandler_FormatErrorIncludesClassverifies. Holds.
Must-Fix: Mutation-Surviving Test Gaps
M1. investigationMsg handler discards ask without test detection
File: pkg/tui/tui.go:729-732
Failure scenario: Remove m.approvals.Add(ask) at line 731 (replace with _ = m.buildAskFromVerdict(msg.verdict)). The code compiles. All tests pass. An LLM-authored actionable verdict never reaches the approvals strip — the user sees the typewriter summary but has no Accept button. PagerDuty writes (notes, re-escalations) silently stop working through the watcher path.
Verified by running: Mutation applied, go test ./pkg/tui/... -run 'TestBuildAsk|TestApprovals' -count=1 passed, mutation reverted.
Severity: HIGH — this is the only path from AI verdict to PD write. The ask_wiring_test.go tests validate buildAskFromVerdict in isolation but no test exercises the investigationMsg handler in Update() that actually adds the ask to the strip.
Fix: Add an integration test that sends an investigationMsg{verdict: actionable} through model.Update() and asserts m.approvals.Count() > 0.
M2. switchApprovalsFocusMode Enter handler discards cmd without test detection
File: pkg/tui/msgHandlers.go:1121-1126
Failure scenario: Change return m, cmd to return m, nil at line 1126. The code compiles. All tests pass. User presses Enter on an approval → the ask is removed from the strip (Accept still fires internally) but the returned tea.Cmd — which is what actually posts the PD note or triggers re-escalation — is silently dropped. The UI shows "accepted" but nothing happens.
Verified by running: Mutation applied, all approvals tests passed, mutation reverted.
Severity: HIGH — this is the final gate between user Accept and PD write. approvals_test.go tests Accept() in isolation (verifying the callback fires) but no test drives the Enter key through switchApprovalsFocusMode and asserts the returned cmd is non-nil.
Fix: Add a test that creates a model with approvalsExpanded=true, an ask with a non-nil Action that returns a non-nil cmd, sends an Enter tea.KeyMsg through keyMsgHandler, and asserts the returned tea.Cmd is non-nil.
Nice-to-Have
N1. Approvals queue is unbounded
File: pkg/tui/approvals.go:43-51
Failure scenario: A malfunctioning or adversarial LLM returns actionable verdicts on every watcher cycle (every 15 seconds). Each adds an Ask via approvals.Add(). Over hours, the asks slice grows without bound. Memory impact is modest per-ask but accumulates.
Severity: LOW — bounded in practice by MaxIterations (6 tool turns), investigation timeout (90s), and the watcher poll interval. An explicit cap (e.g., 50 asks, dropping oldest) would be belt-and-suspenders.
N2. classifyToolError fallback returns "request failed" — consider "unknown error"
File: pkg/ai/tools/handlers.go:243
Severity: COSMETIC — "request failed" is slightly misleading for non-request errors (e.g., JSON marshal failure). "unknown error" would be more honest. Not a correctness issue.
Clean Categories
- Policy gate correctness: Verified by mutation (
TestRevertCheck_PolicyGate), by deny-all test, and by readinggatedTool.Execute. Thedefault:branch (Allow path) runs the handler. Case-sensitivity is handled by exact string match inisInList. Unregistered tool names cannot reach a handler —GatedBetaToolsiterates only registered tools, and the SDK only calls tools it received in the tools list. - SuggestedCommand exec safety:
copyToClipboardCmdreturns asetStatusMsg— noexec.Command, no clipboard write, no shell invocation. Verified byTestBuildAskFromVerdict_SuggestedCommand_CopiesNotExecutes. - Token leakage:
ClassifyProviderErrorcatches*anthropic.Errorviaerrors.Asand extracts only themessagefield, never the raw JSON body. Raw errors appear only atlog.Debuglevel. Tool handler errors go throughclassifyToolErrorwhich returns fixed class labels. The watcher buffer and approvals strip never contain raw API responses. - MaxIterations safety: Guarded at two layers —
resolveInvestigationConfigonly overrides ifv > 0, andwatcherInvestigateCmdclamps<= 0to6. SDK treatsMaxIterations: 0as unlimited, but this code never passes 0. - Investigation timeout:
context.WithTimeout(context.Background(), cfg.timeout)atinvestigation.go:123. Default 90s, configurable, guarded against non-positive. - Race conditions:
go test -racepasses on all PR packages. - No new deadcode beyond the ~32 pre-existing entries in main.
Bottom Line
The seven fixes from the review rounds all hold. The feature is architecturally sound: policy enforcement is correct, the write path requires explicit user Accept, no exec path exists for suggested commands, and token leakage is properly guarded.
Two must-fix items remain — both are the "unwired code" test gap pattern this project keeps hitting. The production call sites that bridge buildAskFromVerdict → strip → Accept → PD write are untested at the integration level. The individual unit tests pass regardless of whether these call sites exist. These should be addressed before merge to prevent silent regressions.
clcollins
left a comment
clcollins
left a comment
There was a problem hiding this comment.
PR #418 Final Verification Pass — Report
Summary
All seven verification categories are clean. No must-fix issues found. The five fix rounds held, and no new gaps were discovered.
1. Mutation Testing (Verified by Running)
All three known mutations were re-executed on a clean tree and confirmed killed:
| ID | Site | Mutation | Killing Test | Result |
|---|---|---|---|---|
| M1 | pkg/tui/tui.go:731 |
m.approvals.Add(ask) → _ = ask |
TestUpdate_InvestigationMsg_ActionableVerdictAddsAsk |
KILLED |
| M2 | pkg/tui/msgHandlers.go:1126 |
return m, cmd → return m, nil |
TestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNote |
KILLED |
| M3 | pkg/tui/tui.go:734-739 |
m.approvals.Add(Ask{...}) → _ = ta |
TestUpdate_InvestigationMsg_ToolAsksAddToolPermissionAsks |
KILLED |
Each mutation compiled, was tested, the named test failed, and the mutation was reverted.
2. M4 Hunting — No Fourth Mutation Survived
Grepped all approvals.Add, .Action, Accept, Dismiss call sites and stubbed each in turn:
AskSuggestedCommandAction → nil: killed byTestBuildAskFromVerdict_SuggestedCommand_CopiesNotExecutesDismiss→ no-op: killed byTestUpdate_ApprovalsDismiss_DoesNotReturnActionCmdpostAINoteCmd→ nil result: killed byTestBuildAskFromVerdict_DraftNote_ActionCallsPDAddNotegatedTool.Executebypass policy: killed byTestHeadline_DenyEverything_ZeroHandlerInvocations+TestGatedBetaTools_AskCallsCallbackAction != ""guard inverted: killed byTestUpdate_InvestigationMsg_ActionableVerdictAddsAsk
No surviving mutation found. The approvals chain is well-covered.
3. Verdict → PagerDuty Write Chain (Verified by Reading)
CLEAN. Traced buildAskFromVerdict → approvals.Add → user A keypress → switchApprovalsFocusMode → user Enter → approvals.Accept → ask.Action() → postAINoteCmd.
Actionclosures are never invoked onAdd,Render,RenderExpanded, orDismissAcceptis only called fromswitchApprovalsFocusModeon explicitEnterkeypress- No auto-fire, no fire-on-render, no fire-from-message-handler paths
4. Policy Engine Bypass Resistance (Verified by Reading + Running)
CLEAN.
gatedTool.Executechecksdecide()before every handler invocation- Empty tool name →
Deny(policy.go:44) - Unknown mode →
Deny(policy.go:82 default case) GatedBetaToolstakes a read-locked snapshot — post-snapshot registrations invisibledecideis always a non-nil closure (investigation.go:79-81)isInListuses exact string match — no case-variant bypass- Test coverage:
TestDecide_Exhaustivecovers all mode×class combinations + edge cases
5. SuggestedCommand — No Exec Path (Verified by Reading)
CLEAN. copyToClipboardCmd (model.go:1011-1014) returns setStatusMsg — displays text in the status bar with "copy to terminal" instruction. No exec.Command, no os/exec import in pkg/ai/. All registered tool handlers are ClassRead. Verified by TestBuildAskFromVerdict_SuggestedCommand_CopiesNotExecutes.
6. Token Leakage (Verified by Reading)
CLEAN. All AI error paths use ClassifyProviderError. Tool handlers use formatError which:
- Logs raw errors at
Debuglevel only (log.Debug) - Returns classified categories to tool results: "timeout", "auth error", "not found", "network error", "request failed"
- Tested by
TestHandler_ErrorDoesNotLeakInternalsandTestHandler_FormatErrorIncludesClass
Investigation error path (tui.go:711-712): raw error at Debug, classified at Warn, watcher buffer gets observation text only.
7. Doc and Test Accuracy (Verified by Running)
- All test names in PR body exist: checked every
Test\w+token againstgrep -rn 'func Test'— zero mismatches - Plan doc:
AsBetaToolsreferences correctly describe it as removed (D1 fix holds); no stale symbol references in code (grep -rn 'AsBetaTools' pkg/returns nothing) - Deadcode: ~32 entries, all pre-existing mocks/helpers/unused config functions; no new entries from PR files
- Race tests:
go test -race ./pkg/...— all 15 packages pass, zero data races
8. Test Suite (Verified by Running)
go test ./pkg/... -count=1: all pass (15/15 packages)go test -race ./pkg/... -count=1: all pass, no data racescmd/has one pre-existing env failure (TestConfigureLogging_SetsLogWriterwrites to/var/log/srepd.log— read-only filesystem in this env). Not related to PR #418.
Nice-to-Have
None identified. The implementation is clean and well-tested.
Verdict
Ship it. All five fix rounds hold. No new issues. The tool registry, policy engine, and approvals chain are correctly wired and comprehensively tested against mutation.
73d2f4c to
1d78873
Compare
1 participant
Summary
Phase 3 of the AI rearchitecture: adds a read-only tool layer, pure policy engine, approvals strip, and watcher investigation loop.
pkg/ai/tools/): 7 read-only tools wrapping existing PD/OCM accessors, exposed asanthropic.BetaToolwith policy-gated executionpkg/ai/policy/): pureDecide(cfg, toolName, class, input) → Allow/Deny/Ask— no I/O, exhaustively testedpkg/tui/approvals.go): TUI component for Ask decisions with keyboard navigation, accept/dismiss actions, expanded list viewpkg/tui/investigation.go): boundedBetaToolRunner.NextMessageloop with verdict extraction (silent/noteworthy/actionable)watcher_max_tool_turns,watcher_investigation_timeout,ai_permission_mode,ai_auto_allow_tools,ai_allowed_command_prefixesValidation findings fixed (PR #418 fix round)
BLOCKING
buildAskFromVerdictwith real actions (PostNote, re-escalate, clipboard), keyboard nav (j/k/Enter/d/Esc),Akey toggleTestWatcherInvestigateCmd_DenyEverything_ProductionPathandTestWatcherInvestigateCmd_AllowPath_HandlerRunsuse httptest.NewServerTestGatedBetaTools_AllowPath_HandlerRunsandTestGatedBetaTools_AllowPath_Truncationin integration_test.goClassifyProviderError(investigation.go + tui.go)MUST-FIX
AsBetaTools()andungatedToolentirely; updated revert-check test to useGatedBetaToolswith AllowapprovalsExpandedtoggle onAkey pressNICE-TO-HAVE
Validation findings fixed (PR #418 round 3 — mutation coverage)
BLOCKING
Ask.Actionwiring untested — replacing DraftNote's Action with a compiling no-op leaves entirepkg/tuisuite passingask_wiring_test.go: each kind's Action invoked and observable effect asserted (PD mock call, clipboard msg, re-escalate msg). Mutation-kills verified for all 3 kinds.MUST-FIX
inferAskKindfallback could produce nil Action for unhandled kinddefaultcase inbuildAskFromVerdictswitch falling back to DraftNote behavior. Tested viaTestBuildAskFromVerdict_UnknownText_FallbackHasActionandTestBuildAskFromVerdict_UnhandledKind_FallbackAction.NICE-TO-HAVE
flashNotification(4s durable) instead ofsetStatuspkg/ai/policy/policy.go:70-71— no change neededValidation findings fixed (PR #418 round 5 — M3 + doc accuracy)
BLOCKING
toolAsksloop intui.go) completely untested — replacing loop body with_ = tacompiles and all tests passapprovals_update_test.go:TestUpdate_InvestigationMsg_ToolAsksAddToolPermissionAsks(2 distinct tools → 2 AskToolPermission),TestUpdate_InvestigationMsg_EmptyToolAsksAddsNoToolPermissionAsks(empty → zero),TestUpdate_InvestigationMsg_ToolAsksWithVerdictAction(Action + toolAsk → both types). Revert check verified.MUST-FIX
AsBetaTools()at 3 locations as if it still existsTestDecide,TestParseWatcherVerdict)TestDecide_ExhaustiveandTestParseWatcherVerdict_ValidTiers; all 37 rows re-verified mechanically viagrep -rn 'func <Name>('Call-site audit
grep -n 'approvals\.Add' pkg/tui/*.goshows exactly 2 production call sites:tui.go:731— verdict Action path (covered by round 4'sTestUpdate_InvestigationMsg_ActionableVerdictAddsAsk)tui.go:735— toolAsks loop (covered by round 5'sTestUpdate_InvestigationMsg_ToolAsksAddToolPermissionAsks)No fourth gap found.
Headline criterion
Verified by
TestHeadline_DenyEverything_ZeroHandlerInvocations(unit) andTestWatcherInvestigateCmd_DenyEverything_ProductionPath(production-path with httptest.NewServer).Criterion → Test traceability
TestHeadline_DenyEverything_ZeroHandlerInvocationspkg/ai/tools/integration_test.goTestRevertCheck_PolicyGatepkg/ai/tools/integration_test.goTestGatedBetaTools_AllowPath_HandlerRunspkg/ai/tools/integration_test.goTestGatedBetaTools_AllowPath_Truncationpkg/ai/tools/integration_test.goTestWatcherInvestigateCmd_DenyEverything_ProductionPathpkg/tui/investigation_test.goTestWatcherInvestigateCmd_AllowPath_HandlerRunspkg/tui/investigation_test.goTestTruncate_SmallMaxBytes_NoPanicpkg/ai/tools/handlers_test.goTestHandler_FormatErrorIncludesClasspkg/ai/tools/handlers_test.goTestApprovalsStrip_RenderShowsActualCountpkg/tui/approvals_test.goTestApprovalsStrip_DraftNoteAcceptInvokesPDMockpkg/tui/approvals_test.goTestApprovalsStrip_RenderExpandedShowsAskspkg/tui/approvals_test.goTestApprovalsStrip_MoveSelectionpkg/tui/approvals_test.goTestApprovalsStrip_AcceptSelectedpkg/tui/approvals_test.goTestDecide_ClassReadAllowedInEveryModepkg/ai/policy/policy_test.goTestDecide_Exhaustivetable (21 cases)pkg/ai/policy/policy_test.goTestRegistry_DuplicateRejectionpkg/ai/tools/registry_test.goTestRegistry_BetaToolsShapepkg/ai/tools/registry_test.goTestGatedBetaTools_AskCallsCallbackpkg/ai/tools/integration_test.goTestGetIncident_HappyPath, etc.pkg/ai/tools/handlers_test.goTestHandler_ErrorDoesNotLeakInternalspkg/ai/tools/handlers_test.goTestParseWatcherVerdict_ValidTierstablepkg/ai/tools/verdict_test.goTestApprovalsStrip_AddAndCount,TestApprovalsStrip_DismissRemovespkg/tui/approvals_test.goTestBuildAskFromVerdict_DraftNote_ActionCallsPDAddNotepkg/tui/ask_wiring_test.goTestBuildAskFromVerdict_SuggestedCommand_CopiesNotExecutespkg/tui/ask_wiring_test.goTestBuildAskFromVerdict_EscalationSuggestion_ReEscalatespkg/tui/ask_wiring_test.goTestBuildAskFromVerdict_UnknownText_FallbackHasActionpkg/tui/ask_wiring_test.goTestInferAskKind_Fallback_IsSensibleDefaultpkg/tui/ask_wiring_test.goTestBuildAskFromVerdict_UnhandledKind_FallbackActionpkg/tui/ask_wiring_test.goTestUpdate_InvestigationMsg_ActionableVerdictAddsAskpkg/tui/approvals_update_test.goTestUpdate_InvestigationMsg_SilentVerdictDoesNotAddAskpkg/tui/approvals_update_test.goTestUpdate_InvestigationMsg_NoteworthyVerdictDoesNotAddAskpkg/tui/approvals_update_test.goTestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNotepkg/tui/approvals_update_test.goTestUpdate_ApprovalsDismiss_DoesNotReturnActionCmdpkg/tui/approvals_update_test.goTestUpdate_InvestigationMsg_ToolAsksAddToolPermissionAskspkg/tui/approvals_update_test.goTestUpdate_InvestigationMsg_EmptyToolAsksAddsNoToolPermissionAskspkg/tui/approvals_update_test.goTestUpdate_InvestigationMsg_ToolAsksWithVerdictActionpkg/tui/approvals_update_test.goRevert checks
Round 2
gatedTool.ExecuteTestWatcherInvestigateCmd_DenyEverything_ProductionPathgatedTool.ExecuteTestWatcherInvestigateCmd_AllowPath_HandlerRunsRound 3 — Ask.Action wiring
Each kind's Action was replaced with a compiling no-op (
_ = x; return nil) and the test suite run:DraftNote mutation:
SuggestedCommand mutation:
EscalationSuggestion mutation:
All 3 mutations caught. Real code restored, all tests pass.
Round 4 — Update-level mutation coverage
Two remaining links in the AI-verdict → user-approval → PagerDuty-write chain that survived all prior tests.
M1 —
investigationMsg→ approvals strip (tui.go:731):Mutation:
m.approvals.Add(ask)→_ = askM2 — approvals Enter handler drops its
tea.Cmd(msgHandlers.go:1126):Mutation:
return m, cmd→_ = m.approvals.Accept(...); return m, nilBoth mutations caught. Real code restored, all tests pass.
Round 5 — toolAsks loop (M3)
Mutation: replaced
m.approvals.Add(Ask{...})loop body with_ = taBoth tests FAIL with the mutation. Real code restored, all tests pass.
Deviations from plan
get_recent_eventsomitted:pkg/deltadoes not exist (plan 412 didn't build it)BetaToolRunner.NextMessage(per-turn control) instead ofRun()(no policy gate possible)formatErrordrops raw error text from tool results but logs at Debug and appends classified categoryAsBetaTools()entirely rather than gating (zero production callers = latent policy bypass)Security properties preserved
formatErrordrops raw error,ClassifyProviderErrorat Warn)//nolint, nopanic()in library codeMaxIterationsnever set to 0replacedirectives, no vendoringTest plan
go test ./pkg/ai/policy/... -count=1— all passgo test ./pkg/ai/tools/... -count=1— all passgo test ./pkg/tui/... -count=1— all passgo vet ./...— cleangofmt -s -l cmd pkg— cleangolangci-lint run --timeout 5m— 0 issuesgo test -race ./pkg/ai/... ./pkg/tui/... -count=1— cleandeadcode ./...— no new deadcode from our changes (32 pre-existing entries)make readme-check— OKmake quickstart-check— OKmake quickstart-verify— OKcmdtest failure is pre-existing (TestConfigureLogging_SetsLogWriter— read-only filesystem)Visual validation
tui-mcp is not available in this container environment (no
npx). Golden snapshots and integration tests are the only visual evidence. The changes in this round are purely behavioral (Action wiring) with no rendering changes.🤖 Generated with Claude Code
Maintainer review findings fixed (PR #418 round 6)
F1 — BLOCKING: configured model never reaches investigation
Problem:
watcher.go:249passed""as model towatcherInvestigateCmd.investigation.go:107-109substituted"claude-sonnet-4-6"— a bare model IDthat fails on Bedrock (requires inference-profile ID
us.anthropic.claude-sonnet-4-6).Fix:
ModelReporteroptional interface toai.Provider(followsHealthCheckerpattern)Model()onanthropicProvider(used by all 3 Anthropic-family providers)ResolvedModel(p)helper for callersrunDetectorsnow passesai.ResolvedModel(m.aiProvider)instead of""TestWatcherInvestigateCmd_ModelPlumbing_ExplicitModelpkg/tui/investigation_test.goTestWatcherInvestigateCmd_ModelPlumbing_EmptyModelReturnsErrorpkg/tui/investigation_test.goTestResolvedModel/anthropic_provider_exposes_configured_modelpkg/ai/provider_test.goTestResolvedModel/bedrock_provider_yields_inference-profile_ID_not_bare_modelpkg/ai/provider_test.goTestResolvedModel/non-ModelReporter_provider_returns_emptypkg/ai/provider_test.goTestResolvedModel/nil_provider_returns_emptypkg/ai/provider_test.goRevert checks
Re-introduced the hardcoded fallback (
model = "claude-sonnet-4-6") atinvestigation.go:107-109,confirmed
TestWatcherInvestigateCmd_ModelPlumbing_EmptyModelReturnsErrorFAILS:Restored the fix, confirmed tests pass.
F2 — DECISION: defer AskToolPermission to plan 415
Problem:
tui.go:734-740builtAskToolPermissionasks with noActioncallback.Acceptreturnednil— the ask silently disappeared.Fix (deferral, not implementation):
toolAsks→AskToolPermissionloop frominvestigationMsghandlerAskToolPermissionconstant andaskKindLabelcaseToolAsksAddToolPermissionAsks,EmptyToolAsksAddsNoToolPermissionAsks,ToolAsksWithVerdictAction) withTestUpdate_InvestigationMsg_ToolAsksAreNotSurfacedTestUpdate_InvestigationMsg_ActionableVerdictAddsAsk) passes untoucheddocs/ai-agents.mdinteractive mode rowtoolAsktype andtoolAsksfield are still live: theonAskcollector ininvestigation.gotracks ask-class decisions for diagnostic purposes and futureplan 415 consumption.
deadcode ./...confirms no new entries.TestUpdate_InvestigationMsg_ToolAsksAreNotSurfacedpkg/tui/approvals_update_test.goTestUpdate_InvestigationMsg_ActionableVerdictAddsAskpkg/tui/approvals_update_test.goF3 — MINOR: ask pile-up dedup
Problem: Model retrying the same tool call accumulated duplicate entries in
collectedAsksunboundedly.Fix: Dedup by tool name + SHA-256 of input JSON, per investigation. Even
though F2 removes UI surfacing, the collector should not grow unboundedly.
TestWatcherInvestigateCmd_AskDedup_SameToolAndInputpkg/tui/investigation_test.goTest verification
Round 6 CI gates
gofmt -s -l cmd pkg— cleango vet ./...— cleangolangci-lint run --timeout 5m— 0 issuesgo test ./pkg/... -count=1— all passgo test -race ./pkg/... -count=1— cleandeadcode ./...— no new entries (32 pre-existing)make quickstart-check— OKcmdtest failure is pre-existing (TestConfigureLogging_SetsLogWriter— read-only/var/log)Round 7 — runDetectors call-site test gap
Problem
The round-6 fix (F1) was correct, but no test exercises the actual call site
(
runDetectorsatwatcher.go:249). The round-6 tests prove:watcherInvestigateCmduses the model it receives (TestWatcherInvestigateCmd_ModelPlumbing_ExplicitModel)ResolvedModelreturns the right value (TestResolvedModel)But neither tests that
runDetectorspasses the right model. Confirmed:re-introducing the exact F1 bug (
"claude-sonnet-4-6"hardcoded at line 249)left the entire test suite green.
Fix
Added
TestRunDetectors_ModelPlumbingthat exercisesrunDetectorsdirectly:bedrockProviderfake implementingModelReporterwith inference-profile IDus.anthropic.claude-sonnet-4-6detectServiceStormcapturingFactoryto intercept the model passed to the tool runnerclaude-sonnet-4-6watcherSystemPrompt,contextStr(Messages), andinvestigationCfg.maxToolTurns(MaxIterations) reach the runnerTestRunDetectors_ModelPlumbingpkg/tui/watcher_integration_test.go:897Revert checks
Replaced
ai.ResolvedModel(m.aiProvider)atwatcher.go:250with:Test result:
Restored
ai.ResolvedModel(m.aiProvider), all tests pass.Argument audit
Checked the other arguments
runDetectorspasses towatcherInvestigateCmd:m.toolRunnerFactorym.toolRegistrym.investigationCfgrunDetectorsm.watcherSystemPromptrunDetectorsobs.SummarydetectAll, not a model field — tested by detector testscontextStrbuildWatcherContexttested directly but not throughrunDetectorsai.ResolvedModel(m.aiProvider)nil(onAsk)All closeable gaps covered by extending
TestRunDetectors_ModelPlumbing.Test verification
Round 7 CI gates
golangci-lint cache clean && golangci-lint run --timeout 5m— 0 issuesgo test ./pkg/... -count=1— all passgo test -race ./pkg/... -count=1— cleandeadcode ./...— no new entries (32 pre-existing)make quickstart-check— OKmake quickstart-verify— OK