Skip to content

fix: route canonical path failures through blocked classification#1211

Merged
Wirasm merged 1 commit intodevfrom
fix/resolver-canonical-path-blocked
Apr 14, 2026
Merged

fix: route canonical path failures through blocked classification#1211
Wirasm merged 1 commit intodevfrom
fix/resolver-canonical-path-blocked

Conversation

@Wirasm
Copy link
Copy Markdown
Collaborator

@Wirasm Wirasm commented Apr 14, 2026

Summary

Follow-up to the CodeRabbit review on #1206. The early `getCanonicalRepoPath()` wrap in `IsolationResolver.resolve()` threw directly, bypassing the classification flow that `createNewEnvironment` uses. Permission errors, ENOENT, malformed worktree pointers, etc. would surface as unclassified crashes instead of becoming an actionable `blocked` result.

Change

Mirror `createNewEnvironment`'s contract:

  • `isKnownIsolationError(err)` → `{ status: 'blocked', reason: 'creation_failed', userMessage: classifyIsolationError(err) + ' Execution blocked...' }`
  • Unknown errors → throw (programming bugs stay visible as crashes, not silent isolation failures)

Single file behavior change in `packages/isolation/src/resolver.ts:114`. Two tests added in `resolver.test.ts`:

  • EACCES classifies to "Permission denied" blocked message
  • Unknown error propagates as throw

Testing

  • `bun run type-check` — clean
  • `bun run lint` — zero warnings
  • `bun test packages/isolation/src/resolver.test.ts` — 36 pass (2 new)
  • Full test suite green

Review reference

CodeRabbit comment on #1206 — "Canonical-path failures still escape resolve() as thrown exceptions."

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling during repository path resolution. When infrastructure-related issues prevent execution, users now receive a clear "blocked" status with descriptive messaging instead of an exception.
  • Tests

    • Added comprehensive test coverage for repository path resolution failure scenarios, validating the system's handling of both infrastructure-related and unexpected errors.

Follow-up to #1206 review: the early getCanonicalRepoPath() wrap in
resolve() threw directly, escaping the classification flow that
createNewEnvironment uses. Permission errors, malformed worktree
pointers, ENOENT, etc. surfaced as unclassified crashes instead of
becoming an actionable `blocked` result.

Mirror createNewEnvironment's contract:
- isKnownIsolationError → return { status: 'blocked', reason:
  'creation_failed', userMessage: classifyIsolationError(err) + suffix }
- unknown errors → throw (programming bugs stay visible as crashes,
  not silent isolation failures)

Adds two tests in resolver.test.ts:
- EACCES classifies to "Permission denied" blocked message
- Unknown error propagates as throw

Addresses CodeRabbit review comment on #1206.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 112657bf-b917-497a-97ec-54d9dbac674e

📥 Commits

Reviewing files that changed from the base of the PR and between fd3f043 and c9c8f09.

📒 Files selected for processing (2)
  • packages/isolation/src/resolver.test.ts
  • packages/isolation/src/resolver.ts

📝 Walkthrough

Walkthrough

The resolver's canonical path computation is updated to convert known infrastructure-related failures into a blocked status with a user message, while unknown failures are rethrown. Tests validate both error handling paths.

Changes

Cohort / File(s) Summary
Resolver error handling
packages/isolation/src/resolver.ts
Updated canonical repo path computation to convert known infrastructure failures (e.g., EACCES) to status: 'blocked' results with classified user messages, while unknown errors are rethrown after normalization. Removed prior behavior that always logged and threw wrapped errors.
Error handling test suite
packages/isolation/src/resolver.test.ts
Added test suite canonical path resolution failure handling with two tests: one validating known infrastructure errors (EACCES) return blocked results with appropriate user messages, another ensuring unknown errors are rethrown rather than returning blocked results.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 A resolver so wise now knows the way—

When paths are blocked, it smartly says "nay,"

Infrastructure woes turned user-friendly notes,

While unknown errors still get rethrown strokes! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the problem, solution, and testing; however, it lacks several required template sections including UX Journey, Architecture Diagram, Label Snapshot, Change Metadata, Validation Evidence (full format), Security Impact, Compatibility, Human Verification, Side Effects, and Rollback Plan. Complete the missing template sections: provide UX Journey diagrams, Architecture Diagram, required label metadata, full validation evidence with command results, Security Impact assessment, Compatibility details, Human Verification scope, Side Effects analysis, and Rollback Plan.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the primary change: routing canonical path failures through blocked classification instead of throwing directly.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 fix/resolver-canonical-path-blocked

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 Wirasm merged commit 5a4541b into dev Apr 14, 2026
4 checks passed
@Wirasm Wirasm deleted the fix/resolver-canonical-path-blocked branch April 14, 2026 12:19
joaobmonteiro pushed a commit to joaobmonteiro/Archon that referenced this pull request Apr 26, 2026
…leam00#1211)

Follow-up to coleam00#1206 review: the early getCanonicalRepoPath() wrap in
resolve() threw directly, escaping the classification flow that
createNewEnvironment uses. Permission errors, malformed worktree
pointers, ENOENT, etc. surfaced as unclassified crashes instead of
becoming an actionable `blocked` result.

Mirror createNewEnvironment's contract:
- isKnownIsolationError → return { status: 'blocked', reason:
  'creation_failed', userMessage: classifyIsolationError(err) + suffix }
- unknown errors → throw (programming bugs stay visible as crashes,
  not silent isolation failures)

Adds two tests in resolver.test.ts:
- EACCES classifies to "Permission denied" blocked message
- Unknown error propagates as throw

Addresses CodeRabbit review comment on coleam00#1206.
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