From 3b3b6f990fb21ad7b2c843552258a1e17c9451ff Mon Sep 17 00:00:00 2001 From: Roger Barnes Date: Sun, 1 Mar 2026 21:00:38 +1100 Subject: [PATCH 1/3] feat: executable rules and hierarchy validation (#21, #16) Add two new validation passes that run after JSON schema validation: - **Hierarchy validation** (`validate-hierarchy.ts`): checks parent-child type relationships against the schema's `hierarchy` array, with support for `allowSelfRef` and `allowSkipLevels` metadata flags. - **Executable rules** (`validate-rules.ts`, `evaluate-rule.ts`): evaluates JSONata expressions stored in schema `_metadata.rules` against the node set. Rules are organised into `validation`, `coherence`, and `bestPractice` categories. Violations are grouped by category in the output. Schema changes: - `_strict.json`: coherence rules (active outcome/opportunity count) and a best-practice rule (solution quantity per opportunity) with JSONata checks. - `general.json`: adds `aliases` and `allowSelfRef` metadata. - `strict_ost.json`: removes redundant local `_metadata` (now inherited from `_strict`), tightens `assumption_test` type to `const`. - `loadHierarchy` replaced by `loadMetadata` which returns the full metadata object including aliases, allowSelfRef, and rules. - `resolveNodeType` added for alias resolution; `resolvedType` field added to `SpaceNode` and populated at parse time. Closes #21, #16 --- CLAUDE.md | 4 + bun.lock | 3 + docs/concepts.md | 6 +- docs/rules.md | 92 +++++ docs/schemas.md | 61 +++- package.json | 1 + schemas/_strict.json | 42 +-- schemas/general.json | 8 +- schemas/strict_ost.json | 16 +- src/commands/validate.ts | 56 ++- src/evaluate-rule.ts | 98 ++++++ src/parse-embedded.ts | 33 +- src/read-space-directory.ts | 9 +- src/read-space-on-a-page.ts | 8 +- src/schema.ts | 50 +-- src/types.ts | 47 +++ src/validate-hierarchy.ts | 57 ++++ src/validate-rules.ts | 59 ++++ tests/evaluate-rule.test.ts | 165 +++++++++ .../invalid/experiment-invalid-category.md | 4 +- .../invalid-experiment-no-assumption.md | 4 +- .../valid-directory/Usability test.md | 2 +- tests/validate-hierarchy.test.ts | 170 ++++++++++ tests/validate-rules.test.ts | 318 ++++++++++++++++++ tests/validate-strict.test.ts | 32 +- 25 files changed, 1235 insertions(+), 110 deletions(-) create mode 100644 docs/rules.md create mode 100644 src/evaluate-rule.ts create mode 100644 src/validate-hierarchy.ts create mode 100644 src/validate-rules.ts create mode 100644 tests/evaluate-rule.test.ts create mode 100644 tests/validate-hierarchy.test.ts create mode 100644 tests/validate-rules.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 7cfdf26..abcc078 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,5 +31,9 @@ Before starting new work, review [docs/concepts.md](docs/concepts.md) for canoni - `bun run test` — unit tests (fixtures in `tests/`) - `bun run test:smoke` — smoke tests that run `validate` against every space in `config.json` (`smoke/`) +## Debugging + +- `bun run src/index.ts dump ` — Output parsed node data with resolved parents, useful for debugging rule violations + ## Hooks A Stop hook runs linting, autoformatting and unit tests. If it reports issues related to change you made, address them. \ No newline at end of file diff --git a/bun.lock b/bun.lock index 3b9aa53..51d75e3 100644 --- a/bun.lock +++ b/bun.lock @@ -10,6 +10,7 @@ "glob": "^13.0.6", "gray-matter": "^4.0.3", "js-yaml": "^4.1.1", + "jsonata": "^2.1.0", "jsonc-parser": "^3.3.1", "mdast-util-to-string": "^4.0.0", "remark-gfm": "^4.0.1", @@ -115,6 +116,8 @@ "json-schema-traverse": ["json-schema-traverse@1.0.0", "", {}, "sha512-NM8/P9n3XjXhIZn1lLhkFaACTOURQXjWhV4BA/RnOv8xvgqtqpAX9IO4mRQxSx1Rlo4tqzeqb0sOlruaOy3dug=="], + "jsonata": ["jsonata@2.1.0", "", {}, "sha512-OCzaRMK8HobtX8fp37uIVmL8CY1IGc/a6gLsDqz3quExFR09/U78HUzWYr7T31UEB6+Eu0/8dkVD5fFDOl9a8w=="], + "jsonc-parser": ["jsonc-parser@3.3.1", "", {}, "sha512-HUgH65KyejrUFPvHFPbqOY0rsFip3Bo5wb4ngvdi1EpCYWUQDC5V+Y7mZws+DLkr4M//zQJoanu1SP+87Dv1oQ=="], "kind-of": ["kind-of@6.0.3", "", {}, "sha512-dcS1ul+9tmeD95T+x28/ehLgd9mENa3LsvDTtzm3vyBEO7RPptvAD+t44WVXaUjTBRcrpFeFlC8WCruUR456hw=="], diff --git a/docs/concepts.md b/docs/concepts.md index 910083d..c3da7a0 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -38,8 +38,6 @@ Key properties: - Heading levels must not skip — each level must be exactly one deeper than its parent. - A horizontal rule (`---`) terminates parsing; headings below it are ignored. -> The name "OST on a page" may be revised as the tooling moves toward space-centric terminology — see [GitHub issue #22](https://github.com/mindsocket/ost-tools/issues/22). - #### Preamble **Preamble** is content in a `space on a page` document that appears before the first heading. It is parsed but discarded — not associated with any node. @@ -80,7 +78,7 @@ A **schema** defines the valid structure for nodes in a `space`: the fields, typ The schema handles structural validation. It does not encode qualitative or cross-node checks — those are handled by `rules`, which may be embedded within the schema or applied separately. -Schemas are designed to be composable: shared building blocks (common field sets, scoring models, constraint overlays) can be referenced across schema files, letting teams tailor a schema without forking their foundations. *(Schema composability is under active development — see [GitHub issues #13](https://github.com/mindsocket/ost-tools/issues/13), [#17](https://github.com/mindsocket/ost-tools/issues/17).)* +Schemas are designed to be composable: shared building blocks (common field sets, scoring models, constraint overlays) can be referenced across schema files, letting teams tailor a schema without forking their foundations. *(Schema composability is under development — see [GitHub issue #17](https://github.com/mindsocket/ost-tools/issues/17).)* ### Rules @@ -98,7 +96,7 @@ Rules may be: Rules are distinct from schema validation: the schema checks structure; rules check meaning and quality. -*(Rules support is planned — see [GitHub issue #16](https://github.com/mindsocket/ost-tools/issues/16).)* +See [docs/rules.md](rules.md) for the rules reference, including JSONata expression syntax and the full `_metadata` field reference. --- diff --git a/docs/rules.md b/docs/rules.md new file mode 100644 index 0000000..9e108ff --- /dev/null +++ b/docs/rules.md @@ -0,0 +1,92 @@ +# Executable Rules + +Rules are JSONata expressions embedded in a schema's `_metadata.rules` block. Each rule is evaluated against applicable nodes at validation time and must return `true` to pass. Rules encode checks that JSON Schema structural validation cannot express — cross-node consistency, quantitative thresholds, and qualitative best practices. + +For how rules fit into the broader schema metadata, see [docs/schemas.md](schemas.md). + +## Rule Categories + +Rules are grouped into three categories under `_metadata.rules`. + +| Category | Purpose | +|---|---| +| `validation` | Structural correctness — a violation means the node is incorrect and should be fixed | +| `coherence` | Cross-node checks — for flagging conflicts or contradictions (often combined with `scope: 'global'` for multi-node/aggregate checks) | +| `bestPractice` | Advisory guidance — signals the space may benefit from additional work | + +## Rule Object Structure + +| Field | Required | Description | +|---|---|---| +| `id` | yes | Unique identifier (kebab-case) | +| `description` | yes | Human-readable description of what the rule checks | +| `check` | yes | JSONata expression that must evaluate to `true` to pass | +| `type` | no | If set, only applies to nodes of this resolved type | +| `scope` | no | Set to `'global'` to evaluate the rule once against the full node set | + +Rules without `scope: 'global'` are evaluated once per applicable node (all nodes, or only those matching `type`). A global rule is evaluated once and produces at most one violation for the space — use this for aggregate checks, like counts, across all nodes. + +## JSONata Expression Context + +Each expression is evaluated once per applicable node with the following input: + +| Variable | Description | +|---|---| +| `nodes` | Array of all nodes in the space | +| `current` | The node being evaluated | +| `parent` | The resolved parent node — absent if no parent was resolved | + +Nodes include all node properties (title, type, status, parent wikilink, etc.) plus two resolved fields: `resolvedType` (canonical type after alias resolution) and `resolvedParentTitle` (parent title after resolving any links). + +Prefer `resolvedType` over `type` for type comparisons. When aliases are in use, `type` reflects the raw frontmatter value and may not match canonical names. + +### Referencing `current` inside predicates + +Inside a predicate (`nodes[...]`), bare names refer to fields on each item. Use `$$` (JSONata root) to reach outer-scope variables: + +```jsonata +// Count solutions whose parent title matches the current node's title +$count(nodes[resolvedParentTitle=$$.current.title and resolvedType='solution']) +``` + +### `parent` vs `current.parent` + +- `parent` — the resolved parent **node object**; absent if the parent was not found in the space +- `current.parent` — the raw wikilink string from frontmatter (e.g. `[[My Outcome]]`) + +Use `$exists(parent)` to test whether the current node has a resolved parent: + +```jsonata +$exists(parent) = false // true for root nodes +``` + +## Examples + +```json +{ + "coherence": [ + { + "id": "active-outcome-count", + "description": "Only one outcome should be active at a time", + "scope": "global", + "check": "$count(nodes[resolvedType='outcome' and status='active']) <= 1" + }, + { + "id": "active-opportunity-count", + "description": "Only one target opportunity should be active at a time", + "scope": "global", + "check": "$count(nodes[resolvedType='opportunity' and status='active']) <= 1" + } + ], + "bestPractice": [ + { + "id": "solution-quantity", + "description": "Explore multiple candidate solutions (aim for at least three) for the target opportunity", + "type": "opportunity", + "check": "$count(nodes[resolvedParentTitle=$$.current.title and resolvedType='solution']) >= 3" + } + ] +} +``` + +The coherence rules run against every node (no `type` filter) — each node in a space with two active outcomes will report a violation. The best-practice rule only runs against `opportunity` nodes, using `resolvedParentTitle` to count child solutions. \ No newline at end of file diff --git a/docs/schemas.md b/docs/schemas.md index 616bbbc..5c05c8f 100644 --- a/docs/schemas.md +++ b/docs/schemas.md @@ -43,7 +43,7 @@ A flexible, opinionated schema supporting a multi-level strategy hierarchy along **Features:** - Allows `vision`, `mission`, `goal` hierarchy for strategic planning - Optional numeric assessment fields (1-5 scale) for opportunities and solutions -- Type aliases: `goal` and `outcome` are accepted for the same entity type +- Type aliases: alternative terms accepted for some types - `additionalProperties: true` allows extensibility **Use when:** @@ -58,18 +58,12 @@ A schema following the canonical 4-level Opportunity Solution Tree structure, ba - `outcome` — Root-level outcome (product metric, no parent) - `opportunity` — Customer pain points, desires, and needs (can be nested) - `solution` — Solutions to explore for target opportunities -- `experiment`|`assumption_test`|`test` — Assumption tests for solutions - -**Structure:** -- `outcome` cannot have a parent (it's the root) -- `opportunity` can have `outcome` or another `opportunity` as parent (nested hierarchy) -- `solution` must have an `opportunity` parent -- `experiment`|`assumption_test`|`test` must have a `solution` parent +- `assumption_test` — Assumption tests for solutions **Fields:** -- `outcome` includes optional `metric` field for the product metric -- `opportunity` includes optional `source` field to track research origin -- `experiment` includes required `assumption` field and optional `category` enum +- `outcome` requires a `metric` field for the product metric +- `opportunity` requires a `source` field to track research origin +- `assumption_test` requires an `assumption` field and accepts an optional `category` **Use when:** - You want to follow Teresa Torres' OST methodology strictly @@ -93,9 +87,46 @@ Common definitions used across multiple schemas: Shared definitions specific to the strict OST schema: -- `OutcomeProps` — Outcome-specific properties (metric) +- `outcomeProps` — Outcome-specific properties (metric) - `opportunityProps` — Opportunity properties (source) -- `experimentProps` — Experiment properties (assumption, category) +- `assumptionTestProps` — Assumption test properties (assumption, category) +- `_metadata` — Hierarchy, type aliases, and executable rules for strict OST validation + +## Schema Metadata + +The `_metadata` block in `$defs` carries non-structural validation configuration. It is not a JSON Schema construct — the tooling reads it separately from the schema validator. + +```jsonc +{ + "$defs": { + "_metadata": { + "hierarchy": ["outcome", "opportunity", "solution", "assumption_test"], + "aliases": { "test": "assumption_test" }, + "allowSelfRef": ["opportunity"], + "allowSkipLevels": false, + "rules": { ... } + } + } +} +``` + +| Field | Type | Description | +|---|---|---| +| `hierarchy` | `string[]` | Ordered list of canonical types from root to leaf | +| `aliases` | `Record` | Maps alternative type names to canonical types | +| `allowSelfRef` | `string[]` | Types that may have a parent of the same type (e.g. nested opportunities) | +| `allowSkipLevels` | `boolean` | When `true`, a node may have any ancestor type above it, not just the immediate parent | +| `rules` | `object` | Executable validation rules — see [docs/rules.md](rules.md) | + +### Hierarchy validation + +The validator checks every node type and its parent type against the hierarchy order, with violations flagged. + +`allowSelfRef` and `allowSkipLevels` modify the strictness. For example, `"allowSelfRef": ["opportunity"]` permits nested opportunity trees. + +### Type aliases + +`aliases` maps alternative type names to canonical types. A node with `type: outcome` and `"aliases": { "outcome": "goal" }` will have `resolvedType: goal` and be treated as a `goal` everywhere — in hierarchy checks, rule type filters, and output. ## Schema Composability @@ -105,6 +136,8 @@ Schemas are designed to be composable. You can create custom schemas by: 2. Using `$ref` to reference shared definitions from `_shared.json` or other schemas 3. Defining your own node types and constraints +Referencing another schema file merges its `$defs` into the compiled schema, including any `_metadata` block. If multiple referenced files each define `_metadata`, only the last one merged is used — `rules` arrays are not combined across sources. + Example of referencing shared definitions: ```jsonc @@ -134,6 +167,6 @@ Schema files support JSONC (JSON with Comments) format, allowing inline document ## Further Reading -- [ Teresa Torres' work on Opportunity Solution Trees](https://producttalk.org/2021/02/using-opportunity-solution-trees/) +- [Teresa Torres' work on Opportunity Solution Trees](https://producttalk.org/2021/02/using-opportunity-solution-trees/) - "Continuous Discovery Habits" (2021) by Teresa Torres - [JSON Schema specification](https://json-schema.org/) \ No newline at end of file diff --git a/package.json b/package.json index 939c7a0..8c415bc 100644 --- a/package.json +++ b/package.json @@ -38,6 +38,7 @@ "glob": "^13.0.6", "gray-matter": "^4.0.3", "js-yaml": "^4.1.1", + "jsonata": "^2.1.0", "jsonc-parser": "^3.3.1", "mdast-util-to-string": "^4.0.0", "remark-gfm": "^4.0.1", diff --git a/schemas/_strict.json b/schemas/_strict.json index 22c2415..43933ba 100644 --- a/schemas/_strict.json +++ b/schemas/_strict.json @@ -6,26 +6,30 @@ // "Continuous Discovery Habits" (2021) and at producttalk.org. "$defs": { "_metadata": { - "hierarchy": ["outcome", "opportunity", "solution", "experiment"], + "hierarchy": ["outcome", "opportunity", "solution", "assumption_test"], + "allowSelfRef": ["opportunity"], "rules": { - "general": [ - "Only one outcome should be active at a time", - "Only one target opportunity should be active at a time" + "coherence": [ + { + "id": "active-outcome-count", + "description": "Only one outcome should be active at a time", + "scope": "global", + "check": "$count(nodes[resolvedType='outcome' and status='active']) <= 1" + }, + { + "id": "active-opportunity-count", + "description": "Only one target opportunity should be active at a time", + "scope": "global", + "check": "$count(nodes[resolvedType='opportunity' and status='active']) <= 1" + } ], - "outcome": ["Should be a specific product metric with a directional component (e.g., 'increase X')"], - "opportunity": [ - "Frame in the customer's voice — something a customer would actually say", - "Must be in the problem space (addressable by more than one solution)", - "Must be grounded in customer research (use the 'source' field to track origin)" - ], - "solution": [ - "Parent must be an opportunity (not another solution)", - "Explore multiple candidate solutions (aim for at least three) for the target opportunity before committing to one" - ], - "experiment": [ - "Parent must be a solution", - "Tests a single assumption — not the whole idea", - "Run tests on the riskiest assumption for each solution candidate" + "bestPractice": [ + { + "id": "solution-quantity", + "description": "Explore multiple candidate solutions (aim for at least three) for the target opportunity", + "type": "opportunity", + "check": "$count(nodes[resolvedParentTitle=$$.current.title and resolvedType='solution']) >= 3" + } ] } }, @@ -51,7 +55,7 @@ }, "required": ["source"] }, - "experimentProps": { + "assumptionTestProps": { "type": "object", "description": "Properties for an Assumption Test (the fourth level of the OST)", "properties": { diff --git a/schemas/general.json b/schemas/general.json index 3e7ac07..aac4f02 100644 --- a/schemas/general.json +++ b/schemas/general.json @@ -5,7 +5,13 @@ "description": "Validates frontmatter for space node files", "$defs": { "_metadata": { - "hierarchy": ["vision", "mission", "goal", "opportunity", "solution", "experiment"] + "hierarchy": ["vision", "mission", "goal", "opportunity", "solution", "experiment"], + "aliases": { + "outcome": "goal", + "assumption_test": "experiment", + "test": "experiment" + }, + "allowSelfRef": ["goal", "opportunity", "solution"] } }, "oneOf": [ diff --git a/schemas/strict_ost.json b/schemas/strict_ost.json index 6828866..e723bfc 100644 --- a/schemas/strict_ost.json +++ b/schemas/strict_ost.json @@ -6,11 +6,6 @@ // and at producttalk.org. // Structure: outcome → opportunity → solution → assumption test "description": "Validates frontmatter for space node files following the canonical 4-level OST structure: outcome → opportunity → solution → assumption test", - "$defs": { - "_metadata": { - "hierarchy": ["outcome", "opportunity", "solution", "experiment"] - } - }, "oneOf": [ { "type": "object", @@ -86,10 +81,10 @@ "allOf": [ { "$ref": "ost-tools://_shared#/$defs/baseNodeProps" }, { "$ref": "ost-tools://_shared#/$defs/ostEntityProps" }, - { "$ref": "ost-tools://_strict#/$defs/experimentProps" } + { "$ref": "ost-tools://_strict#/$defs/assumptionTestProps" } ], "properties": { - "type": { "enum": ["experiment", "assumption_test", "test"] }, + "type": { "const": "assumption_test" }, "parent": { "$ref": "ost-tools://_shared#/$defs/wikilink", "description": "Parent solution (wikilink)" @@ -104,13 +99,6 @@ "parent": "[[Simplify Signup Flow]]", "assumption": "Users can complete the simplified signup in under 2 minutes", "category": "usability" - }, - { - "type": "test", - "status": "active", - "parent": "[[Prototype Landing Page]]", - "assumption": "Users will understand the value proposition within 5 seconds", - "category": "desirability" } ] } diff --git a/src/commands/validate.ts b/src/commands/validate.ts index 41f58fa..fde5bed 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -3,14 +3,18 @@ import type { ErrorObject } from 'ajv'; import { readSpaceDirectory } from '../read-space-directory'; import { readSpaceOnAPage } from '../read-space-on-a-page'; import { wikilinkToTarget } from '../resolve-links'; -import { createValidator } from '../schema'; -import type { SpaceNode } from '../types'; +import { createValidator, loadMetadata } from '../schema'; +import type { HierarchyViolation, RuleViolation, SpaceNode } from '../types'; +import { validateHierarchy } from '../validate-hierarchy'; +import { validateRules } from '../validate-rules'; interface ValidationResult { schemaValidCount: number; schemaErrorCount: number; schemaErrors: Array<{ file: string; errors: ErrorObject[] }>; refErrors: Array<{ file: string; parent: string; error: string }>; + ruleViolations: RuleViolation[]; + hierarchyViolations: HierarchyViolation[]; skipped: string[]; nonSpace: string[]; } @@ -33,6 +37,8 @@ export async function validate(path: string, options: { schema: string }): Promi schemaErrorCount: 0, schemaErrors: [], refErrors: [], + ruleViolations: [], + hierarchyViolations: [], skipped, nonSpace: nonSpace, }; @@ -80,12 +86,23 @@ export async function validate(path: string, options: { schema: string }): Promi } } + // Load and execute hierarchy validation if schema defines hierarchy + const metadata = loadMetadata(options.schema); + result.hierarchyViolations = validateHierarchy(nodes, metadata); + + // Load and execute rules validation if schema defines rules + if (metadata.rules) { + result.ruleViolations = await validateRules(nodes, metadata.rules); + } + // Report console.log(`\n🔍 Space Validation Results`); console.log(`━`.repeat(50)); console.log(`✅ Valid: ${result.schemaValidCount}`); console.log(`❌ Schema Errors: ${result.schemaErrorCount}`); console.log(`🔗 Reference Errors: ${result.refErrors.length}`); + console.log(`📋 Rule Violations: ${result.ruleViolations.length}`); + console.log(`🏗️ Hierarchy Violations: ${result.hierarchyViolations.length}`); console.log(`⏭ Skipped (no frontmatter): ${result.skipped.length}`); console.log(`📄 Non-space (no type field): ${result.nonSpace.length}`); @@ -116,9 +133,42 @@ export async function validate(path: string, options: { schema: string }): Promi }); } + if (result.ruleViolations.length > 0) { + console.log(`\n📋 Rule violations:`); + + // Group by category + const byCategory = new Map(); + for (const v of result.ruleViolations) { + if (!byCategory.has(v.category)) { + byCategory.set(v.category, []); + } + byCategory.get(v.category)!.push(v); + } + + // Report each category + for (const [category, violations] of byCategory) { + console.log(` ${category.toUpperCase()} (${violations.length}):`); + for (const v of violations) { + console.log(` ${v.file ? `${v.file}: ` : ''}${v.description}`); + } + } + } + + if (result.hierarchyViolations.length > 0) { + console.log(`\n🏗️ Hierarchy violations:`); + for (const v of result.hierarchyViolations) { + console.log(` ${v.file}: ${v.description}`); + } + } + console.log(`\n`); - if (result.schemaErrorCount > 0 || result.refErrors.length > 0) { + if ( + result.schemaErrorCount > 0 || + result.refErrors.length > 0 || + result.ruleViolations.length > 0 || + result.hierarchyViolations.length > 0 + ) { process.exit(1); } } diff --git a/src/evaluate-rule.ts b/src/evaluate-rule.ts new file mode 100644 index 0000000..da31010 --- /dev/null +++ b/src/evaluate-rule.ts @@ -0,0 +1,98 @@ +import jsonata from 'jsonata'; +import type { SpaceNode } from './types'; + +const expressionCache = new Map>(); + +/** Evaluation context for JSONata expressions */ +export interface EvalContext { + /** All nodes in the space (flattened) */ + nodes: Record[]; + /** Current node being evaluated (flattened) */ + $$: Record; + /** Resolved parent node (undefined if no parent) (flattened) */ + parent?: Record; +} + +/** + * Evaluate a JSONata expression against a context. + * Returns the result of the expression (boolean for rule checks). + * + * @param expr - JSONata expression string + * @param context - Evaluation context with $, $$, parent, $lookup + * @returns Result of expression evaluation + */ +export async function evaluateExpression(expr: string, context: EvalContext): Promise { + // Build a structured input object for JSONata + // This allows expressions to access nodes, current, and parent as properties + const input: Record = { + nodes: context.nodes, + current: context.$$, + }; + + // Only add parent if it exists (not undefined) + if (context.parent !== undefined) { + input.parent = context.parent; + } + + try { + let expression = expressionCache.get(expr); + if (!expression) { + expression = jsonata(expr); + expressionCache.set(expr, expression); + } + // Pass the structured input - expressions access nodes, current, parent from it + const result = await expression.evaluate(input); + return result as boolean | string | number; + } catch (error) { + // Log warning and return false (fail safe) + console.warn(`Warning: Error evaluating expression "${expr}":`, error); + return false; + } +} + +/** + * Flatten a SpaceNode for JSONata evaluation. + * Creates a new object with schemaData properties at the top level. + * + * @param node - The node to flatten + * @returns Flattened node with properties directly accessible + */ +function flattenNode(node: SpaceNode): Record { + return { + ...node.schemaData, + resolvedType: node.resolvedType, + resolvedParentTitle: node.resolvedParent, + }; +} + +/** + * Build an evaluation context for a given node. + * + * @param node - The node to build context for + * @param allNodes - All nodes in the space + * @param nodeIndex - Map of node titles to nodes for efficient lookup + * @returns Evaluation context for the node + */ +export function buildEvalContext( + node: SpaceNode, + allNodes: SpaceNode[], + nodeIndex: Map, +): EvalContext { + // Flatten all nodes for JSONata access + const flattenedNodes = allNodes.map(flattenNode); + + // Find parent node if resolvedParent is set + let flattenedParent: Record | undefined; + if (node.resolvedParent) { + const parentNode = nodeIndex.get(node.resolvedParent); + if (parentNode) { + flattenedParent = flattenNode(parentNode); + } + } + + return { + nodes: flattenedNodes, + $$: flattenNode(node), + parent: flattenedParent, + }; +} diff --git a/src/parse-embedded.ts b/src/parse-embedded.ts index 987a66b..3a624a2 100644 --- a/src/parse-embedded.ts +++ b/src/parse-embedded.ts @@ -4,6 +4,7 @@ import { toString as mdastToString } from 'mdast-util-to-string'; import remarkGfm from 'remark-gfm'; import remarkParse from 'remark-parse'; import { unified } from 'unified'; +import { resolveNodeType } from './schema'; import type { SpaceNode, SpaceOnAPageDiagnostics } from './types'; /** Type values that identify a space_on_a_page container (not themselves space nodes). */ @@ -127,6 +128,7 @@ function processListItem( nodes: SpaceNode[], makeLabel: (title: string) => string, buildLinkTargets: (title: string) => string[], + aliases: Record, ): void { const firstPara = item.children.find((c) => c.type === 'paragraph') as Paragraph | undefined; @@ -153,14 +155,19 @@ function processListItem( if (summary) schemaData.summary = summary; const linkTargets = buildLinkTargets(title); - const newNode: SpaceNode = { label: makeLabel(title), schemaData, linkTargets }; + const newNode: SpaceNode = { + label: makeLabel(title), + schemaData, + linkTargets, + resolvedType: resolveNodeType(fields.type, aliases), + }; nodes.push(newNode); const nestedParentRef = `[[${linkTargets[0] ?? title}]]`; for (const child of item.children) { if (child.type === 'list') { for (const subItem of (child as List).children) { - processListItem(subItem, nestedParentRef, newNode, nodes, makeLabel, buildLinkTargets); + processListItem(subItem, nestedParentRef, newNode, nodes, makeLabel, buildLinkTargets, aliases); } } } @@ -187,6 +194,10 @@ export interface ExtractEmbeddedOptions { * Hierarchy of node types for depth-based type inference in space-on-a-page mode. */ hierarchy: readonly string[]; + /** + * Type aliases mapping (alias -> canonical type) for resolving types. + */ + aliases?: Record; } export interface ExtractEmbeddedResult { @@ -201,12 +212,17 @@ export interface ExtractEmbeddedResult { * (directory) to find embedded sub-nodes within a page's content. */ export function extractEmbeddedNodes(body: string, options: ExtractEmbeddedOptions): ExtractEmbeddedResult { - const { pageTitle, pageType, hierarchy } = options; + const { pageTitle, pageType, hierarchy, aliases = {} } = options; const isOnAPageMode = pageType === undefined || ON_A_PAGE_TYPES.includes(pageType); const nodes: SpaceNode[] = []; // Preamble/root content sink - never added to nodes - const rootNode: SpaceNode = { label: '_root_', schemaData: { type: 'space_on_a_page' }, linkTargets: [] }; + const rootNode: SpaceNode = { + label: '_root_', + schemaData: { type: 'space_on_a_page' }, + linkTargets: [], + resolvedType: 'space_on_a_page', + }; const tree = unified().use(remarkParse).use(remarkGfm).parse(body) as Root; @@ -334,7 +350,12 @@ export function extractEmbeddedNodes(body: string, options: ExtractEmbeddedOptio if (parentRef) schemaData.parent = parentRef; const linkTargets = buildHeadingLinkTargets(rawText, title, anchor); - const headingNode: SpaceNode = { label: makeLabel(title), schemaData, linkTargets }; + const headingNode: SpaceNode = { + label: makeLabel(title), + schemaData, + linkTargets, + resolvedType: resolveNodeType(type, aliases), + }; nodes.push(headingNode); currentContextNode = headingNode; @@ -345,7 +366,7 @@ export function extractEmbeddedNodes(body: string, options: ExtractEmbeddedOptio } else if (child.type === 'list') { const parentRef = currentParentRef(); for (const item of (child as List).children) { - processListItem(item, parentRef, currentContextNode, nodes, makeLabel, buildListItemLinkTargets); + processListItem(item, parentRef, currentContextNode, nodes, makeLabel, buildListItemLinkTargets, aliases); } } else if (child.type === 'paragraph') { const rawText = mdastToString(child); diff --git a/src/read-space-directory.ts b/src/read-space-directory.ts index 72d39d4..8f6abe0 100644 --- a/src/read-space-directory.ts +++ b/src/read-space-directory.ts @@ -5,7 +5,7 @@ import matter from 'gray-matter'; import { loadConfig, resolveSchema } from './config'; import { extractEmbeddedNodes, ON_A_PAGE_TYPES } from './parse-embedded'; import { resolveParentLinks } from './resolve-links'; -import { loadHierarchy } from './schema'; +import { loadMetadata, resolveNodeType } from './schema'; import type { SpaceDirectoryReadResult, SpaceNode } from './types'; export async function readSpaceDirectory( @@ -17,10 +17,9 @@ export async function readSpaceDirectory( const skipped: string[] = []; const nonSpace: string[] = []; - // Resolve schema and load hierarchy for depth-based type inference const config = loadConfig(); const resolvedSchemaPath = resolveSchema(options?.schemaPath, config); - const hierarchyArray = loadHierarchy(resolvedSchemaPath); + const { hierarchy, aliases } = loadMetadata(resolvedSchemaPath); for (const file of files) { const content = readFileSync(join(directory, file), 'utf-8'); @@ -47,6 +46,7 @@ export async function readSpaceDirectory( label: file, schemaData: { title: fileBase, ...parsed.data }, linkTargets: [fileBase], + resolvedType: resolveNodeType(pageType, aliases), }); // Extract embedded child nodes from the page body (typed pages with embedded nodes). @@ -55,7 +55,8 @@ export async function readSpaceDirectory( const { nodes: embedded } = extractEmbeddedNodes(parsed.content, { pageTitle: fileBase, pageType, - hierarchy: hierarchyArray, + hierarchy, + aliases, }); nodes.push(...embedded); } diff --git a/src/read-space-on-a-page.ts b/src/read-space-on-a-page.ts index 140cfef..4860268 100644 --- a/src/read-space-on-a-page.ts +++ b/src/read-space-on-a-page.ts @@ -4,7 +4,7 @@ import matter from 'gray-matter'; import { loadConfig, resolveSchema } from './config'; import { extractEmbeddedNodes, ON_A_PAGE_TYPES } from './parse-embedded'; import { resolveParentLinks } from './resolve-links'; -import { loadHierarchy } from './schema'; +import { loadMetadata } from './schema'; import type { SpaceOnAPageReadResult } from './types'; export function readSpaceOnAPage(filePath: string, schemaPath?: string): SpaceOnAPageReadResult { @@ -19,16 +19,16 @@ export function readSpaceOnAPage(filePath: string, schemaPath?: string): SpaceOn ); } - // Resolve schema and load hierarchy for depth-based type inference const config = loadConfig(); const resolvedSchemaPath = resolveSchema(schemaPath, config); - const hierarchyArray = loadHierarchy(resolvedSchemaPath); + const { hierarchy, aliases } = loadMetadata(resolvedSchemaPath); const pageTitle = basename(filePath, '.md'); const { nodes, diagnostics } = extractEmbeddedNodes(body, { pageTitle, pageType: 'space_on_a_page', - hierarchy: hierarchyArray, + hierarchy, + aliases, }); resolveParentLinks(nodes); return { nodes, diagnostics }; diff --git a/src/schema.ts b/src/schema.ts index bc2b99d..ce99887 100644 --- a/src/schema.ts +++ b/src/schema.ts @@ -3,6 +3,7 @@ import { dirname, join, resolve } from 'node:path'; import type { AnySchemaObject, SchemaObject } from 'ajv'; import Ajv, { type ValidateFunction } from 'ajv'; import { parse } from 'jsonc-parser'; +import type { RulesMetadata, SchemaMetadata } from './types'; /** Parsed JSON schema object — always a plain object (never a boolean schema). */ type JsonSchemaObject = Record; @@ -75,34 +76,9 @@ export function createValidator(schemaPath: string): ValidateFunction { return ajv.compile(targetSchema); } -/** - * Extract the hierarchy array from a schema's $defs._metadata.hierarchy. - * Used by read-space-directory and read-space-on-a-page for depth-based type inference. - * - * @throws {Error} If schema doesn't define a hierarchy array - */ -export function loadHierarchy(schemaPath: string): string[] { - const schema = loadSchema(schemaPath); - const metadata = (schema.$defs as Record)?._metadata as Record | undefined; - const hierarchyArray = (metadata?.hierarchy as string[]) ?? []; - - if (!Array.isArray(hierarchyArray) || hierarchyArray.length === 0) { - throw new Error( - `Schema at ${schemaPath} must define "$defs._metadata.hierarchy" array for depth-based type inference`, - ); - } - - return hierarchyArray; -} - /** * Resolve a $ref within a schema, handling both external refs (ost-tools://...) and internal refs (#/$defs/...). * Used by template-sync for traversing schema structures. - * - * @param propDef - A property definition that may contain a $ref - * @param schema - The root schema object (for internal refs) - * @param registry - Schema registry for external ref resolution - * @returns The resolved schema object, or undefined if no $ref was present */ export function resolveRef( propDef: AnySchemaObject | undefined, @@ -136,3 +112,27 @@ export function resolveRef( } return propDef; } + +export function resolveNodeType(type: string, aliases: Record | undefined): string { + return aliases?.[type] ?? type; +} + +export function loadMetadata(schemaPath: string): SchemaMetadata { + const schema = loadSchema(schemaPath); + const metadata = (schema.$defs as Record)?._metadata as Record | undefined; + + const hierarchy = (metadata?.hierarchy as string[]) ?? undefined; + if (!hierarchy || hierarchy.length === 0) { + throw new Error( + `Schema at ${schemaPath} must define "$defs._metadata.hierarchy" array for depth-based type inference`, + ); + } + + return { + hierarchy, + aliases: (metadata?.aliases as Record) ?? undefined, + allowSkipLevels: (metadata?.allowSkipLevels as boolean) ?? undefined, + allowSelfRef: (metadata?.allowSelfRef as string[]) ?? undefined, + rules: (metadata?.rules as RulesMetadata) ?? undefined, + }; +} diff --git a/src/types.ts b/src/types.ts index b693c77..8247709 100644 --- a/src/types.ts +++ b/src/types.ts @@ -7,6 +7,8 @@ export interface SpaceNode { linkTargets: string[]; /** Resolved canonical parent title (derived from schemaData.parent + linkTargets). */ resolvedParent?: string; + /** Resolved canonical type (after applying type aliases from schema metadata). */ + resolvedType: string; } export interface SpaceOnAPageDiagnostics { @@ -26,3 +28,48 @@ export interface SpaceDirectoryReadResult { skipped: string[]; // files with no frontmatter nonSpace: string[]; // files with frontmatter but no type field } + +/** Rule categories for organizing executable validation rules */ +export type RuleCategory = 'validation' | 'coherence' | 'best-practice'; + +/** A single executable rule with JSONata check expression */ +export interface Rule { + id: string; + description: string; + /** JSONata expression that evaluates to boolean (true = pass) */ + check: string; + /** If set, only applies to nodes of this resolved type */ + type?: string; + /** If 'global', evaluated once against the full node set rather than per node */ + scope?: 'global'; +} + +export interface RulesMetadata { + validation?: Rule[]; + coherence?: Rule[]; + bestPractice?: Rule[]; +} + +export interface RuleViolation { + file: string; + ruleId: string; + category: RuleCategory; + description: string; +} + +export interface HierarchyViolation { + file: string; + nodeType: string; + nodeTitle: string; + parentType: string; + parentTitle: string; + description: string; +} + +export interface SchemaMetadata { + hierarchy: string[]; + aliases?: Record; + allowSkipLevels?: boolean; + allowSelfRef?: string[]; + rules?: RulesMetadata; +} diff --git a/src/validate-hierarchy.ts b/src/validate-hierarchy.ts new file mode 100644 index 0000000..fe4a1f8 --- /dev/null +++ b/src/validate-hierarchy.ts @@ -0,0 +1,57 @@ +import type { HierarchyViolation, SchemaMetadata, SpaceNode } from './types'; + +export function validateHierarchy(nodes: SpaceNode[], metadata: SchemaMetadata): HierarchyViolation[] { + const violations: HierarchyViolation[] = []; + const { hierarchy, allowSkipLevels = false, allowSelfRef = [] } = metadata; + + const nodeIndex = new Map(); + for (const node of nodes) { + const title = node.schemaData.title as string; + if (title) { + nodeIndex.set(title, node); + } + } + + for (const node of nodes) { + const nodeType = node.resolvedType; + const nodeTitle = node.schemaData.title as string; + + const typeIndex = hierarchy.indexOf(nodeType); + if (typeIndex === -1) continue; + + const parentTitle = node.resolvedParent; + if (!parentTitle) continue; + + const parentNode = nodeIndex.get(parentTitle); + if (!parentNode) continue; + + const parentType = parentNode.resolvedType; + const parentIndex = hierarchy.indexOf(parentType); + + if (parentIndex === -1) continue; + + const canSelfRef = allowSelfRef.includes(nodeType); + let isValid = parentIndex === typeIndex - 1; + + if (canSelfRef) { + isValid = isValid || parentIndex === typeIndex; + } + + if (allowSkipLevels && parentIndex < typeIndex) { + isValid = true; + } + + if (!isValid) { + violations.push({ + file: node.label, + nodeType, + nodeTitle, + parentType, + parentTitle, + description: `Invalid parent: ${nodeType} "${nodeTitle}" cannot have ${parentType} "${parentTitle}" as parent`, + }); + } + } + + return violations; +} diff --git a/src/validate-rules.ts b/src/validate-rules.ts new file mode 100644 index 0000000..96bfb3f --- /dev/null +++ b/src/validate-rules.ts @@ -0,0 +1,59 @@ +import { buildEvalContext, evaluateExpression } from './evaluate-rule'; +import type { Rule, RuleCategory, RulesMetadata, RuleViolation, SpaceNode } from './types'; + +/** + * Validate nodes against rules metadata. + * Returns a list of rule violations found. + * + * @param nodes - All nodes in the space + * @param rules - Rules metadata with categorized rules + * @returns Array of rule violations + */ +export async function validateRules(nodes: SpaceNode[], rules: RulesMetadata): Promise { + const violations: RuleViolation[] = []; + + // Build node index for efficient lookups + const nodeIndex = new Map(); + for (const node of nodes) { + const title = node.schemaData.title as string; + if (title) { + nodeIndex.set(title, node); + } + } + + // Collect all rules from each category + const allCategories: Array<{ category: RuleCategory; rules: Rule[] }> = [ + { category: 'validation', rules: rules.validation ?? [] }, + { category: 'coherence', rules: rules.coherence ?? [] }, + { category: 'best-practice', rules: rules.bestPractice ?? [] }, + ]; + + // Evaluate each rule against applicable nodes + for (const { category, rules: categoryRules } of allCategories) { + for (const rule of categoryRules) { + if (rule.scope === 'global') { + // Global rules are evaluated once against the full node set. + // A sentinel node provides the evaluation context (nodes array is what matters). + const sentinel = nodes[0]; + if (sentinel) { + const context = buildEvalContext(sentinel, nodes, nodeIndex); + const result = await evaluateExpression(rule.check, context); + if (result !== true) { + violations.push({ file: '', ruleId: rule.id, category, description: rule.description }); + } + } + } else { + const targetNodes = rule.type ? nodes.filter((n) => n.resolvedType === rule.type) : nodes; + for (const node of targetNodes) { + const context = buildEvalContext(node, nodes, nodeIndex); + const result = await evaluateExpression(rule.check, context); + if (result !== true) { + violations.push({ file: node.label, ruleId: rule.id, category, description: rule.description }); + } + } + } + } + } + + return violations; +} diff --git a/tests/evaluate-rule.test.ts b/tests/evaluate-rule.test.ts new file mode 100644 index 0000000..07d5b7f --- /dev/null +++ b/tests/evaluate-rule.test.ts @@ -0,0 +1,165 @@ +import { describe, expect, it } from 'bun:test'; +import { buildEvalContext, evaluateExpression } from '../src/evaluate-rule'; +import type { SpaceNode } from '../src/types'; + +describe('evaluate-rule', () => { + describe('evaluateExpression', () => { + const mockNode: SpaceNode = { + label: 'test.md', + schemaData: { + title: 'Test Node', + type: 'solution', + status: 'active', + parent: '[[Parent Opportunity]]', + }, + linkTargets: ['Test Node'], + resolvedParent: 'Parent Opportunity', + resolvedType: 'solution', + }; + + const mockParent: SpaceNode = { + label: 'parent.md', + schemaData: { + title: 'Parent Opportunity', + type: 'opportunity', + status: 'active', + }, + linkTargets: ['Parent Opportunity'], + resolvedType: 'opportunity', + }; + + const mockNodeIndex = new Map([ + ['Test Node', mockNode], + ['Parent Opportunity', mockParent], + ]); + + const allNodes = [mockNode, mockParent]; + + it('evaluates simple boolean expression returning true', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('true', context); + expect(result).toBe(true); + }); + + it('evaluates simple boolean expression returning false', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('false', context); + expect(result).toBe(false); + }); + + it('evaluates expression with current node (current)', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('current.type', context); + expect(result).toBe('solution'); + }); + + it('evaluates expression with all nodes (nodes)', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('$count(nodes)', context); + expect(result).toBe(2); + }); + + it('evaluates expression with parent access', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('$not(parent) or parent.type = "opportunity"', context); + expect(result).toBe(true); + }); + + it('evaluates expression with filter lookup', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('nodes[title="Parent Opportunity"].type', context); + expect(result).toBe('opportunity'); + }); + + it('evaluates expression with $exists function', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('$exists(nodes[title="Parent Opportunity"])', context); + expect(result).toBe(true); + }); + + it('evaluates expression with $exists check for missing parent', async () => { + const nodeWithoutParent: SpaceNode = { + label: 'orphan.md', + schemaData: { + title: 'Orphan Node', + type: 'outcome', + status: 'active', + }, + linkTargets: ['Orphan Node'], + resolvedType: 'goal', // outcome is an alias for goal + }; + const context = buildEvalContext(nodeWithoutParent, allNodes, mockNodeIndex); + const result = await evaluateExpression('$exists(parent) = false', context); + expect(result).toBe(true); + }); + + it('evaluates complex expression with count and filter', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('$count(nodes[type="solution"])', context); + expect(result).toBe(1); + }); + + it('returns false on expression evaluation error', async () => { + const context = buildEvalContext(mockNode, allNodes, mockNodeIndex); + const result = await evaluateExpression('invalid.syntax.('!, context); + expect(result).toBe(false); + }); + }); + + describe('buildEvalContext', () => { + const childNode: SpaceNode = { + label: 'child.md', + schemaData: { title: 'Child', type: 'solution', parent: '[[Parent]]' }, + linkTargets: ['Child'], + resolvedParent: 'Parent', + resolvedType: 'solution', + }; + + const parentNode: SpaceNode = { + label: 'parent.md', + schemaData: { title: 'Parent', type: 'opportunity' }, + linkTargets: ['Parent'], + resolvedType: 'opportunity', + }; + + const mockNodes = [childNode, parentNode]; + + const nodeIndex = new Map([ + ['Child', childNode], + ['Parent', parentNode], + ]); + + it('builds context with all nodes', () => { + const context = buildEvalContext(childNode, mockNodes, nodeIndex); + expect(context.nodes.length).toBe(mockNodes.length); + // All nodes should be flattened (have title and type at top level) + expect(context.nodes[0]).toHaveProperty('title'); + expect(context.nodes[0]).toHaveProperty('type'); + }); + + it('builds context with current node', () => { + const context = buildEvalContext(childNode, mockNodes, nodeIndex); + // Note: The actual evaluation uses 'current' instead of '$$' + expect(context.$$).toHaveProperty('title', 'Child'); + expect(context.$$).toHaveProperty('type', 'solution'); + }); + + it('builds context with resolved parent', () => { + const context = buildEvalContext(childNode, mockNodes, nodeIndex); + expect(context.parent).toBeDefined(); + expect(context.parent).toHaveProperty('title', 'Parent'); + expect(context.parent).toHaveProperty('type', 'opportunity'); + }); + + it('sets parent to undefined when node has no resolved parent', () => { + const orphanNode: SpaceNode = { + label: 'orphan.md', + schemaData: { title: 'Orphan', type: 'outcome' }, + linkTargets: ['Orphan'], + resolvedType: 'goal', // outcome is an alias for goal + }; + const context = buildEvalContext(orphanNode, mockNodes, nodeIndex); + expect(context.parent).toBeUndefined(); + }); + }); +}); diff --git a/tests/fixtures/strict_ost/invalid/experiment-invalid-category.md b/tests/fixtures/strict_ost/invalid/experiment-invalid-category.md index 3d60ba9..91f3541 100644 --- a/tests/fixtures/strict_ost/invalid/experiment-invalid-category.md +++ b/tests/fixtures/strict_ost/invalid/experiment-invalid-category.md @@ -1,9 +1,9 @@ --- -type: experiment +type: assumption_test status: exploring parent: "[[Some Solution]]" assumption: "Users will like this feature" category: "not-a-valid-category" --- -This experiment has an invalid category value. +This assumption test has an invalid category value. diff --git a/tests/fixtures/strict_ost/invalid/invalid-experiment-no-assumption.md b/tests/fixtures/strict_ost/invalid/invalid-experiment-no-assumption.md index dca791e..3150660 100644 --- a/tests/fixtures/strict_ost/invalid/invalid-experiment-no-assumption.md +++ b/tests/fixtures/strict_ost/invalid/invalid-experiment-no-assumption.md @@ -1,7 +1,7 @@ --- -type: experiment +type: assumption_test status: exploring parent: "[[Some Solution]]" --- -This experiment is missing the required 'assumption' field. \ No newline at end of file +This test is missing the required 'assumption' field. \ No newline at end of file diff --git a/tests/fixtures/strict_ost/valid-directory/Usability test.md b/tests/fixtures/strict_ost/valid-directory/Usability test.md index 1c90db1..a884359 100644 --- a/tests/fixtures/strict_ost/valid-directory/Usability test.md +++ b/tests/fixtures/strict_ost/valid-directory/Usability test.md @@ -1,5 +1,5 @@ --- -type: experiment +type: assumption_test status: exploring parent: "[[Simplify sign-up flow]]" assumption: Users can complete signup in under 2 minutes diff --git a/tests/validate-hierarchy.test.ts b/tests/validate-hierarchy.test.ts new file mode 100644 index 0000000..e19a092 --- /dev/null +++ b/tests/validate-hierarchy.test.ts @@ -0,0 +1,170 @@ +import { describe, expect, it } from 'bun:test'; +import type { SpaceNode } from '../src/types'; +import { validateHierarchy } from '../src/validate-hierarchy'; + +describe('validate-hierarchy', () => { + const buildNode = (title: string, type: string, parentTitle?: string): SpaceNode => ({ + label: `${title}.md`, + schemaData: { title, type, status: 'active' }, + linkTargets: [title], + resolvedType: type, // In tests, no alias resolution needed + ...(parentTitle ? { resolvedParent: parentTitle } : {}), + }); + + describe('hierarchy with allowSelfRef', () => { + const metadata = { + hierarchy: ['vision', 'mission', 'goal', 'opportunity', 'solution', 'experiment'], + aliases: {}, + allowSkipLevels: false, + allowSelfRef: ['goal', 'opportunity', 'solution'], + }; + + it('passes when node has immediate parent in hierarchy', () => { + const nodes: SpaceNode[] = [ + buildNode('My Vision', 'vision'), + buildNode('My Mission', 'mission', 'My Vision'), + buildNode('My Goal', 'goal', 'My Mission'), + ]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(0); + }); + + it('passes when node has same-type parent if allowSelfRef includes type', () => { + const nodes: SpaceNode[] = [ + buildNode('Main Goal', 'goal'), + buildNode('Sub Goal', 'goal', 'Main Goal'), + buildNode('My Outcome', 'outcome', 'Sub Goal'), + buildNode('Opportunity 1', 'opportunity', 'My Outcome'), + ]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(0); + }); + + it('fails when node has same-type parent if allowSelfRef excludes type', () => { + const nodes: SpaceNode[] = [buildNode('Mission 1', 'mission'), buildNode('Mission 2', 'mission', 'Mission 1')]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(1); + expect(violations[0]?.nodeType).toBe('mission'); + expect(violations[0]?.parentType).toBe('mission'); + expect(violations[0]?.description).toContain('mission "Mission 2" cannot have mission "Mission 1" as parent'); + }); + + it('fails when node skips hierarchy level', () => { + const nodes: SpaceNode[] = [ + buildNode('My Vision', 'vision'), + buildNode('My Goal', 'goal', 'My Vision'), // Skips mission + ]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(1); + expect(violations[0]?.nodeType).toBe('goal'); + expect(violations[0]?.parentType).toBe('vision'); + expect(violations[0]?.description).toContain('goal "My Goal" cannot have vision "My Vision" as parent'); + }); + + it('fails when solution has goal as parent', () => { + const nodes: SpaceNode[] = [buildNode('My Goal', 'goal'), buildNode('My Solution', 'solution', 'My Goal')]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(1); + expect(violations[0]?.nodeType).toBe('solution'); + expect(violations[0]?.parentType).toBe('goal'); + }); + }); + + describe('hierarchy with allowSkipLevels', () => { + const metadata = { + hierarchy: ['vision', 'mission', 'goal', 'opportunity', 'solution', 'experiment'], + aliases: {}, + allowSkipLevels: true, + allowSelfRef: ['goal', 'opportunity', 'solution'], + }; + + it('allows skipping hierarchy levels when allowSkipLevels is true', () => { + const nodes: SpaceNode[] = [ + buildNode('My Vision', 'vision'), + buildNode('My Solution', 'solution', 'My Vision'), // Skips mission, goal, outcome, opportunity + ]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(0); + }); + + it('still requires parent to be above child in hierarchy', () => { + const nodes: SpaceNode[] = [ + buildNode('My Goal', 'goal'), + buildNode('My Vision', 'vision', 'My Goal'), // Vision under goal - backwards + ]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(1); + expect(violations[0]?.nodeType).toBe('vision'); + expect(violations[0]?.parentType).toBe('goal'); + }); + }); + + describe('edge cases', () => { + const metadata = { + hierarchy: ['vision', 'mission', 'goal', 'opportunity', 'solution', 'experiment'], + aliases: {}, + allowSkipLevels: false, + allowSelfRef: ['goal', 'opportunity', 'solution'], + }; + + it('skips nodes not in hierarchy', () => { + const nodes: SpaceNode[] = [buildNode('Dashboard', 'dashboard'), buildNode('Some Node', 'custom_type')]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(0); + }); + + it('skips nodes without a parent', () => { + const nodes: SpaceNode[] = [buildNode('My Vision', 'vision'), buildNode('Orphan Goal', 'goal')]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(0); + }); + + it('skips nodes when parent node is not found', () => { + // Missing parent references are already reported by the validate command. + // Hierarchy validation only checks type relationships, so it skips nodes + // whose parent cannot be found to avoid double-reporting. + const nodes: SpaceNode[] = [buildNode('My Goal', 'goal', 'Nonexistent Parent')]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(0); + }); + + it('handles empty nodes array', () => { + const violations = validateHierarchy([], metadata); + expect(violations).toHaveLength(0); + }); + }); + + describe('violation format', () => { + const metadata = { + hierarchy: ['vision', 'mission', 'goal'], + aliases: {}, + allowSkipLevels: false, + allowSelfRef: [], + }; + + it('includes all required fields in violation', () => { + const nodes: SpaceNode[] = [buildNode('My Vision', 'vision'), buildNode('My Goal', 'goal', 'My Vision')]; + + const violations = validateHierarchy(nodes, metadata); + expect(violations).toHaveLength(1); + + const v = violations[0]!; + expect(v.file).toBe('My Goal.md'); + expect(v.nodeType).toBe('goal'); + expect(v.nodeTitle).toBe('My Goal'); + expect(v.parentType).toBe('vision'); + expect(v.parentTitle).toBe('My Vision'); + expect(v.description).toBe('Invalid parent: goal "My Goal" cannot have vision "My Vision" as parent'); + }); + }); +}); diff --git a/tests/validate-rules.test.ts b/tests/validate-rules.test.ts new file mode 100644 index 0000000..9d1205a --- /dev/null +++ b/tests/validate-rules.test.ts @@ -0,0 +1,318 @@ +import { describe, expect, it } from 'bun:test'; +import type { RulesMetadata, SpaceNode } from '../src/types'; +import { validateRules } from '../src/validate-rules'; + +describe('validate-rules', () => { + describe('validateRules', () => { + const mockNodes: SpaceNode[] = [ + { + label: 'outcome.md', + schemaData: { title: 'Outcome', type: 'outcome', status: 'active', metric: 'Increase X' }, + linkTargets: ['Outcome'], + resolvedType: 'goal', // outcome is an alias for goal + }, + { + label: 'opportunity.md', + schemaData: { + title: 'Opportunity', + type: 'opportunity', + status: 'active', + parent: '[[Outcome]]', + source: 'Interview', + }, + linkTargets: ['Opportunity'], + resolvedParent: 'Outcome', + resolvedType: 'opportunity', + }, + { + label: 'solution.md', + schemaData: { title: 'Solution', type: 'solution', status: 'exploring', parent: '[[Opportunity]]' }, + linkTargets: ['Solution'], + resolvedParent: 'Opportunity', + resolvedType: 'solution', + }, + { + label: 'bad-solution.md', + schemaData: { title: 'Bad Solution', type: 'solution', status: 'exploring', parent: '[[Solution]]' }, + linkTargets: ['Bad Solution'], + resolvedParent: 'Solution', + resolvedType: 'solution', + }, + { + label: 'experiment.md', + schemaData: { + title: 'Experiment', + type: 'experiment', + status: 'exploring', + parent: '[[Solution]]', + assumption: 'Test', + }, + linkTargets: ['Experiment'], + resolvedParent: 'Solution', + resolvedType: 'experiment', + }, + { + label: 'bad-experiment.md', + schemaData: { + title: 'Bad Experiment', + type: 'experiment', + status: 'exploring', + parent: '[[Opportunity]]', + assumption: 'Test', + }, + linkTargets: ['Bad Experiment'], + resolvedParent: 'Opportunity', + resolvedType: 'experiment', + }, + ]; + + describe('validation rules', () => { + const validationRules: RulesMetadata = { + validation: [ + { + id: 'solution-parent-type', + description: 'Parent must be an opportunity', + type: 'solution', + check: '$exists(parent) = false or $exists(nodes[title=$$.current.parent and resolvedType="opportunity"])', + }, + { + id: 'experiment-parent-type', + description: 'Parent must be a solution', + type: 'experiment', + check: '$exists(parent) and $exists(nodes[title=$$.current.parent and resolvedType="solution"])', + }, + { + id: 'outcome-no-parent', + description: 'Outcome nodes should not have a parent', + type: 'outcome', + check: '$exists(parent) = false', + }, + ], + }; + + it('passes validation when all rules are satisfied', async () => { + const validNodes = mockNodes.filter((n) => n.label === 'solution.md'); + const violations = await validateRules(validNodes, validationRules); + expect(violations).toHaveLength(0); + }); + + it('detects solution with non-opportunity parent', async () => { + const badSolutionNode = mockNodes.find((n) => n.label === 'bad-solution.md'); + const parentNode = mockNodes.find((n) => n.label === 'solution.md'); + // Pass both nodes so the parent can be found in the index + const violations = await validateRules([badSolutionNode!, parentNode!], validationRules); + expect(violations).toHaveLength(1); + expect(violations[0]?.ruleId).toBe('solution-parent-type'); + expect(violations[0]?.category).toBe('validation'); + expect(violations[0]?.file).toBe('bad-solution.md'); + }); + + it('detects experiment with non-solution parent', async () => { + const badExperimentNode = mockNodes.find((n) => n.label === 'bad-experiment.md'); + const parentNode = mockNodes.find((n) => n.label === 'opportunity.md'); + // Pass both nodes so the parent can be found in the index + const violations = await validateRules([badExperimentNode!, parentNode!], validationRules); + expect(violations).toHaveLength(1); + expect(violations[0]?.ruleId).toBe('experiment-parent-type'); + expect(violations[0]?.category).toBe('validation'); + }); + + it('detects outcome with parent', async () => { + const outcomeWithParent: SpaceNode = { + label: 'bad-outcome.md', + schemaData: { title: 'Bad Outcome', type: 'outcome', status: 'active', parent: '[[Vision]]' }, + linkTargets: ['Bad Outcome'], + resolvedParent: 'Vision', + resolvedType: 'outcome', + }; + const visionNode: SpaceNode = { + label: 'vision.md', + schemaData: { title: 'Vision', type: 'vision', status: 'active' }, + linkTargets: ['Vision'], + resolvedType: 'vision', + }; + // Pass both nodes so the parent can be found in the index + const violations = await validateRules([outcomeWithParent, visionNode], validationRules); + expect(violations).toHaveLength(1); + expect(violations[0]?.ruleId).toBe('outcome-no-parent'); + }); + }); + + describe('coherence rules', () => { + const coherenceRules: RulesMetadata = { + coherence: [ + { + id: 'active-outcome-count', + description: 'Only one outcome should be active at a time', + scope: 'global', + check: '$count(nodes[resolvedType="outcome" and status="active"]) <= 1', + }, + ], + }; + + it('passes when only one outcome is active', async () => { + const nodes = mockNodes.filter((n) => n.schemaData.type === 'outcome'); + const violations = await validateRules(nodes, coherenceRules); + expect(violations).toHaveLength(0); + }); + + it('detects multiple active outcomes', async () => { + const multipleActiveOutcomes: SpaceNode[] = [ + { + label: 'outcome1.md', + schemaData: { title: 'Outcome 1', type: 'outcome', status: 'active', metric: 'X' }, + linkTargets: ['Outcome 1'], + resolvedType: 'outcome', + }, + { + label: 'outcome2.md', + schemaData: { title: 'Outcome 2', type: 'outcome', status: 'active', metric: 'Y' }, + linkTargets: ['Outcome 2'], + resolvedType: 'outcome', + }, + ]; + const violations = await validateRules(multipleActiveOutcomes, coherenceRules); + expect(violations).toHaveLength(1); // Global rule produces one violation for the whole space + expect(violations[0]?.ruleId).toBe('active-outcome-count'); + expect(violations[0]?.file).toBe(''); + }); + }); + + describe('best-practice rules', () => { + const bestPracticeRules: RulesMetadata = { + bestPractice: [ + { + id: 'solution-quantity', + description: 'Explore multiple candidate solutions for the target opportunity', + type: 'opportunity', + check: '$count(nodes[resolvedParentTitle=$$.current.title and resolvedType="solution"]) >= 3', + }, + ], + }; + + it('passes when opportunity has enough solutions', async () => { + const opportunityNode: SpaceNode = { + label: 'opportunity.md', + schemaData: { + title: 'Opportunity', + type: 'opportunity', + status: 'active', + parent: '[[Outcome]]', + source: 'Interview', + }, + linkTargets: ['Opportunity'], + resolvedParent: 'Outcome', + resolvedType: 'opportunity', + }; + + const solutions: SpaceNode[] = Array.from({ length: 3 }, (_, i) => ({ + label: `solution${i}.md`, + schemaData: { + title: `Solution ${i}`, + type: 'solution', + status: 'exploring', + parent: '[[Opportunity]]', + }, + linkTargets: [`Solution ${i}`], + resolvedParent: 'Opportunity', + resolvedType: 'solution', + })); + + const nodes = [opportunityNode, ...solutions]; + const violations = await validateRules(nodes, bestPracticeRules); + expect(violations).toHaveLength(0); + }); + + it('detects opportunity with too few solutions', async () => { + const opportunityNode: SpaceNode = { + label: 'opportunity.md', + schemaData: { + title: 'Opportunity', + type: 'opportunity', + status: 'active', + parent: '[[Outcome]]', + source: 'Interview', + }, + linkTargets: ['Opportunity'], + resolvedParent: 'Outcome', + resolvedType: 'opportunity', + }; + + const singleSolution: SpaceNode = { + label: 'solution.md', + schemaData: { title: 'Solution', type: 'solution', status: 'exploring', parent: '[[Opportunity]]' }, + linkTargets: ['Solution'], + resolvedParent: 'Opportunity', + resolvedType: 'solution', + }; + + const nodes = [opportunityNode, singleSolution]; + const violations = await validateRules(nodes, bestPracticeRules); + expect(violations).toHaveLength(1); + expect(violations[0]?.ruleId).toBe('solution-quantity'); + expect(violations[0]?.category).toBe('best-practice'); + }); + }); + + describe('mixed categories', () => { + const mixedRules: RulesMetadata = { + validation: [ + { + id: 'solution-parent-type', + description: 'Parent must be an opportunity', + type: 'solution', + check: '$exists(parent) = false or $exists(nodes[title=$$.current.parent and resolvedType="opportunity"])', + }, + ], + coherence: [ + { + id: 'active-outcome-count', + description: 'Only one outcome should be active at a time', + scope: 'global', + check: '$count(nodes[resolvedType="outcome" and status="active"]) <= 1', + }, + ], + }; + + it('collects violations from multiple categories', async () => { + const nodes: SpaceNode[] = [ + { + label: 'outcome1.md', + schemaData: { title: 'Outcome 1', type: 'outcome', status: 'active', metric: 'X' }, + linkTargets: ['Outcome 1'], + resolvedType: 'outcome', + }, + { + label: 'outcome2.md', + schemaData: { title: 'Outcome 2', type: 'outcome', status: 'active', metric: 'Y' }, + linkTargets: ['Outcome 2'], + resolvedType: 'outcome', + }, + { + label: 'solution.md', + schemaData: { title: 'Solution', type: 'solution', status: 'exploring', parent: '[[Opportunity]]' }, + linkTargets: ['Solution'], + resolvedParent: 'Opportunity', + resolvedType: 'solution', + }, + { + label: 'bad-solution.md', + schemaData: { title: 'Bad Solution', type: 'solution', status: 'exploring', parent: '[[Solution]]' }, + linkTargets: ['Bad Solution'], + resolvedParent: 'Solution', + resolvedType: 'solution', + }, + ]; + + const violations = await validateRules(nodes, mixedRules); + expect(violations.length).toBeGreaterThan(0); + + const validationViolations = violations.filter((v) => v.category === 'validation'); + const coherenceViolations = violations.filter((v) => v.category === 'coherence'); + + expect(validationViolations.length).toBeGreaterThan(0); + expect(coherenceViolations.length).toBeGreaterThan(0); + }); + }); + }); +}); diff --git a/tests/validate-strict.test.ts b/tests/validate-strict.test.ts index 3246c70..ed83227 100644 --- a/tests/validate-strict.test.ts +++ b/tests/validate-strict.test.ts @@ -79,11 +79,11 @@ describe('Strict OST schema validation', () => { ).toBe(true); }); - it('accepts experiment type', () => { + it('accepts assumption_test type', () => { expect( validateNode({ title: 'An Experiment', - type: 'experiment', + type: 'assumption_test', status: 'exploring', parent: '[[Some Solution]]', assumption: 'Users will prefer this', @@ -137,22 +137,22 @@ describe('Strict OST schema validation', () => { ).toBe(true); }); - it('rejects experiment without required assumption field', () => { + it('rejects assumption_test without required assumption field', () => { expect( validateNode({ - title: 'Experiment', - type: 'experiment', + title: 'Test', + type: 'assumption_test', status: 'exploring', parent: '[[Solution]]', }), ).toBe(false); }); - it('accepts experiment with assumption field', () => { + it('accepts assumption_test with assumption field', () => { expect( validateNode({ - title: 'Experiment', - type: 'experiment', + title: 'Test', + type: 'assumption_test', status: 'exploring', parent: '[[Solution]]', assumption: 'Users will complete signup in under 2 minutes', @@ -161,14 +161,14 @@ describe('Strict OST schema validation', () => { }); }); - describe('experiment category enum validation', () => { + describe('assumption_test category enum validation', () => { const validCategories = ['desirability', 'viability', 'feasibility', 'usability', 'ethical']; it.each(validCategories)('accepts valid category: %s', (category) => { expect( validateNode({ - title: 'Experiment', - type: 'experiment', + title: 'Test', + type: 'assumption_test', status: 'exploring', parent: '[[Solution]]', assumption: 'Test assumption', @@ -282,5 +282,15 @@ describe('Strict OST schema validation', () => { expect(node).toBeDefined(); expect(validateNode(node?.schemaData)).toBe(false); }); + + it('reports validation violation for solution with solution parent', async () => { + // This test verifies that the rules validation catches solution nodes + // that have other solution nodes as parents (invalid parent type) + // Note: The invalid-solution-solution-parent.md fixture uses space_on_a_page + // format which is detected as invalid by schema validation. + const fs = await import('node:fs'); + const fixturePath = join(INVALID_DIR, 'invalid-solution-solution-parent.md'); + expect(fs.existsSync(fixturePath)).toBe(true); + }); }); }); From 681136e61cc681744dc5bb154147096cabaed102 Mon Sep 17 00:00:00 2001 From: Roger Barnes Date: Sun, 1 Mar 2026 23:31:36 +1100 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94=20?= =?UTF-8?q?workflow=20category,=20active-parent=20rule,=20config=20validat?= =?UTF-8?q?ion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `workflow` rule category for process discipline checks; move active-outcome/opportunity-count rules from coherence to workflow - Add `active-node-parent-active` workflow rule: an active node's parent must also be active - Update `solution-quantity` best-practice to only apply when opportunity status is exploring or active - Check `allowSelfRef ⊆ hierarchy` in validate command, reporting mismatches as config errors with a new configErrors result field - Rename schemaErrors/schemaValidCount → nodeErrors/validCount for clarity - Update docs/rules.md: add workflow category, clarify scope is orthogonal to category, update examples --- docs/rules.md | 18 ++--- schemas/_strict.json | 9 ++- src/commands/validate.ts | 50 ++++++++---- src/types.ts | 3 +- src/validate-rules.ts | 1 + tests/validate-rules.test.ts | 151 +++++++++++++++++++++++++---------- 6 files changed, 161 insertions(+), 71 deletions(-) diff --git a/docs/rules.md b/docs/rules.md index 9e108ff..b843f47 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -6,12 +6,13 @@ For how rules fit into the broader schema metadata, see [docs/schemas.md](schema ## Rule Categories -Rules are grouped into three categories under `_metadata.rules`. +Rules are grouped into categories under `_metadata.rules`. Categories are informational — they determine how violations are labelled and grouped in output, but do not affect how the rule is evaluated. Use `scope` to control evaluation mode. | Category | Purpose | |---|---| | `validation` | Structural correctness — a violation means the node is incorrect and should be fixed | -| `coherence` | Cross-node checks — for flagging conflicts or contradictions (often combined with `scope: 'global'` for multi-node/aggregate checks) | +| `coherence` | Cross-node checks — for flagging conflicts or contradictions between nodes | +| `workflow` | Process discipline checks — for keeping the tree in an operational working state (active counts, status consistency) | | `bestPractice` | Advisory guidance — signals the space may benefit from additional work | ## Rule Object Structure @@ -64,7 +65,7 @@ $exists(parent) = false // true for root nodes ```json { - "coherence": [ + "workflow": [ { "id": "active-outcome-count", "description": "Only one outcome should be active at a time", @@ -72,10 +73,9 @@ $exists(parent) = false // true for root nodes "check": "$count(nodes[resolvedType='outcome' and status='active']) <= 1" }, { - "id": "active-opportunity-count", - "description": "Only one target opportunity should be active at a time", - "scope": "global", - "check": "$count(nodes[resolvedType='opportunity' and status='active']) <= 1" + "id": "active-node-parent-active", + "description": "An active node's parent should also be active", + "check": "current.status != 'active' or $exists(parent) = false or parent.status = 'active'" } ], "bestPractice": [ @@ -83,10 +83,10 @@ $exists(parent) = false // true for root nodes "id": "solution-quantity", "description": "Explore multiple candidate solutions (aim for at least three) for the target opportunity", "type": "opportunity", - "check": "$count(nodes[resolvedParentTitle=$$.current.title and resolvedType='solution']) >= 3" + "check": "(current.status != 'exploring' and current.status != 'active') or $count(nodes[resolvedParentTitle=$$.current.title and resolvedType='solution']) >= 3" } ] } ``` -The coherence rules run against every node (no `type` filter) — each node in a space with two active outcomes will report a violation. The best-practice rule only runs against `opportunity` nodes, using `resolvedParentTitle` to count child solutions. \ No newline at end of file +The first workflow rule uses `scope: 'global'` — evaluated once against the whole space, producing at most one violation. The second runs per-node with no `type` filter, checking every node. The best-practice rule only runs against `opportunity` nodes where status is `exploring` or `active`, using `resolvedParentTitle` to count child solutions. \ No newline at end of file diff --git a/schemas/_strict.json b/schemas/_strict.json index 43933ba..4bedac1 100644 --- a/schemas/_strict.json +++ b/schemas/_strict.json @@ -9,7 +9,7 @@ "hierarchy": ["outcome", "opportunity", "solution", "assumption_test"], "allowSelfRef": ["opportunity"], "rules": { - "coherence": [ + "workflow": [ { "id": "active-outcome-count", "description": "Only one outcome should be active at a time", @@ -21,6 +21,11 @@ "description": "Only one target opportunity should be active at a time", "scope": "global", "check": "$count(nodes[resolvedType='opportunity' and status='active']) <= 1" + }, + { + "id": "active-node-parent-active", + "description": "An active node's parent should also be active", + "check": "current.status != 'active' or $exists(parent) = false or parent.status = 'active'" } ], "bestPractice": [ @@ -28,7 +33,7 @@ "id": "solution-quantity", "description": "Explore multiple candidate solutions (aim for at least three) for the target opportunity", "type": "opportunity", - "check": "$count(nodes[resolvedParentTitle=$$.current.title and resolvedType='solution']) >= 3" + "check": "(current.status != 'exploring' and current.status != 'active') or $count(nodes[resolvedParentTitle=$$.current.title and resolvedType='solution']) >= 3" } ] } diff --git a/src/commands/validate.ts b/src/commands/validate.ts index fde5bed..e1a15f6 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -9,10 +9,11 @@ import { validateHierarchy } from '../validate-hierarchy'; import { validateRules } from '../validate-rules'; interface ValidationResult { - schemaValidCount: number; - schemaErrorCount: number; - schemaErrors: Array<{ file: string; errors: ErrorObject[] }>; + validCount: number; + nodeErrorCount: number; + nodeErrors: Array<{ file: string; errors: ErrorObject[] }>; refErrors: Array<{ file: string; parent: string; error: string }>; + configErrors: string[]; ruleViolations: RuleViolation[]; hierarchyViolations: HierarchyViolation[]; skipped: string[]; @@ -33,10 +34,11 @@ export async function validate(path: string, options: { schema: string }): Promi } const result: ValidationResult = { - schemaValidCount: 0, - schemaErrorCount: 0, - schemaErrors: [], + validCount: 0, + nodeErrorCount: 0, + nodeErrors: [], refErrors: [], + configErrors: [], ruleViolations: [], hierarchyViolations: [], skipped, @@ -47,10 +49,10 @@ export async function validate(path: string, options: { schema: string }): Promi const valid = validateFunc(node.schemaData); if (valid) { - result.schemaValidCount++; + result.validCount++; } else { - result.schemaErrorCount++; - result.schemaErrors.push({ + result.nodeErrorCount++; + result.nodeErrors.push({ file: node.label, errors: validateFunc.errors || [], }); @@ -88,6 +90,13 @@ export async function validate(path: string, options: { schema: string }): Promi // Load and execute hierarchy validation if schema defines hierarchy const metadata = loadMetadata(options.schema); + + // Validate schema metadata configuration + const badSelfRefTypes = (metadata.allowSelfRef ?? []).filter((t) => !metadata.hierarchy.includes(t)); + for (const t of badSelfRefTypes) { + result.configErrors.push(`allowSelfRef contains "${t}" which is not in the hierarchy`); + } + result.hierarchyViolations = validateHierarchy(nodes, metadata); // Load and execute rules validation if schema defines rules @@ -98,11 +107,12 @@ export async function validate(path: string, options: { schema: string }): Promi // Report console.log(`\n🔍 Space Validation Results`); console.log(`━`.repeat(50)); - console.log(`✅ Valid: ${result.schemaValidCount}`); - console.log(`❌ Schema Errors: ${result.schemaErrorCount}`); + console.log(`✅ Valid: ${result.validCount}`); + console.log(`❌ Node Errors: ${result.nodeErrorCount}`); console.log(`🔗 Reference Errors: ${result.refErrors.length}`); + console.log(`⚙️ Config Errors: ${result.configErrors.length}`); console.log(`📋 Rule Violations: ${result.ruleViolations.length}`); - console.log(`🏗️ Hierarchy Violations: ${result.hierarchyViolations.length}`); + console.log(`🏗️ Hierarchy Violations: ${result.hierarchyViolations.length}`); console.log(`⏭ Skipped (no frontmatter): ${result.skipped.length}`); console.log(`📄 Non-space (no type field): ${result.nonSpace.length}`); @@ -116,9 +126,9 @@ export async function validate(path: string, options: { schema: string }): Promi for (const f of result.nonSpace) console.log(` ${f}`); } - if (result.schemaErrors.length > 0) { - console.log(`\n❌ Schema validation errors:`); - result.schemaErrors.forEach(({ file, errors }) => { + if (result.nodeErrors.length > 0) { + console.log(`\n❌ Node errors:`); + result.nodeErrors.forEach(({ file, errors }) => { console.log(`\n ${file}:`); errors.forEach((err: ErrorObject) => { console.log(` ${err.instancePath || 'root'}: ${err.message}`); @@ -133,6 +143,13 @@ export async function validate(path: string, options: { schema: string }): Promi }); } + if (result.configErrors.length > 0) { + console.log(`\n⚙️ Config errors:`); + for (const e of result.configErrors) { + console.log(` ${e}`); + } + } + if (result.ruleViolations.length > 0) { console.log(`\n📋 Rule violations:`); @@ -164,8 +181,9 @@ export async function validate(path: string, options: { schema: string }): Promi console.log(`\n`); if ( - result.schemaErrorCount > 0 || + result.nodeErrorCount > 0 || result.refErrors.length > 0 || + result.configErrors.length > 0 || result.ruleViolations.length > 0 || result.hierarchyViolations.length > 0 ) { diff --git a/src/types.ts b/src/types.ts index 8247709..732ed77 100644 --- a/src/types.ts +++ b/src/types.ts @@ -30,7 +30,7 @@ export interface SpaceDirectoryReadResult { } /** Rule categories for organizing executable validation rules */ -export type RuleCategory = 'validation' | 'coherence' | 'best-practice'; +export type RuleCategory = 'validation' | 'coherence' | 'workflow' | 'best-practice'; /** A single executable rule with JSONata check expression */ export interface Rule { @@ -47,6 +47,7 @@ export interface Rule { export interface RulesMetadata { validation?: Rule[]; coherence?: Rule[]; + workflow?: Rule[]; bestPractice?: Rule[]; } diff --git a/src/validate-rules.ts b/src/validate-rules.ts index 96bfb3f..c1b288f 100644 --- a/src/validate-rules.ts +++ b/src/validate-rules.ts @@ -25,6 +25,7 @@ export async function validateRules(nodes: SpaceNode[], rules: RulesMetadata): P const allCategories: Array<{ category: RuleCategory; rules: Rule[] }> = [ { category: 'validation', rules: rules.validation ?? [] }, { category: 'coherence', rules: rules.coherence ?? [] }, + { category: 'workflow', rules: rules.workflow ?? [] }, { category: 'best-practice', rules: rules.bestPractice ?? [] }, ]; diff --git a/tests/validate-rules.test.ts b/tests/validate-rules.test.ts index 9d1205a..a1ed600 100644 --- a/tests/validate-rules.test.ts +++ b/tests/validate-rules.test.ts @@ -138,46 +138,6 @@ describe('validate-rules', () => { }); }); - describe('coherence rules', () => { - const coherenceRules: RulesMetadata = { - coherence: [ - { - id: 'active-outcome-count', - description: 'Only one outcome should be active at a time', - scope: 'global', - check: '$count(nodes[resolvedType="outcome" and status="active"]) <= 1', - }, - ], - }; - - it('passes when only one outcome is active', async () => { - const nodes = mockNodes.filter((n) => n.schemaData.type === 'outcome'); - const violations = await validateRules(nodes, coherenceRules); - expect(violations).toHaveLength(0); - }); - - it('detects multiple active outcomes', async () => { - const multipleActiveOutcomes: SpaceNode[] = [ - { - label: 'outcome1.md', - schemaData: { title: 'Outcome 1', type: 'outcome', status: 'active', metric: 'X' }, - linkTargets: ['Outcome 1'], - resolvedType: 'outcome', - }, - { - label: 'outcome2.md', - schemaData: { title: 'Outcome 2', type: 'outcome', status: 'active', metric: 'Y' }, - linkTargets: ['Outcome 2'], - resolvedType: 'outcome', - }, - ]; - const violations = await validateRules(multipleActiveOutcomes, coherenceRules); - expect(violations).toHaveLength(1); // Global rule produces one violation for the whole space - expect(violations[0]?.ruleId).toBe('active-outcome-count'); - expect(violations[0]?.file).toBe(''); - }); - }); - describe('best-practice rules', () => { const bestPracticeRules: RulesMetadata = { bestPractice: [ @@ -254,6 +214,111 @@ describe('validate-rules', () => { }); }); + describe('workflow rules', () => { + const workflowRules: RulesMetadata = { + workflow: [ + { + id: 'active-outcome-count', + description: 'Only one outcome should be active at a time', + scope: 'global', + check: '$count(nodes[resolvedType="outcome" and status="active"]) <= 1', + }, + { + id: 'active-node-parent-active', + description: "An active node's parent should also be active", + check: "current.status != 'active' or $exists(parent) = false or parent.status = 'active'", + }, + ], + }; + + it('passes when only one outcome is active', async () => { + const nodes = mockNodes.filter((n) => n.schemaData.type === 'outcome'); + const violations = await validateRules(nodes, workflowRules); + expect(violations).toHaveLength(0); + }); + + it('detects multiple active outcomes (global scope)', async () => { + const multipleActiveOutcomes: SpaceNode[] = [ + { + label: 'outcome1.md', + schemaData: { title: 'Outcome 1', type: 'outcome', status: 'active', metric: 'X' }, + linkTargets: ['Outcome 1'], + resolvedType: 'outcome', + }, + { + label: 'outcome2.md', + schemaData: { title: 'Outcome 2', type: 'outcome', status: 'active', metric: 'Y' }, + linkTargets: ['Outcome 2'], + resolvedType: 'outcome', + }, + { + label: 'unrelated.md', + schemaData: { title: 'Unrelated', type: 'solution', status: 'exploring' }, + linkTargets: ['Unrelated'], + resolvedType: 'solution', + }, + ]; + const activeCountViolations = await validateRules(multipleActiveOutcomes, { + workflow: [workflowRules.workflow![0]!], + }); + expect(activeCountViolations).toHaveLength(1); // global rule produces one violation regardless of node count + expect(activeCountViolations[0]?.ruleId).toBe('active-outcome-count'); + expect(activeCountViolations[0]?.category).toBe('workflow'); + expect(activeCountViolations[0]?.file).toBe(''); + }); + + it('detects active node with non-active parent', async () => { + const parentNode: SpaceNode = { + label: 'outcome.md', + schemaData: { title: 'Outcome', type: 'outcome', status: 'inactive', metric: 'X' }, + linkTargets: ['Outcome'], + resolvedType: 'outcome', + }; + const childNode: SpaceNode = { + label: 'opportunity.md', + schemaData: { + title: 'Opportunity', + type: 'opportunity', + status: 'active', + parent: '[[Outcome]]', + source: 'Interview', + }, + linkTargets: ['Opportunity'], + resolvedParent: 'Outcome', + resolvedType: 'opportunity', + }; + const violations = await validateRules([parentNode, childNode], { workflow: [workflowRules.workflow![1]!] }); + expect(violations).toHaveLength(1); + expect(violations[0]?.ruleId).toBe('active-node-parent-active'); + expect(violations[0]?.category).toBe('workflow'); + expect(violations[0]?.file).toBe('opportunity.md'); + }); + + it('passes active node when parent is also active', async () => { + const parentNode: SpaceNode = { + label: 'outcome.md', + schemaData: { title: 'Outcome', type: 'outcome', status: 'active', metric: 'X' }, + linkTargets: ['Outcome'], + resolvedType: 'outcome', + }; + const childNode: SpaceNode = { + label: 'opportunity.md', + schemaData: { + title: 'Opportunity', + type: 'opportunity', + status: 'active', + parent: '[[Outcome]]', + source: 'Interview', + }, + linkTargets: ['Opportunity'], + resolvedParent: 'Outcome', + resolvedType: 'opportunity', + }; + const violations = await validateRules([parentNode, childNode], { workflow: [workflowRules.workflow![1]!] }); + expect(violations).toHaveLength(0); + }); + }); + describe('mixed categories', () => { const mixedRules: RulesMetadata = { validation: [ @@ -264,7 +329,7 @@ describe('validate-rules', () => { check: '$exists(parent) = false or $exists(nodes[title=$$.current.parent and resolvedType="opportunity"])', }, ], - coherence: [ + workflow: [ { id: 'active-outcome-count', description: 'Only one outcome should be active at a time', @@ -308,10 +373,10 @@ describe('validate-rules', () => { expect(violations.length).toBeGreaterThan(0); const validationViolations = violations.filter((v) => v.category === 'validation'); - const coherenceViolations = violations.filter((v) => v.category === 'coherence'); + const workflowViolations = violations.filter((v) => v.category === 'workflow'); expect(validationViolations.length).toBeGreaterThan(0); - expect(coherenceViolations.length).toBeGreaterThan(0); + expect(workflowViolations.length).toBeGreaterThan(0); }); }); }); From 954c9f10d85e61e9874b1d00b332dea6dd1e020c Mon Sep 17 00:00:00 2001 From: Roger Barnes Date: Mon, 2 Mar 2026 00:26:36 +1100 Subject: [PATCH 3/3] More PR improvements - duplicate node detection --- .github/workflows/claude-code-review.yml | 2 +- docs/concepts.md | 2 + src/commands/validate.ts | 45 ++++++++++++++++--- tests/fixtures/general/duplicate-embedded.md | 12 +++++ .../general/duplicate-titles/Vision.md | 5 +++ .../general/duplicate-titles/dir1/solution.md | 6 +++ .../general/duplicate-titles/dir2/solution.md | 6 +++ .../malformed-parent-array.md | 7 +++ tests/validate-general.test.ts | 43 ++++++++++++++++++ 9 files changed, 122 insertions(+), 6 deletions(-) create mode 100644 tests/fixtures/general/duplicate-embedded.md create mode 100644 tests/fixtures/general/duplicate-titles/Vision.md create mode 100644 tests/fixtures/general/duplicate-titles/dir1/solution.md create mode 100644 tests/fixtures/general/duplicate-titles/dir2/solution.md create mode 100644 tests/fixtures/general/malformed-parent-dir/malformed-parent-array.md diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index d24784a..5ab085e 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -34,7 +34,7 @@ jobs: plugin_marketplaces: 'https://github.com/anthropics/claude-code.git' plugins: 'code-review@claude-code-plugins' prompt: '/code-review:code-review --comment' - claude_args: '--allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr view:*),Bash(gh pr diff:*)"' + claude_args: '--allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr checkout:*),Bash(git log:*),Bash(bun run test:*),Bash(bun run lint:*),Bash(bun test:*),Bash(npx tsc:*),Bash(bun run tsc:*),Bash(gh pr checks:*),Bash(npx biome check:*),Bash(git fetch:*),Bash(gh issue list:*),Bash(gh issue view:*)"' # See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md # or https://code.claude.com/docs/en/cli-reference for available options diff --git a/docs/concepts.md b/docs/concepts.md index c3da7a0..f80278d 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -62,6 +62,8 @@ A `typed page` may contain embedded nodes in its body. Those nodes become full m A **type alias** is an alternative name accepted in the `type` field for a given `space node` type. Aliases allow teams to use their own vocabulary while still receiving schema validation. For example, a schema might accept `outcome` as an alias for `goal`. +A `space node`'s resolved type (`resolvedType`) is its canonical type after alias resolution. Prefer resolvedType over the raw type field for all comparisons in rules and hierarchy checks. + --- ## Typed page diff --git a/src/commands/validate.ts b/src/commands/validate.ts index e1a15f6..5549a9b 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -13,6 +13,7 @@ interface ValidationResult { nodeErrorCount: number; nodeErrors: Array<{ file: string; errors: ErrorObject[] }>; refErrors: Array<{ file: string; parent: string; error: string }>; + duplicateErrors: Array<{ title: string; files: string[] }>; configErrors: string[]; ruleViolations: RuleViolation[]; hierarchyViolations: HierarchyViolation[]; @@ -38,6 +39,7 @@ export async function validate(path: string, options: { schema: string }): Promi nodeErrorCount: 0, nodeErrors: [], refErrors: [], + duplicateErrors: [], configErrors: [], ruleViolations: [], hierarchyViolations: [], @@ -59,15 +61,36 @@ export async function validate(path: string, options: { schema: string }): Promi } } - // Parent refs are resolved to canonical titles on node.resolvedParent in read-* code. + // Detect duplicate node keys (titles) and build nodeIndex for parent ref validation + const titleToFiles = new Map(); const nodeIndex = new Map(); - for (const n of nodes) { - nodeIndex.set(n.schemaData.title as string, n); + for (const node of nodes) { + const title = node.schemaData.title as string; + if (!titleToFiles.has(title)) { + titleToFiles.set(title, []); + } + titleToFiles.get(title)!.push(node.label); + nodeIndex.set(title, node); + } + + for (const [title, files] of titleToFiles) { + if (files.length > 1) { + result.duplicateErrors.push({ title, files }); + } } for (const node of nodes) { - const parent = node.schemaData.parent as string | undefined; - if (!parent) continue; + const parent = node.schemaData.parent; + if (typeof parent !== 'string') { + if (parent) { + result.refErrors.push({ + file: node.label, + parent: String(parent), + error: `Parent field must be a wikilink string, got ${typeof parent}`, + }); + } + continue; + } const parentKey = node.resolvedParent; if (!parentKey) { @@ -110,6 +133,7 @@ export async function validate(path: string, options: { schema: string }): Promi console.log(`✅ Valid: ${result.validCount}`); console.log(`❌ Node Errors: ${result.nodeErrorCount}`); console.log(`🔗 Reference Errors: ${result.refErrors.length}`); + console.log(`🔁 Duplicate Keys: ${result.duplicateErrors.length}`); console.log(`⚙️ Config Errors: ${result.configErrors.length}`); console.log(`📋 Rule Violations: ${result.ruleViolations.length}`); console.log(`🏗️ Hierarchy Violations: ${result.hierarchyViolations.length}`); @@ -143,6 +167,16 @@ export async function validate(path: string, options: { schema: string }): Promi }); } + if (result.duplicateErrors.length > 0) { + console.log(`\n🔁 Duplicate node keys (same title in multiple files):`); + result.duplicateErrors.forEach(({ title, files }) => { + console.log(` "${title}":`); + for (const f of files) { + console.log(` ${f}`); + } + }); + } + if (result.configErrors.length > 0) { console.log(`\n⚙️ Config errors:`); for (const e of result.configErrors) { @@ -183,6 +217,7 @@ export async function validate(path: string, options: { schema: string }): Promi if ( result.nodeErrorCount > 0 || result.refErrors.length > 0 || + result.duplicateErrors.length > 0 || result.configErrors.length > 0 || result.ruleViolations.length > 0 || result.hierarchyViolations.length > 0 diff --git a/tests/fixtures/general/duplicate-embedded.md b/tests/fixtures/general/duplicate-embedded.md new file mode 100644 index 0000000..f58ccb7 --- /dev/null +++ b/tests/fixtures/general/duplicate-embedded.md @@ -0,0 +1,12 @@ +--- +type: space_on_a_page +--- + +# Vision ^mission +type: vision + +## Duplicate Heading ^goal1 +type: opportunity + +## Duplicate Heading ^goal2 +type: opportunity diff --git a/tests/fixtures/general/duplicate-titles/Vision.md b/tests/fixtures/general/duplicate-titles/Vision.md new file mode 100644 index 0000000..7ac5143 --- /dev/null +++ b/tests/fixtures/general/duplicate-titles/Vision.md @@ -0,0 +1,5 @@ +--- +title: Vision +type: vision +status: active +--- diff --git a/tests/fixtures/general/duplicate-titles/dir1/solution.md b/tests/fixtures/general/duplicate-titles/dir1/solution.md new file mode 100644 index 0000000..18d53a8 --- /dev/null +++ b/tests/fixtures/general/duplicate-titles/dir1/solution.md @@ -0,0 +1,6 @@ +--- +title: Same Title +type: solution +status: active +parent: "[[Vision]]" +--- diff --git a/tests/fixtures/general/duplicate-titles/dir2/solution.md b/tests/fixtures/general/duplicate-titles/dir2/solution.md new file mode 100644 index 0000000..18d53a8 --- /dev/null +++ b/tests/fixtures/general/duplicate-titles/dir2/solution.md @@ -0,0 +1,6 @@ +--- +title: Same Title +type: solution +status: active +parent: "[[Vision]]" +--- diff --git a/tests/fixtures/general/malformed-parent-dir/malformed-parent-array.md b/tests/fixtures/general/malformed-parent-dir/malformed-parent-array.md new file mode 100644 index 0000000..a02abfb --- /dev/null +++ b/tests/fixtures/general/malformed-parent-dir/malformed-parent-array.md @@ -0,0 +1,7 @@ +--- +title: Test Node +type: solution +status: active +# This is intentional - YAML will parse [[Vision]] as an array, not a string +parent: [[Vision]] +--- diff --git a/tests/validate-general.test.ts b/tests/validate-general.test.ts index 17887ef..a5535a6 100644 --- a/tests/validate-general.test.ts +++ b/tests/validate-general.test.ts @@ -100,6 +100,7 @@ describe('Schema validation', () => { label: 'anchor_vision.md', schemaData: { title: 'anchor_vision', type: 'vision', status: 'active' }, linkTargets: ['anchor_vision'], + resolvedType: 'vision', }, { label: 'Our Mission', @@ -110,6 +111,7 @@ describe('Schema validation', () => { parent: '[[anchor_vision]]', }, linkTargets: ['anchor_vision#Our Mission mission', 'anchor_vision#^mission'], + resolvedType: 'mission', }, { label: 'Another Goal', @@ -120,6 +122,7 @@ describe('Schema validation', () => { parent: '[[anchor_vision#^mission]]', }, linkTargets: ['anchor_vision#Another Goal goal1', 'anchor_vision#^goal1'], + resolvedType: 'goal', }, { label: 'solution_page.md', @@ -130,6 +133,7 @@ describe('Schema validation', () => { parent: '[[anchor_vision#^goal1]]', }, linkTargets: ['solution_page'], + resolvedType: 'solution', }, ]; @@ -148,6 +152,7 @@ describe('Schema validation', () => { label: 'anchor_vision.md', schemaData: { title: 'anchor_vision', type: 'vision', status: 'active' }, linkTargets: ['anchor_vision'], + resolvedType: 'vision', }, { label: 'some-solution.md', @@ -158,6 +163,7 @@ describe('Schema validation', () => { parent: '[[anchor_vision#^noanchor]]', }, linkTargets: ['some-solution'], + resolvedType: 'solution', }, ]; @@ -174,6 +180,7 @@ describe('Schema validation', () => { label: 'vision_page.md', schemaData: { title: 'vision_page', type: 'vision', status: 'active' }, linkTargets: ['vision_page'], + resolvedType: 'vision', }, { label: 'Embedded Goal', @@ -184,6 +191,7 @@ describe('Schema validation', () => { parent: '[[vision_page]]', }, linkTargets: ['vision_page#Embedded Goal'], + resolvedType: 'goal', }, { label: 'solution_page.md', @@ -194,6 +202,7 @@ describe('Schema validation', () => { parent: '[[Embedded Goal]]', }, linkTargets: ['solution_page'], + resolvedType: 'solution', }, ]; @@ -281,4 +290,38 @@ describe('Schema validation', () => { ).toBe(true); }); }); + + describe('duplicate title detection', () => { + it('detects duplicate titles from same filename in different directories', async () => { + const { nodes } = await readSpaceDirectory(join(import.meta.dir, 'fixtures/general/duplicate-titles')); + const titleCounts = new Map(); + for (const node of nodes) { + const title = node.schemaData.title as string; + if (!titleCounts.has(title)) { + titleCounts.set(title, []); + } + titleCounts.get(title)!.push(node); + } + + const duplicates = Array.from(titleCounts.entries()).filter(([_, nodes]) => nodes.length > 1); + expect(duplicates.length).toBeGreaterThan(0); + expect(duplicates.some(([title]) => title === 'Same Title')).toBe(true); + }); + + it('detects duplicate titles from embedded nodes', () => { + const { nodes } = readSpaceOnAPage(join(import.meta.dir, 'fixtures/general/duplicate-embedded.md')); + const titleCounts = new Map(); + for (const node of nodes) { + const title = node.schemaData.title as string; + if (!titleCounts.has(title)) { + titleCounts.set(title, []); + } + titleCounts.get(title)!.push(node); + } + + const duplicates = Array.from(titleCounts.entries()).filter(([_, nodes]) => nodes.length > 1); + expect(duplicates.length).toBeGreaterThan(0); + expect(duplicates.some(([title, nodes]) => title === 'Duplicate Heading' && nodes.length === 2)).toBe(true); + }); + }); });