Repository navigation
Conversation
An early step, before setup-node and npm ci, makes the review step's own decision and, for a run that will review, adds 👀 to the /review comment and creates the Code review check run in progress, with a token minted for it alone and revoked before the review step starts. The publish step completes that check run; a final step completes it with the reason when there's no review: neutral 'Review budget used up' when no provider is available, failure with the error, or cancelled. Closes #20
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![ncode-review[bot]](https://avatars.githubusercontent.com/u/12114478?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 NCode Review at 932db29: 9 new findings on the changed lines.
Not on a changed line:
- 🟡 F5 Nit: Step outcomes passed as a positional comma-separated string
action.yml:104. CODE_REVIEW_STEP_OUTCOMES joins four outcomes with commas, and finish.ts reads them back by position. Nothing ties the order in action.yml to the destructuring in finish.ts. Fix: Pass one env var per step outcome, or a JSON object, so finish.ts reads each outcome by name. - 🟡 F9 Nit: GitHub request headers still built by hand outside github.ts
src/main.ts:69. readClosedIssues in main.ts and lastReviewedHead in comments.ts still build the accept and authorization headers themselves, while the new code uses the shared helpers in github.ts. Fix: Send these requests through a shared helper in github.ts, or accept the duplication on purpose.
🤖 Review completeVerdict: 💬 Comment — This PR adds an early acknowledgement step that reacts 👀 and opens an in-progress "Code review" check run before setup-node and npm ci, so the reviewer sees it within seconds. A finish step then always completes that check run: neutral when the budget is used up or the review is skipped, failure when it breaks, cancelled when the run is cancelled. The design holds together: shared code that needs no npm packages lives in checks.ts and github.ts, review decisions go through decideReview, and secrets are filtered out of status text. The main risks are that the acknowledgement token is not revoked if the ack script dies before it runs, and that provider.ts now depends on how check runs are worded. The rest is minor cleanup. 8 reviewers run — 8 findings (run details)
Skipped: api-compat, data-model, intent, performance, tests Findings
Trial: findings are posted as review threads and in the check run. $0.77 on Amazon Bedrock, 92s, at 997bd51. (how to interact) |
…mes by name - Move post() and decide() into src/acknowledgement.ts as functions of their api, env and fetch, and test the token revocation, the head check and the step outputs. - Move the review step's status record and the finish step's status into checks.ts (stoppedRecord, finishStatus) and test the secret fallbacks and the stopped=true output. - Pass each step outcome to the finish step in its own variable. - Share required() and take() through src/env.ts. - Map no provider to the budget status in main.ts, so checks.ts no longer loads provider.ts and config.ts. - Assert the exact app-token, publish and finish conditions in action.yml.
|
Autopilot: answering #74 (review) and the summary #74 (comment).
|
|
/review |
There was a problem hiding this comment.
🤖 NCode Review at 70ce898: 7 new findings on the changed lines.
…nt without a pull request before the fork rule stoppedBy in provider.ts maps no provider to a used-up budget that passes the job and any other error to a failure, and main's error handler uses it. decideReview now checks for a missing pull request before the skip rules, so such an event reads as having no pull request rather than as a fork.
|
Autopilot: Round 2 answered. No blocker or should-fix is left open; the remaining threads are nits, answered above. Recommend merging once you've resolved them. |
There was a problem hiding this comment.
🤖 NCode Review at 997bd51: 8 new findings on the changed lines.
# Conflicts: # src/publish.ts
…sh scripts load no npm package
|
Autopilot: merged
|
# Conflicts: # CHANGELOG.md # src/results.ts # src/review.ts
# Conflicts: # CHANGELOG.md
|
Autopilot: Round 3 answered: #74 (review) and the summary #74 (comment). The review-round limit (3) is reached. No verified blocker or should-fix is left open. F1 repeats round 2's F2 and still holds: the |
# Conflicts: # CHANGELOG.md # src/main.ts # src/provider.ts
… loads no config.ts
# Conflicts: # CHANGELOG.md
# Conflicts: # src/main.ts # src/review.ts
…les, so the acknowledgement and finish scripts load no npm package This branch's results.ts imported depthLabel from review.ts and creditsRanOutReason from provider.ts as values, which since main's acknowledgement (#74) made acknowledge.ts load the agent SDK through comments.ts and results.ts.
Within seconds, before setup-node and
npm ci, the action now reacts 👀 to a/reviewcomment and creates the "Code review" check runin_progress("Review started at ; it usually takes a few minutes", linked to the run). It does this for automatic reviews too. The early steps use the review step's own decision (decideReviewinsrc/command.ts, now shared withmain.ts). The acknowledgement's app token is minted from the existingapp-private-keyinput, used only bysrc/acknowledge.ts(plainnode, no npm packages) and revoked before the review step starts. It is never in the review step's env, andtest/acknowledge.test.tschecks the env blocks inaction.yml. The publish step updates that check run (PATCH) rather than posting a second one. A finalalways()step completes it when nothing was published:No consumer workflow change is needed: no new input or permission.
Decisions
nodebefore setup-node. The acknowledgement runs with the runner's ownnode, which needs TS type stripping (Node ≥ 22.18;ubuntu-latesthas 22.23). The tests pass on Node 22.23.3 here. If a runner's Node can't run it, those steps continue on error and the review runs as before.skip-token-revoke: true) and revokes it in afinally, so no app token is valid while the agents run. The publish and finish steps mint theirs after the review step, as before.chooseProvidermakes one 1-token Bedrock call before it relies on the key. Only HTTP 401/403 counts as refused; any other answer or a network error goes on with Bedrock as before.::warning::, and the check is neutral, so automation can carry on.Manual checks that need a live run (from the issue's Acceptance)
/reviewfrom a writer gets 👀 and an in-progress "Code review" check within about ten seconds, in a run whose model call later fails./reviewcomment, a fork pull request, and a/reviewfrom someone without write access get nothing.Tests:
npm run typecheckandnpm test(111 pass). The e2e tests were not run.Closes #20