Conversation
afogel
force-pushed
the
slice/v5
branch
2 times, most recently
from
August 9, 2026 22:03
7940f9b to
6bde5e3
Compare
afogel
force-pushed
the
slice/v5
branch
2 times, most recently
from
August 10, 2026 07:15
48f41f6 to
e6eada7
Compare
afogel
force-pushed
the
slice/v5
branch
2 times, most recently
from
August 11, 2026 10:51
545568a to
2568560
Compare
afogel
commented
Aug 13, 2026
afogel
left a comment
Owner
Author
There was a problem hiding this comment.
Sandi Metz review — V5 (second host) + five-slice stack
Verdict: The second host proves the exchange (governStep / renderDecision / hookmap data), not an unchanged adapter. Host #1 source is +0/−0; adapter growth (mirrors, exit_status.from, load-time gates) landed once in the shared package. That is the stronger R3.4 claim.
Debt is twin-role naming that only lights up when both children exist, plus tools as shared schema that only OpenCode enacts.
Owner
Author
Round 3 response — all nine threads answered
Each finding was verified against the code before any fix was planned, and each fix was measured as a live fail-open before the rule that closes it was written.
Where I pushed back
Two mistakes of mine worth recording
Open, deliberately — the V6 listRanked by what actually leaks:
And the standing asymmetry, not this repo's to fix: a throw from host #2's load-time gate is only a log line — OpenCode unloads the plugin and governs nothing. |
decisions live in
CRITICAL 7B -- the request table's answer for `modify` is wrong whenever
the entry declares `outputs:`. governStep builds an output location off
the entry's SHAPE, so resolveModify fills `applied_output` and the
`args: { from: applied_input }` this gate demands becomes correct,
declared and unfillable. Measured: stage "honoured", rewrite nowhere,
secret ran. assertRequestGateDeclaresNoOutputs does not cover it -- it
returns early unless `arguments` is a non-empty string.
CRITICAL 7D -- `outputs.within` naming a container that is not the live
object. `within: $` renders a `result` key that satisfies the sink rule
and merges a clone of the whole PAYLOAD: leaf and mirror both keep the
plaintext and `tool`/`session_id`/`callID` land on OpenCode's live
result object as junk. `within: $.result.metadata` leaves the mirror
plaintext -- the leak `outputs.mirrors` exists to close, reached from
the container instead.
IMPORTANT 7A -- a declared decision name outside the tables was silently
unchecked, because the enforcement loop iterated the TABLE and not the
hookmap. Declaring an inert `decisions.block` is strictly worse than
omitting it: without the entry renderDecision throws and the posture
answers it, audited. Throw-not-skip now applies to decision names, the
way it already did to hook names.
7E, NOT on the directed list, found while measuring the others: the shim
asks governsTool with `input.tool` while governStep asks it with
whatever `tool_name` resolves to. This file's header has recorded since
Task 2 that they "would diverge, and nothing detects that". Measured
with `tool_name: $.args.command` beside `tools: [bash]` -- shim says
governed, governStep returns `ungoverned`, no Guardian request, no audit
entry, `rm -rf /` through. That sentence is retracted; the check exists.
7B, 7D, 7E and round 3's 6b are one fault, so they get one check.
`assertEntryMatchesGate` holds each entry to the gate the shim actually
runs it as: the payload shape it must declare, the one it must not, and
the `$.` paths the shim's own payload assembly fixes.
AND THE CONCERN THAT GENERATED 6a AND 7B BOTH -- the tables were
hand-derived from withResultOutput/resolveModify with nothing tying them
together, and a table that can be wrong is a fail-open generator. A
ninth gate in test/invariants.test.ts now runs the adapter's own
projection for every decision name at both gates and asserts the answer
matches the table the shim enforces. Verified non-vacuous: restoring the
pre-6a values fails it. Placed there for the reason that file already
gives for its siblings -- the claim needs two artifacts at once.
Mutation-tested six ways, each killing only its own: request entry check
3, result entry check 4, fixed-path loop 5, outputs.within 2, tool_name
1, unknown-decision throw 4. Variants 1-7 all refused across the probe
set; every legitimate configuration still accepted.
Minor: the header said "all three of these hold" for a list this diff
made longer again -- the count is gone rather than corrected.
call resolveModify instead of imitating it
CRITICAL, variant 8 -- and my own round-4 report understated it. I wrote
that `outputs.from` "would withhold the title instead of the output",
and declined it as the hookmap's business. `outputs.from` is also the
leaf that goes ON THE WIRE as this step's `outputs[0].value` -- the
value the policy runtime is asked ABOUT. Measured with
`from: $.result.title`, `within: $.result` (nests correctly, loads
clean): the envelope carried `cat .env`, the tool's own title, where the
shipped hookmap puts `rm -rf /`; the Guardian answered `allow`, entirely
correctly, for the value it was shown; render `{}`; nothing applied,
nothing thrown; `rm -rf /` delivered in leaf and mirror and audited as a
clean allow. Not "withholds the wrong field" -- never asks the question.
Which leaf is the output is as shim-fixed as which container is, so it
joins `RESULT_GATE_ENTRY.fixedPaths`.
Closing it also moves the incoherent `from`/`within` pair from a
runtime throw that a posture answers to a load-time refusal.
IMPORTANT -- the ninth gate never imported `resolveModify`. For `modify`
-- the only decision whose table entry differs between gates, and the
exact decision 7B was about -- it hand-seeded the field it then
asserted, re-implementing the `outputLocation === undefined` branch
inline, which at the request gate is a tautology. Verified: reintroducing
7B from the adapter side left the gate 35 pass / 0 fail. It now calls
`resolveModify`, with a real modification document per gate, because
`modificationDocumentOf` builds those differently for a request and a
result payload and handing the result gate a request-shaped one is a bug
that file already records. Verified the other way too: the same
adapter-side mutation now fails it.
The gate's header claimed it "runs the adapter's own withResultOutput/
resolveModify projection for every decision name at both gates", false
on both halves. Corrected to say which function answers for which
decision, and why the distinction is what makes the modify row real.
Recorded, not closed: a hookmap that omits `outputs.mirrors` is not
refused and leaks -- measured, the leaf gets the withheld marker and
`metadata.output` keeps the plaintext. `mirrors` stays free because
`metadata` is per-tool, which is the one thing only a hookmap can know;
the asymmetry with `from`/`within` is now stated in both the shim and
the hookmap rather than left to be rediscovered.
Mutation T (stop fixing outputs.from) kills 2. Full probe: variants 1-8
all refused across 18 fixtures, every legitimate shape still accepted.
…t at the pair it is named for Three non-behavioural items from the final review. The hookmap header said "THREE THINGS IN THIS FILE ARE NOT FREE CHOICES" over a four-bullet list, and the fourth bullet called itself "the sharpest of the three" -- the same Minor already fixed twice in acs-plugin.ts, in the file that teaches the rule. The count is retired rather than corrected, per this branch's no-frozen-counts discipline, with a line naming this as the third time a quoted count in this slice went stale on the line quoting it. result-gate.test.ts said `"bash" -- the tool's TITLE` directly above an assertion of `cat .env`. The round-5 correction landed in the assertion and not in the comment above it; `bash` is `tool.name`, and both are now named so they stay distinguished. And the "incoherent from/within pair" test did not exercise round 5's change: round 4's `within` rule already refuses that pair, verified by its still passing under mutation T. Retargeted to `from: $.tool` / `within: $.result` -- a correct `within` with `from` outside it, which only the `outputs.from` rule catches. Mutation T now kills 3, and the retargeted test is among them. Softened the side-effect claim with it. A `from` outside a correct `within` used to load clean and fail at stage "request", which is a failure whose OUTCOME DEPENDS ON THE NEGOTIATED POSTURE -- fail-closed throws out of governStep, `proceed` is an audited ungoverned delivery. That is narrower than "a posture-answered runtime throw moved to a load-time refusal", which is what the comment claimed.
hosts/opencode/acs-plugin.ts's two hooks made the same seven moves in the
same order, written out twice: validate `tool`, honour `tools`, validate
`sessionID`, assemble the payload, resolve the session config, govern the
step, apply what comes back. OpenCode's `Plugin` type needs two method
names; it never needed two copies of the exchange behind them, which is
what thread 3773262484 found. They are one module-private `handle` now,
and the two hook methods are edges: each names its own event, assembles
its own payload, and constructs its own live half.
Payload assembly stays at the edge, because that is where the gates
genuinely differ -- the request gate reads `args` off the mutable
`output` object, the only place OpenCode puts them there, while the
result gate reads `args` off `input` and hands the whole live
`{title, output, metadata, attachments}` object as `result`. The
assembler is called from the ORDERED position, after both validations
rather than before, and is handed the two values `handle` has just
checked, so a gate cannot put an unvalidated `tool` or `sessionID` on a
payload. The `{gate: "request"|"result"}` tag is carried through rather
than inferred from which field is present -- that inference is what Task
4 removed from the applier.
The order survives exactly, and every step of it is there for a measured
reason this file already records: `assertUsableTool` first, because
`Array.prototype.includes` answers a silent `false` for a malformed
`tool`; `governsTool`'s early return before `assertUsableSessionId`, so
an out-of-scope tool costs no session validation and no handshake;
`assertUsableSessionId` before `governStep`, because `buildEnvelope`'s
own throw lands in the stage-"request" catch and is answered by the
negotiated posture, where a `proceed` is an ungoverned step.
Merging removed the risk that two copies drift apart and introduced the
opposite one: a single later edit now reorders both gates at once.
hosts/opencode/test/gate-ordering.test.ts pins the order as behaviour,
the same three questions asked at both gates, each arranged so only one
ordering produces the observed answer. Verified non-vacuous by
reordering `handle` three ways: the tools check ahead of
`assertUsableTool` fails the first at both gates, `assertUsableSessionId`
ahead of the skip fails the second at both, and the session check moved
past the handshake fails the third at both.
The doc comments that carry measurements moved to whatever still
describes their subject rather than being collapsed: the result gate
keeps its own `tools: [bash]` scope, its missing-`metadata.exit`
posture-proceed analysis, and why a throw there does not mean what it
means at the request gate; the two `sessionId` forms and the reason
mixing them is a bug class moved onto `handle`, which is where both
calls now sit side by side. Comments that described the two hook bodies
-- in this file's header, in `GateEntryShape`, in `HOOK_EXPECTATIONS`,
on both assert helpers, and in apply-host-output.ts's own reasoning
about who constructs `live` -- say what the code does now.
bun test: 740 pass, 1 skip, 0 fail (734/1/0 before, +6 new).
bun run typecheck clean. verify-zero-diff.sh clean.
… test's write from failing another's assertion
Minor 1 and 2 are the same phrase in two files, both left behind by the
merge that made them wrong. `applyOpenCodeOutput` has ONE call site now,
fed by two constructors, so `assertEntryMatchesGate`'s message ("its two
call sites hardcode which live half the applier gets") and
apply-host-output.ts's own reasoning ("real protection for today's two,
fully-typed call sites") both name something that no longer exists. The
identical phrasing was corrected in `GateEntryShape` and in the paragraph
twenty lines above the second one, in the same commit -- these two were
missed. The reason given for leaving the first, that tests assert on
these strings, does not hold: the load-time-gate tests match
/declares no usable "outputs"/, /declares no usable "arguments"/ and
/"hooks\.tool\.execute\.after" declares no usable/, none of which reaches
this clause.
Minor 3 is the audit canary in gate-ordering.test.ts. Nothing on the
paths it exercises should write an entry, and nothing does -- but the
file redirected ACS_AUDIT_LOG once and asserted absolute non-existence
per test, so the moment a reordering let one case reach `governStep`,
that case's entry failed every LATER test's assertion too. Reproduced
with the sessionID check moved past the handshake: four failures, two of
them on tests the reordering never touched, reporting
`Expected: false / Received: true` and naming nothing about the defect.
The claim stays; the coupling goes -- the log is removed in a
`beforeEach`, so each case asserts what IT wrote.
Re-measured all three mutations against the decoupled suite. Each now
fails exactly the two tests it breaks, one per gate, and nothing else:
the tools check ahead of `assertUsableTool` fails the malformed-tool
pair; `assertUsableSessionId` ahead of the skip fails the out-of-scope
pair; the session check past the handshake fails the unusable-sessionID
pair.
bun test: 740 pass, 1 skip, 0 fail. bun run typecheck clean.
verify-zero-diff.sh clean.
…ed it Three comment-only repairs from the whole-branch review, all the class Global Constraint 6 exists to prevent: the repo saying something its code does not support. `MUST_WITHHOLD_BY_REPLACING` and `MUST_LAND_A_REWRITE` are in no tracked source -- fix round 3 replaced both with the `CARRIED_AT_*` tables -- and three references in acs-plugin.ts still read as current locators. They point at the tables now. Swept every backticked symbol in that file and in the hookmap against tracked source: those two, and nothing else. The names framed as retired (`isGovernedTool`, `assertRefusalRendersUnconditionally`, `assertRequestGateUnscopable`) stay as they are. "Three shapes are refused" stood above four bullets in the hookmap's own result-gate comment. Retired rather than corrected, like the five other sites, and for the reason the file's header already gives. Swept both files for the rest: five loadHookmap gates, four header bullets, the seven shared moves, six variants across four rounds, host #1's two additive test files -- each checked against what it counts, each holds. THE ONE THAT MATTERS. "Omitting `mirrors` is a session-record leak rather than a delivered secret" is true only while the result gate stays `tools`-scoped, and both sites said it without the condition. Measured here, not taken from the review: the shipped hookmap minus that gate's `tools: [bash]` registers clean through `AcsPlugin`, and a `read` payload then makes `buildEnvelope` throw on `$.result.metadata.exit`, which `governStep` answers at stage "request" with the delivery posture -- one audit entry, `outcome: "proceeded"`, and the file's contents standing in the leaf, unasked. Measuring it also corrected the review's own account of it. `mirrors` does NOT backstop that route: present and absent produce identical outcomes, because the envelope throw lands before `assertOutputIsReplaceable` is reached. What `mirrors` does refuse is a different payload -- one whose envelope builds while its declared mirror is absent -- where it stops the deployment loudly, a job it was never written for. So the two lines are independent, and a V6 reader deleting either should know which fault each is actually holding. No code changed. Closing the composite is V6's, named at all three sites: require `tools` where `exit_status`/`outputs` name per-tool fields, or move the hookmap-decidable posture faults to load time. The runbook's enumeration of those six gains no row -- a payload missing `metadata.exit` was never one of them -- but the bullet that already claimed unreachability now says what that claim rests on, since `assertEntryMatchesGate` pins `tool_name`, `outputs.from`, `outputs.within` and the payload shapes, and has no opinion about `tools`. bun test: 740 pass, 1 skip, 0 fail. bun run typecheck clean. verify-zero-diff.sh clean.
The re-review is right, and the imprecision was in the two sentences added to fix an imprecision. "`mirrors` has been refusing a fault it was never written for" describes measured behaviour correctly and then gets the intent backwards: `replacingOutput`'s own doc comment specifies that check by name -- a declared mirror "resolving to no value in the payload -- adds a field the tool never produced, the same defect an absent `from` would be" (result-output.ts, §V5 review, Important 2). The adapter overlooked nothing, and reading it as an oversight invites a V6 reader to go close a gap that is not there. What is actually unplanned is one step narrower. The guard runs over the mirrors a hookmap DECLARED, and the only reason this deployment declares one is the session-record leak `outputs.mirrors` was added for. A deployment declaring none never fires it -- correctly, by that guard's own design, having been told there is no mirror. So the protection is contingent on a declaration nothing requires, and deleting `mirrors` here disarms a working guard as a side effect of dropping a mirror. Both sites say that now, which is the distinction between "the adapter forgot to guard this" and "the adapter guards it, and whether the guard is armed depends on a line added for an unrelated reason". Cosmetic, in the same neighbourhood: the earlier splice left "And this does not weaken that scope's own justification" pointing at nothing nearby. The referent is named -- neither qualification, unreachable- through-config nor one-load-clean-edit-away, weakens why this gate is scoped to bash. bun test: 740 pass, 1 skip, 0 fail. bun run typecheck clean. verify-zero-diff.sh clean.
…s not Host #1's frozen hosts/claude-code/acs-hook.ts already owns assertHostAcceptsEveryDecision; host #2's own load-time gate had drifted to assertHostHonoursEveryDecision, splitting a shared assertHost*EveryDecision stem on nothing but a verb. A third host would have had to invent a third verb. Renamed host #2's function to match host #1's, so both hosts now express the same role under one name, with the mechanism difference carried in data (HOOK_EXPECTATIONS on host #1, CARRIED_AT_REQUEST_GATE/CARRIED_AT_RESULT_GATE on host #2) rather than in the name. Every comment that used the verb difference to mean "the other host's function" now says which file it means, since both files share the name from here on.
…ired The comment introducing this function already said what it is -- "the one exchange both gates run" -- but the function itself was still called `handle`, a name for a callback invoked when something happens. Claude's twin is `main()`, correctly named as a process entry; this extracted function is not a program entry, it is the shared body both OpenCode hook methods call. Renamed to `runExchange`, and updated every comment and call site that named it.
acs-plugin.ts's LiveHalf was a structural alias for apply-host-output.ts's LiveHookObjects, taken via Parameters<> because that type was module-private. Exporting the type removes the reason for the alias, so acs-plugin.ts now imports and uses LiveHookObjects directly. The doc comment on LiveHalf explained why the alias existed; that reason is gone, so the comment goes with it rather than being reworded to describe a type that is now simply imported.
The file's own function was already renamed to applyOpenCodeOutput (twin of asClaudeCodeOutput) in an earlier review round, but the file name stayed the generic apply-host-output.ts -- exactly the name that invited moving this into the shared adapter in the first place, which R3.2 forbids. Renamed to apply-opencode-output.ts (and its test alongside it) with git mv, and every import, path citation, and thrown-message prefix that named the old file updated to match. docs/demos/v5-runbook.md keeps one verbatim-captured log line naming the old file, per this file's own rule that a dated capture is reproduced unedited; its accompanying footnote now says so for the file name the same way it already did for the function's own earlier rename.
…sking a second source governStep derived the tool itself, by resolving the hookmap entry's own tool_name path against the payload, while each shim scoped on its own host field a call earlier. Two questions off two sources: a hookmap pointing tool_name elsewhere had the shim proceeding on one name while governStep scoped on another, measured as a governed step returned stage "ungoverned" with no Guardian request, no decision and no audit entry. host #2's load-time assertEntryMatchesGate refused that one hookmap, but the two-ask pattern lived in the adapter, so a third host copying it re-opened the gap. GovernStepInput now carries an optional scopedTool, and a gate whose entry declares a tools list refuses a caller that names none -- raised outside every try a posture is consulted from, because a posture-answered refusal would be an ungoverned step under proceed. Optional because host #1 declares no tools at either gate and its shipped source is frozen for this slice. toolNameFor is deleted rather than left unused: with the tool told, and a declared list refusing an untold caller, nothing it computed could decide a skip any more. assertEntryMatchesGate stays for what it still says -- tool_name is what names the tool on the wire, and re-measured with the tool told, a divergent path asks the policy runtime about a tool called "rm -rf /".
…an calling both a stop
…le basis for scoping GovernStepInput.scopedTool said what the field was for and never what it means to honour it. The contract is that a caller telling makes it authoritative about its own tool: a tools list is a list in the caller's vocabulary, checked against the caller's word, corroborated by nothing. That has a residual, now measured and written where a third host reads it rather than only in host #2's shim -- a gate listing "Bash" against a shim that dispatches "bash" skips every call silently and unaudited, where the derived name used to govern it. Corroboration was considered and refused: two vocabularies differing is what a legitimate host with qualified tool names looks like, so refusing on disagreement would re-introduce the second source this round removed and refuse those hosts. It is decidable in a host's own load gate instead, which is what host #2's assertEntryMatchesGate already is. The invariants gate's comment claimed tools at PostToolUse "would be legitimate, would work". Measured with tools: [Bash] added there and the shim invoked for Bash: exit 2 on every call, for the listed tool as much as an unlisted one. While acs-hook.ts is frozen and cannot tell, host #1 cannot declare tools at any gate; the gate's own diagnostic now names that fault first, and the applier fault second. The divergent-envelope test asserted only that a decision arrived, so a later permissive manifest would have turned it into an allow and left the over-blocking claim standing. It pins deny and runtime_error:tool_unknown -- the registry refusing a tool it does not know, not this deployment's rm -rf / rule, which the envelope never named.
…just the conclusions drawn from them The invariants gate's comment corrected its conclusion last round and left the captured evidence that produced it standing thirty lines above -- so the block said "tools at PostToolUse is exit 2" underneath a capture showing exit 0 with a clean wrapper, and the next reader trusts the capture. Same defect in the slices doc, where "host #1's PostToolUse ... a clean no-op" sat directly under an edit that corrected everything around it. Re-measured on this tree: real shim as a subprocess against a live Guardian, three hookmap configs by both gates by both tools, every payload carrying the same tool_response shape so only the tool name varies within a gate and no exit code can be blamed on an unbuildable envelope. tools at PostToolUse is exit 2 for Read as much as for Bash; the blast radius is the gate that declares the list, not the tool, with the other gate at exit 0 in both directions as a control; and the stderr is governStep's scopedTool refusal, not the applier's missing-wrapper message -- the applier fault is now unreachable through a tools list on this host, because the refusal preempts every render. The table replaces both captures, and the duplicate measurement fix round 1 added below it is folded in, since two measurements of one thing is how this drifted. The frozen hookmap was restored byte-identical, verified by sha256 either side of the run and by verify-zero-diff after it. Also softens a claim that outran its evidence: the adapter could export a reusable load-time helper for the tool_name third of host #2's gate, parameterised by a shim's dispatch-field path. It is not built, and why is now stated instead of asserting there is nothing the adapter could offer.
Twin of createFileSessionConfigStore, but the bare name read as the general factory and was not (PR #14 review). The symbol was introduced on slice/v1 (603882b); it lands here on slice/v5, where the review that found it lives, rather than rewriting four PRs currently under review to carry a rename their reviews did not ask for.
Every one is prose I wrote or edited in this round, and each was measured false
against the current tree rather than reasoned about.
govern-step.ts carried the twin of the round-3 tools captures that fix round 2
deleted from invariants.test.ts as "gone, not moved". Same drift, one file
over: it still promised a clean exit-0 no-op at the result gate and scoped the
blocking stop to unlisted tools, where the measurement is exit 2 at the gate
that declares the list, for the listed tool as much as the unlisted one. The
table stays in invariants.test.ts and both sites here cite it instead of
restating it -- two copies of one measurement is exactly how this happened.
The claim that assertEntryMatchesGate makes a hookmap's tools vocabulary and
its shim's "the same string by construction" is false: that gate pins
tool_name, outputs.from, outputs.within and each gate's payload shape, and
tools is precisely what its fixedPaths does not pin, as acs-plugin.ts already
said in plain words. Measured -- the shipped hookmap with the request gate's
tools recased to [Bash], one token, loads clean through loadHookmap and
assertHostAcceptsEveryDecision, registers both hooks, and skips a real bash
call carrying rm -rf / with no throw, args untouched, zero audit entries. The
hazard is pre-existing and unchanged by this round, since the shim already
skipped on that mismatch; what this round added was the claim it was closed.
Recorded as a residual with its measurement and a destination, beside the six
posture-seam faults, including why a manifest-registry check is not a drive-by:
on this host an over-refusal at load unregisters the plugin and the whole
session runs ungoverned.
assertUsableTool claimed a malformed tool would be silently false at both call
sites and uncaught by the adapter. Both halves false: with tools declared, 42,
null and {} all throw on the guard's own typeof half, 0 calls and 0 audit
events; with no tools declared, governsTool answers true, so the step is
governed and buildEnvelope reads the same malformed value from the payload.
That second case is why this boundary check is still right.
index.ts still told a third-host author, on the public export surface, that
"governStep asks it again so a shim that never asked still skips". A shim that
never asks and never tells does not skip -- it is refused.
Also: the untold test warned against a governsTool mutation it cannot catch,
because an untold scopedTool short-circuits before the predicate is reached
(measured: that mutation fails three tests, none of them the one warning about
it). It now says what it measures, and a told companion pins the half the
warning described. build-envelope.ts said "the one name it registers" against
three registered; README's ACS_DEBUG parenthetical was garbled.
…ce the reconstructed numbers described neither The three claims repaired here were rebuilt from memory after a `git checkout` destroyed the round's edits, and none of them survived being run again. `assertUsableTool`'s reason gave the unscoped-gate row as a governed step with the Guardian asked once. Measured against host #2's own hookmap with a gate's `tools` line deleted, it is `stage: "request"` with 0 Guardian calls and 1 audit event for `42`, `null` and `{}` -- the honoured row belongs to the empty string, which the paragraph never mentioned and which is the strongest reason the function exists: `buildEnvelope` checks that path for type and not for length, so `""` builds a real envelope carrying an empty tool name, and a live Guardian on this repo's own manifest denies `rm -rf /` under it with `runtime_error:tool_unknown` -- the registry, never this deployment's rule. The adapter promised that an empty render is fail-closed on every applier measured. Three of the four shipped applier/gate combinations are silent no-ops: host #2's applier lands nothing at either gate with `rm -rf /` intact, and host #1's result gate writes exit 0. Only host #1's request gate stops anything. That promise sat on the shared adapter, where a third-host author writing a no-op applier would have read it. Both sites citing `test/invariants.test.ts`'s twelve-row table as the evidence for the blocking stop now cite the four-row applier measurement instead: that table measures `governStep`'s own scoping refusal and records the applier fault as unreachable, so it never measured an applier at all. No test in the tree pins the four rows either, because nothing shipped can produce an empty render, and the comment now says so rather than pointing at a table that says otherwise. The tool-vocabulary residual's destination is named: V8, whose harness reports drift rather than refusing it, which is the direction that residual's own measurement argues for.
…not only through the loader The rewritten paragraph called the unscoped configuration one "nothing refuses", which was the earlier round's measurement of a different mutation (recasing) standing in for this one (deletion). Both copies were then run through AcsPlugin itself -- loadHookmap and assertHostAcceptsEveryDecision -- and register both hooks, so the sentence now reports what was run.
…ne is The comment said no test in this tree pins any of the four applier/gate rows. hosts/opencode/test/apply-opencode-output.test.ts pins the host #2 request-gate row three times over -- once for its own sake and twice as the control in a prototype-pollution test -- by handing the applier the same empty render governStep would produce. The other three rows really are unpinned, and no test drives the render through governStep to any applier, which is the claim that survives measurement. Also turns one cross-reference the right way round: GateEntryShape is above the paragraph citing it, not below.
Comments across hosts/opencode, packages/host-adapter, and their tests
accumulated review-round archaeology ("§V5 review round 3", "Task 5",
"fix round N", PR and thread citations), internal shaping identifiers
(S1, S14, R3.2, N27, and dozens more) that resolve only against
documents the reader does not have open, and prose compressed to the
point of being cryptic.
Every comment here now stands on its own: what the code does, the
invariant it protects, and the reasoning the code itself cannot show.
Where a constraint came from an identifier, the constraint is stated
in words instead. hosts/opencode/acs-plugin.ts carried the heaviest
load — its top-of-file header alone ran 336 lines of review narrative
and shrank to under 100 while keeping every load-bearing rule intact.
describe/it test names lost the same archaeology and identifiers, with
their descriptions kept intact or spelled out where a bare identifier
would otherwise leave the name vague.
No executable code, error-message string, fixture, or assertion value
changed — verified with a diff scan that every added/removed line is
either a comment or a describe/it string, plus a clean `bun run
typecheck` and `bun test` (747 pass, 1 skip, 0 fail, unchanged from
before this pass).
Claude-Session: https://claude.ai/code/session_019qZbQWyJHrYG7UodpyKjYr
Seven thrown errors ended with coordinates into this project's own review history -- "(§V5 review round 3, Task 5, fix round 4, Important 7A)". Someone whose hookmap is misconfigured reads these at runtime and has no way to resolve any of it. The diagnostics themselves, which are detailed and good, are unchanged. Claude-Session: https://claude.ai/code/session_019qZbQWyJHrYG7UodpyKjYr
…d it The OpenCode hookmap, the drift-demo manifest and the zero-diff script were the densest remaining archaeology: review rounds, task numbers, fix rounds, requirement and affordance numbers, none of them resolvable from this repository. Each stood in for a fact, and the fact is now written in its place -- including every measurement, which is kept. Two of the rewrites are load-bearing rather than cosmetic. The zero-diff script's two failure messages are read by whoever trips the gate, and they cited a requirement number instead of saying what the gate protects; they now state it. The drift manifest's header described itself by the slice that added it rather than by what it is: a second manifest that turns AGT's stock drift gate on so an observe-only warn is reachable, leaving the ordinary manifest untouched. No data changed. The hookmap's non-comment lines are byte-identical, so its load-time gates see the same document, and the zero-diff gate still reports zero.
…skip A hookmap entry that declares a `tools` list skips every tool the list does not name, and the skip built no envelope, consulted no posture, failed at nothing and recorded nothing. A list that stops matching the names a host actually sends therefore skips everything and produces a session that looks quiet rather than ungoverned -- and OpenCode's two gates are the only ones in this repo that declare `tools` at all. `AuditEntry` could not express it. Every field of the entry it had would have been a lie: `failure`, because nothing failed and the deployment's own hookmap asked for the skip; `posture`/`posture_source`, because no posture was consulted and at the shim's own call site no handshake has even run. So the type becomes a union discriminated on `outcome`, with a third value `ungoverned` carrying the tool that arrived beside the list that declined it -- neither half is the drift signal alone. `AuditEvent` distributes its `Omit` so each arm keeps its own fields, and the roundtrip contract test compares the two declarations arm by arm, since `keyof` over a union is the intersection of its arms and would have quietly stopped checking anything. Emitted from both call sites, which is a requirement rather than a courtesy: the shim's own `governsTool` check returns one call before `governStep`, so auditing only the adapter's copy would leave the record depending on which skip got there first. That also moves `assertUsableSessionId` ahead of the scope check in the OpenCode shim -- the entry is filed against a session, and one filed under an empty id joins to nothing. The handshake, which is what the old ordering actually saved, still happens only for a governed tool. Volume is the known cost and is stated at the type: OpenCode fires the result gate for every tool while the gate lists `bash` alone, so an ordinary session's log is mostly this arm. That is the honest count of the steps the deployment does not govern. Not de-duplicated per session, because the host that could hold that state is not the host whose hooks are fresh subprocesses, and the two must record the same things.
`AcsPlugin` hands OpenCode its gates as object keys, and OpenCode dispatches by reading them -- `const hook = plugin[eventName]; if (!hook) continue;`, out of 1.18.18's own compiled `Plugin.trigger` rather than off its published types. A key that stops matching the name OpenCode fires is never called: no throw, no exit code, no audit entry, an entirely ungoverned session that looks like a quiet one. The Claude Code shim fails closed in the same situation, so this host was the weaker of the two and nothing said so. `expectationFor` already refused the converse -- a hookmap entry for a hook this plugin never registers, which "would sit in the hookmap looking like governance and govern nothing". This adds the missing direction, comparing `HOOK_EXPECTATIONS`'s own keys against the object the factory actually returns, so a rename can no longer be half-applied: either half now stops the deployment at load. It cannot see an upstream rename, and the doc comment says so rather than letting a reader take the check for more. Rename both sides and the internal agreement still holds -- there is nothing to compare against, because the plugin API hands a factory a context and takes back a bag of keys without publishing which names it will fire. A test asserts that limit directly, so the gap is pinned rather than described. Deliberately NOT checked: that every registered hook is one the hookmap declares. That direction is real but refusing it here is the wrong answer, for the reason `satisfiesGate` already gives about over-refusal on this host -- a throw from this factory leaves the plugin unloaded and the whole session ungoverned, so refusing a hookmap that declares one gate and not the other would trade one governed gate for none. Measured instead: `governsTool` answers true for an unmapped hook and `governStep`'s first guard throws, at the first call of that gate, before anything is asked or audited. The refusal cases inject the defect into a copy of the real shim rather than exporting the check: this file exports exactly one symbol because a second export makes OpenCode's loader mis-invoke it and disables governance for the session, so the gate is driven over real code instead of a reimplementation.
The result gate declared no ask and no defer, so either arriving there made renderDecision throw, which governStep answers with the negotiated posture -- and under the shipped default, proceed, that delivered the tool's output, secret included. The request gate held both closed. Declarable only now: withResultOutput attached applied_output for deny alone, so the sink this gate mandates rendered nothing for ask and defer however correctly it was written, and acs-plugin refused the declaration for exactly that reason. withholdsAtResultGate widened the producer to every disposition but allow and modify, so the mandated sink is now the one that works. CARRIED_AT_RESULT_GATE, the gate's carries-nothing error factory, the hookmap's own account of why those two were exempt, and the invariant suite's mirror of the shim table all said deny alone. They say the rule now. The two result-gate tests that measured the leak assert the withholding instead -- leaf and mirror both -- and the load gate that refused the mandated sink accepts it. What it costs is stated where the entries are: OpenCode cannot put a question to a human about an output that already exists, so an ask a human would have approved is over-blocked, silently.
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
1 participant
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.
Closes #6
Stacked on
slice/v4. Review the diff against that base, notmain. Slice 5 of 8.What this slice is for
OpenCode is governed by the same Guardian, same bundle, same policy. The Guardian, the AGT bridge, and AGT itself do not change.
Without it, you still have one host. That is a plugin, not a wire contract. The M×N claim is unproven.
V1–V4 made Claude Code honest — visible wire, all verdicts, result-gate redaction — so this slice is not “OpenCode might work.” It is the architectural punchline: two structurally different hosts, one policy runtime, zero AGT code for the second. Session memory and the scorecard are still V6–V8.
What a reviewer is looking at
Claude Code is a subprocess that prints JSON. OpenCode is a long-lived in-process plugin that mutates live objects. Both run the same
governStep→renderDecisionpipeline. This host applies the rendered output instead of printing it.The one deployment-side edit is an additive manifest
tools:entry: OpenCode’s shell tool isbash, Claude Code’s isBash, and an unregistered name fails AGT closed before any rule runs.Host #1’s shipped source is unchanged. Adapter changes land in
packages/host-adapter, which both hosts already share.What ships
hosts/opencode/— plugin shim, hookmap,applyOpenCodeOutputoutputs.mirrors(OpenCode’smetadata.outputotherwise keeps the secret),tools:scoping in the adapterscripts/verify-zero-diff.shOpenCode catches a throwing plugin factory and continues unloaded — host #2 cannot hard-stop at load. That is a known limit, not a silent omission.
Captured walkthrough:
docs/demos/v5-runbook.md.