feat(standards): standards registry, rollup reads, and verdict write path - #458
Conversation
…ite path Adds the cited standard as a first-class record (#446, ADR-064). The parser already extracts standards-org references per paragraph into spec_references; this compiles them into a per-scope rollup and lets a reviewing client persist a currency verdict that the next rollup reflects deterministically. - migration 043: `standards` table (org_code, standard_code UNIQUE; status enum default 'unknown'; nullable title/current_version/source_url/last_verified_at/ notes), reversible. - db: pure `buildStandardsRollup` + `parseStandardCitation` (standards.ts) and the DB read/upsert layer (standards-read.ts), mirroring the ADR-063 reference-graph pure/read split. Citations normalize to (orgCode uppercased, standardCode) on a first-whitespace split; the registry is global and joined per cited standard. - api: GET /libraries/{id}/standards, GET /projects/{id}/standards, and PUT /standards/{orgCode}/{standardCode} (upsert verdict; server stamps last_verified_at). openapi.yaml updated in the same change; contract gate green. - mcp: list_library_standards / list_project_standards (read tier) + record_standard_verification (write tier) with contract-map + capability entries. - finding: a cited standard the registry marks superseded/withdrawn surfaces as a rollup finding with citing specs + anchors, plus summary counts. Closes #446 Co-Authored-By: Claude Fable 5 <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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces a global standards registry with normalized citations, scoped library/project rollups, verification upserts, superseded/withdrawn findings, REST/OpenAPI endpoints, and MCP tools. ChangesStandards registry
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RESTAPI
participant StandardsQueries
participant Database
Client->>RESTAPI: Request standards rollup
RESTAPI->>StandardsQueries: Read scoped rollup
StandardsQueries->>Database: Query citations and registry
Database-->>StandardsQueries: Citation and verdict rows
StandardsQueries-->>RESTAPI: Compiled rollup
RESTAPI-->>Client: Rollup response
Client->>RESTAPI: Record verification
RESTAPI->>StandardsQueries: Upsert verdict
StandardsQueries->>Database: Store status and last_verified_at
Database-->>RESTAPI: Standard record
RESTAPI-->>Client: Verification response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
The library standards path is /libraries/{id}/standards, but it referenced the
shared LibraryId parameter (name: libraryId) — a name/path mismatch redocly's
path-parameters-defined rule fails on (the runtime contract gate doesn't catch
it). Mirror the existing /libraries/{id}/reference-graph route with an inline
`name: id` path parameter.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review 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
🧹 Nitpick comments (1)
src/db/migrations/043_create_standards.ts (1)
25-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd CHECK constraints mirroring the org_code guard for standard_code and case normalization.
The unique key's correctness depends on
org_codealways being stored uppercase andstandard_codenever being empty (both invariants are only enforced in application code —parseStandardCitation/recordStandardVerification). A future write path bypassing that normalization (raw SQL, backfill script, bug) would silently break the rollup JOIN rather than fail loudly.standards_org_code_nonemptyalready guards one invariant; consider mirroring it forstandard_codeand the uppercase rule.🛡️ Proposed additional constraints
pgm.addConstraint( 'standards', 'standards_org_code_nonempty', 'CHECK (length(trim(org_code)) > 0)' ); + pgm.addConstraint( + 'standards', + 'standards_standard_code_nonempty', + 'CHECK (length(trim(standard_code)) > 0)' + ); + pgm.addConstraint( + 'standards', + 'standards_org_code_upper', + 'CHECK (org_code = upper(org_code))' + );🤖 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/migrations/043_create_standards.ts` around lines 25 - 39, Add CHECK constraints in the migration alongside standards_org_code_nonempty: require trim(standard_code) to have positive length, and enforce org_code is stored uppercase (for example, org_code = upper(org_code)). Use clear constraint names consistent with the existing standards_* naming scheme.
🤖 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/db/queries/standards-read.ts`:
- Around line 185-214: Validate the normalized identifiers in
recordStandardVerification before executing the upsert: after trimming orgCode
and standardCode (and uppercasing orgCode), reject either value when it is empty
or whitespace-only. Throw the function’s established validation/error type with
a clear message, ensuring invalid inputs cannot reach the INSERT ... ON CONFLICT
operation.
In `@src/mcp/standards-handlers.ts`:
- Around line 24-25: Update the `orgCode` and `standardCode` schemas in the
standards handler to reject whitespace-only values by trimming before applying
the non-empty validation, while preserving the intended uppercase normalization
for `orgCode`; ensure validation occurs before `recordStandardVerification`
persists the trimmed fields.
---
Nitpick comments:
In `@src/db/migrations/043_create_standards.ts`:
- Around line 25-39: Add CHECK constraints in the migration alongside
standards_org_code_nonempty: require trim(standard_code) to have positive
length, and enforce org_code is stored uppercase (for example, org_code =
upper(org_code)). Use clear constraint names consistent with the existing
standards_* naming scheme.
🪄 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: f321c376-a139-4246-ba72-bd2420de49bc
📒 Files selected for processing (18)
docs/adr/064-standards-registry.mdopenapi.yamlsrc/api/contract.integration.test.tssrc/api/router.tssrc/api/standards.integration.test.tssrc/api/standards.tssrc/db/index.tssrc/db/migrations/043_create_standards.tssrc/db/queries/standards-read.integration.test.tssrc/db/queries/standards-read.tssrc/db/queries/standards.test.tssrc/db/queries/standards.tssrc/mcp/capabilities.tssrc/mcp/contract-map.tssrc/mcp/standards-handlers.tssrc/mcp/standards-tools.tssrc/mcp/standards.integration.test.tssrc/mcp/tools.ts
…nown)
OpenAPI declares the verdict body optional (requestBody.required=false,
ADR-064 §3), but a PUT with no application/json header leaves req.body
undefined, and z.object().safeParse(undefined) rejected it with 422.
Parse req.body ?? {} so a documented no-body write records an empty
verdict — all fields reset, status defaults to 'unknown'. Pinned with an
integration regression asserting the missing-body path returns 200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex (GPT-5.5, xhigh) adversarial review — outcomesRan Codex as a second reviewer against [P2] No-body [P2] No public way to record a verdict for empty-code (no-whitespace) citations — DECLINED (working as designed) 🤖 Codex review of record; fix + assessment by Claude Fable 5. |
…he org-only key A whitespace-only orgCode/standardCode trimmed to '' and upserted the exact (org, '') key ADR-064 §2 reserves for unparseable org-only .SEC citations, attaching a verdict to every ambiguous citation for that org. Defense in depth: - MCP record_standard_verification now trims before min(1) (z.string().min(1) accepted a lone space); REST already rejects blank path segments. - recordStandardVerification rejects a trimmed-empty key before the INSERT. - Migration 043 adds standard_code-nonempty + org_code-uppercase CHECKs, so a raw-SQL/backfill bypass fails loudly instead of silently breaking the JOIN. Regressions pin the whitespace-only path at the MCP and DB boundaries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/api/contract.integration.test.ts (1)
95-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep these operations in
RESPONSE_COVERED, not the allowlist.The standards integration tests already call
assertResponsefor all three operations. Allowlisting them bypasses the coverage check and would permit those assertions to disappear without this guard detecting it. Move these entries toRESPONSE_COVERED.🤖 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/api/contract.integration.test.ts` around lines 95 - 97, Move the three standards operation entries from the allowlist into RESPONSE_COVERED in the contract integration test configuration. Preserve the existing assertResponse coverage checks for each operation and ensure they are no longer treated as allowlisted exceptions.src/db/queries/standards-read.ts (1)
126-131: 🚀 Performance & Scalability | 🔵 TrivialFull registry table scan on every rollup call.
readRegistryunconditionally selects every row instandards, then joins in-memory viabuildStandardsRollup. Functionally correct today (unmatched registry rows are simply discarded), but this meansgetStandardsRollupreads the entire global registry — not the subset cited in scope — on every library/project rollup request. As the registry accumulates verdicts across many organizations/projects over time, this becomes an unbounded full-table read per request instead of a targeted lookup keyed on the standards actually cited in that scope.Given ADR-064's stated intent that the registry stays "global, not scope-owned" and relatively small, this is likely fine for now — flagging for awareness rather than as a blocker. If the registry grows large, consider filtering the SQL by the distinct
(org_code, standard_code)pairs derived fromreadCitationsinstead of pulling the whole table.Also applies to: 139-162
🤖 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/standards-read.ts` around lines 126 - 131, readRegistry currently loads the entire global standards table for every buildStandardsRollup call. Refactor readRegistry and its callers, including getStandardsRollup, to accept the distinct (org_code, standard_code) pairs from readCitations and filter the SQL query to those keys, preserving the existing in-memory join behavior and handling an empty key set without scanning the table.
🤖 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/api/standards.ts`:
- Around line 71-74: In the request-body parsing logic, change the fallback used
by VerificationBodySchema.safeParse so only undefined is treated as an absent
body; preserve explicit null for schema validation and rejection. Add an
integration regression test for a PUT request with body "null" that asserts a
422 response before persistence.
---
Nitpick comments:
In `@src/api/contract.integration.test.ts`:
- Around line 95-97: Move the three standards operation entries from the
allowlist into RESPONSE_COVERED in the contract integration test configuration.
Preserve the existing assertResponse coverage checks for each operation and
ensure they are no longer treated as allowlisted exceptions.
In `@src/db/queries/standards-read.ts`:
- Around line 126-131: readRegistry currently loads the entire global standards
table for every buildStandardsRollup call. Refactor readRegistry and its
callers, including getStandardsRollup, to accept the distinct (org_code,
standard_code) pairs from readCitations and filter the SQL query to those keys,
preserving the existing in-memory join behavior and handling an empty key set
without scanning the table.
🪄 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: e9286526-7c05-49b2-a359-a3984707c092
📒 Files selected for processing (18)
docs/adr/064-standards-registry.mdopenapi.yamlsrc/api/contract.integration.test.tssrc/api/router.tssrc/api/standards.integration.test.tssrc/api/standards.tssrc/db/index.tssrc/db/migrations/043_create_standards.tssrc/db/queries/standards-read.integration.test.tssrc/db/queries/standards-read.tssrc/db/queries/standards.test.tssrc/db/queries/standards.tssrc/mcp/capabilities.tssrc/mcp/contract-map.tssrc/mcp/standards-handlers.tssrc/mcp/standards-tools.tssrc/mcp/standards.integration.test.tssrc/mcp/tools.ts
The no-body fallback used req.body ?? {}, which also coerced an explicit JSON
null body to {} (an empty verdict). Narrow it to req.body === undefined so only
a genuinely absent body defaults; anything present is validated. In practice
express.json() strict mode already rejects a top-level null with 400 before the
handler, so this is a backstop for a relaxed-parser config — pinned by a
regression asserting a null body is rejected, never a silent reset.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 3 minutes. |
Resolve append-point conflicts from the merged standards-registry PR (#458, ADR-064) on the shared surfaces both features extend: src/db/index.ts, src/mcp/capabilities.ts, and src/mcp/contract-map.ts — keeping both features' entries (discipline mapping + standards registry), which are disjoint. openapi.yaml, router.ts, tools.ts, and the contract test auto-merged. Trim src/db/index.ts back under the 400-line ESLint cap by exporting only the discipline symbols external consumers use (listDisciplines, replace/clear rule writers, DisciplineNotFoundError); resolveEffectiveRules/disciplineForSection and the resolved-view types stay internal to the db module (the listing queries import them via relative path), so the union of both features' barrel exports fits. Verified on the merged tree: unit 1648/1648; both contract gates (REST↔openapi, REST↔MCP) green with both features' routes/tools; discipline (53) and standards (21) integration suites pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Why
The parser already extracts standards-organization references (ASTM, ANSI, NFPA, …) per paragraph into
spec_references, and the coordination report flags a standard cited in the body but missing from REFERENCES. What the API did not offer is the standard itself as a first-class record: no compiled "every standard cited across this library/project" list, no place to record the current published version / source location, and no way for a reviewing client to persist "verified current on DATE." Spec-editing firms review standards currency on a cadence; a headless API should hand any client the compiled list and accept the verdict back, so subsequent reports cite verification state deterministically instead of re-deriving it.What
standardstable —org_code,standard_code, nullabletitle/current_version/source_url/last_verified_at/notes,statusenum (current | superseded | withdrawn | unknown, defaultunknown), UNIQUE (org_code, standard_code).GET /libraries/{id}/standardsandGET /projects/{id}/standards— each row carries orgCode, standardCode, citation count, citing specs, capped paragraph anchors, and the joined registry verdict (status, current version, source URL, last verified).PUT /standards/{orgCode}/{standardCode}— Zod-validated verdict upsert; server stampslast_verified_at.superseded/withdrawnsurfaces as a rollup finding (standard_superseded/standard_withdrawn) with citing specs + anchors, plus summary counts.list_library_standards/list_project_standards(read tier) +record_standard_verification(write tier), withcontract-map.tsparity entries (ADR-044) and capability tiers (ADR-045).Structure mirrors the ADR-063 reference-graph read model: a pure, DB-free builder (
src/db/queries/standards.ts, unit-tested) fed by a read/upsert layer (src/db/queries/standards-read.ts).Design decisions
Ambiguous calls the issue left open, resolved and documented (full rationale in
docs/adr/064-standards-registry.md):standardsrow (keyed on org+code) is shared across every scope that cites it. The rollups are scope-relative over citations and LEFT JOIN the single registry row.spec_references.standard_codeholds the whole cited string (e.g."ASTM C150"). Both the rollup and the write path split on the first whitespace —orgCode= leading token (uppercased),standardCode= remainder — so a recorded verdict re-joins to its citations exactly. A no-whitespace.SECRID like"ANSI/TIA-568.1"is a documented KNOWN AMBIGUITY (org-only, empty code); DOCX, the product path, always emits"ORG ident".last_verified_atis server-stamped on every PUT — recording a verdict is the verification event; the client never supplies the timestamp. PUT-replace semantics: omitted optional fields reset to null.CoordinationSummaryis a separate optional follow-up.standardCodewith reserved chars (e.g. a slash inA653/A653M) is percent-encoded in the path; a round-trip test proves it survives Express 5 routing.Testing
pnpm test— 1648 passed, incl.standards.test.ts: split, rollup, findings, anchor cap)pnpm lintgreen (eslint +tsc --noEmit+ prettier)PUTverdict reflected in the next rollup as status + finding; percent-encoded slash code round-trips🤖 Co-authored by Claude Fable 5. Closes #446.
Summary by CodeRabbit
current,superseded,withdrawn,unknown) and automatically stamplast_verified_at.