fix: close out the eight-issue audit (validator, test runner, skill runner, generator)

[SingleSourceStudios]

Closes out all eight findings from @antnewman's validator / test-runner / skill-runner / generator audit. One commit per issue, each with a regression fixture or unit test.

Validator

• validator: fixture.expect shape not cross-checked against contracts.outputs #18 fixture.expect is now cross-checked against contracts.outputs, mirroring the existing input check. Comparison operators excepted.
• validator: quality.gates section entirely unvalidated #19 quality.gates is now validated: array shape, required id and check, action in retry | fail, integer max_retries, string on_exhaustion, and operation cross-checked against interface.surface. Normative fields were taken from docs/COVENANT.md (id/check/action/max_retries/on_exhaustion), not the name/severity/on_fail guessed in the issue.
• validator + test runner: fixture.retry silently coerced via "|| 0" #21 fixture.retry must be a non-negative integer; the runner now uses ?? 0 instead of || 0.
• validator: depends_on cycle detection missing (runner is single point of failure) #24 depends_on cycle detection at validation time, reporting the cycle path. The existence check also now handles array-valued depends_on, which the spec schema allows but the old check mishandled.

Test runner

• test runner: validateOutput partial-match allows undeclared output fields #20 Opt-in strict_output: true per fixture fails the run when actual output carries fields absent from contracts.outputs. Default behaviour unchanged (non-breaking); the default-mode question is deferred to the v1.1 spec discussion. Landing this surfaced two latent bugs, fixed in the same commit: executeFixture read this.expect (never set), so fixture.expect was never enforced at all; and partialEqual was partial only at the top level, contradicting the documented partial-match semantics. The docx-generation example expected file_path literals the contract-driven simulator cannot honour; those lines are dropped and validation_result.valid remains the real assertion.
• test runner: hasCircularDependencies silently ignores deps not in fixture list #25 hasCircularDependencies now throws on a depends_on naming an unknown fixture instead of silently treating it as no dependency.

DX papercuts

• skill-runner: stdout JSON parse failure mis-attributed to "process runner failed" #22 Process-runner stdout that fails to parse as JSON is now reported as a parse failure with the first 200 chars of raw stdout, instead of process runner failed: exit 0.
• generate.js: fs.stat(...).catch(() => false) treats permission errors as nonexistence #23 Both fs.stat(...).catch(() => false) sites in generate.js narrowed to ENOENT; permission and I/O errors propagate.

Verification

• Jest: 72 passed (13 new) across 7 suites
• npm run test:fixtures: 4/4 example skills green
• npm run test:conformance: 15/15

Fixes #18
Fixes #19
Fixes #20
Fixes #21
Fixes #22
Fixes #23
Fixes #24
Fixes #25

---

Summary by cubic

Closes out audit issues #18–#25 across the validator, test runner, skill runner, and generator. Adds strict output checks, stronger validation, and clearer error messages for safer, more predictable runs.

