Bound what the brain stage can hand to the speaker - #75
Conversation
A reply is model output going straight to Piper, so VOICE_SYSTEM_PROMPT asking for short plain text is a request rather than a guarantee. A confused or truncated model still emits markdown, emoji, control bytes, or pages of prose, and an empty reply speaks as silence instead of as a failure anyone can hear. Route both backends through sanitize_reply() at brain()'s return, so there is one choke point instead of a guard per backend that the next backend would forget to add. The spoken error strings returned on timeout and transport failure pass through unchanged. Three things the naive version of each guard gets wrong, so they are worth naming. ANSI sequences are dropped whole, because _brain_cli returns the claude CLI's stdout and removing only the ESC byte would leave "[31m" to be read aloud. Emphasis stripping is boundary-aware: a delimiter run counts as markdown only where it hugs a word on exactly one side, so a spoken glob (*.py), math (5 * 6), and snake_case identifiers keep their literal symbols. An unterminated fence is dropped with its contents, since a truncated code block is the case where speaking the code would be worst. ssh_cli_send's shlex.quote of the untrusted prompt is unchanged and still covers the transport half of the issue. closes #24 wake-20260716T1705-46da79
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d0503b969
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Each of the three stripping rules recognised a subset of its real grammar and let the remainder through to Piper, which is the one thing this stage exists to prevent. The subsets were drawn from the cases seen so far, so each one fails again as soon as a reply uses a construct nobody had hit yet. CSI took digits and semicolons as its parameter bytes, but ECMA-48 allows all of 0x30-0x3f, so a colon-separated true-colour SGR never matched and the control pass took only its ESC, leaving "[38:2::255:0:0m" to be read aloud. Fences took backticks alone, though markdown opens a block with either character, so a tilde-fenced block was spoken as code. Emphasis was inferred from a delimiter run hugging a word on one side, which is not what markdown means by emphasis: it means a matched pair. Reading it that way ate the unpaired runs that carry meaning in a reply about code (*args, **kwargs, _private, VALUE_), and because inline ticks were peeled before it ran, it could not tell `__init__` from emphasis either -- a code span is literal precisely so that it can. Matching a pair rather than a one-sided hug needs care about cost: content that may cross its own delimiter makes an unpaired run rescan the rest of the reply, which is quadratic on exactly the long reply this stage is for. Barring content from crossing its own delimiter keeps that linear and is what nesting means anyway, so the pass repeats until the innermost pairs are gone.
A review finding asked for "__init__" to keep its underscores in the spoken reply, reading their loss as corruption. It is the same token as "__bold__": one paired "__" run hugging content, which CommonMark renders as strong emphasis either way. No rule keeps the delimiters around the identifier without also reading them aloud around real bold text, and the reply gains nothing for the trouble, since an underscore is not pronounced. A reply that needs the identifier verbatim asks with a code span, which is the one thing that tells the two apart, and the stage already orders itself so that it can. The finding is plausible enough to arrive again as a fix. Pinning the identifier and the bold text in one test states that they are one decision, so reversing it fails the suite rather than trading a spoken "init" for spoken underscores.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ddfc1f440
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Four review findings, each reproduced before it was touched. The fence one deletes speech, which is the worst of them: nothing downstream can tell a truncated reply from a short one. Fences are block constructs, but both fence rules matched anywhere, so a reply that MENTIONS ``` read as opening a block and the unterminated-fence rule dropped everything after it. Measured: "To open a block, type ``` and then your code. That is the whole trick." spoke as "To open a block, type". Two mentions in a sentence were worse -- they paired into a block and ate the words between. Anchoring both rules to a line start (up to 3 spaces, as markdown allows) is the actual grammar. An inline run left behind is not spoken anyway: it pairs off as a code span, or the final tick sweep takes it. The other three leak markup into speech rather than removing it: A CSI sequence cut off mid-parameters carries no final byte, so the pattern missed it and the control pass took only the ESC, leaving "[31" to be read aloud. The final byte is optional now, which is what the OSC branch beside it already does for the same reason. A terminated sequence still matches whole. "1" + U+FE0F + U+20E3 is one keycap glyph. The class took the variation selector and left the digit wearing U+20E3, so an emoji combining mark reached Piper despite the emoji-free guarantee. U+20E3 and U+FE0E join the class. A link destination may carry balanced parens, so "/wiki/Foo_(bar)" stopped the non-nesting pattern at the inner ")" and left the outer one to be spoken after the label. One nesting level is what links in prose use. 79 checks pass, up from 62. All four proven red first.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97434d05e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Anchoring the fence rules to a line start fixed an inline mention being read
as a block, but a fence inside a blockquote or a list item sits behind its
marker, not at the margin -- so both rules missed it. The miss did not stop at
a leftover backtick: the marker rule ran afterwards and exposed the fence, and
the code-span rule read the bare tick run as a span and handed its contents to
Piper. Stripping markers first is what makes a fence's position knowable;
mangling a block's interior on the way past costs nothing, since the block is
deleted on the next line.
CRLF and a closer longer than its opener each read as "no closer", which sent
the rest of the reply to the unterminated-fence rule to be deleted -- silently,
which is the failure this file is built to avoid. Line endings are normalised
once at the top rather than per-rule, and the closer is now the opener plus any
further run of that character, as markdown defines it.
The link destination pattern went the other way. It spelled out one level of
paren nesting, so a destination nesting twice did not match at all, and a link
that does not match is a whole URL spoken aloud -- worse than the trailing ")"
the nesting was added to suppress. Every depth a pattern names has a next one,
so the destination is scanned and its parens counted instead, which has no
depth to outgrow. An unbalanced destination consumes to the end, the same call
the unterminated-fence rule makes: the reply was cut mid-link, and what follows
an unclosed "(" is the URL.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 388ab283e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The reply is model output and the voice loop is always listening, so how long this stage can be made to think is a property worth bounding. (`+)(.+?)\1 tried every opener length against every closing position: cost grew 7.8x per doubling of a backtick run, so 4,000 ticks took 2.3s and 8,000 about 18s. A run of one character was enough to stop the loop answering. Markdown closes a run with the next run of exactly its own length, and a run that never finds its match is literal text. Comparing runs reads that rule directly and costs time in the input's length: 8,000 ticks now take 2.8ms. The pattern was also reading the rule wrong, which is why it was slow -- in "`a``b`" it closed the first tick at the nearest tick rather than at the next run of length one, splitting one span into two. Both fall out of the same fix. The emphasis rules above already bar their content from crossing their own delimiter for this reason; the code-span pass was the one that still could. The test pins elapsed time rather than a shape, because a stall is the failure mode, and the budget sits far above a scan of that input and far below the old cost at that size.
|
@codex review |
">" alone opens a blockquote -- the space is optional -- so ">```" is a fenced block inside one. The marker rule required whitespace, so that ">" stayed on the line, the fence never reached a line start, the block was never recognised, and its body was spoken. That is the exact failure the pass ordering above exists to prevent, reached through the one marker that does not need its space. A list marker does need it: "-foo" is a word, not a bullet. So the two take separate branches instead of sharing one rule. The space stays optional-but- single, which is what markdown counts as the marker itself; further indent is content. The test covers the spaceless, nested, and spaced forms together, so the fix cannot regress the shape that already worked.
|
Went through all nine. Two were real and are fixed; one is false; the rest are filed as #76 rather than widened into this PR. Fixed — "Allow blockquote fences without a space" (79a7741). Real, and the best find of the round. Fixed — the cubic code-span pattern (828832a). Measured before filing: 7.8x per doubling, 4k ticks 2.3s, 8k ~18s. Replaced Rejected — "Handle longer Markdown closing fences". Not reproducible. Filed as #76 — indented code blocks, autolinks, nested-bracket labels, DCS/C1 forms, control-bytes-before-parsing, links inside code spans, arrows read as emoji. All reproduced and recorded there with real input/output. The reason they are filed rather than fixed here: each is "one more ordered pass" on a |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 828832a5a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Pairing runs by length fixed the cubic pattern but left a second route to the same stall, and the docstring claimed a linearity the code did not have. Searching forward from each opener re-walks every run behind it whenever nothing closes it, so 400 runs of distinct lengths -- 80KB -- took 2.0s. Cubic in a single run's length was gone; superlinear in the number of runs was not. Indexing runs by length once, with a per-length cursor that only moves forward, makes an opener's cost independent of how many runs sit behind it. i never goes back, so a candidate already behind it is behind every later opener too, and skipping it is permanent. 80KB now takes 0.024s, and doubling the run count quadruples the input for 4x the time -- linear in what arrives. The claim in the docstring was the actual defect here. It described the property the fix was reaching for rather than the one it had, which is how a bound stops being checked. The test pins the reviewer's exact input.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e052a465e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Letting ">" stand alone as a marker, two commits ago, made a line of nothing but ">" strippable one character at a time -- and the strip-until-stable loop rescans the whole reply on every pass. So it cost a pass per marker, each one a full scan: 8,000 markers took 0.29s and scaled as the square, and the 50,000 a reply could carry took about 6s. The old rule needed a space after ">", so that input matched nothing and the loop stopped after one pass. The stall arrived with the fix for the marker that had no space. Matching the run with a trailing + removes the loop rather than tightening it. The loop only ever existed because re.sub does not rescan what it exposed, so "> > ```" needed a second pass to reach its inner marker; a pattern that takes both markers at once needs no second pass. 50,000 markers now take 0.014s, and the nested forms this protects -- "> > " and ">>" -- still resolve to the fence behind them. Each repetition consumes at least its marker character, so the run cannot match empty and the + cannot spin. Third stall in this file tonight, each from a different mechanism: regex backtracking, per-opener rescanning, and now a fix-and-rescan loop. They are not three bugs so much as one design generating them, which is what #76 is for.
|
Four threads: one real and fixed, three already filed. Fixed — "Bound quote-marker stripping before truncation" (bbd4c87). Real, and a regression I introduced two commits ago. Letting Fixed by matching the run with a trailing Already filed as #76 — links inside code spans, and nested link labels. Both reproduced there with real input/output. Added to #76 — the four-space list continuation fence. It is the same root as the indented-code-block gap already filed: Worth stating plainly, since this is the third stall in this file tonight and each came from a different mechanism — regex backtracking, per-opener rescanning, now a fix-and-rescan loop. They are not really three bugs. They are one design (ordered regex passes over unbounded input) that keeps producing them, and each fix has been the thing that exposed the next. That is #76's actual question, and it is a decision for Joe rather than another round here. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd4c87b5b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Three threads on bbd4c87. All three are real; none is getting a blind fix. Confirmed and measured: the emphasis quadratic (pipeline.py:918). Your reading is exactly right, including that it runs ahead of the cap. Worth noting what it sits under: pipeline.py:842 claims this design "keeps a long reply ... linear." Each Already filed: the two link gaps (pipeline.py:802, :1003). Nested bracket labels and links inside code spans are both in #76 with reproductions. They came back because I resolved the threads on the strength of the filing rather than a fix — fair. Why no fourth fix tonight. This is the fourth stall from the fourth distinct mechanism in this file today (backtracking, per-opener rescanning, a fix-and-rescan loop, now a peel-per-pass loop), and three of the four were introduced or exposed by the fix for the previous one. The root is not any single pass: Moving the bound would kill the class, including stalls nobody has found yet, but it trades away real behaviour: a reply whose first N characters are a code fence would speak silence instead of the prose after it. Depth-bounding the loop fixes this stall and leaves pathological runs to be read aloud as asterisks. A real parser ends the class. Those are different products, not different patches, so the measurements are in #76 and the call is Joe's. #75 is not merge-ready and I am not asking for it to be. Nothing here is merged. |
Three review findings on this branch turned out to be one shape: a pass that
runs before the pass that would have made it correct.
The link head read a label as "everything up to the first ]", so "[the
[docs]](url)" matched nothing, and a link this pass does not see is not a stray
bracket -- it is a whole URL read aloud, the failure the destination counting
already exists to avoid. Labels now pair the way destinations already count.
Pairing forward from each "[" would have re-walked the tail for every bracket
nothing closes ("["*8000 + "](url)" measured 5.2s on the Pi, against 0.44s for
the pattern it replaces), which is the tick rule's stall in another alphabet. So
brackets are paired with one stack pass instead, and that is also a fix for the
old pattern's own curve: the same input is now 0.007s.
An escape reads differently on each bracket. CommonMark says "\[" opens
nothing, but doing that here hands the URL to Piper, so an escaped "[" still
opens a head while an escaped "]" still does not close one. Both err toward
finding the link, which is the only direction that stays quiet.
Links were also stripped before code spans were read, so `[label](url)` in a
span -- the syntax being described, not a link to follow -- lost the syntax it
was describing. Link stripping moves inside the span loop next to emphasis,
which already had this right for `__init__`.
Tests: 122 -> 134 checks.
|
Worked all three review threads. Verified each against the real code path before touching anything; one of them turned out to be right in a way I had to be argued into. Label brackets ( Code-span ordering ( Quadratic Also pinned an escape rule the first pass got wrong: CommonMark says Tests: 122 -> 134 checks, all passing. |
Resolve the pipeline.py conflict by taking main's reply sanitizer (#75) and reformatting it to the ruff standard this branch adds. Add test_sanitize_reply.py to the fast no-model test loop so the new sanitizer is covered by the CI.
1 participant
Closes #24.
VOICE_SYSTEM_PROMPTasks the model for short plain text, which is a request rather than a guarantee. A confused or truncated model still returns markdown, emoji, control bytes, or pages of prose, and today all of it goes straight to Piper. An empty reply is spoken as silence, so a failed turn is indistinguishable from a turn that did nothing.What this does
Adds
sanitize_reply()and routes both backends through it atbrain()s return, so there is one choke point instead of a guard per backend that the next backend would forget to add. Verified that every productionspeak()of model output goes throughbrain(); the one unsanitizedspeak()isSTT_REPROMPT, a module constant.The spoken error strings returned on timeout and transport failure pass through unchanged, and
ssh_cli_sendsshlex.quoteof the untrusted prompt is untouched, so the transport half of the issue still holds.Three things the naive version gets wrong
Each of these came out of review and has a test pinning it:
\x1b[31merror\x1b[0mis spoken as "bracket 31 m error"*and_BOT_SPREN_STATE_DIRbecomesBOTSPRENSTATEDIR;*.pybecomes.py;5 * 6becomes5 6```...```pairsThe truncation bug is the subtlest:
_truncate_spokentried". ","! ","? "in order and returned on the first kind that cleared the halfway mark, so a reply whose last period preceded its last exclamation cut early and silently dropped up to 40% of the speakable text. It now takes the latest sentence end of any kind, with a fail-on-old regression test.Tests
test_sanitize_reply.py, 40 checks, all passing. Covers oversized, empty, markdown-laden, ANSI, emoji, and identifier-preservation cases, plus proof thatbrain()sanitizes both the cli and bridge paths.test_brain_dispatch.py7/7 andtest_pipeline_bridge.py4/4 still pass.Deferred
#74 — a fenced block holding plain prose is dropped rather than unwrapped. Fixing it needs a prose-vs-code heuristic, which is larger than this issue scoped, and the current default is the safe side of the tradeoff.
wake-20260716T1705-46da79