Skip to content

fix: remove stray extra '>' when buffering note open tags#1

Merged
randomsnowflake merged 1 commit into
masterfrom
bug-audit-2026-04-22
Apr 23, 2026
Merged

fix: remove stray extra '>' when buffering note open tags#1
randomsnowflake merged 1 commit into
masterfrom
bug-audit-2026-04-22

Conversation

@randomsnowflake

Copy link
Copy Markdown
Owner

Summary

Bug audit of recent history (full repo is only 8 commits, so I reviewed everything). Found and fixed one clear, user-visible bug.

NoteProcessor.appendOpenTag emits malformed tags into note buffers

src/parser/processors/NoteProcessor.ts had a trailing literal > after the selfClosing ternary in the template literal:

this.state.noteBuffer += `<${tagName}${attrPairs ? ` ${attrPairs}` : ""}${selfClosing ? "/>" : ">"}>`;
//                                                                                            ^ stray

Every captured open tag produced a doubled closing bracket:

  • selfClosing=false produced <b foo="bar">> instead of <b foo="bar">
  • selfClosing=true produced <br/>> instead of <br/>

Because the SAX parser always feeds nested-within-note elements through this method (SaxOmniFocusParser.ts line ~194), every task/project note that contains markup ends up stored with these stray > characters. HTMLCleaner strips tags with /<[^>]+>/g, which consumes the first > but leaves the stray second one behind, so the fix is visible to end users in the rendered ASCII chart and in the JSON output (raw note).

I verified the bug with a one-off script before fixing, then strengthened the existing NoteProcessor coverage test in tests/additional-coverage.test.ts so the buffer format is now asserted directly (previously the test only called the methods and checked flags).

What else I looked at and left alone

  • getItemStatus in src/filter/statusFilter.ts has a dead-code branch that always returns "paused" whether the blocking context is dropped or paused. Looks like a latent bug, but the existing test explicitly pins the current behavior (tag-dropped -> "paused"), so "fixing" it would be a semantics change, not a bug fix. Flagging here rather than changing.
  • EntityDeduplicator.selectTasksToKeep drops entire recurring groups when every instance is completed. Plausibly intentional and the class is not exported to library consumers, so I left it.
  • parsePatchDescriptor does a 2-way destructure on fileName.split("="), which is lossy if a filename ever contains more than one =. OmniFocus's format does not produce such names, so not actionable today.
  • No shell execution, no AppleScript/osascript paths, no path-traversal exposure from readdir-derived names, no obvious missing-await or unhandled-rejection issues.

Test plan

  • npm run check (lint + typecheck + 35 vitest tests + build) passes
  • New assertions in tests/additional-coverage.test.ts fail against the old template literal and pass against the fix

NoteProcessor.appendOpenTag's template literal had a trailing '>'
after the selfClosing ternary, so every captured open tag emitted
a doubled closing bracket: `<b foo="bar">>` or `<br/>>`. Notes
stored by the SAX parser accumulated these malformed tags, and the
HTML cleaner's tag-strip regex left stray '>' characters in user-
visible output.

Also strengthen the existing NoteProcessor coverage test to assert
the exact buffer content for both selfClosing=true and the regular
open/close flow so the regression cannot reappear silently.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@randomsnowflake randomsnowflake merged commit ed7d1a7 into master Apr 23, 2026
5 checks passed
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