From 1b283ed94bb677d6f4fad81f206b77f55510124e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 22 Jul 2026 00:49:50 -0400 Subject: [PATCH] fix(source-control): externalize hardcoded bot logins + branch-to-issue grammar Two independent posture fixes from #912, batched under one source-control bump (0.16.2 -> 0.16.3). W1 -- dependency-manager hold-merge login set is configurable. The babysit merge gate held only the built-in dependabot/renovate product bots (DEPENDENCY_MANAGER_LOGINS), so a non-dependabot/renovate dependency bot an operator runs slipped the cross-tier hold. is_dependency_author now also matches any login in the new babysit_extra_dependency_manager_logins userConfig, threaded as the --extra-dependency-manager-logins merge-wrapper flag (mirroring the existing --approver-bot-logins arg-threading through evaluate()). Logins normalize identically on both sides (casefold, strip app/ and [bot]). Ships empty -> unconfigured installs match the built-in set alone. Wired only to the merge gate (the one is_dependency_author call site), not the snapshot. W2 -- branch-to-issue grammar is configurable. parse-branch-issue.sh hardcoded the /- (and routine-issue-) convention, so a repo on a different scheme (e.g. Jira keys feature/PROJ-123-slug) silently failed to derive a Closes #N line. The script now accepts an ERE pattern positional (last capture group = issue id), passed from the new branch_issue_pattern userConfig at the /pull-request create call site; the built-in convention stays the default when unset (an unsubstituted ${user_config...} placeholder is treated as absent). CLAUDE_PLUGIN_OPTION_* reaches hook processes only, not skill-invoked scripts, so the value is passed as an arg rather than read from the environment. Docs synced: plugin.json userConfig (both keys), babysit SKILL.md config table, reference/feedback.md, source-control README config table, and create.md call site. Tests: is_dependency_author extra-login normalization cases + an evaluate()-level integration test that flips the dependency hold via the config (a pure-function test would pass even with broken wiring). Closes #917 Co-Authored-By: Claude Opus 4.8 --- .../source-control/.claude-plugin/plugin.json | 13 ++++- plugins/source-control/CHANGELOG.md | 20 +++++++ plugins/source-control/README.md | 2 + .../skills/babysit-prs/SKILL.md | 16 +++--- .../skills/babysit-prs/reference/feedback.md | 4 +- .../skills/babysit-prs/reference/safety.md | 16 ++++-- .../babysit-prs/scripts/babysit_classify.py | 32 ++++++++++- .../babysit-prs/scripts/babysit_merge.py | 22 +++++++- .../scripts/tests/test_babysit_feedback.py | 20 +++++++ .../scripts/tests/test_babysit_merge.py | 55 +++++++++++++++++++ .../skills/pull-request/SKILL.md | 2 + .../skills/pull-request/reference/create.md | 15 ++++- .../scripts/parse-branch-issue.sh | 39 ++++++++++--- .../scripts/parse-branch-issue.test.sh | 14 ++++- 14 files changed, 240 insertions(+), 30 deletions(-) diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index 08cc6d7c6..88146da85 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "source-control", - "version": "0.16.2", + "version": "0.16.3", "description": "Git and GitHub delivery workflow: /commit (Conventional Commits + Co-Authored-By trailer via safe heredoc mechanics), /pull-request (prep, create, CI monitoring, review-comment triage, merge, CI-log fetch), /babysit-prs (self-pacing fleet loop — safe by default; opt-in worker/autopilot tiers add gate-checked merge and thread resolution behind a deterministic Python engine), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention merged across its config layers and the babysit-prs config, or apply — interview the repo and write the convention config to a chosen layer), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep — never --abort). The commit-subject / PR-title convention is configurable via a source-control.md config written by a re-runnable setup skill, layered across a ~/.claude user-global file, the tracked team file, and a gitignored .claude/source-control.local.md personal overlay merged per key; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", @@ -102,6 +102,12 @@ "title": "Babysit extra bot logins", "description": "Additional logins to treat as bots when structural detection cannot identify them. Absent: structural detection only." }, + "babysit_extra_dependency_manager_logins": { + "type": "string", + "multiple": true, + "title": "Babysit extra dependency-manager logins", + "description": "Additional dependency-manager bot logins beyond the built-in dependabot/renovate set whose PRs the merge gate holds absent --allow-dependency, the same as the built-ins. Absent: built-in dependency-manager set only." + }, "babysit_approval_downgrade_logins": { "type": "string", "multiple": true, @@ -156,6 +162,11 @@ "description": "Abort a CI-log ZIP fetch larger than this", "default": 52428800, "min": 1 + }, + "branch_issue_pattern": { + "type": "string", + "title": "Branch-to-issue grammar (ERE)", + "description": "POSIX ERE for extracting the numeric GitHub issue number from the current branch name; the LAST capture group holds it and must resolve to digits (Closes #N honors only a numeric issue). Set this for a non-default branch scheme that places the number differently, e.g. '^[^/]+/([0-9]+)-' for 'alice/1234-slug' or '-([0-9]+)$' for 'feat/add-widget-1234'. Absent: the built-in '/-' (and routine-issue-) convention." } } } diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 893c10552..c4b36b12a 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,26 @@ All notable changes to the `source-control` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.16.3] + +### Changed + +- **Dependency-manager hold-merge login set is now configurable (`#917` W1).** The merge gate held + only the built-in `dependabot`/`renovate` product bots (`DEPENDENCY_MANAGER_LOGINS`); a + non-dependabot/renovate dependency bot an operator runs slipped the cross-tier hold. The gate now + also holds any login in the new `babysit_extra_dependency_manager_logins` userConfig (threaded as + the `--extra-dependency-manager-logins` merge-wrapper flag, matching the existing arg-threading of + `--approver-bot-logins`); logins are normalized on both sides (casefold, strip `app/` and `[bot]`). + Ships empty, so an unconfigured install matches the built-in set alone. +- **Branch-to-issue grammar is now configurable (`#917` W2).** `parse-branch-issue.sh` hardcoded the + `/-` (and `routine-issue-`) convention, so a repo that places the GitHub issue + number differently in its branch names silently failed to derive a `Closes #N` line. The script now + accepts an ERE `pattern` positional (last capture group = the numeric GitHub issue number, e.g. + `^[^/]+/([0-9]+)-` for `alice/1234-slug`), wired from the new `branch_issue_pattern` userConfig at + the `/pull-request create` call site. The placeholder is single-quoted there so an unset value + reaches the script as an inert literal (double-quoting a dotted `${…}` name is a Bash + `bad substitution`) and falls back to the built-in convention. + ## [0.16.2] ### Fixed diff --git a/plugins/source-control/README.md b/plugins/source-control/README.md index 861112e04..79c7364e9 100644 --- a/plugins/source-control/README.md +++ b/plugins/source-control/README.md @@ -163,6 +163,7 @@ repo's owner. | `babysit_review_gate_context` | string | review gate treated as absent | | `babysit_ci_gateway_context` | string | gateway check unused | | `babysit_extra_bot_logins` | string (multiple) | structural bot detection only | +| `babysit_extra_dependency_manager_logins` | string (multiple) | built-in dependabot/renovate dependency-manager set only | | `babysit_approval_downgrade_logins` | string (multiple) | an approval carrying blocking-looking prose is downgraded to ignored structurally (every bot); a named login instead surfaces its own as material. Real APPROVED-state reviews and plain clean approvals are ignored regardless. | | `babysit_skip_downgrade_logins` | string (multiple) | downgrade heuristic dormant | | `babysit_max_quiet_recheck_seconds` | number | 14400 | @@ -172,6 +173,7 @@ repo's owner. | `babysit_worktree_root` | directory | `worktrees/` under the plugin data dir | | `worktree_stale_days` | number | 14 (staleness threshold for `/worktree status`) | | `fetch_logs_max_bytes` | number | 52428800 (CI-log ZIP size cap for `fetch-logs`) | +| `branch_issue_pattern` | string | built-in `/-` (and `routine-issue-`) branch-to-issue grammar; set an ERE (last capture group = the numeric GitHub issue number) for a scheme that places the number differently, e.g. `^[^/]+/([0-9]+)-` (`alice/1234-slug`) or `-([0-9]+)$` (`feat/add-widget-1234`) | The commit-subject / PR-title convention is separate: run **`/source-control:setup`** to interview your repo and write the diff --git a/plugins/source-control/skills/babysit-prs/SKILL.md b/plugins/source-control/skills/babysit-prs/SKILL.md index 857f93bf5..0a98ba225 100644 --- a/plugins/source-control/skills/babysit-prs/SKILL.md +++ b/plugins/source-control/skills/babysit-prs/SKILL.md @@ -199,12 +199,13 @@ home in [reference/safety.md](reference/safety.md). Both fail closed without `-- gate refuses the merge; re-snapshot and reassess the new head instead of using `--allow-unpinned-head` — the wrapper rejects that flag outright, so no unattended unpinned merge exists. The pin is carried to GitHub's server-side match-head-commit guard. It refuses - a dependency-manager-authored PR absent `--allow-dependency`, refuses merge on an unprotected + a dependency-manager-authored PR absent `--allow-dependency` (held set: built-in dependabot/renovate + plus any `babysit_extra_dependency_manager_logins`, which you MUST append via + `--extra-dependency-manager-logins ""` when set — see safety.md's merge command forms — or + those extra bots are silently not held), refuses merge on an unprotected repo (zero required reviews and zero required contexts) for a non-self author absent `--allow-unprotected`, never uses `--admin`, and cannot resolve threads, reply, or - force-push. React to `blockers`; do not bypass the gate. A `ready:false` immediately - following a `ready:true` on the same expected head is often GitHub's own mergeability - recompute lag — re-run the read-only check once before treating it as a real block. + force-push. React to `blockers`; do not bypass the gate. A `ready:false` immediately following a `ready:true` on the same expected head is often GitHub's own mergeability recompute lag — re-run the read-only check once before treating it as a real block. - **Once ready, stop.** When the gate proves a PR ready (safe mode) or its merge is deferred to a human (Pinned-Command Degradation, [reference/safety.md](reference/safety.md)), report that @@ -223,8 +224,7 @@ home in [reference/safety.md](reference/safety.md). Both fail closed without `-- ([reference/orchestration.md](reference/orchestration.md)); that pre-push-outdated rule is agent discipline, not machine-enforced, so a thread a worker's own push merely displaced (`isOutdated` flipped while both comment pins still match) is still resolvable — the machine-enforced fix for - that displacement bypass is tracked in #571. - In autopilot pass `--resolve --include-human` for threads the agent has addressed; the + that displacement bypass is tracked in #571. In autopilot pass `--resolve --include-human` for threads the agent has addressed; the script still cannot merge, reply, or dismiss reviews. Never treat exit code 0 alone as proof a specific thread was resolved — always parse the per-thread JSON `action` field (`resolved` vs `skipped-*` / `refused-stale-pin` / `resolve-failed`) and the @@ -283,8 +283,7 @@ branch, make only clear branch-owned fixes, re-check the head SHA, push, clean u The values below substitute from this plugin's stored configuration when this skill loads. A surviving literal `${user_config.…}` placeholder means that key is unset — apply its documented unset behavior. Reference files use `` slots; fill every slot from -this block. Values reach scripts ONLY as explicit CLI flags (option environment variables never -reach skill-invoked scripts). Configuration selects targets and thresholds; it never widens tier authority. +this block. Values reach scripts ONLY as explicit CLI flags (option environment variables never reach skill-invoked scripts). Configuration selects targets and thresholds; it never widens tier authority. | Key | Value | Flag delivery | Unset behavior | | --- | --- | --- | --- | @@ -302,6 +301,7 @@ reach skill-invoked scripts). Configuration selects targets and thresholds; it n | `babysit_review_gate_context` | `${user_config.babysit_review_gate_context}` | `--review-gate-context` (snapshot) | gate treated as absent | | `babysit_ci_gateway_context` | `${user_config.babysit_ci_gateway_context}` | `--ci-gateway-context` (snapshot) | gateway check unused | | `babysit_extra_bot_logins` | `${user_config.babysit_extra_bot_logins}` | `--extra-bot-logins` (snapshot) | structural bot detection only | +| `babysit_extra_dependency_manager_logins` | `${user_config.babysit_extra_dependency_manager_logins}` | `--extra-dependency-manager-logins` (merge gate) | built-in dependabot/renovate dependency-manager set only | | `babysit_approval_downgrade_logins` | `${user_config.babysit_approval_downgrade_logins}` | `--approval-downgrade-logins` (snapshot) | an approval carrying blocking-looking prose is downgraded to ignored structurally (every bot); a named login instead surfaces its own as material. Real APPROVED-state reviews and plain clean approvals are ignored regardless. | | `babysit_skip_downgrade_logins` | `${user_config.babysit_skip_downgrade_logins}` | `--skip-downgrade-logins` (snapshot) | downgrade heuristic dormant | | `babysit_max_quiet_recheck_seconds` | `${user_config.babysit_max_quiet_recheck_seconds}` | `--max-quiet-recheck-seconds` (snapshot) | `14400` | diff --git a/plugins/source-control/skills/babysit-prs/reference/feedback.md b/plugins/source-control/skills/babysit-prs/reference/feedback.md index 52f91c81f..994f87c0a 100644 --- a/plugins/source-control/skills/babysit-prs/reference/feedback.md +++ b/plugins/source-control/skills/babysit-prs/reference/feedback.md @@ -95,7 +95,9 @@ actually is, not by hardcoding a bot's login: dependency version (a lockfile, a manifest version field, a pinned Action SHA/tag for a third-party action). Accepting the new version is a human policy call. Dependency-manager PRs are never merged autonomously in any tier — `SKILL.md` states the invariant, and the merge - wrapper enforces it mechanically (`safety.md`, Guarded Mutation Wrappers). + wrapper enforces it mechanically (`safety.md`, Guarded Mutation Wrappers). The wrapper matches a + built-in dependency-manager login set (dependabot/renovate); a non-dependabot/renovate dependency + bot an operator runs is added to that hold via `babysit_extra_dependency_manager_logins`. - **Policy/content-sync bot PR**: the diff is this repository receiving content it does not own back from an upstream repository, through a mechanism the repository itself declares (for example a sync manifest naming which local paths are managed from where). This is the reviewed diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index 6f11b2e15..72bc142f1 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -193,7 +193,11 @@ auto-mode safety classifier and blocks the call before the wrapper runs. - The merge CLI refuses a dependency-manager-authored PR absent `--allow-dependency`, and refuses to merge on an unprotected repository — zero required reviews AND zero required status contexts — when the PR author is not one of ``, absent `--allow-unprotected`. Both - overrides are human decisions, never passed autonomously. + overrides are human decisions, never passed autonomously. The held dependency-manager set is the + built-in dependabot/renovate bots plus, when `babysit_extra_dependency_manager_logins` is + configured (non-empty, not a literal unexpanded token), the logins appended via + `--extra-dependency-manager-logins ` — supply it on every merge + command below, exactly as `--method` is, or those extra bots are not held. - The merge wrapper's `--autopilot-merge-tier` flag layers the #476 tier criteria (issue-linked, lane-authored, no blocking label, a distinct-bot approval on the live head, no human blocking comment) onto the base gate. It is **fail-closed**: the umbrella flag refuses (exit `3`) unless @@ -234,13 +238,15 @@ it, and any later gate-off flip, is a separate announced operator step. never the four-flagless base command, which would ignore every tier criterion: ```text - bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" owner/repo#N --allowed-owners --self-logins @me, --merge --expected-head --autopilot-merge-tier --lane-logins --approver-bot-logins --block-labels + bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" owner/repo#N --allowed-owners --self-logins @me, --merge --expected-head --autopilot-merge-tier --lane-logins --approver-bot-logins --block-labels --extra-dependency-manager-logins ``` The umbrella `--autopilot-merge-tier` is fail-closed: it refuses (exit `3`) unless `--lane-logins`, `--approver-bot-logins`, and `--block-labels` are all supplied, and any of - those three without the umbrella is a usage error (exit `2`). Add `--method ` - when configured, exactly as for the base merge readiness gate above. + those three without the umbrella is a usage error (exit `2`). Add `--method ` and + `--extra-dependency-manager-logins ` when configured, exactly as + for the base merge readiness gate above (omit each when its value is empty or a literal + unexpanded token). - **Second-account approve mechanic.** The approving review the gate's distinct-bot criterion requires is submitted out-of-band by the agent — the gate only verifies one exists on the live @@ -325,7 +331,7 @@ narrow allow rule. For a merge: ```text -bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" owner/repo#42 --allowed-owners --merge --expected-head --method +bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" owner/repo#42 --allowed-owners --merge --expected-head --method --extra-dependency-manager-logins ``` When the autopilot merge tier is enabled, this degraded handoff carries the tier flags too: diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py index 2a18c1832..31e3bb775 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py @@ -241,10 +241,36 @@ def normalized_bot_login(item: dict[str, Any]) -> str: return author_login(item).casefold().removesuffix("[bot]") -def is_dependency_author(login: str) -> bool: - """Pure dependency-manager author test feeding the cross-tier hold-merge rule.""" +def normalize_dependency_login_set(logins: Any) -> frozenset[str]: + """Normalize dependency-manager logins the way `is_dependency_author` matches: + casefold, strip a leading `app/` and a trailing `[bot]`. + + Distinct from `normalize_login_set`, which does not strip the `app/` prefix a + GitHub App author carries -- the dependency test compares against that + stripped form, so a configured `app/foo` and a raw `foo` must collapse. + """ + return frozenset( + str(login).casefold().removeprefix("app/").removesuffix("[bot]") + for login in (logins or []) + if str(login).strip() + ) + + +def is_dependency_author(login: str, extra: Any = frozenset()) -> bool: + """Dependency-manager author test feeding the cross-tier hold-merge rule. + + Matches the built-in `DEPENDENCY_MANAGER_LOGINS` product bots plus any + caller-supplied `extra` logins -- a non-dependabot/renovate dependency bot an + operator runs, which should keep the same hold-merge protection. `extra` ships + empty, so an unconfigured caller matches the built-in set alone. Both sides are + normalized identically (casefold, strip `app/` prefix and `[bot]` suffix). + """ normalized = str(login or "").casefold().removeprefix("app/").removesuffix("[bot]") - return normalized in DEPENDENCY_MANAGER_LOGINS + if normalized in DEPENDENCY_MANAGER_LOGINS: + return True + if not extra: + return False + return normalized in normalize_dependency_login_set(extra) def body_text(item: dict[str, Any]) -> str: diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index d0c978f28..ebcef4e31 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -587,6 +587,7 @@ def evaluate( allow_dependency: bool, allow_unprotected: bool, tier: AutopilotMergeTierConfig | None = None, + extra_dependency_manager_logins: frozenset[str] = frozenset(), ) -> dict[str, Any]: owner = split_owner(repo) pr_data = gh_json( @@ -712,7 +713,12 @@ def evaluate( ) # A dependency-manager PR is held in every tier unless explicitly allowed: # its update should be reviewed, not auto-merged on a green gate alone. - if is_dependency_author(str(author_login or "")) and not allow_dependency: + if ( + is_dependency_author( + str(author_login or ""), extra_dependency_manager_logins + ) + and not allow_dependency + ): blockers.append( f"author {author_login!r} is a dependency manager " "-- held (pass --allow-dependency to override)" @@ -836,6 +842,15 @@ def main() -> int: action="store_true", help="permit merging a dependency-manager-authored PR (held by default)", ) + parser.add_argument( + "--extra-dependency-manager-logins", + default=None, + help=( + "comma-separated extra dependency-manager bot logins beyond the " + "built-in dependabot/renovate set; their PRs are held absent " + "--allow-dependency, same as the built-ins" + ), + ) parser.add_argument( "--allow-unprotected", action="store_true", @@ -1001,6 +1016,10 @@ def main() -> int: if token.strip().casefold() != "@me" ) + extra_dependency_manager_logins = frozenset( + parse_csv_set(args.extra_dependency_manager_logins) + ) + try: result = evaluate( repo, @@ -1011,6 +1030,7 @@ def main() -> int: args.allow_dependency, args.allow_unprotected, tier, + extra_dependency_manager_logins=extra_dependency_manager_logins, ) except (RuntimeError, ValueError, json.JSONDecodeError) as exc: # Surface any gh/parse failure as JSON rather than a traceback. diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_feedback.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_feedback.py index 4ab30ec90..4c7a2969f 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_feedback.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_feedback.py @@ -37,6 +37,26 @@ def test_humans_and_other_bots_are_not_dependency_authors(self) -> None: with self.subTest(login=login): self.assertFalse(fb.is_dependency_author(login)) + def test_extra_logins_are_recognized(self) -> None: + # A non-dependabot/renovate dependency bot an operator runs keeps the + # same hold-merge protection once named. Matching is normalization- + # insensitive on both sides (casefold, strip `app/` and `[bot]`). + extra = frozenset({"MyDepBot"}) + for login in ("mydepbot", "MyDepBot[bot]", "app/mydepbot"): + with self.subTest(login=login): + self.assertTrue(fb.is_dependency_author(login, extra)) + + def test_extra_login_config_form_is_normalized(self) -> None: + # The configured entry itself may carry `app/` / `[bot]`; it still matches + # the normalized incoming login. + self.assertTrue(fb.is_dependency_author("mydepbot", frozenset({"app/MyDepBot[bot]"}))) + + def test_unconfigured_extra_matches_builtin_set_only(self) -> None: + # Empty extra (the shipped default) never widens the built-in set. + self.assertFalse(fb.is_dependency_author("mydepbot")) + self.assertFalse(fb.is_dependency_author("mydepbot", frozenset())) + self.assertTrue(fb.is_dependency_author("dependabot", frozenset())) + class ActorKindTests(unittest.TestCase): def test_authoritative_bot_typename_wins_over_unknown_login(self) -> None: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py index 68d3c6375..98aab1bdc 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py @@ -651,5 +651,60 @@ def test_clean_and_prose_are_not_blocking(self) -> None: self.assertFalse(merge.approval_reports_blocking(body), body) +class DependencyHoldIntegrationTests(unittest.TestCase): + """The dependency-manager hold in `evaluate`'s base path, driven end-to-end + through the real evaluate() with gh seams stubbed -- a pure-function test of + `is_dependency_author` would pass even if the config never reached the call, + so this exercises the actual wiring: CLI-arg-shaped frozenset -> evaluate() + param -> the line-715 hold. + """ + + DEP_BOT = "acme-bot" + + def _evaluate(self, extra: frozenset[str]) -> dict[str, Any]: + pr = _pr(author={"login": self.DEP_BOT}, reviewDecision="APPROVED") + + def gh_json(args: list[str]) -> Any: + if args[:2] == ["pr", "view"]: + return pr + if args[0] == "api": + return RULES + raise AssertionError(f"unexpected gh_json call: {args}") + + with ( + mock.patch.object(merge, "gh_json", side_effect=gh_json), + mock.patch.object(merge, "fetch_review_threads", return_value=[]), + mock.patch.object( + merge, "fetch_pull_request_reviews", + return_value=[_approval("some-reviewer[bot]", HEAD)], + ), + mock.patch.object(merge, "fetch_issue_comments", return_value=[]), + mock.patch.object( + merge, "fetch_pull_request_review_comments", return_value=[], + ), + ): + return merge.evaluate( + "owner/repo", PR_NUMBER, HEAD, {"owner"}, frozenset(), + False, False, None, + extra_dependency_manager_logins=extra, + ) + + def _dependency_blockers(self, result: dict[str, Any]) -> list[str]: + return [b for b in result["blockers"] if "dependency manager" in b] + + def test_unconfigured_does_not_hold_a_custom_dep_bot(self) -> None: + # acme-bot is not a built-in dependency manager; absent config it is a + # normal author and the dependency hold does not fire. + result = self._evaluate(frozenset()) + self.assertEqual(self._dependency_blockers(result), []) + + def test_configured_extra_login_flips_the_hold(self) -> None: + # Naming acme-bot in the extra set makes the very same PR held. + result = self._evaluate(frozenset({self.DEP_BOT})) + blockers = self._dependency_blockers(result) + self.assertEqual(len(blockers), 1, result["blockers"]) + self.assertIn(self.DEP_BOT, blockers[0]) + + if __name__ == "__main__": unittest.main() diff --git a/plugins/source-control/skills/pull-request/SKILL.md b/plugins/source-control/skills/pull-request/SKILL.md index a8d1b6167..267d94d76 100644 --- a/plugins/source-control/skills/pull-request/SKILL.md +++ b/plugins/source-control/skills/pull-request/SKILL.md @@ -33,6 +33,8 @@ Consumer conventions come from the consuming project's own `CLAUDE.md`, `AGENTS. **PR-body attribution** (the `🤖 Generated with [Claude Code]…` line) resolves from the `pr_body_attribution` key across the same three `source-control.md` layers ([../../reference/config-resolution.md](../../reference/config-resolution.md)) — absent → the default line, `none` → omitted, any other value → that line. It is the PR-body analogue of `/commit`'s `trailer_policy` and gated separately; see [reference/create.md](reference/create.md) §2.4.1 for the assembly. +**Branch-to-issue grammar** — the effective `branch_issue_pattern` is `${user_config.branch_issue_pattern}` (this line is the substituted surface; when it still shows the literal `${user_config.branch_issue_pattern}` token the key is unset). When it holds a real ERE — a POSIX ERE whose LAST capture group is the numeric GitHub issue number, e.g. `^[^/]+/([0-9]+)-` — [reference/create.md](reference/create.md) §2.4.0 fills its `` slot with that value; unset uses the built-in `/-` (and `routine-issue-`) convention. + ## Emit checklist For PR lifecycle runs spanning 3+ phases, copy `${CLAUDE_PLUGIN_ROOT}/skills/pull-request/templates/checklist.md` into your project's working-notes location (or track it inline) and tick each `- [ ]` as the phase produces its output. Stateful surface; survives `/clear`. diff --git a/plugins/source-control/skills/pull-request/reference/create.md b/plugins/source-control/skills/pull-request/reference/create.md index 9c951f518..c2fd87b01 100644 --- a/plugins/source-control/skills/pull-request/reference/create.md +++ b/plugins/source-control/skills/pull-request/reference/create.md @@ -131,10 +131,23 @@ Stage specific files (never `git add -A`). Then invoke `/commit` (this plugin's ### 2.4.0 Resolve linked issue(s) -Before building PR body, parse branch for primary issue number and prompt for any additional closures. Keyword line is injected at top of body in §2.4.1. +Before building PR body, parse branch for the primary (numeric GitHub) issue number and prompt for any additional closures. Keyword line is injected at top of body in §2.4.1. + +By default the parser uses the built-in `/-` (and `routine-issue-`) convention: ```bash ISSUE_NUM=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/parse-branch-issue.sh" 2>/dev/null || true) +``` + +If SKILL.md's "Branch-to-issue grammar" surface shows a configured `branch_issue_pattern` (a real ERE, not the literal `${user_config…}` token — this reference file is Read raw, so the value is resolved there, never here), pass it as a **single-quoted** second positional; the empty first argument keeps the branch-name default (`git branch --show-current`). Single-quoting shields ERE metacharacters like the `$` end-anchor from the shell: + +```bash +ISSUE_NUM=$(bash "${CLAUDE_PLUGIN_ROOT}/skills/pull-request/scripts/parse-branch-issue.sh" "" '' 2>/dev/null || true) +``` + +Fill `` with the resolved ERE. Its last capture group must resolve to the numeric GitHub issue number (a non-numeric capture — e.g. a bare Jira key — is looked up below, found absent, and dropped to the no-closure path); configure a scheme that captures the number wherever it sits, e.g. `^[^/]+/([0-9]+)-` for `alice/1234-slug` or `-([0-9]+)$` for `feat/add-widget-1234`. + +```bash CLOSES_LINE="" if [[ -n "$ISSUE_NUM" ]]; then # Validate issue exists in current repo BEFORE shipping `Closes #N`. diff --git a/plugins/source-control/skills/pull-request/scripts/parse-branch-issue.sh b/plugins/source-control/skills/pull-request/scripts/parse-branch-issue.sh index 5b67f5d00..677055dc2 100755 --- a/plugins/source-control/skills/pull-request/scripts/parse-branch-issue.sh +++ b/plugins/source-control/skills/pull-request/scripts/parse-branch-issue.sh @@ -1,13 +1,24 @@ #!/usr/bin/env bash -# Parse an issue number from a branch name following the convention -# `/-` or the cloud-routine variant `/routine-issue--`. +# Parse the (numeric GitHub) issue number from a branch name. The default +# convention is `/-` (or the cloud-routine variant +# `/routine-issue--`), but the grammar is configurable so a +# consumer whose branches place the number differently — e.g. a username-scoped +# scheme `alice/1234-slug` or a trailing-number scheme `feat/add-widget-1234` — +# is not silently unparsable. # # Usage: -# parse-branch-issue.sh [branch-name] +# parse-branch-issue.sh [branch-name] [pattern] # -# With no arg, falls back to `git branch --show-current`. -# Prints the captured issue number on stdout and exits 0 on match. -# Exits 1 with no output if the branch lacks an issue number. +# With no branch arg, falls back to `git branch --show-current`. +# `pattern` is an ERE whose LAST capture group holds the issue id; it must +# resolve to the numeric GitHub issue number (the caller emits `Closes #`, +# which GitHub honors only for a numeric issue in this repo — a non-numeric +# capture is looked up, found absent, and dropped). When omitted, `pattern` +# falls back to the branch_issue_pattern userConfig +# (CLAUDE_PLUGIN_OPTION_BRANCH_ISSUE_PATTERN, when a caller exports it) and then +# the built-in default. +# Prints the captured issue id on stdout and exits 0 on match. +# Exits 1 with no output if the branch does not match. set -uo pipefail BRANCH="${1:-}" @@ -19,8 +30,20 @@ if [[ -z "$BRANCH" ]]; then exit 1 fi -if [[ "$BRANCH" =~ ^[a-z]+/(routine-issue-)?([0-9]+)- ]]; then - echo "${BASH_REMATCH[2]}" +# A surviving literal `${user_config...}` placeholder means the key is unset — +# treat it as absent so the default applies rather than a bogus pattern. +PATTERN="${2:-}" +# shellcheck disable=SC2016 # matching the literal placeholder text, not expanding it +[[ "$PATTERN" == *'${user_config'* ]] && PATTERN="" +[[ -n "$PATTERN" ]] || PATTERN="${CLAUDE_PLUGIN_OPTION_BRANCH_ISSUE_PATTERN:-}" +[[ -n "$PATTERN" ]] || PATTERN='^[a-z]+/(routine-issue-)?([0-9]+)-' + +if [[ "$BRANCH" =~ $PATTERN ]]; then + # Issue id = the last (rightmost) capture group, so the default pattern's + # optional leading group still resolves to the trailing number, and a custom + # single-group pattern resolves to its one group. + n=${#BASH_REMATCH[@]} + echo "${BASH_REMATCH[n - 1]}" exit 0 fi diff --git a/plugins/source-control/skills/pull-request/scripts/parse-branch-issue.test.sh b/plugins/source-control/skills/pull-request/scripts/parse-branch-issue.test.sh index bb7902ced..042cdea54 100755 --- a/plugins/source-control/skills/pull-request/scripts/parse-branch-issue.test.sh +++ b/plugins/source-control/skills/pull-request/scripts/parse-branch-issue.test.sh @@ -14,9 +14,9 @@ PASS=0 FAIL=0 run_test() { - local desc="$1" input="$2" expected_out="$3" expected_exit="$4" + local desc="$1" input="$2" expected_out="$3" expected_exit="$4" pattern="${5:-}" local actual_out actual_exit - actual_out=$(bash "$PARSER" "$input" 2>/dev/null) + actual_out=$(bash "$PARSER" "$input" "$pattern" 2>/dev/null) actual_exit=$? if [[ "$actual_out" == "$expected_out" && "$actual_exit" -eq "$expected_exit" ]]; then echo "PASS: $desc" @@ -38,6 +38,16 @@ run_test "feat/just-a-feature no number" "feat/just-a-feature" "" 1 run_test "worktree-foo-bar wrong prefix" "worktree-foo-bar" "" 1 run_test "cursor/abc-xyz cloud-agent no number" "cursor/abc-xyz" "" 1 +# Configurable grammar (W2): a consumer whose branches place the (numeric +# GitHub) issue number differently passes an ERE whose LAST capture group is +# that number. The downstream `Closes #N` needs a numeric GitHub issue id, so +# the capture must resolve to digits. +run_test "username-scoped scheme via custom pattern" "alice/1234-fix" "1234" 0 '^[^/]+/([0-9]+)-' +run_test "trailing-number scheme via custom pattern" "feat/add-widget-1234" "1234" 0 '-([0-9]+)$' +run_test "custom pattern, no match -> exit 1" "main" "" 1 '^[^/]+/([0-9]+)-' +# shellcheck disable=SC2016 # the placeholder is a literal test input, not an expansion +run_test "unsubstituted user_config placeholder falls back to default" "feat/42-x" "42" 0 '${user_config.branch_issue_pattern}' + echo echo "Results: ${PASS} passed, ${FAIL} failed" [[ $FAIL -eq 0 ]]