test: [de-fork] trace-golden e2e harness (session trace as behavioral oracle)#1015
test: [de-fork] trace-golden e2e harness (session trace as behavioral oracle)#1015anandgupta42 wants to merge 4 commits into
Conversation
… oracle) A novel integration-test technique for de-fork stages S5 (native/plugin tool parity) and S7 (validator continuation): drive a real headless session with a scripted model, capture the trace it emits, normalize out nondeterminism, and partial-order-match it against a committed golden. Test infra only — no product-code change. Explicitly NOT a security oracle: tracing is best-effort/failure-suppressing and TraceSpan has no deny state, so an absent span is not proof of non-execution (S3's HardPolicy uses an independent audit probe instead). This harness corroborates observable behavior only. - `normalize.ts` — versioned allowlist-projection normalizer; every field classified stable-keep/volatile-drop/volatile-canonicalize; throws on an unknown span/trace field so schema drift surfaces. Structural, timing- independent sibling ranking (tool-call siblings under a generation collapse to one concurrent bucket; root-attached tools keep real ranks). callId uniqueness canonicalized; loop-summary hashes stripped structurally; realpath-variant path scrubbing (no macOS `/private<HOME>` leak). - `match.ts` — partial-order structural diff; concurrent siblings compared as an unordered multiset via full canonical signature. - `driver.ts` — drives a real `opencode run` subprocess against the fork's own `TestLLMServer`, recovering the trace path from the CLI's `trace_saved` event. - `trace-golden.test.ts` — 34 tests; each of the reviewed correctness fixes has a describe block that fails on revert. Missing golden fails unless `TRACE_GOLDEN_UPDATE=1` (rejected under CI). Scenario auto-discovery. - `stability-check.ts` — drives the smoke scenario through N independent subprocess runs; verified 100/100 → 1 unique hash. - `fixtures/real-traces/` — 8 benign smoke-session traces (PONG/BANANA/etc.; no PII) that pin the normalizer against real schema shapes. Test Plan: - `bun test test/altimate/trace-golden/`: 34 pass, 0 fail. - `stability-check.ts`: 100/100 runs, 1 unique hash, matches golden. - `bun run typecheck`: clean. NOTE: the CI wiring that runs the live subprocess test lands in a combined "wire de-fork CI checks" follow-up (that ci.yml region is co-edited by a sibling stage). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
📝 WalkthroughWalkthroughThe PR adds a trace-golden E2E harness that drives real headless CLI sessions, captures traces, normalizes nondeterministic data, partially matches span topology, compares committed goldens, and validates stability across repeated isolated runs. ChangesTrace-golden verification
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Scenario
participant CliFixture
participant LocalLLM
participant OpenCodeCLI
participant Trace
participant Normalizer
participant Matcher
Scenario->>CliFixture: load prompt, setup files, and scripted turns
CliFixture->>LocalLLM: serve deterministic responses
CliFixture->>OpenCodeCLI: run headless session
OpenCodeCLI->>Trace: save trace file
Trace-->>CliFixture: emit trace_saved path
CliFixture->>Normalizer: normalize captured trace
Normalizer->>Matcher: provide normalized trace
Matcher-->>Scenario: report golden diffs
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/opencode/test/altimate/trace-golden/driver.ts (2)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Effect.fn/Effect.fnUntracedand fail via the Effect channel.
driveScenariois a public, traceable effect andpushTurnan internal helper; per guidelines these should beEffect.fn("Driver.driveScenario")andEffect.fnUntraced. Two related idiom points:
pushTurn(Line 44)throw new Error(...)executes during generator evaluation and surfaces as a defect, not a typed failure — prefer failing through the Effect channel for consistency with Line 80.- Line 80 uses
yield* Effect.fail(new Error(...)); aSchema.TaggedErrorClassyielded directly (yield* new DriveError(...)) would align with the typed-error guideline.As per coding guidelines: "Use
Effect.fn(\"Domain.method\")for named/traced effects andEffect.fnUntracedfor internal helpers" and "preferyield* new MyError(...)overyield* Effect.fail(new MyError(...))".Also applies to: 63-66, 80-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/trace-golden/driver.ts` around lines 42 - 48, Convert the public driveScenario effect to Effect.fn("Driver.driveScenario") and the internal pushTurn helper to Effect.fnUntraced. Replace pushTurn’s thrown missing-toolName error with an Effect-channel failure, and update the related error path around line 80 to yield the DriveError instance directly rather than wrapping it with Effect.fail.Source: Coding guidelines
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the
FileSystem.FileSystemservice over rawnode:fs/promisesfor effectful I/O.Reading the trace here goes through
Effect.tryPromise(() => fs.readFile(...)); yieldingFileSystem.FileSystemkeeps the read inside the Effect environment (testable, mockable, uniform error channel) rather than dropping to a platform API.As per coding guidelines: "Prefer
FileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O".Also applies to: 90-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/trace-golden/driver.ts` around lines 11 - 16, Replace the raw node:fs/promises dependency and its Effect.tryPromise read in the trace-loading flow with the Effect FileSystem.FileSystem service. Update the relevant Effect environment requirements and use the service’s file-read operation so I/O remains injectable and errors stay in the Effect channel, including the additional occurrence around the trace read at the referenced later section.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_2e819da4cffeMhWRAOCOslv3mp.json`:
- Around line 32-40: Sanitize the recorded trace fixture by replacing every
occurrence of the developer-specific /Users/anandgupta/... path with the generic
/home/developer/... equivalent throughout the entire fixture, including all
referenced path locations. Do not alter other trace content.
In `@packages/opencode/test/altimate/trace-golden/stability-check.ts`:
- Around line 225-229: Validate CONCURRENCY in main alongside RUNS before
discovering scenarios or starting the pool, rejecting non-finite values and
values below 1 with a clear stability-check error that includes the invalid
value. Keep the existing RUNS validation and downstream runPool behavior
unchanged.
In `@packages/opencode/test/altimate/trace-golden/trace-golden.test.ts`:
- Around line 171-183: Replace the manual temporary-directory scaffolding with
the tmpdir fixture in all three sites:
packages/opencode/test/altimate/trace-golden/trace-golden.test.ts lines 171-183
and 193-202, and lines 903-948. In each test, use await using tmp = await
tmpdir() from fixture/fixture.ts, remove mkdtempSync/try-finally/rmSync cleanup,
and create the symlink target under the fixture path in the realpath test;
preserve each test’s existing assertions and behavior.
---
Nitpick comments:
In `@packages/opencode/test/altimate/trace-golden/driver.ts`:
- Around line 42-48: Convert the public driveScenario effect to
Effect.fn("Driver.driveScenario") and the internal pushTurn helper to
Effect.fnUntraced. Replace pushTurn’s thrown missing-toolName error with an
Effect-channel failure, and update the related error path around line 80 to
yield the DriveError instance directly rather than wrapping it with Effect.fail.
- Around line 11-16: Replace the raw node:fs/promises dependency and its
Effect.tryPromise read in the trace-loading flow with the Effect
FileSystem.FileSystem service. Update the relevant Effect environment
requirements and use the service’s file-read operation so I/O remains injectable
and errors stay in the Effect channel, including the additional occurrence
around the trace read at the referenced later section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f64e5f0-8452-4731-a59a-2041eea93f87
📒 Files selected for processing (19)
docs/internal/2026-07-18-trace-golden-e2e-technique.mdpackages/opencode/test/altimate/trace-golden/DRIVER-NOTES.mdpackages/opencode/test/altimate/trace-golden/driver.tspackages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_0c832d792ffeGW44jvnbD1ofca.jsonpackages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_0c9a811f7ffeVHtkexaP3SKoMg.jsonpackages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_0dfeccc49ffetVwiuQmrDKxNxh.jsonpackages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_0e3a95d6bffexTX5KtOaS4FOT9.jsonpackages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_0e3f286e5ffeGgjTZeBHqhT14w.jsonpackages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_0fb0d89c3ffemtOM9qTPAFNZyK.jsonpackages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_2e819da4cffeMhWRAOCOslv3mp.jsonpackages/opencode/test/altimate/trace-golden/fixtures/real-traces/ses_2e81c7baeffeaz1CvDUlNzeL4Y.jsonpackages/opencode/test/altimate/trace-golden/match.tspackages/opencode/test/altimate/trace-golden/normalize.tspackages/opencode/test/altimate/trace-golden/scenarios/smoke/golden.jsonpackages/opencode/test/altimate/trace-golden/scenarios/smoke/model-script.jsonpackages/opencode/test/altimate/trace-golden/scenarios/smoke/prompt.jsonpackages/opencode/test/altimate/trace-golden/scenarios/smoke/setup.jsonpackages/opencode/test/altimate/trace-golden/stability-check.tspackages/opencode/test/altimate/trace-golden/trace-golden.test.ts
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const ciYml = fsSync.readFileSync(ciYmlPath, "utf-8") | ||
| const subprocessPathsLine = ciYml.split("\n").find((line) => line.includes("SUBPROCESS_PATHS=")) | ||
| expect(subprocessPathsLine).toBeDefined() | ||
| expect(subprocessPathsLine).toContain("test/altimate/trace-golden/") |
There was a problem hiding this comment.
CRITICAL: This assertion will fail on merge of this PR.
.github/workflows/ci.yml:150 defines SUBPROCESS_PATHS without test/altimate/trace-golden/, and this PR does not modify ci.yml. The PR description explicitly defers that wiring to a sibling stage: "The CI wiring that runs the live subprocess test lands in a combined 'wire de-fork CI checks' follow-up (that ci.yml region is co-edited by a sibling stage)."
Unlike the live cliIt.live(...) scenarios below, this test is a plain test(...) — it is NOT gated by OPENCODE_SKIP_SUBPROCESS=1, so it executes in the MAIN CI test pass and fails there the moment this PR merges, before the follow-up lands. The local bun test run cited in the Test Plan (34 pass / 0 fail) must not have included this describe block, or it would have caught this.
Either land the ci.yml change in this PR (it is a one-line edit to the same SUBPROCESS_PATHS string), gate this test on the wiring actually being present, or move it into the follow-up PR that adds the wiring.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Pass (commit e40d0d1 → b89b4ad) — 4 files, no new issuesThe new commit
No new issues introduced by these changes. Files Reviewed (4 files)
Previous Review Summaries (3 snapshots, latest commit e40d0d1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e40d0d1)Status: No Issues Found | Recommendation: Merge Incremental Pass (commit 516eb68 → e40d0d1) — 4 files, prior CRITICAL resolvedThe new commit
No new issues introduced by these changes. Files Reviewed (4 files)
Previous review (commit 516eb68)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Incremental Pass (commit 337144a → 516eb68) — 3 files, no new issuesThis commit addressed two prior advisory findings from other reviewers; both fixes verified clean:
No new issues introduced by these changes. Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 337144a)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (5 source files; 11 JSON scenarios/fixtures + 2 docs skimmed)
Reviewed by glm-5.2 · Input: 67.6K · Output: 14.5K · Cached: 1.1M Review guidance: REVIEW.md from base branch |
![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: 337144a789
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ciYml = fsSync.readFileSync(ciYmlPath, "utf-8") | ||
| const subprocessPathsLine = ciYml.split("\n").find((line) => line.includes("SUBPROCESS_PATHS=")) | ||
| expect(subprocessPathsLine).toBeDefined() | ||
| expect(subprocessPathsLine).toContain("test/altimate/trace-golden/") |
There was a problem hiding this comment.
The current .github/workflows/ci.yml main pass sets OPENCODE_SKIP_SUBPROCESS=1, while its SUBPROCESS_PATHS assignment does not include test/altimate/trace-golden/. Consequently the live golden scenario is skipped, and this ordinary test(...) assertion itself fails in the main pass on every CI run. Include this directory in the workflow's subprocess pass as part of this commit rather than relying on an unspecified follow-up.
Useful? React with 👍 / 👎.
| 3. **Normalize** out nondeterminism (timestamps, spanIds, costs, abs paths, durations, host). | ||
| 4. **Diff the normalized span topology** against a committed golden. | ||
|
|
||
| A change in observable behavior = a diff in the golden. A tool moving from native→plugin (S5) that changes nothing observable produces an identical golden. A `DROP DATABASE` that should be blocked produces a `hard_policy_denied` span — the deny is **in the trace**, so the golden proves enforcement end-to-end, through whatever route the session took, not just at the unit that implements it. |
There was a problem hiding this comment.
For an S3/HardPolicy scenario, this states that a hard_policy_denied span exists and proves enforcement, but the correction at the top of this document and src/altimate/observability/tracing.ts establish that spans only have ok | error status and permission denials emit no such event. The later S3 stage-hook section repeats this impossible assertion, so following the specification could incorrectly treat the trace as a security oracle; rewrite these remaining claims to require the independent audit probe and dispatcher counters.
Useful? React with 👍 / 👎.
| /** One scripted assistant turn: either plain text (session ends) or a tool call (session continues). */ | ||
| export interface ScriptedTurn { | ||
| readonly kind: "text" | "tool" | ||
| readonly text?: string | ||
| readonly toolName?: string | ||
| readonly toolInput?: unknown |
There was a problem hiding this comment.
For any scenario intended to exercise concurrent sibling tool calls, this schema can represent only one tool call per assistant turn, and pushTurn invokes llm.tool(...), which queues a separate reply().tool(...) response for every array element. Thus two adjacent tool entries become two generations with singleton tool children rather than one generation dispatching multiple calls, so the live harness cannot exercise the concurrent topology that the partial-order matcher and planned S7 concurrency scenarios are meant to protect. Add a grouped/multi-tool turn representation and queue all calls in one Reply.
Useful? React with 👍 / 👎.
| function replaceAllPrefixed(text: string, root: string, placeholder: string): string { | ||
| if (!root) return text | ||
| const normalizedRoot = root.endsWith(path.sep) ? root.slice(0, -1) : root | ||
| if (!normalizedRoot) return text | ||
| return text.split(normalizedRoot).join(placeholder) |
There was a problem hiding this comment.
When a traced tool uses mkdtemp or otherwise emits a randomized directory directly below the OS temp root, this replacement only changes the prefix and leaves the random component intact; for example the committed fixture ses_0e3f286e5ffeGgjTZeBHqhT14w.json:72 becomes <TMP>/diffverify-CGXm/newfile.txt, so an equivalent later run with another suffix produces a different golden. The driver supplies only fixture.home and cannot enumerate temp directories created by tools, so scenarios involving such tools will flap unless generated temp-directory components are canonicalized as well.
Useful? React with 👍 / 👎.
| function toolContentKey(span: TraceSpan): string { | ||
| return `${span.kind} ${span.name} ${stableStringify(span.input ?? null)} ${stableStringify(span.output ?? null)}` |
There was a problem hiding this comment.
When concurrent same-rank tool calls have the same name/input and outputs containing run-random IDs or paths before a stable distinguishing suffix, this key sorts their raw, unsanitized values even though normalizeSpan scrubs those values later. A different random prefix can therefore reverse which logical call receives each DFS ordinal, making equivalent normalized traces and stability hashes differ. Build the ordering key from the same scrubbed representation used in the normalized spans rather than from the raw trace payload.
Useful? React with 👍 / 👎.
| const golden = yield* readJson<NormalizedTrace>(goldenPath) | ||
| const matchResult = match(golden, actual) | ||
| if (!matchResult.pass) { | ||
| return yield* Effect.fail(new Error(matchResult.format())) |
There was a problem hiding this comment.
For an S5 native-to-plugin migration, this generic scenario runner accepts a scenario solely when its normalized trace matches the golden. The design document explicitly notes that native and plugin routes produce identical traces and therefore requires an independent dispatcher sentinel, but neither the scenario schema nor this pass path checks one. A migration that accidentally continues invoking the old native implementation will consequently pass its advertised parity gate; require and validate a route-specific sentinel for S5 scenarios before accepting the trace match.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 19 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/test/altimate/trace-golden/stability-check.ts">
<violation number="1" location="packages/opencode/test/altimate/trace-golden/stability-check.ts:150">
P2: `checkScenario` at line 149 reads `goldenPath` unconditionally with no existence check. If `golden.json` is missing (e.g., for a newly discovered scenario directory), this produces a raw ENOENT or JSON parse error instead of a clear message. Consider a guard similar to `resolveGoldenAction` in the test file to produce a helpful error like: "golden.json not found — run with TRACE_GOLDEN_UPDATE=1 first".</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| const overallStart = Date.now() | ||
|
|
||
| const goldenPath = path.join(dir, "golden.json") | ||
| const golden = JSON.parse(await fs.readFile(goldenPath, "utf-8")) as NormalizedTrace |
There was a problem hiding this comment.
P2: checkScenario at line 149 reads goldenPath unconditionally with no existence check. If golden.json is missing (e.g., for a newly discovered scenario directory), this produces a raw ENOENT or JSON parse error instead of a clear message. Consider a guard similar to resolveGoldenAction in the test file to produce a helpful error like: "golden.json not found — run with TRACE_GOLDEN_UPDATE=1 first".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/altimate/trace-golden/stability-check.ts, line 150:
<comment>`checkScenario` at line 149 reads `goldenPath` unconditionally with no existence check. If `golden.json` is missing (e.g., for a newly discovered scenario directory), this produces a raw ENOENT or JSON parse error instead of a clear message. Consider a guard similar to `resolveGoldenAction` in the test file to produce a helpful error like: "golden.json not found — run with TRACE_GOLDEN_UPDATE=1 first".</comment>
<file context>
@@ -0,0 +1,265 @@
+ const overallStart = Date.now()
+
+ const goldenPath = path.join(dir, "golden.json")
+ const golden = JSON.parse(await fs.readFile(goldenPath, "utf-8")) as NormalizedTrace
+ const goldenHash = stableStringify(golden)
+
</file context>
…ONCURRENCY - Two `real-traces/` fixtures contained a developer's `/Users/anandgupta/...` home path (PII + CI-portability risk). Replaced with `/home/developer/...` throughout — the fixtures are used only for normalizer idempotency (normalize twice, compare), so the content swap doesn't affect any assertion (CodeRabbit). - `stability-check.ts` now validates `CONCURRENCY` the same way as `RUNS` (`Number.isFinite && >= 1`), so a bad env/arg fails with a clear message instead of `runPool` spawning zero workers and crashing downstream (CodeRabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 516eb684c5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function scrubText(text: string, entries: RootEntry[]): string { | ||
| let out = text | ||
| for (const { root, placeholder } of entries) out = replaceAllPrefixed(out, root, placeholder) | ||
| return scrubDynamicTokens(out) |
There was a problem hiding this comment.
Because every string in span inputs and outputs flows through this function, behavioral values that merely resemble trace bookkeeping are erased. For example, a finops tool changing its result from $10 to $20, or a configuration read changing 500ms to 1000ms, normalizes both versions to the same payload, so the golden can pass despite a user-visible regression. Restrict cost/duration scrubbing to generated summary fields such as the root output and narrative rather than arbitrary tool/model content.
Useful? React with 👍 / 👎.
|
|
||
| for (const [name, content] of Object.entries(setup.files ?? {})) { | ||
| yield* Effect.tryPromise({ | ||
| try: () => fs.writeFile(path.join(fixture.home, name), content, "utf-8"), |
There was a problem hiding this comment.
When a scenario needs a nested fixture such as models/orders.sql or .opencode/config.json, its parent directory does not yet exist and fs.writeFile fails with ENOENT before the CLI session runs. The stability runner repeats the same direct-write behavior, so such scenarios cannot be exercised by either advertised path; create each destination's parent directories recursively first.
Useful? React with 👍 / 👎.
| if (span.input !== undefined) out.input = scrubPaths(span.input, entries) | ||
| if (span.output !== undefined) out.output = scrubPaths(span.output, entries) |
There was a problem hiding this comment.
When one assistant generation dispatches multiple tools, Tracer.logToolCall appends both generationToolCalls and pendingToolResults as each tool completes, and logStepFinish serializes that completion order into the generation output and the next generation input. A scheduling flip therefore changes these fields even though the normalizer correctly treats the sibling tool spans as an unordered concurrent group, causing equivalent concurrent runs to mismatch or produce different stability hashes. Canonicalize these derived multi-tool fields from the child spans rather than preserving their completion-ordered strings.
Useful? React with 👍 / 👎.
| return fsSync | ||
| .readdirSync(dir, { withFileTypes: true }) | ||
| .filter((e) => e.isDirectory()) | ||
| .filter((e) => REQUIRED_FILES.every((f) => fsSync.existsSync(path.join(dir, e.name, f)))) |
There was a problem hiding this comment.
When a newly committed scenario omits any listed file, this filter silently removes it from both the live suite and the stability runner, so CI passes without exercising the scenario at all. This also affects scenarios created from the technique document's documented shape (prompt.json + model-script.json + golden.json), which does not include setup.json; treat absent setup as empty or register an explicit failure for incomplete directories instead of excluding them.
Useful? React with 👍 / 👎.
| function pushTurn(llm: CliFixture["llm"], turn: ScriptedTurn): Effect.Effect<void> { | ||
| if (turn.kind === "tool") { | ||
| if (!turn.toolName) throw new Error("driveScenario: tool turn missing toolName") | ||
| return llm.tool(turn.toolName, turn.toolInput ?? {}) |
There was a problem hiding this comment.
When a scenario contains tool calls in multiple assistant turns, each invocation of llm.tool(...) constructs a new Reply, whose private sequence restarts at zero, so every turn emits call_1. The normalizer deliberately preserves duplicate-ID equality as a continuation-correctness signal, meaning a multi-turn S7 golden will encode artificial duplicate call IDs and cannot detect whether the real flow maintains distinct IDs. Allow the script to supply IDs or allocate them monotonically across the whole driven session.
Useful? React with 👍 / 👎.
…dir determinism, tmpdir convention - CRITICAL (fails on merge): the plain `test(...)` asserting ci.yml's SUBPROCESS_PATHS includes `test/altimate/trace-golden/` runs in the MAIN CI pass but the branch tip didn't wire that path, so it would fail the moment this PR merged. Add `test/altimate/trace-golden/` to SUBPROCESS_PATHS in ci.yml — the assertion now passes AND the live golden scenario actually runs in the bounded subprocess pass where it belongs (it spawns a real CLI + mock LLM). Makes the PR self-contained instead of deferring to a sibling stage. - Temp-dir determinism (normalize.ts): a tool that creates its own scratch dir directly under the OS temp root (`mkdtemp` -> `$TMPDIR/diffverify-<rand>/`) left a per-run-random segment behind after `<TMP>` prefix scrubbing, so any future live scenario touching such a tool would flap on the random suffix. Add TMP_SUBDIR_PATTERN to canonicalize the first path segment under `<TMP>` to `<TMPDIR>` (any dir a tool makes there is inherently per-run) — keeps nested paths stable. +1 unit test (two different suffixes normalize equal). - tmpdir() convention (Major): the 3 manual `mkdtempSync` + `try/finally rmSync` scaffolds now use `await using tmp = await tmpdir()` (test/fixture/fixture.ts) with automatic per-test cleanup, per test/AGENTS.md. - Documented two known limitations tied to scenario types not shipped in this PR (per YAGNI — the guardrails land WITH the first such scenario, not before one exists): driver.ts's single-tool-per-turn schema can't yet generate S7 concurrent-sibling topology end-to-end; and the generic scenario runner needs a per-route dispatcher sentinel for S5 native->plugin parity (trace-match alone can't distinguish identical native/plugin routes). PII: fixture home paths already sanitized on the branch tip (verified no `/Users/...` remains). 34 pass / 1 skip / 0 fail, typecheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e40d0d15b3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // there is inherently per-run, so collapsing it keeps nested paths (`.../newfile.txt`) | ||
| // deterministic instead of flapping on the random suffix. Runs AFTER path scrubbing has | ||
| // produced the literal `<TMP>`, so it is a no-op on text that never carried a temp path. | ||
| const TMP_SUBDIR_PATTERN = /(<TMP>)\/[^\/"'\s]+/g |
There was a problem hiding this comment.
When a scenario intentionally uses a stable temp path, this pattern replaces its first component unconditionally rather than only canonicalizing generated directory names. For example, tool payloads containing /tmp/input-a and /tmp/input-b both normalize to <TMP>/<TMPDIR>, so a behavioral regression that changes which fixed file is accessed can still match the golden. Restrict this replacement to recognizable randomized mkdtemp names or explicitly registered generated directories.
Useful? React with 👍 / 👎.
| // there is inherently per-run, so collapsing it keeps nested paths (`.../newfile.txt`) | ||
| // deterministic instead of flapping on the random suffix. Runs AFTER path scrubbing has | ||
| // produced the literal `<TMP>`, so it is a no-op on text that never carried a temp path. | ||
| const TMP_SUBDIR_PATTERN = /(<TMP>)\/[^\/"'\s]+/g |
There was a problem hiding this comment.
On Windows, replacing os.tmpdir() leaves paths such as <TMP>\diffverify-CGXm\newfile.txt, but this regex recognizes only /. Any scenario whose tool creates a randomized directory under the Windows temp root therefore retains the random suffix and can produce a different stability hash on every run; accept both path separators when applying this canonicalization.
Useful? React with 👍 / 👎.
| const result = yield* fixture.opencode.run(options.prompt, { | ||
| ...options.runOpts, | ||
| format: "json", | ||
| }) |
There was a problem hiding this comment.
When the session exits before consuming every queued turn—precisely the failure mode an S7 continuation scenario is meant to catch—opencode.run still succeeds and driveScenario never checks fixture.llm.pending; conversely, an unexpected extra generation receives TestLLMServer's automatic "ok" fallback and is only recorded as a miss. During initial golden creation or an explicit update, either case can self-approve a trace that did not execute the declared script, so assert that no queued responses or unmatched requests remain before returning.
Useful? React with 👍 / 👎.
| const topTools = trace.summary.topTools | ||
| ?.map((t) => ({ name: t.name, count: t.count })) | ||
| // count desc (real signal) then name asc (deterministic tiebreak) — the raw topTools array ties | ||
| // on Map-insertion order, which is completion order and therefore not reproducible (tracing.ts:1332). | ||
| .sort((a, b) => b.count - a.count || a.name.localeCompare(b.name)) |
There was a problem hiding this comment.
When more than ten tool names tie at the cutoff, this only sorts the ten entries already selected by Trace.enrichSummary. That producer sorts by count alone and then calls .slice(0, 10), so ties retain the tool-span Map insertion order, which is concurrent completion order; equivalent runs with eleven one-shot concurrent tools can therefore omit different names and produce different normalized hashes. Apply a stable name tiebreak before the producer truncates, or avoid treating an ambiguously truncated list as deterministic.
Useful? React with 👍 / 👎.
| const { prompt } = yield* readJson<{ prompt: string }>(path.join(scenarioDir, "prompt.json")) | ||
| const setup = yield* readJson<{ files?: Record<string, string> }>(path.join(scenarioDir, "setup.json")) | ||
| const scriptRaw = yield* readJson<ScriptedTurn[]>(path.join(scenarioDir, "model-script.json")) | ||
| const script = resolvePlaceholders(scriptRaw, fixture.home) |
There was a problem hiding this comment.
Only model-script.json is passed through resolvePlaceholders, while the prompt and setup-file contents are used verbatim. A scenario that writes configuration such as a plugin path containing <HOME>/plugin.ts, or places <HOME> in its prompt, therefore sends the literal placeholder to the real CLI; the live test and stability runner both reproduce the same broken setup rather than exercising the intended fixture. Resolve the prompt and setup object with the fixture home as well.
Useful? React with 👍 / 👎.
…anon + full placeholder resolution - normalize.ts temp-dir canonicalization was too broad: it collapsed the ENTIRE first segment under `<TMP>` to `<TMPDIR>`, so distinct STABLE names (`<TMP>/input-a` vs `<TMP>/input-b`) mapped to the same placeholder — a regression that swapped which fixed file a tool touched would still match the golden (codex). Narrow TMP_SUBDIR_PATTERN to canonicalize ONLY a recognizable mkdtemp random tail (`<delimiter><>=6 base62>`), preserving the stable prefix, and accept `\` as well as `/` so it also fires on the Windows temp root (codex). +1 test that two stable names stay distinct; existing random-suffix test updated. - Fixture placeholder resolution: only `model-script.json` was passed through resolvePlaceholders, so a scenario with `<HOME>` in its prompt or a setup file sent the literal placeholder to the real CLI — and both the live test and stability-check.ts reproduced the same broken setup (codex). Resolve prompt + setup as well, in both runners. - Documented the script-consumption gap (codex): TestLLMServer exposes `pending`/`misses`, but a blanket exact-consumption assertion would flake on a real CLI's auxiliary (title/summary) generations, so that guard belongs per-scenario with the first S7 continuation scenario, not as a blanket gate. 35 pass / 1 skip / 0 fail, typecheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b89b4ad38b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // regression that swaps which fixed file a tool touches still diffs — while killing the flap on a | ||
| // genuinely random `mkdtemp` tail. Both `/` and `\` (Windows temp root) separators are accepted. | ||
| // Runs AFTER path scrubbing produced the literal `<TMP>`, so it is a no-op on non-temp text. | ||
| const TMP_SUBDIR_PATTERN = /(<TMP>[/\\][^/\\"'\s]*?[-_.])[A-Za-z0-9]{6,}(?=[/\\"'\s]|$)/g |
There was a problem hiding this comment.
When a scenario uses stable temp directories whose names contain a delimiter followed by six or more alphanumeric characters, this regex incorrectly treats the stable suffix as random; for example, <TMP>/input-orders/data and <TMP>/input-customers/data both become <TMP>/input-<RAND>/data, allowing a wrong-path regression to match the golden. Fresh evidence for the previously raised stable-name concern is the current {6,} matcher, while its negative test covers only the one-character suffixes input-a and input-b; restrict canonicalization to explicitly known/generated directories or another signal that distinguishes actual random tails.
Useful? React with 👍 / 👎.
| }, | ||
| "output": { | ||
| "parts": 4, | ||
| "content": "You are powered by the model named claude-sonnet-4-6. The exact model ID is anthropic/claude-sonnet-4-6\nHere is some useful information about the environment you are running in:\n<env>\n Working directory: /home/developer/codebase/altimate-code/.claude/worktrees/openui/packages/opencode\n Workspace root folder: /home/developer/codebase/altimate-code/.claude/worktrees/openui\n Is directory a git repo: yes\n Platform: darwin\n Today's date: Sun Mar 22 2026\n</env>\n<directories>\n \n</directories>\n\nSkills provide specialized instructions and workflows for specific tasks.\nUse the skill tool to load a skill when a task matches its description.\n<available_skills>\n <skill>\n <name>teach</name>\n <description>Teach your AI teammate a pattern by showing it an example file from your codebase</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/teach/SKILL.md</location>\n </skill>\n <skill>\n <name>training-status</name>\n <description>Show what your AI teammate has learned — patterns, rules, glossary, and standards</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/training-status/SKILL.md</location>\n </skill>\n <skill>\n <name>pii-audit</name>\n <description>Classify schema columns for PII (SSN, email, phone, name, address, credit card) and check whether queries expose them. Use for GDPR/CCPA/HIPAA compliance audits.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/pii-audit/SKILL.md</location>\n </skill>\n <skill>\n <name>cost-report</name>\n <description>Analyze Snowflake query costs and identify optimization opportunities</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/cost-report/SKILL.md</location>\n </skill>\n <skill>\n <name>data-viz</name>\n <description>Build modern, interactive data visualizations and dashboards using code-based component libraries (shadcn/ui, Recharts, Tremor, Nivo, D3, Victory, visx). Use this skill whenever the user asks to visualize data, build dashboards, create analytics views, chart metrics, tell a data story, build a reporting interface, create KPI cards, plot graphs, or explore a dataset — even if they mention PowerBI, Tableau, Streamlit, Metabase, Looker, Grafana, or similar tools. Also trigger when the user says \"make a dashboard\", \"show me the data\", \"chart this\", \"visualize trends\", \"build an analytics page\", \"data story\", or anything involving turning raw data into interactive visual interfaces. If the task involves presenting data visually — this is the skill. Always prefer building a real, interactive, code-based UI over exporting to or recommending a BI platform.\n</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/data-viz/SKILL.md</location>\n </skill>\n <skill>\n <name>sql-review</name>\n <description>Pre-merge SQL quality gate — lint 26 anti-patterns, grade readability/performance A-F, validate syntax, and scan for injection threats. Use before committing or reviewing SQL changes.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/sql-review/SKILL.md</location>\n </skill>\n <skill>\n <name>dbt-troubleshoot</name>\n <description>Debug dbt errors — compilation failures, runtime database errors, test failures, wrong data, and performance issues. Use when something is broken, producing wrong results, or failing to build. Powered by altimate-dbt.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/dbt-troubleshoot/SKILL.md</location>\n </skill>\n <skill>\n <name>altimate-setup</name>\n <description>Configure Altimate platform credentials for datamate and API access</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/altimate-setup/SKILL.md</location>\n </skill>\n <skill>\n <name>dbt-docs</name>\n <description>Document dbt models and columns in schema.yml with business context — model descriptions, column definitions, and doc blocks. Use when adding or improving documentation for discoverability. Powered by altimate-dbt.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/dbt-docs/SKILL.md</location>\n </skill>\n <skill>\n <name>dbt-analyze</name>\n <description>Analyze downstream impact of dbt model changes using column-level lineage and the dependency graph. Use when evaluating the blast radius of a change before shipping. Powered by altimate-dbt.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/dbt-analyze/SKILL.md</location>\n </skill>\n <skill>\n <name>lineage-diff</name>\n <description>Compare column-level lineage between two versions of a SQL query to show added, removed, and changed data flow edges.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/lineage-diff/SKILL.md</location>\n </skill>\n <skill>\n <name>dbt-develop</name>\n <description>Create and modify dbt models — staging, intermediate, marts, incremental, medallion architecture. Use when building new SQL models, extending existing ones, scaffolding YAML configs, or reorganizing project structure. Powered by altimate-dbt.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/dbt-develop/SKILL.md</location>\n </skill>\n <skill>\n <name>train</name>\n <description>Train your AI teammate on team standards from a document or style guide</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/train/SKILL.md</location>\n </skill>\n <skill>\n <name>sql-translate</name>\n <description>Translate SQL queries between database dialects (Snowflake, BigQuery, PostgreSQL, MySQL, etc.)</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/sql-translate/SKILL.md</location>\n </skill>\n <skill>\n <name>schema-migration</name>\n <description>Analyze DDL migrations for data loss risks — type narrowing, missing defaults, dropped constraints, breaking column changes. Use before applying schema changes to production.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/schema-migration/SKILL.md</location>\n </skill>\n <skill>\n <name>query-optimize</name>\n <description>Analyze and optimize SQL queries for better performance</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/query-optimize/SKILL.md</location>\n </skill>\n <skill>\n <name>dbt-test</name>\n <description>Add schema tests, unit tests, and data quality checks to dbt models. Use when validating data integrity, adding test definitions to schema.yml, writing unit tests, or practicing test-driven development in dbt. Powered by altimate-dbt.</description>\n <location>file:///home/developer/codebase/altimate-code/.claude/worktrees/openui/.opencode/skills/dbt-test/SKILL.md</location>\n </skill>\n</available_skills>\n\n## Altimate Knowledge\n\nKnowledge from previous sessions and team training. Apply it consistently.\n\n### Learned Rules\n_Always follow these rules. They were taught by the user through corrections and explicit instruction._\n\n#### no-positional-group-by (applied 24x)\n## Never use positional GROUP BY\n\n**Rule:** Always use explicit column names in GROUP BY. Never use positional references (`GROUP BY 1, 2, 3`).\n\n**Why:** Positional GROUP BY is fragile. Adding, removing, or reordering a SELECT column silently shifts which columns are grouped — the query still runs but produces wrong aggregations with no error.\n\n**Wrong:**\n```sql\nselect\n order_date,\n order_channel,\n sum(revenue) as revenue\nfrom orders\ngroup by 1, 2\n```\n\n**Right:**\n```sql\nselect\n order_date,\n order_channel,\n sum(revenue) as revenue\nfrom orders\ngroup by order_date, order_channel\n```\n\n**Edge case — computed expressions:** When grouping by an expression (e.g. `date_trunc`, `date_part`), repeat the full expression in GROUP BY rather than using a positional alias:\n\n```sql\n-- Wrong\nselect date_trunc('month', order_date) as month, sum(revenue)\nfrom orders\ngroup by 1\n\n-- Right\nselect date_trunc('month', order_date) as month, sum(revenue)\nfrom orders\ngroup by date_trunc('month', order_date)\n```\n\n**Applies to:** All SQL written in dbt models, ad-hoc queries, CTEs, subqueries, and window function PARTITION BY clauses.\n\n\nInstructions from: /home/developer/codebase/altimate-code/.claude/worktrees/openui/packages/opencode/AGENTS.md\n# opencode database guide\n\n## Database\n\n- **Schema**: Drizzle schema lives in `src/**/*.sql.ts`.\n- **Naming**: tables and columns use snake*case; join columns are `<entity>_id`; indexes are `<table>*<column>\\_idx`.\n- **Migrations**: generated by Drizzle Kit using `drizzle.config.ts` (schema: `./src/**/*.sql.ts`, output: `./migration`).\n- **Command**: `bun run db generate --name <slug>`.\n- **Output**: creates `migration/<timestamp>_<slug>/migration.sql` and `snapshot.json`.\n- **Tests**: migration tests should read the per-folder layout (no `_journal.json`).\n\n# opencode Effect guide\n\nInstructions to follow when writing Effect.\n\n## Schemas\n\n- Use `Schema.Class` for data types with multiple fields.\n- Use branded schemas (`Schema.brand`) for single-value types.\n\n## Services\n\n- Services use `ServiceMap.Service<ServiceName, ServiceName.Service>()(\"@console/<Name>\")`.\n- In `Layer.effect`, always return service implementations with `ServiceName.of({ ... })`, never a plain object.\n\n## Errors\n\n- Use `Schema.TaggedErrorClass` for typed errors.\n- For defect-like causes, use `Schema.Defect` instead of `unknown`.\n- In `Effect.gen`, prefer `yield* new MyError(...)` over `yield* Effect.fail(new MyError(...))` for direct early-failure branches.\n\n## Effects\n\n- Use `Effect.gen(function* () { ... })` for composition.\n- Use `Effect.fn(\"ServiceName.method\")` for named/traced effects and `Effect.fnUntraced` for internal helpers.\n- `Effect.fn` / `Effect.fnUntraced` accept pipeable operators as extra arguments, so avoid unnecessary `flow` or outer `.pipe()` wrappers.\n\n## Time\n\n- Prefer `DateTime.nowAsDate` over `new Date(yield* Clock.currentTimeMillis)` when you need a `Date`.\n\n## Errors\n\n- In `Effect.gen/fn`, prefer `yield* new MyError(...)` over `yield* Effect.fail(new MyError(...))` for direct early-failure branches.\n" |
There was a problem hiding this comment.
This committed trace contains the complete session system prompt, including a learned rule explicitly described as having been taught through user corrections, its application count, the installed internal skill catalog, and local worktree/configuration details; the same disclosure appears in ses_2e819da4cffeMhWRAOCOslv3mp.json. Shipping raw session-specific training data as a supposedly benign smoke fixture exposes information unrelated to the test to every repository reader, so replace these payloads with synthetic or comprehensively sanitized content.
Useful? React with 👍 / 👎.
| /** Recursively replaces absolute-path prefixes anywhere inside a JSON-cloneable value with placeholders. */ | ||
| function scrubPaths(value: unknown, entries: RootEntry[]): unknown { | ||
| if (typeof value === "string") return scrubText(value, entries) | ||
| if (Array.isArray(value)) return value.map((v) => scrubPaths(v, entries)) |
There was a problem hiding this comment.
When environment-fingerprint skill selection is enabled and the working directory differs from the worktree root, src/altimate/fingerprint/index.ts:42-47 runs both directory scans concurrently, lets them append into one shared tags array, and preserves whichever completion order wins in the traced output.tags. This generic array mapping retains that race order, so identical sessions whose two directories contribute different tags can produce different normalized hashes and golden diffs; canonicalize this known set-like fingerprint field before comparison.
Useful? React with 👍 / 👎.
Review comments addressed (2 rounds)Thanks to @chatgpt-codex-connector, @coderabbitai, and @kilo-code-bot. Round 1 —
Round 2 —
Deferred (documented at the site): blanket exact script-consumption assertion (would flake on a real CLI's auxiliary title/summary generations — belongs per-scenario with the first S7 continuation scenario) and the top-tool tie-break (producer-side, latent). 35 pass / 1 skip, typecheck clean. |
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes.
Deferring exact script-consumption checks until the first S7 continuation scenario—and leaving the producer-side top-tool tie-break as documented latent work—sounds appropriately scoped. Nice work validating this with 35 passing tests, one skip, and clean type checking. 🐇✨ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
PINEAPPLE
Summary
A novel integration-test technique for de-fork stages S5 (native/plugin tool parity) and S7 (validator continuation): drive a real headless session with a scripted model, capture the trace it emits, normalize out nondeterminism, and partial-order-match it against a committed golden. Test infra only — no product-code change.
Explicitly NOT a security oracle: tracing is best-effort/failure-suppressing and
TraceSpanhas no deny state, so an absent span is not proof of non-execution. (S3's HardPolicy uses an independent audit probe instead.) This harness corroborates observable behavior only.normalize.ts— versioned allowlist-projection normalizer; every field classified stable-keep / volatile-drop / volatile-canonicalize; throws on an unknown field so schema drift surfaces. Structural, timing-independent sibling ranking;callIduniqueness canonicalized; loop-summary hashes stripped structurally; realpath-variant path scrubbing (no macOS/private<HOME>leak).match.ts— partial-order structural diff; concurrent siblings compared as an unordered multiset via full canonical signature.driver.ts— drives a realopencode runsubprocess against the fork's ownTestLLMServer.trace-golden.test.ts— 34 tests; each reviewed correctness fix has a describe block that fails on revert. Missing golden fails unlessTRACE_GOLDEN_UPDATE=1(rejected under CI). Scenario auto-discovery.stability-check.ts— 100/100 independent subprocess runs → 1 unique hash.fixtures/real-traces/— 8 benign smoke-session traces (PONG/BANANA/6×7; no PII, only model-version strings) pinning the normalizer against real schema shapes.This went through three review rounds (a Codex technique review + a Codex code review + my verification) that took it from a plausible-but-flawed oracle → concurrency-safe → 7 correctness blockers fixed. All verified.
Test Plan
bun test test/altimate/trace-golden/: 34 pass / 0 fail (verified).stability-check.ts: 100/100 runs, 1 unique hash, matches golden.bun run typecheck: clean.The CI wiring that runs the live subprocess test lands in a combined "wire de-fork CI checks" follow-up (that ci.yml region is co-edited by a sibling stage).
Checklist
Closes #1012
Summary by cubic
Adds a trace-golden E2E harness for de-fork S5 (tool parity) and S7 (validator continuation). Runs a real headless
opencode runsession with a scripted model, normalizes the trace, and partial-order-matches it to a committed golden to catch behavioral drift. Wired into CI; test infra only, no product code changes. Not a security oracle.New Features
normalize.ts: versioned allowlist projection; drops/canonicalizes volatile fields; rejects unknown keys; stable sibling ranking; path scrubbing.match.ts: partial-order structural diff with unordered multiset compare for concurrent siblings.driver.ts+trace-golden.test.ts: launch realopencodeCLI againstTestLLMServer; auto-discover scenarios; update goldens viaTRACE_GOLDEN_UPDATE=1; 35 tests.stability-check.ts: 100/100 runs produce one normalized hash.Bug Fixes
/home/developer/...for portability.stability-check.ts: validateCONCURRENCYlikeRUNSand fail early on bad input.test/altimate/trace-golden/toSUBPROCESS_PATHSso the live golden test runs in the subprocess pass.normalize.ts: narrowed temp-path canonicalization to only mkdtemp-style random tails (preserves distinct stable prefixes; supports/and\) to avoid false matches.model-script.json) to prevent leaking literal placeholders into the CLI.Written for commit b89b4ad. Summary will update on new commits.
Summary by CodeRabbit