Escape captured control sequences where a query result is rendered for a person (#752)

[philcunliffe]

Fixes#752.

hyp query sql wrote captured bytes straight to process.stdout, which is not
wrapped the way dispatch wraps stderr. Every string column of every dataset is
verbatim captured text, so an ESC in a prompt, a log body, an HTTP header
value or a filename reached the operator's terminal and was interpreted.

Design record: LLP 0224 (Decision, Accepted).

The option taken, and why the others were rejected

Taken: sanitize in formatCell for the human-facing formats, escaping rather
than stripping.table and markdown escape; json and jsonl stay
byte-exact, so the data stays extractable and pipelines keep exact bytes.
Escaping rather than stripping keeps the row honest about what was captured: a
rendered cell is the payload the operator asked to see, so silently dropping
bytes turns a query into a lie about the capture.

Rejected: gate on process.stdout.isTTY. It makes one command print
different bytes depending on whether it is piped, so what an operator saw and
what they saved to a file disagree, and a bug reproduces differently under
redirection. The format flag is already in the operator's hand and already
decides every other rendering question; a pipeline that wants exact bytes asks
for --format jsonl and gets them on a TTY too. No assertion in this PR touches
isTTY, and none needs to.

Rejected: do nothing and document the output as untrusted (the cat on a
binary file argument).cat has no idea what its bytes are. This renderer
knows every cell it is laying out and has already clipped each one for the
context budget; a renderer that lays output out in aligned columns has already
promised the columns mean something.

sanitizeLabel: neither reused nor duplicated, decomposed

sanitizeLabel is the wrong function for a cell (it truncates, and returns
undefined for empty), but its character class is exactly the right vocabulary.
Copying that class would have created the two-divergent-notions problem the
issue warns about.

So the class in src/core/util/json_util.js is now composed from three named
groups, and both policies are built from those groups in that one file:

group	sanitizeLabel	escapeForDisplay
TERMINAL_CONTROL_CHARS	strips	escapes
BIDI_FORMATTING_CHARS	strips	escapes
INVISIBLE_FORMATTING_CHARS	strips	leaves alone

sanitizeLabel's behaviour is unchanged: the recomposed class is the same set
of code points as the literal it replaces, and sanitizeLabel strips exactly the code points it stripped before LLP 0224 asserts that over all of the BMP
against a held copy of the original regex.

The exact character class escaped, and the reasoning

Escaped, with the spelling used:

characters	spelled	why
\^@-\^_ (C0), \u007F (DEL), \^@-\^_ (C1)	\n\r\t, else \uXXXX	moves the cursor, erases lines, opens escape sequences. C1 matters on its own: JSON.stringify escapes C0 but leaves \^[ (8-bit CSI) raw
\u2028, \u2029	\uXXXX	Unicode line/paragraph separators: a second line where the table planned one
\u061C, \u200E-\u200F, \u202A-\u202E, \u2066-\u2069	\uXXXX	bidi marks, embeddings, overrides and isolates reorder what follows, and an unterminated one keeps reordering past the end of the cell

Deliberately left alone:

• Zero-width and default-ignorable formatting (ZWSP, ZWNJ, ZWJ, word joiner,
  BOM, soft hyphen, variation selectors). Stripped from a label because a
  label is a map key and two keys that render identically dilute the entrypoint
  tracker's 32-entry eviction cap. A query cell is not a key, so nothing
  downstream is diluted, while ZWJ and the variation selectors are load-bearing
  inside ordinary emoji: a family emoji is a ZWJ sequence and U+FE0F is what
  colours a heart. Escaping them would visibly corrupt legitimate captured prose
  on a large fraction of real rows to defend against a character that cannot
  repaint anything.
• A backslash already in the value is not doubled. Captured data is full of
  Windows paths, regexes and JSON blobs. Disambiguating a literal two-character
  \n from an escaped newline would mangle all of them; the ambiguity is
  cosmetic and neither spelling can move a cursor.
• Confusables. Same reason already recorded against the label class: this
  bounds what a value does, not what it looks like.

Behaviour change worth naming: a newline inside a table cell now prints as
\n instead of breaking the row. That is visible for multi-line prose, and it
is the point: a newline in a cell is how a captured value forges a row.

What changed

• src/core/util/json_util.js: the three named groups, plus
  escapeForDisplay beside sanitizeLabel (exported through
  src/core/util/index.js as hypaware/core/util).
• src/core/query/format.js: formatCell escapes its finished text (not only
  the string branch, because the object branch goes through JSON.stringify,
  which passes C1 and bidi through). Table headers are escaped as defence in
  depth. Widths are measured on the escaped text, which is what is padded and
  printed. mdEscape loses its now-dead newline replacement. The --output
  spill receipt escapes its preview line by line (the newlines between preview
  rows are structure the receipt produced, not captured bytes) while the file it
  wrote keeps every byte.
• src/core/query/overview.js: folded in, because it is the same query plane,
  the same captured columns, and one shared cell() helper. provider, date
  and tool_name go through cell(); model and repo_root are the two sites
  that did not. The escape sits on each captured value rather than on the
  assembled row because this block paints its own bars and headings - a
  sweep over the finished table would strip the colour along with the attack.
  The test asserts exactly that: with color: true, every raw ESC left in the
  output matches ^\^[\[[0-9;]*m.

hyp vector search is fixed for free: it renders through the same
renderResult.

Surface survey (the issue asked for this and did not do it)

Surveyed for captured strings reaching a terminal without sanitizeLabel or
JSON.stringify. Nothing outside src/core/query/ is fixed here; each of these
wants its own issue, because they are label-plane surfaces where
strip-versus-escape is a separate argument.

Note JSON.stringify is not equivalent to either policy for a terminal: it
escapes C0 but passes DEL/C1 (including 8-bit CSI \^[), bidi overrides and
zero-width characters through untouched. Several --json paths below rely on it.

Worst uncovered surface: hyp graph neighbors
(hypaware-core/plugins-workspace/context-graph/src/verb.js:169,197,202,206,209).
display() (:228) and disambiguator() (:242) apply a 48-character clamp
and no stripping. Labels come straight off ai_gateway_messages via
ai-gateway-graph/src/graph_contract.js: client_name (:90), model
(:102), tool_name (:115), a file_path basename taken from inside a
tool_use block (:133, :515, fully model-controlled), git_remote (:146),
a Bash-parsed program (:180), skill names (:206+).

hyp status / hyp daemon status, all read back out of
$HYP_HOME/run/status.json, a file this build may not have written. Raw:
gateway upstream names (src/core/daemon/status.js:323,341,347,859, no
sanitize and no count cap), listen_fallback_from (:823,827,828),
daemon.state / daemon.mode / daemon.error
(src/core/commands/status.js:591,593,594 - error is V8's JSON.parse
message, which quotes a verbatim excerpt of status.json), sources[] /
sinks[] (:330,339), the client probe error (:364, quotes
~/.claude/settings.json or ~/.codex/config.toml), remote-config etags from
the ETag HTTP response header (:464,466,469,472 and
daemon/status.js:1041), reconciler request keys (:503,506), layered-config
drops (:452). hyp daemon status repeats this at
src/core/commands/daemon.js:87-106, including a source's own error string.
Note: commits a535050 / 2b799c1 / 33c3934 (issues #680 / #681) address
exactly the status.json subset above but are not merged into master, so
none of it is present on c483c1a.

Other captured values reaching a stream raw:hyp purge prints one cached
cwd per line (src/core/commands/purge.js:166,175); hyp policy show prints
policy-store directories and source names
(src/core/commands/policy.js:483,744,748); hyp session prints a session id
read out of a Codex rollout header and the rollout filename
(ai-gateway/src/session_command.js:332,366,413-419,503); hyp backfill plan
prints discovered transcript filenames and scan errors
(src/core/commands/backfill.js:1108,1158); the walkthrough echoes attach and
backfill adapter errors that quote client config files
(src/core/cli/walkthrough.js:1621,1961); the Claude/Codex/OpenClaw attach
adapters echo previous config values read from files HypAware does not own
(claude/src/index.js:506,509, codex/src/index.js:452,461,466,
openclaw/src/attach.js:533-543); hyp report list and the remote commands
echo HTTP response body fields (src/core/cli/report_commands.js:268,526-535,
src/core/cli/remote_commands.js:692,825,834); hyp plugin echoes fetched
manifest/registry fields (src/core/commands/plugin.js:264,287-297,335). The
dev logger mirror (src/core/observability/logger.js:101) puts captured
attributes through JSON.stringify only, and the gateway logs the client's raw
HTTP request path through it (ai-gateway/src/source.js:274-283).

Currently sanitized anywhere in the product: exactly one field family,
recent_entrypoints[].entrypoint / .client_name, cleaned at record
(ai-gateway/src/entrypoint_activity.js:64,66) and again at read
(src/core/daemon/status.js:412,418).

Discrimination evidence

Every test was watched failing before being kept. Reverts are
git checkout c483c1a -- <file>; mutants are hand edits to the fix.

change reverted / mutated	result
src/core/query/format.js reverted to c483c1a	query-format-escaping: 6 of 8 fail (1 table, 2 markdown, 3 newline, 4 tab/CR, 7 alignment, 8 receipt). 5 (json/jsonl byte-exact) and 6 (non-ASCII survives) pass, as they must: they guard preserved behaviour
src/core/query/overview.js reverted to c483c1a	query-overview: 1 of 63 fails (renderOverview escapes captured columns and keeps its own colour)
src/core/util/json_util.js + index.js reverted to c483c1a	util-json-util fails to load at all (no escapeForDisplay export): 1 file-level failure. Finer mutants below
mutant: escapeForDisplay returns its input unchanged	9 fail across all three files (6 format, 1 overview, 2 json_util)
mutant: escapeForDisplay strips instead of escaping	9 fail, same set: every assertion is on the escaped spelling, so a strip does not satisfy any of them
mutant: add INVISIBLE_FORMATTING_CHARS to the display class	2 fail: ordinary non-ASCII text is not touched by either human format and escapeForDisplay escapes bidi formatting and leaves zero-width formatting alone. This is the guard on the emoji decision
mutant: drop \u2028-\u2029 from TERMINAL_CONTROL_CHARS	2 fail: sanitizeLabel strips exactly the code points it stripped before LLP 0224 (the BMP equivalence oracle) and escapeForDisplay replaces control characters with visible escapes. This is the guard on the class refactor
mutant: escape json and jsonl too	2 fail: json and jsonl stay byte-exact and the receipt test. This is the guard on the byte-exactness half of the decision

Checks

npm test 4038 tests, 4037 pass, 0 fail, 1 skipped
npm run typecheck clean
npm run smoke -- status_diagnostics ok
npm run smoke -- local_parquet_export ok
npm run smoke -- gateway_codex_capture ok (extra, touches the query path)
npm run smoke -- local_only_export_withhold ok (extra, touches the query path)

walkthrough_picker_to_first_query is not run: it fails on master for an
unrelated reason tracked in #750.

Not verified

• No test renders into a real terminal emulator. The claim that \^[ is
  inert and a raw ESC sequence is not rests on the escape output being pure
  printable ASCII, not on an observed terminal.
• Column alignment for non-ASCII was already approximate and still is. Widths
  are measured in UTF-16 code units, so a CJK or emoji cell was misaligned
  before this change and is misaligned by the same amount after. This PR only
  guarantees that an escaped cell aligns correctly, which it does because the
  escape output is one column per character.
• The surface survey above is a read of the tree, not a runtime audit: each
  listed line was traced to an origin by reading, not by observing a value
  arrive.

🤖 Generated with Claude Code

[philcunliffe]

LLP 0224 was a collision; renumbered to 0225

This PR minted llp/0224-captured-text-is-escaped-for-display.decision.md. PR #759
(closing #742) minted llp/0224-maintenance-skips-are-a-standing-surface.decision.md
about an hour earlier. Both branches were open, so both authors checked "is 0224
free" against master and every remote branch, and both got a truthful yes at the
moment they looked.

Neither PR's CI catches this: the duplicate-numbers check runs against one branch
at a time, so each is internally consistent. The collision only exists in the union,
and it would have surfaced as a conflict at the second merge, or worse, as two
different documents numbered 0224 in the corpus if the merges touched no common
file. They do not: the two PRs share zero files.

Resolution: this PR renumbers to LLP 0225, by push order (#759 pushed
1129a12 at 23:52, this branch pushed a156378 at 00:0x). LLP 0156 permits
renumbering that does not change meaning, and this document has never been on
master.

Pushed as 588bf91. The change is mechanical: git mv of the doc plus
LLP 0224 to LLP 0225 across the six files that reference it (three source, three
test, including the UNSAFE_LABEL_CHARS_BEFORE_LLP_0224 fixture name). Verified
after: node --test test/core/llp-ref-hygiene.test.js 11/11, so every @ref
resolves to an anchor that exists in the renamed doc, and npm test 4037 pass / 0
fail / 1 pre-existing skip, unchanged from the pre-renumber run.

Nothing about the design changed, and the review that follows will read the head
with 0225.

[philcunliffe]

neutral review - round 1

Head reviewed: 588bf91a526616ad3346d5970eec5b0c0e6814c4 (post-renumber to LLP 0225).
All 9 checks SUCCESS. Reviewed in a detached worktree; nothing was written to the
branch.

Four findings, all minor or nit. No security gap, no mangled legitimate string,
no sanitizeLabel regression, no alignment or byte-exactness break.

The two things I most wanted checked both came back clean, and were checked the
hard way:

• The sanitizeLabel decomposition. The reviewer did not trust the PR test. It
  extracted the pre-PR regex from HEAD~2, composed the new class itself, and
  compared character by character over the entire BMP and the entire astral
  range: 0 disagreements in 1,114,112 code points. The display class is a strict
  subset of the label class (79 code points versus 106).
• The escape class, attacked from both sides. 36 attack vectors (7-bit ESC, 8-bit
  C1 introducers, OSC/DCS/APC, bidi, control sequences buried inside object values,
  and attacker-controlled column names including the zero-row early-return branch):
  0 leaks, 0 forged rows. Then 21 legitimate strings (ZWJ family emoji, VS16,
  regional-indicator and tag-sequence flags, NFD combining accents, RTL letters as
  opposed to overrides, Windows paths, JSON blobs): 0 mangled.

Its completeness argument is the part worth keeping: every terminal escape must
open with ESC or a C1 introducer, both of which are in the class, so no character
outside it can start a sequence, and the deliberate exclusions cannot move a
cursor.

---

VERDICT: findings

All four findings are minor or nit. No security gap, no mangled legitimate string, no sanitizeLabel regression, no alignment or byte-exactness break, no vacuous test, no convention violation. The fix itself is sound and I could not get anything through it.

---

1. minor — llp/0225-captured-text-is-escaped-for-display.decision.md:163-166 — the Verification section claims a discrimination result that two of its five tests do not have.

The text reads: "Unit tests, each shown failing against the pre-fix source before being kept: ESC in a table cell, ESC and bidi in a markdown cell, json/jsonl byte-exactness, non-ASCII and emoji survival, and column alignment with an escaped cell."

I checked out HEAD~2's four source files and ran the three new test files against them. Six of the eight format tests fail, plus the overview test, plus util-json-util.test.js fails to load (the escapeForDisplay named export does not exist yet). But ok 5 - json and jsonl stay byte-exact, control characters included and ok 6 - ordinary non-ASCII text is not touched by either human formatpass against the pre-fix source. Those are exactly two of the five items the sentence enumerates.

Why it matters: the Verification section of an Accepted LLP is the record a later reader trusts when deciding whether a test is load-bearing. As written it asserts a property those two tests do not have, and the next person to touch this file has no way to tell which of the listed tests actually discriminate. (The PR body reportedly gets this right by calling out two tests that "pass pre-fix by design"; the LLP does not, and the LLP is the artefact that survives.)

The tests themselves are fine and are not vacuous — I proved both by mutation, see below. This is a doc-accuracy fix only.

Exact fix: replace lines 163-166 with a sentence that separates the two roles, e.g.

Unit tests. Six were shown failing against the pre-fix source before being
kept: `ESC` in a `table` cell, `ESC` and bidi in a `markdown` cell, a newline
that cannot forge a row, tab/CR spellings, column alignment with an escaped
cell, and the `--output` receipt. Two more pass pre-fix by construction and
guard preserved behaviour rather than the fix: `json`/`jsonl` byte-exactness
(fails if the escape is ever applied to a machine format) and non-ASCII and
emoji survival (fails if the display class is widened to the zero-width
group). Both were confirmed by mutating the code they guard.

---

2. nit — src/core/query/format.js:344-353 — the receipt rationale and its @ref landed inside the full parameter's description.

* @param{string}outputPath* @param{{columns: string[],rows: Record<string,unknown>[]}}full*Thereceiptisahuman-facingrenderinitsownright,whatever*`--format`thefilegot,soitspreviewisescapedtoo. ...
** @refLLP0225#decision [constrained-by]: thereceiptisahumanrender,soitescapes** @param{string}contentthealready-renderedfilecontent(sizedforthereceipt)

In JSDoc, everything between a @param tag and the next tag is that parameter's description, so this paragraph and the @ref document full rather than the function, and the @param list is now split in two by a block of prose. tsc does not care (typecheck is clean) and llp-ref-hygiene accepts it (11/11), so nothing is broken; it just reads as a paste that missed its target, and it makes the @ref look attached to a parameter.

Why it matters: this is the one place in the diff where the rationale-to-construct attachment CLAUDE.md asks for is visibly wrong, and /ref-story renders this file by those attachments.

Exact fix: move the paragraph and the @ref line up so they sit in the description block above @param {string} outputPath, immediately after the existing "Render the stdout receipt..." paragraph, leaving the three @param tags and @returns contiguous at the bottom.

---

3. nit — src/core/query/overview.js:706 and :844 — truncate runs after escapeForDisplay, so a \uXXXX escape can be cut in half.

truncate(escapeForDisplay(hasModelLabel(r) ? String(r.model).trim() : '(model not recorded)'),MAX_MODEL_WIDTH),
...
truncate(cell(r.tool_name),MAX_MODEL_WIDTH),

MAX_MODEL_WIDTH is 30 and truncate slices to width - 1 then appends …. A model name whose ESC sits near the boundary renders as a partial escape. Measured, one raw ESC at offsets 22-28 in a 30-column budget:

pad=24 "mmmmmmmmmmmmmmmmmmmmmmmm\u001…"
pad=25 "mmmmmmmmmmmmmmmmmmmmmmmmm\u00…"
pad=26 "mmmmmmmmmmmmmmmmmmmmmmmmmm\u0…"
pad=27 "mmmmmmmmmmmmmmmmmmmmmmmmmmm\u…"
pad=28 "mmmmmmmmmmmmmmmmmmmmmmmmmmmm\…"

This is cosmetic only and I want to be clear it is not a security hole: the truncated tail is still pure printable ASCII, cannot reintroduce a control character, and the trailing … makes the elision evident. src/core/query/format.js does not have this problem because applyContextControls clips the raw value before renderResult escapes it (I swept ESC across offsets 0-29 at --max-cell 20 and got zero truncated escapes and zero leaks).

Why it matters: it is the one place the PR's own stated invariant ("the output is pure ASCII, one column per character") is applied in the wrong order relative to a width clamp, and a reader debugging a mangled \u00… tail will not immediately know it is a display artefact rather than corrupt captured data.

Exact fix: clip first, escape second, matching format.js:

escapeForDisplay(truncate(hasModelLabel(r) ? String(r.model).trim() : '(model not recorded)',MAX_MODEL_WIDTH)),

and for the tool column, replace truncate(cell(r.tool_name), MAX_MODEL_WIDTH) with a clip-then-cell form. Note the trade this makes: the escaped result can then exceed 30 columns, which renderTable at overview.js:939 absorbs correctly because it computes widths from the finished strings with no cap. If the author prefers the current ordering, say so in the @ref gloss instead; either is defensible, but the ordering should be deliberate and stated.

---

4. nit — llp/0225...decision.md:145-148 (Consequences) names the table newline change but not the markdown one.

The LLP records: "A newline inside a table cell now prints as \n instead of breaking the row." It does not record that markdown's handling also changed. Pre-fix, mdEscape did .replace(/\n/g, ' '); that replacement is deleted at format.js:298-300, and a newline in a markdown cell now renders as a literal \n rather than a space.

I confirmed there is no conflict and no double-handling: mdEscape is module-private and both of its call sites (format.js:233 header, format.js:237 body) pass text that has already been through escapeForDisplay, so no raw newline can reach it and the removal is dead-code removal. Markdown rows stay one line in every one of the 36 attack cases I ran. So the code is correct; only the record is incomplete.

Why it matters: --format markdown is what the shipped query skill tells an agent to use for "tables you show the user" (hypaware-core/plugins-workspace/claude/skills/hypaware-query/SKILL.md:16), so this is a user-visible rendering change in the one format aimed at end users, and it is the sort of thing the next reader will want to find in the Consequences list rather than in a diff.

Exact fix: extend the existing bullet to "A newline inside a table cell now prints as \n instead of breaking the row, and a newline in a markdown cell prints as \n rather than being flattened to a space. That is a visible change to multi-line prose, and it is the point..."

---

The escape class, attacked from both sides

What I tried to sneak through raw. 36 vectors, each rendered through the real renderResult in table and markdown and through buildQuerySqlOutput's --output receipt, then scanned for any surviving character in [\u0000-\u0009\u000b-\u001f\u007f-\u009f\u2028\u2029\u061c\u200e\u200f\u202a-\u202e\u2066-\u2069] (U+000A excluded because it is the renderer's own line terminator), plus a line-count assertion that the table stays exactly three lines.

7-bit ESC forms: CSI hide ESC[8m, cursor-up overwrite ESC[1A, OSC 0 window-title set, OSC 8 hyperlink with ESC\ string terminator, clear-screen + home, set-scroll-region, DEC line-drawing charset switch, ESC c full reset, ESC P ... ESC \ DCS. 8-bit C1 forms: CSI 0x9B, OSC 0x9D, DCS 0x90, ST 0x9C, NEL 0x85, SS3 0x8F, APC 0x9F. Bare C0: CR line-overwrite, LF row-forge, TAB, VT, FF, NUL, BEL, BS-erase, SO/SI, DEL. Bidi: RLO U+202E, LRE/PDF, RLI/PDI isolates, ALM U+061C, LRM/RLM, LS/PS U+2028/2029. Non-string carriers: a control sequence buried two levels deep inside an object value, inside an array element, and via the Date/bigint/boolean branches of rawCell. Plus attacker-controlled column names (SELECT 1 AS "<esc>") in table and markdown, both with rows and in the zero-row branch that returns early.

Result: 0 leaks, 0 forged rows, in all three renders. The zero-row early returns are covered too, which is easy to miss (format.js:196 uses headers, not columns).

On the C1 claim specifically: I verified it end-to-end. hyp query sql "... 'x' || chr(155) || 'y' ..." --format json emits the C1 byte raw (M-bM-^@M-. for the bidi case, C1 likewise), confirming JSON.stringify escapes C0 and passes C1 and bidi through untouched. So the author's stated reason for including C1 holds. Whether a modern UTF-8 terminal actually acts on U+009B is arguable (xterm in UTF-8 mode generally does not), which makes C1 coverage defence in depth rather than the load-bearing part; either way it costs nothing legitimate, because U+0080-U+009F encode no printable character.

Completeness argument for the class: every terminal escape sequence must begin with ESC (0x1B) or a C1 introducer (0x80-0x9F). Both ranges are in TERMINAL_CONTROL_CHARS, so there is no character outside the class that can open a sequence, and once the introducer is escaped the remainder is inert printable text. The deliberate exclusions are correspondingly safe: zero-width and default-ignorable characters cannot move a cursor, and a backslash cannot either. I also confirmed the leftovers are genuinely unable to repaint — the 27 code points that sanitizeLabel strips but escapeForDisplay does not are exactly 00AD 180E 200B-200D 2060-2064 FE00-FE0F FEFF, all invisible-formatting, none an introducer.

Gaps I looked for and judged out of scope rather than findings: astral tag characters U+E0000-E007F are default-ignorable and not in either class, but they are load-bearing in tag-sequence emoji and cannot drive a terminal; the label plane does not cover them either, so this is pre-existing and consistent. Confusables are excluded by an argument the LLP inherits from LLP 0164 and I agree with.

What I tried to get wrongly mangled. 21 legitimate strings, each asserted to survive verbatim in all four formats (table, markdown, json, jsonl): NFC accents, NFD combining accents, CJK, Hebrew and Arabic RTL letters (not overrides), an Arabic/Latin bidirectional mix, a four-person ZWJ family emoji, a skin-tone + ZWJ profession emoji, a VS16 heart-on-fire, a regional-indicator flag, a tag-sequence flag, a Windows path with five backslashes, a regex full of backslashes, a JSON blob with an embedded literal \n, Devanagari with a virama, Thai combining marks, mathematical symbols, and — deliberately — a soft hyphen, a ZWSP and a mid-string BOM, which the design says must be left alone.

Result: 0 mangled. Confirmed again through the real CLI: node bin/hypaware.js query sql "select 'café 👨‍👩‍👧' as legit" renders the emoji and the accent as raw UTF-8, while the same row's ESC and RLO come out as \u001b and \u202e.

I then mutated the class to widen it (DISPLAY_UNSAFE_CHARS also taking INVISIBLE_FORMATTING_CHARS) and the "ordinary non-ASCII text is not touched" test fails, so the boundary is guarded from the over-escape side, not just the under-escape side. That is what makes that test non-vacuous despite passing pre-fix.

Also checked, clean

The sanitizeLabel decomposition, exhaustively and independently. I did not trust the PR's test. I extracted the pre-PR regex literal from git show HEAD~2:src/core/util/json_util.js and the three group strings from HEAD, composed the new class myself, and compared them character by character over the entire BMP (0x0000-0xFFFF) and the entire astral range (0x10000-0x10FFFF): 0 disagreements in 1,114,112 code points. I then repeated the BMP sweep through the real exported sanitizeLabel against the old literal as oracle: 0 disagreements. The display class is a strict subset of the label class (79 code points vs 106; nothing escaped that is not also stripped). escapeForDisplay changes a character if and only if it is in the display class (0 disagreements over the BMP), leaves every astral code point untouched (0 of 1,048,576 mangled), and never leaves a residual control or bidi character in its own output.

The PR's own test at test/core/util-json-util.test.js:78-88 does what it claims: it is a real 65,536-iteration sweep, and it correctly declares its oracle regex without the g flag, avoiding the lastIndex statefulness trap that would have made .test() alternate and silently halve the coverage. Mutating one code point out of the composed class (dropping \u180E) fails it. I would still keep my astral sweep in mind as uncovered by the repo's test, though it is provably moot while all three groups stay BMP-only.

Column widths, clipping and truncated escapes.renderTable measures on the escaped text and pads with it, and the escaped header participates in the width (format.js:207-212). Alignment holds for an escaped cell that sets the column width and for one shorter than its header. The 80-column cap at format.js:211 does let a long cell overflow and push later columns right — but that is pre-existing and unrelated to escaping: a 20-character all-ESC cell (120 escaped) and a plain 120-character cell produce byte-identical line lengths [83, 83, 123, 83]. Default --max-cell is 200 (verb_codec.js:23), so cells above 80 already overflowed before this PR. Escaping cannot cut an escape in half in this file, because applyContextControls clips the raw value before renderResult escapes: I swept an ESC across offsets 0-29 at --max-cell 20 and found 0 leaks and 0 partial escapes. (The one place the ordering is reversed is overview.js, finding 3.) Separately, the width computation still counts UTF-16 code units, so a wide CJK or emoji cell misaligns in a real terminal — pre-existing, unchanged, and the LLP is careful to scope its "one column per character" claim to the escapes it emits, which is accurate.

overview.js, and the colour-bar reasoning. All four body blocks route every captured column through the escape: provider and date via cell() (:702, :751, and the sparkline at :402), model at :706, repo_root at :810, tool_name at :844. I found no captured column bypassing it. I then verified the author's stated reason for escaping per cell rather than per row: with color: true and control sequences in all four columns, 62 raw ESC runs survive and every one matches ESC [ ... m — SGR colour only, no CSI, no OSC, nothing else; with color: false, zero raw ESC in the entire block. A sweep over the assembled table really would have taken the colour with it. renderTable at overview.js:939 computes widths from the finished strings with no cap, so escaped cells cannot misalign that block.

json/jsonl byte-exactness, via a real query rather than the unit test. I ran the same statement through node bin/hypaware.js query sql with a temp HYP_HOME at both HEAD and HEAD~2, in both machine formats, and byte-compared: json 136 bytes byte-identical, jsonl 103 bytes byte-identical. The raw RLO is still present in the post-fix json output, confirming it is untouched rather than coincidentally absent. Mutating renderResult to escape the machine formats fails the byte-exactness test, so that test is a real guard.

The MCP path.src/core/mcp/server.js imports only jsonReplacer from format.js and returns JSON.stringify(safe, null, 2) at :137-139. It never reaches formatCell, renderResult or renderTable, so it is unaffected. The --remote path goes through the same verb render, so it inherits the format rule correctly.

Who parses hyp query sql table output. Nobody. The only split(' ') in the repo is inside the PR's own alignment assertion at test/core/query-format-escaping.test.js:93. The shipped query skills direct agents to --format json for reasoning and --format jsonl --output for lossless extraction; only --format markdown is aimed at rendering to a person. hypaware-core/plugins-workspace/vector-search/src/commands.js:70 calls renderResult with the caller's chosen format and so inherits the rule, as the LLP's Scope section says.

mdEscape interaction. No conflict, no double-handling (see finding 4). One thing I checked and cleared: mdEscape turns | into \|, so a value ending in a backslash immediately before a pipe would produce \\| and break the cell. escapeForDisplay cannot create that, because every escape it emits terminates in an alphanumeric (\n, \t, \u001b), never a bare backslash. The hazard exists only for a backslash already in the captured data, which is pre-existing and unchanged by this PR.

Tests re-derived, including mutants. Full npm test: 4037 pass / 0 fail / 1 pre-existing skip. npm run typecheck: clean. Smokes status_diagnostics, local_parquet_export, gateway_codex_capture: all ok (I did not run package_bin_boot or walkthrough_picker_to_first_query, red on master per #758/#750). Pre-fix discrimination: 6 of 8 format tests fail, the overview test fails, and util-json-util.test.js fails to load — the author's "eight results" claim is accurate as a count, with the caveat in finding 1 about how the LLP describes two of them. Mutants I ran independently: escapeForDisplay returns its input unchanged → 9 failures; display class drops the bidi group → 5 failures; display class widens to include zero-width → 2 failures including the "non-ASCII" test; machine formats also escaped → byte-exactness and receipt tests fail; INVISIBLE_FORMATTING_CHARS narrowed by one code point → the BMP sweep fails; overview.jscell() stops escaping → the overview test fails. One mutant survives: measuring table widths on the raw column name instead of the escaped header (format.js:210) passes everything. That branch only matters when a column name carries a control character, which is the defence-in-depth path the author flags as such at format.js:194-196, so I am recording it as an untested branch rather than a finding.

LLP 0225 itself. Number free: no 0220-0224 exists on this branch and no file or reference to LLP 0224 survives anywhere in the tree — the renumber in 588bf91 is complete, including the UNSAFE_LABEL_CHARS_BEFORE_LLP_0225 fixture name. All five anchors cited from code and tests (#decision, #escape-class, #escape-not-strip, #format-not-tty, #one-vocabulary) are defined in the doc; llp-ref-hygiene passes 11/11. Every cross-reference resolves: LLP 0189 #choke-point exists at 0189:82, LLP 0164 and LLP 0054 exist. Header block matches the corpus convention and all three Systems values (CLI, Query, Observability) are established vocabulary. No merged LLP is rewritten by this PR — the only doc touched is the new one. Substantive claims I spot-checked and confirmed: the format-not-isTTY switch (no isTTY anywhere in format.js), the --output receipt behaviour, the "one vocabulary two policies" refactor, the zero-width exclusion rationale, and even the out-of-scope factual claim that hyp graph neighbors clamps labels to 48 characters without stripping (context-graph/src/verb.js:230,245 — accurate).

Conventions. No em dash (U+2014) in any of the eight files. No trailing semicolons in the new JavaScript. No @typedef, no inline import('...') types. @import specifiers in the touched files are root-anchored .js paths ('../../../src/core/query/types.js', '../../../hypaware-plugin-kernel-types.js'). The new value import at format.js:3 and overview.js:18 is a relative .js runtime import, which is correct for a value.

On the untested-terminal caveat. The author states they could not test against a real terminal emulator, and the LLP repeats it under "Not verified". I judge nothing material to depend on it. The safety argument does not rest on observing a terminal: it rests on the output being pure printable ASCII, which I verified exhaustively over the whole BMP (escapeForDisplay never leaves a control or bidi character in its output, for any input code point) and end-to-end through the real CLI with cat -v. A string containing no ESC and no C1 byte cannot open an escape sequence on any terminal, so the missing emulator test would only re-confirm a property already established by construction.

[philcunliffe]

neutral review - round 2 (final)

Head reviewed: 5bcd50e. All 9 checks SUCCESS. Reviewed in a detached worktree;
nothing was written to the branch.

Three findings, none ship-blocking. All four round-1 findings are fixed, and the
reviewer re-derived the pre-fix split itself in a .git-free copy of the tree rather
than trusting the fixer.

Finding 1 is the recursive one and worth reading: the round-2 commit rewrote the LLP
Verification section to fix round-1 finding 1, and in the same commit added a ninth
test without updating the enumeration it had just corrected. Same paragraph, same
class of inaccuracy, one round later. The substance of the six-versus-two split is
right; the omission is the new test, which is also the most interesting one (the
attacker-controlled column name, the branch round 1 found untested).

---

Review complete. Worktree and main repo are clean; scratch files removed.

---

VERDICT: findings

Three findings: one minor, two nits. None are ship-blocking. All four round-1 findings are genuinely fixed, the round-2 rendering change is behaviourally correct, and nothing round 1 cleared regressed. Head reviewed: 5bcd50e, all 9 checks SUCCESS, reviewed in a detached worktree; nothing was written to the branch.

---

1. minor — llp/0225-captured-text-is-escaped-for-display.decision.md:164-165 — the rewritten Verification section is stale by one test, in the same commit that added the test.

The section now reads "Unit tests. Six were shown failing against the pre-fix source before being kept: ESC in a table cell, ESC and bidi in a markdown cell, a newline that cannot forge a row, tab/CR spellings, column alignment with an escaped cell, and the --output receipt. Two more pass pre-fix by construction..." That is 8 tests. test/core/query-format-escaping.test.js now holds 9, because this same commit added the escaped-header width test at :99.

I re-derived the split myself rather than trusting the fixer: I built a .git-free copy of the tree and restored the four pre-fix source files from the merge base c483c1a, then ran the file. Result: 7 fail, 2 pass, not 6 and 2.

not ok 1 - table format escapes every control and bidi character in a cell
not ok 2 - markdown format escapes control and bidi, and still escapes pipes
not ok 3 - a newline in a table cell cannot forge a row
not ok 4 - tab and carriage return get their familiar spellings
ok 5 - json and jsonl stay byte-exact, control characters included
ok 6 - ordinary non-ASCII text is not touched by either human format
not ok 7 - column widths and alignment survive an escaped cell
not ok 8 - table column width is measured on the escaped header, not the raw column name
not ok 9 - the spill receipt escapes its preview but the file it wrote does not

The six items the sentence names each map to a test that really does fail pre-fix, and the two it calls preserved-behaviour guards really do pass pre-fix, so the split's substance is correct. The omission is the new test 8 alone.

Why it matters: this is the same class of inaccuracy round-1 finding 1 raised, in the same paragraph, reintroduced by the round-2 commit that rewrote it. The Verification section reads as an exhaustive enumeration of that file, and the omitted test is the most interesting one (attacker-controlled column name, the defence-in-depth branch that round 1 found untested).

Exact fix: change Six to Seven at :164 and extend the enumeration at :165-167, e.g. ...column alignment with an escaped cell, column width measured on an escaped column name, and the --output receipt.

---

2. nit — src/core/query/overview.js:706 and :844 — the round-2 rendering change ships with no test; reverting it passes the entire suite.

I mutated both hunks back to the round-1 ordering (truncate(escapeForDisplay(...), MAX_MODEL_WIDTH) and truncate(cell(r.tool_name), MAX_MODEL_WIDTH)) and ran everything: node --test test/core/query-overview.test.js63/63 pass, npm test4038 pass / 0 fail. The single overview escaping test at test/core/query-overview.test.js:1021 uses values well under 30 characters, so it never reaches the clamp and cannot see the ordering at all.

The fix is correct (verified independently, see below), but the invariant the new JSDoc at :1025-1027 now states in prose is unguarded: a future edit that reorders it back regresses silently, exactly as it did before round 1 caught it by reading. This is the one place in the PR where an asserted invariant has no assertion behind it, and the PR is otherwise unusually well pinned.

Why it matters: the fixer did add a pin for the mutant round 1 reported, so the standard the PR sets for itself is that a found defect gets a test. The defect it fixed in round 2 did not get one.

Exact fix: add to test/core/query-overview.test.js, next to the existing escaping test:

test('overview clips the raw value before escaping, so no escape is cut in half',()=>{constESC='\u001b'constout=renderProviderMix([{provider: 'p',model: 'm'.repeat(28)+ESC+'zzzz',input_tokens: 1,cached_tokens: 1,output_tokens: 1}],false)assert.equal(out.includes(ESC),false)// No partial `\uXXXX`: every backslash-u carries all four hex digits.assert.equal(/\\u[0-9a-f]{0,3}(?![0-9a-f])/.test(out),false)})

and the same shape against renderToolMix for the cell(value, width) path.

---

3. nit — src/core/query/overview.js:1025-1027 (and llp/0225...:85-87) — width no longer bounds the rendered column, and neither the JSDoc nor the LLP says so.

The new JSDoc says width"clips the raw trimmed value before escaping rather than after". True, but the consequence is that the rendered cell is no longer bounded by width: 30 raw ESC clip to 30 and then expand 6x. I built one and looked at the output:

 provider model <175 columns> input cached output by input+output
p \u001b\u001b ... \u001b… 10 20 30 ▒▒▒▒▒███...
q claude-opus-5 10 20 30 ▒▒▒▒▒███...

Alignment is correct - both data rows measure 230 characters and every later column starts at the same offset, because renderTable at :939 computes widths from the finished strings with no cap. But format.js:211 caps its widths at Math.min(width, 80) and overview.js has no equivalent, so MAX_MODEL_WIDTH = 30 is now advisory in the models and tools blocks and a hostile row wraps the whole table on any normal terminal. Pre-round-2 (and on master) these two columns could not exceed 30.

This is the trade round 1 named explicitly when it proposed the fix, and it is the right trade: it only fires on data that already contains control characters (escaping is a no-op on legitimate text, so real rows are unaffected - confirmed over the whole legitimate corpus), and a 175-column row is strictly better than a half-cut escape. So this is a documentation gap, not a defect.

Exact fix (doc-only): extend the cell JSDoc paragraph at :1025-1027 with a sentence such as "The escaped result can therefore exceed width - up to six times it for an all-control value - which renderTable absorbs because it measures finished strings with no cap." Optionally mirror it in LLP 0225's Consequences.

---

Round-1 findings, re-derived

1. minor, LLP Verification claimed five tests all discriminated — fixed, but with a new problem. The six-versus-two split is now present and every item it names is correct: I re-derived it against the pre-fix sources at c483c1a in a .git-free copy and got 7 fail / 2 pass. Both preserved-behaviour guards are non-vacuous, confirmed by my own mutation: widening DISPLAY_UNSAFE_CHARS to include INVISIBLE_FORMATTING_CHARS fails only test 6; wrapping the json branch in escapeForDisplay fails only test 5. The new problem is that the paragraph was not updated for the ninth test the same commit added - finding 1 above.
2. nit, receipt rationale and @ref inside the full param's JSDoc — fixed.src/core/query/format.js:344-354: the prose and @ref LLP 0225#decision now sit in the function description block directly under "Render the stdout receipt...", and the three @param tags plus @returns are contiguous at :351-354. llp-ref-hygiene 11/11.
3. nit, truncate after escapeForDisplay in overview.js — fixed, correctly.:706 is now escapeForDisplay(truncate(raw, MAX_MODEL_WIDTH)), which restores master's clip-on-raw ordering; :844 is cell(r.tool_name, MAX_MODEL_WIDTH) and cell clips the trimmed raw value at :1038 before escaping at :1039. I swept an ESC across offsets 0-40 in both the model and tool columns: 0 raw ESC leaks and 0 partial \uXXXX fragments in either, versus the five partial escapes round 1 measured at pads 24-28. Escaping cannot reintroduce a truncation artefact because it runs strictly after the clip and only expands. Alignment verified by building an over-width cell (finding 3 above): rows stay equal-length and columns stay aligned. cell()'s other three call sites (:402, :702, :751) and its two internal uses in formatCount (:1093, :1095) all pass one argument, so width === undefined and they take the identical pre-existing path - byte-identical behaviour, and the corpus run confirms it. Two follow-ons: no test (finding 2) and an unstated width consequence (finding 3).
4. nit, Consequences named only the table newline change — fixed and accurate.llp/0225...:94-97 now covers both. I verified the underlying claim rather than the wording: git show c483c1a:src/core/query/format.js:253 had .replace(/\|/g, '\\|').replace(/\n/g, ' '), format.js:299 now has only the pipe replacement, and a real markdown render of 'a\nb' gives "| c |\n| --- |\n| a\\nb |\n" - a literal \n, one line, exactly as recorded.

• The pinned mutant — genuinely pinned, and it discriminates for the right reason. I re-derived the mutant myself (format.js:210, let width = headers[i].length to let width = column.length). Unmutated: 9/9 pass. Mutated: only test 8 fails, and both its assertions fail (the divider segment measures 2 instead of 7, and A lands at index 4 instead of 9), because padEnd never truncates so the header overruns its own column. Full npm test under the mutant: 4037 pass / 1 fail, so CI would catch it. The test derives its expected width from the escaped form rather than hardcoding 7, which is the right way round.

Also checked, clean

The process near-miss — the tree is clean of it.git diff 588bf91..5bcd50e touches exactly four files, +35/-12, and every hunk is one of the four findings' fixes or the new test; there is no stray reversion and no old blob. json_util.js and util/index.js are untouched between 588bf91 and 5bcd50e, so they are byte-identical to what round 1 reviewed. The full PR diff c483c1a..5bcd50e is the same eight files round 1 saw (+558/-33). The shared index in /work/hypaware has 0 staged entries and the only status line is the pre-existing untracked hypaware/ directory, so nothing survived there either. My own worktree finished with an empty git status --porcelain; I created it with git worktree add --detach (its own index) rather than copying, and removed my scratch directory.

Attack corpus, replayed against this head. 27 vectors (7-bit ESC forms: CSI hide, cursor-up, OSC 0 with BEL, OSC 8 hyperlink with ESC\ ST, clear+home, set-scroll-region, DEC line-drawing, ESC c full reset, DCS; 8-bit C1: CSI 0x9B, OSC 0x9D, DCS 0x90, NEL 0x85, SS3 0x8F, APC 0x9F, ST 0x9C; bare C0: CR, LF, TAB, VT, FF, NUL, BEL, BS, SO, SI, DEL; bidi: RLO, LRE/PDF, isolates, ALM, LRM/RLM, LS/PS). Each rendered through renderResult in table and markdown as a cell value, as an attacker-controlled column name with rows and through the zero-row early return, through buildQuerySqlOutput's --output receipt stdout, and through all four overview blocks. Scanned for any surviving character in the control/bidi class (U+000A excluded as the renderer's own terminator) plus a three-line assertion on the table. 0 leaks, 0 forged rows. The receipt behaves correctly in both directions: for the C1 CSI vector its stdout preview reads \u009b8m while the file it wrote holds the raw byte.

Legitimate corpus, replayed. 21 strings (NFC and NFD accents, CJK, Hebrew and Arabic RTL letters, an Arabic/Latin mix, a four-person ZWJ family emoji, a skin-tone ZWJ profession emoji, a VS16 heart-on-fire, a regional-indicator flag, a tag-sequence flag, a Windows path, a backslash-heavy regex, a JSON blob with a literal \n, Devanagari with a virama, Thai combining marks, maths symbols, and deliberately a soft hyphen, a ZWSP and a mid-string BOM) asserted to survive verbatim in table, markdown, json and jsonl, plus in the two overview columns the round-2 change touched. 0 mangled - so the reorder did not introduce a mangle on the path it altered.

json/jsonl byte-exactness.jsonl and json are byte-identical to JSON.stringify(...) for a row carrying ESC, RLO, a C1 introducer and a ZWJ emoji, and the raw RLO and raw C1 are both still present in the json output - untouched, not coincidentally absent.

overview with color: true. With control sequences in provider, model, repo_root and tool_name: 46 raw ESC runs survive and every one matches ESC [ ... m - SGR colour only, 0 non-SGR. With color: false: 0 raw ESC and no character of the leak class anywhere in the block. Round 1's reasoning for escaping per captured cell rather than sweeping the assembled table still holds exactly.

Tests and gates, all run in this worktree after a fresh npm install.npm test: 4038 pass / 0 fail / 1 pre-existing skip (4037 at round 1, +1 for the new test). npm run typecheck: clean, including the new optional @param {number} [width]. node --test test/core/llp-ref-hygiene.test.js: 11/11. node --test test/core/query-format-escaping.test.js: 9/9. node --test test/core/query-overview.test.js: 63/63. Smokes status_diagnostics and local_parquet_export: both ok. I did not run package_bin_boot or walkthrough_picker_to_first_query (red on master per #758/#750).

Conventions, over all eight changed files. No em dash (U+2014) anywhere. No @typedef, no inline import('...') types. No trailing semicolons in code - the four ;$ hits are all mid-sentence prose inside comments. @import specifiers are root-anchored .js paths ('../../../src/core/query/types.js', '../../../hypaware-plugin-kernel-types.js', '../../src/core/query/types.js' in the test). On the fixer's own note about replicating finding 2's mistake: the final state is right - the width paragraph at overview.js:1025-1027 sits in the function description block, above the @ref at :1029 and above the @param list at :1031-1033, so nothing is attached to a parameter.

Non-display use of cell().overview.js:402 uses cell(r.date) as a planning key in chooseOverviewWindow (sorted, and compared against '(none)') rather than as a rendered string. That is pre-existing from the round-1 head, unchanged by round 2, and harmless (a date carrying a control character would sort differently, nothing more). renderRepoMix:810 escapes after shortRepo, which is a path-component reduction rather than a width clip, so it has no escape-cutting hazard.

[philcunliffe]

neutral triage - ship

Review budget exhausted with three residual findings. All three are preferences.
This PR can merge. Deferred to #766.

Finding 2 was the one to think hardest about, and triage re-derived both halves

"Correct code, no test" is the shape that reaches blocker status when the unguarded
invariant is load-bearing for security. So triage did not classify it from the
label; it measured what a future re-reorder would actually cost.

• The shipped code is correct. A raw ESC swept across offsets 0-40 in both the
  model and tool columns at this head: 0 raw ESC leaks, 0 partial \uXXXX
  fragments.
• The invariant is genuinely unguarded. Mutating both hunks back to the round-1
  ordering passes query-overview 63/63 and the full suite 4038 / 0.
• What the silent regression would be. Under that mutant, the same sweep produces
  8 partial fragments and still 0 raw ESC leaks. The truncated tail is pure
  printable ASCII with a visible ellipsis, and nothing in it can open an escape
  sequence.

So the security property survives the mutation and only the cosmetic spelling
regresses. That is a test nicety by the stated definition, and it is the first thing
to land as follow-up, because the PR's own standard (a found defect gets a pin) is
right.

Finding 3: a wrapped table versus a bounded row that lies

Triage built the hostile row and looked at it. A model of 30 raw ESC renders as a
175-character cell, making the row 235 characters, which wraps the table on any
normal terminal. Alignment still holds (renderTable measures finished strings, so
it widens rather than misaligns), and legitimate values are untouched, since the
escape is a no-op on text without control or bidi characters, so the widening is
reachable only when the data already carries an attack.

Weighed against what that same row did before this PR: it repainted the screen, hid
itself, or reordered what the operator read, while the columns appeared bounded. A
visibly wide but honest row is strictly better than a bounded row that lies. Round 1
named this trade when it proposed the reorder; what is actually wrong is that neither
the JSDoc nor the LLP states the consequence.

Finding 1

The Verification enumeration is stale by one test, in the very commit that rewrote it
to fix the same class of error. Embarrassing, not a production defect, and an
editorial correction under LLP 0156's mechanical class.

The baseline that frames all three

Before this PR, every captured string column was a live terminal-injection vector on
the exact surface an operator uses to triage hostile traffic. Two independent
adversarial rounds at this head found 0 leaks, 0 forged rows, 0 mangled legitimate
strings, byte-exact json/jsonl, an unaffected MCP path, and a sanitizeLabel
decomposition byte-identical over all 1,114,112 code points. Blocking would keep the
injection vector shipped to protect a doc sentence, a missing cosmetic pin, and an
undocumented width trade.

Verified at head 5bcd50e after a fresh install: npm test 4038 pass / 0 fail / 1
pre-existing skip, npm run typecheck clean, smokes status_diagnostics and
local_parquet_export ok.