Stop silently destroying properties; give properties/callouts a real section_id (#90) - #97
Conversation
…real section_id (#90) `properties.section` stored heading text, and UNIQUE(page_id, name, section) plus INSERT OR IGNORE dropped any property whose page repeated a heading. The shipped v0.11.0-rc.97 DB holds 4,416 of 4,581 parsed properties and nothing logged the gap — the extractor's own "Properties:" line counted attempted inserts, so it reported rows the DB did not contain. Measuring every candidate key against the whole corpus showed the obvious fix is not enough. Re-keying on section_id still destroys 87 rows, because the manual documents one property name several times within a single section (dot1x defines `interface` twice under "Server" — once for the server table, once for the client one). Section is not an identity here at all; SQLite in fact refuses to build the index on real data. Only removing the constraint reaches zero loss: today (page,name,section-text) 165 dropped, 141 distinct issue (page,name,section_id) 87 dropped, 74 distinct hybrid (page,name,section_id,text) 51 dropped, 50 distinct none 0 So the constraint is removed rather than re-keyed, and extractors now assert parsed == stored (V-extractor-no-silent-drops) so a future drop fails the build instead of shipping. Verified by reintroducing the old code path: it reproduces the 165-row loss exactly and now exits 1. Attribution: properties/callouts gain section_id, correlated by line range in the pass all three parsers already share. A property under an h4–h6 folds to its enclosing h1–h3 section, so `sections` stays the retrieval unit and get_page is unchanged; `section` keeps the raw h4 text, so the finer heading stays recoverable and the decision reversible. Property attribution goes from 72% resolvable to 99.7%; callouts had none at all and now resolve 89%. Remaining NULLs are genuinely above any heading (page-level warnings, properties under a title-duplicate H1) and stay NULL rather than being forced into a section. Schema v9. Migration rebuilds properties in place, preserving rowids so the external-content FTS index stays valid; verified against the real rc.97 artifact (4,416 rows preserved, integrity_check ok, foreign_key_check clean). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rt (#90) Storing every property surfaces a fact the old UNIQUE constraint hid by deleting it: 111 name/section groups (276 rows) are indistinguishable by `section`, because it holds heading TEXT and repeats. lookup_property(name="name") on PPP AAA now returns three rows all reading "Properties" — the profile name, the login name, and the active-user name — with nothing to tell them apart. That ambiguity is created by the previous commit, so this finishes it rather than deferring: without the anchor, dropping the constraint would leave lookup_property measurably worse (three confusing rows instead of one wrong-but-clean one). lookupProperty and searchAll's related.properties now carry section_anchor (the section's anchor_id: properties, properties-1, properties-2), unique within a page and feedable straight back into routeros_get_page(page, section=...) to read that exact fragment. Verified round-trip against the real corpus: each of ppp-aaa's three `name` rows resolves to its own section. Null when a property sits above any heading — the key is always present so consumers can branch on it. routeros_lookup_property's description now teaches the workflow, per the mcp-tool-descriptions rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSchema v9 adds section attribution for properties and callouts. The extractor resolves rows by source line, persists section IDs, and fails on count mismatches. Lookup and search results now expose section anchors for repeated property names. ChangesSection-aware extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Docusaurus
participant Extractor
participant SQLite
participant LookupTool
Docusaurus->>Extractor: provide Markdown pages
Extractor->>SQLite: store sections, properties, and callouts with section_id
Extractor->>SQLite: compare parsed and stored row counts
LookupTool->>SQLite: query property and section anchor
SQLite-->>LookupTool: return section_anchor
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing data integrity bug in the Docusaurus extractor + SQLite schema where properties rows were silently dropped due to UNIQUE(page_id, name, section) combined with INSERT OR IGNORE. It removes the unsatisfiable uniqueness constraint, adds real section attribution (section_id) for both properties and callouts, and introduces a hard guard to fail extraction when parsed rows don’t match what was actually stored.
Changes:
- Schema v9: drop the
propertiesuniqueness constraint, addproperties.section_id+callouts.section_id, and migrate existing v8 DBs while preserving FTS rowids. - Extraction: attribute properties/callouts to h1–h3
sectionsvia line-range mapping (folding h4–h6 to the nearest h1–h3 ancestor), and assert parsed == stored (V-extractor-no-silent-drops). - Query/tool output: return
section_anchorto disambiguate same-named properties within a page and enable direct round-trip intorouteros_get_page(..., section=...).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| VALIDATION.md | Adds V-extractor-no-silent-drops to make “parsed vs stored” divergence a build-breaking invariant. |
| src/query.ts | Adds section_anchor to property lookup rows and to related.properties so same-named properties are distinguishable. |
| src/query.test.ts | Extends fixtures + tests to validate same-page disambiguation via section_anchor and round-trip into getPage(section=...). |
| src/paths.ts | Bumps SCHEMA_VERSION to 9 and documents the v9 migration intent. |
| src/mcp.ts | Updates routeros_lookup_property description/workflow to incorporate section_anchor as the disambiguator. |
| src/extract-docusaurus.ts | Implements section attribution via line coordinates, adds section_id wiring for inserts, and enforces parsed==stored with an exit-1 guard. |
| src/extract-docusaurus.test.ts | Adds fixtures/tests for repeated headings (ppp-aaa), h4–h6 folding behavior, and callout attribution. |
| src/db.ts | Reorders schema creation to support new FKs; migrates v8→v9 for properties (rebuild) and callouts (add column) while preserving FTS invariants. |
| MANUAL.md | Updates schema documentation to describe section_id and the deliberate h4–h6 folding behavior. |
| fixtures/docusaurus/ppp-aaa.md | Adds a realistic repeated-heading fixture used to pin the previously-lost-property behavior. |
| CHANGELOG.md | Documents the user-visible behavior fix (no more silent drops; new section_anchor surface). |
…oisoning the db.ts singleton Two defects surfaced by CI on this branch, neither caused by the #90 change itself. 1. A stray NUL byte landed in a template literal I added (`${p.name}<NUL>${p.section}`, intended to be a space). Syntactically valid, so tests/typecheck/biome all passed — but it made the whole file read as binary: `file` reported "data" and grep silently matched nothing in it. Replaced with the intended space. Scanned the other 270 source/doc files; this was the only one. 2. `import { EXCERPT_MARK_* } from "./query.ts"` was static. query.ts transitively loads db.ts, and an ESM static import is hoisted ABOVE this file's own `process.env.DB_PATH = ":memory:"` — so it opened the real on-disk ros-help.db and poisoned the db.ts singleton for the rest of the run. query.test.ts's V-db-wipe-guard caught it ("DB singleton is at .../ros-help.db"), which is exactly its job: without it, beforeAll()'s DELETEs would run against a real database. Pre-existing and order-dependent — it only bites when this file is the first to touch db.ts, and bun's test-file order is not stable (CI started with schema-roundtrip, local with extract-videos), so it hid until this branch drew the unlucky order. A re-run went green, which is precisely why it needed fixing rather than re-rolling. Verified the hoist directly: `process.env.DB_PATH=":memory:"` followed by a static db.ts import resolves to the repo-root path, not ":memory:". Not reproducible locally under bun 1.3.13 (CI runs 1.3.14), so the fix is grounded in the proven hoist and the repo's own rule rather than a local repro of the cross-file poisoning — .github/instructions/extractor-import-side-effects.instructions.md, hazard 2, mandates the dynamic form for this exact reason. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/extract-docusaurus.test.ts`:
- Around line 344-354: Add a storage regression test alongside the existing
collision test that uses the parsed page fixture to persist both dot1x interface
properties under the server section, then query the stored properties and assert
both rows remain. Ensure the fixture uses the same name and sectionAnchor with
distinct section IDs so a UNIQUE(page_id, name, section_id) constraint violation
is detected.
In `@src/extract-docusaurus.ts`:
- Around line 759-765: Update the rebuild flow around the section, callouts,
properties deletion block and insertAll so all deletes, inserts, FTS rebuilds,
and parsed/stored count validation execute within one transaction. Move
validation inside that transaction and throw when counts mismatch, allowing any
insert error or validation failure to roll back the destructive changes instead
of committing partial or invalid data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f195084-c875-47dc-a26a-e27d9055a66a
⛔ Files ignored due to path filters (1)
fixtures/docusaurus/ppp-aaa.mdis excluded by!fixtures/**
📒 Files selected for processing (10)
CHANGELOG.mdMANUAL.mdVALIDATION.mdsrc/db.tssrc/extract-docusaurus.test.tssrc/extract-docusaurus.tssrc/mcp.tssrc/paths.tssrc/query.test.tssrc/query.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Bun and TypeScript, including Bun-native runtime APIs such as
bun:sqlite,Bun.serve, andbunx.Rosetta uses Bun and TypeScript; prefer
bunandbun testrather than Node/npm-oriented substitutes.
Files:
src/paths.tssrc/mcp.tssrc/query.test.tssrc/db.tssrc/extract-docusaurus.test.tssrc/extract-docusaurus.tssrc/query.ts
*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep this Copilot instructions file short; put substantive rules in narrow instruction files under
.github/instructions/*.instructions.md.
Files:
VALIDATION.mdCHANGELOG.mdMANUAL.md
VALIDATION.md
📄 CodeRabbit inference engine (CLAUDE.md)
Put load-bearing invariants and their CI proof in
VALIDATION.md.
Files:
VALIDATION.md
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer each document's canonical home and do not duplicate operational detail, schema blocks, or long rule lists in the routing index.
Files:
VALIDATION.mdCHANGELOG.mdMANUAL.md
src/{query,mcp,browse}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Route MCP, query, classifier, TUI, and canonicalizer behavior through shared core code, usually
src/query.ts; keepsrc/mcp.tsandsrc/browse.tsthin adapters.
Files:
src/mcp.tssrc/query.ts
CHANGELOG.md
📄 CodeRabbit inference engine (CLAUDE.md)
Record user-visible shipped changes in
CHANGELOG.mdunder[Unreleased]or release sections.
Files:
CHANGELOG.md
{README.md,src/setup.ts,MANUAL.md}
📄 CodeRabbit inference engine (AGENTS.md)
Keep public client setup snippets aligned across
README.md,src/setup.ts, andMANUAL.md, including the Codex commandcodex mcp add rosetta -- bunx@tikoci/rosetta``.
Files:
MANUAL.md
MANUAL.md
📄 CodeRabbit inference engine (CLAUDE.md)
Put installation, operations, release/re-extraction procedures, and schema reference in
MANUAL.md.
Files:
MANUAL.md
🔇 Additional comments (16)
src/paths.ts (1)
105-114: LGTM!src/mcp.ts (1)
727-738: LGTM!src/query.test.ts (2)
136-160: LGTM!
901-928: LGTM!src/db.ts (4)
119-157: LGTM!
167-211: LGTM!
241-252: LGTM!
277-277: LGTM!src/query.ts (3)
372-379: LGTM!
1060-1069: LGTM!
1093-1116: LGTM!VALIDATION.md (1)
50-50: LGTM!CHANGELOG.md (1)
57-58: LGTM!MANUAL.md (1)
371-372: LGTM!Also applies to: 391-404
src/extract-docusaurus.test.ts (1)
8-17: LGTM!Also applies to: 33-33, 46-53, 271-343, 357-402
src/extract-docusaurus.ts (1)
78-112: LGTM!Also applies to: 295-320, 333-355, 456-487, 504-516
… the constraint (#90) CodeRabbit review: the existing tests only covered repeated heading TEXT across distinct anchors (ppp-aaa), which a UNIQUE(page_id, name, section_id) regression would survive. That is a real gap — and it is the gap over the PR's central claim, since re-keying on section_id is exactly the fix #90 proposed and the measurement rejected. Adds the harder case at both layers: - parse (extract-docusaurus.test.ts): dot1x's "Server" section documents `interface` more than once, with different descriptions — a server table and a client table under one heading. Confirmed against the fixture: 3 `interface` rows share the `server` anchor. - storage (query.test.ts): two rows sharing name AND section_id both persist. If anyone re-adds the constraint, the fixture INSERT throws and beforeAll() fails loudly. Proven, not assumed: temporarily re-adding UNIQUE(page_id, name, section_id) to db.ts makes the suite fail with "SQLiteError: UNIQUE constraint failed: properties.page_id, properties.name, properties.section_id". It passes 311/311 without. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes #90.
The issue's proposed fix doesn't work
#90 suggests making
UNIQUEusesection_idso it stops destroying rows. Measured against the whole corpus (365 cached pages, re-parsed with the real extractor), that still destroys 87 rows. Splitting the drops into exact duplicates (identical name/type/default/description — safe) vs genuinely distinct properties (real loss):UNIQUEkey(page, name, section)text(page, name, section_id)(page, name, section_id, section)UNIQUEat allThe reason is structural: the manual documents the same property name more than once within a single section, with different meanings.
dot1xdefinesinterfacetwice underServer— once for the server table, once for the client one — which matches #92's census finding that 12% of fragments hold more than one table.(page, name, section)encodes an assumption the corpus does not honor.The strongest evidence arrived by accident: trying to reinstate the old index on real data, SQLite refuses to build it. The constraint has never been satisfiable by this corpus.
So the constraint is removed, not re-keyed. Extractors now assert
parsed == stored(V-extractor-no-silent-drops) and fail the build on any gap.Also worth noting: #90's headline "165" bundles 24 harmless exact duplicates. The real damage was 141 distinct properties.
Results
Grounded against the CI-built v0.11.0-rc.97 artifact, and a full rebuild from the 365-page cache.
Properties:line counted attempted inserts, so it was reporting rows the DB did not contain.ppp-aaa'snameis back to its three real meanings ("PPP profile name", "Name used for authentication", "User name supplied at the authentication stage") instead of only the first.Remaining NULLs are genuine, not failures: 12 properties on
loop-protect/servicessit under a page H1 thatparseSectionsdeliberately drops as a title-duplicate; 104 callouts are page-level:::warningblocks above the first heading. Forcing an attribution would be worse than admitting there is none.The h4–h6 decision (#90 asks for this to be stated)
A property under an h4–h6 folds to its enclosing h1–h3 ancestor.
sectionsstays the retrieval unit androuteros_get_pageis unchanged.Grounded rather than assumed:
sectionstable can't model: an h2'stexttoday includes its h4 children's prose, so you must either duplicate it or silently shrink whatget_pagereturns for an existing section. That's a retrieval-unit redesign, not a data-loss fix.With
UNIQUEgone the fold destroys nothing — it costs only join precision, andsectionstill names the h4 verbatim, so the decision stays reversible from the same parse.Second commit:
section_anchorStoring every property surfaces what the constraint had been hiding by deletion: 111 name/section groups (276 rows) are indistinguishable by
section, since it is heading text that repeats.lookup_property(name="name")on PPP AAA would return three rows all reading "Properties".That ambiguity is created by the first commit, so this finishes it rather than deferring — otherwise dropping the constraint leaves
lookup_propertymeasurably worse (three confusing rows instead of one wrong-but-clean one).lookupPropertyandrelated.propertiesnow returnsection_anchor, unique within a page and feedable straight back intorouteros_get_page(page, section=…). Round-trip verified against the real corpus.Verification
INSERT OR IGNORE+ the constraint) reproduces the loss exactly —parsed 4575, stored 4410 — 165 row(s) lost— and exits 1. Clean builds exit 0.UNIQUEgone,section_idpresent on both tables, FTS still queryable (rowids preserved across the rebuild, so the external-content index stays valid),integrity_checkok,foreign_key_checkclean.ppp-aaaas Properties: 165 rows silently destroyed on insert; section identity stored as heading text (properties + callouts) #90 asks (new fixture), plusdot1xfor callout attribution — it demonstrates both the resolved case and the legitimate-NULL case.Scope notes
extract-properties.tsdeliberately untouched — Confluence-era, not inrelease.yml.MANUAL.mdschema block,VALIDATION.md(V-extractor-no-silent-drops), andCHANGELOG.mdupdated.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation