Apply gofmt -w across the codebase (struct field comment realignment
only — no semantic changes) and silence two errcheck warnings on
fmt.Sscanf / fmt.Fprintf return values in internal/router/discovery
with explicit `_, _ =` discards. Required so `make check` is green
before tagging v0.1.0.
The router.SecureProvider interface previously required a public
IsSecure() bool method. Any test mock — or future production type —
could satisfy it by returning true, defeating the W1 "only wrapped
providers may flow past the boundary" contract through convention
rather than at the type level.
Replaces IsSecure() bool with an unexported security.Marker interface
that has a single secured() method. Go's method-set semantics key
unexported methods by their defining package, so only types declared in
internal/security can satisfy Marker. *SafeProvider gets the lone
secured() implementation; router.SecureProvider embeds Marker.
The seal forces every test mock that previously implemented IsSecure()
to either (a) be wrapped with security.WrapProvider(mp, nil) at the use
site, or (b) drop the method entirely if the mock never flows through
SecureProvider. 93 use sites across 11 test files were updated via a
per-package secureMock helper. WrapProvider with a nil firewall ref is
a no-op pass-through, so test behavior is unchanged.
Empirically: a type from outside internal/security can declare
`secured()` but the compiler will reject assigning it to
router.SecureProvider because the unexported method belongs to the
other package's namespace. Convention → compile-time guarantee.
Closes the last remaining 2026-05-19 audit finding by documenting the
existing transitive guarantee rather than restructuring the hook
contract.
The audit observed that PostToolUse hooks receive raw tool output
before the firewall scan runs, and proposed reordering or splitting
the event into raw-local-only and redacted-for-LLM variants. After
Wave 1 (SafeProvider boundary at every router arm + non-engine
provider consumer), the audit's threat model is closed transitively:
- Shell hooks see raw output but never reach an LLM.
- Prompt hooks route Stream calls through routerStreamer → router →
arm.Provider, every arm.Provider is now *SafeProvider, outgoing
messages are scanned at the boundary.
- Agent hooks spawn an elf whose engine has Firewall set;
buildRequest scans inline.
Reordering would regress legitimate shell-hook use cases (audit,
forensic, local alert) that need raw access. Splitting the contract
forces every existing hook config to migrate and introduces a
wrong-variant footgun. Neither is justified by the residual risk.
Three changes ship with the ADR:
- ADR-004 records the decision and the conditions for re-opening it.
- Doc comments on hook.PostToolUse and the dispatcher call site in
the engine point at the ADR.
- internal/hook/posttooluse_redaction_test.go locks in the invariant:
a prompt PostToolUse hook firing on a secret-bearing tool result
produces a redacted prompt at the inner provider. If this test
fails, ADR-004's Position A is no longer correct and the audit
finding re-opens.
Brings the project to a clean `make lint` baseline (0 issues).
Mechanical:
- Wrap deferred resp.Body.Close() in closures (router/discovery.go,
router/probe.go) so the unchecked return surfaces as `_ = ...`.
- Apply `_ = ...` (single or multi-return blank) to test-file calls
that intentionally ignore errors: os.MkdirAll / os.WriteFile / os.Chdir
in setup paths, Close / Shutdown in teardown, Submit / Spawn / Send /
LoadDir in tests that assert on side effects.
Structural:
- engine.handleRequestTooLarge drops the unused req parameter and
rebuilds the request from compacted history (SA4009 — argument was
overwritten before first use).
- provider.ClassifyHTTPStatus and google.applyCapabilityOverrides switch
to tagged switches over the discriminator (QF1002).
- tui.app.go MouseWheel + inputMode and cmd/gnoma main slm-status use
tagged switches in place of equality chains (QF1003).
- cmd/gnoma main.go merges a var decl with its immediate assignment
(S1021).
- Three empty-branch sites (dispatcher_test, loader_test,
coordinator_test) become real assertions or get the dead `if` removed
(SA9003).
Removes five unused funcs/vars/fields that golangci-lint had been
flagging (anthropic.toolCallDoneEvent, mistral.translateMessages,
hook.newError, subprocess.vibeParser.lastAssistantMsgID, tui.cBase),
two ineffectual assignments (tui/rendering.go visible-window loop,
subprocess stream_test setup), and a stale if/HasPrefix that's now a
strings.TrimPrefix.
Wires errcheck onto every subprocess / stream lifecycle path so a
failed close or shutdown is at least logged rather than silently
dropped:
- engine/loop.go: stream.Close on both the error and success paths
- mcp/manager.go: Shutdown when StartAll partial-fails; Transport
close after Initialize failure
- mcp/transport.go: stdin.Close + syscall.Kill on graceful-timeout
fallback
- slm/download.go: Close propagated as a named-return error on the
success path; explicitly discarded on the rollback path
- slm/classifier.go, slm/manager.go, hook/prompt.go, context/summarize.go,
config/write.go, cmd/gnoma/main.go, tool/fs/grep.go: explicit
ignores or error logging on Close / Shutdown / WalkDir / Scanln
Production-code errcheck and ineffassign are now zero. Remaining
golangci-lint output is test-only Close-in-defer noise plus
stylistic staticcheck QF suggestions, left alone.
- M2: stop echoing the matched pattern name in the user-visible
[BLOCKED: ...] message returned by the firewall. The pattern (and
the matched secret class) still appear in the operator log, but the
string sent back into the prompt is now generic.
- H1: document Rule.Pattern semantics on the Rule type and pin them
with a regression test. Pattern is a case-sensitive, exact substring
match against the JSON-serialised tool arguments — not a glob,
regex, or whitespace-insensitive match. The new test exercises both
matches and the documented gotchas (double-space, case drift, tab).
- H3: every code path in CommandExecutor.Execute that converts a hook
failure into Allow via FailOpen now emits a WARN naming the hook
and the failure mode (timeout / launch_error / parse_error), so
chronic hook failure or abuse is visible in operator logs.
Also tightens errcheck on permission/rule.go (Printer.Print on a
strings.Builder cannot error in practice; make the intent explicit).
Plugin loader resolves HookSpec.Exec as a relative path joined to the
plugin directory, and manifest.checkSafePath rejects absolute paths and
'..' traversal — Exec was always meant to be an executable path.
The hook executor was wrapping it in 'sh -c', adding a redundant shell
interpretation step that turned any space, quote, or metacharacter in
the path into command-injection surface. Switch to exec.Command(path)
with no shell wrapping.
Closes audit finding C3. Adds a regression test that fails under the
old 'sh -c' code path: a canary file created via shell sequencing
remains absent when the executor treats Exec as a literal filename.
Hook command tests now write small /bin/sh scripts to t.TempDir and
point Exec at those — matching production semantics (resolved binary
path) rather than inline shell strings.
vikingowl