fix(test): update stale mocks in cleanup-service 'continues processing' test by Wirasm · Pull Request #1232 · coleam00/Archon

Wirasm · 2026-04-15T07:17:13Z

Summary

Problem: runScheduledCleanup > continues processing after error on one environment test fails on dev with expected ['env-error (path missing)'] but got []
Why it matters: Blocks all PRs from merging to dev (CI gate failure introduced by regression in PR fix(isolation): complete reports false success when worktree remains on disk (fixes #964) #1034)
What changed: Updated test mock setup in cleanup-service.test.ts — replaced two stale mockExecFileAsync.mockRejectedValueOnce calls with proper mockGetById + mockGetCodebase return values for both test environments
What did not change: No runtime behavior changed; cleanup-service.ts is untouched

UX Journey

Before

(No user-facing change — test-only fix)

After

(Same — test-only fix, no UX change)

Architecture Diagram

Before

cleanup-service.test.ts
  └── continues processing after error
        ├── mockExecFileAsync.mockRejectedValueOnce(...)  [stale — exec no longer used for worktree check]
        └── mockExecFileAsync.mockRejectedValueOnce(...)  [stale]

After

cleanup-service.test.ts
  └── continues processing after error
        ├── mockGetById.mockResolvedValueOnce({ id: 'env-error', ... })   [~] new mock target
        ├── mockGetCodebase.mockResolvedValueOnce({ id: 'codebase-1', ... })  [+]
        ├── mockGetById.mockResolvedValueOnce({ id: 'env-good', ... })    [~]
        └── mockGetCodebase.mockResolvedValueOnce({ id: 'codebase-1', ... })  [+]

Connection inventory:

From	To	Status	Notes
`cleanup-service.test.ts`	`mockExecFileAsync`	removed	Was targeting exec-based worktree check; no longer relevant
`cleanup-service.test.ts`	`mockGetById`	modified	Now provides env records for both test envs
`cleanup-service.test.ts`	`mockGetCodebase`	modified	Now provides codebase records for both test envs

Label Snapshot

Risk: risk: low
Size: size: XS
Scope: tests
Module: core:cleanup-service

Change Metadata

Change type: bug
Primary scope: core

Linked Issue

Closes CI: runScheduledCleanup 'continues processing after error' test fails on dev #1230

Validation Evidence (required)

bun run validate

Type check: ✅ Pass (all 10 packages)
Lint: ✅ Pass (0 errors, 0 warnings)
Format: ✅ Pass
Tests: ✅ Pass (3045 passed, 0 failed)

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 validate passes cleanly; the previously-failing test now passes
Edge cases checked: Confirmed other tests in the same describe block that call removeEnvironment via paths where pathExists=true are unaffected (they don't hit the getById early-return)
What was not verified: No manual runtime testing needed — this is a test-only fix with no production code changes

Side Effects / Blast Radius (required)

Affected subsystems/workflows: @archon/core test suite only
Potential unintended effects: None — no runtime code changed
Guardrails/monitoring for early detection: CI will confirm

Rollback Plan (required)

Fast rollback command/path: git revert HEAD — single commit
Feature flags or config toggles: None
Observable failure symptoms: The same test assertion failure as described in CI: runScheduledCleanup 'continues processing after error' test fails on dev #1230

Risks and Mitigations

None — test-only change with no production code impact.

Summary by CodeRabbit

Tests
- Improved test reliability for environment cleanup operations by refining mock data dependencies.

…g' test (#1230) After PR #1034 changed worktree existence checks from execFileAsync to fs/promises.access, the mockExecFileAsync rejections had no effect. removeEnvironment needs getById + getCodebase mocks to proceed past the early-return guard, otherwise envs route to report.skipped instead of report.removed. Replace the two stale mockExecFileAsync rejection calls with proper mockGetById and mockGetCodebase return values for both test environments. Fixes #1230

coderabbitai · 2026-04-15T07:17:20Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4f1b499f-fdd6-49c8-af33-517e38f4037c

📥 Commits

Reviewing files that changed from the base of the PR and between f61d576 and 1004cf3.

📒 Files selected for processing (1)

packages/core/src/services/cleanup-service.test.ts

📝 Walkthrough

Walkthrough

A test update that replaces git execution mocks with database lookup mocks to align with production changes, allowing the error-handling scenario to execute properly through the actual removeEnvironment code path.

Changes

Cohort / File(s)	Summary
Test Mock Strategy Update `packages/core/src/services/cleanup-service.test.ts`	Replaced `mockExecFileAsync` rejection mocks (simulating git worktree failures) with sequential `mockGetById` and `mockGetCodebase` mocks for two environments. Test now exercises the actual `removeEnvironment` data-fetching logic instead of short-circuiting at git layer, while preserving assertion that both environments appear in removal report as "path missing".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

fix(isolation): complete reports false success when worktree remains on disk (fixes #964) #1034 — PR that introduced git worktree prune and post-removal verification steps in WorktreeProvider.destroy(), triggering the cascade that broke this test's original mock strategy.

Poem

🐰 A test once mocked at the git layer deep,
But the cleanup path took a twist in its sleep.
Now we mock the database, the truth-teller's way,
And the error continues—hooray, hooray, hooray! 📋✨

✨ 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 archon/task-fix-issue-1230

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 and usage tips.}

Wirasm · 2026-04-15T07:20:47Z

🔍 Comprehensive PR Review

PR: #1232 — fix(test): update stale mocks in cleanup-service 'continues processing' test
Reviewed by: code-review agent (5 agents planned; 4 artifacts not generated)
Date: 2026-04-15

Summary

Surgical test-only fix (+27/-4, 1 file) that replaces stale mockExecFileAsync calls with the correct mockGetById + mockGetCodebase sequence that removeEnvironment actually uses. Mock ordering matches the real call sequence in the service. No production code was touched.

Verdict: APPROVE

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

🟢 Low Issues

Partial mock shapes for `mockGetById` vs full shapes for `mockListAllActiveWithCodebase`

📍 packages/core/src/services/cleanup-service.test.ts:714–738

The mockGetById returns omit several fields present on the full IsolationEnvironment type (created_at, created_by_platform, workflow_type, etc.). This is not a bug — removeEnvironment only reads the five fields that are present. The minimal shape is consistent with how mockGetById is used elsewhere in the file (lines 132–145).

Recommendation: Leave as-is.

✅ What's Good

Mock interleaving (env-error getById → getCodebase, then env-good getById → getCodebase) correctly mirrors the sequential for loop in runScheduledCleanup and sequential calls inside removeEnvironment
mockResolvedValueOnce ordering matches the actual call sequence in cleanup-service.ts:158–173
Comment block clearly labels which mock calls belong to which environment
No new mock.module() calls — no risk of cross-file mock pollution
No afterAll(() => mock.restore()) added for mock.module() cleanup (per CLAUDE.md)
Assertion at lines 743–744 correctly verifies both environments in report.removed, exercising the intended "continues processing after error" behavior

Next Steps

Ready to merge once CI passes (docker-build, ubuntu/windows tests were pending at review time). No changes requested.

Reviewed by Archon comprehensive-pr-review workflow

Wirasm · 2026-04-15T07:21:26Z

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
No changes needed — nothing to push
Philosophy: Fix everything unless clearly a new concern

Findings Summary

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

Fixes Applied

(none)

Tests Added

(none)

Skipped (1)

Finding	Reason
Partial mock shapes for `mockGetById` vs full shapes for `mockListAllActiveWithCodebase` (`cleanup-service.test.ts:714–738`)	Review agent explicitly recommended leaving as-is: minimal shape is consistent with existing `removeEnvironment` test patterns and omitted fields are irrelevant to the code path under test. Expanding would add noise without catching a real bug.

Suggested Follow-up Issues

(none)

Validation

✅ Type check | ✅ Lint | ✅ Tests (no code changed)

Self-fix by Archon · aggressive mode · no changes needed

…g' test (coleam00#1230) (coleam00#1232) After PR coleam00#1034 changed worktree existence checks from execFileAsync to fs/promises.access, the mockExecFileAsync rejections had no effect. removeEnvironment needs getById + getCodebase mocks to proceed past the early-return guard, otherwise envs route to report.skipped instead of report.removed. Replace the two stale mockExecFileAsync rejection calls with proper mockGetById and mockGetCodebase return values for both test environments. Fixes coleam00#1230

Wirasm mentioned this pull request Apr 15, 2026

CI: runScheduledCleanup 'continues processing after error' test fails on dev #1230

Closed

Wirasm marked this pull request as ready for review April 15, 2026 08:53

Wirasm merged commit 5c8c39e into dev Apr 15, 2026
4 checks passed

Wirasm deleted the archon/task-fix-issue-1230 branch April 15, 2026 09:13

coderabbitai Bot mentioned this pull request Apr 20, 2026

refactor(core): migrate mock.module() to spyOn() — single bun test invocation #1300

Closed

2 tasks

Wirasm mentioned this pull request Apr 22, 2026

fix(orchestrator): use codebase cwd for direct chat provider execution #1233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(test): update stale mocks in cleanup-service 'continues processing' test#1232

fix(test): update stale mocks in cleanup-service 'continues processing' test#1232
Wirasm merged 1 commit into
devfrom
archon/task-fix-issue-1230

Wirasm commented Apr 15, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 15, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

Wirasm commented Apr 15, 2026

Uh oh!

Wirasm commented Apr 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Wirasm commented Apr 15, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

UX Journey

Before

After

Architecture Diagram

Before

After

Label Snapshot

Change Metadata

Linked Issue

Validation Evidence (required)

Security Impact (required)

Compatibility / Migration

Human Verification (required)

Side Effects / Blast Radius (required)

Rollback Plan (required)

Risks and Mitigations

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

Wirasm commented Apr 15, 2026

🔍 Comprehensive PR Review

Summary

🟢 Low Issues

Partial mock shapes for mockGetById vs full shapes for mockListAllActiveWithCodebase

✅ What's Good

Next Steps

Uh oh!

Wirasm commented Apr 15, 2026

⚡ Self-Fix Report (Aggressive)

Findings Summary

Fixes Applied

Tests Added

Skipped (1)

Suggested Follow-up Issues

Validation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Wirasm commented Apr 15, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 15, 2026 •

edited

Loading

Partial mock shapes for `mockGetById` vs full shapes for `mockListAllActiveWithCodebase`