chore: strip deployment-specific refs from pr-review plugin#3
Closed
m0nk111 wants to merge 1 commit into
Closed
Conversation
This commit supersedes the previous commit on this branch (b5496a8). The previous one introduced two new environment variables (PR_REVIEW_TARGET_REPO, PR_REVIEW_BOT_LOGIN) that were both unnecessary: - PR_REVIEW_TARGET_REPO duplicates a value that the /pr-reviewer:setup skill's Step 6 already substitutes into REPO at build time. The tarball is intentionally deployment-specific; baking REPO in is by design. So REPO is back to being a hardcoded string at the top of v2.7/main.py and v2.8/main.py, with a comment explaining that the setup skill rewrites this line before packaging. - PR_REVIEW_BOT_LOGIN duplicates information that is already available from GITHUB_PERSONAL_ACCESS_TOKEN (the same token that posts the review). The dedup guard now resolves the bot user at runtime via a cached GET /user call (see _get_bot_login). That works for both GITHUB_TOKEN and GITHUB_PERSONAL_ACCESS_TOKEN, with no extra config. Other than that the commit covers the same ground as b5496a8: - skills/github-pr-review/SKILL.md: drop the reference URL to m0nklabs/cryptotrader/pull/379 and the 'Worked example (matches the format on PR #379)' header. - skills/index.js: regenerated from the fixed SKILL.md. - plugins/pr-review/agent-canvas-automation/README.md: drop the deployment name from the intro. - plugins/pr-review/agent-canvas-automation/v2/README.md: drop the 7 direct cryptotrader URLs from the demo table, the hardcoded automation ID in the deploy steps, and the m0nk111-post ref in the MCP-detection row. Co-authored-by: openhands <openhands@all-hands.dev>
m0nk111-post
force-pushed
the
chore/strip-cryptotrader-refs
branch
from
b5496a8 to
fdb735a
Compare
m0nk111-post
pushed a commit
that referenced
this pull request
Jun 17, 2026
…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>
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
3 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
Strip all deployment-specific references from the
pr-reviewplugin so thatm0nk111/extensionscan serve as a generic extensions registry without leaking a single test deployment's identity into prompts and logs.What changed since the previous version of this PR
The previous commit on this branch (b5496a8) introduced two new environment variables —
PR_REVIEW_TARGET_REPOandPR_REVIEW_BOT_LOGIN— that turned out to be unnecessary:PR_REVIEW_TARGET_REPOduplicates a value that the/pr-reviewer:setupskill already substitutes intoREPOat build time. The tarball is intentionally deployment-specific: the setup skill's Step 6 ("Apply exactly five constant substitutions near the top of the file") rewrites theREPOline to the operator's chosenowner/repobefore packaging and uploading the tarball. SoREPOis back to being a hardcoded string at the top ofv2.7/main.pyandv2.8/main.py, with a comment explaining that the setup skill rewrites this line.PR_REVIEW_BOT_LOGINduplicates information that is already available fromGITHUB_PERSONAL_ACCESS_TOKEN(the same token that posts the review). The dedup guard now resolves the bot user at runtime via a cachedGET /usercall (see_get_bot_login). That works for bothGITHUB_TOKENandGITHUB_PERSONAL_ACCESS_TOKEN, with no extra config.The README env-vars section that I added in the previous commit is gone for the same reason.
What changed in the previous version of this PR (and is still in this one)
skills/github-pr-review/SKILL.md: drop the reference URL tom0nklabs/cryptotrader/pull/379and the "Worked example (matches the format on PR #379)" header.skills/index.js: regenerated from the fixedSKILL.mdvianode scripts/build-skills-catalog.mjs.plugins/pr-review/agent-canvas-automation/README.md: drop the hardcoded deployment name from the intro.plugins/pr-review/agent-canvas-automation/v2/README.md: drop the 7 directm0nklabs/cryptotraderURLs from the demo table, the hardcoded automation ID in the deploy steps, and them0nk111-postreference in the MCP-detection row.plugins/pr-review/agent-canvas-automation/v2.7/main.pyandv2.8/main.py: replace all hardcoded"m0nk111-post"literals with a call to the new_get_bot_login(github_token)helper.REPOis hardcoded back to the deployment default with a comment explaining the/pr-reviewer:setupsubstitution contract.Behavioural impact
No new env vars. The duplicate-review guard now makes one
GET /userrequest on first call (cached thereafter for the rest of the cron tick), so each review cycle is one extra GitHub API call. That's within the 5000/hr PAT budget.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|pull/379|PR 379|PR #379|m0nk111-post"on the whole repo -> only the two intentionalREPO = "m0nklabs/cryptotrader"lines in v2.7/v2.8 main.pynode scripts/build-skills-catalog.mjsregeneratedskills/index.jscleanlyOut of scope (separate PRs)
v2.9/main.pyPR after this lands.