Skip to content

fix(tests): pin session id in the three session-filtered job-listing tests - #45

Merged
axisrow merged 1 commit into
mainfrom
fix/test-env-leak-43
Aug 2, 2026
Merged

fix(tests): pin session id in the three session-filtered job-listing tests#45
axisrow merged 1 commit into
mainfrom
fix/test-env-leak-43

Conversation

@axisrow

@axisrow axisrow commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes the three test failures that #41's PR body described as "pre-existing status/result job-listing flakes reproducing identically on unmodified main". They are not flakes — they fail deterministically whenever the suite runs inside a Claude Code session, and pass everywhere else.

Tracked as Finding 3 in #43.

Root cause

status and result without an explicit job id filter the job list down to the current Claude session via filterJobsForCurrentSession (job-control.mjs:22-28). That filter is a no-op only when CODEX_COMPANION_SESSION_ID is unset:

function filterJobsForCurrentSession(jobs, options = {}) {
  const sessionId = getCurrentSessionId(options);
  if (!sessionId) return jobs;
  return jobs.filter((job) => job.sessionId === sessionId);
}

Three tests wrote fixture jobs carrying no sessionId and called run() with no env, so the child inherited whatever the host exported:

  • unset (CI, bare shell) → filter is a no-op → green
  • exported (inside a Claude Code session) → every fixture job is filtered away → red
✖ status shows phases, hints, and the latest finished job
✖ status preserves adversarial review kind labels
✖ result returns the stored output for the latest finished job by default
  AssertionError: No finished Codex jobs found for this repository yet.

Note the earlier hypothesis recorded in #43CLAUDE_PLUGIN_DATA leaking through run() — was wrong and is corrected there. The test still fails with that variable unset, and resolveStateDir returns an identical path in-process and in the child.

Fix

Follow the pattern already established by "status without a job id only shows jobs from the current Claude session" (runtime.test.mjs:1703), which does this correctly: pin sessionId: "sess-current" on the fixture jobs and pass a matching CODEX_COMPANION_SESSION_ID to the child.

Tests only — no production code is touched.

Rejected alternative

Scrubbing CODEX_COMPANION_SESSION_ID centrally in run() (tests/helpers.mjs) looks tidier and is wrong: it makes the filter a no-op, so listings then include unrelated jobs, breaking four tests that depend on the filter being activestatus --wait times out cleanly, both cancel tests, and the stop-hook review-gate test. Measured 3 failures before, 7 after. Recorded in #43 so it isn't retried.

Test plan

All runs with CODEX_COMPANION_SESSION_ID exported (i.e. the condition that used to be red):

Version

No bump — test-only change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FQBJirbWNKWhnpFUnWogB8

…tests

`status` and `result` without an explicit job id filter the job list down to
the current Claude session (filterJobsForCurrentSession, job-control.mjs:22).
That filter is a no-op only when CODEX_COMPANION_SESSION_ID is unset.

Three tests wrote fixture jobs with no `sessionId` and invoked run() with no
`env`, so the child inherited whatever the host exported. In CI and a bare
shell the variable is unset, the filter is a no-op, and they pass. Inside a
Claude Code session the host exports a real session id, every fixture job is
filtered away, and all three fail:

  ✖ status shows phases, hints, and the latest finished job
  ✖ status preserves adversarial review kind labels
  ✖ result returns the stored output for the latest finished job by default
    AssertionError: No finished Codex jobs found for this repository yet.

These were previously described as pre-existing flakes. They are not flaky —
they are deterministic given the ambient environment.

Fix follows the pattern already used by "status without a job id only shows
jobs from the current Claude session" (runtime.test.mjs:1703): pin
sessionId: "sess-current" on the fixture jobs and pass a matching
CODEX_COMPANION_SESSION_ID to the child. Production code is untouched.

Scrubbing the variable in helpers.mjs run() was tried first and rejected: it
makes the filter a no-op, so the listing then includes unrelated jobs and
breaks four other tests that rely on the filtering being active (status
--wait, both cancel tests, the stop-hook gate test).

Full suite is now 95/95 with the variable exported, where it was 92/95 before.

Refs #43

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FQBJirbWNKWhnpFUnWogB8
@axisrow
axisrow force-pushed the fix/test-env-leak-43 branch from ed63724 to ff072aa Compare August 2, 2026 08:48
@axisrow

axisrow commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

🔍 Local review (cycle 1)

Reviewed locally (/review + Codex companion gpt-5.6-sol / effort=xhigh), no bots pinged. Reviewed head ff072aa, base 3aad7f0.

Verdict Reviewer Finding Location
SKIP (follow-up) claude buildEnv does not pin CODEX_COMPANION_SESSION_ID — latent, no test fails today tests/fake-codex-fixture.mjs:901-914
codex No findings; verdict approve

Totals: 0 FIX, 0 UNVERIFIED, 1 SKIP (deferred to follow-up).

Verification performed during triage

The diagnosis was re-verified against production source rather than taken from the PR body:

  • filterJobsForCurrentSession (job-control.mjs:22-28) — confirmed no-op when CODEX_COMPANION_SESSION_ID is unset, strict job.sessionId === sessionId when set.
  • Three call sites: :261 (buildStatusSnapshot), :303 (resolveResultJob), :339 (cancel). The PR targets the first two.
  • Reference bypass (:303): reference ? listJobs(...) : filterJobsForCurrentSession(...) — an explicit job id skips the filter. This is why the neighbouring status task-live --wait test (runtime.test.mjs:1901) is a bare run() yet immune; probed green under a synthetic session id. Not an oversight.
  • Fixture realism: tracked-jobs.mjs:64-68 writes ...(sessionId ? { sessionId } : {}) — production omits the key when the env var is falsy, so both the pre-fix and post-fix fixture shapes are realistic job records. The fix does not invent an impossible state to make tests pass.
  • No assertion was weakened — every assert.match/assert.equal in the three tests is byte-identical; only fixture data and child env changed.

Generalization check (beyond the PR's own claim): the PR reports 96/96 with the author's session id exported. Re-ran the full suite with a synthetic value (CODEX_COMPANION_SESSION_ID=probe-host-session) → also 96/96. This proves the tests are pinned to their own value rather than accidentally agreeing with one particular host session.

Codex's next_steps asked for a run in a writable environment (its sandbox rejected tempdir creation with EPERM, so it validated syntax only). That was already satisfied locally: 96/96 twice, plus npm run build and npm run check-version green.

Deferred finding

buildEnv (fake-codex-fixture.mjs:901-914) pins PATH, CLAUDE_PLUGIN_DATA and the broker idle timeout, but spreads process.env without overriding CODEX_COMPANION_SESSION_ID. No test fails today — the buildEnv-based tests either pass an explicit job reference or assert on output that survives filtering (verified by full-suite probe). It is a latent gap of the same class, not a defect in this PR, and is left to the follow-up rather than widened into this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant