fix(implement): match PR author by login in PAT mode - #108
Merged
Conversation
The post-pipeline PR verifier filtered on `pr.user.type === "Bot"`, which is correct under App installation tokens but wrong under `GITHUB_PERSONAL_ACCESS_TOKEN` mode where the bot authors PRs as the PAT owner (`type === "User"`). PAT-mode runs were marked `failed` with `"implement completed but no PR was found"` even when the PR was opened correctly — a false negative that breaks the `succeeded`-keyed orchestrator cascade for `bot:ship`. Branch on auth mode: PAT mode resolves the active token's login via `/user` and matches `pr.user.login`; App mode keeps the existing `type === "Bot"` filter (App tokens can't call `/user`). On `/user` failure, fall back to the bot-type filter rather than crashing. Adds 4 unit tests pinning both modes including the regression case. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a regression in the implement workflow’s “did the agent open a PR?” verifier when running under GITHUB_PERSONAL_ACCESS_TOKEN mode, where PRs are authored as a real user (not a bot account) and were previously being filtered out.
Changes:
- Add PAT-mode author matching by resolving the authenticated user’s login via
octokit.rest.users.getAuthenticated()and matchingpr.user.login. - Keep App-mode behavior by continuing to match PRs authored by bot accounts (
pr.user.type === "Bot"). - Add unit tests covering App vs PAT author matching behavior for the verifier.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/workflows/handlers/implement.ts |
Adds auth-mode-aware PR author matching and a PAT /user lookup helper. |
test/workflows/handlers/implement.test.ts |
Adds focused unit tests for the post-pipeline PR-open verifier across auth modes. |
📝 WalkthroughWalkthroughThis PR makes the implement workflow handler auth-mode-aware: in PAT mode it resolves the authenticated user's login and matches recent PRs by that login; in app-installation mode it continues to match bot-authored PRs. Tests and docs are added/updated. ChangesAuth-Mode PR Author Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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. Comment |
- Replace `typeof pat !== "string" || pat.length === 0` with `config.githubPersonalAccessToken === undefined`. The zod `nonEmptyOptionalString` preprocess already collapses empty/whitespace to undefined, and the simpler check matches the established idiom in app.ts/connection-handler.ts. - Add a 5th test covering the `/user` failure branch: PAT mode + rejected `getAuthenticated()` → fallback to Bot-type filter → User-type PR rejected. Lifts implement.ts line coverage to 90%. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Update `## PR detection` to describe the new auth-mode-aware filter introduced in this PR: App-token mode keeps the `type === 'Bot'` match; PAT mode resolves the active login via `/user` and matches `pr.user.login`. Documents the `/user`-failure fallback behaviour. Required by the FR-019 docs-sync guard which gates `src/workflows/` changes on a matching `docs/use/workflows/*.md` update. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot review on PR #108 flagged that the previous fallback path (when `octokit.rest.users.getAuthenticated()` failed in PAT mode, return null and use the bot-type filter) is unsafe: it could silently claim an unrelated bot PR (Dependabot, Renovate, …) that happened to be opened during the implement run's time window. Drop the catch in `resolveExpectedAuthorLogin`. App-mode still short-circuits with `null`. PAT-mode now lets `/user` errors bubble up to the handler's outer `try/catch`, which reports the run as `failed` — no chance of mis-claiming. Tests: rename the existing fallback test to assert "fails closed" and add Copilot's requested regression: PAT mode + /user fails + unrelated Dependabot PR present → still failed (must NOT match PR #999). Coverage: 93% (was 90%), 6 tests pass. Docs: `docs/use/workflows/implement.md` and the `findRecentOpenedPr` JSDoc updated to reflect the fail-closed semantic. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/workflows/handlers/implement.test.ts`:
- Around line 137-144: Add an assertion to the test "App mode: accepts a PR
authored by the App bot" to ensure the App-token path doesn't call the user
endpoint; specifically, after calling implementHandler(ctx) and before
finishing, assert that the mocked getAuthenticated (or getAuthenticatedUser)
function was not called (e.g., expect(getAuthenticated).not.toHaveBeenCalled()).
Locate the test using implementHandler and buildCtx and add the not-called
assertion against the getAuthenticated mock to pin the App-mode contract.
- Around line 167-174: The test only asserts result.status === "failed" which is
too weak; locate the implementHandler branch that rejects PRs from unrelated
bots (and the constant or message it returns for that case) and change the test
to assert the exact failure detail as well (e.g., expect(result).toMatchObject({
status: "failed", reason: <the exported constant or exact message used by
implementHandler> }) or assert the specific field name implementHandler uses
such as result.reason/result.error/result.message equals that expected value);
use buildCtx and implementHandler as the references to find the correct failure
token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f62523f4-4d5c-4892-8fbe-57d2415f6b30
📒 Files selected for processing (3)
docs/use/workflows/implement.mdsrc/workflows/handlers/implement.tstest/workflows/handlers/implement.test.ts
Address CodeRabbit nits on PR #108: - App-mode test now asserts `octokit.rest.users.getAuthenticated` was NOT called. Pins the App-token contract so a regression that unconditionally calls /user (which 403s on installation tokens) would fail this test. - "PAT mode: rejects a PR authored by an unrelated bot" now asserts the exact `reason` string ("implement completed but no PR was found"). Prevents an unrelated failure path from masking a regression in PR-matching logic. Pure test-only change; logic unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/workflows/handlers/implement.test.ts`:
- Around line 71-79: The test fixture currently builds prData with created_at
using now + (p.createdAtOffsetMs ?? 1000) which can produce future timestamps;
change the creation to use a past offset (subtract the offset from now) so
created_at = new Date(now - (p.createdAtOffsetMs ?? 1000)).toISOString() for
each PR in prs, ensuring prData.created_at is anchored in the past and keeps the
handler's `since` window behavior correct.
- Around line 182-215: The tests enforce "fail-closed" on users.getAuthenticated
errors but the PR requires PAT mode to fall back to bot-type filtering; update
the two tests that mock ctx.octokit.rest.users.getAuthenticated to reject so
they no longer assert result.status === "failed". Instead assert the handler
preserved the original error text in result.reason but proceeded using bot-type
filtering: ensure the outcome is not a hard failure (result.status !== "failed")
and that no unrelated bot PRs are incorrectly claimed (for the second test
assert result.reason does not contain "999" unless the bot filter legitimately
matched). Reference implementHandler, ctx.octokit.rest.users.getAuthenticated,
mockConfig.githubPersonalAccessToken, and the PR numbers 107 and 999 when making
the assertion changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: efece2e7-143d-4c99-be59-c98423893171
📒 Files selected for processing (1)
test/workflows/handlers/implement.test.ts
CodeRabbit nit on PR #108: `buildCtx` was generating `created_at = now + offsetMs`, producing future PR timestamps that would break if the verifier ever enforced realistic time bounds (`created_at <= now`). Switch to `now - offsetMs`. The handler's window is `created >= since - 5s`; since the mocked pipeline resolves synchronously, `since` is captured a few ms after `now`, so a 1s past offset still satisfies the filter. Pure test-only, 6/6 pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
🎉 This PR is included in version 1.11.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/workflows/handlers/implement.tsfiltered onpr.user.type === "Bot". InGITHUB_PERSONAL_ACCESS_TOKENmode (introduced by feat(bot): PAT override + artifact sandbox + secret-exfil hardening #104) the bot authors PRs as the PAT owner withtype === "User", so every PAT-mode implement run was markedfailedwith"implement completed but no PR was found"even when the PR opened correctly.octokit.rest.users.getAuthenticated()and matchespr.user.login; App mode keeps thetype === "Bot"filter (App installation tokens can't call/user)./usererrors (no fallback to bot-type filter). Falling back is unsafe: an unrelated bot PR (Dependabot, Renovate) opened in the same run window would be silently claimed as ours. The outer handlertry/catchreports the run asfailed, surfacing the underlying error instate.failedReason.Why this matters
Issue #93's
implementrun onfeat/93-resolve-ci-recheck-gateopened PR #107 correctly, butworkflow_runswas written asfailedbecause of this false negative. Forbot:shiporchestration thesucceeded-keyed cascade now sees a green run as red, blocking iteration.Test plan
bun run typecheckcleanbunx eslinton changed files: 0 errors (2 pre-existing complexity/length warnings on the handler arrow, untouched)bun test test/workflows/handlers/implement.test.ts: 6/6 pass, 93% line coverage onimplement.ts/useris NOT called)/user503 (no extra PRs) → failed (fails closed)/user502 + Dependabot bot PR present → failed (must NOT claim PR #999)🤖 Generated with Claude Code