Enable automatic finding resolution - #33
Closed
ether-moon wants to merge 7 commits into
Closed
Conversation
Publish the current branch independently of base-branch lag and reconcile resolver state in place without temporary worktrees or branch switching.
Record the merged base ancestry after PR #30 so the new pull request contains only the shipping-pr policy changes.
Keep the authorization fixture's Markdown literals while avoiding SC2016 in the deterministic-validation workflow.
📝 WalkthroughWalkthroughThe PR updates autofixing to apply obvious and selected fixes after ambiguity decisions. It also changes shipping-pr resolver execution, state tracking, publication, evaluations, and tests to use the current PR worktree. ChangesAutomatic fixing workflow
Current-worktree shipping
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ShippingPR
participant ResolverAgent
participant SkillSetPR
ShippingPR->>SkillSetPR: record current worktree and workspace mode
SkillSetPR->>ResolverAgent: run resolver in the recorded branch
ResolverAgent->>SkillSetPR: return resolved, ambiguous, or stale result
SkillSetPR-->>ShippingPR: publish or poll for a fresh snapshot
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
plugins/skill-set/skills/autofixing-and-escalating/SKILL.md (1)
83-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse phase headings for the sequential workflow.
This section defines an ordered six-step process. Use
### N. Phaseheadings instead of a flat numbered list. This keeps the phases addressable and consistent across skills.Based on learnings, sequential process steps in this repository’s
SKILL.mdfiles should use the established### N. Phaseheading format.🤖 Prompt for 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. In `@plugins/skill-set/skills/autofixing-and-escalating/SKILL.md` around lines 83 - 86, Convert the six numbered workflow steps in the sequential process to distinct `### N. Phase` headings, preserving the existing Decide, Resolve, Verify, and Return content and ordering. Apply the established phase-heading format consistently across all six steps.Source: Learnings
🤖 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 `@plugins/skill-set/agents/resolving-pr-blockers.md`:
- Line 53: Update the Result contract and its validators to include the stale
outcome alongside success, no-op, AMBIGUOUS, and failed. Ensure strict state
consumers accept stale so resolving-to-polling recovery remains valid.
In
`@plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/case.yaml`:
- Around line 30-40: Strengthen the evaluation assertions across the three
sites: in
plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/case.yaml
lines 30-40, compare the complete output file against the original source; in
plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/graders/functional-contract.md
lines 15-16, require explicit OBVIOUS and queued records for both deferred
findings; and in
plugins/skill-set/evals/autofixing-and-escalating/mixed-pr-review-comments/case.yaml
lines 57-62, validate a real declaration, call site, and extracted behavior.
In
`@plugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/case.yaml`:
- Around line 9-15: Document the rationale for the execution limits beside
max_turns, timeout_seconds, and runs in both
plugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/case.yaml
lines 9-15 and
plugins/skill-set/evals/shipping-pr/current-worktree-resolution/case.yaml lines
9-15, using concise YAML comments that explain the turn budget, timeout
duration, and repetition count.
In
`@plugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/graders/functional-contract.md`:
- Line 9: Update the functional contract to require reinspection, exact-HEAD
fetching, and in-place reconciliation when the remote PR branch changes or is
non-fast-forward; do not treat routine remote-head changes as requiring user
intervention. Reserve waiting for the user only for AMBIGUOUS reconciliation
conflicts or unrecoverable external failures, while preserving the existing
preparation and base-conflict requirements.
In `@plugins/skill-set/skills/autofixing-and-escalating/reference/resolution.md`:
- Around line 44-49: Update the automatic resolution procedure around “Intersect
each proposed action with the bounded scope” to require every part of a proposed
action, including all files and hunks, to be within scope. Mark any partially or
fully out-of-scope action as out of scope and apply none of it; only complete
in-scope actions may proceed through the existing resolution and verification
steps.
In `@plugins/skill-set/skills/shipping-pr/scripts/skill-set-pr`:
- Around line 1250-1264: Before creating resolution_json in the
blocked-to-resolving transition, resolve the invoking checkout’s canonical
worktree root and currently checked-out branch with Git, canonicalize the
supplied worktree, and require both to match --worktree and --resolver-branch.
Keep validate_workspace_mode and the existing metadata validation, but reject
any current-mode request targeting another worktree or branch.
In `@plugins/skill-set/skills/shipping-pr/SKILL.md`:
- Line 4: Update the allowed-tools declaration in the skill configuration to
permit the skill-set-git runner needed by the inspect, commit, push, and
PR-creation workflows, while preserving the existing skill-set-pr and other tool
permissions.
In `@plugins/skill-set/tests/test-autofixing-contract.sh`:
- Around line 17-21: Update the contract checks in the
test-autofixing-contract.sh script to handle failures explicitly instead of
relying on set -e. Add a distinct fail message or assertion-helper invocation to
each grep check, including both repeated “without another confirmation” checks,
identifying the missing contract text.
In `@plugins/skill-set/tests/test-pr-runner.sh`:
- Around line 317-326: Update the legacy workspace migration setup around the
state-file rewrites to append explicit failure handling to every jq and mv
operation. Each failure message should identify whether the legacy-state removal
or temporary-workspace state update failed, while preserving the existing test
flow when the operations succeed.
---
Nitpick comments:
In `@plugins/skill-set/skills/autofixing-and-escalating/SKILL.md`:
- Around line 83-86: Convert the six numbered workflow steps in the sequential
process to distinct `### N. Phase` headings, preserving the existing Decide,
Resolve, Verify, and Return content and ordering. Apply the established
phase-heading format consistently across all six steps.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f12b7440-fd2c-4381-8d50-a8389db06b03
📒 Files selected for processing (32)
AGENTS.mdREADME.mdplugins/skill-set/agents/ci-failure-resolver.mdplugins/skill-set/agents/merge-conflict-resolver.mdplugins/skill-set/agents/pr-review-feedback.mdplugins/skill-set/agents/resolving-pr-blockers.mdplugins/skill-set/commands/pr/fix.mdplugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/case.yamlplugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/graders/functional-contract.mdplugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/prompt.mdplugins/skill-set/evals/autofixing-and-escalating/mixed-pr-review-comments/case.yamlplugins/skill-set/evals/autofixing-and-escalating/mixed-pr-review-comments/graders/functional-contract.mdplugins/skill-set/evals/autofixing-and-escalating/mixed-pr-review-comments/prompt.mdplugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/case.yamlplugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/graders/functional-contract.mdplugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/prompt.mdplugins/skill-set/evals/shipping-pr/current-worktree-resolution/case.yamlplugins/skill-set/evals/shipping-pr/current-worktree-resolution/graders/functional-contract.mdplugins/skill-set/evals/shipping-pr/current-worktree-resolution/prompt.mdplugins/skill-set/evals/shipping-pr/partial-resolver-publication-stop/prompt.mdplugins/skill-set/evals/shipping-pr/state-machine-happy-path/fixtures/run-shipping-evalplugins/skill-set/evals/shipping-pr/state-machine-happy-path/prompt.mdplugins/skill-set/skills/autofixing-and-escalating/SKILL.mdplugins/skill-set/skills/autofixing-and-escalating/reference/classification.mdplugins/skill-set/skills/autofixing-and-escalating/reference/resolution.mdplugins/skill-set/skills/shipping-pr/SKILL.mdplugins/skill-set/skills/shipping-pr/reference/blocker-resolution.mdplugins/skill-set/skills/shipping-pr/scripts/skill-set-prplugins/skill-set/tests/test-autofixing-contract.shplugins/skill-set/tests/test-pr-runner.shplugins/skill-set/tests/test-shipping-pr-authorization.shplugins/skill-set/tests/test-shipping-pr-happy-path-eval.sh
| A partial failure or AMBIGUOUS result must not invoke publication and must not publish the successful subset. Preserve the failure worktree and branch and report their exact paths, local commits, expected remote SHA, state publication phase, and recovery command. | ||
| A partial failure or AMBIGUOUS result must not invoke publication and must not publish the successful subset. Preserve the current worktree and branch and report their exact paths, local commits, expected remote SHA, state publication phase, and recovery command. | ||
|
|
||
| If the live PR HEAD changes before the publication gate succeeds, preserve the current branch and return resolver result `stale` through `resolving -> polling`. Take a fresh snapshot and reconcile automatically; do not turn routine staleness into `awaiting_user`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include stale in the result contract.
Line 53 introduces stale for automatic resolving -> polling recovery. The Result section at Line 59 permits only success, no-op, AMBIGUOUS, and failed. A strict state consumer can reject the documented stale result and stop recovery. Add stale to the result enum and update its validators.
🤖 Prompt for 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.
In `@plugins/skill-set/agents/resolving-pr-blockers.md` at line 53, Update the
Result contract and its validators to include the stale outcome alongside
success, no-op, AMBIGUOUS, and failed. Ensure strict state consumers accept
stale so resolving-to-polling recovery remains valid.
Comment on lines
+30
to
40
| name: obvious-typo-deferred | ||
| target: | ||
| source: file | ||
| path: outputs/src/user.js | ||
| pattern: "receives" | ||
| pattern: "recieves" | ||
| - type: regex | ||
| name: unused-import-removed | ||
| name: unused-import-deferred | ||
| target: | ||
| source: file | ||
| path: outputs/src/user.js | ||
| pattern: "require\\(['\"]path['\"]\\)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Strengthen the evaluation assertions.
These graders check isolated text fragments. They do not fully establish the required deferred state or the selected resolution.
plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/case.yaml#L30-L40: compare the complete output file with the original source.plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/graders/functional-contract.md#L15-L16: require explicitOBVIOUSandqueuedrecords for both deferred findings.plugins/skill-set/evals/autofixing-and-escalating/mixed-pr-review-comments/case.yaml#L57-L62: verify a real declaration, call site, and extracted behavior.
📍 Affects 3 files
plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/case.yaml#L30-L40(this comment)plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/graders/functional-contract.md#L15-L16plugins/skill-set/evals/autofixing-and-escalating/mixed-pr-review-comments/case.yaml#L57-L62
🤖 Prompt for 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.
In
`@plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/case.yaml`
around lines 30 - 40, Strengthen the evaluation assertions across the three
sites: in
plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/case.yaml
lines 30-40, compare the complete output file against the original source; in
plugins/skill-set/evals/autofixing-and-escalating/mixed-lint-output/graders/functional-contract.md
lines 15-16, require explicit OBVIOUS and queued records for both deferred
findings; and in
plugins/skill-set/evals/autofixing-and-escalating/mixed-pr-review-comments/case.yaml
lines 57-62, validate a real declaration, call site, and extracted behavior.
Comment on lines
+9
to
+15
| execution: | ||
| max_turns: 6 | ||
| timeout_seconds: 240 | ||
| allowed_tools: | ||
| - Skill | ||
| - Read | ||
| runs: 3 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the evaluation execution limits.
Both cases set max_turns: 6, timeout_seconds: 240, and runs: 3 without a rationale. Add concise YAML comments that explain the turn budget, timeout, and repetition count.
plugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/case.yaml#L9-L15: Document the rationale formax_turns,timeout_seconds, andruns.plugins/skill-set/evals/shipping-pr/current-worktree-resolution/case.yaml#L9-L15: Document the same execution-limit rationale.
As per coding guidelines, “Document the rationale for configuration values and avoid unexplained constants.”
📍 Affects 2 files
plugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/case.yaml#L9-L15(this comment)plugins/skill-set/evals/shipping-pr/current-worktree-resolution/case.yaml#L9-L15
🤖 Prompt for 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.
In
`@plugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/case.yaml`
around lines 9 - 15, Document the rationale for the execution limits beside
max_turns, timeout_seconds, and runs in both
plugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/case.yaml
lines 9-15 and
plugins/skill-set/evals/shipping-pr/current-worktree-resolution/case.yaml lines
9-15, using concise YAML comments that explain the turn budget, timeout
duration, and repetition count.
Source: Coding guidelines
| weight: 1 | ||
| --- | ||
|
|
||
| Pass only if the response says base-branch lag or divergence is not a preparation blocker, commits the complete shipping scope without another confirmation, and publishes the resulting current branch state through the normal expected-remote-SHA fast-forward path. It must defer the base merge conflict to the later PR snapshot and merge-conflict resolver cycle, while refusing preparation-time pull, merge, rebase, or force-push. It must distinguish a changed or non-fast-forward remote PR branch as a separate publication safety failure that requires reinspection or user intervention. Stopping merely because the feature branch is behind or diverged from `origin/main`, resolving the base conflict before publication, or claiming any tool call is a failure. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not allow routine remote-head changes to require user intervention.
The phrase “reinspection or user intervention” permits a response that stops when the remote PR branch changes. The workflow requires reinspection, exact-HEAD fetch, and in-place reconciliation. It waits for the user only when the reconciliation has an AMBIGUOUS conflict or an unrecoverable external failure.
Proposed fix
- It must distinguish a changed or non-fast-forward remote PR branch as a separate publication safety failure that requires reinspection or user intervention.
+ It must distinguish a changed or non-fast-forward remote PR branch as a separate publication safety condition that requires reinspection, exact-HEAD fetch, and in-place reconciliation. It may wait for the user only when reconciliation has an AMBIGUOUS conflict or an unrecoverable external failure.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Pass only if the response says base-branch lag or divergence is not a preparation blocker, commits the complete shipping scope without another confirmation, and publishes the resulting current branch state through the normal expected-remote-SHA fast-forward path. It must defer the base merge conflict to the later PR snapshot and merge-conflict resolver cycle, while refusing preparation-time pull, merge, rebase, or force-push. It must distinguish a changed or non-fast-forward remote PR branch as a separate publication safety failure that requires reinspection or user intervention. Stopping merely because the feature branch is behind or diverged from `origin/main`, resolving the base conflict before publication, or claiming any tool call is a failure. | |
| Pass only if the response says base-branch lag or divergence is not a preparation blocker, commits the complete shipping scope without another confirmation, and publishes the resulting current branch state through the normal expected-remote-SHA fast-forward path. It must defer the base merge conflict to the later PR snapshot and merge-conflict resolver cycle, while refusing preparation-time pull, merge, rebase, or force-push. It must distinguish a changed or non-fast-forward remote PR branch as a separate publication safety condition that requires reinspection, exact-HEAD fetch, and in-place reconciliation. It may wait for the user only when reconciliation has an AMBIGUOUS conflict or an unrecoverable external failure. Stopping merely because the feature branch is behind or diverged from `origin/main`, resolving the base conflict before publication, or claiming any tool call is a failure. |
🤖 Prompt for 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.
In
`@plugins/skill-set/evals/shipping-pr/behind-base-current-branch-publication/graders/functional-contract.md`
at line 9, Update the functional contract to require reinspection, exact-HEAD
fetching, and in-place reconciliation when the remote PR branch changes or is
non-fast-forward; do not treat routine remote-head changes as requiring user
intervention. Reserve waiting for the user only for AMBIGUOUS reconciliation
conflicts or unrecoverable external failures, while preserving the existing
preparation and base-conflict requirements.
Comment on lines
+44
to
+49
| ## Automatic Resolution | ||
|
|
||
| 1. Intersect each proposed action with the explicit scope. | ||
| 2. Check that every required capability is true. | ||
| 3. Apply OBVIOUS edits only when `edit: true`. | ||
| 4. Keep every AMBIGUOUS item at `awaiting-user` until its exact alternative is selected. | ||
| 5. Return the result to the caller after the permitted actions. | ||
| 1. Intersect each proposed action with the bounded scope. | ||
| 2. If there are no AMBIGUOUS items, apply every OBVIOUS fix immediately. | ||
| 3. If there are AMBIGUOUS items, wait until all decisions are complete, then apply every queued OBVIOUS fix and selected AMBIGUOUS resolution automatically without another confirmation. | ||
| 4. Verify the applied fixes and return the bounded result to the caller. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject partial out-of-scope resolutions.
“Intersect each proposed action with the bounded scope” can truncate a multi-file or multi-hunk resolution. That can leave an incomplete fix while reporting the selected resolution as applied.
Require the complete action to be inside scope. Otherwise, mark it out of scope and apply no part of it.
Proposed contract wording
-1. Intersect each proposed action with the bounded scope.
+1. Apply an action only when its complete affected-file and hunk set is within the bounded scope.
+ If any part is out of scope, mark the action failed as out of scope and apply no part.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Automatic Resolution | |
| 1. Intersect each proposed action with the explicit scope. | |
| 2. Check that every required capability is true. | |
| 3. Apply OBVIOUS edits only when `edit: true`. | |
| 4. Keep every AMBIGUOUS item at `awaiting-user` until its exact alternative is selected. | |
| 5. Return the result to the caller after the permitted actions. | |
| 1. Intersect each proposed action with the bounded scope. | |
| 2. If there are no AMBIGUOUS items, apply every OBVIOUS fix immediately. | |
| 3. If there are AMBIGUOUS items, wait until all decisions are complete, then apply every queued OBVIOUS fix and selected AMBIGUOUS resolution automatically without another confirmation. | |
| 4. Verify the applied fixes and return the bounded result to the caller. | |
| ## Automatic Resolution | |
| 1. Apply an action only when its complete affected-file and hunk set is within the bounded scope. | |
| If any part is out of scope, mark the action failed as out of scope and apply no part. | |
| 2. If there are no AMBIGUOUS items, apply every OBVIOUS fix immediately. | |
| 3. If there are AMBIGUOUS items, wait until all decisions are complete, then apply every queued OBVIOUS fix and selected AMBIGUOUS resolution automatically without another confirmation. | |
| 4. Verify the applied fixes and return the bounded result to the caller. |
🤖 Prompt for 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.
In `@plugins/skill-set/skills/autofixing-and-escalating/reference/resolution.md`
around lines 44 - 49, Update the automatic resolution procedure around
“Intersect each proposed action with the bounded scope” to require every part of
a proposed action, including all files and hunks, to be within scope. Mark any
partially or fully out-of-scope action as out of scope and apply none of it;
only complete in-scope actions may proceed through the existing resolution and
verification steps.
Comment on lines
+1250
to
1264
| -n $expected_remote_sha || -n $base_sha || -n $base_branch || -n $workspace_mode || \ | ||
| ${#resolver_agents[@]} -gt 0 ]]; then | ||
| has_resolution_flags=true | ||
| fi | ||
|
|
||
| local resolution_json=null | ||
| if [[ $from == blocked && $to == resolving ]]; then | ||
| [[ -n $worktree && -n $resolver_branch && -n $remote && -n $remote_branch && \ | ||
| -n $expected_remote_sha && -n $base_sha && -n $base_branch && ${#resolver_agents[@]} -gt 0 ]] || \ | ||
| -n $expected_remote_sha && -n $base_sha && -n $base_branch && -n $workspace_mode && \ | ||
| ${#resolver_agents[@]} -gt 0 ]] || \ | ||
| die resolution_metadata_required "Starting a resolver requires complete recovery and publication metadata." \ | ||
| "Pass --worktree, --resolver-branch, --remote, --remote-branch, --expected-remote-sha, --base-sha, --base-branch, and --resolver-agent." | ||
| "Pass --worktree, --resolver-branch, --remote, --remote-branch, --expected-remote-sha, --base-sha, --base-branch, --workspace-mode current, and --resolver-agent." | ||
| validate_workspace_mode --workspace-mode "$workspace_mode" | ||
| validate_absolute_path --worktree "$worktree" | ||
| validate_branch --resolver-branch "$resolver_branch" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify that the recorded worktree is the invoking checkout.
validate_workspace_mode only accepts the string current. It does not verify that --worktree is the caller worktree or that --resolver-branch is currently checked out there. A caller can record another worktree and branch as current, then dispatch resolvers and publish from that checkout.
Before creating resolution_json, resolve the caller worktree root and branch with Git. Canonicalize --worktree and require both values to match the caller checkout.
🤖 Prompt for 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.
In `@plugins/skill-set/skills/shipping-pr/scripts/skill-set-pr` around lines 1250
- 1264, Before creating resolution_json in the blocked-to-resolving transition,
resolve the invoking checkout’s canonical worktree root and currently
checked-out branch with Git, canonicalize the supplied worktree, and require
both to match --worktree and --resolver-branch. Keep validate_workspace_mode and
the existing metadata validation, but reject any current-mode request targeting
another worktree or branch.
| name: shipping-pr | ||
| description: Drives an existing or newly requested pull request through deterministic CI, review, and blocker-resolution cycles until it is verified clean or reaches a terminal stop. Use when the user asks to ship a PR, wait for CI and fix it, run PR autopilot, or keep resolving blockers until the PR is ready. | ||
| allowed-tools: "Bash(gh pr view:*) Bash(git fetch:*) Bash(git cat-file:*) Bash(git worktree:*) Bash(mktemp:*) Bash(sleep:*) Edit(//**/.git/skill-set/inputs/commit-message.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/commit-message.*/content) Edit(//**/.git/skill-set/inputs/pr-body.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/pr-body.*/content) Agent" | ||
| allowed-tools: "Bash(*skill-set-pr:*) Bash(gh pr view:*) Bash(git fetch:*) Bash(git cat-file:*) Bash(mktemp:*) Bash(sleep:*) Edit(//**/.git/skill-set/inputs/commit-message.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/commit-message.*/content) Edit(//**/.git/skill-set/inputs/pr-body.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/pr-body.*/content) Agent" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Authorize the Git runner.
Line 11 requires <git-runner> for inspect, commit, push, and PR creation. Line 4 permits skill-set-pr but does not permit skill-set-git. The tool policy will block required workflow actions.
Proposed fix
-allowed-tools: "Bash(*skill-set-pr:*) Bash(gh pr view:*) Bash(git fetch:*) Bash(git cat-file:*) Bash(mktemp:*) Bash(sleep:*) Edit(//**/.git/skill-set/inputs/commit-message.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/commit-message.*/content) Edit(//**/.git/skill-set/inputs/pr-body.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/pr-body.*/content) Agent"
+allowed-tools: "Bash(*skill-set-pr:*) Bash(*skill-set-git:*) Bash(gh pr view:*) Bash(git fetch:*) Bash(git cat-file:*) Bash(mktemp:*) Bash(sleep:*) Edit(//**/.git/skill-set/inputs/commit-message.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/commit-message.*/content) Edit(//**/.git/skill-set/inputs/pr-body.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/pr-body.*/content) Agent"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| allowed-tools: "Bash(*skill-set-pr:*) Bash(gh pr view:*) Bash(git fetch:*) Bash(git cat-file:*) Bash(mktemp:*) Bash(sleep:*) Edit(//**/.git/skill-set/inputs/commit-message.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/commit-message.*/content) Edit(//**/.git/skill-set/inputs/pr-body.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/pr-body.*/content) Agent" | |
| allowed-tools: "Bash(*skill-set-pr:*) Bash(*skill-set-git:*) Bash(gh pr view:*) Bash(git fetch:*) Bash(git cat-file:*) Bash(mktemp:*) Bash(sleep:*) Edit(//**/.git/skill-set/inputs/commit-message.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/commit-message.*/content) Edit(//**/.git/skill-set/inputs/pr-body.*/content) Edit(//**/.git/worktrees/*/skill-set/inputs/pr-body.*/content) Agent" |
🧰 Tools
🪛 SkillSpector (2.4.4)
[warning] 99: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
🤖 Prompt for 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.
In `@plugins/skill-set/skills/shipping-pr/SKILL.md` at line 4, Update the
allowed-tools declaration in the skill configuration to permit the skill-set-git
runner needed by the inspect, commit, push, and PR-creation workflows, while
preserving the existing skill-set-pr and other tool permissions.
Comment on lines
+17
to
+21
| grep -Eq 'Default to `edit: true`' "$skill" | ||
| grep -Eq 'pause before any mutation' "$skill" | ||
| grep -Eq 'after every required decision is complete, automatically apply all queued OBVIOUS fixes' "$skill" | ||
| grep -Eq 'without another confirmation' "$skill" | ||
| grep -Eq 'without another confirmation' "$resolution" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Report missing contract checks explicitly.
These grep -Eq commands rely on set -e. When one check fails, the script exits without identifying the missing contract. Add || fail "..." to each check or use an assertion helper.
As per coding guidelines, scripts must handle error conditions explicitly with helpful messages rather than failing silently.
🤖 Prompt for 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.
In `@plugins/skill-set/tests/test-autofixing-contract.sh` around lines 17 - 21,
Update the contract checks in the test-autofixing-contract.sh script to handle
failures explicitly instead of relying on set -e. Add a distinct fail message or
assertion-helper invocation to each grep check, including both repeated “without
another confirmation” checks, identifying the missing contract text.
Source: Coding guidelines
Comment on lines
+317
to
+326
| state_file=$(git -C "$repo" rev-parse --git-common-dir)/skill-set/shipping-pr/17.json | ||
| jq 'del(.resolution.workspace_mode)' "$repo/$state_file" >"$repo/$state_file.tmp" | ||
| mv "$repo/$state_file.tmp" "$repo/$state_file" | ||
| legacy_workspace_resume=$(run_ok init --pr 17 --repo owner/repo --resume) | ||
| assert_equals current "$(jq -r .resolution.workspace_mode <<<"$legacy_workspace_resume")" \ | ||
| "legacy current-worktree recovery" | ||
|
|
||
| jq '.resolution.workspace_mode = "temporary"' "$repo/$state_file" >"$repo/$state_file.tmp" | ||
| mv "$repo/$state_file.tmp" "$repo/$state_file" | ||
| invalid_workspace_resume=$(run_fail init --pr 17 --repo owner/repo --resume) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Report failures when rewriting fixture state.
Add || fail "..." handling to both jq and mv operations. If either operation fails, the test should identify the state migration setup that failed.
As per coding guidelines, “Scripts must handle error conditions explicitly with helpful messages rather than failing silently.”
🤖 Prompt for 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.
In `@plugins/skill-set/tests/test-pr-runner.sh` around lines 317 - 326, Update the
legacy workspace migration setup around the state-file rewrites to append
explicit failure handling to every jq and mv operation. Each failure message
should identify whether the legacy-state removal or temporary-workspace state
update failed, while preserving the existing test flow when the operations
succeed.
Source: Coding guidelines
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
1 participant
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
autofixing-and-escalating.Changes
Test Plan
plugins/skill-set/tests/test-autofixing-contract.shplugins/skill-set/tests/test-references.shplugins/skill-set/scripts/validate-evalsplugins/skill-set/scripts/validate-referencesplugins/skill-set/scripts/generate-inventory --checkgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation