feat(e2e): historian eval scenario corpus, mutation battery, and freeze governance (U5-U6) - #72
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![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: 8b24853224
ℹ️ 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".
| for (const [index, raw] of input.scenarios.entries()) { | ||
| for (const violation of scanForSensitiveContent(raw, { forbiddenTokens: input.forbiddenTokens })) { |
There was a problem hiding this comment.
The privacy gate scans only input.scenarios, but the operator-supplied approvals are later persisted verbatim in manifest.json. If an approver contains a forbidden token, secret, session ID, or local path, parseApproval accepts the nonempty string and the sensitive value is published despite the privacy approval; include approvals in this pre-parser scan, as the retrieval-benchmark promoter does.
Useful? React with 👍 / 👎.
| if (typeof entry.green !== "boolean" || !Array.isArray(entry.results) || entry.results.length === 0) { | ||
| evidenceFail(`scenarios[${index}]: fields-invalid`); |
There was a problem hiding this comment.
This parser only requires a nonempty result array and internally consistent booleans, so a tampered artifact can replace all results with one known class marked green, set the aggregate flags green, and still pass loadRelease because checkMutationEvidence checks only the matching fingerprint and entry-level flag. Consequently the strict consumer path can accept evidence that no longer demonstrates most of the required mutation battery; validate unique required class coverage and its applicability constraints, or recompute and compare the evidence on load.
Useful? React with 👍 / 👎.
| const destination = join(input.releasesRoot, input.releaseVersion); | ||
| if (existsSync(destination)) { |
There was a problem hiding this comment.
Promotion only rejects the exact destination version, so releases can be installed out of numeric order. For example, promoting v2 with scenario X and then promoting v1 with X tombstoned succeeds; the numerically later immutable v2 now resurrects X and cannot be corrected in place, violating the stated vN+1 tombstone rule. Require a new version to be later than every installed release before publication.
Useful? React with 👍 / 👎.
| const manifestPath = join(releasesRoot, entry, RELEASE_FILES.manifest); | ||
| if (!existsSync(manifestPath)) continue; |
There was a problem hiding this comment.
If an installed vN directory has lost or never received its manifest, this branch silently ignores it and promotion proceeds without inheriting any of its tombstones. A subsequent release can therefore reintroduce a retired scenario precisely when prior release state is incomplete; a version-shaped directory without a manifest should block promotion rather than be treated as carrying no tombstones.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function runProbeWrongAnswer(scenario: HistorianEvalScenario): MutationResult { | ||
| const probe = scenario.probes[0]; |
There was a problem hiding this comment.
The mutation battery tests only the first probe of each scenario. In the added corpus every claim-id probe appears after another probe, so the entire corpus can produce green evidence without ever exercising the claim-id path in compareProbeAnswer; a regression that accepts an incorrect claim identifier would therefore pass the admission gate. Run the wrong-answer mutation for each probe or at least require coverage of every answer type.
Useful? React with 👍 / 👎.
| const scenarioFiles = readdirSync(join(releaseDir, RELEASE_FILES.scenariosDir)).sort(); | ||
| const scenarios = scenarioFiles.map((file) => | ||
| parseScenario(JSON.parse(readFileSync(join(releaseDir, RELEASE_FILES.scenariosDir, file), "utf8")), file), | ||
| ); |
There was a problem hiding this comment.
promoteRelease enforces the 10–30 scenario budget, but the exported strict consumer path does not repeat that invariant after reading the scenario directory. A separately assembled or corrupted release containing fewer than 10 or more than 30 scenarios can therefore pass parsing, linting, tuple verification, approvals, and evidence checks even though the same corpus is rejected by promotion; enforce the release-level budget in loadRelease as well.
Useful? React with 👍 / 👎.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (17 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 8b24853)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8b24853)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (17 files)
Reviewed by gemini-3.7-flash · Input: 160.6K · Output: 17.8K · Cached: 861.8K |
…ze governance (U5-U6) Dev corpus: 13 hand-audited scenarios authored latent-truth-first, each lint-clean, covering all seven hard-negative families (proposed-but- rejected, explored-never-accepted, assistant speculation, user correction, current-vs-historical, in-conversation prompt injection, conflicting evidence) with expected-absent predicates phrased to match only the forbidden active formation, never the legitimate rejection record. Mutation battery: per scenario, crafted wrong outputs per class fed through the scorer's raw-output seam must go red AT the expected stage, consulting one policy table; stage migration, a passing mutation, a non-discriminating near-miss matcher, a probe-less scenario, and a family set no false-authoritative class covers all fail the battery. Freeze governance clones the retrieval-benchmark promotion pattern: privacy scan before any parser, operator approvals bound to the exact release-tuple fingerprint (one per kind: privacy, gold-intent), the battery recomputed inside promoteRelease so evidence cannot be forged or stale, review-dir plus staging round-trips through the strict consumer path, atomic rename into an immutable release, and tombstone-in-vN+1 errata that later releases must carry forward.
8b24853 to
1865bcf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1865bcf46e
ℹ️ 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".
| if (unexpected.length > 0 || entries.length !== expected.length) { | ||
| fail([`release: unexpected entries (${unexpected.length > 0 ? unexpected.join(", ") : "missing files"})`]); | ||
| } | ||
| const manifest = parseManifest(JSON.parse(readFileSync(join(releaseDir, RELEASE_FILES.manifest), "utf8"))); |
There was a problem hiding this comment.
When loading a release from outside the promoter's in-memory flow, parseManifest verifies only self-consistency: an editor can change either approver string—or fabricate both approvals—and retain the tuple fingerprint, after which loadRelease accepts the release as approved. Because nothing trusted outside the directory authenticates the manifest, the operator-approval gate can be bypassed; accept an expected manifest fingerprint or another external trust anchor on the strict consumer path.
Useful? React with 👍 / 👎.
| "family": "explored-never-accepted", | ||
| "predicate": { | ||
| "kind": "normalized-substring", | ||
| "value": "use cursor-based pagination" |
There was a problem hiding this comment.
For this scenario, predicateMatches performs normalized literal substring matching, but the transcript never contains “use cursor-based pagination”; a bad historian output such as “Cursor-based pagination handles concurrent inserts best” therefore promotes the explicitly undecided option without matching this expectedAbsent predicate, allowing the false-authoritative check to pass. Use a stable core span such as “cursor-based pagination”; the sibling keyset predicate has the same problem.
Useful? React with 👍 / 👎.
| compartments: [ | ||
| { | ||
| start: 1, | ||
| end: messageCount, | ||
| title: `Baseline for ${scenario.id}`, |
There was a problem hiding this comment.
For any otherwise valid scenario with gold.compartments.minCount > 1, this baseline always emits exactly one compartment. assertBaselineValidates consequently sees a structural FAIL before running any mutations, so promoteRelease rejects every such scenario even though the contract permits higher minimums. Construct the baseline with at least the declared number of valid contiguous compartments.
Useful? React with 👍 / 👎.
| const expected: string[] = [RELEASE_FILES.evidence, RELEASE_FILES.manifest, RELEASE_FILES.scenariosDir]; | ||
| const unexpected = entries.filter((entry) => !expected.includes(entry)); | ||
| if (unexpected.length > 0 || entries.length !== expected.length) { | ||
| fail([`release: unexpected entries (${unexpected.length > 0 ? unexpected.join(", ") : "missing files"})`]); |
There was a problem hiding this comment.
When the strict loader is given a malformed or externally assembled release whose unexpected filename contains a forbidden token, customer identifier, or local path, this diagnostic writes that filename directly to logs before any privacy scan. Promotion scans scenario values but not filesystem entry names, so report only the count of unexpected entries, as otherwise unreviewed names cross the privacy boundary.
Useful? React with 👍 / 👎.
| const entries = readdirSync(releaseDir).sort(); | ||
| const expected: string[] = [RELEASE_FILES.evidence, RELEASE_FILES.manifest, RELEASE_FILES.scenariosDir]; | ||
| const unexpected = entries.filter((entry) => !expected.includes(entry)); |
There was a problem hiding this comment.
When loading an externally assembled or tampered release, these name-only entry checks allow manifest.json, mutation-evidence.json, the scenarios directory, and individual scenario files to be symlinks because the subsequent filesystem calls follow them. Self-consistent target bytes therefore pass loadRelease, after which changing files outside the installed vN directory changes what later runs load without modifying that supposedly immutable tree; use lstat to require real regular artifact files, a real scenarios directory, and regular scenario children.
Useful? React with 👍 / 👎.
| "bazel", | ||
| "turborepo" | ||
| ], | ||
| "goldAnswer": "turborepo" |
There was a problem hiding this comment.
When the Turborepo claim is promoted but omitted from this probe turn by the injection budget, the absent sourceClaimRef leaves compareProbeAnswer with no backing gold claim, so a wrong or empty response is charged as a historian probe FAIL instead of error-trimmed. Add sourceClaimRef: "exp-turborepo" so infrastructure trimming is not misreported as model quality; several other newly added multiple-choice probes omit their evident backing claim in the same way.
Useful? React with 👍 / 👎.
1 participant
Stack 4/5 for the historian structural eval lane (beads
magic-context-x4l.11). Base: #71 (engine).Dev corpus: 13 hand-audited scenarios authored latent-truth-first (gold expectations first, transcript rendered from them), lint-clean, covering all seven hard-negative families. Expected-absent predicates are phrased to match only the forbidden active formation, never the legitimate rejection record ("X was rejected because Y" is the preferred durable formation per R3). The 3-run live stability audit per scenario (U5 step 3) is an operator gate documented in the README (5/5).
Mutation battery (
mutations.ts, R13/KTD5): per scenario, crafted wrong outputs per class must go red AT the expected stage, consulting a single policy table (EXPECTED_OUTCOMES). Red on: stage migration in either direction, a passing mutation, a near-miss perturbation the matcher still accepts (matcher-discrimination proof), a probe-less scenario, or a family set no false-authoritative class covers.Freeze governance (
promote.ts, KD4/KTD7): clones the retrieval-benchmark promotion pattern — privacy scan before any parser, operator approvals bound to the exact release-tuple fingerprint (one per kind: privacy, gold-intent, never minted here), the battery recomputed insidepromoteReleaseso evidence cannot be forged or stale, review-dir + staging round-trips through the strict consumer path, atomic rename into an immutable release, tombstone-in-vN+1 errata carried forward by every later release, and a strict fail-closed evidence parser for read-back.Verified:
bun run test:historian-eval-unit(64 tests) green;run-historian-eval.ts --lintand--mutations(arrive in 5/5) green over the 13-scenario corpus.Stack: 1/5 plumbing → 2/5 contract → 3/5 engine → 4/5 (this) → 5/5 CI