Skip to content

Fix: watch was executing the flags it did not recognise - #65

Merged
0xLeif merged 1 commit into
mainfrom
leif/watch-flag-guard
Aug 18, 2026
Merged

Fix: watch was executing the flags it did not recognise#65
0xLeif merged 1 commit into
mainfrom
leif/watch-flag-guard

Conversation

@0xLeif

@0xLeif 0xLeif commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Three defects that surfaced during the last triage but were out of that PR's scope. Each was verified here before being planned.

watch was executing the flags it did not recognise

Anything flag-shaped that watch did not own stayed in the argv and became the command.

My first probe of this was worthless, and worth saying so: a non-tty run refuses on "stdin is not a TTY" before parsing anything, so the refusal was the TTY check and proved nothing about flags. Driven through a real PTY.spawn:

before   rune watch --timeout 5 -- echo hi   ->  exit 127, child never ran
after    rune watch --timeout 5 -- echo hi   ->  exit 1, "--timeout takes its value inline: --timeout=VALUE"
control  rune watch --timeout=5 -- echo hi   ->  exit 0, child ran (unchanged)

run has guarded this since it grew flags. watch never did, which made it the worse of the two — run at least says something. The guard now lives in Command.flag_error and is shared rather than copied: the two had already drifted once, with run growing the inline-value branch and watch having no guard to grow it in.

--grep ignored --since

filter was handed the sliced text and then called transcript.grep, which searched the whole transcript. A read from a cursor recorded after the first line still returned that line, and grep_matches counted it — so a caller paging with --since=<last cursor> got the entire history back on every page, under a count that looked like it had filtered.

--max-output did not bound the separate streams

A 200-byte budget returned 10,506 bytes across four fields, because only clean_output and raw_output went through apply_output_limit. Now 1,012. With neither bound set, the shape is byte-for-byte unchanged.

An adversarial pass found two merge-blockers in this work, and was right about both

The watch guard shipped with no test. Reverting all three guard files left the suite fully green — CI could not distinguish the fix from its absence, including its own headline case. That is the failure this repo cares most about, and I had walked straight into it. Six tests now cover it, including a drift guard and the case it must not lose: a child's own flags surviving.

Moving exports broke the spec contract. unknown_flag_error and INLINE_VALUE_ERROR moved from RunCommand to Command, leaving pty_runner.spec.md documenting things that no longer exist and six new exports documented nowhere. specsync coverage reported two hard errors; both are gone.

It also caught a false claim in my own comment: watch's VALUE_FLAGS said it "cannot drift from the parser" while --log is appended by hand, because its pattern is written inline in scan_head. Corrected to what is actually true.

One reported loss was checked and deliberately left: the guard covers the leading position only, so rune watch echo hi --log=/tmp/x still writes rune's own log to the child's path. Measured byte-identical before and after this change, so it is pre-existing — recorded in watch.spec.md invariant 20 rather than fixed here.

Test Plan

  • 584 examples, 0 failures; rubocop clean; docs-check green
  • specsync check — 4/4 specs, 31/31 files, 7181/7181 LOC (100%)
  • Controls: watch guard reverted → 4 of 33 fail; grep reverted → 1 of 2; stream bounds reverted → 1 of 3
  • watch re-verified through a real controlling terminal, not a non-tty stub

One API change worth naming

RunCommand::INLINE_VALUE_ERROR still resolves through inheritance but now carries a second placeholder, so format(..., name:) alone raises KeyError. It was introduced days ago and has never appeared in a tagged release, so the exposure is a caller who took it from main.

🤖 Generated with Claude Code

https://claude.ai/code/session_018rf59AtQmJcodUJ6vXDZNY

Three defects that surfaced during the last triage but were out of that PR's
scope. Each verified before being planned.

watch had no unknown-flag guard: anything flag-shaped it did not recognise
stayed in the argv and became the command. My first probe of this was worthless
— a non-tty run refuses on "stdin is not a TTY" before parsing anything, so the
refusal was the TTY check and proved nothing. Driven through a real PTY.spawn,
`rune watch --timeout 5 -- echo hi` exited 127 with the child never running.
run has guarded this since it grew flags; watch never did, which made it the
worse of the two, because run at least says something.

The guard now lives in Command.flag_error and is shared rather than copied: the
two had already drifted once, with run growing the inline-value branch and watch
having no guard to grow it in.

--grep ignored --since. `filter` was handed the sliced text and then called
transcript.grep, which searched the whole transcript, so a read from a cursor
recorded after the first line still returned that line and grep_matches counted
it. A caller paging with --since=<last cursor> got the whole history on every
page under a count that looked like it had filtered.

