fix(test): isolate connection.test.ts to prevent mock.module() pollution

[narigondelsiglo]

Summary

• Problem: connection.test.ts was in the same bun test batch as workflows.test.ts. The latter calls mock.module('./connection', () => ({ getDatabaseType: () => 'postgresql' })), which permanently replaces the module in Bun's process-wide cache (oven-sh/bun#7823) — mock.restore() does NOT undo it. This caused getDatabaseType() to always return 'postgresql' in connection.test.ts regardless of process.env.DATABASE_URL, producing silent false-passes.
• Why it matters: The connection module is foundational — SQLite vs. PostgreSQL routing affects every DB operation. A polluted test gives false confidence that getDatabaseType() behaves correctly for both paths.
• What changed: src/db/connection.test.ts is moved into its own bun test invocation (batch 4) that runs before the batch containing src/db/workflows.test.ts.
• What did not change: No production code was modified; all other test batches remain identical.

UX Journey

Before

bun run test
  batch N: [..., connection.test.ts, ..., workflows.test.ts, ...]
            ^ workflows.test.ts patches mock.module('./connection')
              connection.test.ts runs AFTER → getDatabaseType() always returns 'postgresql'
              → tests pass but assertions are wrong

After

bun run test
  batch 4: [connection.test.ts]            ← isolated, runs before mock pollution
  batch N: [..., workflows.test.ts, ...]   ← mock patch only affects later batches
             → connection.test.ts results are trustworthy

Architecture Diagram

Before

packages/core/package.json test script
  batch N (single invocation)
    connection.test.ts  ──▶  getDatabaseType()  [polluted by workflows.test.ts]
    workflows.test.ts   ──▶  mock.module('./connection')  [permanent side-effect]

After

packages/core/package.json test script
  batch 4 (new, isolated invocation)
    connection.test.ts  ──▶  getDatabaseType()  [clean process, no pollution]
  batch N (unchanged invocation)
    workflows.test.ts   ──▶  mock.module('./connection')  [side-effect contained]

Connection inventory:

From	To	Status	Notes
packages/core/package.json test script	connection.test.ts	modified	Moved to own batch (batch 4)
packages/core/package.json test script	workflows.test.ts	unchanged	Still in its original batch

Label Snapshot

• Risk: risk: low
• Size: size: XS
• Scope: tests
• Module: core:db

Change Metadata

• Change type: chore
• Primary scope: core

Linked Issue

• Related to the mock-isolation rules documented in CLAUDE.md (@archon/core test isolation section)

Validation Evidence (required)

bun run type-check   # ✅ pass
bun run lint         # ✅ pass (0 errors, 0 warnings)
bun run format:check # ✅ pass
bun run test         # ✅ pass (all tests pass after fix)

• Evidence: All four checks passed after moving connection.test.ts to its own invocation.
• No commands skipped.

Security Impact (required)

• New permissions/capabilities? No
• New external network calls? No
• Secrets/tokens handling changed? No
• File system access scope changed? No

Compatibility / Migration

• Backward compatible? Yes
• Config/env changes? No
• Database migration needed? No

Human Verification (required)

• Verified scenarios: bun run test passes end-to-end with the batch split in place.
• Edge cases checked: Confirmed that connection.test.ts correctly tests both SQLite (no DATABASE_URL) and PostgreSQL paths after isolation.
• What was not verified: Running with a live PostgreSQL instance (SQLite path verified locally).

Side Effects / Blast Radius (required)

• Affected subsystems: @archon/core test suite only.
• Potential unintended effects: None — only the order/grouping of test invocations changed; no production code touched.
• Guardrails: bun run validate catches any future batch ordering regressions.

Rollback Plan (required)

• Fast rollback: git revert HEAD — one-line change, zero risk.
• Feature flags: None.
• Observable failure symptoms: connection.test.ts assertions fail with unexpected 'postgresql' result when DATABASE_URL is unset.

Risks and Mitigations

