fix: sanitise the relay's strings on the headless pairing path

[nicodes]

Important

Correction, added after Review 1. Struck: "the one place a relay string reached an output without going through sanitize", in the sentence below beginning "headlessCodeDisplay printed the relay-supplied...". It was the one place the pairing code did — which is what nicodes/ormos-be#320 says — and generalising it to "a relay string" overstated the coverage. Two other relay-controlled strings still reach an output unsanitised, one of them on a live terminal, tracked in nicodes/ormos-be#420. The code change in this PR is unaffected; only the claim was wrong.

headlessCodeDisplay printed the relay-supplied VerificationURL and UserCode
with a plain Fprintf — the one place a relay string reached an output without
going through sanitize (struck: it was the one place the pairing code did;
see the correction above).

Inert where it is printed today: headless means no TTY, so the bytes land in a
pipe or a journal where an escape sequence does nothing. The exception is
closed rather than justified because its safety comes from where the output
happens to go rather than from the output itself, and journalctl on a
terminal is that assumption ending.

sanitize, not stripCtl: stripCtl drops C0 and DEL and leaves the C1 controls
(U+009B is CSI) and the Cf format characters, and U+202E can rewrite the
reading order of the one string whose whole purpose is telling the operator
which code to trust.

Test description, updated after Reviews 2 and 3. The test now asserts, per SLOT rather than anywhere in the output, that BOTH the sanitised URL and the sanitised code survive on their own indented line — so it cannot pass by printing nothing, by dropping the URL, by transposing the two fields, or by burying a payload in the trailing sentence. Each of those four mutations was shown to fail.

The test reuses the pairing screen's own payloads, from
TestPairingScreenStripsEscapesFromTheRelay and
TestPairingScreenStripsC1AndFormatCharacters, so the two outputs are held to
one standard rather than each to its own. It covers both framings — a first
code and a re-issued one — and asserts the code still survives as inert text,
so it cannot pass by printing nothing.

Closes nicodes/ormos-be#320

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

[nicodes]

Review 1

Produced by the reviewer agent role. Engine note: the opencode reviewer could not run — its model quota is exhausted for this billing cycle — so this was produced by a Claude reviewer carrying .opencode/agents/reviewer.md's role definition verbatim, with the branch-diff scope pinned per the nested-repo rule.

Redaction notice. This review is posted verbatim except for finding 1, where the specific unfixed code paths, their call chains and a worked payload have been removed. This is a public repository and that material describes where a protection currently fails, with enough detail to act on, before a fix exists. The finding's conclusion — that this PR's central claim is overstated — is kept in full, because the false claim is itself public and has to be corrected where it was made. The redacted detail is filed on the private tracker as nicodes/ormos-be#420. Nothing else is altered, except that invisible control characters in the review's prose are written as code points (U+202E, U+009B) rather than embedded literally.

---

Findings

1. The PR's central claim is overstated — headlessCodeDisplay was not the only place a relay string reached an output unsanitised (medium; documentation/evidence defect, not a code defect)

The PR body and the commit message both say:

"the one place a relay string reached an output without going through sanitize"

I verified this independently by enumerating every writer in the agent (grep for Fprint*/fmt.Print*/logf across the module) and tracing each argument back to its source. Two live counterexamples remain after this PR.

