feat(parser): hierarchy-inference confidence — provenance jsonb + read-time scorer - #418
Merged
Conversation
…e jsonb + read-time scorer
Approved brainstorm output: persist {signalUsed, agreed} per paragraph,
derive 0-1 confidence + evidence at read time, surface in onboarding
report triage + paragraph meta. ADR-054 to follow with implementation.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closes-tracking: #412 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>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Migration 041 adds paragraphs.signal_provenance (jsonb, NULL = honestly
unscored). Write path threads {signalUsed, agreed} through flattenDfs/
insertTree and cloneParagraphs; read paths (getSpecTree, ancestors,
subtree) derive meta.inference at read time via the shared
deriveInference helper, so the scoring formula can improve without
migration or reparse (ADR-055).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… contract prose Address six verified adversarial-review findings on the hierarchy-confidence branch (ADR-055): 1. snapshotMemberTrees now SELECTs signal_provenance, so frozen revision trees retain meta.inference instead of silently unscoring every node. 2. buildNodeTree validates node_type via NodeTypeSchema (loud DatabaseError on mismatch) once per row, feeding the validated NodeType to both the type field and deriveInference — removing the unchecked `as NodeType` casts. Fixture rows that used the non-enum 'paragraph' placeholder are corrected to 'pr1'. 3. get_spec MCP tool description documents meta.inference on structural nodes. 4. hierarchy-summary's unscored reason is honest about all causes (pre-provenance parse, non-DOCX source, or manually inserted) while keeping the DOCX re-import upgrade path; openapi unscoredReason prose updated to match. 5. openapi SpecNodeInference absence list drops the incorrect "hidden" case and adds manually inserted paragraphs. 6. openapi importLibraryMaster pipeline prose names the hierarchy-inference summary step. Pinned with a revisions integration regression test asserting a structural node in a frozen snapshot carries meta.inference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds persisted hierarchy provenance, computes read-time inference confidence, and exposes a hierarchy summary in onboarding and MCP responses. It also updates schemas, database reads/writes, and tests to carry the new inference fields. ChangesHierarchy-Inference Confidence Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant classifyOne
participant scoreHierarchyConfidence
participant paragraphs
participant buildNodeTree
participant summarizeHierarchy
classifyOne->>scoreHierarchyConfidence: signalUsed, agreed, conflicts, nodeType
scoreHierarchyConfidence-->>classifyOne: SpecNodeInference
classifyOne->>paragraphs: insert signal_provenance
buildNodeTree->>paragraphs: select signal_provenance, conflicts
buildNodeTree->>scoreHierarchyConfidence: deriveInference(provenance, conflicts, nodeType)
scoreHierarchyConfidence-->>buildNodeTree: meta.inference
buildNodeTree->>summarizeHierarchy: tree, source
summarizeHierarchy-->>buildNodeTree: HierarchySummary
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
thewrz
marked this pull request as ready for review
July 8, 2026 00:13
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/ast/normalized-ilvl.ts (1)
31-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilent fallback to 0 for unmapped node types.
nodeTypeToNormalizedIlvlreturns0(same as'part') for anyNodeTypenot present inNODE_TYPE_TO_NORMALIZED_ILVL, since the map isPartial. If a futureNodeTypevariant is added without updating this map, the scorer'sCONFLICT_ILVL_STEPdistance calculation (src/parser/docx/hierarchy-confidence.ts) would silently compute a wrong penalty instead of failing loudly, undermining the "single source of truth" comment's intent.Consider throwing (or asserting exhaustively) on an unmapped type instead of silently defaulting to 0, so a future missing mapping surfaces immediately rather than corrupting confidence scores.
♻️ Proposed fix
export function nodeTypeToNormalizedIlvl(nodeType: NodeType): number { - return NODE_TYPE_TO_NORMALIZED_ILVL[nodeType] ?? 0; + const ilvl = NODE_TYPE_TO_NORMALIZED_ILVL[nodeType]; + if (ilvl === undefined) { + throw new Error(`nodeTypeToNormalizedIlvl: no mapping for node type "${nodeType}"`); + } + return ilvl; }🤖 Prompt for 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. In `@src/ast/normalized-ilvl.ts` around lines 31 - 33, `nodeTypeToNormalizedIlvl` currently hides missing `NodeType` mappings by defaulting to 0, which can silently corrupt hierarchy scoring. Update the `NODE_TYPE_TO_NORMALIZED_ILVL` lookup in `nodeTypeToNormalizedIlvl` to fail loudly for unmapped types instead of returning a fallback, using an exhaustive assertion or throwing an error when the map has no entry. Keep the behavior aligned with the “single source of truth” intent so future `NodeType` additions are forced to update this mapping and do not affect `hierarchy-confidence` calculations silently.src/lib/hierarchy-summary.ts (1)
82-99: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider capping
lowConfidencefor pathological docs.
lowConfidenceis unbounded — a badly-imported DOCX with many low-confidence paragraphs would return the full list in the onboarding report/MCP payload. Since this mirrors the existingeditabilitypattern, it's likely an accepted tradeoff, but a.slice(0, N)cap could bound response size for large specs.🤖 Prompt for 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. In `@src/lib/hierarchy-summary.ts` around lines 82 - 99, Cap the lowConfidence list returned by summarizeHierarchy to avoid unbounded onboarding/MCP payloads for pathological documents. Keep the existing sort in summarizeHierarchy, then apply a fixed upper limit before returning lowConfidence, following the same bounded-response pattern used elsewhere so the counts remain unchanged while the payload stays small.src/parser/docx/inference.test.ts (1)
453-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating duplicated
ClassifiedParagraphfixture builders.
makeClassifiedhere mirrors near-identical builders insrc/parser/docx/consensus-stats.test.ts(para),src/parser/docx/derive-template.test.ts(para), andsrc/parser/docx/numbering-profile-apply.test.ts(cp) — all four had to be edited in lockstep just to add the newagreed: []field in this PR. Extracting one shared builder (e.g. in a test-utils module) would prevent this repeated multi-file churn the next timeClassifiedParagraphgains/changes a field.♻️ Sketch of a shared helper
// src/parser/docx/test-fixtures.ts export function makeClassifiedParagraph( overrides: Partial<ClassifiedParagraph> & Pick<ClassifiedParagraph, 'nodeType' | 'resolvedIlvl'> ): ClassifiedParagraph { return { paragraph: { text: '', isVanish: false }, signalUsed: 1, conflicts: [], agreed: [], isVanish: false, ...overrides, }; }🤖 Prompt for 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. In `@src/parser/docx/inference.test.ts` around lines 453 - 468, The test fixture builder for ClassifiedParagraph is duplicated across multiple docx tests, so future field changes require repeated edits. Consolidate makeClassified and the equivalent para/cp helpers into a shared test utility (for example, a docx test-fixtures helper) and update the tests to use that single builder, keeping the agreed field and other defaults centralized.src/db/queries/specs.ts (1)
157-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
parseNodeTypeis duplicated verbatim inparagraphs.ts.Both copies guard the same DB↔AST
node_typeinvariant and even carry mirrored comments. Extracting a single shared helper (e.g. undersrc/db/queries/) avoids future drift between the two guards.🤖 Prompt for 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. In `@src/db/queries/specs.ts` around lines 157 - 170, The parseNodeType helper is duplicated in both specs.ts and paragraphs.ts, so extract the shared DB-to-AST node_type validation into a single reusable helper under src/db/queries and have both call sites use it. Keep the current behavior and error handling intact by moving the NodeTypeSchema safeParse logic and DatabaseError path into the shared function, then update the existing parseNodeType usages in the relevant query modules to reference that common helper instead of maintaining two mirrored copies.
🤖 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.
Nitpick comments:
In `@src/ast/normalized-ilvl.ts`:
- Around line 31-33: `nodeTypeToNormalizedIlvl` currently hides missing
`NodeType` mappings by defaulting to 0, which can silently corrupt hierarchy
scoring. Update the `NODE_TYPE_TO_NORMALIZED_ILVL` lookup in
`nodeTypeToNormalizedIlvl` to fail loudly for unmapped types instead of
returning a fallback, using an exhaustive assertion or throwing an error when
the map has no entry. Keep the behavior aligned with the “single source of
truth” intent so future `NodeType` additions are forced to update this mapping
and do not affect `hierarchy-confidence` calculations silently.
In `@src/db/queries/specs.ts`:
- Around line 157-170: The parseNodeType helper is duplicated in both specs.ts
and paragraphs.ts, so extract the shared DB-to-AST node_type validation into a
single reusable helper under src/db/queries and have both call sites use it.
Keep the current behavior and error handling intact by moving the NodeTypeSchema
safeParse logic and DatabaseError path into the shared function, then update the
existing parseNodeType usages in the relevant query modules to reference that
common helper instead of maintaining two mirrored copies.
In `@src/lib/hierarchy-summary.ts`:
- Around line 82-99: Cap the lowConfidence list returned by summarizeHierarchy
to avoid unbounded onboarding/MCP payloads for pathological documents. Keep the
existing sort in summarizeHierarchy, then apply a fixed upper limit before
returning lowConfidence, following the same bounded-response pattern used
elsewhere so the counts remain unchanged while the payload stays small.
In `@src/parser/docx/inference.test.ts`:
- Around line 453-468: The test fixture builder for ClassifiedParagraph is
duplicated across multiple docx tests, so future field changes require repeated
edits. Consolidate makeClassified and the equivalent para/cp helpers into a
shared test utility (for example, a docx test-fixtures helper) and update the
tests to use that single builder, keeping the agreed field and other defaults
centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc0fda11-a394-4e89-aaac-4ed31cf9b070
📒 Files selected for processing (40)
ARCHITECTURE.mddocs/adr/055-hierarchy-inference-confidence.mddocs/superpowers/plans/2026-07-07-hierarchy-confidence.mddocs/superpowers/specs/2026-07-07-hierarchy-confidence-design.mdopenapi.yamlsrc/api/onboarding.integration.test.tssrc/api/onboarding.tssrc/api/revisions.integration.test.tssrc/ast/index.tssrc/ast/inference-schemas.tssrc/ast/normalized-ilvl.tssrc/ast/schemas.tssrc/ast/types.tssrc/db/index.tssrc/db/migrations/041_paragraphs_signal_provenance.tssrc/db/queries/associations.integration.test.tssrc/db/queries/derive.tssrc/db/queries/inference-meta.tssrc/db/queries/paragraphs.tssrc/db/queries/revisions.tssrc/db/queries/specs.test.tssrc/db/queries/specs.tssrc/lib/hierarchy-summary.test.tssrc/lib/hierarchy-summary.tssrc/lib/jobs.test.tssrc/lib/jobs.tssrc/mcp/onboarding-handlers.tssrc/mcp/onboarding-tools.tssrc/mcp/onboarding.integration.test.tssrc/mcp/tools.tssrc/parser/docx/consensus-stats.test.tssrc/parser/docx/derive-template.test.tssrc/parser/docx/hierarchy-confidence.test.tssrc/parser/docx/hierarchy-confidence.tssrc/parser/docx/index.tssrc/parser/docx/inference.test.tssrc/parser/docx/inference.tssrc/parser/docx/numbering-profile-apply.test.tssrc/parser/docx/types.tssrc/parser/index.ts
CodeRabbit nitpicks 1+4: nodeTypeToNormalizedIlvl no longer aliases unmapped types onto part/ilvl-0 (a future NodeType addition now surfaces immediately instead of silently corrupting conflict-distance penalties), and the verbatim- duplicated parseNodeType guard is extracted to db/queries/node-type.ts with a context-named error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai Addressed your review in a64aecb — per-finding resolution:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5 a64aecb 2>/dev/null || echo "commit not found locally"
git show a64aecb --stat 2>/dev/null | head -50Length of output: 1621 🏁 Script executed: #!/bin/bash
git show a64aecb -- src/ast/normalized-ilvl.ts
echo "-----"
git show a64aecb -- src/db/queries/node-type.tsLength of output: 3483 Both fixes look solid — confirmed in commit a64aecb.
On the two declined items:
Nothing further from me on these four items. [resolve] |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ast/normalized-ilvl.ts (1)
1-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo parallel structures risk silent drift.
NODE_TYPE_TO_NORMALIZED_ILVLandNODE_TYPES_BY_NORMALIZED_ILVLencode the same ordering in two independently-maintained literals, despite the comment calling this a "single source of truth." If a future node type (e.g.pr8) is added to only one of them, nothing here enforces consistency, and the round-trip test only exercises entries present in the array — a map-only addition would go unnoticed.Derive one from the other to guarantee they can't diverge.
♻️ Proposed fix to derive the map from the array
-export const NODE_TYPE_TO_NORMALIZED_ILVL: Partial<Record<NodeType, number>> = { - part: 0, - article: 1, - pr1: 2, - pr2: 3, - pr3: 4, - pr4: 5, - pr5: 6, - pr6: 7, - pr7: 8, -}; - export const NODE_TYPES_BY_NORMALIZED_ILVL: readonly NodeType[] = [ 'part', 'article', 'pr1', 'pr2', 'pr3', 'pr4', 'pr5', 'pr6', 'pr7', ]; + +export const NODE_TYPE_TO_NORMALIZED_ILVL: Partial<Record<NodeType, number>> = + Object.fromEntries(NODE_TYPES_BY_NORMALIZED_ILVL.map((nodeType, ilvl) => [nodeType, ilvl]));🤖 Prompt for 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. In `@src/ast/normalized-ilvl.ts` around lines 1 - 29, The normalized ILVL ordering is duplicated in NODE_TYPE_TO_NORMALIZED_ILVL and NODE_TYPES_BY_NORMALIZED_ILVL, so they can drift out of sync. Update the logic in normalized-ilvl.ts to derive one structure from the other using the shared NodeType ordering, keeping the canonical list in a single place. Make sure the exported constants stay consistent for the inference engine, hierarchy-confidence scorer, and report summarizer.
🤖 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/ast/normalized-ilvl.ts`:
- Around line 31-40: Replace the generic Error thrown in
nodeTypeToNormalizedIlvl with a module-local SpecrError to match the typed-error
convention. Add an AST-specific error type in src/ast and use it for the missing
NODE_TYPE_TO_NORMALIZED_ILVL mapping failure path in nodeTypeToNormalizedIlvl.
Keep the existing guard and message semantics, but ensure the failure is raised
through the new SpecrError class so callers can discriminate this error type
consistently.
---
Nitpick comments:
In `@src/ast/normalized-ilvl.ts`:
- Around line 1-29: The normalized ILVL ordering is duplicated in
NODE_TYPE_TO_NORMALIZED_ILVL and NODE_TYPES_BY_NORMALIZED_ILVL, so they can
drift out of sync. Update the logic in normalized-ilvl.ts to derive one
structure from the other using the shared NodeType ordering, keeping the
canonical list in a single place. Make sure the exported constants stay
consistent for the inference engine, hierarchy-confidence scorer, and report
summarizer.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93ae7c2f-eb16-4e43-a427-fee0b3462103
📒 Files selected for processing (5)
src/ast/normalized-ilvl.test.tssrc/ast/normalized-ilvl.tssrc/db/queries/node-type.tssrc/db/queries/paragraphs.tssrc/db/queries/specs.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/db/queries/paragraphs.ts
CodeRabbit follow-up: nodeTypeToNormalizedIlvl threw a raw Error at a module surface, against the SpecrError typed-error convention. Adds src/ast/error.ts (AstError extends SpecrError, mirroring parser/error.ts) exported via the barrel; test pins the type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The "single source of truth" comment claimed one canonical ordering, but the lookup map and the by-ilvl list were two independently-maintained literals that could silently drift (a map-only node-type addition would slip past the round-trip test). Derive NODE_TYPE_TO_NORMALIZED_ILVL from the authoritative NODE_TYPES_BY_NORMALIZED_ILVL so the two can never diverge. Output-identical: 0/703 fixture-corpus classification drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Soft-removal (PATCH .../removal) sets vanish on a single node with no cascade to descendants, and the owner-facing renderers suppress the whole vanished subtree (renderPrNode returns '' before recursing). The hierarchy-summary walker skipped the vanished node itself but still recursed into its children, so get_onboarding_report could count and flag paragraphs that no longer render — telling a reviewer to inspect a low-confidence paragraph that was already removed. Prune the entire subtree at a vanish node, matching reporting.ts's "vanish ∪ descendants" exclusion. Regression test pins a scored, low-confidence descendant under a removed parent staying out of the report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
Review loop — remaining items handledCodeRabbit body nitpick (2nd review):
|
4 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The 5-signal DOCX inference engine records disagreement (
meta.conflicts) but nothing records how strongly the winning classification was supported — a paragraph classified by the indentation fallback alone looked identical to one nailed by numbering.xml with full corroboration. Editability classification and style consensus already expose 0–1 confidence in the API contract; hierarchy inference did not, so human review triage (onboarding report, future review canvas #143) had nothing to rank by. The blind spot of unanimous-but-weak wins is the point.Implements the approved design
docs/superpowers/specs/2026-07-07-hierarchy-confidence-design.md(ADR-055 — the design doc's "ADR-054" was taken by first-class-clients in the interim).What
paragraphs.signal_provenancejsonb (migration 041) records{ signalUsed, agreed }—agreedcomputed against the final post-correctMisalignedArticleresolution. NULL = honestly unscored, never a fake number. The formula can improve without migration or reparse.src/parser/docx/hierarchy-confidence.ts: base = winner reliability tier (0.95/0.85/0.6/0.6/0.35), bounded corroboration bonus, severity-weighted conflict penalty (ilvl-distance scaled), clamp [0,1]. Evidence strings name signals, never vendors.meta.inferenceon every paragraph read (GET /specs/{id}, paragraph write responses, MCPget_paragraph/get_spec) and ahierarchysection in the onboarding report (REST job + MCPget_onboarding_report), review threshold 0.6, worst-first triage list. SEC-sourced specs read as "explicit structure", never suspect; unscored always carries its reason.conflictsuntouched ("persisted, never dropped").Invariants proven
0/703 fixtures changed, snapshots byte-identical.Testing
hierarchysection +meta.inferenceon tree reads🤖 Co-authored by Claude Fable 5 (planned via 5-reader understand workflow; implemented by 8 sequential task agents each adversarially verified; 3-lens adversarial review, 6 findings fixed). Closes #412.
Summary by CodeRabbit