fix(test): isolate usage-log fixtures from the real OpenCodex home - #997
fix(test): isolate usage-log fixtures from the real OpenCodex home#997Yuxin-Qiao wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe test setup creates a temporary ChangesManagement API test isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Still wanted — the fixture isolation is correct and the setup/teardown restores the environment properly. It has drifted far behind |
4b6532b to
cdebe0e
Compare
Review readiness checklistThis PR is kept in draft until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there.
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
✅ PR quality gates passed This pull request now targets The title was left unchanged. The draft is owned by the checklist message below. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
✅ Deterministic PR hygiene checks passed. |
|
Rebased onto current |
|
This fixes a genuine hazard and the fix itself is right. Worth being explicit about why it matters, because the diff looks small: The implementation is complete for this file — per-test scratch home, prior value restored (including the One thing missing: nothing tests the promise. All nine assertions in this file are about metrics; none asserts that the real home was left untouched. So the safety property this PR exists to establish is the one property the suite would not notice losing. If someone later reorders the Please add one test that pins it — run with Worth a look while you are in there: if other test files call Small ask on an otherwise clean change; I would like it to land. |
Second loop of the bug campaign, scoped to author corrected replacements for
four contributor PRs and close theirs as absorbed. Three adversarial audit
rounds refuted the premise, and the plan now records that instead of the
outcome it was written to produce.
The decisive finding: I judged "has the author responded to our review" by
`updatedAt`, which moves when WE comment and therefore can never show author
activity. Comparing last-commit time against review time gives the real
picture:
#1092 commits 09:38:19Z review 09:09:51Z -> acted, in under 30 minutes
#1068 commits 08:52:23Z review 09:13:15Z -> predates the review
#1036 commits 08-05 review 09:12:51Z -> no response yet
#997 commits 02:51Z review 09:16:02Z -> no response yet
Under the wrong reading I was about to close #1092 as absorbed — taking
credit for work its author did in direct response to my own request — and to
credit #1068 with agreeing to a review it had not yet read. Neither is a
process nit; both would have been visible to the contributor.
So the plan changes shape. #1092 and #1068 become re-reviews. #1036 and #997
get a stated 72-hour response window with a mandatory head re-check before
anything is authored or closed; "has not replied within an hour" is not
abandonment, and #997's author was active at 02:51Z. This loop therefore
authors no absorbed layer at all, and says so.
#1068's re-review carries a finding neither side has: its new test asserts
all three DeepSeek ids are in `noVisionModels`, but routing merges the
registry list, which holds only the `-free` one. Reproduced with the test's
own routeModel config — Pro=false, Flash=false, Flash-free=true — so two of
three cases fail. Latent because no check currently runs that suite.
Also corrected: the layers were called dependency-ordered when they share no
files, so s3/s4 are independent heads off `origin/dev` and the
`--update-refs` cascade is reserved for genuine chains; and #978's exclusion
no longer claims it is "already correct" when it needs an author-side docs
change.
Terminal outcome is deferred, not DONE. Shrinking the criteria to match what
finished would have hidden exactly the thing worth recording.
Records what the stack-and-absorb campaign actually produced once live data replaced its assumptions. #1068 merged at 10:45:57Z while I was writing its review, and the merged code is not what I reviewed: `noVisionModels: [...OPENCODE_ZEN_TEXT_ONLY_MODELS, ...DEEPSEEK_THINKING_MODELS]` — the union the review asked for. Running the merged suite against `origin/dev` gives 9 pass / 0 fail, including a new test pinning the six probed text-only models. My 10:53Z comment claiming a failing test was true of the head I had fetched and false of what landed, so it got a public correction eight minutes later. That is the second process error in this unit, and both are now written down rather than quietly fixed. First: judging author activity by `updatedAt`, which moves when WE comment. Second: commenting against a stale fetched head. Each produced a wrong public statement to a contributor. The rules are re-fetch immediately before commenting, and compare last-commit time against review time. #1092's author restored the fail-closed guard within 30 minutes of the review, with a comment keeping `unknown` distinct in debug. Credited explicitly; asked only to split the unrelated `imageInput` scope. #1036 and #997 are deferred, not absorbed. Their last commits (08-05 07:18Z, 08-06 02:51Z) predate our reviews and neither carries the requested change, but roughly two hours have passed and #997's author was active that morning. 050 states a 72-hour response window with a mandatory head re-check before anything is authored or closed. The window exists because this unit demonstrated the alternative: two of four absorb targets were being actively fixed by their authors while we drafted replacements. #994 updated with the dev-only landing (merge 7d0c02d, ancestor of `origin/dev`) and deliberately left open pending the reporter's provider confirmation and a release. Zero contributor PRs closed by this loop.
|
Added the pin test in I also verified the pin actually bites: with the On the wider hazard: I scanned every test file that calls |
|
Heads-up on the
Fix options: sync the |
|
You beat me to it, and your version is better than the one I had staged. I had a commit ready to push onto this branch adding the assertion — then found Verified on your head rebased onto current And I ablated it to make sure it is not vacuous — commenting out Exactly one test goes red, and it is yours. The other nine keep passing, which is the whole point — they are metrics tests and cannot see this hazard. That is a genuine regression test. No changes requested from me. Your branch is now 2 commits, both yours, and it needs a rebase onto current Thanks for turning that around quickly, and sorry for the near-collision — I should have re-fetched your head before preparing a commit rather than working from the diff I had. |
Two loops circled a problem the campaign had invented. The user named the answer in one line: their PRs report maintainerCanModify=true, so push our completion commit onto their branch. Their commits stay, blame stays accurate, their PR merges, and attribution is the commit graph rather than a paragraph. No replacement PR, no close, no "absorbed with credit to" prose. #1036 landed that way. Four Agent59353 commits, then ours on top, pushed with a lease pinned to their head after re-verifying the remote. The change is bounded: live-transport records the bare names we actually advertised this request, the event state carries them, and both translate call sites convert only for names in that set. Their conversion logic is byte- unchanged. The stateless fallback passes through, since it has no state to consult and an unconverted call is recoverable while a wrongly converted one edits a file — disclosed in the PR comment as a judgment call open to disagreement. Ablation: restoring the name-only gate gives 21 pass / 1 fail, red on exactly the new collision test. #997 was not ours to push. The lease rejected it as stale: the author landed their own fix mid-flight, and theirs is stronger than what we staged — we pinned getConfigDir(), they assert the resolved usage.jsonl receives the row and the default location does not. Verified instead of overwritten, 10 pass with a 9/1 ablation on their own assertion. That is the third time in this campaign that acting on a stale head produced wasted or wrong work: updatedAt as an activity signal, a "failing test" comment posted eight minutes after #1068 merged with the fix, and a commit built against a head the author had already improved. The lease caught the third; the first two reached a contributor as a wrong public statement. The rule is now written down — re-fetch immediately before preparing a commit, not before pushing.
|
Thank you — this is landing as #1150, with both of your commits cherry-picked under your authorship. The detail that made this adoptable rather than merely acceptable: you assert the target path and the content explicitly, not just that the real home is left alone. A test that only stopped writing would pass silently if the isolation regressed later — yours would not. One conflict resolved during the pick, for the record: your two commits are stacked, so applying them required keeping the superset of imports from the later commit while applying the earlier one's scratch-home Your #999 landed in the same PR — see the comment there. Verified: Leaving this PR open for you. |
|
Closing — the fix landed as #1150 with both commits cherry-picked under my authorship, verified 10/10 on this file plus the full suite. Thanks for the adoption and the ablation check. |
2 participants
Summary
tests/management-api-logs-metrics.test.tsexercisesGET /api/logsdisplay metrics throughaddRequestLog, which persists every row tousage.jsonl. Unlike its sibling suites that also persist (usage-failure-persistence.test.ts,api-key-attribution.test.ts), it did not redirectOPENCODEX_HOMEitself and relied entirely on thebunfig.tomlpreload (tests/preload.ts).bunfig.tomlfrom the current directory. Runningbun test <absolute path>from outside the repo (agent/IDE pattern) skips it, and the fixture rows (ok-a/ok-b/fail,no-such-model,unpriced-model,combo/my-combo, ...) are appended to the developer's real~/.opencodex/usage.jsonl. The dashboard Usage page then renders never-used models with real token counts.OPENCODEX_HOME(mkdtemp, restored and cleaned inafterEach), matching the established pattern in the two sibling suites.Reproduction before the fix
Verification
OPENCODEX_HOME: 11 fixture rows were written before the fix.usage.jsonluntouched.bun run typecheckandbun run privacy:scanpass.management-provider-validation.test.tscases that fail only in this dev environment becausechatgpt.comresolves to a non-global address (write-time DNS/SSRF gate returns 400); upstream CI fordevis green.Summary by CodeRabbit
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I fixed all correct Codex and CodeRabbit findings.
My PR is ready for review.