fix(resolve): address review comments and add tests for fuzzy project recovery#732
fix(resolve): address review comments and add tests for fuzzy project recovery#732
Conversation
… recovery
- Refactor tryFuzzyProjectRecovery to return discriminated result type
(match | suggestions | none) instead of logging/throwing internally.
Callers now own side effects — log.warn only after confirming success.
- Fix Seer: after fuzzy recovery, getProject failure no longer shows
misleading error referencing the original unfound slug.
- Fix Seer: recursive handleProjectSearch failure no longer references
the recovered slug the user never typed.
- Fix Cursor: multiple fuzzy matches no longer throw before JSON early
return — JSON consumers get { items: [] } instead of exception.
- Add 8 unit tests for tryFuzzyProjectRecovery covering single match,
multiple matches, no matches, empty orgs, API failures, cross-org
search, slug deduplication, and partial org failures.
Follow-up to PR #728.
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛Resolve
Upgrade
Other
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
|
Codecov Results 📊✅ 134 passed | Total: 134 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ❌ Patch coverage is 66.15%. Project has 1632 uncovered lines. Files with missing lines (2)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 95.19% 95.25% +0.06%
==========================================
Files 234 234 —
Lines 34349 34355 +6
Branches 0 0 —
==========================================
+ Hits 32697 32723 +26
- Misses 1652 1632 -20
- Partials 0 0 —Generated by Codecov Action |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
When fuzzy recovery finds a similar project but getProject returns an error, mention the matched project in the ResolutionError suggestions so the user knows what was tried.
| return withTelemetryContext({ | ||
| org: recovered.org, | ||
| project: recovered.project, | ||
| org: fuzzyResult.org, | ||
| project: fuzzyResult.project, | ||
| }); |
There was a problem hiding this comment.
Bug: The resolveOrgProjectTarget function logs a fuzzy-match warning before verifying project accessibility, leading to potentially misleading warnings followed by permission errors.
Severity: MEDIUM
Suggested Fix
Before logging the fuzzy match warning in resolveOrgProjectTarget, add a call to getProject() to verify that the user has access to the recovered project. The warning should only be logged if the accessibility check succeeds, which would align its behavior with the resolveProjectBySlug function.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/lib/resolve-target.ts#L1493-L1496
Potential issue: In the `resolveOrgProjectTarget` function, when a fuzzy project match
is found, a warning is logged to the user before verifying that the user has access to
the recovered project. This is inconsistent with another code path in the same PR,
`resolveProjectBySlug`, which correctly verifies access first. This can lead to a
confusing user experience where a user is told a similar project will be used, but then
the command fails with a permission error because the recovered project is inaccessible
to them.
There was a problem hiding this comment.
Valid observation about the inconsistency. In this code path (resolveOrgProjectTarget), the function returns just { org, project } — it doesn't need projectData and doesn't call getProject. The downstream command (trace/span/log list) will make its own API call and surface any permission error directly.
Adding a getProject check here would add an extra API call on a path that works ~99% of the time (fuzzy match found = project exists in that org). The risk is a brief misleading warning followed by a clear 403 error. I'll leave this as-is for now since the edge case is very narrow and the downstream error is still actionable.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b9e42a5. Configure here.
| org: recovered.org, | ||
| project: recovered.project, | ||
| org: fuzzyResult.org, | ||
| project: fuzzyResult.project, |
There was a problem hiding this comment.
Fuzzy recovery warns without verifying project accessibility
Medium Severity
resolveOrgProjectTarget emits the "Using similar project" log.warn() and returns the fuzzy-matched slugs without first verifying that the recovered project is accessible. In contrast, resolveProjectBySlug correctly calls getProject() via withAuthGuard and only warns after confirming proj.ok. If the matched project turns out to be inaccessible, the user sees a misleading recovery message followed by an unrelated downstream error. The same issue exists in tryFuzzyRecoveryForList, which warns before the caller's recursive handleProjectSearch call verifies the project. The PR description explicitly states "callers only warn after confirming the recovered project is accessible."
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b9e42a5. Configure here.
There was a problem hiding this comment.
Same point as the Seer comment above — replied there with rationale. resolveOrgProjectTarget returns { org, project } without needing projectData, and the downstream command's API call will surface any permission error directly. Adding a getProject() check here would add an extra API call for a very narrow edge case.
For tryFuzzyRecoveryForList in org-list.ts, the caller's recursive handleProjectSearch does call findProjectsBySlug which verifies the project exists. If it doesn't exist, the recursion guard (_isRecoveryAttempt=true) prevents looping and falls through to the error.
1 participant
Summary
Follow-up to PR #728 — addresses 3 missed review comments and adds unit tests for patch coverage.
Review fixes
tryFuzzyProjectRecoverynow returns a discriminated result type (match|suggestions|none) instead of logging/throwing. Callers only warn after confirming the recovered project is accessible.handleProjectSearchcall fails, the error references the original slug the user typed, not the recovered one.suggestionsare returned instead of thrown — callers pass them toResolutionErrorfor human mode or return{ items: [] }for JSON mode.Coverage
tryFuzzyProjectRecoverycovering: single match, multiple matches, no matches, empty orgs, API failures, cross-org search, slug deduplication, partial org failures