feat(e2e): run deterministic metamorphic pairs - #100
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 108 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
871f0b2 to
1bdc20e
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 871f0b2213
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous Review Summaries (4 snapshots, latest commit eb4549b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit eb4549b)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous review (commit 1accf60)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous review (commit 9e73636)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous review (commit 1bdc20e)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Reviewed by gemini-3.7-flash · Input: 135.7K · Output: 11.8K · Cached: 800.8K |
- preserve canary evidence from partial valid output - validate seeded baselines, turn maps, and score invariants - prevent vacuous coverage and empty-report success
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e7363685d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
| }, | ||
| }); | ||
| const importances = shuffledImportances(compartments.length, seed); |
There was a problem hiding this comment.
Fresh evidence after the earlier compartment-count review is that this fix now passes the generated compartment count into shuffledImportances, which rejects counts above 100. A lint-green scenario may have 51–100 turns and gold.compartments.minCount between 101 and turns.length * 2, so baseline construction still throws supports at most 100 transcript turns rather than scoring the pair. Permit repeated importance values after exhausting the 1–100 range, or otherwise decouple this limit from the legal compartment count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in eb4549b.
Two bounds were conflated. baselineCompartments yields max(1, min(minCount, turns * 2)), and buildScriptedOutput raises minCount to at least turns, so the compartment count is bounded by transcript capacity — turns * 2. That is also the only bound lintScenario enforces (contract.ts:1313, exceeds-message-capacity). 100 is unrelated: it is the size of the importance value range. So your case is reachable exactly as described — 60 turns with minCount: 110 is contract-legal and threw instead of scoring.
Repeating values is safe, which I checked before choosing it over raising the cap: importance appears nowhere in invariants.ts or scorer.ts, so no invariant or oracle reads it. The shuffle only needs baseline text to vary across seeds, not to be unique. shuffledImportances now indexes the same shuffled permutation with index % MAX_DISTINCT_IMPORTANCE_VALUES, so values stay within 1-100 and counts past 100 wrap.
Two tests cover it: distinct values up to 100 then repeats (100 distinct across 150 compartments, all within 1-100), and a 60-turn scenario with minCount: 110 that lint does not fault on the compartment count and that now builds 110 compartments. The old test asserting the throw is gone with the throw.
For the record the shipped corpus is nowhere near this — max effective count is 4 across all 13 dev scenarios — so this was a latent trap for future scenarios rather than a live failure.
| role: "baseline", | ||
| transformId: null, | ||
| transformVersion: null, | ||
| seed: null, |
There was a problem hiding this comment.
When options.seeds contains multiple values and only one seed-dependent baseline promotes the canary, the .some(...) check collapses all baseline executions into this single hit with seed: null. The report therefore cannot identify or reproduce the offending seed, even though baselines are now independently built per seed; record a hit for each matching seeded baseline and retain its seed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in eb4549b.
The .some(...) reduced every seeded baseline to one boolean and then pushed a single hit with seed: null, so the report could name neither the offending seed nor how many seeds promoted the canary — while baselines is keyed by seed and holds exactly that information. Replaced with a loop over baselines entries that records one hit per matching seed and carries the seed. Iteration follows seeds insertion order, and buildMetamorphicReport sorts hits, so the report stays deterministic.
That leaves canaryHit reachable only from the derivative path, so its role === "baseline" branches were dead code returning the same seed: null this fix removes. I narrowed the helper to derivatives rather than leaving that shape sitting next to the fix.
Contract change worth flagging: a baseline hit now carries a real seed instead of null, so records a baseline canary without derivative coordinates was updated to expect DETERMINISTIC_SEEDS[0]. Added names the offending seed when one seeded baseline promotes the canary — three seeds, canary promoted only under seed 22 — which fails on the old code with seed: null.
- preserve canary evidence from partial valid output - validate seeded baselines, turn maps, and score invariants - prevent vacuous coverage and empty-report success
9e73636 to
1accf60
Compare
The scripted metamorphic baseline drew one importance per compartment from a shuffled 1-100 permutation and threw once the compartment count passed 100. Two bounds were conflated: the compartment count is bounded by transcript capacity (turns * 2, the only bound the scenario contract lints), while 100 is the size of the importance value range. A scenario with 60 turns and a compartment minimum of 110 satisfies the contract, so baseline construction threw instead of scoring the pair. Counts past the range now wrap onto the same shuffled order. Importance is read by no invariant and no scorer, so repeated values cost no discrimination; the shuffle only needs to keep baseline text varying across seeds. Baseline injection-canary detection also collapsed every seeded baseline into one hit with a null seed, so a report could not name or reproduce the seed that promoted the canary. Baselines are built per seed, so the hit is now recorded per matching seed and carries it. That leaves canaryHit reachable only for derivatives, so its dead baseline branches are gone. Verified: 61 metamorphic tests and the full 293-test historian-eval-unit lane pass, and the package typechecks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb4549b448
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const turnMapValid = derivative.turnMap.length === base.transcript.turns.length && | ||
| new Set(derivative.turnMap).size === derivative.turnMap.length && | ||
| derivative.turnMap.every( | ||
| (index) => Number.isInteger(index) && index >= 0 && index < derivative.scenario.transcript.turns.length, | ||
| ); |
There was a problem hiding this comment.
Fresh evidence after the earlier turn-map fix is that this validation checks only that the map is a permutation and that the derivative gold is self-consistent with it; it never verifies that derivative.scenario.transcript[turnMap[i]] actually derives from baseline turn i. A transform that reorders turns but returns a different valid permutation can therefore be admitted whenever the wrongly mapped ranges still contain the relevant predicates, and the report then scores a pair under incorrect provenance. Validate the map against the transcript transformation, or extend the transform result with enough edit metadata to distinguish intentional text rewrites from misplaced turns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 92ae4e8.
Worse than described, in fact. The gold check compares remapGold(base.gold, derivative.turnMap) against derivative.scenario.gold, and buildDerivative (transforms.ts:88) constructs that gold as remapGold(base.gold, turnMap) from the same two inputs — so for every transform built through it the comparison is a tautology, not merely a self-consistency check. It can only fail for a transform that hand-rolls its scenario, which is exactly what the existing broken-remap fixture does.
Fixed the way your second option suggests, since the first is not decidable: paraphrase-irrelevant and rename-unrelated-symbols legitimately rewrite message text, so derivative.turns[turnMap[i]] === base.turns[i] cannot be asserted for them. Transform now carries preservesTurnText, and admission verifies each mapped turn against its baseline turn for the transforms that declare it — reorder-independent-turns, move-accepted-decision, duplicate-rejected-proposal, all three of which copy turns by index without touching text. That is precisely the class where a "different valid permutation" can hide.
Stating the remaining limitation rather than implying it is closed: the two text-rewriting transforms declare false and their maps stay unbound to the transcript. Distinguishing an intentional rewrite from a misplaced turn there needs per-message edit metadata in TurnTransform, which is a larger contract change than this PR should carry. Adding the field to Transform made the compiler enumerate every implementation, including the three test fixtures, so a new transform cannot silently skip the decision.
New test rejects a text-preserving transform whose turn map misstates provenance swaps two turns while reporting the identity map. Verified discriminating: it fails with only the new branch disabled and passes with it restored.
| if (contentFingerprint === scenarioFingerprint(base)) { | ||
| diagnostics.push("derivative semantic fingerprint equals baseline"); |
There was a problem hiding this comment.
This check treats any semantic fingerprint difference as sufficient, so a transform that leaves the intended transcript relation intact but accidentally changes probes, trigger, or other authored scenario fields is admitted as long as the resulting scenario still lints. For example, changing a probe question makes the fingerprint differ and passes admission, while raw-output scoring always emits an empty probeVerdicts array, allowing the pair to remain green even though it no longer evaluates the same scenario. Require all authored fields outside the transcript and the explicitly remapped gold ranges to remain equal to the baseline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 92ae4e8, including the probe consequence.
The guard only fires when the fingerprint is equal, so it proves a transform did something, never that it did the right thing — and scenarioFingerprint (contract.ts:769) covers families, transcript, expectedHistorianRuns, gold, and probes, so a change to any one of them satisfies it. Your probe example checks out end to end: probes is inside the fingerprint, and scoreRawOutputWithInjectedClaims — the scorer the runner uses — hardcodes probeVerdicts: [] (scorer.ts:804, and errorScore at 846). So a mutated probe both satisfies admission and is invisible to scoring, leaving the pair green while evaluating a different scenario.
Implemented as you specified: authoredSemanticsOutsideTranscript canonicalizes the scenario with id, title, transcript, and gold removed, and admission requires equality with the baseline. id and title are excluded because buildDerivative deliberately derives them and the existing rejects a transform that changes only derived labels test depends on a title-only change still being caught by the fingerprint guard; transcript and gold are excluded because they are what a transform is licensed to change, with gold already pinned by the turn-map check above. Net effect is that any surviving fingerprint difference now has to come from the transcript or gold, which is what the guard was meant to mean.
All five shipped transforms pass unchanged, as expected — buildDerivative overrides only id, transcript, and gold, so probes, trigger, and families already passed through untouched. New test rejects a transform that mutates authored fields outside the transcript drifts one probe question and is verified discriminating against the check.
| "src/cache-analysis.test.ts", | ||
| "src/metamorphic-eval/injection-canary.test.ts", | ||
| "src/metamorphic-eval/invariants.test.ts", | ||
| "src/metamorphic-eval/metamorphic.test.ts", |
There was a problem hiding this comment.
Checked the historian contract job in .github/workflows/ci.yml: it invokes test:historian-eval-unit, whose selector globs only src/historian-eval/**/*.test.ts, while this addition places the new deterministic runner suite only in standaloneUnitFiles. Consequently it runs through the mode-based host suites instead of the unconditional historian-eval-contracts job; those host jobs have unrelated prerequisites (for example, e2e-host-opencode depends on e2e-opencode), so a prerequisite failure can skip all of these new deterministic checks. Add this suite to the dedicated historian-eval unit selection or invoke a metamorphic selection directly from that no-needs contract job.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 92ae4e8. Best catch of the round — the gate this PR exists to add was not running in the job that cannot be skipped.
Reproduced before changing anything. historianEvalUnitFiles() returned exactly six files, all under src/historian-eval/, and zero metamorphic ones. That also explains a false signal in my own verification earlier in this PR: bun run test:historian-eval-unit reported 293 passing tests and I read that as covering the runner, while the 63 metamorphic tests were only ever running because I invoked bun test src/metamorphic-eval/ separately.
Took your first option. The selection now globs src/metamorphic-eval/**/*.test.ts alongside the historian-eval glob, so historian-eval-contracts — the job whose own comment explains it declares no needs so that a gate downstream of check-plugin cannot be silently skipped — picks the suite up with no workflow edit. That job went from 6 files and 293 tests to 10 files and 356.
Kept the four entries in standaloneUnitFiles rather than moving them. assertSrcTestsClassified unions its sources into a Set, so being claimed twice is not an error, and no single selection pulls both lists, so nothing runs twice in one invocation. Removing them would have dropped the suite from the --mode ts, rust, and pi lanes, which is a coverage loss the finding does not ask for. Verified assertSrcTestsClassified, validate-mode-manifest, and the test:validate-manifest suite all still pass.
…uite Three findings from the codex round on eb4549b, all verified against the code before changing it. Pair admission never checked that the declared turn map described the transcript it shipped with. It verified the map was a permutation, then compared remapGold(base.gold, turnMap) against the derivative's gold — which buildDerivative computes from those same two inputs, so the comparison is a tautology for every transform built through it. A transform that reordered turns one way and reported a different valid permutation was admitted whenever the mismapped ranges still held the relevant predicates, and the pair then scored under wrong provenance. Transforms now declare preservesTurnText, and admission verifies derivative.turns[turnMap[i]] against baseline turn i for those that do. The declaration is required because a rewritten turn is indistinguishable from a misplaced one; paraphrase-irrelevant and rename-unrelated-symbols declare false and stay unbound. The fingerprint guard only rejected a derivative identical to its baseline, so it proved a transform did something, not that it did the right thing. Every field in scenarioFingerprint satisfied it, including probes — which raw-output scoring never evaluates, since scoreRawOutputWithInjectedClaims always emits an empty probeVerdicts array. A transform that only drifted a probe question was admitted and stayed green. Authored fields outside the transcript, gold, and derived labels must now equal the baseline. The metamorphic suite also sat only in standaloneUnitFiles, so it was selected by mode-based host jobs that declare needs (e2e-host-opencode needs e2e-opencode) and never by historian-eval-contracts, the no-needs job that exists precisely so a deterministic gate cannot be skipped by an unrelated prerequisite failure. The historian-eval unit selection now globs src/metamorphic-eval too, taking that job from 6 files and 293 tests to 10 files and 356. Verified: each new test fails with only its own check disabled and passes with it restored; the 356-test unconditional lane, the manifest validation suite, assertSrcTestsClassified, and typecheck all pass.
GitHub retargeted this PR to main when #99 merged, so it conflicted against main rather than the stacked base. Resolved by keeping #99's reviewed foundation and layering #100's additions where independent. Took main's test selection: it already globs src/metamorphic-eval into the historian-eval unit selection, the same fix made here, and additionally drops the metamorphic entries from standaloneUnitFiles. This file now has no diff against main. Combined `alwaysApplicable: false` with `preservesTurnText` in transforms.ts; they constrain different things. Re-verified the three `preservesTurnText: true` declarations against main's rewritten transforms: `reorderedTurns` and `duplicatedTurns` still copy turns verbatim, and duplicate's turn map shifts mapped positions past the insertion. Took main's invariants.ts, including `normalizeContent` over `normalizeMemoryContent`, and re-added `compareScoreInvariants` for runner.ts. Kept both `verdict-monotonicity` and `scenario-verdict-equality`: equality also catches FAIL to PASS, while monotonicity carries `introducedFailReasons`. The score-level invariant assertion now expects all six. Verified: typecheck, 131 metamorphic tests, the 431-test historian-eval unit lane, the manifest suite, `assertSrcTestsClassified`, and `validate-mode-manifest`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4512726ce1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| })), | ||
| facts: expectedClaims.map((claim) => ({ | ||
| category: claim.category, | ||
| content: normalizeMemoryContent(claim.predicate.value), |
There was a problem hiding this comment.
When a lint-clean expected predicate literally contains <, >, ", or ', normalizeMemoryContent leaves that spelling unchanged while buildMockHistorianOutput rejects it as non-round-trippable. The scenario contract accepts these predicate strings when matching authored transcript text, so baseline construction throws and records baseline scoring failed instead of evaluating the pair. Fresh evidence beyond the earlier whitespace-normalization fix is that XML-entity spellings still require a round-trip-safe equivalent.
Useful? React with 👍 / 👎.
| const turnMapValid = derivative.turnMap.length === base.transcript.turns.length && | ||
| new Set(derivative.turnMap).size === derivative.turnMap.length && | ||
| derivative.turnMap.every( | ||
| (index) => Number.isInteger(index) && index >= 0 && index < derivative.scenario.transcript.turns.length, | ||
| ); |
There was a problem hiding this comment.
Fresh evidence after the earlier turn-map fix is that this map is only baseline-to-derivative: when a text-preserving transform adds a turn, as duplicate-rejected-proposal legitimately does, every mapped baseline turn can match while the unmapped derivative turn is never examined. A faulty transform can therefore append arbitrary lint-clean evidence with an identity map; the remapped-gold and fingerprint checks pass, and the scripted scorer can report a green pair because it derives facts from gold rather than the appended transcript. Require insertion provenance or verify that every unmapped derivative turn is the declared duplicate.
Useful? React with 👍 / 👎.
The base moved to main after PR #100 merged. Only metamorphic.test.ts conflicted, in the import block and the helper block, and both were resolved as unions. Main split this branch's "preserves distinct importances and rejects transcripts beyond their range" into two tests and added a turn-map provenance check gated on a new required Transform field. Main's split supersedes the single test, so this merge keeps main's pair rather than reintroducing the original, and the three local Transform fixtures now declare preservesTurnText: false for the inapplicable fixtures and true for the identity fixture, which does preserve turn text and therefore still lints red on its fingerprint alone.
1 participant
Summary
Metamorphic pairs can now run without model credentials and produce a deterministic, machine-readable report. This is the second layer of the four-PR evaluation stack.
Design
Stack
Validation
Stack created with GitHub Stacks CLI • Give Feedback 💬