feat(ce-review): validate the review run artifact against an executable schema - #830
Merged
Merged
Conversation
ce:review writes two kinds of artifact. Per-persona records are validated by the parent before persistence. The run-level review-summary.json is validated by nothing, and the difference shows on disk: across 26 runs, 138 per-persona files share 21 shapes with 111 of them identical, while 7 run-level artifacts have 7 distinct shapes and were written under two different filenames. The plan gives the run artifact a Zod source of truth, generates a committed JSON Schema from it behind a drift gate, and exposes validation as a CLI command that ships to consumers. Two review rounds reshaped it: the original design put the validator in scripts/, which is not published, so no consumer could ever run it.
review-summary.json carries the verdict, the input ledger, and the reconciliation arithmetic for a review run, but nothing has ever checked its shape. findings-schema.json covers only the per-persona record. The result is that no two run artifacts on disk share a shape, and the fields the contract documents most carefully — input_findings, disposition_counts, run_status — appear in none of them. The shape now has a Zod source of truth, with the ledger modeled as a discriminated union on record_type so admitted rows and rejected-payload summaries are machine-separable without inferring from which keys happen to be present. Shared vocabulary is composed rather than duplicated, every user-influenced string carries a length bound, and cross-field invariants that JSON Schema cannot express are enforced directly: a rejection summary must report at least one rejected finding, and its severity list must match its count. The committed JSON Schema is generated from that source and gated against drift. It ships under skills/ for linking and inspection but is deliberately not inlined into any persona prompt, where it would add roughly 10 KB to every reviewer dispatch for content no reviewer uses. The generator derives its options from one shared helper used by both the write and check paths, and reads the committed file from disk rather than through a static import. Both guard against failures this repository has already hit once, recorded in docs/solutions/best-practices. Fixtures capture the seven historical shapes, sanitized of review content, so the measured non-conformance is pinned without depending on local state that git ignores.
A schema nothing runs is documentation. This exposes validation as `systematic validate-review-artifact <path>`, which ships in dist/ and so reaches every consumer — the earlier design put it in scripts/, which the published package does not include, so it would have worked only inside this repository. Four exit statuses let a caller tell outcomes apart that would otherwise collapse into "failed": valid, schema violation, operational problem such as a missing file or malformed JSON, and an artifact predating the contract with no schema_version. That last one is what makes the historical corpus excludable by a machine rather than by a note. Error output names the JSON path and issue code and nothing else. Zod does not give that property for free — its issues carry generated prose that can interpolate values, plus fields like expected and maximum, and reportInput would add the raw input outright. Since these artifacts quote source code, each issue is projected to an explicit allowlist. The one exception is our own refinement messages, which are authored constants with no artifact data in them, and which would otherwise be reduced to a bare "custom" code that says nothing. The path argument is required and must resolve inside the artifact directory, which keeps the command from doubling as a probe for whether arbitrary files exist and parse.
…rites The parent already treats every reviewer return as untrusted and validates it before persisting. It applied none of that discipline to the artifact it writes itself, which is why every run has been free to invent a shape. The contract now has the parent stamp schema_version and run the validator after writing, with the failure path stated: repair and re-run, never report a verdict over an artifact that failed, and never delete the artifact to escape the check, because a failing artifact is evidence. It also says plainly what this does not do. An agent that never runs the command can still finalize an artifact. The value is that the check is independently runnable, so a human, a CI job, or a later audit can verify any artifact without the producing agent's cooperation — not that compliance is guaranteed. Claiming otherwise would repeat a documented mistake in this repository, where a validator a sub-agent could decline to call was treated as enforcement. The historical corpus is excluded by an explicit decision rather than a half-trusted read: 7 synthesis artifacts across 26 runs, two filenames, no two shapes alike, and 19 runs with no synthesis artifact at all. A reader for that would need seven special cases and still could not answer the questions the corpus exists to answer.
Review found the legacy classifier too permissive. It answered yes for null and for any non-object, so a JSON array or a bare string parsed cleanly and exited with the legacy status — reported as predating the contract and excluded from analysis rather than flagged as broken. Legacy now means an object without a version field. Everything else fails. The exception that lets authored refinement messages through the output whitelist was safe by convention only. Both messages are now named constants the schema and its tests share, so a future refinement that invents its own message string fails a test rather than quietly widening what the validator is allowed to print. The finding type modeled its filtered case at runtime but not in the type: a finding could be marked unvalidated with no reason and still typecheck, then fail validation. It is a discriminated union now, so that state cannot be constructed. Two tests proved less than they claimed. The no-echo test asserted that a few sample strings were absent, which a regression printing some other Zod field would have passed; it now asserts the exact output shape. The historical fixture test passed if a version issue appeared anywhere in the error list, which would hold even when a fixture failed for an unrelated reason; it now asserts each fixture's expected failure. The command also documents its four exit statuses and points at the schema on failure, because its caller is an agent expected to repair the artifact and retry, and a bare issue code at a JSON path is not enough to act on. The drift check no longer ignores trailing whitespace, which let a committed schema missing its final newline report as current.
fro-bot
approved these changes
Aug 22, 2026
fro-bot
left a comment
fro-bot
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: PASS
This PR gives review-summary.json — previously the only unvalidated artifact ce:review writes — a Zod source of truth, a generated JSON Schema behind a committed drift gate, and an independently runnable systematic validate-review-artifact <path> CLI command. I verified the claims rather than just reading the description:
bun run typecheck— clean.bun run lint— clean for every file this PR touches (the 11 pre-existing complexity warnings are all inreceipt-readback.ts,workflow-guard.ts, and unrelated test files, none of which are in this diff).bun test tests/unit— 1943 pass, 0 fail, matching the PR description exactly.bun run review-schema:driftandbun run registry:drift— both clean.- New file conventions:
node:protocol imports,.jsextensions on relative imports, zeroany/@ts-ignore, functions only (no classes) — all followed.
Code quality highlights
src/lib/review-artifact-schema.ts: the heterogeneous ledger is a proper discriminated union (AdmittedInputFindingSchema/RejectedInputFindingSchema) rather than an inferred shape from optional fields, and the two invariants JSON Schema can't express (severity-count-matches-count, filtered-requires-reason) are enforced viasuperRefinewith authored, tested messages (REVIEW_ARTIFACT_CUSTOM_MESSAGES).src/cli.tsvalidate-review-artifact: path handling is genuinely defense-in-depth — rejects..traversal in the raw argument, walks every path segment for symlinks before resolving, and independently confirms the realpath'd target is still inside.context/systematic/ce-reviewviaisWithinDirectory. Error output never echoes artifact content (verified by the "does not echo artifact strings" test), which matters since this reads content from potentially adversarial persona/user-influenced fields.- Exit-status design (0/1/2/3) correctly distinguishes "legacy, pre-contract artifact" from "actually malformed" — a JSON array, bare string, number, and
nullare all correctly treated as validation failures (exit 1), not excused as legacy (exit 3). This is exactly the escape hatch the description says the code-round review caught, and it's now pinned down by four explicit tests. - The generator (
scripts/generate-review-artifact-schema.ts) reads the committed file at check-time rather than importing it, avoiding ESM module-cache staleness — a subtle correctness detail that's easy to get wrong.
Non-blocking concerns
src/lib/AGENTS.md's module tables (Discovery/Loading/Config & Integration Layer) weren't updated to include the newreview-artifact-schema.tsmodule, even thoughARCHITECTURE.mdwas. Minor doc-completeness gap, not a functional issue.pathContainsSymlinkinsrc/cli.tswalks the path manually for symlinks even though the subsequentrealpathSync+isWithinDirectorycheck would already reject an escaping symlink target. This is intentional stricter-than-necessary behavior (rejects symlinks unconditionally, not just escaping ones, which also closes a TOCTOU window) — worth a one-line comment explaining why both checks exist, but not a bug.tests/unit/generate-review-artifact-schema.test.tsmutates the real committed schema file in place (restored viatry/finally); consistent with this repo's existing drift-test pattern, so not a new risk, just noting it for anyone unfamiliar with the convention.
Missing tests
None identified. Coverage is unusually thorough: conforming fixture, 7 historical fixtures with exact expected issue signatures per fixture, symlink escape, parent-directory traversal, non-regular-file target, missing file, malformed JSON, missing argument, custom-message preservation, and secret-non-echo behavior are all directly tested.
Risk assessment: LOW
Purely additive — new schema module, new CLI subcommand, new CI gate, docs/skill updates. No existing code paths are modified in a way that changes behavior (the SKILL.md/contract doc changes add a validation requirement on the parent's own write path, not a breaking change to any consumer). The CLI command is scoped to a fixed subdirectory with fail-closed path resolution, so the security blast radius of a malicious <path> argument is well contained and tested. Blast radius of a schema defect would be limited to the validator rejecting/accepting artifacts incorrectly — verifiable and low-severity, not silently corrupting data.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | marcusrbrown/systematic |
| Run ID | 32592191594 |
| Cache | hit |
| Session | ses_fd52b557bffe0zkHfhtAUufqfh |
Review noted the new module reached the architecture codemap but not the module table in src/lib/AGENTS.md, which is where someone working in that directory looks first. Also records why the symlink rejection and the realpath containment check both exist. They overlap, and the overlap is deliberate: containment rejects a symlink whose target escapes the artifact root, while the segment walk rejects every symlink in the path outright and narrows the window between checking and reading. Without the note the next reader would reasonably delete one.
fro-bot
approved these changes
Aug 22, 2026
fro-bot
left a comment
fro-bot
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: PASS
Re-review after the 5eb886e docs(lib): register the review artifact schema module push. This commit addresses both non-blocking notes from my prior review (run 32592191594):
src/lib/AGENTS.mdnow documentsreview-artifact-schema.tsin the Config & Integration Layer table, including its exports and a description of the discriminated-union ledger andsuperRefineinvariants.pathContainsSymlinkinsrc/cli.tsnow carries a comment explaining why it's deliberately redundant with the laterrealpathSync+isWithinDirectorycontainment check (narrows the symlink-swap TOCTOU window rather than being dead logic).
Re-ran verification after the push:
bun run typecheck— clean.bun test tests/unit/review-artifact-schema.test.ts tests/unit/generate-review-artifact-schema.test.ts tests/unit/validate-review-artifact.test.ts— 38 pass, 0 fail.
No new concerns from this diff (src/cli.ts +4 comment lines, src/lib/AGENTS.md +1 table row — no behavior change). All findings from the prior full review still stand: discriminated-union schema design, defense-in-depth CLI path handling, correct legacy-vs-malformed exit-status separation, and thorough test coverage.
Blocking issues
None
Non-blocking concerns
None — both items from the prior review round were resolved in this commit.
Missing tests
None identified.
Risk assessment: LOW
This commit is documentation and a code comment only; no functional or behavioral change. The underlying feature remains purely additive (new schema module, new CLI subcommand, new CI gate) with no changes to existing consumer-facing behavior.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | marcusrbrown/systematic |
| Run ID | 32594669442 |
| Cache | hit |
| Session | ses_fd52b557bffe0zkHfhtAUufqfh |
marcusrbrown
enabled auto-merge (squash)
August 23, 2026 05:37
fro-bot
approved these changes
Aug 23, 2026
fro-bot
left a comment
fro-bot
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: PASS
Re-review after d22a37e, a merge of main into this branch. This is a housekeeping merge only — it pulls in an unrelated main commit (cbbe54d, a fro-bot/agent version bump touching .github/workflows/fro-bot.yaml, bun.lock, package.json). It does not touch any file this PR authored or modifies the feature in any way.
Verified directly rather than assuming:
git show d22a37e --stat— confirms the merge only carries inmain's unrelated dependency-bump diff (3 files, all outside this PR's scope).bun run typecheck— clean.bun test tests/unit/review-artifact-schema.test.ts tests/unit/generate-review-artifact-schema.test.ts tests/unit/validate-review-artifact.test.ts— 38 pass, 0 fail.- Confirmed the PR's own
package.jsonadditions (review-schema:generate,review-schema:drift) survived the merge intact, no conflict markers, no silent overwrite.
All findings from the two prior review rounds still stand and remain resolved: discriminated-union schema design, defense-in-depth CLI path handling with documented rationale, correct legacy-vs-malformed exit-status separation, thorough test coverage, and both previously-flagged non-blocking items (AGENTS.md registration, symlink-check comment) remain fixed.
Blocking issues
None
Non-blocking concerns
None
Missing tests
None identified.
Risk assessment: LOW
This change is a merge-only sync with main; it introduces zero new lines of feature logic. The underlying feature remains purely additive (new schema module, new CLI subcommand, new CI gate) with no changes to existing consumer-facing behavior.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | marcusrbrown/systematic |
| Run ID | 32620794191 |
| Cache | hit |
| Session | ses_fd52b557bffe0zkHfhtAUufqfh |
This was referenced Aug 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ce:reviewwrites two kinds of artifact. Per-persona records are validated by the parent before it persists them. The run-levelreview-summary.json— which carries the verdict, the input ledger, and the reconciliation arithmetic — is validated by nothing.The difference is visible on disk. Across 26 run directories:
111 of the 138 per-persona files share a single shape. No two run-level artifacts have ever shared one, and the seven are split across two different filenames — even the name drifted. The fields the contract documents most carefully,
input_findingsanddisposition_countsandrun_status, appear in none of them.verdictappears in all fourreview-summary.jsonfiles and was absent from the contract entirely.These two paths are not a clean experiment: per-persona files are written many per run, the run-level shape was redesigned twice recently, and the per-persona schema only became strict lately. The measurement is a strong correlation between an unchecked write path and shape divergence, not proof of cause. It was enough to act on.
What this changes
The shape now has a Zod source of truth. The heterogeneous ledger is a discriminated union on
record_type, so admitted rows and rejected-payload summaries are machine-separable rather than inferred from which keys happen to be present. Shared vocabulary is composed rather than copied. Every user-influenced string carries a length bound. Invariants JSON Schema cannot express are enforced directly — a rejection summary must report at least one rejected finding, and its severity list must match its count.A committed JSON Schema is generated from that source behind a drift gate. It ships under
skills/for linking and inspection but is deliberately never inlined into a persona prompt, where it would add roughly 10 KB to every reviewer dispatch for content no reviewer reads.Validation is exposed as
systematic validate-review-artifact <path>, which ships indist/and so reaches every consumer. Four exit statuses distinguish outcomes that would otherwise collapse into "failed": valid, schema violation, operational problem, and an artifact predating the contract. That last one is what makes the historical corpus excludable by a machine rather than by a note.The contract now has the parent stamp a version and run the validator after writing, with the failure path stated: repair and re-run, never report a verdict over an artifact that failed, and never delete the artifact to escape the check.
What it does not do
The contract says this plainly, and so does this description: an agent that never runs the command can still finalize an artifact. This is enforcement by visible failure, not by containment. The durable value is that the check is independently runnable — a human, a CI job, or a later audit can verify any artifact without the producing agent's cooperation. Claiming more would repeat a documented mistake in this repository, where a validator a sub-agent could decline to call was treated as enforcement.
On the original issue
#793 asked for a
schema_versionfield, a compatibility path for in-flight reviewers, and a decision on the historical corpus.The first was premature. There was no conforming producer to version — a version field describes a shape, and no stable shape existed. It lands here anyway, at the end rather than the beginning, because at creation time it costs nothing.
The second is not reachable the way the issue assumed. The parent inlines the schema into each persona's dispatch prompt and then validates returns against that same bundled file, so producer and consumer are one version by construction within a run. The reachable failure is a model ignoring the contract, which a version field cannot detect.
The third is resolved as an explicit exclusion. Seven synthesis artifacts, two filenames, no two shapes alike, and 19 of 26 runs with no synthesis artifact at all. A reader for that would need seven special cases and still could not answer the questions the corpus exists to answer.
Verification
1943 unit tests pass. Typecheck, lint, content-integrity, and all three drift gates clean. End to end through the built binary under Node: a legacy artifact exits 3, a conforming artifact exits 0, a missing argument exits 2, and a JSON array exits 1 rather than being excused as legacy.
Two review rounds ran against the plan before implementation and one against the code after. The code round found a real escape hatch — anything parsing as JSON but not an object was classified as merely old and excluded from scrutiny instead of failing.
Closes #793