feat(validators): add runtime schema validators (BDK-002)#183
feat(validators): add runtime schema validators (BDK-002)#183flyingrobots merged 7 commits intomainfrom
Conversation
Implements GRAPH_SCHEMA.md constraints as runtime validators: - New src/validators.js with node ID, edge type, confidence, and composite edge validation (self-edge rejection, prefix warnings) - EDGE_TYPES canonical source moved to validators, re-exported by edges - createEdge() now validates IDs, type safety, and self-edges - 42 new validator tests, existing tests updated for schema-valid IDs
- Add Unreleased section to CHANGELOG with validator additions - Update GUIDE.md node ID examples to use prefix:identifier format - Document validator exports in library usage section - Update commit directive examples with schema-valid targets
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR centralizes node/edge schema validation into a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/hooks.test.js (1)
12-56:⚠️ Potential issue | 🔴 CriticalUpdate
parseDirectivestests to use prefixed node ID format.The tests use bare paths (e.g.,
docs/auth-spec.md,src/session.js,api/endpoints.md) that will fail validation whenprocessCommitpasses them tocreateEdge. SincevalidateEdgeenforcesprefix:identifierformat, these directives will throw errors in real usage:
docs/auth-spec.md→ rejected (missing prefix)src/session.js→ rejected (missing prefix)api/endpoints.md→ rejected (missing prefix)Replace bare paths with prefixed formats consistent with the system's node ID grammar:
docs/auth-spec.md→file:docs/auth-spec.mdorspec:authsrc/session.js→file:src/session.jsormodule:authapi/endpoints.md→file:api/endpoints.mdorspec:apiTests like "handles BLOCKS and DEPENDS-ON" already use correct format (
task:deploy,module:auth), so follow that pattern.
🤖 Fix all issues with AI agents
In `@src/edges.js`:
- Line 6: Remove the unused import alias `_EDGE_TYPES` from the import statement
in src/edges.js; keep importing `validateEdge` only (i.e., change `import {
validateEdge, EDGE_TYPES as _EDGE_TYPES } from './validators.js'` to import just
`validateEdge`) since `EDGE_TYPES` is already re-exported later with `export {
EDGE_TYPES } from './validators.js'`, ensuring no other references to
`_EDGE_TYPES` (or renaming) remain in the file.
- Around line 27-31: createEdge currently ignores validateEdge's result.warnings
so unknown-prefix warnings are never surfaced; update createEdge to handle
warnings from validateEdge (the object returned by validateEdge used in
createEdge) by iterating over result.warnings and emitting them (e.g.,
console.warn with a clear message and the warning text) before proceeding or
alternatively include result.warnings in the function's return value so callers
can see them; ensure you reference validateEdge, the result variable, and
result.warnings in your change.
In `@src/validators.js`:
- Around line 15-23: The JSDoc for CANONICAL_PREFIXES is ambiguous about
containing system prefixes; update the comment or the data so intent is clear:
either change the CANONICAL_PREFIXES JSDoc to explicitly state "superset
including system prefixes (includes SYSTEM_PREFIXES such as 'commit')" or
refactor the arrays so CANONICAL_PREFIXES and SYSTEM_PREFIXES are disjoint and
introduce a computed union (e.g., ALL_PREFIXES = [...CANONICAL_PREFIXES,
...SYSTEM_PREFIXES]) used by classifyPrefix; ensure classifyPrefix still checks
SYSTEM_PREFIXES first if you keep the superset approach and update
references/tests accordingly.
- Around line 130-155: The self-edge check in validateEdge runs even when source
or target IDs are invalid, causing redundant/self-edge errors; update
validateEdge so the SELF_EDGE_FORBIDDEN check (the branch that tests source ===
target and SELF_EDGE_FORBIDDEN.includes(type)) only runs when both
validateNodeId(source) and validateNodeId(target) returned valid (use
srcResult.valid && tgtResult.valid) to suppress the self-edge message for
already-invalid IDs while keeping all other validations (validateEdgeType,
validateConfidence) intact.
In `@test/validators.test.js`:
- Around line 102-116: Add an explicit test and inline comment clarifying that
'commit' is classified as 'system' due to prefix precedence: update the test
suite for classifyPrefix (function classifyPrefix in tests) to include a
specific "commit precedence" test that asserts classifyPrefix('commit') ===
'system' and add a short comment referencing CANONICAL_PREFIXES and
SYSTEM_PREFIXES to explain that SYSTEM_PREFIXES is checked first; alternatively,
if the duplication of 'commit' in CANONICAL_PREFIXES is unintended, remove it
from CANONICAL_PREFIXES in src/validators.js so classifyPrefix behaviour matches
intent.
- Remove unused _EDGE_TYPES import alias in edges.js - Surface validateEdge() warnings via console.warn in createEdge() - Separate CANONICAL_PREFIXES (18 user-facing) from SYSTEM_PREFIXES; add ALL_PREFIXES computed union for convenience - Guard self-edge check behind valid IDs to avoid redundant errors - Update parseDirectives tests to use prefixed node IDs - Add classifyPrefix precedence test and self-edge guard test
…#180) The warning originates from roaring → @mapbox/node-pre-gyp calling url.resolve() during native binary loading. Not fixable in our code. Use NODE_OPTIONS to propagate --disable-warning to vitest workers.
- hooks.js: change \s*(.+) to \s*(\S.*) in DIRECTIVE_PATTERN to eliminate polynomial backtracking on whitespace-heavy inputs - ci.yml: add explicit `permissions: contents: read` per least-privilege
|
@coderabbitai review please |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 22-23: The package.json scripts "test" and "test:watch" use
POSIX-only inline NODE_OPTIONS='--disable-warning=DEP0169' which breaks on
Windows; either install and use cross-env and update those scripts to prefix
with cross-env (e.g., cross-env NODE_OPTIONS=--disable-warning=DEP0169) or
remove the inline env and set the same option in Vitest config by adding
test.poolOptions.forks.execArgv = ['--disable-warning=DEP0169'] in
vitest.config.js; choose one approach, update the "test" and "test:watch" script
entries accordingly, and ensure cross-env is added to devDependencies if you
pick that option.
In `@src/validators.js`:
- Around line 50-54: The exported function extractPrefix currently assumes
nodeId is a string and will throw if called with null/undefined or non-string;
add a defensive type guard at the start of extractPrefix (e.g., check typeof
nodeId !== 'string' or nodeId == null) and return null for non-string input so
callers of extractPrefix (and public API consumers) don't get a TypeError; keep
the existing behavior for strings (find ':' and slice) and update any unit tests
to assert null is returned for null/undefined/non-string inputs.
- Add type guard to extractPrefix() for null/undefined/non-string input - Move --disable-warning=DEP0169 from POSIX-only NODE_OPTIONS in package.json to vitest.config.js poolOptions for cross-platform compat - Add extractPrefix non-string input test
- package.json engines: >=22.0.0 - CI matrix: [22, 24] - GUIDE.md prerequisites updated
1 participant
Summary
src/validators.js— runtime enforcement of GRAPH_SCHEMA.md constraints (node ID grammar, edge types, confidence type safety, self-edge rejection, prefix classification)validateEdge()intocreateEdge()so all edges are validated at write timeEDGE_TYPEScanonical source to validators module (backwards-compatible re-export fromedges.js)prefix:identifiernode IDsWhat's validated now
prefix:identifierregex, max 256 charsblocksanddepends-onTest plan
npx vitest rungreenRef: #180
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Tests
Chores