Skip to content

fix(parsers): keep task, heading, link, frontmatter and canvas parsing linear - #753

Open
aliasunder wants to merge 9 commits into
worktree-task-dependency-hangfrom
worktree-parser-linear-patterns
Open

aliasunder wants to merge 9 commits into
worktree-task-dependency-hangfrom
worktree-parser-linear-patterns

Conversation

@aliasunder

Copy link
Copy Markdown
Owner

Summary

Several Markdown parsers that run on every note when the index is built, and on every change, could backtrack or loop far past linear time on crafted text: a long run of [[ or (, a heading line of trailing spaces, a task line with long whitespace runs in its recurrence field, a properties block with tens of thousands of keys, or a canvas with thousands of groups. A note like that stalled the server for seconds to minutes. Each pattern now runs in linear time and matches exactly the text it matched before, so nothing Obsidian users see changes.

Stacked on #752, which fixes the task dependency-list pattern.

Changes

  • obsidian-markdown/tasks.ts, headings.ts: the task-line, recurrence and Dataview-field patterns and the closing-hash pattern rewritten in linear form; heading-boundary and task-heading attribution in one linear pass.
  • obsidian-markdown/links.ts, plaintext.ts, remark-ports/list.ts: wikilink, Markdown-link, Templater and plain-text stripping as single-pass delimiter scans behind the same functions, and a linear list-looseness check. The scans keep no per-character tables.
  • obsidian-markdown/frontmatter.ts: duplicate keys are found in one pass over every map, with the same refusal message, line and column as before for a document whose only errors are duplicates.
  • obsidian-markdown/canvas.ts: group ownership runs only within a fixed work budget (content nodes × groups + groups² up to 16,000,000), so canvases of up to about 2,000 groups keep their nesting and larger ones list every node, label and connection without it; nesting renders with an explicit stack.
  • ARCHITECTURE.md: parser work on note text must stay linear, proven by adversarial tests.

Verification

  • npm test: 7,726 passed, 2 skipped. npm run build, npm run lint (no errors), npx knip and Prettier pass.
  • Differential tests against a copy of each old pattern, over fixed-seed corpora (200,000 cases per pattern) plus every string in the existing tests and fixtures: no differences. A 300,000-document comparison of list and tag output against Obsidian 1.14.4 found no differences.
  • Restoring each old pattern makes its adversarial test miss its deadline or scaling bound.
  • A built image with a crafted note in the vault before boot answers /healthz in 1.4 s and finds the note in search in 1.5 s.

🤖 Generated with Claude Code

Scan trailing fields and heading boundaries without retrying suffixes. Preserve task metadata, descriptions and section ranges.

Implemented-By: codex · gpt-6.1-sol
Preserve link targets, offsets and display text with shared delimiter cursors. Scan list looseness without retrying whitespace suffixes.

Implemented-By: codex · gpt-6.1-sol
Preserve YAML duplicate refusals with a document-order walk. Bound canvas ownership work and render nested groups without recursive string construction.

Implemented-By: codex · gpt-6.1-sol
@aliasunder
aliasunder added this pull request to stack #754 November 8, 2026 01:12
Ship-Check: pr-review · gpt-6.1-sol
Comment thread ARCHITECTURE.md

- **YAML properties:** public-node traversal checks duplicate keys with separate node-identity and scalar-value Sets. Block/flow traversal order and source-token offsets preserve the first duplicate-only refusal; another syntax error may take precedence when both occur.
- **Canvas ownership:** nesting runs only when `content nodes × groups + groups²` is at most `16,000,000`, checked before ownership scans. About 2,000 groups and 2,000 content nodes keep nesting; larger work counts produce a flat rendition containing every valid node, group label and connection. Both renditions sort nodes in reading order, which costs O(n log n); nested rendering uses an explicit stack and appends each section once.
- **Canvas measurement context:** a crossed-layout ownership pass measured 3,648 ms at 128,000,000 checks, now above budget. Scaling that single sample gives `3,648 × 16,000,000 / 128,000,000 = 456 ms` (about 0.45 s) at the budget. This is extrapolation under the original host/load conditions; neither figure is a worst-case latency ceiling or a guarantee on another host. Parent-bounded tests measure at-budget crossed layouts and a bounded multiple-canvas run.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

State the canvas budget's rationale without the latency extrapolation
Low severity · conventions · medium confidence

The bullet records one historical measurement (3,648 ms at 128,000,000 checks) and an extrapolated 456 ms at the budget, then disclaims both; no test or script reproduces either figure, so they are investigation narrative rather than durable rationale and read as a latency spec.

Failure scenario: A later change to CANVAS_GROUP_WORK_BUDGET or to smallestContainingGroup leaves the 3,648 ms/456 ms figures describing code that no longer exists, and a reader treating 456 ms as the budget's cost has nothing in the repo to check it against.

Suggested fix
Keep the constraint, drop the anecdote: "**Canvas ownership:** nesting runs only when `content nodes × groups + groups²` is at most `16,000,000`, checked before any ownership scan, so ownership work stays bounded on adversarial canvases. Parent-bounded tests time at-budget crossed layouts and a multiple-canvas run."

umm-actually · deepseek/deepseek-v4.1-flash-1017

Comment on lines +719 to +726
expect(readParserComplexityReport(fixture)).toEqual({
fixture,
node: process.version,
checksPerCanvas: 16_000_000,
copies,
medianMs: expect.any(Number),
semantics: "exact",
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Give the canvas adversarial fixtures a timing ceiling
Low severity · tests · high confidence

The canvas adversaries assert only expect.any(Number) for the median and depend on execFileSync's 30 s SIGKILL deadline, so a large latency regression passes; the sibling adversaries in this PR (links, tasks, headings, list) assert sub-second ceilings and doubled-size ratios.

Failure scenario: A change that makes ownership scanning quadratic at the 16,000,000-check budget (e.g. dropping the budget gate's early return) raises the crossed-layout run to ~12 s; the child finishes below the 30 s watchdog and the test passes on expect.any(Number), even though ARCHITECTURE.md documents ~0.45 s at budget.

Suggested fix
Have `runFixture` throw on a ceiling (e.g. `if (medianMs > 1_000) throw new Error(`canvas exceeded ceiling ${medianMs}`)`) and, for `canvas-multiple`, on the per-copy ratio, so a latency regression fails long before the 30 s watchdog.

umm-actually · deepseek/deepseek-v4.1-flash-1017

Comment on lines +25 to +28
const closingSource = legacySource.replace(
"stripClosingHashes(matchedText)",
'matchedText.replace(/(?:^|[ \\t]+)#+[ \\t]*$/, "")',
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Guard the closing-hash oracle substitution in the heading test
Low severity · tests · high confidence

The legacy oracle is rebuilt by a bare String.replace; if the call site's text changes (rename, added .trim(), reworded call) the replace silently no-ops and the compiled oracle calls production's own stripClosingHashes, so the fixed-seed comparison checks closing-hash stripping against itself.

Failure scenario: Rename stripClosingHashes or change its call to stripClosingHashes(matchedText.trim()): the replace finds nothing, both parsers now use the production function, and a closing-hash regression (e.g. dropping the ^-anchored ### case) makes both sides agree, so the 10,000-document comparison still passes.

Suggested fix
Assert the splice happened before compiling, mirroring the existing guard and tasks-linear.test.ts: `if (closingSource === legacySource) throw new Error("closing-hash oracle anchor missing")`.

umm-actually · deepseek/deepseek-v4.1-flash-1017

Comment on lines +633 to +638
tasks.extractTasks("- [ ] x [dependsOn:: a]")
} finally {
execSpy.mockRestore()
}

const dependencyPatterns = observedPatterns.filter((pattern) =>
const dependencyPatterns = [...new Set(observedPatterns)].filter((pattern) =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Exercise the Dataview dependency pattern on input it can match
Low severity · tests · medium confidence

The added [dependsOn:: a] input makes the new FieldMatcher observable, but every comparison for it runs the inner pattern — and an oracle rebuilt from that same pattern's source — against whole lines that begin with - , so both sides are null in all cases and only the emoji pattern's ID-sequence is actually compared. A Dataview-side regression that leaves the substituted sequence intact cannot fail this test.

Failure scenario: Change fieldPattern's closer class/key handling (e.g. accept only ] instead of [)]): observedPatterns still yields both patterns and its legacy twin is rebuilt from the changed source, while every generated line (- [ ] x [dependsOn:: …], - [ ] x (dependsOn:: …),) and every extracted fixture line starts with -, so all comparisons stay null === null and the test passes while (dependsOn:: a) fields stop parsing.

Suggested fix
Compare the field matcher the way tasks-linear.test.ts does: extract `dataviewField` and assert `consumedMatch(matcher.exec(payload + `[dependsOn:: ${value}]`))` against the frozen legacy pattern applied to the same input, instead of `fieldPattern.exec(wholeLine)`.

umm-actually · deepseek/deepseek-v4.1-flash-1017

Comment on lines +195 to +196
contentNodes: readonly CanvasNode[],
groups: readonly CanvasNode[],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Take renderNestedNodes' two node lists as a named-params object
Low severity · conventions · high confidence

renderNestedNodes(contentNodes, groups) takes two adjacent readonly CanvasNode[] parameters, the shape AGENTS.md says to replace with a named-params object because a transposed call compiles silently; the swapped roles render every content node as a group heading and every group as card text.

Failure scenario: A later edit or reuse calls renderNestedNodes(groups, contentNodes): TypeScript accepts it, the rendition loses all group headings and nests card text as headings, and only the exact-output assertions in one test file catch it.

Suggested fix
Take named params: `const renderNestedNodes = (params: { contentNodes: readonly CanvasNode[]; groups: readonly CanvasNode[] }): string[] =>` and call `renderNestedNodes({ contentNodes, groups })`.

umm-actually · deepseek/deepseek-v4.1-flash-1017

@umm-actually

umm-actually Bot commented Nov 8, 2026

Copy link
Copy Markdown

Stop re-parsing the metadata tail from every signifier candidate
Medium severity · subtle bugs · high confidence

src/vault-mcp/obsidian-markdown/tasks.ts:1117 — beyond the diff's line ranges, in code the changes touch or depend on.

Pre-existing: findMetadataStart runs parseTaskMetadata on a fresh slice of the task body for every signifier candidate, so a line with many field-shaped emoji costs candidates × line length per call (an O(n) string copy plus a full strip pass each). Every vault_update_task and vault_create_task pays it through splitTaskLine, so a crafted long task line stalls the write path this PR otherwise made linear.

Failure scenario: vault_update_task targets a line like '- [ ] ' + '📅 '.repeat(25000) (~100 KB): findMetadataStart slices and fully re-parses the tail for each of the 25,000 📅 candidates — hundreds of megabytes of copying plus repeated strip passes — stalling the mutation call for seconds; the existing adversary fixtures exercise extractTasks and describeTaskLine but never this loop, so no test would catch a regression here.

Suggested fix
Strip fields right-to-left once and record where the strip stops to derive the metadata boundary, instead of re-running parseTaskMetadata on a fresh slice for every candidate; add an adversary fixture at a 10,000-signifier size to pin the bound.

umm-actually · z-ai/glm-5.3-flash

@umm-actually

umm-actually Bot commented Nov 8, 2026

Copy link
Copy Markdown

Thread fence state through the Kanban lane walk, not a per-card rescan
Medium severity · subtle bugs · high confidence

src/vault-mcp/vault-operations/task-mutations.ts:483 — beyond the diff's line ranges, in code the changes touch or depend on.

Pre-existing: every card in a lane walk pays a fence/comment rescan from line 0 (isExcludedFromLane → isInsideFenceOrComment, which restarts its scan at line 0 on each call), so position-aware writes cost cards × note length. The PR's new linear-parser guarantee does not cover this write path, so a crafted board still stalls task mutations even though extraction is now linear.

Failure scenario: A vault holds a kanban-plugin note whose single lane holds ~10,000 cards; vault_update_task with position: 2 (same-lane reorder) or vault_create_task with an integer position walks the lane, and each of the 10,000 cards triggers a from-line-0 fence/comment rescan — the call stalls for seconds to tens of seconds on input the PR's other adversaries show should parse in milliseconds.

Suggested fix
Thread the fence/comment state once across the lane walk — findTaskLineByBlockId in this same file already does exactly that — or precompute a per-line inside-fence/comment array in one pass before the loop and index into it.

umm-actually · z-ai/glm-5.3-flash

@umm-actually

umm-actually Bot commented Nov 8, 2026

Copy link
Copy Markdown

Qualify the Files section's canvas claim with the nesting budget
Low severity · correctness · medium confidence

ARCHITECTURE.md:346 — beyond the diff's line ranges, in code the changes touch or depend on.

ARCHITECTURE.md's Files section still describes canvas linearization as always applying group membership by spatial rect containment with innermost-group resolution, but the new 16,000,000 work-unit budget makes linearizeCanvas render a flat rendition — every group heading at H2 and every node in plain reading order, with no card placed under its owning group. The new Note-content parsers section documents the bound, yet the Files bullet, which is where a reader learns what vault_read_file returns for a canvas, states the capability unconditionally.

Failure scenario: A user syncs a canvas with 2,000 groups and 6,001 content nodes (2,000 × 8,001 = 16,002,000 work units, above the 16,000,000 budget) and asks an agent to summarize it via vault_read_file. Guided by the Files section, the agent expects each card nested under its owning group's heading; the rendition instead lists all groups at H2 and all cards in reading order, so the agent reports the wrong card-to-group membership.

Suggested fix
Qualify the Files → Canvas bullet with the bound, e.g. 'group membership by spatial rect containment (innermost group wins; equal rects tiebreak deterministically by id) within the parser work budget — larger canvases list every node, group label and connection without nesting (see Note-content parsers)'.

umm-actually · z-ai/glm-5.3-flash

@umm-actually

umm-actually Bot commented Nov 8, 2026

Copy link
Copy Markdown

umm-actually reviewed at f82e474

8 new finding(s) posted (8 tracked finding(s) across all runs).

Context notes
  • Priority docs already in context: ARCHITECTURE.md
  • Priority docs not included: deploy/railway/README.md, deploy/render/README.md, deploy/local/README.md, deploy/remote/README.md (missing, unreadable, or over budget)

umm-actually · z-ai/glm-5.3-flash, deepseek/deepseek-v4.1-flash-1017

aliasunder and others added 5 commits November 7, 2026 20:33
Make delimiter reuse, YAML error order, list source positions and task metadata handling explicit without changing parser behavior.

Ship-Check: code-quality · gpt-6.1-sol
Ship-Check: test-audit · gpt-6.1-sol
Describe the existing flat rendition above the ownership-work budget.

Ship-Check: bug-check · gpt-6.1-sol
…r-patterns

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Key ownership buckets by node identity so repeated group IDs cannot replay descendants. Preserve nesting for unique IDs and render every node once for duplicate IDs. Update duplicate-ID expectations, add a bounded 28-group regression with complete output checks, and document the rendering guarantee.

Implemented-By: codex · gpt-6.1-sol
@aliasunder aliasunder closed this Nov 8, 2026
@aliasunder aliasunder reopened this Nov 8, 2026
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