Skip to content

fix(checkbox-toggle): Make checkbox line numbers come from the markdown parser#134

Closed
bezhermoso wants to merge 4 commits into
block:mainfrom
bezhermoso:bezhermoso/checkbox-sourcepos-pairing
Closed

fix(checkbox-toggle): Make checkbox line numbers come from the markdown parser#134
bezhermoso wants to merge 4 commits into
block:mainfrom
bezhermoso:bezhermoso/checkbox-sourcepos-pairing

Conversation

@bezhermoso

Copy link
Copy Markdown
Contributor

Stacked on #133 — review only the last two commits (Derive checkbox line numbers from Commonmarker sourcepos and the specs commit) until #133 merges, after which this PR will show just its own diff.

What's new

Checkbox line numbers now come from the markdown parser itself instead of a parallel scanner. Every rendered checkbox is wired to the exact source line the parser says it came from, and a checkbox only becomes interactive when its source line is something the toggle endpoint will actually accept — anything else stays a plain, disabled checkbox.

Rationale

The plan renderer previously ran two independent interpretations of the same document: the markdown parser decided which list items render as checkboxes, while a separate hand-rolled scanner (a regex with naive code-fence tracking) decided which source lines are task lines. The two lists were then paired up by position — third checkbox gets the third scanned line.

Those two interpretations disagree on a surprising number of real-world constructs. The scanner counts task-shaped lines the parser never renders (inside indented code fences, ~~~ markers nested in backtick fences, mismatched fence lengths, 4-space indented code, raw HTML blocks) and misses items the parser does render (ordered-list tasks, blockquoted tasks, empty tasks). One disagreement anywhere shifts the pairing for every checkbox after it, so a click could silently edit a different line than the one the user toggled — including lines inside code blocks. And because the wrong line number and the wrong text arrive together as a self-consistent pair, the server-side safety check introduced in #133 validates them against each other and cannot detect the mix-up.

This change removes the second interpretation entirely. The parser now reports the source position of each list item alongside the rendered HTML, so the single authority that decides "this renders as a checkbox" also supplies "and it came from this line." Disagreement is no longer possible by construction. The position metadata is consumed during rendering and stripped from the final output.

Two smaller consequences:

  • Constructs the parser renders as checkboxes but the toggle endpoint won't accept (ordered-list tasks, blockquoted tasks) previously grabbed a neighboring task's line and text; they now simply stay disabled — client and server agree on what is toggleable because they now share one task-line pattern.
  • The renderer and the toggle endpoint previously each had their own copy of that pattern; they now reference a single shared definition, so they can't drift apart.

Testing

Beyond the existing checkbox specs (which pass unchanged), this adds an agreement suite that pins the invariant the whole toggle flow rests on: for every interactive checkbox, the text at its claimed line number must equal its claimed text, and the position resolver must accept that pair. The suite covers each divergent construct individually plus a gauntlet document combining fences, ordered/blockquoted tasks, HTML blocks, and duplicate task lines.

🤖 Generated with Claude Code

bezhermoso and others added 4 commits July 9, 2026 13:42
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bezhermoso bezhermoso changed the title Make checkbox line numbers come from the markdown parser fix(checkbox-toggle): Make checkbox line numbers come from the markdown parser Jul 10, 2026
HamptonMakes added a commit that referenced this pull request Jul 16, 2026
Checkbox toggles previously located their target by searching the whole
document for the task line's text, so duplicate task lines (or a task
whose text prefixes another) made the toggle fail and revert. Worse, the
renderer paired rendered checkboxes to source lines by index using a
hand-rolled scanner that disagrees with Commonmarker on real constructs
(indented fences, ~~~ inside backtick fences, ordered/blockquoted
tasks), so a click could silently edit the wrong line.

- Renderer: derive each checkbox's source line from Commonmarker's
  sourcepos metadata, making the parser the single authority on both
  "renders as a checkbox" and "came from this line". Checkboxes whose
  source line the toggle endpoint would reject stay disabled. The shared
  TASK_LINE_PATTERN keeps renderer and endpoint from drifting apart.
- Toggle endpoint: accept an optional line param, verify the line's text
  equals old_text (422 loudly on any drift — line and text must agree),
  then map the pair to an occurrence ordinal and apply a plain
  replace_exact. The public operations API is unchanged: no line-based
  addressing is exposed to agents.
- Document the existing replace_section operation in agent
  instructions as the semantic way to scope edits to one section.

The sourcepos technique and the loud-failure design are from Bez
Hermoso's PRs #133/#134; this lands them without adding a public
line-range qualifier to the operations API.

Co-Authored-By: Bez Hermoso <bezalelhermoso@gmail.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HamptonMakes added a commit that referenced this pull request Jul 16, 2026
…#138)

Checkbox toggles previously located their target by searching the whole
document for the task line's text, so duplicate task lines (or a task
whose text prefixes another) made the toggle fail and revert. Worse, the
renderer paired rendered checkboxes to source lines by index using a
hand-rolled scanner that disagrees with Commonmarker on real constructs
(indented fences, ~~~ inside backtick fences, ordered/blockquoted
tasks), so a click could silently edit the wrong line.

- Renderer: derive each checkbox's source line from Commonmarker's
  sourcepos metadata, making the parser the single authority on both
  "renders as a checkbox" and "came from this line". Checkboxes whose
  source line the toggle endpoint would reject stay disabled. The shared
  TASK_LINE_PATTERN keeps renderer and endpoint from drifting apart.
- Toggle endpoint: accept an optional line param, verify the line's text
  equals old_text (422 loudly on any drift — line and text must agree),
  then map the pair to an occurrence ordinal and apply a plain
  replace_exact. The public operations API is unchanged: no line-based
  addressing is exposed to agents.
- Document the existing replace_section operation in agent
  instructions as the semantic way to scope edits to one section.

The sourcepos technique and the loud-failure design are from Bez
Hermoso's PRs #133/#134; this lands them without adding a public
line-range qualifier to the operations API.

Co-authored-by: Bez Hermoso <bezalelhermoso@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@HamptonMakes

Copy link
Copy Markdown
Collaborator

Bez, this was a fantastic catch. Making Commonmarker the single authority for both “this renders as a checkbox” and “this came from this source line” is absolutely the right architecture. Your examples showed that the old parallel scanner was not merely fragile—it could drift and silently target the wrong line, which is much more serious than the duplicate-text failure we started with.

We carried this sourcepos approach into #138, together with the line+text verification insight from #133, and that combined solution is now merged on main. The final shape differs slightly because we kept line addressing private to the checkbox flow rather than adding it to the public operations API, but the parser-driven targeting here is foundational to what shipped.

I’m closing this as superseded by #138. You are credited as a co-author on the merged commit. Thank you for digging deeply enough to find the real invariant and for proposing such a strong fix—the final implementation is safer because of your work. 🙏

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.

2 participants