[REDACTED: two file:line locations, their call chains from relay-controlled input to the writer, a worked payload, and the reason one of them is reachable on an interactive terminal rather than the inert headless path. Filed as nicodes/ormos-be#420.]

One point from the redacted material does belong here, because it is about this PR's wording rather than about the weakness: the path this PR hardened is, by the PR's own argument, the inert one (no TTY). At least one path still open is the interactive one. That inverts the PR's framing of which case mattered.

Note the issue's wording is accurate and the PR's is not: nicodes/ormos-be#320 says "the one remaining place the pairing code reaches an output without going through sanitize". The PR generalised "the pairing code" to "a relay string". Per this project's evidence standard ("Never overstate coverage"), the broadened claim is the defect — correct it in place on the PR, and file a follow-up.

[REDACTED: the list of sites checked and cleared, which scopes the follow-up. Included in nicodes/ormos-be#420. One item is safe to state and worth stating: login.go:87 prints a relay-supplied name with %q, and I confirmed %q escapes both ESC and U+202E via strconv quoting, so it neutralises without sanitize. Strictly that is another relay string reaching stderr without sanitize, which is why the claim's phrasing matters.]

2. The new test's "payload survived" guard covers the code but not the URL (low)

login_test.go asserts strings.Contains(out, tc.survives) where tc.survives is only ever the sanitized UserCode ("AB]0;OWNED[2JCD", "ABCDEF"). A sanitize that deleted the entire VerificationURL would still pass the new test — the stated purpose of that assertion ("otherwise this passes by printing nothing") is only half met. Mitigated at suite level: TestHeadlessCodeDisplayShowsCodeAndURL (login_test.go:394-414) asserts a benign URL and code round-trip and now exercises the sanitize path, so the regression is caught — just not by the test whose comment claims to catch it.

3. No gate enforces the invariant (informational)

There is no CI check tying relay-sourced strings to sanitize — .github/workflows/{ci,actionlint,release}.yml contain no such rule, and nothing in Makefile/scripts greps for it. The invariant rests entirely on per-site unit tests, so a newly added print site fails nothing. Given finding 1 shows two sites were missed by inspection, this is the durable fix rather than another point patch.

4. A wholly non-printable code degrades to a blank line, in both paths (informational, pre-existing)

deviceStart validates out.UserCode != "" before sanitising (login.go:158), so a UserCode of two U+202E runes passes validation and sanitize reduces it to "". Headless then prints an empty code line; the TUI (login_tui.go:76 + View's if m.code == "") sits on "requesting a pairing code…" forever. Both are the safe failure direction and neither is introduced here, but the two paths now share the quirk, so the follow-up may as well cover both.

Verified positives

Method, stated exactly as run — I made no edits to the repository and ran nothing against a working tree.

• Scope. gh pr diff 61 --repo nicodes/ormos and git -C .../ormos-320 diff origin/main...HEAD are byte-identical; branch fix/320-sanitize-headless-pairing, one commit, working tree clean.
• sanitize vs stripCtl is load-bearing, and provable. I rebuilt both helpers plus the format string in a standalone program outside the repo and ran the test's own two payloads and assertions through three mappers. Results: identity (pre-fix) FAILs both cases on both the forbidden and survives assertions; stripCtl PASSes "C0 escapes" but FAILs "C1 and format characters" on U+202E and U+009B; sanitize PASSes both. So the new assertions are reachable, they do fail without the fix, and the test genuinely discriminates the chosen helper from the rejected one. (I could not prove failure by reverting in-repo — that would mutate the checkout — so this is an equivalent-code harness, not the real package.)
• No mangling of legitimate input. Probed through sanitize: a query-and-fragment URL, a UTF-8/percent-encoded path, IDN punycode, ABCD-1234, a space-containing code, and an emoji — all returned byte-identical. Only genuinely non-printable runes are removed: U+200B ZWSP and U+00A0 NBSP are deleted (both are non-print per unicode.IsPrint, which admits only ASCII space among separators). Neither is legal unencoded in a URL nor present in a device code, so no behavioural regression for a non-hostile relay — but note the semantics are deletion, not escaping, so such a string would be silently altered rather than visibly quoted.
• Newline stripping cannot break the output. The format string supplies its own newlines around both %s fields; removing newlines from the fields only prevents the relay from adding rows.
• ExpiresIn stays %d and is bounded by maxDeviceFlowSeconds at login.go:159 — no injection through the number.
• Test mechanics are sound: fresh strings.Builder per iteration, both restarted framings, subtests over a map with no shared state or t.Parallel hazard.
• Gates. go build ./... clean, go vet ./internal/system/ clean, go test ./... green (all three packages), targeted run of the new plus both sibling pairing tests green. CI on the PR: actionlint pass, ci pass, darwin pass.

Residual risks and testing gaps

• Nothing covers the two unsanitised sites in finding 1; no test would fail if a relay put an escape into an error body today.
• No integration-level test asserts "the headless start prints nothing a terminal will interpret" end to end; coverage is per-function only.
• sanitize addresses only non-printable runes. A relay can still make the printed URL misleading with entirely printable text (a userinfo-prefixed authority, homoglyph domains). Not claimed by this PR, and out of its scope, but the pairing screen's threat model ("telling the operator which code to trust") is not fully closed by escape-stripping.

The code change is correct, minimal, well-tested, and picks the right helper for demonstrable reasons. The blocker is the claim, not the diff: correct the "one place" statement on PR #61, and file the follow-up.

NEEDS ATTENTION

[nicodes]

Review 1 reply

All four findings addressed. Fixed in 385bb47.

Finding 1 (medium, blocking) — the central claim was overstated: CORRECTED, and the substance filed

You are right, and the sharpest part is the bit about which path mattered. This change's own argument for why it was low-urgency was "headless means no TTY, so escapes are inert" — and at least one path still open is the interactive one. That inverts the framing rather than merely qualifying it.

The claim is struck in three places, in place rather than deleted:

• the PR body, with a labelled correction at the top
• the branch's commit message, in 385bb47
• the test's own doc comment, which now names the boundary so the test cannot be read as closing more than it does

Filed the substance as nicodes/ormos-be#420 — on the private tracker, not here, because this is a public repository and the detail describes an unfixed weakness with enough specificity to act on. That is also why your finding 1 is the one part of the review I redacted when posting it; the redaction notice at the top says exactly what was removed and why. Your conclusion is published in full, because the false claim was public and had to be corrected where it was made.

Your observation that the issue's wording was accurate and mine was not is worth crediting directly: #320 says "the one remaining place the pairing code reaches an output", and I generalised "the pairing code" to "a relay string". I have noted on #320 that its own scope needs no correction, so the closure is not quietly widened.

The %q point is a good one and is in #420's cleared list: login.go:87 is strictly another relay string reaching stderr without sanitize, neutralised only because strconv quoting escapes both ESC and U+202E. That it is safe for a different reason than every other site is precisely why a blanket claim was the wrong shape.

Finding 2 (low) — the survived-payload guard only covered the code: FIXED

Correct, and it defeated the assertion's stated purpose. survives is now codeSurvives and urlSurvives, checked in one loop.

Shown to fail, using the exact hole you described — sanitise the code, drop the URL:

login_test.go:481: restarted=false: the URL did not survive as inert text
                   (want "https://app.example.test/pairEND")
login_test.go:481: restarted=true:  ... (same)

Before this it passed. Your point that TestHeadlessCodeDisplayShowsCodeAndURL already caught it at suite level is accurate, and it is still the reason this was low rather than medium — but a comment claiming an assertion it did not make is the same class of defect as finding 1, one scale down.

Finding 3 (informational) — no gate ties relay strings to sanitize: agreed, folded into #420

This is the durable fix and I have made it a Done when clause on #420 rather than leaving it as a note: a gate, shown to fail by adding an unsanitised print and watching CI reject it. Your framing is what makes it convincing — two sites were missed by inspection, so a third point patch buys less than a check that fails on the next one.

Not doing it in this PR: it wants a decision about mechanism (a vet-style analyser, or a grep over print sites with an allowlist for the %q case) and would arrive with no convention behind it. That is a change worth reviewing on its own terms.

Finding 4 (informational, pre-existing) — a wholly non-printable code degrades to a blank line: accepted, covered in #420

Confirmed the mechanism: deviceStart checks out.UserCode != "" at login.go:158 before anything sanitises, so a code of only Cf runes passes validation and reduces to empty. Headless then prints a blank code line and the TUI waits forever on "requesting a pairing code…".

Not introduced here, and both directions are safe failures, but you are right that the two paths now share it — which is the argument for fixing it once rather than twice. It is a Done when clause on #420.

On what you could not verify

Noted that you proved the helper choice with an equivalent-code harness outside the repo rather than by reverting in place, since reverting would have mutated the checkout. For the record, the in-repo mutation was run before the first push and is recorded in the original commit message: reverting the two sanitize calls fails the C0 case on "\x1b" and "\a" and both survived-payload assertions. Your harness and that run agree.

The homoglyph / userinfo-authority point in your residual risks is real and out of scope here, as you say — sanitize is about what a terminal interprets, not about what a human misreads. Worth a separate issue if the pairing screen's threat model is revisited; not filing one now rather than filing a placeholder I cannot scope.

[nicodes]

Review 2 reply

Approve verdict noted. All three low findings acted on rather than deferred; fixed in 3b946d0 and a PR-body edit.

Finding 1 (low) — the strikethrough was on the retraction, not the claim: FIXED

Embarrassingly literal reading of my own markup, and you are right that it inverted: a struck-through retraction reads as retracting the retraction. The callout now strikes the quoted claim itself:

Struck: "the one place a relay string reached an output without going through sanitize"

Finding 2 (low) — the doc comment published more than I redacted: FIXED

This one lands. My own rationale one commit earlier was that identifying the interactive path is what the redaction was for, and then I named both paths in a permanent public source comment — where, as you say, labelling one "headless" identifies the other by elimination.

Trimmed to "It is not the last unsanitised relay string in the agent. Tracked in nicodes/ormos-be#420", which is all the comment needed to stop the test being over-read. Your reasoning for why it was not a real disclosure (the line is in public source and greppable) is why I am treating this as tightening rather than an incident.

Finding 3 (low) — order-blind assertions: FIXED, and this was the best find in the review

Verified your overlay result before changing anything: transposing the two sanitize arguments passed go test ./internal/system/ green, whole package. And the hazard is not hypothetical — this PR is the change that touched that argument list, and the two %s verbs are adjacent in one format string.

The output is now cut on the "and enter code" line and each string checked in its own slot. On the transposition:

the URL did not survive as inert text in its own slot (want "https://app.example.test/pair]0;PWN")
the code did not survive as inert text in its own slot (want "AB]0;OWNED[2JCD")
FAIL github.com/nicodes/ormos/internal/system  16.238s

That is the same mutation you measured green. It also completes what the assertion was for: not merely that both strings survive, but that each is where the operator is told to look. A URL to type as a code and a code to open in a browser is a pairing that cannot complete, with nothing objecting.

Finding 4 (informational) — "two" is narrower than #420 records: accepted as stated

Correct: "two" counts live unsafe paths, not relay strings that bypass sanitize — #420 also lists the %q-neutralised login.go:87 and the undecided audit-log bidi question. Leaving the count as-is, because it is the count the sentence is making and it errs toward under-claiming in a public repo, which is the direction I want here. Raising it was right given the original defect was exactly a scope generalisation.

Finding 5 (informational) — #420 sized s: RE-SIZED to m

Agreed and done, on the label and with a note on the issue saying the estimate moved and why. Four deliverables, one of them a CI gate carrying the proof-of-failure obligation, plus an undecided mechanism choice and an audit-log design decision. Not an s.

Finding 6 (informational) — cross-repo closing keyword: will verify, not assume

Fair. Closes nicodes/ormos-be#320 from a public PR to a private issue is not something either of us can confirm without merging, and the workflow requires #320 actually reach Done. I will check its state after merge and close it by hand if the keyword does not fire, rather than treating the merge as having done it.

On your method

Worth noting you proved the mutations with go test -overlay against the real package where Review 1 used an out-of-tree harness, and that the stripCtl row therefore now confirms the helper choice in-package. That is a stronger result than the original commit message claimed, and it is the reason I trusted findings 3's measurement enough to reproduce it directly.

[nicodes]

Review 2

Produced by the reviewer agent role, scope pinned to the branch diff. Engine note: the opencode reviewer could not run — its model quota is exhausted for this billing cycle — so this was produced by a Claude reviewer carrying .opencode/agents/reviewer.md's role definition verbatim.

Redaction notice. Posted verbatim except for one bullet in the Review 1 disposition, where the reviewer independently confirmed the unfixed path from Review 1's finding 1 and named its file:line and call chain. That is the same material redacted from Review 1 and for the same reason: this is a public repository and the weakness is unfixed. It is on the private tracker as nicodes/ormos-be#420. The confirmation itself — that finding 1's substance was real and is filed correctly — is kept. Nothing else is altered.

---

All gates run and all mutation proofs done via go test -overlay — the repo was never modified (HEAD still 385bb47, git status --porcelain empty before and after).

Method, exactly as run

• Scope. gh pr diff 61 --repo nicodes/ormos and git -C .../ormos-320 diff origin/main...HEAD written to files and diffed — byte-identical. Branch fix/320-sanitize-headless-pairing, local HEAD == git ls-remote origin == 385bb47, two commits, working tree clean.
• Failure proofs. go test -overlay=<json> with a patched copy of internal/system/login.go in a scratchpad outside the repo. This runs the real package against the mutation, unlike Review 1's out-of-tree harness. git status --porcelain empty and HEAD unchanged after every run.
• Gates. gofmt -l . → no output. go vet ./... → exit 0. go build ./... → ok. go test -race -count=1 ./... → ormos ok 1.0s, internal/system ok 17.6s, relay ok 1.1s (uncached; the first run reported (cached), so I forced -count=1).
• CI. gh pr checks 61 on 385bb47: actionlint pass, ci pass, darwin pass, auto-merge skipping.

Review 1 findings: disposition verified

Finding 1 (medium, blocking) — CORRECTED, accurately, in all three claimed places.

• PR body: [!IMPORTANT] callout at the very top, quoting the false sentence, naming the correct scope ("the one place the pairing code did"), stating the generalisation overstated coverage, pointing at nicodes/ormos-be#420. Accurate.
• Commit 385bb47: a labelled CORRECTION block quoting the struck sentence verbatim and attributing the generalisation to the author. Accurate.
• Test doc comment: now says "the one place the PAIRING CODE reached an output unsanitised" and adds "It is not the last unsanitised relay string in the agent". I verified the narrowed claim independently: UserCode reaches an output at exactly two sites, login_tui.go:76 (sanitized) and login.go:279 (sanitized by this PR). The claim is now true as written.
• [REDACTED: independent confirmation of the unfixed path from finding 1, with its file:line and the call chain by which relay-controlled bytes reach a live terminal. Filed as nicodes/ormos-be#420.] The conclusion, which is not redacted: finding 1's substance was real and is filed correctly, and I confirmed the load-bearing mechanism myself rather than taking the issue's word for it.
• nicodes/ormos-be#420 exists, is bug/s, and its content is accurate.
• nicodes/ormos-be#320 carries the author's note that its own wording needed no correction. That satisfies "correct it on the issue as well when the issue carries it" in the right direction — it records that the issue did not carry the false claim.
• Redaction judgment: handled correctly. The notice states what was removed (file:line locations, call chains, a worked payload, the interactive-reachability reason, the cleared-sites list), why (public repo, unfixed weakness, actionable detail), where it went (#420), and keeps the conclusion in full because the false claim was public. That is the right ordering of the two rules.

Finding 2 (low) — FIXED, and the claimed failure reproduces exactly.

codeSurvives and urlSurvives are both asserted inside the for _, restarted := range []bool{false, true} loop, so both fields × both framings × both subtests = 8 assertions. Overlay results:

mutation of login.go:279	result
none (baseline)	PASS both subtests
sanitize(s.VerificationURL) → ""	FAIL, 4 errors — the URL did not survive as inert text (want "https://app.example.test/pairEND"), and restarted=true, on both subtests
sanitize(s.UserCode) → ""	FAIL, 4 errors, "the code did not survive"
both sanitize calls removed (pre-fix state)	FAIL, 16 errors — forbidden and survives on every case
both → stripCtl	FAIL on "C1 and format characters" only; "C0 escapes" passes

The reply's quoted failure message matches mine character for character, including the line number. The stripCtl row confirms the helper choice is load-bearing — now proved in-package, not by an equivalent-code harness.

Findings 3 and 4 — deferral is acceptable. Both are Done when clauses on #420, and the gate clause carries the project's own proof-of-failure requirement. Finding 3 needs a mechanism decision that would arrive here with no convention behind it; finding 4 is pre-existing and now shared by two paths, so fixing it once is the correct shape. Neither belongs in a point-fix PR.

New findings

1. The PR body's strikethrough is on the retraction, not on the claim (low)

The body reads ~~That claim is struck.~~. Rendered, the sentence asserting the retraction is the one with a line through it, while the false sentence lower in the body renders normally and un-struck. A reader who skims past the callout reads the false claim as current, and a reader who reads the callout sees a struck-through retraction, which literally reads as retracting the retraction.

The project rule is "strike it with a labelled correction". The label is there and the prose retraction is unmissable (it precedes the claim), so the substance is not in doubt — but the markup is on the wrong text. Fix before merge: move the ~~ ~~ onto the quoted claim. Ten seconds; not a blocker, because the retraction is explicit and positioned first.

2. The test doc comment publishes slightly more than the author redacted (low)

The comment says the "error path to stderr and the headless log echo both remain, and one of those reaches a live terminal." Because the second is explicitly labelled headless, the sentence identifies the first as the interactive-reachable one — which is part of what the redaction notice says was removed. Review 1's kept text published that one path is interactive but not which; this comment narrows it to a one-grep target in a permanent public source comment.

My judgment: acceptable, but at the ceiling, and inconsistent with the author's own rationale one commit earlier. The marginal uplift is near zero — the vulnerable line is in public source and reachable by grepping — and no payload or call chain is given. But the comment's purpose is only to stop the test being over-read, which "It is not the last unsanitised relay string in the agent — tracked in nicodes/ormos-be#420" achieves without naming paths. I'd trim the two path names. AGENTS.md's "never add sensitive information to comments in a public repository" is the rule that argues for trimming; the public source is the reason it isn't a real disclosure.

3. The whole assertion family is order-blind: swapping the two sanitize arguments passes everything (low)

Both the new test and TestHeadlessCodeDisplayShowsCodeAndURL assert only strings.Contains(out, want) against the full output, never which slot each string landed in. Proved by overlay: mutating login.go:279 to sanitize(s.UserCode), sanitize(s.VerificationURL) — printing the URL under "and enter code" and the code under "open" — passes go test -count=1 ./internal/system/ green, whole package.

Failure scenario: an edit to that argument list (this PR is the change that touched it) transposes the fields; the operator is shown a URL to type as a pairing code and a code to open in a browser, pairing cannot complete, and no test objects. This is a usability/trust break rather than a security one, and it is pre-existing for the sibling test — but the new test's stated purpose is precisely "the assertion cannot pass by printing nothing", and slot-correctness is the other half of that. Cheap fix: assert the code appears after the "and enter code" marker, or split the output on that marker.

4. "Two other relay-controlled strings" is a narrower count than #420 records (informational)

#420 also lists a relay-supplied name reaching stderr with no sanitize, neutralised only because %q quoting escapes ESC and U+202E, and an undecided audit-log bidi question. So "two" is the count of live unsafe paths, not of relay strings that bypass sanitize. This errs in the safe direction for a public repo and #420 is linked, so I raise it only because the PR's original defect was exactly a count/scope generalisation.

5. #420 is sized s for four Done when clauses (informational)

Those clauses include a new CI gate with a proof-of-failure requirement, an undecided mechanism choice, an audit-log design decision, and a degradation fix across two paths. Per the project's sizing rule that is worth re-sizing before dispatch. Orchestrator's call, not this PR's.

6. Cross-repo closing keyword, public PR → private issue (informational)

The body says Closes nicodes/ormos-be#320 and #320 is still OPEN. Cross-repository auto-close across a public/private boundary is not something I could verify without merging. Confirm #320 actually closes after merge rather than assuming it, since the workflow requires the issue reach Done.

Residual risks and testing gaps

• Nothing covers #420's paths; a relay putting an escape into an error body today fails no test. Unchanged from Review 1, correctly out of scope, now tracked.
• No gate ties relay-sourced strings to sanitize. Deferred to #420.
• Slot-correctness of the headless output is untested (finding 3), proved by overlay.
• sanitize deletes rather than escapes, and U+200B / U+00A0 are removed silently. Unchanged, benign for a non-hostile relay.
• Homoglyph and userinfo-authority spoofing of the printed URL is untouched; sanitize is about what a terminal interprets, not what a human misreads. Out of scope, as the reply says.

Assessment

Review 1's blocker was a claim, and the claim is now corrected in all three places, accurately, with the substance filed on the private tracker in a form I independently verified. Finding 2 is genuinely fixed and I reproduced its exact claimed failure in the real package, plus three further mutations, without touching the repo. Findings 3 and 4 are properly deferred with proof-of-failure baked into the acceptance criteria. Gates and CI are green on the head commit. No claim in the current PR body, commit messages, or code comments overstates what the change does — the narrowed claims are all true as written.

The three low findings are worth acting on — finding 1 before merge (a misplaced ~~), findings 2 and 3 either here or as a follow-up.

APPROVE

[nicodes]

Review 3 reply

All four findings addressed. Fixed in 41f0911 plus two PR-body edits.

Finding 1 (low) — the const landed inside the test's doc comment: FIXED

Caught cleanly, and the go/parser proof is not something I would have thought to run on my own comment. Reproduced it before fixing:

const/var codeInstruction                              doclines=16
func TestHeadlessCodeDisplaySanitisesTheRelaysStrings   doclines= 0

Your framing of why it matters more than a misplaced blank line is the part that stings and is correct: the text that ended up documenting a string constant is the "do not read this as closing more than it does" caveat that a blocking finding required be placed on this test. So the third location of the Review 1 correction silently detached from the thing it was correcting.

The const is now local to the test, which is its only user — your second suggestion, taken because it makes the misbinding unrepeatable rather than just currently fixed. Doc block binds to the test again (doclines=14).

And your observation that nothing in CI catches this is worth recording: there is no golangci-lint/revive/staticcheck in this repo, and gofmt is silent on it, so a detached doc comment is invisible to every gate. That is a real gap, not just this instance.

Finding 2 (low) — the claim was still un-struck where it was published: FIXED

You are right, and I misread my own remedy. Review 2's finding had two grounds and I fixed one: the callout now strikes a quotation, which is a labelled correction about the sentence, while the sentence itself carried on asserting the false thing in plain text. AGENTS.md says strike it, in place.

The body's own sentence is now struck inline, with the callout left as-is. Also noting for the record that you were right to flag the dispatch framing — "the strikethrough was moved onto the claim" was my description and it was inaccurate; it was moved onto a quotation of the claim.

Finding 3 (low) — the body's test description was two commits stale: FIXED

Correct, and it is the same defect shape as everything else in this PR's history, so it does not get a pass for under-claiming. The body now describes what the test actually does — per-slot, own-line, both payloads — and names the four mutations shown to fail.

Finding 4 (informational) — the sibling keeps the hole, and my reply implied otherwise: stated

Fair hit on the reply, not just the code. I headed that section "FIXED" and the commit message named both tests as having the hole before describing a fix to one. A reader concludes both were changed.

Stating it plainly: TestHeadlessCodeDisplayShowsCodeAndURL is unchanged and remains order-blind. Leaving it that way, because the package-level gate now fails on transposition and that sibling's job is the benign round-trip — but your residual risk is the real reason to write it down: if the sanitisation test is ever renamed or removed, the hole reopens with nothing saying so.

The residual gap — the two-way cut's granularity: CLOSED

This was the best thing in the review and I did not expect it to be exploitable. You proved that replacing the code line with (see below) and printing the payload inside the trailing "expires in" sentence passed both tests and the whole package — the operator told to enter (see below).

Both payloads are now required on their own two-space-indented line, which is how the format string prints them. On your exact mutation:

the code did not survive as inert text on its own line in its own slot (want "ABCDEF")
FAIL  github.com/nicodes/ormos/internal/system

The transposition case still fails with 8 errors, so the strengthening did not trade one coverage for another. That also makes the comment's claim ("each is where the operator is told to look for it") true rather than slightly stronger than the assertion, which was your point.

On the t.Fatalf framing limitation

Accepted as-is: the marker-drift Fatalf aborts the subtest so only restarted=false reports. Harmless because both framings share one format string, and a marker-drift failure is loud either way. Not worth converting to an Errorf and continuing, since the slots genuinely cannot be told apart at that point.

[nicodes]

Review 3

Posted late. This review was produced before the # Review 3 reply already on this PR, but only the reply was posted — the same omission Review 4 caught on #62. The published chain therefore ended on Review 2, followed by a reply to a review no reader could open. A verdict that is not published cannot gate a merge. Posting verbatim, unedited, in its original position in the chain.

Produced by the reviewer agent role. Engine note: the opencode reviewer could not run — its model quota is exhausted for this billing cycle — so this was produced by a Claude reviewer carrying .opencode/agents/reviewer.md's role definition verbatim.

---

Scope: nicodes/ormos#61, branch fix/320-sanitize-headless-pairing, three commits, head 3b946d0. gh pr diff 61 --repo nicodes/ormos and git -C .../ormos-320 diff origin/main...HEAD are byte-identical; local HEAD == git ls-remote origin == 3b946d0; working tree clean. All mutation proofs run with go test -overlay against patched copies in a scratchpad; git status --porcelain empty and HEAD unchanged before and after every run.

Findings

1. The new const was inserted inside the test's doc comment, so the rationale now documents a string constant and the test has no doc comment at all (low; introduced by 3b946d0)

internal/system/login_test.go:416-434. Lines 416-431 are one contiguous comment block; const codeInstruction is at 432 and func TestHeadlessCodeDisplaySanitisesTheRelaysStrings at 434. There is no blank line between line 429 and line 430, so Go binds the whole 16-line block to the const.

Proved with go/parser (out of tree, parser.ParseComments, printing each decl's Doc):

TestHeadlessCodeDisplayShowsCodeAndURL            doclines= 1 first="// Headless display prints the code and verification URL plainly."
const codeInstruction                             doclines=16 first="// The headless path was the one place the PAIRING CODE reached an output"
TestHeadlessCodeDisplaySanitisesTheRelaysStrings  doclines= 0 first="<none>"

Concrete failure scenario: a maintainer opening the security test finds no statement of what it covers, while a one-line string constant carries a rationale about TTYs, journalctl, and payload provenance. More pointedly, this is where Review 1's finding 1 was corrected in its third location — the caveat "It is not the last unsanitised relay string in the agent. Tracked in nicodes/ormos-be#420 — named here so this test is not read as closing more than it does" exists specifically so this test cannot be over-read. It no longer attaches to the test. Editor hovers and any doc extraction over the test package show it on codeInstruction. Rendered as one paragraph, it also reads as two unrelated statements fused.

Nothing in CI catches this: .github/workflows/ is actionlint, ci, release, dependabot — no golangci-lint, revive, or staticcheck, and gofmt -l . is silent on it (verified).

This is the same class as Review 2's finding 1 — correct content, wrong location — one commit after that finding was fixed, and it was introduced after the approve. Fix: blank line at 430 and move the const above the test's doc block, or make it a local const inside the test function, which is the only place it is used.

2. The false claim in the PR body is still un-struck where it was published (low; carried from Review 2 finding 1)

Review 2's finding 1 gave two grounds: the markup was on the retraction, and "the false sentence lower in the body renders normally and un-struck. A reader who skims past the callout reads the false claim as current." The callout now strikes a quotation of the claim and adds a pointer to it, so the reviewer's literal remedy is satisfied. The second ground is not: the body's fourth paragraph still reads, in plain text,

headlessCodeDisplay printed the relay-supplied VerificationURL and UserCode with a plain Fprintf — the one place a relay string reached an output without going through sanitize.

AGENTS.md says "correct a published false claim in place, where it was published: strike it with a labelled correction rather than deleting it." A struck quotation in a callout plus a pointer is a labelled correction about the sentence; the sentence itself is still asserted. Fix: wrap the claim in the body sentence too. The callout can stay as-is.

Note for the record: the framing this review was dispatched with — "the PR body's strikethrough was moved onto the claim" — is not what the body shows. It was moved onto a quotation of the claim.

3. The PR body's description of the test is now stale (low)

The body's last paragraph still says the test "asserts the code still survives as inert text, so it cannot pass by printing nothing." Two commits have since changed exactly that: 385bb47 added urlSurvives, and 3b946d0 moved both checks into per-slot assertions. The body describes neither. This under-claims, which is the safe direction in a public repo, but the project's standard is that a description not matching what was applied is a defect in its own right — and every finding in this PR's history has been of that shape. One sentence fixes it.

4. The sibling test keeps the order-blind hole, and nothing says so (informational)

TestHeadlessCodeDisplayShowsCodeAndURL (login_test.go:393-414) still asserts strings.Contains over the whole output. Verified by overlay, running that test alone:

mutation of login.go:279	sibling alone	new test	package
transpose both args	PASS	FAIL	FAIL
transpose only when restarted	PASS	FAIL	FAIL

Leaving it is defensible: the package-level gate now fails on transposition, which is what matters, and the sibling's job is the benign round-trip. But 3b946d0's message names both tests as having the hole, then describes the fix without noting that only one test was changed, and the Review 2 reply heads the section "Finding 3 (low) — order-blind assertions: FIXED". A reader concludes both were fixed. Recommend one clause in the reply stating the sibling was left as-is and why. The residual risk is that if the sanitisation test is ever renamed or removed, the sibling silently reopens the hole.

Verified positives

Method stated exactly as run. No repository mutation at any point.

The slot-cut logic is correct.

• Marker uniqueness: grep over the module finds "and enter code" at exactly one print site, login.go:274. The only near-miss is login_tui.go:111, a different string in a different function never in this test's output. The restarted=true prefix line contains no marker, confirmed in every captured failure output.
• Halves map to the right slots. open = optional restart line + "To register this machine, open\n\n <url>\n\n"; code = "\n\n <code>\n\nThe code expires in …". Verified against real captured output in all five mutation runs.
• The restarted=true framing prepends a line to the open half only, so it cannot move either payload across the boundary.

The !found t.Fatalf is reachable, not dead. Overlay reworded the printed line to and type in the code:

login_test.go:487: restarted=false: the output no longer contains "and enter code",
                   so the slots cannot be told apart:

So the answer to the drift question is: loudly, not silently.

The claimed transposition failure reproduces exactly. 8 errors — both payloads × both framings × both subtests — matching the Review 2 reply's quoted messages character for character. The package FAILs where Review 2 measured this same mutation green.

Two further mutations I devised, both caught. Transpose only in one framing: FAILs, 4 errors, restarted=true only. Code printed in both slots: FAILs on the URL slot in both framings and both subtests.

The const duplication is safe against drift. I could not construct a wording change that keeps the marker findable and leaves both Contains checks passing. Marker moved before both payloads → the URL check fails. Marker duplicated → Cut takes the first occurrence and the URL falls into the code half → fails. Marker reworded → Fatalf. Every path is a loud failure.

Finding 2's fix is complete and correct. The doc comment no longer names either remaining path; it states the fact and the #420 reference only. The narrowed claim ("the one place the PAIRING CODE reached an output unsanitised") remains true — UserCode reaches an output at exactly login_tui.go:76 and login.go:279, both now sanitised.

No code change in 3b946d0. git show --stat touches internal/system/login_test.go only; login.go is unchanged since the first commit on this branch. Nothing in the production path could have regressed.

Gates, all uncached. gofmt -l . no output; go vet ./... exit 0; go test -race -count=1 ./... → ormos ok 1.009s, internal/system ok 17.554s, relay ok 1.026s. HEAD 3b946d0, git status --porcelain empty, after everything.

CI is green on this exact commit. actionlint pass, ci pass, darwin pass. Confirmed the run's head_sha is 3b946d0a7cfa…, i.e. the checks ran on the post-approval commit, not on 385bb47.

Commit message accuracy. Every measurable claim in 3b946d0 verified true. The only gap is the omission in finding 4 above.

Residual risks and testing gaps

• Granularity limit of a two-way cut. The code slot is "everything after the marker", which includes the trailing "The code expires in … " sentence. I proved this is exploitable by an edit: replacing the code line with (see below) and printing the code inside the trailer sentence instead passes both tests and the whole package. The operator would be told to enter (see below). A three-way cut, or asserting the payload is on its own indented line, would close it. Not a security gap and not a regression — it is the natural boundary of the chosen technique — but the comment's stated purpose ("each is where the operator is told to look for it") is a slightly stronger claim than the assertion delivers.
• t.Fatalf inside the for restarted loop aborts the subtest, so a marker-drift failure reports only the restarted=false framing. Harmless here, but it means the Fatalf cannot ever report a framing-specific marker loss.
• Unchanged from Review 2: nothing covers #420's paths; no gate ties relay-sourced strings to sanitize; sanitize deletes rather than escapes and silently removes U+200B/U+00A0; homoglyph and userinfo-authority URL spoofing is untouched.
• #320's cross-repo closing keyword is still unverifiable without merging, as the reply acknowledges.

Assessment

The substantive half of 3b946d0 is right and its claims are exact. The slot cut is correctly placed, the marker is genuinely unique, the halves genuinely correspond to the two slots, the !found guard is reachable and fails loudly, and I independently caught the claimed transposition plus two mutations the author did not try. The production code is untouched and cannot have regressed. Gates and CI are green on the head commit.

What holds this back is that the same commit re-introduced the exact defect class the review it was answering had just closed: correct prose, wrong location. The test's rationale — including the "do not read this as closing more than it does" caveat that a previously blocking finding required be placed there — now documents a string constant, and the test documents nothing. Combined with the un-struck claim still standing in the body and the body's now-stale description of the test, three documentation defects remain in a PR whose entire history is about documentation accuracy. None is a code defect, none is more than a few minutes of work, and I would approve on sight of them fixed.

NEEDS ATTENTION

[nicodes]

Review 4

Produced by the reviewer agent role, scope pinned to the branch diff. Engine note: the opencode reviewer could not run — its model quota is exhausted for this billing cycle — so this was produced by a Claude reviewer carrying .opencode/agents/reviewer.md's role definition verbatim. Posted verbatim.

Repo untouched throughout (HEAD still 41f0911, git status --porcelain empty before and after every run). All mutation proofs via go test -overlay with patched copies in a scratchpad outside the repo.

---

Method, exactly as run

• Scope. gh pr diff 61 --repo nicodes/ormos and git -C .../ormos-320 diff origin/main...HEAD. Local HEAD = git ls-remote origin = gh pr view --json headRefOid = 41f09117. Four commits, working tree clean.
• Doc-binding proof. A go/parser + go/ast program walking f.Decls, reporting len(FuncDecl.Doc.List) and len(GenDecl.Doc.List), run against both 41f0911's login_test.go and 3b946d0's.
• Mutation proofs. go test -overlay against the real package with patched copies of internal/system/login.go (five mutations) and login_test.go (two probes) written outside the repo.
• Gates. gofmt -l . → no output. go vet ./... → exit 0. go build ./... → ok. go test -race -count=1 ./... → ormos ok 1.008s, internal/system ok 17.556s, relay ok 1.039s.
• CI, pinned to the head commit. Every check-run's head_sha confirmed 41f09117: actionlint success, ci success, darwin success, auto-merge skipped. The legacy combined-status endpoint reports pending, which is total_count: 0 with an empty statuses array — no commit statuses exist on this repo, same as Review 2 observed.
• Markdown render proof. gh api /markdown with mode: gfm, context: nicodes/ormos, on the body's corrected sentence.

Review 3 findings: disposition verified

Finding 1 (low) — the const inside the doc comment: FIXED, and the parser numbers are exactly as claimed.

commit	declaration	doclines
3b946d0	const codeInstruction (top level)	16
3b946d0	func TestHeadlessCodeDisplaySanitisesTheRelaysStrings	0
41f0911	top-level codeInstruction	absent
41f0911	func TestHeadlessCodeDisplaySanitisesTheRelaysStrings	14

At 41f0911 the doc group starts at line 416 and ends with // one standard rather than each to its own. — the full caveat block, bound to the test. The author's reported numbers are correct to the number.

Finding 2 (low) — the claim un-struck in the body's own sentence: FIXED, and it renders.

Because the ~~ pair spans a soft line break I did not assume it renders; GFM's strikethrough extension is not obliged to cross newlines. It does — gh api /markdown returns <del>the one place a relay string reached an output without<br>\ngoing through sanitize</del>. The false claim is struck where it was published, the callout retains the labelled quotation, and the retraction still precedes the claim.

Finding 3 (low) — the stale test description: FIXED, and all four named mutations reproduce as failures. I did not take the description's word for it. Against 41f0911 unmodified:

mutation of internal/system/login.go	new test	error count	sibling
none (baseline)	PASS	—	PASS
print nothing (w → io.Discard)	FAIL	2 (Fatalf, restarted=false only)	—
drop the URL	FAIL	4	—
transpose the two sanitize args	FAIL	8	PASS
trailer burial (code line → (see below))	FAIL	4	PASS
sanitize → stripCtl (both)	FAIL on C1 and format characters only	8	—

The trailer-burial message is character-for-character what the commit message quotes. The transposition is 8 errors, "as before". Both claims hold. The payloads are byte-identical to the pairing-screen tests'.

Finding 4 (informational) — the sibling's order-blind hole: STATED, plainly. Confirmed empirically — the sibling passes under transposition, under trailer burial, and under indent drift. The residual risk the author records is the right one to have written down.

Residual gap — trailer burial: CLOSED, verified above. The strengthening did not trade coverage away.

New findings on 41f0911

1. The two-space indent is duplicated from login.go's format string with nothing marking the coupling, and the failure message misdiagnoses indent drift (low)

login_test.go:500 hardcodes "\n "+slot.want+"\n". Those two spaces are login.go:270-278's format string, in another file, with no const, no reference, and no comment saying so.

Proved: changing login.go's indent from two spaces to four — a cosmetic edit that breaks nothing and that no other test notices — fails the new test with 8 errors, every one reading did not survive as inert text on its own line in its own slot. The payload survived perfectly intact; only the indentation moved. The message sends a maintainer looking at sanitize for a formatting change.

Drift is loud, which is the important half and why this is low rather than medium. But note the asymmetry inside this very commit: the other literal coupled to that format string, codeInstruction, got a named const and a comment saying exactly where it comes from and why. The indent got neither.

I verified a fix rather than proposing one blind. Replacing the assertion with an indent-agnostic form — strings.TrimSpace(line) == slot.want over strings.Split(slot.in, "\n") — passes baseline, passes the four-space indent (no false red), and still fails transposition, trailer burial, drop-URL, and print-nothing. It keeps every real catch and drops the only coupling that isn't the property under test.

On the other half of the question asked: a payload containing a newline cannot reach this assertion. sanitize maps every !unicode.IsPrint(r) rune to -1, and '\n' is not printable, so no relay input can produce a multi-line payload. That concern is moot, correctly.

2. The own-line assertion is vacuous when the expected survivor is empty (low, latent)

strings.Contains(slot.in, "\n "+slot.want+"\n") with slot.want == "" reduces to Contains(slot.in, "\n \n") — exactly what the format string prints when a payload is empty. So the assertion cannot distinguish "the payload survived" from "the payload was eaten entirely".

Proved with a test-file overlay adding one case whose payloads are nothing but control and format characters: it passes green, whole package, in a test whose stated purpose is that it "cannot pass by printing nothing".

Nothing ships broken — both current cases have non-empty survivors, and I confirmed both are genuinely load-bearing. This is a trap for the next author, and specifically for the degradation Reviews 1-3 kept flagging as the residual risk (sanitize deletes rather than escapes, so a hostile relay can make a field vanish). A case written to document that would be a dead assertion, silently green, against AGENTS.md's "confirm a proposed assertion is reachable before shipping it". A one-line if slot.want == "" { t.Fatalf(...) } guard closes it.

3. Overstatement audit: nothing found (informational)

I checked the current PR body, all four commit messages, both changed files' comments, and the Review 3 reply against what the code does.

• stripCtl drops only r < 0x20 || r == 0x7f, so "leaves the C1 controls and the Cf format characters" is exact.
• // Unlike the pairing screen this path writes no OSC 8 hyperlink of its own — the only OSC 8 emitter is hyperlink(), reached only from login_tui.go:107. Accurate, and it is what licenses the stricter bare-\x1b forbid.
• // Local, because this is the only place it is used — codeInstruction appears at three sites in the test and nowhere else. Correct.
• "the one place the PAIRING CODE reached an output unsanitised" — still true at this head.
• "No production code touched; login.go is unchanged since the first commit" — git diff 2620ce4..HEAD -- internal/system/login.go is empty. True.

Two things lean the safe direction and need no action: the body's older paragraph is now redundant with the new one (an under-claim), and 3b946d0's message still carries the framing Review 3's finding 4 objected to. Rewriting it would mean force-pushing, which AGENTS.md forbids; correcting it in the reply, which the author did explicitly, is the only place left. Correct disposition.

4. The Fatalf framing limitation persists, as accepted (informational)

Marker drift aborts the subtest, so only restarted=false reports — measured: 2 errors, not 4. The author accepted this on the ground that both framings share one format string. Confirmed: they do, one Fprintf.

Residual risks and testing gaps

• The sibling stays order-blind and trailer-blind. Everything protecting the headless output's layout now lives in one test, and nothing marks the sibling as depending on it.
• Indent drift produces a misdiagnosed failure, not a silent one. Finding 1.
• A payload emptied by sanitize is invisible to the own-line assertion. Finding 2.
• No lint gate catches a detached doc comment. No golangci-lint, revive, or staticcheck in this repo; gofmt is silent on it. The Review 3 defect was invisible to every gate.
• nicodes/ormos-be#420's paths remain uncovered, correctly deferred, unchanged across four reviews.
• Closes nicodes/ormos-be#320 across the public/private boundary still cannot be verified without merging.

Assessment

Every Review 3 finding is resolved, and I verified each one by the mechanism the finding was about rather than by reading the claim: go/parser for the doc binding, the GFM render API for the strikethrough, and five independent login.go mutations for the description. All numbers the author published — 16, 0, 14, 8 errors, 4 errors, the exact failure string — reproduce. No claim anywhere in the body, the commits, the comments, or the replies overstates what the code does, and the one framing error left in the record is in an immutable commit message and corrected in the only place it can be.

The two new findings are both low and both about the test's own robustness rather than about what it protects. Neither is a false green today, and I proved a replacement that fixes the first while keeping all four mutation catches. Scaling to blast radius — one test in one package, production code unchanged and now reviewed four times, gates and CI green on the head commit specifically — these do not warrant another round before merge. Worth acting on here or as a follow-up; the empty-want guard is the one I would not leave for later, since it undercuts the assertion's stated purpose.

APPROVE

[nicodes]

Review 4 reply

Approve verdict noted. Both new findings acted on here in ba40cdf rather than deferred, since you flagged the second as the one not to leave for later.

Finding 1 (low) — the hardcoded two-space indent: FIXED, using the form you verified

Took your TrimSpace per-line form rather than adding a named const, because it removes the coupling instead of documenting it. Your point about the asymmetry inside the same commit is the one that decided it: codeInstruction got a const and a comment saying where it came from, and the indent — coupled to the same format string in the same way — got neither. One of those was going to drift.

Re-verified all five outcomes you measured, so the fix is not taken on trust either:

four-space indent      PASS   (was 8 false failures)
transpose the args     FAIL   8 errors
drop the URL           FAIL   4 errors
trailer burial         FAIL   4 errors
sanitize -> stripCtl   FAIL   4 forbidden-sequence failures

Every real catch kept, the only false red gone.

Finding 2 (low, latent) — vacuous on an empty want: FIXED

This is the one worth the round. Contains(slot.in, "\n "+want+"\n") with an empty want reduces to looking for a blank indented line, which is exactly what the format string prints when sanitize eats a payload whole — so the assertion could not tell survival from deletion, in a test whose stated purpose is that it cannot pass by printing nothing.

And your framing of who it traps is precise: the degradation Reviews 1-3 kept naming as the residual risk is sanitize deleting rather than escaping, so a case documenting that is the likely next addition here, and it would have been dead code passing green. Against the project's own "confirm a proposed assertion is reachable before shipping it".

An empty want is now a t.Fatalf. Proven with your probe case — payloads of only U+202E and U+009B:

code case has an empty want: this assertion cannot tell survival from deletion,
so it would pass vacuously

where before the change that case passed green. Probe removed; the real degradation stays on nicodes/ormos-be#420, which already carries it as a Done when.

Finding 3 (informational) — overstatement audit clean: noted

Recording your two "safe direction" observations rather than acting on them. The body's older paragraph is now redundant with the new one, and 3b946d0's message still carries the framing Review 3 objected to. You reached the same conclusion I did on the second: a force push would fix the message and AGENTS.md forbids it on a shared branch, so the reply is the only surface left. Leaving both.

Finding 4 (informational) — the Fatalf framing limitation: accepted, unchanged

Your measurement (2 errors, not 4, on print-nothing) confirms the reasoning holds: both framings share one Fprintf, so a marker-drift failure cannot be framing-specific.

On merging with code after the approve

Being explicit, since this is the second time on this PR: ba40cdf landed after your approving verdict. I am treating your approve as covering it, on two grounds you stated — that these findings "do not warrant another round before merge", and that you verified the finding-1 fix yourself and reported exactly which mutations it keeps and drops. I re-ran all five and got your numbers. If you would rather see a Review 5 on ba40cdf, say so and I will hold.

On the lint gap

Worth carrying forward: you and Review 3 both noted no golangci-lint/revive/staticcheck in this repo, and gofmt is silent on a detached doc comment. Review 3's finding 1 was invisible to every gate this repo has — it was caught by a reviewer running go/parser by hand. That is a real gap and I have not filed it, because I would rather scope it as "which linter, and what does it gate" than file a placeholder. Flagging it here so it is in the record.