Add PostToolUse hooks for automatic Forgejo label management (#51) #52
No reviewers
Labels
No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
ldraney/claude-custom!52
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "51-agent-label-behavior"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements PostToolUse hooks that automatically set Forgejo status labels on issues when agents use MCP tools (create_issue_and_branch, submit_pr, comment_on_pr). This enables sprint boards to reflect real workflow status without manual intervention.
Changes
hooks/forgejo-helper.sh-- Added 3 new functions:forgejo_get_issue_number_from_branch()extracts issue number from branch naming convention,forgejo_set_label()does GET+filter+PUT to replace status labels atomically,forgejo_comment_on_issue()posts comments on issues via APIhooks/label-on-branch.sh-- New PostToolUse hook forcreate_issue_and_branch: setsstatus:in-progresslabel on the new issuehooks/label-on-pr.sh-- New PostToolUse hook forsubmit_pr: setsstatus:qalabel on parent issue + comments PR URL on the issuehooks/label-on-verdict.sh-- New PostToolUse hook forcomment_on_pr: parses VERDICT from QA review comments, setsstatus:approvedorstatus:needs-fixon parent issue. No-op for non-review comments.settings.json-- Registered 3 new PostToolUse matcher entries (create_issue_and_branch, added to submit_pr, comment_on_pr)agents/dev.md-- Added "Automatic Label Hooks" awareness sectionagents/qa.md-- Added "Automatic Label Hooks" awareness section with VERDICT parsing detailsTest Plan
forgejo_set_label()correctly replaces status labels (GET + filter + PUT pattern)forgejo_get_issue_number_from_branch()handles branch pattern{issue-num}-{description}label-on-verdict.shonly fires on VERDICT lines, not arbitrary PR commentsbash -nsyntax checksettings.jsonvalidates as valid JSONReview Checklist
source forgejo-helper.sh,jqPostToolUse JSON output)settings.jsonvalid JSON after editsbash -nsyntax checkchmod +x)Related Notes
plan-2026-03-03-sprint-workflow-automation(traceability)PR #52 Review
BLOCKERS
remind-mcp-review-loop.shstill referencespal-e-docs-- The existing hook at/home/ldraney/.claude/hooks/remind-mcp-review-loop.sh(line 10) containsRef: Use mcp__pal-e-docs__get_note(slug="pr-review-loop") for details. This PR removespal-e-docsfrom both agentmcpServerslists but does not update this existing hook. After this PR, the reminder will instruct agents to call a tool they no longer have access to. This hook was not introduced by this PR, but the PR's scope explicitly removes pal-e-docs access, making this reference stale. Recommendation: Either updateremind-mcp-review-loop.shin this PR to remove the pal-e-docs reference, or file a follow-up issue and document it in the PR body.label-on-verdict.shVERDICT parsing could false-match -- The grep pattern on line 34 oflabel-on-verdict.shis:The
-iflag makes this case-insensitive. The first condition checks forAPPROVEDand then excludesNOT APPROVED. However, the pattern### VERDICT:.*APPROVEDwould also match strings like### VERDICT: CONDITIONALLY APPROVEDor### VERDICT: PRE-APPROVED. More importantly, the current QA output template uses exactly### VERDICT: APPROVED / NOT APPROVEDas a single line -- meaning if the QA agent outputs the template verbatim without choosing one (copy-paste error), it would match asNOT APPROVED(second branch), which is the safer failure mode. This is acceptable but worth noting. The real risk is that### Verdict: APPROVED(different heading level or casing) is intentionally handled by-i, but any text betweenVERDICT:andAPPROVEDwould also match. Recommendation: Consider tightening the regex to anchor more specifically, e.g.### VERDICT: APPROVED$to prevent unexpected matches.NITS
Unused variable
PR_NUMinlabel-on-pr.sh-- Line 31 assignsPR_NUMfrom.tool_response.numberbut this variable is never used anywhere in the script. It should either be removed or used (e.g., in the comment body posted to the issue).statusLinediff is cosmetic/encoding noise -- The diff shows changes to thestatusLinecommand insettings.jsonwhere Unicode escape sequences (\\u2500,\\u2022) are replaced with literal Unicode characters. This is functionally equivalent but represents an unrelated change that was likely caused by a JSON serialization difference. Not blocking, but it adds noise to the diff.Key reordering in
settings.json-- TheenabledPlugins,alwaysThinkingEnabled, andskipDangerousModePermissionPromptkeys were reordered at the bottom of the file. Functionally identical but contributes to diff noise.forgejo_get_issue_number_from_branchreturns empty string on failure via two paths -- The|| echo ""fallback on line 207 offorgejo-helper.shis correct, butgrep -oEwith no match exits non-zero. Combined withtrap 'exit 0' ERRin the calling hooks, theecho ""would not execute in hook context because the trap fires first. In practice this is fine because the callers all check for emptyISSUE_NUMand bail, but the|| echo ""in the helper function itself is slightly misleading since it implies it will always produce output.Agent doc changes remove useful SOP references -- The
dev.mddiff removes the SOPs table (worktree-workflow,solo-dev-pr-workflow,pr-lifecycle,template-pr-body,template-issue) and the Related section. The new version is more concise, which is good, but the PR body template is now inlined rather than referencing a canonical source. This is a design choice aligned with the "agents don't access pal-e-docs" model, so it's reasonable, but it means template updates require editing the agent file directly.qa.mddiff removes theblock-docs-writes.shPreToolUse hook from frontmatter -- The diff removes the pal-e-docs write-blocking hook matcher fromqa.mdfrontmatter (lines 15-18 of the old file). This is correct since QA no longer haspal-e-docsin itsmcpServers, so the hook was redundant. Good cleanup.SOP COMPLIANCE
51-agent-label-behavior, matching issue #51plan-2026-03-03-sprint-workflow-automationreferenced in Related Notes.envfiles, credentials, or secrets in the diff. Hooks source credentials from~/secrets/at runtime, which is the established pattern.(#51)VERDICT: NOT APPROVED
Blocker #1 (stale pal-e-docs reference in
remind-mcp-review-loop.sh) needs resolution -- either fix it in this PR or document it as discovered scope with a follow-up issue. Blocker #2 (VERDICT regex precision) is lower severity but worth addressing before merge to prevent edge-case mislabeling in production.QA Fixes Applied (
6c61761)Addressed 3 of the QA findings:
BLOCKER #2 fixed: VERDICT regex tightened (
label-on-verdict.sh)grep -qi '### VERDICT:.*APPROVED'with anchoredgrep -q '^### VERDICT: APPROVED[[:space:]]*$'-iflag (case-insensitive) -- VERDICT line must be exact-case per QA template.*wildcard that allowed partial matches like "CONDITIONALLY APPROVED" or "PRE-APPROVED"&& !exclusion)NIT #1 fixed: Unused
PR_NUMremoved (label-on-pr.sh)PR_NUM=$(... .tool_response.number ...)assignment that was never referencedNIT #4 fixed:
|| echo ""fallback (forgejo-helper.sh)(echo ... | grep ...) 2>/dev/null || trueso that grep's non-zero exit on no-match does not trigger the caller's ERR trap before the fallback can runNot addressed (per scope boundary):
remind-mcp-review-loop.sh): Pre-existing discovered scope, not part of this PRPR #52 Re-Review (post-fix)
Previous review (comment #1271) found 2 blockers and 6 nits. This re-review verifies the fixes and checks for regressions.
Fix Verification
BLOCKER 2 (VERDICT regex in label-on-verdict.sh) -- FIXED
The regex is now properly anchored and case-exact:
^and$anchors prevent partial matches (e.g., "CONDITIONALLY APPROVED")[[:space:]]*is a nice touchConfirmed correct.
BLOCKER 1 (stale pal-e-docs ref in remind-mcp-review-loop.sh) -- SCOPED OUT
Accepted as pre-existing scope. The PR does not modify
remind-mcp-review-loop.sh. A TODO exists for follow-up. No objection.NIT: Unused PR_NUM in label-on-pr.sh -- FIXED
The
PR_NUMvariable assignment is no longer present in the file. Confirmed removed.NIT: grep fallback in forgejo_get_issue_number_from_branch -- FIXED
Now uses a subshell to isolate grep's exit code from the caller's ERR trap:
Confirmed correct.
BLOCKERS
None.
NITS
statusLine unicode escapes replaced with literal UTF-8 (
settings.jsonlines 194-195 in diff): The\u2500and\u2022escapes became literal---and*characters. This is likely a JSON formatter artifact from editing the file. Functionally equivalent assuming the terminal supports UTF-8 (which it does). Non-blocking, just noting it.Agent profile changes are broader than "hook awareness" (
dev.md,qa.md): The diff removespal-e-docsfrom mcpServers, removes PreToolUseblock-docs-writeshooks, replaces SOPs tables with Access tables, and simplifies the output sections. This is legitimate cleanup aligned with the three-agent model, but it extends beyond the issue #51 scope ("PostToolUse hooks + forgejo-helper extensions"). If these changes were intentional and coordinated, no objection. If accidental scope creep, consider splitting into a separate PR.SOP COMPLIANCE
51-agent-label-behavior-> issue #51)plan-2026-03-03-sprint-workflow-automation)~/secrets/)trap 'exit 0' ERR)VERDICT: APPROVED