Repository navigation
fix(pm-dispatch): verify issue_number attach target before enqueue (#3007) - #3149
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit details
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 053b32a2e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| } catch (e) { | ||
| const msg = e instanceof Error ? e.message : String(e); | ||
| if (/\b404\b|Not Found/i.test(msg)) return null; |
There was a problem hiding this comment.
Match the HTTP status without scanning the request endpoint
When a lookup for issue #404 falls back to gh and encounters an authentication, rate-limit, or transient server error, gh() includes /issues/404 in its error-message prefix. This regex therefore returns null even though no HTTP 404 occurred, causing handleTasks() to report an invalid target (404) instead of a retryable lookup failure (502). Reproducing the wrapper message with HTTP 503 Service Unavailable returns null for issue 404 but throws for issue 405. Match the actual response status or restrict matching to the error output rather than the endpoint-containing wrapper.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 291030a — correct catch. The not-found check now uses an exported isNotFoundError() that matches only the HTTP status text (HTTP 404), never the bare digits, so an issue literally numbered 404 in the echoed request path cannot turn a 403/429/500 into a definitive not-found. Unit tests cover both directions (HTTP 404 → true; /issues/404 path with HTTP 403/429/500 → false). Suite now 11/11 in test/attach_verify.test.ts.
Fixes #3007 (queue task 4532).
Problem
POST /v1/taskswithissue_numberconstructed the issue reference from the request with no GitHub lookup: a nonexistent number, a PR number, or a closed issue attached "successfully" (201 withissue_url) and a task was enqueued against a bogus or wrong target.Fix (per triage task 4345, CONFIRMED)
github.ts: newGitHubClient.getIssue()— app route withgh apifallback (mirrorscreateIssue); returnsnullonly on a definitive 404, throwsGitHubErrorotherwise so lookup failures stay distinguishable from invalid targets; exposes thepull_requestmarker asisPullRequest.server.ts: exportedverifyAttachTarget()runs before enqueue —attach_verified: falseplus an explicit "was NOT verified" warning.attach_verified: true.Coordination with #2677 / PR #3143 (task 4417, parked for CI)
Same handler, separate defect — not absorbed. This branch is main-based; the attach-verify helpers were deliberately placed by the Routing section so the two branches merge cleanly: verified on the laptop with a trial merge of
queue/4417-2677-dispatch-preexistinginto this branch ("Automatic merge went well"), aborted after the check.Tests
test/attach_verify.test.ts:verifyAttachTargetwith stubbed lookups (nonexistent → 404, PR → 422, closed → 422, open → verified, lookup throw → 502, app-not-installed → explicit unverified + lookup never attempted) andGitHubClient.getIssuewith a stubbed GitHub fetch (open, PR marker, 404 → null) — 9/9 pass on laptop.Need help on this PR? Tag
@codesmith-botwith what you need.