• Risk: Future additions to the batch could re-introduce pollution.
  • Mitigation: The CLAUDE.md mock-isolation rules are explicit about this pattern; any new mock.module() call must be reviewed against existing batch groupings.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 22999712-23e9-463b-8f68-55dff3fe941d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[narigondelsiglo]

🔍 Comprehensive PR Review

PR: #1253
Reviewed by: 5 specialized agents (code-review, error-handling, test-coverage, comment-quality, docs-impact)
Date: 2026-04-16

---

Summary

This is a minimal, surgical fix that isolates connection.test.ts into its own bun test invocation to prevent mock.module() pollution from workflows.test.ts and other DB test files. The change is correct, well-motivated, and fully aligns with the project's established mock-isolation pattern. Only one finding emerged across all 5 review agents — a stale batch count in CLAUDE.md.

Verdict: ✅ APPROVE

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	0
🟡 MEDIUM	0
🟢 LOW	1

---

🟢 Low Issues

Stale @archon/core batch count in CLAUDE.md

📍 CLAUDE.md:131

CLAUDE.md states @archon/core has "7 batches", but the test script now has 14 bun test invocations after this PR (was already 13 before — the count was stale prior to this change). This is a one-word fix that fits naturally in the same PR.

View fix

-`@archon/core` (7 batches), `@archon/workflows` (5), `@archon/adapters` (3), `@archon/isolation` (3)
+`@archon/core` (14 batches), `@archon/workflows` (5), `@archon/adapters` (3), `@archon/isolation` (3)

Counted batches after this PR:

1. command-handler.test.ts
2. clone.test.ts
3. db/adapters/postgres.test.ts
4. db/connection.test.ts ← new isolated batch (this PR)
5. Large DB+utils batch (db/adapters/sqlite.test.ts + others)
6. utils/path-validation.test.ts
7. services/cleanup-service.test.ts
8. services/title-generator.test.ts
9. workflows/
10. operations/workflow-operations.test.ts
11. operations/isolation-operations.test.ts
12. orchestrator/orchestrator.test.ts
13. orchestrator/orchestrator-agent.test.ts
14. orchestrator/orchestrator-isolation.test.ts

---

✅ What's Good

• Correct and minimal scope: Only packages/core/package.json is touched; no production code modified.
• Correct batch placement: New batch runs connection.test.ts before the large DB batch that contains the mock.module('./connection') callers — mock pollution flows forward within a process, so this ordering is correct.
• Cross-contamination verified: postgres.test.ts (the batch just before the new one) only mocks pg and @archon/paths, not ./connection, so it cannot pollute connection.test.ts.
• Thorough PR description: Explains the Bun bug (oven-sh/bun#7823), before/after diagrams, validation evidence, and rollback plan.
• Correct commit convention: fix(test): ... follows the project's established format.
• Follows CLAUDE.md mock isolation rules: No mock.restore() added for mock.module() calls (which would have no effect).

---

Next Steps

1. Optional (recommended): Apply the 1-word CLAUDE.md fix (7 → 14)
2. Approve and merge ✓

---

Reviewed by Archon comprehensive-pr-review workflow (5 agents)
Artifacts: /home/alvaro/.archon/workspaces/coleam00/Archon/artifacts/runs/ef9b6a7b751111fb7a445ce099616131/review/

[narigondelsiglo]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-archon-fix-github-issue-1776334673384
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (1 total)

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	0
🟡 MEDIUM	0
🟢 LOW	1

View all fixes

• ✅ CLAUDE.md batch count stale (CLAUDE.md:131) — Updated @archon/core batch count from "7 batches" → "14 batches" to match the actual count after this PR adds the 14th isolated batch

---

Tests Added

(none)

---

Skipped (0)

(none — all findings addressed)

---

Suggested Follow-up Issues

(none)

---

Validation

✅ Type check | ✅ Lint | ✅ Tests

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-archon-fix-github-issue-1776334673384

[Wirasm]

This PR looks similar to the already-merged #1269 (merged April 17, 2026). Please check whether your change is still needed — rebase on main and confirm.