Conversation
afogel
left a comment
There was a problem hiding this comment.
Sandi Metz review (naming + architecture + message passing)
Scope: stacked diff vs slice/v1 only.
Verdict: Best Metz extension in the stack — file-as-seam, NULL_TAP, parse→tap→dispatch→tap. Issues are naming / message shape on the reader side, not structure.
Full synthesis: Cursor canvas sandi-metz-v1-v3-review.
afogel
left a comment
There was a problem hiding this comment.
Follow-up: naming symmetry for observability roles
V3 will add an audit twin (AuditSink / AuditEntry / tailAuditLog / renderAuditEntry). Name the envelope side now so the two observability rails rhyme — same stems for the same jobs.
9989355 to
9775953
Compare
afogel
left a comment
There was a problem hiding this comment.
Second-pass Metz review (residuals)
First-pass naming wave largely landed (EnvelopeLogSink/EnvelopeLogEntry, decisionMessageOf, barrel hygiene, ALLOW (policy fired)). Residuals below are incomplete rename hygiene and U21 overloaded for errors.
afogel
left a comment
There was a problem hiding this comment.
Stack review update (V1–V4 in review)
Re-checked tip: open second-pass threads still open (tap path/docs hygiene; U21 warn prose; DecisionMessage/renderDecisionBadge overload for errors; --path naming).
V4 did not touch the Inspector; no new V2 residuals from this pass. Fix rename hygiene before the vocabulary calcifies further under V5+.
The rebase onto V1's review fixes broke two V2 tests, and the cause is worth naming rather than just fixing: both stand up a Guardian whose validate-envelope.ts has been replaced by a hand-written double, copying every other source file verbatim. V1's response to PR #10 added `isToolCallRequest` to that module and server.ts now imports it, so the doubles failed to import at all -- never reaching the pathological throw they exist to exercise. Each double now mirrors the real narrowing. It is unreachable there, since their validateEnvelope always throws before it, but a double that lies about behaviour is worse than one that fails to compile. This is the cost of a hand-written double over a real module, paid exactly where it should be: at the branch that owns the double, when the module it imitates changed underneath. README's count moves with the suite: 157 tests across 17 files. Addresses: #10 #11 review
PR #11's naming review, nine of its twelve findings: the writer's own nickname (`Tap`) was the vocabulary on both sides of S6 -- the Guardian that writes it and the Inspector that reads it -- so a reader of either side had to know how the log gets written before they could name what they were looking at. EnvelopeTap -> EnvelopeLogSink createEnvelopeTap -> createEnvelopeLogSink CreateEnvelopeTapOptions -> CreateEnvelopeLogSinkOptions NULL_TAP -> NULL_ENVELOPE_LOG_SINK TapEntry (both sides) -> EnvelopeLogEntry TapDirection -> EnvelopeLogDirection TailOptions -> TailEnvelopeLogOptions renderEntry -> renderEnvelopeLogEntry `tailEnvelopeLog` was already named this way, and it is the template the rest now match: every name on the rail says which log it is about, so a later slice's second stream reads as this one's sibling rather than as an unrelated mechanism. The Inspector still declares its own entry type rather than importing the Guardian's (R5.1) -- two declarations that now share a name are still two declarations, and test/envelope-tap-roundtrip.test.ts is what keeps them honest. Four filenames are deliberately unchanged (`packages/guardian/src/envelope-tap.ts` and the three `envelope-tap*.test.ts`): the findings are about public API vocabulary, and renaming the paths would churn them for no vocabulary gain. Behaviour is unchanged. The only strings that moved are two stderr diagnostics and one thrown message, none of them asserted anywhere: "envelope tap disabled after failure" -> "envelope log sink disabled after failure", and "does not match the TapEntry shape" -> "... the EnvelopeLogEntry shape". Addresses: #11 review
PR #11 review, SRP: the barrel exported `startGuardian` and `mapVerdict` beside `createEnvelopeLogSink`, `NULL_ENVELOPE_LOG_SINK`, `extractRpcId` and the entry types, so observability read as first-class Guardian vocabulary rather than an internal N26 detail of running one. Checked before removing, per the finding's own condition: nothing outside this package imported any of them from the barrel. `./server.ts` imports the sink relatively, and the two test files that exercise N26 on its own import `../src/envelope-tap.ts` directly -- which is what they already did. So this removes a public surface, not a dependency. The one affordance a consumer needs is untouched: `envelopeLogPath` on `StartGuardianOptions`. You tell a Guardian where to write S6; you do not assemble its sink yourself. The Inspector is unaffected in either direction -- it imports nothing from this package at all (R5.1) and re-declares its own entry type. Addresses: #11 review
PR #11 review, tell-don't-ask: renderDecisionBadge took a whole EnvelopeLogEntry and dug `entry.envelope.result.decision` out of it, so "render this decision" read as "ask this log line what it contains" -- and it tied U21 to the one artifact that happens to carry a decision today. The digging moves into `decisionMessageOf`, one named translation from an S6 line to the small message U21 speaks: DecisionMessage = | { decision, reason_codes, policy_references } | { error: { code, message } } A union, not one object with an optional `error` beside a `decision`: a response carries exactly one of them, and the other arm's fields would have to be invented. `renderEnvelopeLogEntry` now calls `decisionMessageOf` then the badge, and anything else that can build the message can use the badge without owning an envelope log at all. Behaviour is unchanged. Every rendered string is byte-identical, including the `✖ ERROR ?` fallback for a non-numeric JSON-RPC code, which used to be computed inline in the renderer and is now `code: null` in the message -- pinned by a new test so the equivalence is asserted rather than assumed. Two badge tests that asserted `renderDecisionBadge(...) === null` for a request and for a ServerHello now assert the same claim about `decisionMessageOf`, which is where "there is no outcome to badge" moved: the badge itself no longer has a null return. Same claim, new owner. 161 tests (was 157): the two moved, plus four that the reshape makes possible to state directly -- what the message narrows to, the error arm's null code, and a badge rendered from a hand-built message with no log entry in sight. README's count updated. Addresses: #11 review
…2 say so PR #11 review, domain vocabulary. The badge for an allow that carried policy_references read `◐ ALLOW (policy fired — ACS "warn")`. ACS has no `warn` disposition -- the comment directly above the line said so -- so the label was teaching the policy runtime's vocabulary from an ACS-first tool, which is precisely the leak R5.2 exists to prevent. Same colour, same glyph, same trigger; the label is now `◐ ALLOW (policy fired)`, which is what ACS itself says happened. And the gate that should have caught it: R5.2's term list held the word "verdict" but none of the five verdicts, so it looked like coverage while covering nothing -- the badge shipped with a fully green suite. `warn`, `escalate` and `transform` are now on the list, with a comment saying why those three and why `allow`/`deny`/`ask`/`modify`/`defer` are deliberately absent: those five are ACS's own dispositions and the Inspector must name them. Mutation-tested after fixing the label, not before: restoring `— ACS "warn"` to the label fails the widened gate with `{file: "render.ts", term: "warn", found: true}` -- naming both the file and the term -- plus the badge test that now asserts the label carries no such word. Restored byte-identically (sha256 verified) and the suite is green again. This is the one badge-label assertion the fix necessarily updates; the exact-string test for it was rewritten and gained `expect(badge).not.toContain("warn")` so the claim is pinned at the call site as well as by the gate. Addresses: #11 review
The affordance tables are how a reader finds code from an ID, so a row naming a dead symbol is the same ghost-name defect as the finding that started this. N26's label was `writeEnvelopeTap()` -- a function that never existed under that name in any tree -- and it now reads `createEnvelopeLogSink()` → `sink.write()`, which is what the module exports. The affordance ID is also spelled out in envelope-tap.ts's own header, so the trail runs both ways. Shaping doc: N26's row first, then the mermaid node rendered from it, in that order -- the tables are the source of truth and the ripple hook says so. Slices doc: N26's row in §V2, and the "the tap must be total" watch-for, which named `writeEnvelopeTap` as the thing sitting on the decision path. slices/v2/README.md: the same two names, `writeEnvelopeTap` and `TapEntry`. v2-runbook.md: not a rename -- the badge label it quotes stopped being what the tool prints when `— ACS "warn"` came out, so the captured rendering is corrected and the reason recorded beside it. A runbook that quotes output the tool no longer produces is worse than one that says nothing. `docs/superpowers/plans/2026-08-09-v2-envelope-inspector.md` is left alone on purpose: a plan records what was planned, and rewriting it to match later code makes it a worse record. slice/v1 set that precedent for V1's plan (067fcbd) and this follows it. Addresses: #11 review
The rename commit wrote this comment when the affordance tables still said `writeEnvelopeTap()`; the docs commit two later fixed the tables and left the comment asserting the old state. A module doc that misdescribes the doc it points at is the same ghost the whole wave was closing, one indirection further out. Addresses: #11 review
The six second-pass residuals on this PR, which are two pieces of work: a
rename that stopped at the type names, and a vocabulary that let U21 render
the one thing this slice says it does not produce.
## The rename reached the types and not the filesystem
The first wave renamed the artifacts (`EnvelopeLogSink`,
`createEnvelopeLogSink`, `EnvelopeLogEntry`, `NULL_ENVELOPE_LOG_SINK`) and
left the module path, the test filenames, the temp-directory prefixes and
most of the surrounding prose saying "tap". Both vocabularies stayed live,
which is the translation the rename existed to end -- and after a deliberate
artifact rename the filesystem is the loudest remaining teacher.
paths envelope-tap.ts -> envelope-log-sink.ts, and its three tests
with it. The roundtrip test is now
envelope-log-sink-roundtrip.test.ts, so it and
audit-sink-roundtrip.test.ts read as the pair they are.
temp dirs "acs-tap-*" -> "acs-envelope-log-*".
prose README, both slice READMEs, the slices doc's scope note, P3, P5,
both watch-fors and parked row 10, the shaping doc's D10 note,
and the V2 runbook.
One of those was not vocabulary but a false statement: the runbook told an
operator to look for `envelope tap disabled after failure (<path>):
<reason>` on stderr. The sink has emitted `envelope log sink disabled after
failure (...)` since the rename -- so the one line whose whole job is to be
grepped for was the one line that would not match.
## U21 badged errors as though they were decisions
`DecisionMessage` was a union whose second arm was a JSON-RPC error, and both
arms went through `renderDecisionBadge`. So the affordance whose job is
making an ACS decision legible was also what rendered responses carrying no
decision -- while this slice's own watch-for says a schema-invalid envelope
"is answered with a JSON-RPC error", deliberately not a `deny`, because N27
is V3. A type and a function saying otherwise taught the thing V3 exists to
change.
OutcomeMessage the discriminated union of what an S6 response line
reports: a decision, or the error that stood in for
one. `outcomeMessageOf` builds it (was
`decisionMessageOf`).
DecisionMessage now only ever a decision, which is what its name says.
renderDecisionBadge U21, decisions only. It can no longer be handed an
error: the type has no arm for one.
renderRpcError the error line, named as what it is.
renderOutcome dispatches; what the stream renderer calls.
The rendered output is unchanged, including the error line's exact text.
Two smaller ones in the same area:
U21's prose introduced the observe-only case as "how `warn` becomes
visible", minting a disposition ACS does not have -- the
same leak the badge string itself was fixed for in the
first wave, one surface over. R5.2 keeps this package
free of policy-runtime vocabulary, and a word in a doc
teaches it as effectively as a symbol does.
--path -> `--envelope-log`. A sibling audit stream lands in V3,
and a generic `--path` would have been the flag for
whichever stream came first, leaving the sibling to carry
the qualifier. Each flag now rhymes with the env var that
overrides the same thing.
WHAT DELIBERATELY STILL SAYS "TAP": envelope-log-sink.ts's and
tail-envelope-log.ts's own headers, where the old names are the record of
what changed and why; docs/superpowers/plans/, which record what was planned
(the same rule this branch applied to V1's handshake rename); and the shaping
doc's A5/B5 option rows, which are the menu the session chose from, before
the affordance existed to be named.
172 tests across 17 files (171 pass, 1 skip); typecheck clean.
Addresses: #11 review
The 48 review findings went onto slice/v1..v3 after this plan was written, and four of them landed work this plan assumed was V4's. Three of the four leave the plan naming a symbol or a rule shape that never shipped -- the same ghost-name defect PR #11 filed against N26, which is why this is a revision rather than a note to discover mid-task. Task 4 was the biggest task and is now the smallest change of the four: PR #10's review already made renderDecision name no host field, using an output: map of dotted host paths where a dotless path renders top-level. So the set/from/ top_level shape this plan specified is deleted rather than built -- it would be a second way to express what dotted paths already express. What is genuinely left is that `decisions` is one block shared by every hook, plus the three gates that require every decision to declare a permissionDecision PostToolUse does not have: assertRenderableDecisions at load, assertHostAcceptsEveryDecision in the shim, and the shim's missing-wrapper refusal. Each learns the hook; none is relaxed. Task 3 gains a ruling it has to obey rather than undo: assemble-snapshot now takes the narrow ToolCallRequestEnvelope and its own doc says a post_tool_call snapshot is a sibling type, not a widening. So the result path gets its own predicate, type and assembler, and the caller chooses -- resolveInterventionPoint already resolves the point and is already wired, which was half of what this task was for. Task 5's array rejection moved into modifications.ts. Task 10's gate already exists and passes with four of five terms, and its slices-doc amendments already landed with this plan, so it shrinks to widening the gate by updatedToolOutput and mutation-testing that term specifically -- the other four pass whether or not the fifth is listed. C8 is new and was not asked by anything: a clean PostToolUse allow renders nothing, which collides with two fail-open guards that are correct at PreToolUse and wrong at a gate where the tool has already run. Resolved without new hookmap vocabulary -- the allow entry declares one conditional field, additionalContext from reasoning, so the rule is non-empty while the output is, a plain allow renders {} and an observe-only allow reaches the transcript (R1.2). That is the gap V3 closed for PreToolUse's allow, closed the same way at the second gate. Slices doc: the renderDecision correction is struck through and rewritten, C8 added as a watch-for, the gate row corrected from "new" to "widened", the array row repointed at N7's extracted module, and the 347-test measurement marked as predating 49 added tests. N3's affordance row pins the signature Task 4 changes, so Task 4 amends the table in its own commit -- tables are the source of truth and are edited before anything rendered from them. Baseline at this tip: 396 pass, 1 skip, 0 fail (397 tests, 29 files). Slice: #5
Task 3 added `assembleResultSnapshot` beside `assembleSnapshot`, and the affordance tables still named only one of them. A row naming one of two sibling functions is how a reader gets from an affordance ID to half the code, which is the same ghost-name defect PR #11 filed against N26 and PR #10's review filed against four more rows. Table first, then the mermaid rendered from it. V4's own sentence called this "a post_tool_call branch". It is not: PR #10's review had already ruled on slice/v1 that assembleSnapshot takes the narrow ToolCallRequestEnvelope and that a later slice's post_tool_call snapshot is a sibling type beside it rather than a widening of it. Recorded as a correction so the sentence and the code agree, including that the dispatch goes on the predicate that narrowed the envelope -- never the method string, never the resolved point, since mapping.yaml declares six methods with points and this Guardian assembles two. Also recorded as a watch-for: validateEnvelope now payload-checks the result method, which moves a boundary. A malformed result envelope used to fall through to a bare method_not_dispatched -- which the host adapter reads as no decision arrived, and answers with the posture -- and now gets N27's honoured envelope_invalid deny. Intended and fail-closed, and the one place this slice changed what an existing method-shaped failure does. With it, the fact that outputs: [] is schema-valid, reaches the assembler, and is answered deny / runtime_error:path_missing. Plan: Task 3's Step 3 told the implementer to dispatch on the resolved intervention point and then forbade exactly that two paragraphs later. Corrected to name the predicate. That contradiction was mine; the implementer followed the governing instruction. Slice: #5
The rebase onto the redistributed V1/V2 left every file that did not
textually conflict still speaking the old names. Typecheck named them all;
this is that list, plus two things that are not renames:
buildEnvelope assertRenderableDecisions shape-checked the FLAT hookmap
entry (`permissionDecision` as a string). V1 made the
decisions block declarative, so the gate was checking a
shape the shipped hookmap no longer has -- and it is a
gate against a real fail-open (a malformed `modify` entry
throwing inside the shim's catch, where the posture
answers it as a proceed). Rewritten against S1's own
shape, enforcing per field exactly what renderDecision
enforces, so "loadHookmap accepted it" and "renderDecision
can render it" cannot come apart. It still names no host
field, which is what R3.2's gate requires of this module.
sessionConfig isSessionConfig was declared TWICE, byte-identical bodies
with different docs: V1 added it for the handshake's
pre-store validation, V3 added it for the file store's
read path, and neither conflicted with the other because
they landed in different parts of the file. One predicate
now, carrying both reasons -- two would have been free to
drift apart on what "usable" means.
The renames: handshakeResponder -> buildServerHello (guardian main and its
test), renderEntry -> renderEnvelopeLogEntry (inspector main),
guardianClient.post -> createGuardianClient().post and handshake ->
negotiateSessionConfig (client tests), and renderDecision's two-argument
form in the render-decision tests, whose assertions move to the adapter's
own output shape -- the wrapper and hookEventName belong to the shim, and
hosts/claude-code/test/wire-shape.test.ts is what pins those.
Tests are not green at this commit: V3's own fixtures and the wire-shape
pin still describe the pre-rebase shapes. That is the next commit.
Addresses: #10 #11 review
The rebase left the shim's own host-enum gate reading a hookmap shape that
no longer exists, and four fixtures/pins describing pre-rebase renderings.
348 pass / 26 fail before this; 374 pass / 1 skip after.
acs-hook assertHostAcceptsEveryDecision read `rule.permissionDecision`
off each decisions entry -- the FLAT shape V1's redistribution
replaced with a declarative `output` block. Against the shipped
hookmap it therefore read `undefined` for every entry and exited
2 on every hook: the shim could not run at all, which is what 17
of the 26 failures were. It now reads the literal declared at
the one output path this host cares about, which also closes two
cases the field check could not see -- an entry declaring no
permissionDecision path at all, and one sourcing it `from:` a
decision field instead of a literal. Both render JSON Claude
Code reads as no decision, i.e. both are the fail-open this gate
exists for. This is not a rename: it is the propagation 85cc4df
did for buildEnvelope's own gate and missed here.
fixtures build-envelope.test.ts's hookmap and loadHookmap cases, and
posture.test.ts's two written-out hookmaps plus its read of the
real one, all now declare `output` blocks. Two claims had to
move, because the adapter no longer knows the field they named:
"allow names no permissionDecision" is now "allow declares no
output block" (the permissionDecision half of that claim is the
shim's gate, exercised end to end in posture.test.ts), and the
third-entry case is malformed the other way an entry can now be
-- an output field naming neither `value` nor `from`, which is
the branch 85cc4df added and nothing covered.
wire pin Updated from a real run of the shim against a stub, not from
reasoning about what it should write, then read line by line.
Three of the six literals changed, all three because a V3 commit
changed the rendering deliberately: `allow` carrying reasoning
now renders permissionDecisionReason (812419f gave the entry a
reason path, and a fail-open proceed's reason had nowhere to go
before); `modify` renders permissionDecisionReason (0f95527) and
an `updatedInput` carrying the APPLIED tool input rather than
ACS's raw modifications object (b6f1566's N7 -- V1's version
reported a rewrite that could never take effect, R1.6). allow
with no reasoning, deny, ask and defer render byte-identically
to V1.
The ask and defer fixtures gained `ask_details`/`defer_details`.
N7 substitutes a deny for either decision when it cannot read
the window, so a bare fixture pinned that substitution instead
of the hookmap's `ask`/`defer` entry -- leaving two of the three
rarest renderings this file exists for unpinned. The
substitution is covered in validate-decision.test.ts, where it
belongs; both rendered literals are V1's, unchanged.
The stub answers `handshake/hello` now, as V1's own note here
said it would have to once the shim negotiated. Captured both
ways first: the bytes on stdout are identical, because every
case here is a decision that ARRIVES and no posture is ever
consulted. It also stops this suite reading and writing `.acs/`
under the repo's own cwd.
Addresses: #10 #11 #12 review
Nine of PR #12's findings, all in the adapter's vocabulary and the module boundaries that vocabulary was hiding. No behaviour change: 374 pass / 1 skip before and after, typecheck clean, and hosts/claude-code/test/wire-shape.test.ts untouched. decision family `AcsDecision` gains the two refinements the review asked for, both keeping the stem and marking the stage: `ValidatedAcsDecision` (was `ValidatedDecision`, which dropped the stem the Guardian's side established) and `FailureResolvedAcsDecision` (was `PostureDecision`, which named a posture -- proceed|deny -- for a payload that is a decision -- allow|deny -- so every reader translated). `DecisionInput` is gone as a fourth noun for the same role. The `deny` helper moves to decision-message.ts because two modules now substitute one, and two copies are free to drift on the reason_codes a machine reads. delivery kinds `DeliveryFailureKind` no longer carries the two kinds its own comments admitted were "not a delivery failure at all" -- so `classifyDeliveryFailure` no longer advertises a return it never produces. `HostFailureKind` holds them, and `StepFailureKind` is the union an audit entry's `failure.kind` can be. Narrowing rather than renaming is deliberate: the role really is delivery, so widening the name would have kept the lie under better spelling. The genuinely wider role is the private dispatcher, renamed `classifyByStage` -> `classifyStepFailure` so all three read as one `classify<Role>Failure` family. one resolution A posture became an outcome on one line of applyFailurePosture and a decision seventy lines later -- three encodings of one resolution, kept in step by nothing but proximity. RESOLUTION_BY_POSTURE declares the mapping once, typed off AuditEvent's own fields so the table and the artifact cannot drift. audit outcome KEPT as its own stem, with the reason written down. The review allows a third stem if the Inspector's badge needs one, and renderAuditEntry prints the outcome next to `posture=` on the same line: mirroring the posture prints one word twice, and mirroring the decision badges these lines ALLOW/DENY -- the envelope stream's vocabulary for a step whose decision arrived, which is the one thing no line in this log is. three modules validateDecision was 514 lines doing three jobs under a name that admitted one: it also APPLIED §6.3's rewrite and SUBSTITUTED expired ask/defer outcomes. modifications.ts now owns §6.3 (and owns no decision vocabulary, so it throws rather than denying); decision-expiry.ts owns the two expiry substitutions, including the ask-seconds vs defer-milliseconds difference that has its own paragraph there; validate-decision.ts is the switch that sequences them, which is also where Global Constraint 1 is legible -- the `default` arm IS "no branch may alter an arriving deny". The §6.3 apply tests move to modifications.test.ts with their bodies unchanged. The three "keep" findings are keeps: AuditSink, AuditEntry and NULL_AUDIT_SINK are untouched, and their two Inspector twins (TailEnvelopeLogOptions, renderEnvelopeLogEntry) already landed with PR #11's redistribution -- only a comment still naming the pre-rename `renderEntry` is fixed here. Addresses: #12 review
afogel
left a comment
There was a problem hiding this comment.
Sandi Metz re-review (V1–V5 stack)
Previously-fixed names still hold: EnvelopeLogSink, EnvelopeLogEntry, --envelope-log, OutcomeMessage / renderRpcError, Guardian barrel is governance verbs only.
No new V2 issues. A second host can share the Inspector without copying Claude.
afogel
left a comment
There was a problem hiding this comment.
Sandi Metz re-review (V1–V8 stack)
V2 tip unchanged. EnvelopeLogSink ∥ AuditSink still hold. No new issues. Status only.
afogel
left a comment
There was a problem hiding this comment.
V2 tip unchanged. EnvelopeLogSink ∥ AuditSink still hold. Status only.
afogel
left a comment
There was a problem hiding this comment.
V2 tip unchanged. Status only.
The rebase onto V1's review fixes broke two V2 tests, and the cause is worth naming rather than just fixing: both stand up a Guardian whose validate-envelope.ts has been replaced by a hand-written double, copying every other source file verbatim. V1's response to PR #10 added `isToolCallRequest` to that module and server.ts now imports it, so the doubles failed to import at all -- never reaching the pathological throw they exist to exercise. Each double now mirrors the real narrowing. It is unreachable there, since their validateEnvelope always throws before it, but a double that lies about behaviour is worse than one that fails to compile. This is the cost of a hand-written double over a real module, paid exactly where it should be: at the branch that owns the double, when the module it imitates changed underneath. README's count moves with the suite: 157 tests across 17 files. Addresses: #10 #11 review
Slice: #3 Affordances: U20, U21, N26, N50, S6
handleAcsRequest had no try around `await dispatch(...)`. dispatch rethrows any non-EnvelopeValidationError, and that rethrow is live rather than theoretical: validateEnvelope builds its Ajv registry lazily, on the first request instead of at boot, so a tree cloned without --recurse-submodules boots `bun run guardian` cleanly and then turns every request into a Bun.serve `text/html` 500. That is a fail-open of the shape V1 shipped three times. guardian-client calls res.json() unconditionally, so an HTML body throws `JSON Parse error: Unrecognized token '<'`; acs-hook.ts's catch-all exits 1 with empty stdout; Claude Code reads that as "the hook didn't fire" and the tool call proceeds ungoverned. It was also the exit S6 did not cover -- the client received a response the envelope log had no line for at all. The outer net returns a well-formed JSON-RPC error in the ACS-reserved band and puts it back on the tapped path. It is deliberately not an ACS `deny`: N27 stays V3's call. The inner catch around assembleSnapshot / bridge.evaluate / mapVerdict is unchanged. The covering test forces a real throw rather than asserting on a mock. It copies packages/guardian/src one directory deeper, where the schema path resolves to packages/spec/... and does not exist, so buildAjv throws a genuine ENOENT at request time through the real, current source. Without the fix all three of its cases fail; the first reports the HTML body it got. Also corrects the return-site count the comment used to justify this structure: dispatch leaves by six routes, not four, so N27 adds a seventh. Slice: #3
`if (added) wake?.()` sat inside the scan's try, after onMalformedLine was called. A reporter that threw skipped the wake-up, so an entry already pushed to `ready` sat undelivered with no further writes -- arriving only when an unrelated append, or the abort itself, incidentally fired `wake`. Hoisted into a finally. The throw also abandoned the rest of the batch. `offset` is already at `size` by then, so every complete line still sitting in `pending` was never re-scanned: no later tick had anything new to read. onMalformedLine now gets its own try/catch at its call site, so reporting one bad line cannot cost the good ones behind it. Both tests fail against the previous code. The first races tail.next() against a timer rather than reading through `collect`, because collect's own deadline aborts -- and stop() fires wake?.() on the way out, which is exactly the incidental rescue that made this look benign. poll()'s outer try/catch was carried through the task loop as "possibly unreachable, definitely untested". It was reachable, but via onMalformedLine -- which is the bug above, not a feature -- and that route is now closed. Its one remaining route, a failed read, is covered by a new test that points the tail at a directory: existsSync is true, st_size is non-zero, and readSync throws EISDIR every tick. Deterministic, no race. Removing the catch makes that throw escape a bare timer callback and take neighbouring tests with it. Slice: #3
The R5.1 import gate matched `from "..."` alone -- the one form nobody
reaching for a forbidden import by accident would use. Verified against the
real gate, by introducing each form into packages/inspector/src/index.ts:
the old regex caught `from "guardian"` and missed `import "guardian"`,
`await import("guardian")`, the type-position `import("guardian").TapEntry`,
`require("guardian")`, and `from "Guardian"` -- which resolves on macOS. The
widened matcher catches all six, plus the agt-bridge forms, and stays quiet
on code that merely mentions the word. Two unit tests pin each form so a
later simplification of the regex cannot quietly reopen one.
Type position matters as much as value position here: `import("guardian").T`
is erased at build time but is still a compile-time dependency on the
Guardian's type graph, which is precisely what R5.1 forbids.
readSourceFiles now asserts it found something. A renamed directory already
failed loudly -- Glob.scanSync throws ENOENT -- but a directory that still
existed with no non-test .ts under it passed with zero assertions. Proved by
moving the Inspector's sources into a nested test/ directory: with the check
both Inspector gates fail; without it all six tests pass, and the expect()
count drops from 99 to 43.
Slice: #3
"The same bytes that crossed the wire" was not true. The tap receives `await req.json()`, so it stores the JSON value the Guardian parsed, not the request's bytes: the parse collapses duplicate keys, canonicalises number literals (1.0 -> 1, 1e2 -> 100), and hoists integer-like object keys ahead of the rest. Tool argument names are host-controlled, so an `arguments` object carrying both "0" and "a" is a real shape, not a hypothetical. Corrected in wording, not in code. Storing raw bytes would make the entry's `envelope` field a string rather than a JSON value, which breaks the Inspector's pretty-printing and the round-trip contract test -- a worse trade than an accurate sentence. What stays true, and is now what the tree claims: no field stripping, no redaction, no reordering of anything we control. The over-promise originated in the plan's global constraint 11 and propagated verbatim, so it is fixed at the source as well as at each of its copies: render.ts, the runbook, the slice README, the shaping doc's S6 row, the slices doc's V2 watch-for, and two test titles whose assertions were always about value equality rather than byte identity (assertions unchanged; only the titles were overstating them). Also in this commit: - The Quickstart heading claimed "one command on a laptop" above five steps across three terminals. R7.1's own wording is "starts with one command", and "starts with" carries the whole claim. - renderEntry narrowed its JSON.stringify. It returns undefined for an entry with no `envelope` key -- which isTapEntryShape permits, by design -- and Array.join coerced that to a silent blank body. - Slices-doc risk row 10: the tap's two synchronous appendFileSync calls per request sit on the decision path and Bun.serve is single-threaded, so a slow filesystem blocks every in-flight request, not only the one being tapped. Correct for demo scale; recorded, not fixed. - README's Verify count, from the real number: 121 tests across 16 files. Slice: #3
…derr
Three deferred findings against the Guardian, closed together since they
touch the same two files.
server.ts's two catches (the outer net in handleAcsRequest, the
evaluation-failure catch in dispatch) echoed a real error's `.message`
straight through to the ACS client and into S6 -- and a real error, e.g. the
ENOENT a missing schema directory throws, names this machine's absolute
filesystem path in full. The fix is not a generic message: this demo's value
depends on the text staying diagnostic. `toRepoRelativeMessage`, one shared
helper, strips only the repo-root prefix (computed the same way MAPPING_PATH
already is) out of the message before it leaves either catch, leaving the
repo-relative remainder intact. Covered by a new test against
withSchemalessGuardian's real, relocated-source ENOENT: the client-visible
message no longer contains this repo's root, and still names the missing
path relative to it.
withSchemalessGuardian's scratch tree used mkdtempSync, so a killed run left
a randomly-named, unignored, un-excluded copy of the Guardian's own source
sitting inside packages/guardian -- which the root tsconfig's
`packages/**/*.ts` include would then hand to `bun run typecheck` as if it
were real source. Given a stable, predictable name instead
(tmp-schemaless-scratch), added to .gitignore and to tsconfig.json's
exclude, with a comment on why the tree has to live under packages/guardian/
at all (the relative-path trick validate-envelope.ts's own SCHEMA_ROOT
depends on, and Bun's workspace resolution for the copy's own imports).
Verified directly: a simulated killed-run leftover left `bun run typecheck`
clean and did not appear in `git status`.
The one test that exercises the tap's default (unsupplied onError) stderr
reporter printed a real ENOTDIR line into every `bun test` run. Spied,
silenced, and asserted on instead -- "reports once" is now checked at the
call site, not only claimed.
Also: a doc comment's SCHEMA_ROOT example was missing an `acs/` segment,
and extractRpcId/extractId's redundant `as {id: unknown}` casts are gone --
TypeScript's `in`-narrowing on the already-narrowed `object` type gives the
same shape without one.
Slice: #3
…uiet stderr renderDecisionBadge painted only the glyph and decision label when color:true, leaving the appended `reason_codes=[...]` and `policy_references=[...]` segments plain -- a coloured badge read as one coloured half and one plain half. Both segments are now painted dim; paint() no-ops when color is false, so the color:false output the existing exact-string tests assert is unchanged. Test written first, confirmed failing against the unmodified renderer, then fixed. Two more render.ts behaviours were true only by construction, never asserted: a policy_reference with no rule_id renders as the bare policy_id (ACS's schemas do not require rule_id, so this is a real shape, not a hypothetical one); and isTapEntryShape never constrains `envelope`, so a line whose envelope is absent, null, or a bare string is still accepted and delivered rather than reported as malformed -- load-bearing, since the Guardian must be able to tap anything that crossed the wire, including a malformed body it never got a real envelope for. Both pinned with tests that pass immediately, as they should for a pin-current-behaviour item; a one-sentence doc comment was added to referenceList for the first. tail-envelope-log.test.ts's three deliberately-adversarial tests (a throwing onMalformedLine, twice, and a read that fails every tick against a directory) printed real stderr into every `bun test` run. Spied, silenced, and asserted on -- "warns and retries" and "reports the throw" are now checked at the call site instead of only inferred from the process surviving. Slice: #3
…tract
invariants.test.ts's readSourceFiles filtered test files with
`!f.includes("/test/")`, which does not exclude a test/ directory sitting
directly under a scanned src/ -- Glob.scanSync returns paths relative to the
scanned root, with no leading slash, so such a directory produces a path
like `test/foo.ts` with no `/test/` substring to find. No scanned package
has that layout today, so this made the four R3.2/R3.3/R5.1/R5.2 gates
stricter than intended rather than weaker, but it was wrong as written.
Extracted into a named isUnderTestDir() using `/(^|\/)test\//`, which
catches a test/ segment at the start of the path as well as nested, without
false-positiving on a segment that merely starts with "test" (testing/,
latest/). New tests exercise the matcher directly, mirroring the existing
"the import gate itself" pattern; confirmed the old substring check would
have let invariants.test.ts's own directory through unfiltered.
envelope-tap-roundtrip.test.ts -- the contract test that catches drift
between the Guardian's TapEntry and the Inspector's independently-declared
one -- asserted request?.recorded_at's type and request?.method's value but
never response?.recorded_at or response?.method. Added both, so drift on
either side of the pairing is caught, not only the request side.
Slice: #3
The V2 runbook's "S6 carries raw tool arguments" section already named the parse-side canonicalisations JSON.parse applies (duplicate keys collapsed, number literals canonicalised, integer-like keys hoisted), then claimed "nothing between the parse and the file removed, rewrote, or hid any part of it" -- without naming two more exceptions to that claim, both on the stringify side rather than the parse side, since a stringify happens again every time S6's line is written and every time the Inspector pretty-prints it. A number literal beyond a double's range (1e400) parses fine, as Infinity, but JSON.stringify writes Infinity as null -- the field survives, its value does not. -0 survives the parse with its sign intact and loses it the same way, written as "0". Both verified directly. The adjacent load-bearing sentence -- ".acs/envelopes.jsonl records the JSON value the Guardian parsed, unmodified: no field stripping, no redaction, no reordering of anything we control" -- is exactly true and is unchanged. README's Verify section still said 121 tests from before this fix wave; updated to the real count, 127 tests across 16 files (126 pass, 1 skip). Slice: #3
…e root
Review finding against the previous wave's fix. toRepoRelativeMessage took
a pre-extracted string, and both call sites built it with
`error instanceof Error ? error.message : String(error)` -- a guard that
exists because a catch clause's binding is `unknown`, but whose *result*,
when `error instanceof Error` was true, the helper then assumed was a
string. `instanceof Error` does not guarantee that: nothing stops
`.message` from being reassigned to `undefined`, a number, or anything else
after construction. Such an Error made `message.replace(...)` throw
`TypeError: undefined is not an object (evaluating 'message.replace')`.
Contained when it happens inside the inner catch (dispatch's own throw is
itself caught by the outer net), but not when it happens inside the outer
catch itself -- nothing above handleAcsRequest catches it, Bun.serve's fetch
handler has no try, and it reproduces exactly the untapped HTML-500
fail-open this module's header exists to prevent. Confirmed reachable by
hand against the pre-fix helper before implementing.
Fixed by giving the helper the caught value itself, as `unknown`, and
coercing with String() on the whole thing -- total for anything a catch
clause can hand it. Both call sites now pass `error` directly, dropping the
duplicated ternary. Exported (the only exported internal in this module) so
its totality is asserted directly across a spread of unknown shapes, not
only through one HTTP round trip; a second, real integration test
(withNonStringMessageGuardian, structurally identical to
withSchemalessGuardian but swapping in a validate-envelope.ts test double)
still proves the outer catch itself stays total end to end.
Same touch, three more findings against the same fix:
- REPO_ROOT_PATTERN matched the repo root as a bare literal with an
optional trailing separator, so a sibling directory whose name merely
extends the root (a `_old` backup clone, say) had its shared prefix
stripped too -- not a disclosure, since it names an unrelated directory,
but a misleading diagnostic. Anchored with a lookahead requiring a path
separator or end-of-string right after the root.
- extractMethod's redundant `as { method: unknown }` cast, flagged in the
previous wave's report as intentionally left alone pending confirmation,
closed to match extractId/extractRpcId's style.
- withSchemalessGuardian's doc comment claimed the copy always gets its own
module instance. True for the first call under item C's fixed directory
name; a second call importing the same path gets Bun's cached module from
the first call instead, and its own freshly copied files go unread.
Benign today, corrected to say so precisely.
Also corrects this wave's own report: item G's fix makes the *filter* scan
strictly fewer files, not the four gates themselves behave differently --
the code comment already had this right; only the report's prose was off.
bun test: 132 tests across 16 files, 131 pass, 1 skip, 0 fail -- +5 tests
over the previous wave's 127. bun run typecheck: zero errors. README's
Verify count updated to the real number.
Slice: #3
Closes a residual the follow-up review surfaced and correctly parked -- unreachable from any throw site in this repo today, identical exposure existed pre-fix, out of that round's scope -- and the coordinator asked closed anyway: the outer net's stated job is holding for anything, "today" is doing real work in that sentence given V3 adds new routes through this exact path, and this project has already found five fail-opens of this general shape, the last introduced by a fix for a minor. Four shapes still threw out of the shipped function body, reproduced by hand before fixing: an Error whose .message is a throwing accessor; a value whose toString/valueOf both throw; a Proxy throwing on `get`; a Proxy throwing on `getPrototypeOf`, which breaks `instanceof Error` itself before String() ever runs. Fixed by wrapping the whole function body, including the instanceof check, in its own try/catch, returning a fixed "<unprintable error>" on any throw -- honest about total unreadability rather than guessing at a partial string. A comment states this is belt-and-braces, not a reaction to a live bug, naming all four shapes so a future reader doesn't go looking for the throw site that motivated it. Covering tests: one unit test constructing all four shapes directly against the exported helper; one new integration test over real HTTP (withFakeValidateEnvelopeGuardian, generalizing the previous wave's withNonStringMessageGuardian to take both a scratch directory and a fake validate-envelope.ts source) proving the outer net itself still returns parseable JSON-RPC rather than an HTML 500. That integration test uses the throwing-.message-accessor shape, not the getPrototypeOf Proxy the review led with -- discovered while wiring it up that dispatch's own `instanceof EnvelopeValidationError` check hits the identical getPrototypeOf trap first, so the trap's own thrown Error replaces the Proxy before it ever reaches toRepoRelativeMessage; the Proxy case stays covered at the unit level, where it is actually exercised. Also discovered while wiring up two different fake validate-envelope.ts sources: reusing one scratch directory for both let the second test silently inherit the first's cached module (Bun's module cache keys by resolved path), so each fake source now gets its own directory, both gitignored and tsconfig-excluded per item C's precedent. bun test: 134 tests across 16 files, 133 pass, 1 skip, 0 fail -- +2 over the previous wave's 132. bun run typecheck: zero errors. README's Verify count updated. Slice: #3
Read trace/otel-mapping.json against the pinned schemas. Two attributes it marks required have no wire source: `acs.evaluator` is not a field of AcsResult at all, and `acs.capability` maps to payload.capability, which hooks/tool-call-request.json leaves optional. The consequence is sharper than a missing field. A downstream consumer of the ACS wire cannot emit a conformant trace -- only the Guardian can, from process-local knowledge the contract does not carry. That cuts against R5.1/R5.2 and against V2's whole design, where S6 is readable by anything and the Inspector proves it by importing nothing. An OTel exporter reading S6 hits the same wall. Second place this project becomes a forcing function for v0.2 rather than a consumer of v0.1.0, after steps/modelCall. Slice: #3
The ACS Trace pillar was normative and unclaimed. V7 now measures it rather than emitting it: N49 resolves every attribute trace/otel-mapping.json marks required against the v0.1.0 wire schemas, U33 renders the result, and the matrix declares Trace as a pillar this implementation does not claim -- with the reason attached rather than left as silence. Two cells are already known red. acs.evaluator has no field in AcsResult at all; acs.capability maps to payload.capability, which the tool-call-request hook leaves optional. So a downstream consumer of the ACS wire cannot emit a conformant trace -- only the Guardian can, from process-local knowledge the contract does not carry. Scope boundary recorded: V7 measures, it does not build an exporter. An exporter would have to live in the Guardian for that same reason, which makes it a slice of its own. Also corrects V7's demo sentence, which promised "all green". This slice's own body has expected two honestly-red model-call cells since shaping, and D10 adds two more, so the sentence was already false against the doc containing it. A matrix that must be all green to count is a matrix under pressure to redefine the claim -- the opposite of what C2 exists for. It now asks for every cell resolved. README's V7 row follows. Slice: #3
The rebase onto V1's review fixes broke two V2 tests, and the cause is worth naming rather than just fixing: both stand up a Guardian whose validate-envelope.ts has been replaced by a hand-written double, copying every other source file verbatim. V1's response to PR #10 added `isToolCallRequest` to that module and server.ts now imports it, so the doubles failed to import at all -- never reaching the pathological throw they exist to exercise. Each double now mirrors the real narrowing. It is unreachable there, since their validateEnvelope always throws before it, but a double that lies about behaviour is worse than one that fails to compile. This is the cost of a hand-written double over a real module, paid exactly where it should be: at the branch that owns the double, when the module it imitates changed underneath. README's count moves with the suite: 157 tests across 17 files. Addresses: #10 #11 review
PR #11's naming review, nine of its twelve findings: the writer's own nickname (`Tap`) was the vocabulary on both sides of S6 -- the Guardian that writes it and the Inspector that reads it -- so a reader of either side had to know how the log gets written before they could name what they were looking at. EnvelopeTap -> EnvelopeLogSink createEnvelopeTap -> createEnvelopeLogSink CreateEnvelopeTapOptions -> CreateEnvelopeLogSinkOptions NULL_TAP -> NULL_ENVELOPE_LOG_SINK TapEntry (both sides) -> EnvelopeLogEntry TapDirection -> EnvelopeLogDirection TailOptions -> TailEnvelopeLogOptions renderEntry -> renderEnvelopeLogEntry `tailEnvelopeLog` was already named this way, and it is the template the rest now match: every name on the rail says which log it is about, so a later slice's second stream reads as this one's sibling rather than as an unrelated mechanism. The Inspector still declares its own entry type rather than importing the Guardian's (R5.1) -- two declarations that now share a name are still two declarations, and test/envelope-tap-roundtrip.test.ts is what keeps them honest. Four filenames are deliberately unchanged (`packages/guardian/src/envelope-tap.ts` and the three `envelope-tap*.test.ts`): the findings are about public API vocabulary, and renaming the paths would churn them for no vocabulary gain. Behaviour is unchanged. The only strings that moved are two stderr diagnostics and one thrown message, none of them asserted anywhere: "envelope tap disabled after failure" -> "envelope log sink disabled after failure", and "does not match the TapEntry shape" -> "... the EnvelopeLogEntry shape". Addresses: #11 review
PR #11 review, SRP: the barrel exported `startGuardian` and `mapVerdict` beside `createEnvelopeLogSink`, `NULL_ENVELOPE_LOG_SINK`, `extractRpcId` and the entry types, so observability read as first-class Guardian vocabulary rather than an internal N26 detail of running one. Checked before removing, per the finding's own condition: nothing outside this package imported any of them from the barrel. `./server.ts` imports the sink relatively, and the two test files that exercise N26 on its own import `../src/envelope-tap.ts` directly -- which is what they already did. So this removes a public surface, not a dependency. The one affordance a consumer needs is untouched: `envelopeLogPath` on `StartGuardianOptions`. You tell a Guardian where to write S6; you do not assemble its sink yourself. The Inspector is unaffected in either direction -- it imports nothing from this package at all (R5.1) and re-declares its own entry type. Addresses: #11 review
PR #11 review, tell-don't-ask: renderDecisionBadge took a whole EnvelopeLogEntry and dug `entry.envelope.result.decision` out of it, so "render this decision" read as "ask this log line what it contains" -- and it tied U21 to the one artifact that happens to carry a decision today. The digging moves into `decisionMessageOf`, one named translation from an S6 line to the small message U21 speaks: DecisionMessage = | { decision, reason_codes, policy_references } | { error: { code, message } } A union, not one object with an optional `error` beside a `decision`: a response carries exactly one of them, and the other arm's fields would have to be invented. `renderEnvelopeLogEntry` now calls `decisionMessageOf` then the badge, and anything else that can build the message can use the badge without owning an envelope log at all. Behaviour is unchanged. Every rendered string is byte-identical, including the `✖ ERROR ?` fallback for a non-numeric JSON-RPC code, which used to be computed inline in the renderer and is now `code: null` in the message -- pinned by a new test so the equivalence is asserted rather than assumed. Two badge tests that asserted `renderDecisionBadge(...) === null` for a request and for a ServerHello now assert the same claim about `decisionMessageOf`, which is where "there is no outcome to badge" moved: the badge itself no longer has a null return. Same claim, new owner. 161 tests (was 157): the two moved, plus four that the reshape makes possible to state directly -- what the message narrows to, the error arm's null code, and a badge rendered from a hand-built message with no log entry in sight. README's count updated. Addresses: #11 review
…2 say so PR #11 review, domain vocabulary. The badge for an allow that carried policy_references read `◐ ALLOW (policy fired — ACS "warn")`. ACS has no `warn` disposition -- the comment directly above the line said so -- so the label was teaching the policy runtime's vocabulary from an ACS-first tool, which is precisely the leak R5.2 exists to prevent. Same colour, same glyph, same trigger; the label is now `◐ ALLOW (policy fired)`, which is what ACS itself says happened. And the gate that should have caught it: R5.2's term list held the word "verdict" but none of the five verdicts, so it looked like coverage while covering nothing -- the badge shipped with a fully green suite. `warn`, `escalate` and `transform` are now on the list, with a comment saying why those three and why `allow`/`deny`/`ask`/`modify`/`defer` are deliberately absent: those five are ACS's own dispositions and the Inspector must name them. Mutation-tested after fixing the label, not before: restoring `— ACS "warn"` to the label fails the widened gate with `{file: "render.ts", term: "warn", found: true}` -- naming both the file and the term -- plus the badge test that now asserts the label carries no such word. Restored byte-identically (sha256 verified) and the suite is green again. This is the one badge-label assertion the fix necessarily updates; the exact-string test for it was rewritten and gained `expect(badge).not.toContain("warn")` so the claim is pinned at the call site as well as by the gate. Addresses: #11 review
The affordance tables are how a reader finds code from an ID, so a row naming a dead symbol is the same ghost-name defect as the finding that started this. N26's label was `writeEnvelopeTap()` -- a function that never existed under that name in any tree -- and it now reads `createEnvelopeLogSink()` → `sink.write()`, which is what the module exports. The affordance ID is also spelled out in envelope-tap.ts's own header, so the trail runs both ways. Shaping doc: N26's row first, then the mermaid node rendered from it, in that order -- the tables are the source of truth and the ripple hook says so. Slices doc: N26's row in §V2, and the "the tap must be total" watch-for, which named `writeEnvelopeTap` as the thing sitting on the decision path. slices/v2/README.md: the same two names, `writeEnvelopeTap` and `TapEntry`. v2-runbook.md: not a rename -- the badge label it quotes stopped being what the tool prints when `— ACS "warn"` came out, so the captured rendering is corrected and the reason recorded beside it. A runbook that quotes output the tool no longer produces is worse than one that says nothing. `docs/superpowers/plans/2026-08-09-v2-envelope-inspector.md` is left alone on purpose: a plan records what was planned, and rewriting it to match later code makes it a worse record. slice/v1 set that precedent for V1's plan (067fcbd) and this follows it. Addresses: #11 review
The rename commit wrote this comment when the affordance tables still said `writeEnvelopeTap()`; the docs commit two later fixed the tables and left the comment asserting the old state. A module doc that misdescribes the doc it points at is the same ghost the whole wave was closing, one indirection further out. Addresses: #11 review
V1 gained eight cases (isToolCallRequest, resolveInterventionPoint's table reads, the PolicyBridge role), so this number moved with them. Addresses: #10 review
The six second-pass residuals on this PR, which are two pieces of work: a
rename that stopped at the type names, and a vocabulary that let U21 render
the one thing this slice says it does not produce.
## The rename reached the types and not the filesystem
The first wave renamed the artifacts (`EnvelopeLogSink`,
`createEnvelopeLogSink`, `EnvelopeLogEntry`, `NULL_ENVELOPE_LOG_SINK`) and
left the module path, the test filenames, the temp-directory prefixes and
most of the surrounding prose saying "tap". Both vocabularies stayed live,
which is the translation the rename existed to end -- and after a deliberate
artifact rename the filesystem is the loudest remaining teacher.
paths envelope-tap.ts -> envelope-log-sink.ts, and its three tests
with it. The roundtrip test is now
envelope-log-sink-roundtrip.test.ts, so it and
audit-sink-roundtrip.test.ts read as the pair they are.
temp dirs "acs-tap-*" -> "acs-envelope-log-*".
prose README, both slice READMEs, the slices doc's scope note, P3, P5,
both watch-fors and parked row 10, the shaping doc's D10 note,
and the V2 runbook.
One of those was not vocabulary but a false statement: the runbook told an
operator to look for `envelope tap disabled after failure (<path>):
<reason>` on stderr. The sink has emitted `envelope log sink disabled after
failure (...)` since the rename -- so the one line whose whole job is to be
grepped for was the one line that would not match.
## U21 badged errors as though they were decisions
`DecisionMessage` was a union whose second arm was a JSON-RPC error, and both
arms went through `renderDecisionBadge`. So the affordance whose job is
making an ACS decision legible was also what rendered responses carrying no
decision -- while this slice's own watch-for says a schema-invalid envelope
"is answered with a JSON-RPC error", deliberately not a `deny`, because N27
is V3. A type and a function saying otherwise taught the thing V3 exists to
change.
OutcomeMessage the discriminated union of what an S6 response line
reports: a decision, or the error that stood in for
one. `outcomeMessageOf` builds it (was
`decisionMessageOf`).
DecisionMessage now only ever a decision, which is what its name says.
renderDecisionBadge U21, decisions only. It can no longer be handed an
error: the type has no arm for one.
renderRpcError the error line, named as what it is.
renderOutcome dispatches; what the stream renderer calls.
The rendered output is unchanged, including the error line's exact text.
Two smaller ones in the same area:
U21's prose introduced the observe-only case as "how `warn` becomes
visible", minting a disposition ACS does not have -- the
same leak the badge string itself was fixed for in the
first wave, one surface over. R5.2 keeps this package
free of policy-runtime vocabulary, and a word in a doc
teaches it as effectively as a symbol does.
--path -> `--envelope-log`. A sibling audit stream lands in V3,
and a generic `--path` would have been the flag for
whichever stream came first, leaving the sibling to carry
the qualifier. Each flag now rhymes with the env var that
overrides the same thing.
WHAT DELIBERATELY STILL SAYS "TAP": envelope-log-sink.ts's and
tail-envelope-log.ts's own headers, where the old names are the record of
what changed and why; docs/superpowers/plans/, which record what was planned
(the same rule this branch applied to V1's handshake rename); and the shaping
doc's A5/B5 option rows, which are the menu the session chose from, before
the affordance existed to be named.
172 tests across 17 files (171 pass, 1 skip); typecheck clean.
Addresses: #11 review
Same pass as the previous slice, over the envelope log and the Inspector:
review archaeology out ("PR #11 review, second pass", "Retitled by the
whole-branch review", "Backlog item K"), internal identifiers replaced by what
they stand for (S6 becomes "the envelope log", U21 becomes the badge it names,
R5.1 and R5.2 become the boundaries they assert), and the denser paragraphs
rewritten as ordinary sentences.
Claude-Session: https://claude.ai/code/session_019qZbQWyJHrYG7UodpyKjYr
Same rule as the previous slice: a test name is read in test output by someone with no access to the shaping documents, so it says what it checks rather than citing an affordance number for it. Claude-Session: https://claude.ai/code/session_019qZbQWyJHrYG7UodpyKjYr
The envelope-log round trip and the invariant gates still cited requirement and affordance numbers, and the Guardian's startup banner printed one to the operator's terminal, where it is least resolvable of all. Claude-Session: https://claude.ai/code/session_019qZbQWyJHrYG7UodpyKjYr
The rebase onto the redistributed V1/V2 left every file that did not
textually conflict still speaking the old names. Typecheck named them all;
this is that list, plus two things that are not renames:
buildEnvelope assertRenderableDecisions shape-checked the FLAT hookmap
entry (`permissionDecision` as a string). V1 made the
decisions block declarative, so the gate was checking a
shape the shipped hookmap no longer has -- and it is a
gate against a real fail-open (a malformed `modify` entry
throwing inside the shim's catch, where the posture
answers it as a proceed). Rewritten against S1's own
shape, enforcing per field exactly what renderDecision
enforces, so "loadHookmap accepted it" and "renderDecision
can render it" cannot come apart. It still names no host
field, which is what R3.2's gate requires of this module.
sessionConfig isSessionConfig was declared TWICE, byte-identical bodies
with different docs: V1 added it for the handshake's
pre-store validation, V3 added it for the file store's
read path, and neither conflicted with the other because
they landed in different parts of the file. One predicate
now, carrying both reasons -- two would have been free to
drift apart on what "usable" means.
The renames: handshakeResponder -> buildServerHello (guardian main and its
test), renderEntry -> renderEnvelopeLogEntry (inspector main),
guardianClient.post -> createGuardianClient().post and handshake ->
negotiateSessionConfig (client tests), and renderDecision's two-argument
form in the render-decision tests, whose assertions move to the adapter's
own output shape -- the wrapper and hookEventName belong to the shim, and
hosts/claude-code/test/wire-shape.test.ts is what pins those.
Tests are not green at this commit: V3's own fixtures and the wire-shape
pin still describe the pre-rebase shapes. That is the next commit.
Addresses: #10 #11 review
The rebase left the shim's own host-enum gate reading a hookmap shape that
no longer exists, and four fixtures/pins describing pre-rebase renderings.
348 pass / 26 fail before this; 374 pass / 1 skip after.
acs-hook assertHostAcceptsEveryDecision read `rule.permissionDecision`
off each decisions entry -- the FLAT shape V1's redistribution
replaced with a declarative `output` block. Against the shipped
hookmap it therefore read `undefined` for every entry and exited
2 on every hook: the shim could not run at all, which is what 17
of the 26 failures were. It now reads the literal declared at
the one output path this host cares about, which also closes two
cases the field check could not see -- an entry declaring no
permissionDecision path at all, and one sourcing it `from:` a
decision field instead of a literal. Both render JSON Claude
Code reads as no decision, i.e. both are the fail-open this gate
exists for. This is not a rename: it is the propagation 85cc4df
did for buildEnvelope's own gate and missed here.
fixtures build-envelope.test.ts's hookmap and loadHookmap cases, and
posture.test.ts's two written-out hookmaps plus its read of the
real one, all now declare `output` blocks. Two claims had to
move, because the adapter no longer knows the field they named:
"allow names no permissionDecision" is now "allow declares no
output block" (the permissionDecision half of that claim is the
shim's gate, exercised end to end in posture.test.ts), and the
third-entry case is malformed the other way an entry can now be
-- an output field naming neither `value` nor `from`, which is
the branch 85cc4df added and nothing covered.
wire pin Updated from a real run of the shim against a stub, not from
reasoning about what it should write, then read line by line.
Three of the six literals changed, all three because a V3 commit
changed the rendering deliberately: `allow` carrying reasoning
now renders permissionDecisionReason (812419f gave the entry a
reason path, and a fail-open proceed's reason had nowhere to go
before); `modify` renders permissionDecisionReason (0f95527) and
an `updatedInput` carrying the APPLIED tool input rather than
ACS's raw modifications object (b6f1566's N7 -- V1's version
reported a rewrite that could never take effect, R1.6). allow
with no reasoning, deny, ask and defer render byte-identically
to V1.
The ask and defer fixtures gained `ask_details`/`defer_details`.
N7 substitutes a deny for either decision when it cannot read
the window, so a bare fixture pinned that substitution instead
of the hookmap's `ask`/`defer` entry -- leaving two of the three
rarest renderings this file exists for unpinned. The
substitution is covered in validate-decision.test.ts, where it
belongs; both rendered literals are V1's, unchanged.
The stub answers `handshake/hello` now, as V1's own note here
said it would have to once the shim negotiated. Captured both
ways first: the bytes on stdout are identical, because every
case here is a decision that ARRIVES and no posture is ever
consulted. It also stops this suite reading and writing `.acs/`
under the repo's own cwd.
Addresses: #10 #11 #12 review
Nine of PR #12's findings, all in the adapter's vocabulary and the module boundaries that vocabulary was hiding. No behaviour change: 374 pass / 1 skip before and after, typecheck clean, and hosts/claude-code/test/wire-shape.test.ts untouched. decision family `AcsDecision` gains the two refinements the review asked for, both keeping the stem and marking the stage: `ValidatedAcsDecision` (was `ValidatedDecision`, which dropped the stem the Guardian's side established) and `FailureResolvedAcsDecision` (was `PostureDecision`, which named a posture -- proceed|deny -- for a payload that is a decision -- allow|deny -- so every reader translated). `DecisionInput` is gone as a fourth noun for the same role. The `deny` helper moves to decision-message.ts because two modules now substitute one, and two copies are free to drift on the reason_codes a machine reads. delivery kinds `DeliveryFailureKind` no longer carries the two kinds its own comments admitted were "not a delivery failure at all" -- so `classifyDeliveryFailure` no longer advertises a return it never produces. `HostFailureKind` holds them, and `StepFailureKind` is the union an audit entry's `failure.kind` can be. Narrowing rather than renaming is deliberate: the role really is delivery, so widening the name would have kept the lie under better spelling. The genuinely wider role is the private dispatcher, renamed `classifyByStage` -> `classifyStepFailure` so all three read as one `classify<Role>Failure` family. one resolution A posture became an outcome on one line of applyFailurePosture and a decision seventy lines later -- three encodings of one resolution, kept in step by nothing but proximity. RESOLUTION_BY_POSTURE declares the mapping once, typed off AuditEvent's own fields so the table and the artifact cannot drift. audit outcome KEPT as its own stem, with the reason written down. The review allows a third stem if the Inspector's badge needs one, and renderAuditEntry prints the outcome next to `posture=` on the same line: mirroring the posture prints one word twice, and mirroring the decision badges these lines ALLOW/DENY -- the envelope stream's vocabulary for a step whose decision arrived, which is the one thing no line in this log is. three modules validateDecision was 514 lines doing three jobs under a name that admitted one: it also APPLIED §6.3's rewrite and SUBSTITUTED expired ask/defer outcomes. modifications.ts now owns §6.3 (and owns no decision vocabulary, so it throws rather than denying); decision-expiry.ts owns the two expiry substitutions, including the ask-seconds vs defer-milliseconds difference that has its own paragraph there; validate-decision.ts is the switch that sequences them, which is also where Global Constraint 1 is legible -- the `default` arm IS "no branch may alter an arriving deny". The §6.3 apply tests move to modifications.test.ts with their bodies unchanged. The three "keep" findings are keeps: AuditSink, AuditEntry and NULL_AUDIT_SINK are untouched, and their two Inspector twins (TailEnvelopeLogOptions, renderEnvelopeLogEntry) already landed with PR #11's redistribution -- only a comment still naming the pre-rename `renderEntry` is fixed here. Addresses: #12 review
Closes #3
Stacked on
slice/v1. Review the diff against that base, notmain. Slice 2 of 8.What this slice is for
A third terminal prints the actual ACS JSON going back and forth, including envelopes that fail the schema.
Without it, you have to trust us. An ACS-first reader cannot see the contract without reading AGT.
V1 proved a deny can travel the wire. This slice makes that wire visible. The finished MVP still needs all verdicts, a second host, a scorecard, and a watch that the scorecard does not rot — but none of those are worth showing if the JSON itself is hidden.
What a reviewer is looking at
The Guardian writes each request and response to
.acs/envelopes.jsonl.bun run inspectortails that file and renders the stream. Pairing is by JSON-RPCid. The inspector imports nothing from the Guardian, the AGT bridge, or any host — so “inspectable on the wire” is a claim about the file, not about our type graph.A write failure disables the log and never changes a decision. A schema-invalid envelope still surfaces here as a JSON-RPC error, not as an honoured
deny(that is V3).The log carries raw tool arguments and is gitignored. It is a local demo file, not a signed audit chain.
What ships
packages/inspector/— terminal tailer and rendererguardianoragt-bridgeCaptured walkthrough:
docs/demos/v2-runbook.md.