Skip to content

test: init guard coverage follow-ups (PR #48 review F1/F2) - #50

Merged
ManningWorks merged 1 commit into
mainfrom
test/init-guard-coverage-followups
Oct 14, 2026
Merged

ManningWorks merged 1 commit into
mainfrom
test/init-guard-coverage-followups

Conversation

@ManningWorks

Copy link
Copy Markdown
Owner

What this does

Closes the two non-blocking LOW test-coverage findings from the APPROVE review of PR #48 (the issue #41 init-overwrite-guard fix). Both are follow-ups that add missing coverage — no production code changes.

Findings addressed

F1 — --yes + non-TTY combined test (was untested)
init({ yes: true }) previously only ran with a TTY stdin. Added a case with a non-TTY stdin that asserts confirm() is never called, the existing config is overwritten, and init() resolves cleanly. Pins the invariant that --yes short-circuits the guard before the !process.stdin.isTTY check is reached, guarding against a future refactor that moves the TTY check outside the !options.yes guard.

F2 — existence-check non-ENOENT branch untested
init.ts else { throw error } (the existence check) was not covered — only the prompt-path error surfacing was. Stubbed only access from node:fs/promises (all other fs ops stay real via importOriginal); the default stub replicates real access(path, F_OK) so every existing test keeps its real-fs behavior. The new case makes access reject with an EACCES-coded error and asserts init() rejects without writing.

Verification

  • Full gate green: pnpm install && pnpm build && pnpm typecheck && pnpm lint && pnpm test (1045 core tests pass).
  • Mutation-checked both new tests: each fails when its guarded invariant is broken (swallowed existence-check error; TTY check moved outside the --yes guard) and passes when restored.
  • Target file: pnpm --dir packages/core exec vitest run src/commands/__tests__/init.test.ts → 8 passed.

Provenance: #48 (comment)

Close the two non-blocking LOW coverage findings from the APPROVE review of
PR #48 (issue #41 init overwrite guard):

F1 -- no `--yes` + non-TTY combined test. Add a case running `init({ yes:
true })` with a non-TTY stdin: confirm() is never called, the existing config
is overwritten, and init resolves cleanly. Pins the invariant that `--yes`
short-circuits the guard before the `!process.stdin.isTTY` check is reached.

F2 -- existence-check non-ENOENT branch untested. Stub only `access` from
node:fs/promises (all other fs ops stay real via importOriginal); the default
stub replicates real `access(path, F_OK)` (ENOENT when absent) so every other
test keeps its real-fs behavior. The new case makes `access` reject with an
EACCES-coded error and asserts init rejects without writing.

Verified: pnpm install && build && typecheck && lint && test all green (1045
core tests). Mutation-checked both new tests against the exact regressions
they guard (each fails when its invariant breaks).
@vercel

vercel Bot commented Oct 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
projex-docs Ready Ready Preview Oct 14, 2026 12:24am UTC

@coderabbitai

coderabbitai Bot commented Oct 14, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dab44f96-01da-4218-93ec-6b234aff66a3
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@ManningWorks

Copy link
Copy Markdown
Owner Author

Review: APPROVE

Reviewed the diff against merge-base 642f0b0 — identical to the tip of origin/main, so the branch is not behind — one commit 7ae2e70, exactly one file: packages/core/src/commands/__tests__/init.test.ts (+66/−0, no production code). Walked both axes — Standards (sibling test-pattern parity + coding-style baseline) and Spec (the two LOW findings F1/F2 from the #48 review as restated in the follow-up card) — plus all four review asks. Verdict: APPROVE — no CRITICAL / HIGH / MEDIUM / LOW findings. Both findings are closed by tests; neither was skipped.

Live-verified, not assumed: re-ran the sanctioned single-file suite at the PR head — pnpm --dir packages/core exec vitest run src/commands/__tests__/init.test.ts → 8 passed (8) — and re-ran both mutation checks myself (ask 4 below).


What the tests do right

  • F1 pins the real invariant, no tautology — setStdinTTY(undefined) + init({ yes: true }) over an existing config asserts confirm never called, config overwritten (content contains defineProjects), clean resolve, no process.exit. The ordering it guards — !options.yes at init.ts:66 short-circuiting before the !process.stdin.isTTY check at init.ts:70 — is real production behavior exercised, and the in-test comment names the exact refactor it guards against.
  • F2 drives the real else { throw error } branch (init.ts:62) — only access from node:fs/promises is stubbed to reject with an EACCES-coded error; the test asserts init() rejects with that code (toMatchObject({ code: 'EACCES' })), the pre-existing config is byte-identical (nothing written), and no process.exit. Not a tautology: the stubbed rejection demonstrably reaches the production catch (Mutation B below).
  • Stub scope is exactly right — vi.mock('node:fs/promises', async (importOriginal) => ({ ...actual, access: vi.fn() })) keeps every other fs op (readFile/writeFile/rename/unlink/constants) real. The mock specifier matches the production import in init.ts:1 byte-for-byte, so it intercepts the actual binding init.ts uses.
  • Default stub is faithful to access(path, F_OK) — beforeEach sets mockImplementation resolving when existsSync(path) and rejecting with a coded ENOENT otherwise; relative paths resolve against the chdir'd tmpdir exactly as the real relative call at init.ts:57 does. All seven pre-existing tests keep their real-fs behavior (8/8 green).
  • No stub leakage — vi.resetAllMocks() at the top of beforeEach wipes any prior test's mockRejectedValue before the default F_OK stub is re-installed, so the EACCES override cannot leak; afterEach restores cwd, removes the tmpdir, and resets TTY.
  • Comments explain why, not what — the mock factory and each new test carry the rationale (which branch is driven, which invariant is pinned), matching init.ts's own comment style.
  • Scope is clean — exactly one test file; no production code, no package.json version bump, no CHANGELOG (house merge-only pattern); branch fresh off origin/main.

The four review asks

  1. Real invariants, no tautologies — both tests assert behavior (rejects with code / overwrite + no confirm + no exit) while exercising the real production code with only the I/O boundary stubbed. ✅
  2. Stub fidelity + leak-proof setup/teardown — default stub replicates F_OK semantics for every path these tests touch (self-owned tmpdir, full permissions); per-test re-install in beforeEach after vi.resetAllMocks(), restore in afterEach. ✅
  3. Scope — one file, +66, test-only; no version bump, no CHANGELOG. ✅
  4. Evidence spot-check + live mutation re-run — the card's mutation claims are specific, and I re-ran them in a detached worktree at 7ae2e70:
    • Baseline: 8 passed (8).
    • Mutation A (F1): moved the TTY check outside the !options.yes guard — the exact refactor the test guards — → F1 test RED at init.test.ts:243 (expected 'export const projects = []' to contain 'defineProjects' — the config was not overwritten).
    • Mutation B (F2): made the existence check swallow the non-ENOENT error instead of throw-ing (the issue init: empty catch{} around overwrite confirm() swallows ExitPromptError — non-TTY overwrite guard is incidental #41 bug class) → F2 test RED at init.test.ts:193 (init() resolved instead of rejecting EACCES).
    • Restored: 8 passed (8).
      Each test fails when its guarded invariant breaks and passes when restored. ✅

No findings (none CRITICAL / HIGH / MEDIUM / LOW). The two LOWs from the #48 review are now closed by tests with demonstrated teeth.

Approving. Ready for the merge-only tail.

@ManningWorks
ManningWorks merged commit c50b752 into main Oct 14, 2026
7 checks passed

This branch was successfully deployed

1 active deployment
Preview — 7ae2e709 Deployed Oct 14, 2026 by vercel[bot]
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