Skip to content

feat(db): persist editability classification + user override per paragraph - #205

Merged
thewrz merged 2 commits into
mainfrom
feat/issue-134
Jun 17, 2026
Merged

feat(db): persist editability classification + user override per paragraph#205
thewrz merged 2 commits into
mainfrom
feat/issue-134

Conversation

@thewrz

@thewrz thewrz commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Why

ADR-022 D2: a paragraph's machine classification (verdict + confidence + evidence) and a human's override must be stored side by side and never merged. Effective value = override ?? classification.editability. Re-classification must rewrite only the machine field — the machine never silently undoes a human. This is the Wave-2 (O-7) DB substrate the onboarding/editability UI and reclassify orchestration build on.

What

  • Migration 026 (reversible): two nullable JSONB columns on paragraphsclassification + editability_override. NULL = "not yet classified" / "no override". The never-merged contract is enforced physically: every write touches exactly one column.
  • Write path (src/db/queries/editability.ts):
    • storeClassifications(specId, ClassifyResult) — one transaction; UPDATE … SET classification … WHERE id = nodeId AND spec_id = specId. Spec-scoped (a cross-spec nodeId writes nothing) and reclassify-safe by construction (never touches the override).
    • setEditabilityOverride(paragraphId, editability) / clearEditabilityOverride(paragraphId).
  • Read surfacegetSpecTree only: surfaces effective meta.editability = { value, confidence, evidence, override? }, omitted entirely when unclassified (mirrors the feat(parser): surface DOCX inference conflicts via get_paragraph MCP tool #56 conflict / feat(db): persist paragraphs.source_facts JSONB + AST meta round-trip #131 sourceFacts omit-when-empty pattern). MCP/REST tree readers inherit it for free. Revision snapshots carry it too.
  • Closed (.strict()) Zod schemas at the DB boundary — these payloads are our own engine output, not captured external data, so malformed input is rejected as engine drift. A corrupt row is a loud DatabaseError at the read boundary, never a silent drop. (Deliberate, code-commented deviation from the parent design's "all JSONB open" rule, which exists to preserve unknown external clues.)
  • New ClassificationEvidenceSchema / SpecNodeEditability in the AST foundational layer; conventions/types.ts now re-uses the single AST ClassificationEvidence type rather than duplicating it.

DB write-path + read-surface only. Out of scope: HTTP endpoints + reclassify orchestration (O-9 / #136).

Testing

  • Unit tests pass (pnpm test — 948 passed) — closed-schema rejections (confidence out of range, empty evidence, unknown keys/values).
  • Integration tests pass (pnpm test:integration — 421 passed) against real PostgreSQL, incl. the decisive override survives reclassify, round-trip, clear-restores-machine, spec-scope, and corrupt-row-rejects tests.
  • pnpm lint clean (eslint + tsc + prettier).
  • CI green

🤖 Co-authored by Claude Opus 4.8. Closes #134.

Summary by CodeRabbit

  • New Features

    • Paragraph editability classification system with persistent storage and human override capability.
    • Classifications include machine confidence levels and evidence chains.
    • Editability metadata now available when retrieving specifications.
  • Database

    • New migration supporting paragraph editability storage.
    • Query functions for storing classifications and managing editability overrides.
  • Documentation

    • Design specification for paragraph editability classification persistence model added.

…graph

Stores the machine's editability `classification` (verdict + confidence +
evidence) and the human's `editability_override` side by side in two nullable
JSONB columns (migration 026), never merged (ADR-022 D2). Each write touches
exactly one column, so re-classification rewrites only the machine field and can
never silently undo a human override — the never-merged contract is enforced
physically, not by convention.

Effective value = override ?? classification.editability, surfaced on
`SpecNode.meta.editability` via `getSpecTree` (mirroring the #56 conflict /
#131 sourceFacts omit-when-empty pattern), so MCP/REST tree readers inherit it
for free. The machine's why-chain stays readable even under an override (the
O-15 machine-vs-human badge).

The DB-boundary schemas are CLOSED (.strict()): the payloads are our own engine
output, not captured external data, so malformed input is rejected as engine
drift rather than preserved. A corrupt row is a loud DatabaseError at the read
boundary, never a silent drop.

DB write-path + read-surface only; HTTP endpoints and reclassify orchestration
are O-9 (#136).

Closes #134

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

An error occurred during the review process. Please try again later.

📝 Walkthrough

Walkthrough

Adds paragraph editability persistence per Issue #134: a reversible DB migration introduces two nullable JSONB columns (classification, editability_override) on paragraphs. New Zod schemas validate payloads at the DB boundary. Three write-path functions store classifications and manage overrides. getSpecTree gains a deriveEditability helper that computes meta.editability from both columns. Unit and integration tests cover all contracts.

Changes

Paragraph Editability Persistence

Layer / File(s) Summary
AST editability schemas, types, and ClassificationEvidence consolidation
src/ast/schemas.ts, src/ast/types.ts, src/ast/index.ts, src/conventions/types.ts
Adds ClassificationEvidenceSchema and SpecNodeEditabilitySchema as strict Zod schemas; exports SpecNodeEditability type and wires an optional editability field into SpecNodeMeta; expands ast/index.ts re-exports; consolidates ClassificationEvidence from a local conventions interface into a re-export from ast.
DB migration: classification and editability_override columns
src/db/migrations/026_paragraph_editability.ts
Reversible migration adding two nullable jsonb columns (classification, editability_override) to the paragraphs table, with corresponding down drop.
DB write-path: editability query functions
src/db/queries/editability.ts, src/db/index.ts
Defines closed ClassificationSchema and OverrideSchema at the DB boundary; implements storeClassifications (transactional, spec-scoped batch update), setEditabilityOverride, and clearEditabilityOverride; all three re-exported from db/index.ts.
DB read-path: deriveEditability in getSpecTree and revisions
src/db/queries/specs.ts, src/db/queries/revisions.ts
Extends ParagraphTreeRow with the two new JSONB fields; adds deriveEditability helper computing optional SpecNodeEditability (override wins if present); wires into buildNodeTree node meta; updates getSpecTree and snapshotMemberTrees SELECT clauses to fetch the new columns.
Unit and integration tests
src/db/queries/editability.test.ts, src/db/queries/editability.integration.test.ts
Unit tests verify ClassificationSchema/OverrideSchema strictness. Integration tests cover classification round-trip, override surviving reclassification, override clearing, spec-scoping of writes, and loud error on corrupt stored payload.
Issue #134 design document
docs/superpowers/specs/2026-06-16-issue-134-design.md
Full design spec covering storage model, schema boundaries, write/read path contracts, error-handling requirements, and test plan.

Sequence Diagram(s)

sequenceDiagram
  participant ClassificationEngine
  participant storeClassifications
  participant ParagraphsTable
  participant getSpecTree
  participant deriveEditability

  rect rgba(70, 130, 180, 0.5)
    note over ClassificationEngine, ParagraphsTable: Write path
    ClassificationEngine->>storeClassifications: specId, ClassifyResult
    storeClassifications->>ParagraphsTable: BEGIN transaction
    storeClassifications->>ParagraphsTable: UPDATE paragraphs SET classification WHERE id AND spec_id
    storeClassifications->>ParagraphsTable: COMMIT
  end

  rect rgba(60, 179, 113, 0.5)
    note over ParagraphsTable, deriveEditability: Read path
    getSpecTree->>ParagraphsTable: SELECT paragraphs with classification and editability_override
    ParagraphsTable-->>getSpecTree: ParagraphTreeRow[]
    getSpecTree->>deriveEditability: classification, editability_override
    deriveEditability->>deriveEditability: resolve override ?? classification.editability
    deriveEditability-->>getSpecTree: SpecNodeEditability or undefined
    getSpecTree-->>ClassificationEngine: SpecNode with meta.editability
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • #134 (feat(db): persist editability classification + user override per paragraph) — This PR directly implements all acceptance criteria from Issue #134: round-trip classification storage, override surviving reclassification, override clearing, and getSpecTree surfacing effective editability and evidence.
  • feat(api): editability corrections + reclassify with before/after diff #136 — The REST API endpoints for editability corrections depend on the storeClassifications, setEditabilityOverride, and clearEditabilityOverride functions introduced here.

Possibly related PRs

  • wrzonance/SpecR#168: Both PRs modify ParagraphTreeRow and buildNodeTree in src/db/queries/specs.ts; they overlap at the same paragraph-row/tree-building integration point.
  • wrzonance/SpecR#193: This PR consolidates ClassificationEvidence from a local conventions/types.ts interface into a re-export from ast, directly touching the conventions engine type contract that PR #193 also modifies.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: implementing database persistence for both paragraph editability classification and user overrides, which is the core focus of this PR.
Linked Issues check ✅ Passed The PR fully addresses all linked issue #134 requirements: reversible migration with two JSONB columns, Zod schema boundary enforcement, SpecNodeMeta editability surfacing, write path functions, and all four acceptance criteria are met.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #134 objectives: migration, schema definitions, database query functions, and read-path integration. HTTP endpoints and reclassify orchestration are correctly excluded.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/issue-134

Comment @coderabbitai help to get the list of available commands and usage tips.

@thewrz

thewrz commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 `@docs/superpowers/specs/2026-06-16-issue-134-design.md`:
- Around line 128-134: The test name 'override survives reclassify' in the
editability.integration.test.ts file is too brief and lacks context for
documentation. Rename this test to be more descriptive and explicitly reference
the editability context and the machine verdict change it verifies, such as
'editability: override survives reclassification with new machine verdict'. This
expanded name will make it immediately clear that the test verifies the override
persists when a different classification is applied, improving documentation
clarity without changing the test logic or assertions.

In `@src/db/queries/specs.ts`:
- Around line 134-139: The issue is that the early return at line 134 happens
before the override validation (OverrideSchema.parse), which allows malformed
override payloads on unclassified rows to be silently ignored. Move the override
validation that extracts editability to occur BEFORE the null/undefined check on
classification, so that the override payload is validated regardless of the
classification value. Additionally, add a regression test case that verifies a
corrupt override payload combined with a null classification properly fails
validation instead of being silently ignored.
🪄 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: 6a17b21a-4ff1-4068-9442-39e504ffbcbc

📥 Commits

Reviewing files that changed from the base of the PR and between c761bc6 and b6f0197.

📒 Files selected for processing (12)
  • docs/superpowers/specs/2026-06-16-issue-134-design.md
  • src/ast/index.ts
  • src/ast/schemas.ts
  • src/ast/types.ts
  • src/conventions/types.ts
  • src/db/index.ts
  • src/db/migrations/026_paragraph_editability.ts
  • src/db/queries/editability.integration.test.ts
  • src/db/queries/editability.test.ts
  • src/db/queries/editability.ts
  • src/db/queries/revisions.ts
  • src/db/queries/specs.ts

Comment thread docs/superpowers/specs/2026-06-16-issue-134-design.md
Comment thread src/db/queries/specs.ts Outdated
deriveEditability returned undefined for unclassified rows before parsing
editability_override, so a corrupt override payload on an unclassified row
was silently dropped instead of failing loud at the DB boundary. Reorder so
the override is validated first — fulfilling the docstring's promise that both
JSONB columns are schema-checked.

Pin with a buildNodeTree unit test for "corrupt override + null classification"
and rename the override-survives-reclassify integration test to the house-style
'editability: ...' form. Addresses CodeRabbit review on #205.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

feat(db): persist editability classification + user override per paragraph

1 participant