chore: roll up PRs #1 + #2 + strip deployment refs into one reviewable branch#4
Closed
m0nk111-post wants to merge 1 commit into
Closed
chore: roll up PRs #1 + #2 + strip deployment refs into one reviewable branch#4m0nk111-post wants to merge 1 commit into
m0nk111-post wants to merge 1 commit into
Conversation
…te guard + deployment-neutral config Consolidates the work that was previously proposed as PRs #1, #2 and #3 into a single reviewable change. ## What this does 1. Steers the github-pr-review skill and the agent prompt at the GitHub Pull Request Reviews API (POST /repos/{owner}/{repo}/pulls/{n}/reviews with a comments[] array) instead of the issue-comments API. Inline threads, suggestion blocks, and one-click-apply now actually work. 2. Adds the agent-canvas-automation cron runner that watches one repo for a trigger label, forks a fresh OpenHands conversation per (PR, label_event_id), waits for the conversation to finish, parses a ###REVIEW_JSON### contract out of the final response, and posts the result as a single Pull Request Review. v2.7 and v2.8 are shipped together; the difference between them is the duplicate-review guard (see below). 3. Makes the duplicate-review guard robust: before posting a parsed payload, the script queries GitHub for an existing review by the bot user at the same commit_id. If one is found, the script closes the state without re-posting — that fixes the race where the agent posts via the GitHub MCP and the script also posts from the parsed JSON, producing two reviews with identical content. 4. Removes every hardcoded reference to the original test deployment from this repo. The cron runner is generic now: REPO is an empty string with a TODO pointing at /pr-reviewer:setup, the bot-user check is resolved at runtime via GET /user with the same token that posts the review, and the SKILL.md / README references are deployment-neutral. ## Files M plugins/pr-review/scripts/prompt.py M skills/github-pr-review/SKILL.md M skills/index.js A plugins/pr-review/agent-canvas-automation/README.md A plugins/pr-review/agent-canvas-automation/v2/README.md A plugins/pr-review/agent-canvas-automation/v2.7/main.py A plugins/pr-review/agent-canvas-automation/v2.8/main.py ## Behavioural impact - New env vars: none. - One extra GET /user per cron tick (cached thereafter). That's well within the 5000/hr PAT budget. - Until REPO is filled in by /pr-reviewer:setup, _verify_token_and_repo will fail with a 404 against an empty path — fail-fast by design. ## Verification - python3 -c 'import ast; ast.parse(...)' on v2.7 + v2.8 main.py -> OK - grep -rnE 'm0nklabs|cryptotrader|m0nk111-post|pull/379|PR 379|PR #379' on the whole tree -> zero hits (the previous test-deployment literal that used to live in REPO = ... is gone; REPO is now empty). - node scripts/build-skills-catalog.mjs regenerated skills/index.js cleanly. Co-authored-by: openhands <openhands@all-hands.dev>
m0nk111-post
force-pushed
the
chore/clean-fork-off-upstream
branch
from
23ba23e to
1e1b54d
Compare
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
2 participants
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.
What
One commit. Replaces PRs #1, #2 and the (closed) #3 with a single reviewable change. Everything that was discussed in those PRs is now rolled into the body of this one commit.
###REVIEW_JSON###contract, the path-validation, the request-changes coalesce, and the duplicate-review guard.Branch setup
main(currently ate15748cf, identical toOpenHands/extensions/main).chore/clean-fork-off-upstream— 1 commit on top of base, +2431 / −93 across 7 files.mainwas force-reset toe15748cfto drop the three pre-existing commits. The closed PRs (fix(pr-review): post reviews via Pull Request Reviews API, not issue comments #1, feat(pr-review): add agent-canvas-automation v2.7+v2.8 with Reviews API and duplicate-review guard #2) are still reachable through GitHub's PR history if you ever want to see what they looked like as separate reviews.What the one commit does
1. Steer the skill and the prompt at the GitHub Pull Request Reviews API
Previously the github-pr-review skill and the agent prompt both documented the correct API (
POST /repos/{owner}/{repo}/pulls/{n}/reviewswith acomments[]array), but on real PRs the agent kept falling back togh pr comment(issue-level blob) and losing inline threads and suggestion blocks.The fix:
plugins/pr-review/scripts/prompt.py: a "How to Post the Review (CRITICAL — read first)" block right under the/github-pr-reviewtrigger. It includes a worked bash + JSON example, mandates the Reviews API, and explicitly forbidsgh pr comment/POST /issues/{n}/comments. The{owner}/{repo}/{n}tokens are escaped ({{...}}) so they survivestr.format().skills/github-pr-review/SKILL.md: rewritten to match thegithub-code-quality[bot]format —## <Priority> <Category>heading, one-line statement,---separator, "Best fix in this file" anchor, scope confirmation, optional```suggestion ```block. Pre-Review Checks, "One PR Review, One API Call, Many Inline Comments", Good vs Bad Inline Comments, edge cases, and a Summary Checklist.skills/index.js: regenerated from the updatedSKILL.md.2. Add the agent-canvas-automation cron runner (v2.7 + v2.8 shipped together)
A standalone OpenHands Automations script that, on a cron tick:
(PR, label_event_id)and feeds it the review prompt.finished/error/stuck/idle).###REVIEW_JSON###contract out of the final response.Cross-cutting features in both v2.7 and v2.8:
pathis not in the PR diff are dropped with a log line (GitHub returns 422 otherwise).REQUEST_CHANGESis auto-downgraded toCOMMENTwhen the bot user is the PR author (GitHub returns 422 otherwise).(PR, label_event_id)pairs are skipped on the next cron tick.GET /userwith the same token that posts the review) and closes the state without double-posting.###REVIEW_JSON###parser: brace-counting parser with a last-marker-wins rule, handles both fenced ```json` and raw inline JSON.What v2.8 adds on top of v2.7: the duplicate-review guard runs in both the "no JSON" and "have JSON" paths. Without it, a run where the agent posts via the GitHub MCP AND the script posts from the parsed JSON produces two reviews with identical content. With it, only the first one sticks.
3. Make the runner deployment-neutral
This is the bit that was discussed in PR #3 and ended up simpler than the original proposal.
/pr-reviewer:setupskill. Until REPO is filled in,_verify_token_and_repowill fail with a 404 against an empty path — fail-fast by design, and easier to spot than posting to the wrong repo. The setup skill substitutes this line as one of its deployment-constants step._get_bot_login(token)callsGET /userwith the sameGITHUB_TOKEN/GITHUB_PERSONAL_ACCESS_TOKENthat posts the review and caches the result for the rest of the cron tick. Works for both kinds of token with no extra config.What was decided but NOT done
/pr-reviewer:setupskill already substitutes it at build time — turning it into an env var would mean the setup skill has to substitute the substitution point too, with no upside.v2.9/main.pyfollow-up after this lands.Verification
python3 -c "import ast; ast.parse(open('.../v2.7/main.py').read())"-> OKpython3 -c "import ast; ast.parse(open('.../v2.8/main.py').read())"-> OKgrep -rnE "m0nklabs|cryptotrader|m0nk111-post|pull/379|PR 379|PR #379"on the whole tree -> zero hits. (Confirmed both against the working tree and againstgit diff upstream/main..HEAD.)git log --all -S m0nklabs -S m0nk111-post -S cryptotrader --diff-filter=A-> zero hits in any commit reachable from this branch (other than the upstream base).node scripts/build-skills-catalog.mjsregeneratedskills/index.jscleanly.git log upstream/main..HEAD-> 1 commit, linear, no merges.Reviewing this PR
There is exactly one commit. The commit message itself has a "What this does" section that mirrors the three headings above. Click into the diff and you can read the three layers in this order:
skills/github-pr-review/SKILL.mdandplugins/pr-review/scripts/prompt.py(the API switch).plugins/pr-review/agent-canvas-automation/v2.7/main.pyandv2.8/main.py(the cron runner — this is the bulk of the diff).plugins/pr-review/agent-canvas-automation/for the deploy-neutral doc updates.