Skip to content

feat(fix-record): issue v2 from both producers (2/2) - #218

Merged
arthurpanhku merged 1 commit into
claude/fix-record-v2-shapefrom
claude/fix-record-v2-producers
Sep 3, 2026
Merged

feat(fix-record): issue v2 from both producers (2/2)#218
arthurpanhku merged 1 commit into
claude/fix-record-v2-shapefrom
claude/fix-record-v2-producers

Conversation

@arthurpanhku

Copy link
Copy Markdown
Owner

Summary

Stacked on #217 — base is claude/fix-record-v2-shape, so review that first. This is where the defect actually closes.

#217 added the shape and the version-aware re-derivation without changing what any command emits. This switches the two producers to supply the regression evidence, so records issued from here on are v2 and a repair that trades one vulnerability for another can no longer be called verified.

What each path was doing

dvalin verify <workflow> was 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 they are separated — targets filtered by the original target keys, introduced derived from the initial scan's fingerprints — with the gate's own threshold and mode recorded beside them.

dvalin . --fix --verify was not computing it for the record at all. It had the baseline in hand and used it in evaluateVerificationGate (which fails on findings scoring ≥ 7 the baseline lacked), but none of that reached buildFixRecord. So:

331  saveFixRecord(record)      ← says VERIFIED, written to --record, printed
340  if (!gate.passed) throw    ← "1 new high/critical finding target(s) appeared"

A record saying verified, on disk and re-deriving cleanly, for a repair the command rejects a moment later — and then collected by the Evidence Pack, the Action's PR comment, and anything else reading the store.

It now records introduced plus a 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 dissolves rather than needing a patch. With the regression inside the record, filing before the gate throws is no longer a contradiction: the record already says the repair regressed. That the problem disappears instead of needing its own fix is the sign the shape was the right one.

One extraction, and why it is not tidying

Record assembly on the --fix path moved into buildRunFixRecord. With it inline, I mutated the call site to introduced: null and no test failedintroducedSince was unit-tested, but the call site needs an executor, a worktree, and a git tree to reach. A helper that is tested while its only caller is not is the same shape of gap as the original defect: correct machinery, unverified wiring.

Extracting it makes the wiring assertable. Both mutations now fail tests.

Testing

npm run check — both typechecks and 554 passed (544 on #217 + 10 new). dvalin . --diff over the change: 0 findings.

Mutation-tested. Every switch is pinned by a test that fails when it is undone:

Mutation Result
workflow path stops supplying regression 3 failed
workflow path puts introduced back into remainingTargets (the original conflation) 1 failed
--fix call site supplies introduced: null 2 failed
--fix records a none threshold, which would block nothing 2 failed

The headline case is asserted directly on both paths: a repair that clears its target, passes the project's checks, and introduces a critical SQL injection now yields verified: false, outcome: 'regressed'.

Security and AI Governance

  • This change does not expand file, shell, network, model, or approval permissions. — Set arithmetic over scan results already in memory; no new I/O, subprocess, or 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, policy, or audit behavior change. The verdict rules are documented in FVP-1 by feat(fix-record): add the v2 shape and version-aware re-derivation (1/2) #217.
  • 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.

Known residual divergence, not fixed here

evaluateVerificationGate also fails a run when the worktree has no code changes, and that condition is still not represented in the verdict. A run where the targets vanished, the checks passed, and nothing was edited would produce verified: true while the command throws. It is far rarer than the regression case and it is a different question — whether a record should have to prove a change happened at all — so it is not folded in here. The record already carries changes (files + diff hash), so the natural fix is a further FVP assertion rather than more producer logic. Say the word and I will file it.

🤖 Generated with Claude Code

https://claude.ai/code/session_0186VyaiQq3MTo4i4mK8P3aW


Generated by Claude Code

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 e3e5edf into claude/fix-record-v2-shape Sep 3, 2026
14 checks passed
arthurpanhku pushed a commit that referenced this pull request Sep 4, 2026
The pull-request comment was built from `remainingTargets`, `coverage` and
`checks` and never read `after.introduced`, `gate` or `outcome`. Since #218 both
producers issue v2, so the surface that publishes the proof next to the diff was
showing a regressed repair the same way it shows a clean one -- "targets: 1 → 0"
above a NOT VERIFIED header, with the reason buried in the verdict prose.

The comment now names what the re-scan saw that the first scan did not, with the
gate the verdict was reached under, and the outcome. Introduced findings are
listed with the severity the gate itself read (`securitySeverity` first, then the
SARIF level) rather than the raw level, because a finding must not look milder in
the list than in the rule that blocked on it.

A v1 record gets the note instead of the fields. It was judged under rules that
never asked about regressions, and rendering it as though it asked and found
nothing is the one reading the record format spends its credibility to prevent.

The severity gate needs no change: a regressed v2 record already re-derives to
`verified: false` and fails the job. What was missing was the evidence for it.

Covered by tests that run the script out of action.yml against records this
repo's builder produced, since the comment is JavaScript inside YAML inside a
shell script and nothing else typechecks it -- including the invariant that the
script stays free of the single quote that would break its shell wrapper.

README samples in both languages updated to the output the step now produces,
with a regressed comment shown beside the verified one, and the test counts
refreshed against a full local run (584 across 74 files).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015f83ci81a7cKYf253ckwTb
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