Phase 12c: Domain-aware /review-pr routing + domain label hook #102

Merged

forgejo_admin merged 1 commit from 101-domain-aware-review-pr-routing into main

2026-03-14 23:24:46 +00:00

forgejo_admin commented

2026-03-14 23:19:15 +00:00

Contributor

Summary

Replace the static /review-pr skill (which forked to a deprecated generalist QA agent) with a smart domain-aware router. The skill now reads the PR's parent issue domain label and spawns the correct specialized QA agent (frontend-qa, dev-qa, or devops-qa). Also adds a soft-warning hook that reminds users to include domain labels when creating issues.

Changes

skills/review-pr/SKILL.md -- Removed context: fork, agent: qa, and disable-model-invocation: true. Replaced with model-invoked 6-step routing instructions: parse argument, get PR, extract parent issue from Closes #N, read domain labels, map to QA agent, spawn with minimal prompt. Handles edge cases (missing Closes ref, no domain label, unrecognized label, multiple Closes refs).
hooks/check-domain-label.sh (new) -- PreToolUse hook on issue creation that warns (allow, not deny) when no domain: label is mentioned. Fail-open on all errors. Advisory only for docs/meta issues.
settings.json -- Wired check-domain-label.sh into the existing create_issue|create_issue_and_branch matcher.

Test Plan

Verified SKILL.md frontmatter parses correctly with no syntax errors
Verified forbidden frontmatter keys (disable-model-invocation, context, agent) are removed
Verified check-domain-label.sh is executable, passes bash syntax check
Verified hook warns on bodies without domain labels and passes silently on bodies with them
Verified settings.json is valid JSON and the hook is properly wired

Review Checklist

SKILL.md frontmatter valid (no syntax errors)
disable-model-invocation, context: fork, agent: qa removed
argument-hint preserved
Routing table covers all 3 domains: frontend, backend, devops
Edge cases handled: missing Closes ref, no domain label, unrecognized label
Hook is executable and fail-open
Hook emits allow not deny (soft warning)
settings.json wiring correct
No unrelated changes

Plan: plan-pal-e-agency
Forgejo issue: #101

Closes #101

## Summary Replace the static `/review-pr` skill (which forked to a deprecated generalist QA agent) with a smart domain-aware router. The skill now reads the PR's parent issue domain label and spawns the correct specialized QA agent (`frontend-qa`, `dev-qa`, or `devops-qa`). Also adds a soft-warning hook that reminds users to include domain labels when creating issues. ## Changes - **`skills/review-pr/SKILL.md`** -- Removed `context: fork`, `agent: qa`, and `disable-model-invocation: true`. Replaced with model-invoked 6-step routing instructions: parse argument, get PR, extract parent issue from `Closes #N`, read domain labels, map to QA agent, spawn with minimal prompt. Handles edge cases (missing Closes ref, no domain label, unrecognized label, multiple Closes refs). - **`hooks/check-domain-label.sh`** (new) -- PreToolUse hook on issue creation that warns (allow, not deny) when no `domain:` label is mentioned. Fail-open on all errors. Advisory only for docs/meta issues. - **`settings.json`** -- Wired `check-domain-label.sh` into the existing `create_issue|create_issue_and_branch` matcher. ## Test Plan - Verified SKILL.md frontmatter parses correctly with no syntax errors - Verified forbidden frontmatter keys (`disable-model-invocation`, `context`, `agent`) are removed - Verified `check-domain-label.sh` is executable, passes bash syntax check - Verified hook warns on bodies without domain labels and passes silently on bodies with them - Verified `settings.json` is valid JSON and the hook is properly wired ## Review Checklist - [x] SKILL.md frontmatter valid (no syntax errors) - [x] `disable-model-invocation`, `context: fork`, `agent: qa` removed - [x] `argument-hint` preserved - [x] Routing table covers all 3 domains: frontend, backend, devops - [x] Edge cases handled: missing Closes ref, no domain label, unrecognized label - [x] Hook is executable and fail-open - [x] Hook emits `allow` not `deny` (soft warning) - [x] settings.json wiring correct - [x] No unrelated changes ## Related - Plan: `plan-pal-e-agency` - Forgejo issue: #101 Closes #101

forgejo_admin added 1 commit

2026-03-14 23:19:15 +00:00

Add domain-aware /review-pr routing and domain label warning hook a51a7bbd49

Replace the static QA agent fork in /review-pr with a smart router that
reads the parent issue's domain label and spawns the correct specialized
QA agent (frontend-qa, dev-qa, or devops-qa). Add check-domain-label.sh
hook that soft-warns on issue creation without a domain label.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-14 23:21:38 +00:00

Author

Contributor

PR #102 Review

BLOCKERS

None.

NITS

