Skip to content

docs(agents): note Bun process-level assertion gotchas in TS test rule (EXSC-687) - #2134

Open
0xDEnYO wants to merge 2 commits into
mainfrom
claude/sweet-clarke-d9f49b
Open

docs(agents): note Bun process-level assertion gotchas in TS test rule (EXSC-687)#2134
0xDEnYO wants to merge 2 commits into
mainfrom
claude/sweet-clarke-d9f49b

Conversation

@0xDEnYO

@0xDEnYO 0xDEnYO commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Which Linear task belongs to this PR?

EXSC-687 — this note is a byproduct of writing
script/deploy/github/verify-approvals.test.ts in #2128. No separate ticket was cut; the finding
belongs to the same work.

Why did I implement it this way?

Two Bun-specific traps cost real debugging time while writing tests for the approval-check exit-code
gate, and both are invisible until you hit them:

  1. spyOn(process.stdout, 'write') does not capture writes made by the code under test — the
    assertion reports "was not called" even though the write demonstrably happens. The working
    pattern is to inject the output/exit surface as a parameter defaulting to process
    (reportApprovalResult(failures, target: IReportTarget = process) in fix(deploy): gate branch-deploy approval check on exit code (EXSC-687) #2128) and pass a recorder
    in the test.
  2. expect(...).rejects trips @typescript-eslint/await-thenable, because Bun's matcher isn't a
    real Promise. script/deploy/safe/parked-tasks.test.ts already documents this inline and works
    around it with a local expectRejects helper.

Both are "asserting on process-level behaviour" problems, so they're captured as one
[CONV:TEST-PROCESS-SURFACE] section rather than two scattered bullets. Landed in
.agents/rules/402-typescript-tests.md (globs **/*.test.ts) because that's the narrowest owning
rule — a test author hits both traps, and the rule auto-loads exactly when they do. The inline
comment in parked-tasks.test.ts stays where it is; the rule generalises it so the next author
doesn't have to have read that file.

Edited in .agents/ per the repo's symlink convention; .cursor/rules/402-typescript-tests.mdc
and .claude/rules/402-typescript-tests.md already point at it, so no symlink work was needed.
.agents/README.md is unchanged — name, description and globs are all untouched, which is the
only trigger for a README update.

One thing for the reviewer: the verify-approvals.ts reference points at code that only exists
on #2128's branch — it resolves once that PR merges. Everything else cited (expectRejects in
parked-tasks.test.ts) is on main today. The Bun version is called out as observed on 1.3.8
(the local toolchain); the repo pins bun@1.3.13 in packageManager and I did not re-verify the
spy behaviour there, so the guidance is stated unconditionally rather than as version-gated.

Checklist before requesting a review

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

  • I have checked that any arbitrary calls to external contracts are validated and or restricted
  • I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
  • I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

…e (EXSC-687)

Adds [CONV:TEST-PROCESS-SURFACE] to 402-typescript-tests: spyOn on process
globals silently misses writes made by the code under test, so the output/exit
surface must be injected instead; and expect().rejects trips
@typescript-eslint/await-thenable, so a local expectRejects helper is used.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lifi-action-bot
lifi-action-bot marked this pull request as draft July 28, 2026 02:16
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@0xDEnYO, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Limit details: You’ve used all 2 included reviews currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 19b7867d-742b-47d2-895b-047bfa81b7e7

📥 Commits

Reviewing files that changed from the base of the PR and between 9b7861f and b6beb80.

📒 Files selected for processing (1)
  • .agents/rules/402-typescript-tests.md

Walkthrough

Adds TypeScript test guidance for asserting process-level stdout and exit behavior through injected surfaces, and for matching rejected promises with a local helper instead of expect(...).rejects.

Changes

TypeScript test assertion guidance

Layer / File(s) Summary
Process and rejection assertion rules
.agents/rules/402-typescript-tests.md
Documents injectable output/exit surfaces for process behavior and a custom catch-and-match helper for rejection assertions in Bun tests.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested labels: requires-types

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly matches the PR’s main change: documenting Bun process-level assertion gotchas in the TypeScript test rule.
Description check ✅ Passed The description follows the template with the Linear task, rationale, checklists, and reviewer notes filled in.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/sweet-clarke-d9f49b

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@0xDEnYO
0xDEnYO marked this pull request as ready for review July 28, 2026 02:17
@0xDEnYO

0xDEnYO commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Light-triage gate note: bullet 1's factual premise fails to reproduce on Bun 1.3.8 (the exact version the observation cites).

Falsification attempt before this ships as a rule:

  • spyOn(process.stdout, 'write') did capture writes in all three variants tried: direct call in the test file, process.stdout.write from an imported module, and a write followed by process.exit (both spied). spy.mock.calls contained the payloads every time.
  • What does not get captured is console.log output — in Bun it bypasses process.stdout.write entirely (0 recorded calls), unlike Node where console routes through the stream. If the failing assertion in the fix(deploy): gate branch-deploy approval check on exit code (EXSC-687) #2128 test session was over console-based output (or a console.error → stderr mixup), that would explain the observation.
  • Bullet 2 verified true: bun-types/test.d.ts:840 types rejects: Matchers<unknown> (not a Promise), so awaiting it trips @typescript-eslint/await-thenable; the expectRejects reference in script/deploy/safe/parked-tasks.test.ts exists as described.

Suggestion: keep the inject-the-surface advice (good design regardless), but reword bullet 1's justification — as written it bans a working technique on a premise that doesn't repro. If the original failure is reproducible, the rule should name the actual trigger (console vs stream write) instead of the blanket claim. Repro snippet:

import { describe, it, expect, spyOn } from 'bun:test'
import { writeViaStdout, writeViaConsole } from './mod' // process.stdout.write(...) / console.log(...)

it('stdout.write from another module IS captured', () => {
  const spy = spyOn(process.stdout, 'write').mockImplementation(() => true)
  writeViaStdout()
  expect(spy.mock.calls.length).toBeGreaterThan(0) // passes on 1.3.8
  spy.mockRestore()
})

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants