fix: require formal kapi-agent approval before merge-ready#123
Closed
devkade wants to merge 2 commits into
Closed
Conversation
There was a problem hiding this comment.
kapi-agent review
Verdict: REQUEST_CHANGES
Blocking issues
src/application/github-run-contract-adapter.ts:84marks the kapi-agent review surface asreadywhen there is a formal current-head approval and successful check, but it does not include the draft PR condition that this PR documents as part of the approval gate indocs/kapi-agent-approval-gate.md:7-13. A draft PR with an approval/check success would therefore renderkapi-agent: formal current-head ... successas ready even though the documented gate says the PR is not kapi-agent approved unless it is not a draft. Please make the adapter review status stay pending for drafts and add a regression test for that case.
Warnings / risks
- The new ignored-comment path is covered when there is no formal review. If there is both a stale/requested formal review and an approval-shaped comment, the formal review still wins, which looks correct.
Suggestions
- Consider including
draft=truein the pending label when all review/check requirements pass but the PR is draft, so the report explains exactly why it is not merge-ready.
Looks good
- The CLI now fetches
commentsand distinguishes approval-shaped comments from formal PR reviews. recommendedActioncorrectly avoidsmerge-readyunless the formal review is current-head, the check is successful, and the PR is not draft.- Documentation clearly records the formal-review/current-head/check requirement and the PR #110 incident rule.
- Tests cover comment-only approval handling in both CLI report output and the run-contract adapter.
Verification notes
- Verifier gate: PASS —
npm ci && npm run verifyexited 0. - Size gate: PASS — 116 changed lines, under the 200-line limit.
- Revision-explanation status: not required for this first kapi-agent review; found true.
- Local inspection: reviewed the PR diff and relevant files, especially
src/cli/kapi-cli.ts,src/application/github-run-contract-adapter.ts,test/cli-args.test.ts,test/github-run-contract-adapter.test.ts,README.md,docs/kapi-agent-approval-gate.md, andskills/kapi-workflow/SKILL.md.
Engine: pi
There was a problem hiding this comment.
kapi-agent review
Verdict: REQUEST_CHANGES
Blocking issues
- Size gate failed: this PR has 423 changed lines, which exceeds the 200-line kapi-agent review limit. Please split this into multiple PRs, each under 200 changed lines and scoped to one semantic unit.
- Revision explanation gate failed: this PR was revised after a prior kapi-agent changes-requested review, but there is no author explanation of what changed and why. Please add a review response explaining the revision before requesting re-review.
Warnings / risks
- Because the size gate failed, I am not treating this as eligible for approval even if individual pieces look directionally correct.
- The prior blocking feedback concerned draft PR handling in
src/application/github-run-contract-adapter.ts. The next review needs a clear revision note stating whether and how that was addressed, including test coverage.
Suggestions
- Suggested split:
- PR 1: formal approval gate script/workflow only.
- PR 2: CLI/application contract adapter behavior and tests.
- PR 3: documentation updates for the approval contract.
- In the revision explanation, explicitly list:
- what files changed since the prior kapi-agent review;
- how the draft-PR blocking issue was fixed;
- which regression tests cover it;
- verification commands and results.
Looks good
- The documented intent is sound: approval-shaped comments should not count as formal merge approval.
- Requiring current-head formal
kapi-agentreview plus a successfulkapi-agent/reviewcheck is the right contract direction.
Verification notes
- Verifier gate status: FAIL.
- Size gate status: FAIL — 423 changed lines >= 200.
- Revision-explanation status: FAIL — required because this PR was updated after a prior kapi-agent review, but no author explanation was found.
- Prior review exists and requested changes; current head has not already been approved.
Engine: pi
Owner
Author
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
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
require formal kapi-agent approvalthat fails unless formal current-headkapi-agentPR review approval andkapi-agent/reviewsuccess are both presentdocs/kapi-agent-approval-gate.mdCloses #122.
Verification
npm test -- test/kapi-agent-formal-approval-gate.test.ts test/github-run-contract-adapter.test.ts test/cli-args.test.tsnpm run verifyNotes
This directly addresses the PR #110 incident at the GitHub layer:
## kapi-agent review/Verdict: APPROVEin an issue comment is context only. The repo should require bothkapi-agent/reviewandrequire formal kapi-agent approvalin branch protection/rulesets.