• New Features

  • strict_output: true per fixture to fail on undeclared output fields; default behavior unchanged (test runner: validateOutput partial-match allows undeclared output fields #20).
  • Validate quality.gates: required id/check, action in retry|fail, integer max_retries, string on_exhaustion, and operation cross-checked with interface.surface (validator: quality.gates section entirely unvalidated #19).

• Bug Fixes

  • Validator: cross-check fixture.expect against contracts.outputs; enforce retry as a non-negative integer; detect depends_on cycles and support array-valued depends_on (validator: fixture.expect shape not cross-checked against contracts.outputs #18, validator + test runner: fixture.retry silently coerced via "|| 0" #21, validator: depends_on cycle detection missing (runner is single point of failure) #24).
  • Test runner: enforce fixture.expect (fixes this.expect bug) and make partial matching deep; throw on unknown depends_on instead of silently ignoring it (test runner: validateOutput partial-match allows undeclared output fields #20, test runner: hasCircularDependencies silently ignores deps not in fixture list #25).
  • Skill runner: surface JSON parse failures with a clear message and a 200-char stdout sample instead of “exit 0” (skill-runner: stdout JSON parse failure mis-attributed to "process runner failed" #22).
  • CLI generator: narrow fs.stat catch to ENOENT so permission/I/O errors aren’t masked (generate.js: fs.stat(...).catch(() => false) treats permission errors as nonexistence #23).

^{Written for commit 2c42f33. Summary will update on new commits.}

Summary by CodeRabbit

• New Features

  • Added quality gates validation for defining and enforcing quality checks on skill operations
  • Introduced strict output validation for fixtures to detect undeclared fields in responses

• Bug Fixes

  • Improved error handling during skill generation to distinguish permission and I/O failures from missing directories
  • Enhanced JSON parsing error reporting with clearer messages and raw output context
  • Strengthened fixture dependency validation with cycle detection and better error messages

• Tests

  • Added comprehensive test coverage for error handling scenarios, quality gates validation, and strict output behavior

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

This PR addresses seven interconnected validation and error-handling issues across the COVENANT validator, test runner, and CLI. Changes narrow error handling to surface real failures, enhance the validator to enforce fixture schema contracts and quality gates, and improve the test runner to support strict output mode with better error attribution.

Changes

Validation and Test Execution Enhancements

Layer / File(s)	Summary
CLI and skill runner error handling packages/cli/src/generate.js, packages/cli/tests/generate.test.js, packages/core/src/skill-runner.js, packages/core/tests/skill-runner.test.js	Error handling in directory existence checks and JSON parsing now distinguishes real failures (permission, I/O, invalid format) from "missing" cases; error messages include relevant context (raw stdout samples, error codes) instead of generic fallback messages.
Validator fixture and gates schema checking packages/core/src/validator.js	Validator now cross-checks fixture.expect field names against declared contracts.outputs, enforces fixture.retry as non-negative integer and strict_output as boolean, validates quality.gates structure (required/optional fields, enum values, operation references), and detects cyclic depends_on graphs via new findFixtureCycle helper.
Test runner fixture parsing and dependency handling packages/core/src/test.js	Fixture parsing preserves contracts.outputs for strict output checking, defaults retry via nullish coalescing, throws on unknown depends_on references instead of silently ignoring, and rewrites partialEqual to apply validateOutput at nested object levels for consistent contract enforcement.
Test runner output validation and strict mode packages/core/src/test.js	Output validation now consults fixture.expect directly (instead of a runner-level property), implements strict_output mode that fails when actual output contains undeclared fields, and reports failures with fixture.expect JSON for clarity.
Test fixtures for validation rules packages/core/tests/fixtures/{cyclic-fixture-deps,invalid-expect-cross-ref,invalid-gates,invalid-retry-type,invalid-strict-output-type,strict-output-skill,valid-gates}.md	New fixture definitions exercise all validation paths: cross-reference mismatches, type errors, gate validation failure modes, cycle detection, and strict output behavior.
Test coverage for validator and test runner enhancements packages/core/tests/{validator,test-runner,skill-runner}.test.js	Jest suites add coverage for expect cross-refs, retry type validation, strict_output type checking, quality.gates malformed entries, depends_on cycle detection, and runtime strict output enforcement with proper error reporting.
Example fixture expectation updates examples/docx-generation/COVENANT.md	Remove file_path assertions from docx example fixtures; retain only validation_result.valid checks.

Sequence Diagrams

sequenceDiagram
    participant Validator
    participant TestRunner
    participant Execution
    Validator->>Validator: Parse and extract fixtures
    Validator->>Validator: Validate fixture.expect against contracts.outputs
    Validator->>Validator: Type-check retry, strict_output
    Validator->>Validator: Validate quality.gates structure and operations
    Validator->>Validator: Detect cycles in depends_on graph
    Validator-->>TestRunner: Report validation errors or pass
    alt Validation passes
        TestRunner->>TestRunner: Extract fixtures with contractsOutputs
        TestRunner->>TestRunner: Build topological sort (reject unknown deps)
        TestRunner->>Execution: Execute fixtures in dependency order
        Execution->>Execution: Run fixture operation
        Execution->>TestRunner: Return actual output
        TestRunner->>TestRunner: Validate output vs fixture.expect
        alt strict_output enabled
            TestRunner->>TestRunner: Check for undeclared fields
            TestRunner-->>Execution: Fail if leaked fields
        end
        TestRunner-->>TestRunner: Report pass/fail with error details
    else Validation fails
        Validator-->>TestRunner: Reject covenant
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

A rabbit hops through contracts neat,
Ensuring fixtures all compete—
No cycles hide, no fields leak through,
Each gate is checked, each promise true. 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the main changeset as closing eight audit issues across validator, test runner, skill runner, and generator components.
Description check	✅ Passed	The PR description is comprehensive, covering all required sections: summary of changes per component, linked issues with issue numbers, spec impacts, verification results, and a detailed cubic.dev summary.
Linked Issues check	✅ Passed	All eight linked issues (#18–#25) are addressed: validator expect cross-check (#18), quality.gates validation (#19), strict_output opt-in (#20), retry type-check (#21), JSON parse error reporting (#22), fs.stat error narrowing (#23), cycle detection (#24), and unknown depends_on throw (#25).
Out of Scope Changes check	✅ Passed	All changes are scoped to the eight audit issues: validator enhancements, test runner fixes, skill runner error reporting, CLI generator error handling, and corresponding regression tests/fixtures. No extraneous changes detected.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issues-18-25

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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