Repository navigation
chore: add maintainer issue fix skill - #4662
Conversation
|
/bigbonk review for issues |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
|
||
| ## 5. Validate | ||
|
|
||
| 1. Run the regression test and see it pass. Then prove it fails without the fix: commit first, write the pre-fix implementation files over the fix (`git show <base>:<path> > <path>`), run it, and restore them with `git checkout HEAD -- <path>`. Never use `git stash` for this; the stash is shared across worktrees. |
There was a problem hiding this comment.
This pre-fix proof can destroy unrelated work. If the user already has staged or unstaged edits in one of the implementation files, the redirection overwrites the worktree copy and git checkout HEAD -- <path> replaces both the index and worktree with HEAD, contrary to the skill's instruction to preserve unrelated changes. Run the proof in an isolated temporary worktree (or otherwise save and restore the exact index/worktree contents) instead.
| - Stage only the intended paths. Never `git add -A` without checking `git status` first. | ||
| - Never create `.changeset/*.md` files. Changesets are generated from the commit subject, so write a Conventional Commit: `fix(<area>): <behavior>`. Use `feat` only for new behavior. | ||
| - Write PR bodies and scratch files to unique temporary paths (`mktemp`), never fixed names. Other sessions share `/tmp`. | ||
| - Open the PR as a draft. Reference the issue with `Fixes #<number>`, unless it is a private or security report. |
There was a problem hiding this comment.
The preceding rules explicitly support private/security reports, but this still pushes a branch and opens an ordinary draft PR. In this public repository, draft PRs and their branches are public, so merely omitting Fixes #... can disclose the vulnerability and patch. Security reports need the private advisory workflow, or the skill must stop and hand off to a maintainer before pushing anything.
|
|
||
| Take `<sha>` from `git rev-parse HEAD` right after pushing, not from `gh pr view`, which lags. Never tell Bonk what changed, what to focus on or which findings to confirm. For transform or analysis PRs prone to edge-case churn, you may add that it should only flag issues affecting real-world usage. Never use `/bonk`, only `/bigbonk`. | ||
|
|
||
| 2. Before triggering, check `gh run list --workflow bigbonk.yml` for a run in progress. If one is, wait for its review first. |
There was a problem hiding this comment.
gh run list --workflow bigbonk.yml is repository-wide, so this tells the agent to wait whenever any other PR has a Big Bonk run. The workflow itself scopes concurrency per PR (.github/workflows/bigbonk.yml:9-11), so unrelated runs are safe and common. Identify a run for the current PR/trigger rather than treating the first in-progress repository run as a blocker.
|
|
||
| 2. Before triggering, check `gh run list --workflow bigbonk.yml` for a run in progress. If one is, wait for its review first. | ||
| 3. Watch CI in parallel. If CI goes red, fix it before the next round. | ||
| 4. Watch for the first new ask-bonk comment or new PR review posted after the trigger, and address it the moment it lands. Don't re-trigger a silent run before 10 minutes. If a run's log goes quiet for several minutes, cancel it and re-trigger. A posted verdict wins over a later "cancelled" notice. |
There was a problem hiding this comment.
This watcher misses clean re-reviews: Bonk updates its existing summary comment on subsequent rounds rather than necessarily creating a new comment or review. Such a run will look silent here and then be cancelled, which also contradicts the hard rule on line 27 that running reviews are never cancelled. Track the current workflow run and the summary comment's update instead of waiting only for a newly created item.
| 4. Watch for the first new ask-bonk comment or new PR review posted after the trigger, and address it the moment it lands. Don't re-trigger a silent run before 10 minutes. If a run's log goes quiet for several minutes, cancel it and re-trigger. A posted verdict wins over a later "cancelled" notice. | ||
| 5. Handle every finding as "Review Findings" below. Reply to each declined finding in the next trigger comment, then re-trigger on the new head right away. A decline doesn't end the loop. | ||
| 6. Continue until the latest review on the latest head approves or reports no actionable findings. There is no round cap while findings stay in scope. | ||
| 7. After a stack rebase, re-trigger only PRs whose own diff changed (compare `git patch-id --stable`). A rebased PR with an identical diff keeps its LGTM. |
There was a problem hiding this comment.
An unchanged child patch does not make an old approval valid after its base branch changes: the new parent code can change the behavior of the same child diff. This also conflicts with AGENTS.md, which requires /bigbonk after stacked-refactor rebases. Re-review each rebased child against its new base even when its patch-id is unchanged.
| - the CI workflow's checks on the latest head are green (fix Deploy Examples failures too; don't hold the PR in draft for one pending slow check once everything else is green and Bonk approved); | ||
| - the latest Big Bonk review on the latest head has no actionable findings. | ||
|
|
||
| clanky reviews a PR only once it is ready. Read its full review and inline comments (`gh api repos/cloudflare/vinext/pulls/<n>/reviews` and `/comments`), including INFO findings and "approved with comments", and handle them as "Review Findings". Once they are handled, comment `clanky review` to request a fresh review, and treat that as another round. |
There was a problem hiding this comment.
These API paths are hardcoded to cloudflare/vinext, although the Input section permits fixing an issue in another repository after locating its checkout, so that flow will inspect the wrong PR. Also, /comments is not relative to the preceding gh api endpoint and will not fetch review comments as written. Build both full repos/<owner>/<repo>/pulls/<n>/... endpoints from the parsed target and paginate them before deciding clanky is clean.
|
Review: 6 findings. Submitted 6 actionable inline comments on exact head |
|
/bigbonk review for issues Replies to the previous round:
|
|
|
||
| ## 5. Validate | ||
|
|
||
| 1. Run the regression test and see it pass. Then prove it fails without the fix: commit first, so the paths you swap have no uncommitted changes, then write the pre-fix implementation files over the fix (`git show <base>:<path> > <path>`), run it, and restore them with `git checkout HEAD -- <path>`. Never use `git stash` for this; the stash is shared across worktrees. |
There was a problem hiding this comment.
The pre-fix proof does not handle implementation files added or deleted by the fix. For an added path, shell redirection creates or truncates it before git show <base>:<path> fails, so there is no valid base-state run; for a deleted path, restoring with git checkout HEAD -- <path> fails because HEAD no longer contains it. The workflow can therefore stop mid-proof or record a failure caused by the swap itself rather than the reported bug. Handle path status explicitly (remove added files for the base run, recreate deleted files, and restore each state), and only run the test after every swap succeeds.
|
Since last review: 6 resolved, 0 still open, 1 new. Submitted 1 actionable inline comment on exact head |
|
/bigbonk review for issues Reply to the previous round:
|
|
|
||
| ## 5. Validate | ||
|
|
||
| 1. Run the regression test and see it pass. Then prove it fails without the fix: commit first, so the paths you swap have no uncommitted changes. Put each implementation file the fix touched back in its pre-fix state (modified or deleted ones with `git show <base>:<path> > <path>`, added ones removed), run the test, then restore them (`git checkout HEAD -- <path>` for modified and added files, removing files the fix deleted). Never use `git stash` for this; the stash is shared across worktrees. |
There was a problem hiding this comment.
Shell redirection still does not reconstruct the pre-fix Git tree entry. If deleting the implementation file also removed its now-empty parent directory, > <path> fails before git show runs; I reproduced this with a deleted removed/file.ts. More seriously, if a modified path is a symlink, the redirection follows the head symlink and overwrites its target instead of restoring the base symlink blob, potentially changing an unrelated file. Restore modified/deleted paths with a worktree-only Git operation that recreates directories and file types (for example, git restore --source=<base> --worktree -- <paths>), while keeping the explicit remove/restore handling for added/deleted paths.
|
Since last review: 0 resolved, 1 still open, 0 new. Submitted 1 actionable inline comment on exact head |
|
/bigbonk review for issues Reply to the previous round:
|
|
|
||
| ## 5. Validate | ||
|
|
||
| 1. Run the regression test and see it pass. Then prove it fails without the fix: commit first, so the paths you swap have no uncommitted changes. Put the implementation paths the fix touched back in their pre-fix state with `git restore --source=<base> --staged --worktree -- <paths>`, run the test, then restore them with `git restore --source=HEAD --staged --worktree -- <paths>`. Git handles added, deleted and symlinked paths both ways. Never use `git stash` for this; the stash is shared across worktrees. |
There was a problem hiding this comment.
Committing the fix does not guarantee these paths are clean: if an implementation file already contained an unrelated staged or unstaged hunk, staging only the intended fix leaves that hunk behind, and both whole-path restores erase it from the index and worktree. This conflicts with the earlier requirement to preserve unrelated changes. Before swapping, require every target path to be clean, or run the base-state proof in an isolated temporary worktree.
|
Since last review: 1 resolved, 0 still open, 1 new. Submitted 1 actionable inline comment on exact head |
|
/bigbonk review for issues Reply to the previous round:
|
Performance benchmarksCompared 0 improved · 0 regressed · 7 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
|
||
| ## 5. Validate | ||
|
|
||
| 1. Run the regression test and see it pass. Then prove it fails without the fix. Commit the fix, and check that the paths you swap have no other uncommitted changes, since the swap overwrites them. Put the implementation paths the fix touched back in their pre-fix state with `git restore --source=<base> --staged --worktree -- <paths>`, run the test, then restore them with `git restore --source=HEAD --staged --worktree -- <paths>`. Git handles added, deleted and symlinked paths both ways. Never use `git stash` for this; the stash is shared across worktrees. |
There was a problem hiding this comment.
The pre-fix test is intentionally expected to return nonzero, but restoration is only sequenced after it. If the agent runs the steps as a normal restore && test && restore chain or stops on the failed test command, every swapped path remains at <base> with the reverse diff staged. Require unconditional cleanup (capture the test status, restore HEAD, then evaluate the expected failure), or perform the proof in an isolated worktree.
| ## 3. Draft PR and Issue Reply | ||
|
|
||
| 1. Commit and push as `references/rules.md` describes, and open a draft PR against `main`. For stacked PRs, use `gh stack`. | ||
| 2. Start watching the PR's checks right away, with a PR-watching tool if you have one, otherwise `gh pr checks`. Don't end a turn with CI running and nothing watching it. |
There was a problem hiding this comment.
Bare gh pr checks does not watch: the CLI help confirms it prints once and exits with status 8 while checks are pending; --watch is the mode that refreshes until completion. As written, the fallback immediately leaves CI with nothing watching it, contrary to this step's requirement. Use gh pr checks <pr> --watch (and ideally identify the PR explicitly).
|
Since last review: 1 resolved, 0 still open, 2 new. Submitted 2 actionable inline comments on exact head |
|
/bigbonk review for issues Replies to the previous round:
|
| ## 3. Draft PR and Issue Reply | ||
|
|
||
| 1. Commit and push as `references/rules.md` describes, and open a draft PR against `main`. For stacked PRs, use `gh stack`. | ||
| 2. Start watching the PR's checks right away, with a PR-watching tool if you have one, otherwise `gh pr checks <n> --watch`. Don't end a turn with CI running and nothing watching it. |
There was a problem hiding this comment.
gh pr checks <n> --watch still exits immediately when this runs before Actions has registered its first check. In gh v2.103.3, checksRun calls populateStatusChecks before entering the watch loop, and an empty rollup returns no checks reported; therefore the instruction to start watching right away can still leave CI with no watcher. Retry until at least one check appears before invoking --watch, or watch the workflow run directly.
|
Since last review: 2 resolved, 0 still open, 1 new. Submitted 1 actionable inline comment on exact head |
|
/bigbonk review for issues Reply to the previous round:
|
|
Since last review: 1 resolved, 0 still open, 0 new. LGTM! |
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Approved — The change is documentation and agent configuration only. It is consistent with the repository's existing guidance, and I found no issues.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
openai.yaml sets allow_implicit_invocation: false, which narrows when the skill can run. The symlink is relative and stays inside the repository. The rules forbid running user-provided code and forbid unfreezing the lockfile.
📖 AGENTS.md: ✅ LGTM
The new skill guidance is specific to the skill and is correctly kept out of AGENTS.md.
📋 Code Quality: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No runtime code changed.
📝 Documentation: ✅ LGTM
The skill documents are internally consistent and match the repository's existing guidance.
Reviewed commit: 401f773a
|
Adds a maintainer-only issue fix skill, laid out like the triage skill from #4594. It takes a valid bug from reproduction to a review-ready pull request.
Layout
.agents/skills/fix/references/rules.md: the shared rules. They read on their own, so an automated fix run can use the file directly as guidance. They cover:.agents/skills/fix/SKILL.md: the interactive entry point (/fix <issue-url>in Claude Code,$fixin Codex). It covers live state throughgh, independent sub-agent review, the draft PR and issue reply, CI, the Big Bonk loop, clanky and marking ready. Everything else lives in the shared rules..agents/skills/fix/agents/openai.yaml: Codex runs the skill only when it's invoked explicitly..claude/skills/fix: a symlink to.agents/skills/fix, as for triage..agents/skills/triage/SKILL.md: "the fix step" now points at thefixskill instead ofAGENTS.md.Not offered to users
SKILL.mdsetsmetadata.internal: true, sonpx skills add cloudflare/vinextstill offers onlymigrate-to-vinext.Validation
npx skills@1.7.6 add . --liston this branch lists onlymigrate-to-vinext.vp fmt --checkpasses on the new and changed files.