BRE alternation in check-domain-label.sh line 27 -- inconsistent with repo convention. The hook uses grep -qi 'domain:\(frontend\|backend\|devops\)' which is BRE (Basic Regular Expression) alternation syntax. Every other hook in this repo that needs alternation uses ERE via -E flag (e.g., check-pr-template.sh uses grep -qiE, check-agent-spawn.sh uses grep -qE). While BRE works in a non-interactive #!/bin/bash subshell (aliases from the Arch grep->rg mapping do not apply), it is inconsistent. Recommend changing to: grep -qiE 'domain:(frontend|backend|devops)'.
list_issues pagination gap in SKILL.md Step 3. The instruction says "Call mcp__forgejo__list_issues with the same owner and repo. Find the issue matching the number from Step 2." The list_issues tool defaults to 20 results. If the parent issue is older or the repo has many open issues, it may not appear in the first page. Consider adding guidance to set limit higher (e.g., 50) or to filter by state "all" (since the issue might already be closed if the PR was submitted after work completed). The list_issues tool also does not support filtering by issue number directly, so this is a known limitation, but the instructions should at least set state: "all" and a reasonable limit.
SubagentStart context injection does not differentiate specialized QA agents. In hooks/inject-subagent-context.sh (not part of this diff, but relevant), all QA variants (qa|frontend-qa|dev-qa|devops-qa) get the same context string pointing to agent-qa. The specialized agents have their own pal-e-docs profiles (agent-dev-qa, agent-devops-qa, agent-frontend-qa) referenced in their agent .md files. The SubagentStart hook should ideally inject the correct profile slug per agent type. This is a pre-existing issue not introduced by this PR, but worth noting since this PR is the feature that makes these specializations matter for the first time.
SKILL.md Step 6 spawn prompt may not satisfy spawn gate. The spawn gate (check-agent-spawn.sh) requires QA agent prompts to match both an issue pattern (#[0-9]+|Issue|Forgejo issue) and a PR pattern (PR #[0-9]+|PR [0-9]+|Pull Request|pulls/[0-9]+). The template prompt in Step 6 is "Review PR #{pr_number} on {owner}/{repo}. Parent issue: #{issue_number}..." which includes PR #N and #{issue_number}. The PR #N matches the PR pattern, and #N matches the issue pattern. This should pass, but only if the interpolated values are actual numbers. Consider adding a note in Step 6 that the prompt MUST include both patterns to satisfy the spawn gate.

SOP COMPLIANCE

Branch named after issue: 101-domain-aware-review-pr-routing references issue #101
PR body follows template: Summary, Changes, Test Plan, Related sections all present
Related section references plan slug: plan-pal-e-agency
Closes #101 present in PR body
No secrets, .env files, or credentials committed
No unnecessary file changes: exactly 3 files changed, all directly related to the feature
Commit messages: not visible in diff, but PR title is descriptive

VERDICT: APPROVED

The three-file change is clean, well-scoped, and correctly wired. The SKILL.md routing logic is thorough with good edge case handling. The hook is properly fail-open (advisory only). The settings.json wiring is correct -- the new hook is appended to the existing create_issue|create_issue_and_branch matcher alongside the template check. All four nits are non-blocking improvements that can be addressed in a follow-up.

## PR #102 Review ### BLOCKERS None. ### NITS 1. **BRE alternation in `check-domain-label.sh` line 27 -- inconsistent with repo convention.** The hook uses `grep -qi 'domain:\(frontend\|backend\|devops\)'` which is BRE (Basic Regular Expression) alternation syntax. Every other hook in this repo that needs alternation uses ERE via `-E` flag (e.g., `check-pr-template.sh` uses `grep -qiE`, `check-agent-spawn.sh` uses `grep -qE`). While BRE works in a non-interactive `#!/bin/bash` subshell (aliases from the Arch `grep->rg` mapping do not apply), it is inconsistent. Recommend changing to: `grep -qiE 'domain:(frontend|backend|devops)'`. 2. **`list_issues` pagination gap in SKILL.md Step 3.** The instruction says "Call `mcp__forgejo__list_issues` with the same owner and repo. Find the issue matching the number from Step 2." The `list_issues` tool defaults to 20 results. If the parent issue is older or the repo has many open issues, it may not appear in the first page. Consider adding guidance to set `limit` higher (e.g., 50) or to filter by state `"all"` (since the issue might already be closed if the PR was submitted after work completed). The `list_issues` tool also does not support filtering by issue number directly, so this is a known limitation, but the instructions should at least set `state: "all"` and a reasonable `limit`. 3. **SubagentStart context injection does not differentiate specialized QA agents.** In `hooks/inject-subagent-context.sh` (not part of this diff, but relevant), all QA variants (`qa|frontend-qa|dev-qa|devops-qa`) get the same context string pointing to `agent-qa`. The specialized agents have their own pal-e-docs profiles (`agent-dev-qa`, `agent-devops-qa`, `agent-frontend-qa`) referenced in their agent `.md` files. The SubagentStart hook should ideally inject the correct profile slug per agent type. This is a pre-existing issue not introduced by this PR, but worth noting since this PR is the feature that makes these specializations matter for the first time. 4. **SKILL.md Step 6 spawn prompt may not satisfy spawn gate.** The spawn gate (`check-agent-spawn.sh`) requires QA agent prompts to match both an issue pattern (`#[0-9]+|Issue|Forgejo issue`) and a PR pattern (`PR #[0-9]+|PR [0-9]+|Pull Request|pulls/[0-9]+`). The template prompt in Step 6 is `"Review PR #{pr_number} on {owner}/{repo}. Parent issue: #{issue_number}..."` which includes `PR #N` and `#{issue_number}`. The `PR #N` matches the PR pattern, and `#N` matches the issue pattern. This should pass, but only if the interpolated values are actual numbers. Consider adding a note in Step 6 that the prompt MUST include both patterns to satisfy the spawn gate. ### SOP COMPLIANCE - [x] Branch named after issue: `101-domain-aware-review-pr-routing` references issue #101 - [x] PR body follows template: Summary, Changes, Test Plan, Related sections all present - [x] Related section references plan slug: `plan-pal-e-agency` - [x] `Closes #101` present in PR body - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes: exactly 3 files changed, all directly related to the feature - [x] Commit messages: not visible in diff, but PR title is descriptive ### VERDICT: APPROVED The three-file change is clean, well-scoped, and correctly wired. The SKILL.md routing logic is thorough with good edge case handling. The hook is properly fail-open (advisory only). The settings.json wiring is correct -- the new hook is appended to the existing `create_issue|create_issue_and_branch` matcher alongside the template check. All four nits are non-blocking improvements that can be addressed in a follow-up.