--max-output did not bound clean_stdout/clean_stderr at all: a 200-byte budget
returned 10,506 bytes across four fields. Now 1,012.

An adversarial pass found two merge-blockers in this work and was right about
both. First, the watch guard shipped with NO test — reverting all three guard
files left the suite fully green, so CI could not tell the fix from its absence,
including its own headline case. Six tests now cover it, including a drift guard
and the case it must not lose, a child's own flags surviving. Second, moving
unknown_flag_error and INLINE_VALUE_ERROR out of RunCommand left the pty_runner
spec documenting them and added six exports nothing documented; specsync
reported two hard errors, both now gone.

It also caught a false claim in my own comment: watch's VALUE_FLAGS said it
"cannot drift from the parser" while --log is appended by hand, because its
pattern is inline in scan_head. Corrected to what is true.

One reported loss was checked and deliberately left: the guard covers the
leading position only, so `rune watch echo hi --log=/tmp/x` still writes rune's
own log to the child's path. Measured byte-identical before and after, so it is
pre-existing and recorded in invariant 20 rather than fixed here.

Note an API change: RunCommand::INLINE_VALUE_ERROR still resolves through
inheritance but now carries a second placeholder, so format(..., name:) alone
raises KeyError. It has never appeared in a tagged release.

584 examples, 0 failures. Controls: watch guard reverted fails 4 of 33, grep
reverted fails 1 of 2, stream bounds reverted fails 1 of 3.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rf59AtQmJcodUJ6vXDZNY
@0xLeif
0xLeif merged commit 0ae379d into main Aug 18, 2026
10 checks passed
@0xLeif
0xLeif deleted the leif/watch-flag-guard branch August 18, 2026 17:19
0xLeif added a commit that referenced this pull request Aug 18, 2026
CodeQL flagged .sub(/\n+\z/, '') in Screen#to_s as a polynomial-regex risk on
uncontrolled input (PR #66 review).

Measured rather than trusted: a single quantifier anchored at \z has no
backtracking ambiguity to exploit, and both a trailing-newline run and an
adversarial no-match input scale linearly to 800,000 bytes. The alert looks like
a false positive for this specific pattern.

Fixed anyway, and with the better fix regardless of whether the alert was real:
to_s already holds the rendered rows as an array before joining them, so popping
trailing empty rows off that array gives the same result without a regex on the
render path at all. Verified equivalent on a 5-row scrolling grid and on the
all-blank and single-line edge cases.

594 examples, 0 failures.

This also carries the branch through: PR #65 was squash-merged, which gave main
a new commit SHA for content that used to be ac38dba. The branch had been
rebased onto the new main, which orphaned CHG-0070's recorded verification
commit and left specsync check refusing both reopen (content unchanged, so not
stale by its own check) and outright acceptance (recorded commit unreachable).
Undone by merging origin/main into the pre-rebase commit instead of rebasing,
which keeps the accepted commit reachable in history — the standard fix for
exactly the trap this repo's own CLAUDE.md warns about with --amend, encountered
here via rebase instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rf59AtQmJcodUJ6vXDZNY
0xLeif added a commit that referenced this pull request Aug 18, 2026
…wn in (#66)

* Fix: watch was executing the flags it did not recognise

Three defects that surfaced during the last triage but were out of that PR's
scope. Each verified before being planned.

watch had no unknown-flag guard: anything flag-shaped it did not recognise
stayed in the argv and became the command. My first probe of this was worthless
— a non-tty run refuses on "stdin is not a TTY" before parsing anything, so the
refusal was the TTY check and proved nothing. Driven through a real PTY.spawn,
`rune watch --timeout 5 -- echo hi` exited 127 with the child never running.
run has guarded this since it grew flags; watch never did, which made it the
worse of the two, because run at least says something.

The guard now lives in Command.flag_error and is shared rather than copied: the
two had already drifted once, with run growing the inline-value branch and watch
having no guard to grow it in.

--grep ignored --since. `filter` was handed the sliced text and then called
transcript.grep, which searched the whole transcript, so a read from a cursor
recorded after the first line still returned that line and grep_matches counted
it. A caller paging with --since=<last cursor> got the whole history on every
page under a count that looked like it had filtered.

--max-output did not bound clean_stdout/clean_stderr at all: a 200-byte budget
returned 10,506 bytes across four fields. Now 1,012.

An adversarial pass found two merge-blockers in this work and was right about
both. First, the watch guard shipped with NO test — reverting all three guard
files left the suite fully green, so CI could not tell the fix from its absence,
including its own headline case. Six tests now cover it, including a drift guard
and the case it must not lose, a child's own flags surviving. Second, moving
unknown_flag_error and INLINE_VALUE_ERROR out of RunCommand left the pty_runner
spec documenting them and added six exports nothing documented; specsync
reported two hard errors, both now gone.

It also caught a false claim in my own comment: watch's VALUE_FLAGS said it
"cannot drift from the parser" while --log is appended by hand, because its
pattern is inline in scan_head. Corrected to what is true.

One reported loss was checked and deliberately left: the guard covers the
leading position only, so `rune watch echo hi --log=/tmp/x` still writes rune's
own log to the child's path. Measured byte-identical before and after, so it is
pre-existing and recorded in invariant 20 rather than fixed here.

Note an API change: RunCommand::INLINE_VALUE_ERROR still resolves through
inheritance but now carries a second placeholder, so format(..., name:) alone
raises KeyError. It has never appeared in a tagged release.

584 examples, 0 failures. Controls: watch guard reverted fails 4 of 33, grep
reverted fails 1 of 2, stream bounds reverted fails 1 of 3.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rf59AtQmJcodUJ6vXDZNY

* Add: a cell model, so a wide glyph occupies the two columns it is drawn in

The last of the five renderer gaps, and a correction of my own conclusion about
it.

A cell model was built once before and reverted, and the spec recorded that it
had been "measured worse than the gap". That was wrong. The A/B compared two
working trees and misattributed which output came from which side. Re-measured
against three explicit revisions on the same 56,928-byte grok capture that had
emitted a CJK table:

  cc8bb3c  (one column)   "東h京   Tokyo"    "大 阪   Osaka"
  ad76e22  (one column)   "東h京   Tokyo"    "大 阪   Osaka"
  cell model             "東京  Tokyo"     "大阪  Osaka"

The one-column model corrupts real agent output and always did: an agent
positions its columns assuming two per CJK glyph, and a renderer counting one
puts every later write in the wrong place. I had it backwards, reverted a correct
fix, and wrote the mistake into the contract.

I also ruled out the four renderer fixes shipped since — alt screen, DECAWM, IRM,
charsets — by disabling each in turn on top of main. The corrupted rows are
byte-identical in all four cases. It is the column arithmetic.

Rows are now Arrays of cells: nil, a String of one graphic plus its combining
marks, or CONTINUATION for the right half of a wide glyph. That makes two things
true the String could not. A cell holds any number of characters without moving
the cells after it, which is what makes combining marks work at all — appending
a mark to a String row put every later index off by one and the next graphic
overwrote it. And the pair invariant can be restored after the fact, in one heal
pass, instead of being taught to twelve operations that each slice the row. The
first attempt tried the latter and lost.

pad/padded_line are gone: assigning past the end of an array fills with nil and a
nil cell renders blank. That padding was the mechanism by which a column index
became an index into text.

591 examples, 0 failures. Controls: forcing the width table to one column fails 4
of 136 parser examples; making heal a no-op fails 2.

Stacked on leif/watch-flag-guard so the change records do not collide.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rf59AtQmJcodUJ6vXDZNY

* Fix: drop the regex CodeQL flagged from the render path entirely

CodeQL flagged .sub(/\n+\z/, '') in Screen#to_s as a polynomial-regex risk on
uncontrolled input (PR #66 review).

Measured rather than trusted: a single quantifier anchored at \z has no
backtracking ambiguity to exploit, and both a trailing-newline run and an
adversarial no-match input scale linearly to 800,000 bytes. The alert looks like
a false positive for this specific pattern.

Fixed anyway, and with the better fix regardless of whether the alert was real:
to_s already holds the rendered rows as an array before joining them, so popping
trailing empty rows off that array gives the same result without a regex on the
render path at all. Verified equivalent on a 5-row scrolling grid and on the
all-blank and single-line edge cases.

594 examples, 0 failures.

This also carries the branch through: PR #65 was squash-merged, which gave main
a new commit SHA for content that used to be ac38dba. The branch had been
rebased onto the new main, which orphaned CHG-0070's recorded verification
commit and left specsync check refusing both reopen (content unchanged, so not
stale by its own check) and outright acceptance (recorded commit unreachable).
Undone by merging origin/main into the pre-rebase commit instead of rebasing,
which keeps the accepted commit reachable in history — the standard fix for
exactly the trap this repo's own CLAUDE.md warns about with --amend, encountered
here via rebase instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rf59AtQmJcodUJ6vXDZNY

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant