feat(strict): escalate to the human, on evidence - #11
Merged
Conversation
BYPASS.md said plainly that nothing here stops anything: a PreToolUse hook blocks only by exiting 2, no path does, and acceptance asserts none ever will. This changes that for temper's two strongest verdicts — but only where there is evidence, and only by handing the decision to a person. **Evidence first.** temper's push verdict fired on every push, because nothing recorded that a review had happened. A verdict that is always the same carries no information; as a prompt it would be clicked through within a week. /critique-diff and /critique-pr now call `aether review record`, which hashes the *content* of the diff rather than the commit. That is the decision the feature turns on: recording a SHA breaks on the most ordinary flow there is — review the staged diff, commit it, and the SHA has moved while the content has not. Content hashing survives commit, amend and rebase. Reviewed means the whole push diff matches a record, or every commit in it does. The second case is two reviews, two commits, one push — the ordinary way of working, which an exact-match rule would call unreviewed and be wrong. A third state, `unknown`, covers no sha256 tool, no .aether/, and no base branch. It never escalates and never blocks. Absence of evidence is not evidence, and a check nobody can satisfy gets the whole thing switched off. **Then escalation.** `[temper] strict: blocks` turns those verdicts into a permission prompt the agent cannot answer. It travels as `permissionDecision: ask` on exit 0, not as exit 2 — exit 2 is what bash returns for a syntax error, so a deliberate block and a half-written file would be indistinguishable. A syntax error cannot print valid JSON and exit 0, so breakage can neither be mistaken for a stop nor forge one. Global-only: a project switch would sit in the tree the agent is editing. Default off, and with it off the hook is byte-identical to 1.6.0. Deferred, having checked rather than assumed: the plan proposed wrapping the hook registration to normalise exit codes, on the grounds that a corrupt dispatcher exits 2 and blocks everything. True, but _op_copy already writes to a temp and renames, so the engine cannot leave a half-written file — the hazard is disk corruption or a hand edit, not an interrupted install. And the wrapper would break `basename "$cmd"` in the doctor and dedup paths. Smaller payoff, larger blast radius; it gets its own change. A test pins the atomic rename that is the actual mitigation. Two existing tests encoded the old assumption that a push is always a block and were corrected: test_gates now expects a nudge when there is no evidence, and test_hookcost tests blocks with a critical-path commit, which needs none. 53 new assertions; 811 across 13 files; acceptance 46/46.
Found by walking the feature through end to end rather than by a test. `aether_sha` lives in hooks/aether-config.sh, and bin/aether degrades to defaults when it cannot find that file — it prints a warning and carries on. So the function can genuinely be absent, and cmd_review called it anyway: two `aether_sha: command not found` lines on stderr per invocation. The outcome was already safe, because an empty hash falls through to `unknown` and nothing escalates on unknown. But the gate calls `aether review status` with 2>/dev/null, so the noise was invisible exactly where someone would look for it, and a record with an empty hash would have matched every other empty one had the guard order been different. _review_can_hash now checks the function exists and produces output, and both record and status ask it before touching a hash.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BYPASS.md says plainly that nothing here stops anything: a
PreToolUsehook blocks only by exiting 2, no path does, andacceptance.shasserts none ever will. This changes that for temper's two strongest verdicts — but only where there is evidence, and only by handing the decision to a person.The mechanism:
ask, not exit 2Exit 2 is not the only way to stop a tool call, and it is the worst one. Exit 2 is exactly what bash returns for a syntax error, so a deliberate block and a half-written file would be indistinguishable.
The other channel is exit 0 plus JSON, and
permissionDecisionacceptsask:{"hookSpecificOutput": { "hookEventName": "PreToolUse", "permissionDecision": "ask", "permissionDecisionReason": "temper: critical path file detected in staged changes …" }}The agent cannot answer that prompt. You do. For a tool whose threat model is "the agent writes the command the hook inspects", this is the first mechanism that puts the decision somewhere the agent cannot reach. It also needs no bypass marker — approving the prompt is the bypass. And a syntax error cannot print valid JSON and exit 0, so breakage can neither be mistaken for a deliberate stop nor forge one.
Evidence first, because a constant verdict is worthless
temper's push branch returned
pushunconditionally — nothing on disk recorded that a review had happened. Tolerable as a nudge; as a prompt it would be clicked through within a week, which teaches people to click through prompts and leaves things worse than before./critique-diffand/critique-prnow callaether review record. It hashes the content of the diff, not the commit, and that is the decision the feature turns on:Content hashing survives committing, amending and rebasing. The test file walks each of those.
Reviewed means the whole push diff matches a record, or every commit in it does. The second case is two reviews across two commits and one push — the ordinary way of working, which an exact-match-only rule would call unreviewed and be wrong about.
A third state,
unknown, never escalates. It covers a machine with no sha256 tool (aether_shafails closed by printing nothing, which is right for trust and wrong here), a project with no.aether/, and a branch with no base. Absence of evidence is not evidence, and a check nobody can satisfy is the fastest way to get the whole thing switched off.Three deliberate limits
[temper] strictis read from~/.aether/configalone. A project-level switch would sit in the tree the agent is editing; the value here is that turning it off is a write outside the work, which is conspicuous. Tested both directions — a project config can neither enable nor disable it.ask, notdeny.~/.aether/configor deregister the hook. This raises the cost of a bypass from appending a comment to editing a file outside the repository. BYPASS.md says so, at length.Default
off, and with it off the hook's behaviour is byte-identical to 1.6.0 — asserted, not assumed.Deferred, having checked rather than assumed
The plan proposed wrapping the hook registration to normalise exit codes, because a corrupt dispatcher exits 2 and blocks every tool call. The hazard is real, but smaller than I wrote:
_op_copyalready writes to a temp and renames, so the engine cannot leave a half-written file — it takes disk corruption or a hand edit. And the wrapper would breakbasename "$cmd"in the doctor and dedup paths, which identify hooks by path.Smaller payoff, larger blast radius. It gets its own change. A test pins the atomic rename, since that is the actual mitigation.
Two existing tests were wrong afterwards
Both encoded the old assumption that a push is always a block:
test_gates.shexpected rc=2 for a push. With no review record the state isunknown, which is advisory — it now expects a nudge, which is also what an install predatingaether reviewdoes, so old installs behave exactly as before.test_hookcost.shused a push to test "a block is never budgeted". It now uses a critical-path commit, which is decidable from the staged diff and needs no evidence. That also surfaced a fixture leak: a 250-line file staged by the previous suite madecommit_large(a nudge) win overcommit_critical(a block).Verification
tests/test_review.sh(25) — the flow matrix that is the design: amend-rewording stays reviewed, amend-with-new-content does not; two reviews one push; nothing-to-push short-circuits before nothing-recorded; andunknownproven three ways, including by putting every hasher out ofPATH.tests/test_strict.sh(28) — mostly about when it must not fire: nudges are never escalated, hostile payloads never escalate in either mode and never exit 2, the marker still suppresses, the switch is global-only, and stdout is exactly one JSON document (asserted with a parser, not a grep).acceptance.sh --full46/46;aether config doctorandaether docsboth clean.