Repository navigation
test: isolate diagnostic logger spy history - #2362
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title "test: isolate diagnostic logger spy history" matches the changeset. Both victim test files now clear the shared debug-logger spy's call history in beforeEach, which isolates the diagnostic … |
| Description check | |
The description follows the repository template. It has Summary (what changed and why), Impact (user-facing and developer/maintainer impact), Testing (preflight box checked, exact commands and results… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Risk Surface Disclosed | |
PASS — The PR changes only test spy setup in src/ink/output.test.ts and src/utils/context.test.ts. It does not change auth, provider routing, permissions, outbound network behavior, background exe… |
| No Hidden Policy Change | |
The PR changes only src/ink/output.test.ts and src/utils/context.test.ts. The two added lines call mockClear() on diagnostic logger spies before assertions. No product code, trust model, routing… |
Full details: Title check
-
Explanation
The title "test: isolate diagnostic logger spy history" matches the changeset. Both victim test files now clear the shared debug-logger spy's call history in beforeEach, which isolates the diagnostic logger spy. It is concise, scoped to the test change, and uses the conventional test: prefix. It meets the title requirements of being concise, scoped, and matching the diff.
Full details: Description check
-
Explanation
The description follows the repository template. It has Summary (what changed and why), Impact (user-facing and developer/maintainer impact), Testing (preflight box checked, exact commands and results, focused tests, skipped checks and pre-existing failures), and Notes (provider/model path, screenshots, follow-up work). The change is test-only, so the provider/model and screenshot fields are correctly marked n/a. The description is thorough and specific about the root cause, the fix, and the validation performed.
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.
Full details: Risk Surface Disclosed
-
Explanation
PASS — The PR changes only test spy setup in
src/ink/output.test.tsandsrc/utils/context.test.ts. It does not change auth, provider routing, permissions, outbound network behavior, background execution, startup/config-home behavior, skills/plugins/MCP, CI permissions, or release scripts. The risk-surface disclosure requirement is therefore not applicable.
Full details: No Hidden Policy Change
-
Explanation
The PR changes only
src/ink/output.test.tsandsrc/utils/context.test.ts. The two added lines callmockClear()on diagnostic logger spies before assertions. No product code, trust model, routing defaults, telemetry/network behavior, or permission policy changes are present in the reviewed diff.
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
— jatmn's PR Review agent
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
Merge readiness
GitHub reports mergeable_state: blocked while mergeable: true; required checks on head ac438a31303c3e9cdc929e157fdba0968090bb35 are green (eight unit-tests shards, smoke-and-tests, typecheck, dedicated-tests, launcher jobs, pr-intent-scan, changes, and CodeQL including javascript-typescript). No merge conflict or rebase gap (behind_default_count: 0). This is one of four standalone serial-flake fixes; a full green serial seed on main still needs the sibling branches noted in the PR body.
Local validation (read-only): Bun 1.4.4 installed for CI parity (default worker Bun was 1.3.14). Serial UNATTENDED_RETRY on merge-base e365805: seed 4242 → 10431 pass / 16 fail / 31 skip (failures are launcher compile-cache and settings-notification suites, not the named output/context victims); seeds 2027 and 101 → 10474 pass / 0 fail / 3 skip each. Polluter→victim probe profilerRetention.test.ts then output.test.ts on merge-base showed intermittent extra failures in profilerRetention prompt-submit tests; on head the output victim tests (suspicious terminal columns can be flagged again after returning to normal width, etc.) pass in that chain. CI shards 2 (context.test.ts) and 7 (output.test.ts) — three local runs each, all exit 0, 0 fail; no exit 132.
Automated review: CodeRabbit (ASSERTIVE, commit ac438a3) reported no actionable comments on the two changed files. CodeQL completed successfully with no findings attributed to this diff.
Summary
bun run scripts/run-tests.ts --randomize --seed=<S> --max-concurrency=1 --feature=UNATTENDED_RETRY(also present on main). Each failing test passes alone.suspicious terminal columns can be flagged again after returning to normal width(src/ink/output.test.ts) andunknown openai-compatible model fallback logs one debug warning and no console errors(src/utils/context.test.ts) then counted calls from earlier tests.beforeEach(one line per file). The polluting files are not touched.Impact
Testing
bun run typecheck,bun run lint(0 errors), the touched test files andbun run security:pr-scan -- --base origin/main --head HEADpass. With this fix and its three sibling fixes applied together on main,bun run build,bun run check,bun run typecheck,bun run typecheck:type-testsandbun run lintpass, and these all have 0 failures: the serial command with--feature=UNATTENDED_RETRYfor seeds 4242, 2027 and 101; the default serial command (--randomize --max-concurrency=1) for seeds 101, 4242 and 2027; and the eight-shard randomized runs (--shards=8) for seeds 101, 4242 and 2027.ELOCKEDrace appeared once under heavy load and passed on rerun; it is being fixed separately.Notes
bun run scripts/run-tests.ts --randomize --seed=<S> --max-concurrency=1 --feature=UNATTENDED_RETRYrun. Each fixes only its own polluter/victim pair, so a full serial seed is green only once all four are merged:test/serial-model-cache-reset,test/serial-credential-mock-restore,test/serial-ultrathink-restore(sibling branches).Summary by CodeRabbit