crg: add the §11.4 criterion-2 behavior-preservation gate over real review-task history - #555
Open
NikashPrakash wants to merge 19 commits into
Open
NikashPrakash wants to merge 19 commits into
NikashPrakash wants to merge 19 commits into
Conversation
…n 2) The hermetic crg-parity gate compares the kg-native CRG adapter against the crg-bridge mirror over a synthetic corpus with both sides driven in-process. It cannot show that the kg-native derivations reproduce what the legacy Python bridge actually persisted for a real repository, which is what §11.4 criterion 2 asks for. This package replays a corpus of REAL review tasks — pinned by commit SHA in testdata/crg-behavior/manifest.json — against BOTH sides: the bridge's own persisted flow_memberships / community assignment / risk_index / FTS tables plus its live get_impact_radius query, versus the kg-native adapter driven through crg.Bootstrap and the *FromStore readback surfaces (adapter/Store API directly, not the da kg commands). The oracles are the parity gate's own functions, reused unchanged: CompareFlowMemberships, CompareFTS, CompareImpactRadius, PartitionAgreement, SpearmanTau. Surfaces where the two sides genuinely answer the same question gate the run (changed_nodes, flows, fts); surfaces where the bridge computes something structurally different (impact_radius resolves call targets by bare name; communities are file-scoped; risk_score is a coverage heuristic; flow step numbering follows the bridge's path order) are reported with their structural diff but do not fail, until Config.Strict flips them at §11.4 sign-off. A missing Python CLI or unbuilt graph returns ErrBridgeUnavailable so callers SKIP rather than read an absent legacy side as a divergence.
Runs the §11.4 criterion-2 gate over the pinned review-task corpus and regenerates that corpus from real history. Regeneration is an explicit -regen command, never a side effect of a gate run, so a run replays exactly the pinned commits. Exit codes: 0 pass or skip, 1 gating divergence, 2 plumbing error. An unavailable legacy bridge prints a SKIP notice and exits 0. The allowlist entry covers main()'s os.Exit wrapper, which no Go test can enter — the same shape as tools/fsguard and tools/importguard; every other statement in the file is covered.
Installs the legacy code-review-graph CLI with the same venv step the KG CODE lane uses, builds a real graph (postprocess included — the gate reads the bridge's own derived views), and runs the pinned review-task corpus. continue-on-error until the §11.4 criterion-2 sign-off; the comment records what sign-off changes (drop continue-on-error, add -strict).
BEHAVIOR.md states what the gate covers, how to run it locally, the measured reason each advisory surface is not gating yet, and that criterion 4 (the managed-repo reads_from sweep) is a separate workstream. SOAK.md gains the pointer so the criterion-1 soak signal is not mistaken for criterion 2.
A code-review-graph CLI discovered on PATH whose sibling interpreter lacks the package (the `uv tool install` shape) failed the gate with a Python traceback instead of skipping. That is the same environment fact as "not installed", so the query error is classified as ErrBridgeUnavailable and the gate SKIPs. A genuine query failure still fails the run.
A code-review-graph build that persists no flow_memberships rows (release- and native-dependency dependent — the CI-installed release does exactly this, while a local 2.1.0 populates 4,762 flows) made the gate report an empty bridge view as 21 gating divergences. An uncomputed view is not a behavior divergence. Those surfaces now skip, and the report names them in the header and the summary so an unexercised surface is never read as an agreeing one.
Local code-review-graph 2.1.0 reproduces the bridge's flow_memberships exactly (4,762 of 4,762 flows); the current PyPI release diverges two-way on 18 of 21 tasks in CI, and an earlier run of the same job persisted no flow rows at all. That is the criterion-2 signal, not a gate defect — record it as an open finding for sign-off rather than downgrading the surface.
filepath.IsAbs is host-specific: on Windows a POSIX-rooted graph path, and on POSIX a drive-letter path, both slipped past the un-normalized-root guard — the Windows CI leg failed on exactly that. The guard now accepts either spelling, so a graph built on one platform and compared on another is still caught.
filepath.IsAbs treats "1:/x" as absolute on Windows, so asserting it through looksAbsolute failed the Windows leg. The drive-shaped cases are asserted against hasDriveLetter directly, which behaves the same on every host.
The gate previously built ONE graph at HEAD and replayed 21 historical
review tasks against it. That cannot detect a historical-output
regression: the symbols it resolves are today's symbols, and a path that
moved silently resolves to a different symbol, so agreement was an
artifact of the shared input rather than evidence about the release.
Each pinned task is now materialized in its own isolated linked worktree
at its OWN commit and compared under exact oracles. The worktree
lifecycle and all history reading are native (internal/gitwt and go-git
over a repoReader seam); the gate executes no git subprocess.
Three things stop a run from looking like evidence when it is not:
* a corpus contract names the surfaces a run MUST exercise, so a run
that skipped half of them no longer reports the same verdict as one
that compared everything, and a waiver has to be written down,
justified and owned;
* the upstream baseline is RECORDED from the pinned release rather
than hand-authored, so upstream drift reads as a plain text diff;
* an absent or off-release bridge is INCONCLUSIVE and exits non-zero.
It used to exit 0 with a SKIP notice, which made "we could not test
this" and "behavior is preserved" the same green result.
The corpus itself is derived from the release's own indexed languages
and selected language-coverage-first: the previous builder recognized
only the Go and Python declaration forms, so a TypeScript, Rust, Java or
Ruby commit was pinned with an empty identifier list and left the FTS
search surface permanently unexercised.
Every oracle is now exercised against a real pinned-release-shaped SQLite store (schema v9, including the FTS5 external-content table) or a real repository staged in-process with go-git, so the readers stay honest about the schema they decode and a search is a real FTS5 MATCH. Error branches are driven by real conditions rather than stubs of the code under test: dropped and shadow-dropped tables, an index drifted from its content, a NULL in a scanned column, a dangling tree or blob, a commit whose parent is missing, a blob past the scan bound, and a storage fault that serves an object header but fails its content stream. Repository fixtures stage commits, merges and renames through go-git rather than an exec'd git, so no test depends on an ambient git binary or on the developer's global git configuration.
The gate conforms the live bridge against a recorded baseline, so where that baseline comes from decides whether the surface certifies anything. A checked-in "expected upstream" file that no run of the release ever produced is a fabrication, and diffing a bridge against a fabrication proves nothing. The job is therefore record-then-compare: it installs the pinned 2.3.8 wheel, drives the release over the corpus to record the baseline into the job workspace, then runs the gate against those fresh recordings and uploads them with the verdict. Nothing is committed; *.json under the fixtures directory is gitignored so a local recording cannot drift into the repository either. The comparison pass runs under always() because a partial recording must still produce a verdict — a missing recording is a reported evidence gap, not a skipped step. CRG_GATE_TASKS caps both passes at one task, and that is the sharpest trade here. Measured on this runner class (run 33837748527) a full pinned-release build of this repository takes ~15 min — ~25 s of parsing and ~14.5 min of file-based community detection, igraph being unavailable — and every task also runs a standalone postprocess for the lifecycle probe, so one task costs ~29 min per pass and ~58 min across both. The 25 pinned tasks would need roughly a day. The cap softens no oracle: contract coverage is judged over the tasks that actually ran, so every required surface the capped prefix does not exercise still FAILS the gate, and the report states the reduction. Restoring corpus breadth needs a larger budget or a faster community-detection path, not a weaker contract.
Four files in the gate keep statements that need the pinned Python code-review-graph 2.3.8 release installed and a full per-commit graph build, which no Go test can arrange: the worktree materializer's post-build tail, the live bridge's build/postprocess/impact commands, the gate tool's success and verdict arms, and main()'s os.Exit wrapper. Each entry states which statements and why, and both ratchet pragmas are bumped in the same commit. The one non-release entry is views.go's sql.Open error return: the driver is registered by import and the DSN is built from a path already confirmed by os.Stat, so the modernc sqlite driver defers every failure to first use and nothing can make that branch fire. The stale "98.00%; every statement is covered except main()'s os.Exit wrapper" rationale on the gate tool is replaced — it no longer described the file.
goreleaser 2.18.1 errors on homebrew_casks.url.verified, so every branch carrying it fails "Validate GoReleaser config". The block held nothing else, so the anchor and its one reference both go.
The one-task cap was derived from the wrong release. The 15-minute build it was sized against came from a code-review-graph 2.2.0 log, where file-based community detection dominated every build; the PINNED 2.3.8 release costs ~29 s per task per pass (run 34797081532), so all 25 tasks take ~25 min across both record-then-compare passes and fit the 120-minute budget with room to spare. Capping the corpus at one task threw away the gate's breadth for nothing. The comment now says which release the figure came from and warns against re-deriving it from 2.2.0, since that mistake is what produced the cap. The knob stays at 0 (whole corpus) rather than being deleted: contract coverage is judged over the tasks that actually ran, so trimming can never buy a pass if the cost ever changes again. The job summary also reports the recorded baseline by counting the recordings the run actually produced instead of echoing the cap. Also fixes TestMtTDefaultWorkDirIsOutsideTheRepository, which compared filepath.Dir against a raw os.TempDir(): macOS keeps TMPDIR's trailing separator, so the two never matched on the macos runner.
… private Two real exposures in the gate, both reported as new vulnerabilities by the PR scan. countRows assembled `SELECT COUNT(*) FROM "<table>"` by concatenation behind an identifier allowlist. SQL cannot bind a table name, but the gate certifies ONE release with ONE known schema, so the set of countable tables is closed: each now has a literal statement, and the release fixture is validated at LOAD against that set. That also tightens the invariant — the old check accepted any well-formed identifier, so a fixture naming a table the release never writes passed validation and only failed later, mid-probe, as an unanswerable table. defaultWorkDir put the per-commit worktrees at a stable, guessable path under the shared temp root. The path has to stay stable because addWorktree clears whatever an interrupted run left at it, and a stable guessable path in a world-writable directory is one another user can pre-create or aim at a symlink. They now live in the user's own cache directory; with no cache directory the gate asks for -work-dir instead of falling back to the exposure it just removed. Also picks up nodes/edges into the hoisted table-name constants, so the fixture, the probe and the readers share one spelling for all ten tables, and clears four scanner code smells in the new tests (a predeclared-identifier shadow, a redundant condition variable, a single-method interface name, and one test whose per-subtest assertions are now one shared exactness helper).
The release spells a SYMBOL's qualified name "<path>::<symbol>" and a FILE's as just its path. Qualified only rewrote the path when it found the separator and returned everything else verbatim on the theory that a separator-less value is an unresolved bare target like `append` — so every file node kept the absolute path the build wrote it under. The first full-corpus CI run shows what that costs: comparison rows reading `/home/runner/.cache/crg-behavior-worktrees/crg-c4a2634dd566/obs/src/index.ts@obs/src/index.ts`. The two sides still agreed, because both are derived from the same view, but the id space is no longer the repository's: a recorded baseline is specific to the machine and the run that produced it, which is the one property a conformance recording must not have, and the report's divergence lists are unreadable. An absolute separator-less value is a file node and is now trimmed like any other path — including the out-of-root rejection. A RELATIVE one is still a bare target and still passes through untouched, since it names no file. The new test fails on the old behavior for the posix, windows, root-itself and out-of-root cases.
|
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
1 participant
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.
Builds the behavior-preservation gate required by
.agents/workflow/specs/graph-backend-adapter-contract/design.md§11.4criterion 2 — "the behavior-preservation gate passes on a corpus of recent
code-review tasks that consumed CRG output" — for the CRG decommission
(
t6-bridge-decommission). It was the one decommission criterion with noimplementation: grep found the hermetic
crg-parity-gate(criterion 1) and theconsumer audit (criterion 3), and nothing for criterion 2.
Criterion 4 (the managed-repo
reads_from: [crg-bridge]lockfile sweep) is aseparate workstream and is untouched here.
What is new
crg-parity-gate(existing)crg-behavior-gate(this PR)origin/mastercrg-bridgemirror, seeded in-processflow_memberships/nodes.community_id/risk_index/nodes_fts, plus a liveget_impact_radiuscall per taskcrg.Bootstrap+*FromStorereadbackda kgCorpus design
testdata/crg-behavior/manifest.jsonpins, per commit: SHA, subject, thegraph-indexable files it touched, and the declaration identifiers its diff added
or removed. From each entry the gate replays the queries a review of that commit
issues — changed-file impact radius, flows touched, community membership of the
changed symbols, FTS over the changed identifiers — against both sides.
Regeneration is an explicit command (
go run ./tools/crgbehaviorgate -regen),never a side effect of a run. Docs-only commits are dropped (25 commits → 21
review tasks).
Oracle reuse (nothing reinvented)
crg.CompareFlowMemberships,crg.CompareFTS,graphstore.CompareImpactRadius,graphstore.PartitionAgreement,graphstore.SpearmanTau/graphstore.DefaultSpearmanTau— the parity gate's own functions, applied toreal-history inputs. Bare id sets are lifted into
graphstore.ImpactRowso theseed-set comparison reuses the impact oracle too.
Two tiers, and why
Gating:
changed_nodes,flows,fts. Advisory (reported with the structuraldiff, does not fail):
impact_radius,flow_order,communities,risk_index. Each advisory tier entry is a measured derivation difference,documented in
testdata/crg-behavior/BEHAVIOR.md:of its 120,400 stored CALLS edges (88.8%) have a target matching no node — so
its blast radius carries name collisions an id-based traversal cannot
reproduce;
is connected components over CALLS+IMPORTS — no bridge community fits inside a
single native component;
risk_scoreis a coverage/caller heuristic with 9 distinct values across17,360 scored nodes; whole-graph Spearman against the kg-native degree
centrality is -0.21;
but step numbering follows each side's own traversal order.
-strict(andConfig.Strict) promotes every advisory surface to gating — thatflag is the §11.4 sign-off switch, so tightening the gate later needs no code
change.
Uncomputed legacy views skip, loudly (found by the first CI run)
Which materialized views a legacy graph holds depends on the
code-review-graphrelease and optional native deps. The first CI run exposedthis: the CI-installed release persisted zero
flow_membershipsrows forthis repo, while a locally installed 2.1.0 populates 4,762 flows — and the gate
reported the empty view as 21 gating divergences. An uncomputed view is not a
behavior divergence, so those surfaces now skip and the report says so in the
header and the summary:
That line has to be read before
GATE: PASS— a pass withflowsunexercisedis much weaker evidence than a pass with every surface live, and this
release-sensitivity is itself input to the decommission decision.
Local run evidence
Ran against a real
code-review-graphbuild of this worktree (18,513 symbols /228,319 references / 1,153 files), all 21 pinned tasks executed, live Python
bridge on both sides of every query:
Per-task detail (first task, trimmed):
Forced fail — 3 rows deleted from the bridge's persisted
flow_membershipsfor that file, same command, exit code 1:
The failure output names the commit, the query surface and the exact structural
diff, which is what a decommission decision is read off. The unit suite proves
the same non-tautology synthetically (
TestRunFailsAndLocatesAFlowDivergence,TestGateExitsNonZeroOnAGatingDivergence).Skip path verified by removing the venv: the gate reports
SKIP: legacy CRG bridge unavailable, dual-read not executedand exits 0 —including the case where a CLI is on PATH but its interpreter cannot import the
package (
uv tool installshape), which previously produced a traceback.The CI job is RED, on purpose — that is the criterion-2 signal
code-review-graphflowsverdictflow_membershipsrows at allThe CI divergence is two-way — 332 membership rows only in NATIVE, 214 only in
BRIDGE — so it is not just a partially populated table. Either the newer bridge
release derives flows differently or its flow view is unstable across builds.
That is exactly what criterion 2 is meant to expose, so it is recorded as an
open finding in
BEHAVIOR.mdrather than silenced by downgradingflowstoadvisory. The job is
continue-on-errorand non-required; resolving this is aprecondition of the criterion-2 sign-off, not of this PR.
CI wiring
New
crg-behavior-gatejob intest.yml,needs: lint-workflows, modelled oncrg-parity-gate: reuses the existing venv install step, builds a real legacygraph (postprocess included — the gate reads the bridge's own derived views),
then runs the pinned corpus.
continue-on-error: truewith a comment recordingwhat §11.4 sign-off changes (drop
continue-on-error, add-strict). The gatealso runs inside the Go suite as
TestBehaviorGate_RealHistoryCorpus, capped atthree tasks, skipping the same way.
Verification
go test ./... -skip TestConfigLoadSave— green. (One local-only exception:commands/kg's pre-existingcrg_backed_wrapperscase times out when a.venvsymlink makes a real CRG build reachable from the worktree; it passesin 10.7s once the symlink is removed. Unrelated to this change.)
internal/crgbehavior100.0% statement coverage;tools/crgbehaviorgate98.0% with one allowlist entry for
main()'sos.Exitwrapper (ratchet179 → 180), the same shape as
tools/fsguard/tools/importguard.gofmt,go vet,tools/importguard,tools/fsguard,actionlintclean.