Skip to content

fix(help): treat an HTML comment inside a code block as content - #3908

Merged
max-sixty merged 2 commits into
mainfrom
nightly/clean-32816905979
Aug 25, 2026
Merged

fix(help): treat an HTML comment inside a code block as content#3908
max-sixty merged 2 commits into
mainfrom
nightly/clean-32816905979

Conversation

@worktrunk-bot

Copy link
Copy Markdown
Collaborator

The HTML-comment branch in render_markdown ran before the code-fence check, so a fenced line that happened to be an HTML comment was silently dropped instead of rendered as part of the block. This matters because render_markdown_flush renders arbitrary forge markdown — the picker's pr description pane and its per-comment bodies — where a fenced HTML sample is ordinary content, not a docs expansion marker.

The fix guards the branch with !in_code_block. Verified by reverting just the guard: the new test fails with the comment line missing from the output, and passes with it.

Nightly sweep finding (bucket 26/28), not a reported issue.

Mechanism and verification

The loop in src/md_help.rs checked the comment prefix at the top of each iteration, ahead of both the fence handling and the if in_code_block { code_block_lines.push(line) } collector — so the continue consumed the line before it could ever reach the block. The table-row branch is already ordered after the collector, so it was never affected.

Two visible effects:

  • The comment line vanishes from the rendered code block.
  • A fenced <!-- wt list … --> set chop_next_block, arming the chop-to-width path for whatever block came next — a marker leaking out of the fence it was written inside.

Confirming the test is a real regression test — with only the !in_code_block guard reverted:

test md_help::tests::test_html_comment_inside_a_code_block_is_content ... FAILED
fenced comment should render as content: "\u{1b}[2m<p>body</p>\u{1b}[0m\n"

The comment line is simply gone from the output; only <p>body</p> survives.

No snapshots move. A scan of every src/**/*.rs (after_long_help), docs/src/content/docs/**/*.md, and skills/**/*.md for an HTML comment inside a fence returns zero hits, so nothing currently authored in the repo took the old path.

Local verification on this branch:

  • cargo test --bins — 980 passed, 0 failed
  • cargo test --test integration test_help — 47 passed, 0 failed (help snapshots unchanged)
  • cargo fmt --check clean, cargo clippy --bins --all-features clean

The comment branch in `render_markdown` ran before the fence check, so a
fenced line that happened to be an HTML comment was dropped from the block
instead of rendered. `render_markdown_flush` renders arbitrary forge
markdown in the picker's `pr` and comments panes, where a fenced HTML
sample is ordinary content — and a fenced `<!-- wt list -->` additionally
armed the chop marker for whatever block followed.

Guard the branch with `!in_code_block`. No help or docs source has a
comment inside a fence, so no snapshot moves.
@worktrunk-bot worktrunk-bot added the nightly-cleanup Issues found by nightly code quality sweep label Aug 25, 2026

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review. The production change is right: the branch ordering was the bug, render_markdown_flush really does feed the picker's pr and comments panes with arbitrary forge markdown, and I re-ran the repo scan for a fenced HTML comment across src/**/*.rs, docs/src/content/docs/**/*.md and skills/**/*.md — zero hits, so no snapshot moves.

One finding, on the test. tests/CLAUDE.mdInline snapshots over multi-assert says "When a test checks formatted output, use insta::assert_snapshot! with an inline snapshot instead of multiple assert!(x.contains(...)) calls", and the new test spot-checks rendered output with three contains assertions where every adjacent test in the module (test_render_markdown_in_help_code_block, test_render_markdown_in_help_html_comment) uses assert_snapshot!. That costs something concrete here rather than being a style point: contains proves the comment line survived but says nothing about how it renders, which is exactly what this change decides. A snapshot pins that the fenced comment picks up the dim style in the flush path and the gutter bar in the gutter path, plus its position relative to the block that follows — a regression that renders the line undimmed or outside the gutter still passes the contains version and fails the snapshot.

Pushing the conversion rather than leaving it as a suggestion, since this is a bot-authored PR with no human author to apply it. cargo insta test --accept --bins --test-runner nextest filled the two non-empty snapshots; cargo test --bins is 980 passed / 0 failed, cargo fmt --check and cargo clippy --bins --all-features clean.

Not approving: GitHub rejects self-approval on a PR this account authored.

tests/CLAUDE.md asks for insta inline snapshots over multiple
contains-assertions when a test checks formatted output. The
contains version proved the comment line survived but not how it
rendered, which is what this fix decides -- the snapshots pin the
dim style in the flush path and the gutter bar in the gutter path.
@max-sixty
max-sixty merged commit 496c571 into main Aug 25, 2026
38 checks passed
@max-sixty
max-sixty deleted the nightly/clean-32816905979 branch August 25, 2026 16:20
max-sixty pushed a commit that referenced this pull request Aug 26, 2026
…it (#3920)

`tests/CLAUDE.md` tells a test that checks formatted output to use an
inline snapshot, and says nothing about the fact that a snapshot of
*styled* output also bakes the `syntax-highlighting` feature into the
expectation. Following that rule on highlighted output is what took
`main`'s nightly red overnight: #3908's second commit converted three
`contains` checks on `render_markdown_in_help` to inline snapshots, one
of them covering a trailing `console` block, merged green, and
`feature-powerset` failed on the next cron ([run
32935569443](https://github.com/max-sixty/worktrunk/actions/runs/32935569443)).
This adds the caveat to the section that gave the instruction, with the
per-configuration shape #3916 uses to fix that test. Docs only —
`pre-commit run --files tests/CLAUDE.md` passes.

<details><summary>Why here, and the full chain</summary>

## The chain

| When | What |
|---|---|
| 08-25 06:36Z | #3908 opened — `fix(help): treat an HTML comment inside
a code block as content`, test asserted with three `contains` calls |
| 08-25 06:45Z | Self-review cites `tests/CLAUDE.md` → **Inline
snapshots over multi-assert** and pushes `8a141861a`, converting them to
inline snapshots. One captures the trailing `console` fence's rendering,
which under `syntax-highlighting` is `ESC[2mESC[0mESC[2mESC[34mwtESC[0m`
|
| 08-25 16:20Z | Merged |
| 08-26 03:57Z | #3916 opened, having caught the failure on an unrelated
Dependabot PR |
| 08-26 05:49Z | `nightly` cron on `e4238e83`: `feature-powerset` red —
`test md_help::tests::test_html_comment_inside_a_code_block_is_content
... FAILED` |
| 08-26 06:29Z | Diagnosed on #3917; #3918 filed for the same run's
unrelated transient |

The self-review's reasoning was right on its own terms — the `contains`
version proves the comment line survived but says nothing about *how* it
renders, which is what #3908 decides. What it missed is the axis on
which "how it renders" is not a constant.

## Why the required checks can't catch it

`syntax-highlighting` is a default feature, so `test
(linux|macos|windows)`, `fast-checks` and `code-coverage` all build with
it on. `ci`'s `feature-check` job does `cargo check --bin wt
--no-default-features --features cli` on every PR, but `cargo check`
never compiles `#[cfg(test)]` code, so the snapshot is never built
there. The only job that *runs the tests* on that combination is
`feature-powerset` in `nightly`, and `nightly`'s gate runs on a PR only
when the diff touches Cargo/toolchain/nix paths or the PR carries the
`nightly` label — so a `src/`-only PR never sees it. The red is
structurally deferred to the next cron on `main`, i.e. after the merge.

`nightly.yaml`'s own job comment already names the failure mode — "a
snapshot baking feature-dependent output, only surfaces when tests are
built and run per combo" — but that text is in the workflow, not in the
file an author of a unit test reads.

## Why the caveat and not a gate

Gating the whole assertion on `#[cfg(feature = "syntax-highlighting")]`
would compile the test out of the one combination that catches this
class of bug. The pair form keeps the assertion on both sides and is
what the repo already does elsewhere (`src/styling/format.rs` uses
`#[cfg(all(test, feature = "syntax-highlighting"))]` for a module whose
subject *is* the highlighting; here the colouring is incidental, so the
test stays and only the expectation splits).

The last paragraph — apply the `nightly` label to a PR fixing a
`feature-powerset` failure — is the other half of the same incident:
#3916's own `feature-powerset` was skipped until the label went on at
06:32Z, after which it ran green on `2b734bb8`. Nightly's header comment
calls the label "the iteration knob for fixes targeting nightly-only
failures"; nothing under `tests/` said so.

## Gate assessment

- **Evidence level**: Critical by the table's own wording — merged code
took `main` red. 1 occurrence, no prior sighting of this shape in the
tracking log
([#3691](#3691)). Blast
radius was small and it was self-caught within hours, which is why the
proposed change is two paragraphs rather than a process.
- **Structural**: yes. Any author following the snapshot rule on styled
output, reading only that section, gets no warning; the information
lives in `nightly.yaml` and `Cargo.toml`, neither of which a unit-test
author has reason to open.
- **Cost class**: wrong outward action — a red default branch, not
wasted compute.
- **Change type**: targeted fix (a missing caveat on an existing rule),
Gate 2's normal bar.

</details>

---------

Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nightly-cleanup Issues found by nightly code quality sweep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants