Skip to content

feat(fix-record): add the v2 shape and version-aware re-derivation (1/2) - #217

Merged
arthurpanhku merged 4 commits into
mainfrom
claude/fix-record-v2-shape
Sep 3, 2026
Merged

feat(fix-record): add the v2 shape and version-aware re-derivation (1/2)#217
arthurpanhku merged 4 commits into
mainfrom
claude/fix-record-v2-shape

Conversation

@arthurpanhku

Copy link
Copy Markdown
Owner

Summary

Readers first. This PR does not change what any command emits — no producer supplies the new evidence yet, so every record issued today is still v1, byte for byte. The producer switch is 2/2.

The defect

A fix record's verdict is re-derived and compared on every verification (verifyFixRecord), so the verdict has to be a function of the record. It was not quite. The rule read after.remainingTargets, and what that field held was decided by whoever produced the record:

Producer remainingTargets A repair that introduces a new critical finding
dvalin verify <workflow> gate.blocking — original targets plus newly-introduced findings fails
dvalin . --fix --verify original targets only verified: true

The second is the headline flow — the README's, and the GitHub Action's. Worse, src/commands/dvalin.ts files the record at :331 and only throws the gate failure at :340, so a record saying verified: true is already on disk and printed for a repair the tool rejects a moment later. It re-derives cleanly, and flows into the Evidence Pack, the Action's PR comment, and anything else collecting records.

Why not just widen remainingTargets

That moves which producer is wrong and leaves the ambiguity in place. The problem is not a missing check, it is a field whose meaning depends on its author. So v2 splits the question and puts the rule for answering it inside the record.

The v2 shape

  • after.introduced — what the re-scan sees that the first scan did not. Three-state, and the third is the point: [] means looked-for and none found, a list is what was found, and null means not determined — which cannot verify. Without that, a producer which never looks scores better than one which looks and finds something, and a rule that rewards not looking is not a rule. Same principle the format already applies to coverage: absent because unlooked-for is not absent.
  • gate — the threshold and mode the verdict was reached under, so a third party re-derives the same answer without knowing which command produced the record. Today that only works by luck, which is the root cause above.
  • outcomeverified / target-remains / regressed / unverifiable, so a machine reader need not parse reasons prose. verdict.verified stays a boolean; every consumer already branches on it and none has to change.

The list is stored complete, not pre-filtered by severity, with the threshold recorded beside it. Store the observation, derive the judgement, record the parameters of the derivation — the rule this format already applies to exit codes. A stricter reader can re-decide from the record instead of re-scanning.

v1 is frozen, not migrated

Every v1 record was hashed with a verdict evaluateFixVerdictV1 produced, and verification re-derives and compares. Changing that function would retroactively invalidate every record ever issued. "Records re-derive offline, forever" is the promise the format exists for; a bug in the rules does not justify spending it.

So deriveFixVerdict dispatches on the record's own declared version, and renderFixRecord says plainly that a v1 record was issued under rules which never asked about regressions — a reader comparing a v1 and a v2 record must not read them as having answered the same question.

Testing

npm run check — both typechecks and 544 passed (525 on main + 19 new in tests/securityFixRecordV2.test.ts). One pre-existing unhandled EPIPE from tests/mcp/stdio.test.ts teardown, present on clean main.

v1 records are provably untouched: the v1 path builds the identical object (the new keys are omitted, not set to undefined), so the hash is unchanged, and the existing hash-stability test is the guard.

Mutation-tested, per the project's own T-2 standard. Each mutation kills its own tests and, for the first three, leaves the v1 suite green — which is the evidence that v1 behavior did not move:

Mutation v2 tests v1 tests
introduced: null no longer blocks 2 failed green
a blocking regression never blocks 4 failed green
dispatch always uses the v1 rule 6 failed green
v2 shape not validated 1 failed green

Spec

FVP-1 gains FV-10a (a regression, or failing to determine one, fails the verdict), FV-10b (keep the complete set plus the threshold), and FV-12a (re-derive under the record's own declared version).

Security and AI Governance

  • This change does not expand file, shell, network, model, or approval permissions. — Pure data-shape and verdict-derivation change; no new I/O, no new subprocess, no new egress.
  • If it changes agent behavior, prompts, policy, providers, audit logging, or release/build security, I updated the relevant governance evidence in docs/. — No agent or policy behavior changes. docs/spec/FIX-VERIFICATION.md updated with the three new assertions.
  • If it introduces a new model/provider/tool or new data flow, I completed docs/governance/AI-CHANGE-IMPACT-ASSESSMENT.md. — Not applicable; no new model, provider, or tool.

What 2/2 does

Switches both producers to supply regression, which makes the ordering bug in dvalin.ts dissolve rather than need a separate patch: once the regression is in the record, the record is the decision, and filing it before the gate throws stops being a contradiction.

🤖 Generated with Claude Code

https://claude.ai/code/session_0186VyaiQq3MTo4i4mK8P3aW


Generated by Claude Code

A fix record's verdict is re-derived and compared on every verification,
so the verdict has to be a function of the record. It was not quite: the
rule read `after.remainingTargets`, and what that field contained was
decided by whoever produced the record. The workflow path put the gate's
blocking set there, which includes findings the repair introduced. The
`--fix --verify` path put only the original targets. Same field, same
verification code, two meanings.

Widening `remainingTargets` to include new findings would not fix that.
It would move which producer is wrong and leave the ambiguity in place.
So v2 splits the question instead, and puts the rule for answering it
inside the record:

- `after.introduced` — what the re-scan sees that the first scan did not,
  as a complete list rather than one pre-filtered by severity.
- `gate` — the threshold and mode the verdict was reached under, so a
  third party re-derives the same answer without knowing which command
  produced the record. Today that only works by luck.
- `outcome` — which of `verified` / `target-remains` / `regressed` /
  `unverifiable` occurred, so a machine reader need not parse prose.
  `verdict.verified` stays a boolean; every consumer already branches
  on it.

`introduced` is three-state and the third state is the point. `[]` means
looked-for and none found; `null` means not determined, and it cannot
verify. Without that, a producer which never looks scores better than one
which looks and finds something, and a rule that rewards not looking is
not a rule. It is the same principle the format already applies to
coverage: absent because unlooked-for is not absent.

Storing the complete list and deriving the judgement from a recorded
threshold is likewise the rule this format already applies to exit codes.
A stricter reader can re-decide from the same record instead of
re-scanning.

v1 is frozen, not migrated. Every record issued under it was hashed with
a verdict `evaluateFixVerdictV1` produced, and verification re-derives
and compares; changing that function would retroactively invalidate all
of them. Records re-derive offline forever is the promise the format is
for, and a bug in the rules does not justify spending it. So
`deriveFixVerdict` dispatches on the record's own declared version, and
the renderer says plainly that a v1 record was issued under rules that
never asked about regressions.

No production path emits v2 yet -- `buildFixRecord` only issues one when
a caller supplies the regression evidence, and none do. Readers first,
writers next, so this lands verifiable on its own and the switch is a
separate, revertible change.

FVP-1 gains FV-10a (regression and non-determination fail), FV-10b (keep
the complete set plus the threshold), and FV-12a (re-derive under the
record's own version).

Refs #205-adjacent verification work; prepares the producer switch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186VyaiQq3MTo4i4mK8P3aW
Switches the two paths that issue records to supply the regression
evidence v2 requires, which is where the defect actually closes.

`dvalin verify <workflow>` was already computing the right thing and
putting it in the wrong place: `gate.blocking` holds the original targets
still present *and* the findings the repair introduced, and the whole set
went into `after.remainingTargets`. Now the two are separated -- targets
filtered by the original target keys, introduced derived from the initial
scan's fingerprints -- and the gate's own threshold and mode are recorded
beside them.

`dvalin . --fix --verify` was not computing it at all for the record. It
had the baseline in hand and used it in `evaluateVerificationGate`, which
fails on findings scoring 7 or more that the baseline lacked, but none of
that reached `buildFixRecord`. So a repair that removed an eval and added
an SQL injection produced a record saying VERIFIED, saved and printed,
and then the command threw. That record re-derived cleanly and flowed
into the Evidence Pack, the Action's PR comment, and anything else
collecting records. It now carries `introduced` and a recorded gate of
high/new, which is the rule that path already applied -- written down
rather than implied by which command happened to run.

The ordering that made it worst dissolves rather than needing a patch:
with the regression inside the record, a record filed before the gate
throws is not a contradiction, because the record already says the repair
regressed.

Record assembly on the --fix path moved into `buildRunFixRecord`. That is
not tidying. With it inline, replacing the regression evidence with
`null` broke no test -- the helper was unit-tested but the call site was
not reachable without an executor, a worktree, and a git tree. Extracting
it makes the wiring assertable, and both mutations (dropping the
evidence, and recording a `none` threshold) now fail tests.

npm run check: 554 passed. `dvalin . --diff` over the change: 0 findings.

Closes the two-definitions defect. FVP-1 already carries the rules from
the previous change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186VyaiQq3MTo4i4mK8P3aW
@arthurpanhku
arthurpanhku merged commit d836b9b into main Sep 3, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants