* feat(#2796): reviewer lane as a fourth trust-disclosure class
Phase 3 of epic #2782, delivering ADR-2782 D5. A reviewer lane is piped the plan
text, requirements, research findings and CONTEXT.md decisions, and its output is
read back into REVIEWS.md -- an egress channel for the most sensitive artifacts
GSD produces. Making lanes pluggable WITHOUT a disclosure class would open a
data-exfiltration path behind a manifest field, which is why this gates the
feature rather than following it.
- discloseExecutableSurfaces was cyclomatic 51 / cognitive 99 / 110 lines with
risk_level critical. Rather than grow it, it is now a short orchestrator over
four extracted collectors (hooks, commands, mcp -- behaviour-preserving -- plus
the new lane collector), each independently testable. That is also what makes
the 80% mutation threshold survivable: 51 branches in one function cannot be
mutation-covered by whole-function tests.
- A spawn lane discloses its binary AND its full declared args, in rendered and
raw form. Binary-only disclosure would be insufficient and not hypothetically:
a lane declaring python3 with innocuous args could later change them to
['-c', '<program>'] without the binary changing. That is the bug class #1459
already fixed for MCP servers.
- An openai-http lane has no binary, so it discloses the destination host and the
config key naming it. A localhost destination is disclosed and distinguished
from a remote one. Both forms name the egress payload classes.
THE CONSTRAINT THAT SHAPED THE DESIGN: the lane element is appended to the
disclosure signature ONLY when at least one lane is declared. signatureForManifest
is the consent key both the loader and the lifecycle compare, so appending
unconditionally would have changed every installed capability's signature and
re-prompted every user for every capability on their next upgrade -- for a feature
they do not use. Two pre-change goldens are asserted byte-for-byte as the tripwire.
The resolved host is deliberately NOT in the signature. The loader has no config
resolver, so including it would make the loader and the lifecycle compute
different signatures for the same manifest and produce a permanent false-mismatch
loop. It is disclosed and recorded instead; Phase 5b re-resolves and compares at
invocation, which is D5 rule 4's own placement.
reviewsSection and timeoutFloorMs are also excluded from the signature: a cosmetic
change must not force re-consent, because a prompt carrying no security
information is how users learn to click through.
A lane's binary is NOT existence-checked against the staged bundle. It is a PATH
tool, never a bundle artifact; treating it like a hook script would add every lane
to missingArtifacts and block every lane install.
Two defects fixed beyond the fourth class:
- isLocalHostValue mis-parsed a scheme-less host: new URL('localhost:1234') does
NOT throw, it reads 'localhost' as the URL scheme and yields an empty hostname,
so a bare host:port would have been reported as non-local. Now falls back on an
empty hostname rather than only on a caught throw.
- The orchestrator's safeCollect closes a PRE-EXISTING totality gap in the other
three classes: a null manifest, or one with a throwing getter or Proxy trap,
previously threw out of disclosure -- which runs on an UNVALIDATED manifest at
install time. No well-formed input changes; all 51 existing trust tests pass.
Closes#2796
* fix(#2796): close four disclosure gaps found by the isolated security review
All four were REPRODUCED by execution against the shipped module, and all four
passed the existing 41-test suite while live -- each exists because the matrix
did not think to ask.
B (MEDIUM, reachable via plain JSON). Non-string argv members were folded into
the consent SIGNATURE but dropped from the human-facing text, because the summary
rendered the string-filtered args rather than the raw declared array. A manifest
declaring args ['--json', 7, {mode:'exfiltrate-everything'}, true] printed as
'--json' alone -- the host still receives the rest, so the user consented to a
surface never shown. That directly contradicts this design's own Kerckhoffs claim
that nothing about a lane is hidden. The summary now renders the raw array, with
non-strings shown in a visible form, and never throws on a circular or BigInt
member.
F (MEDIUM, reachable). The [local] flag is design-load-bearing, and it was
dropped for every loopback form except the dotted quad and the bare hostname.
Bracketed IPv6 was mangled by splitting on the address's own colons ([::1]:8080
became '['), and legacy IPv4 encodings were not recognised at all. A browser,
curl and the OS resolver all treat 127.1, 2130706433, 0x7f000001 and 0177.0.0.1
as loopback. isLocalHostValue now handles bracketed and bare IPv6, IPv4-mapped
loopback, and inet_aton shorthand/decimal/hex/octal. The dangerous direction was
already clean and is now pinned by tests: localhost.evil.com,
http://user@localhost@evil.com and friends stay REMOTE.
C (LOW). An empty reviewer body flipped hasExecutable true and perturbed the
disclosure signature, producing a re-consent prompt whose only content was
'(no binary declared)'. A prompt carrying no security information is the
click-through-training harm this design explicitly refuses for reviewsSection and
timeoutFloorMs; refusing it there and permitting it here was inconsistent. A body
declaring nothing recognised is no longer a lane. The test is deliberately broad
-- any ONE recognised field suffices -- because requiring specifically a binary,
or specifically a slug, would let a lane declaring only the other slip through
unconsented, which is the far worse failure. Pinned in both directions.
D (LOW-MEDIUM). Disclosure runs BEFORE validation, so a mis-cased or unrecognised
transport reaches this code. Keying on an exact string sent a lane that plainly
declares a hostConfigKey down the spawn branch, printing '(no binary declared)'
for a lane egressing to a live remote host, and left its resolvedHost blank --
which reads as 'no destination', the precise thing the design forbids. Both the
collector and the summary now branch on the declared SHAPE, so such a lane
discloses its key and either a resolved host or the explicit unresolved marker.
Two further findings were reproduced but confirmed NOT reachable through the real
pipeline and are recorded as known limits rather than fixed: a selective-throw
Proxy blanking a whole lane, and NaN/Infinity/undefined colliding to 'null' in a
signature. Every production manifest reaches disclosure through
readManifestBounded's strict JSON.parse, which cannot produce a Proxy, a getter,
a BigInt, a circular reference, NaN or Infinity. The 0/-0 sub-case IS reachable
via valid JSON but is inert -- String(0) === String(-0), so a spawned process
receives identical argv.
9 regression tests added (50 total in this file, up from 41).
* chore(#2796): backfill changeset pr number to 2826
69dbf28ca7