diff --git a/.changes/source-control/feat-10763-worker-resolve-c306d1f7.md b/.changes/source-control/feat-10763-worker-resolve-c306d1f7.md new file mode 100644 index 0000000000..1d2c1cf649 --- /dev/null +++ b/.changes/source-control/feat-10763-worker-resolve-c306d1f7.md @@ -0,0 +1,11 @@ +--- +bump: minor +--- + +### Added + +- `/source-control:babysit-prs` worker tier: the thread resolver takes `--own-pr-lane-logins`, filled from `babysit_lane_logins`. On a PR opened by your own login, `--autonomous` refuses the whole PR. With the flag, it resolves an outdated, bot-opened thread that a configured lane login other than the opener has answered with address-feedback's fixed disposition (`VALID, fixed`) as its latest reply; a left-open disposition is no answer. It still never resolves a CRITICAL or security thread, a thread a person opened, or a thread with a comment from your own login. Other threads report `skipped-own-pr-not-lane-answered`. With `babysit_lane_logins` unset, the lane login is the repository's `merge.agent-identity`, read at the PR's base commit and the default branch head, never the PR head; the setting wins when set. The resolve runs under your existing login, so no permission is added. + +### Fixed + +- The resolver's own-PR refusal never fired. The resolver read the PR author's login from the wrong level of the `gh pr view --json author` payload and always got an empty login, so `--autonomous` treated every PR as someone else's. On your own PRs it could resolve an outdated bot thread you had replied to yourself. It now reads the login correctly, and it refuses when the author cannot be read. With no lane login from either source, the worker now resolves nothing on your own PRs. diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index ae6e1f7a2e..b754082cb3 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -145,7 +145,7 @@ "babysit_lane_logins": { "type": "string", "title": "Babysit pipeline lane logins", - "description": "Author logins recognized as pipeline lanes for the autopilot merge tier's lane-authored criterion. Absent: the tier (when on) refuses fail-closed.", + "description": "Logins recognized as pipeline lanes: the autopilot merge tier's lane-authored criterion, and the worker tier's resolve of outdated bot threads a lane already answered on your own PRs. Absent: the tier (when on) refuses fail-closed, and the worker resolves nothing on your own PRs.", "multiple": true }, "babysit_approver_bot_logins": { diff --git a/plugins/source-control/README.md b/plugins/source-control/README.md index 005e973065..a21f21493c 100644 --- a/plugins/source-control/README.md +++ b/plugins/source-control/README.md @@ -582,7 +582,7 @@ reads it from. | `babysit_merge_method` | string | `"auto"` | `CLAUDE_PLUGIN_OPTION_BABYSIT_MERGE_METHOD` | Merge method for gate-proven merges. auto (default) uses the repository's declared method, then squash; squash, merge, or rebase forces that method when the repository declares none. | | `babysit_stacked_prs` | boolean | `false` | `CLAUDE_PLUGIN_OPTION_BABYSIT_STACKED_PRS` | Lets the merge gate merge a native stacked PR layer, which lands every open layer below it. Each of those layers must pass the same gate. Off by default: a stack layer is held for a human. | | `babysit_autopilot_merge_tier` | boolean | `false` | `CLAUDE_PLUGIN_OPTION_BABYSIT_AUTOPILOT_MERGE_TIER` | Turns on the autopilot merge tier: a distinct bot account submits an approving review, then the gate merges only when every criterion holds. Off by default; PRs go to the human merge-ready list. Requires babysit_lane_logins, babysit_approver_bot_logins, and babysit_merge_block_labels. | -| `babysit_lane_logins` | string (multiple) | *(none)* | `CLAUDE_PLUGIN_OPTION_BABYSIT_LANE_LOGINS` | Author logins recognized as pipeline lanes for the autopilot merge tier's lane-authored criterion. Absent: the tier (when on) refuses fail-closed. | +| `babysit_lane_logins` | string (multiple) | *(none)* | `CLAUDE_PLUGIN_OPTION_BABYSIT_LANE_LOGINS` | Logins recognized as pipeline lanes: the autopilot merge tier's lane-authored criterion, and the worker tier's resolve of outdated bot threads a lane already answered on your own PRs. Absent: the tier (when on) refuses fail-closed, and the worker resolves nothing on your own PRs. | | `babysit_approver_bot_logins` | string (multiple) | *(none)* | `CLAUDE_PLUGIN_OPTION_BABYSIT_APPROVER_BOT_LOGINS` | Bot logins whose approving review satisfies the autopilot merge tier's author-is-not-approver criterion. Absent: the tier (when on) refuses fail-closed. | | `babysit_merge_block_labels` | string (multiple) | *(none)* | `CLAUDE_PLUGIN_OPTION_BABYSIT_MERGE_BLOCK_LABELS` | Labels that veto an autopilot-merge-tier merge, for example do-not-merge. Absent and undeclared in the target repository: the tier (when on) refuses fail-closed. | | `babysit_review_trigger_phrase` | string | *(none)* | `CLAUDE_PLUGIN_OPTION_BABYSIT_REVIEW_TRIGGER_PHRASE` | Comment phrase that requests an AI re-review (posted and recognized). Read only from this option: a target repository cannot supply it. Absent: the review-trigger module stays dormant. | diff --git a/plugins/source-control/reference/providers/github/babysit-prs/safety.md b/plugins/source-control/reference/providers/github/babysit-prs/safety.md index fc085afced..ee45895d86 100644 --- a/plugins/source-control/reference/providers/github/babysit-prs/safety.md +++ b/plugins/source-control/reference/providers/github/babysit-prs/safety.md @@ -58,3 +58,15 @@ release changes how `gh pr merge` treats a queue base, or the GraphQL schema cha ## Rerun a rate-limited review Rerun the whole workflow run with `gh run rerun `, never `gh run rerun --failed`. + +## Resolve identity on a person-opened PR + +The spoke's own-PR exception resolves a lane-answered thread under the PR author's login, and the +lanes App does not resolve threads on a PR a person opened. + +- **Pointer**: when deciding which identity can resolve a thread on a person-opened PR, fetch + [Resolving conversations](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#resolving-conversations) + live; the probe is [pr-unblock run 40737011221](https://github.com/melodic-software/claude-code-plugins/actions/runs/40737011221) + (`publish-feedback / run`, a lanes App resolve on a person-opened PR that failed). **As of**: + 2026-10-28. **Recheck trigger**: a lanes App resolve on a person-opened PR succeeds, or that + section changes who may resolve a conversation. diff --git a/plugins/source-control/skills/babysit-prs/SKILL.md b/plugins/source-control/skills/babysit-prs/SKILL.md index 2cdedba3fb..4b161c107e 100644 --- a/plugins/source-control/skills/babysit-prs/SKILL.md +++ b/plugins/source-control/skills/babysit-prs/SKILL.md @@ -47,7 +47,7 @@ An invocation is `[mode] [scope]`; mode and scope are orthogonal. Combine them f | Mode | What it does | | --- | --- | | *(none)*, the configured `default_tier` (`safe` unless the consumer changed it) | Safe tier: discover in-scope PRs (your own, under the current repo's owner by default), check readiness, fix clear branch-owned issues and push, report blockers and the next cadence. Never resolves threads or merges. | -| `worker` | Everything safe does, and additionally auto-resolves *pre-push-outdated* bot threads and merges PRs the merge gate proves 100% ready. Requires Python. For dedicated 24/7 loops over your own PRs. | +| `worker` | Everything safe does, and additionally auto-resolves *pre-push-outdated* bot threads and merges PRs the merge gate proves 100% ready. On a PR you opened, it resolves only outdated bot threads a configured lane login already answered, never a CRITICAL or security thread and never one a person wrote or joined. Requires Python. For dedicated 24/7 loops over your own PRs. | | `autopilot` | Maximum autonomy for a solo owner: every open PR under the watched owners regardless of author; fix everything it can; resolve every thread it has addressed (bot, AI-review, and human); merge everything the gate proves ready. Requires Python. A deliberate power-user opt-in, never a default. | | `help` (or `?`) | Print the common flows and the effective configuration below; take no other action. | @@ -291,7 +291,7 @@ this block. Values reach scripts ONLY as explicit CLI flags (option environment | `babysit_merge_method` | `${user_config.babysit_merge_method}` | deprecated fallback `--method` (merge wrapper) | `auto`: repo convention, then squash | | `babysit_stacked_prs` | `${user_config.babysit_stacked_prs}` | `--stacked-prs` (merge gate, every form) when `true`; omit it otherwise | `false` (a stack layer is held for a human) | | `babysit_autopilot_merge_tier` | `${user_config.babysit_autopilot_merge_tier}` | prose only. Gates whether the tier's `--autopilot-merge-tier` merge flags are wired at all | `false` (tier disabled; PRs go to the human merge-ready list) | -| `babysit_lane_logins` | `${user_config.babysit_lane_logins}` | `--lane-logins` (merge wrapper, autopilot merge tier) | tier refuses fail-closed when enabled | +| `babysit_lane_logins` | `${user_config.babysit_lane_logins}` | `--lane-logins` (merge wrapper, autopilot merge tier); `--own-pr-lane-logins` (resolve-thread, worker `--autonomous` forms) | tier refuses fail-closed when enabled; on your own PRs the resolver uses the repository's `merge.agent-identity` from its base and default branch, else resolves nothing | | `babysit_approver_bot_logins` | `${user_config.babysit_approver_bot_logins}` | `--approver-bot-logins` (merge wrapper, autopilot merge tier) | tier refuses fail-closed when enabled | | `babysit_merge_block_labels` | `${user_config.babysit_merge_block_labels}` | deprecated fallback `--block-labels` (merge wrapper, autopilot merge tier); omit it when unset | tier refuses fail-closed when enabled and the target repository declares none | | `babysit_review_trigger_phrase` | `${user_config.babysit_review_trigger_phrase}` | `--trigger-phrase` (snapshot, request_review); omit it when unset | review-trigger module dormant | diff --git a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md index 6111fd0e21..a98aeccd0a 100644 --- a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -72,6 +72,7 @@ Each row is executed. `Refused by` says which layer rejected the invocation. On | `resolve.independent-requires-disposition` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --independent-resolver` | 2 | python-cli | -- | `--disposition` | `babysit_resolve_thread.py::main` | --independent-resolver without --disposition is a usage error at exit 2: the mode resolves on validated evidence, and an unnamed disposition has no evidence to validate. | | `resolve.independent-requires-matching-evidence` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --independent-resolver --disposition deferred --thread-id PRRT_abc` | 2 | python-cli | -- | `--tracker-item` | `babysit_resolve_thread.py::main` | Each disposition is admissible only with ITS OWN evidence flag: --disposition deferred without --tracker-item refuses at exit 2 rather than resolving on an unproven claim. | | `resolve.evidence-flag-requires-the-mode` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --fix-commit abc1234` | 2 | python-cli | -- | `--independent-resolver` | `babysit_resolve_thread.py::main` | An evidence flag outside --independent-resolver is refused at exit 2, not accepted and ignored: silently dropping it would report an unchecked claim to the caller as a checked one. | +| `resolve.own-pr-lane-logins-requires-autonomous` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --own-pr-lane-logins lane` | 2 | python-cli | -- | `--autonomous` | `babysit_resolve_thread.py::main` | --own-pr-lane-logins is the worker tier's own-PR exception and nothing else: outside --autonomous it is refused at exit 2, so it can never widen the explicit or independent-resolver modes. | | `resolve.wrapper-reaches-failclosed-cli` | `scripts/source-control-babysit-resolve-thread` | `owner/repo#1` | 3 | python-cli | -- | -- | `babysit_resolve_thread.py::main` | The resolve wrapper is a pure passthrough: no --allowed-owners refuses at exit 3 from the CLI, unchanged by the wrapper. | | `resolve.wrapper-filters-nothing` | `scripts/source-control-babysit-resolve-thread` | `owner/repo#1 --allowed-owners owner --autonomous --resolve --thread-id PRRT_abc --allow-unpinned-thread` | 2 | python-cli | -- | `allow-unpinned-thread` | `babysit_resolve_thread.py::main` | ASYMMETRY: unlike the merge wrapper, the resolve wrapper filters no argument. Its --allow-unpinned-thread refusal comes from Python -- proven by the JSON envelope on stdout -- so an operator may not assume a bash-level refusal exists on this entry point just because one exists on the other. | | `ledger.apply-requires-lease-token` | `skills/babysit-prs/scripts/manage_feedback_ledger.py` | `dispose --pr owner/repo#1 --expected-head-sha 0123456789abcdef0123456789abcdef01234567 --feedback-id F1 --reason contract-row --state-dir {state_dir} --apply` | 2 | python-cli | -- | `lease token` | `manage_feedback_ledger.py::main` | A ledger write is lease-gated: --apply without --lease-token refuses at exit 2 rather than writing an unowned entry. | @@ -104,6 +105,9 @@ These decide per fetched thread, so no argument shape can express them and no ar | `classify.independent-refuses-a-multi-finding-thread` | isResolved=False, botOnly=True, isOutdated=True, humanDeferred=False, severityFlagged=False, findingCount=2 | autonomous=False, only_outdated=False, include_human=False, independent=True | `skipped-multi-finding-thread` | `babysit_resolve_thread.py::classify` | A permission rule reading '--independent-resolver resolves any addressed bot thread' is FALSE for a thread carrying more than one source finding: one --disposition is a claim about ONE finding while resolution clears the whole thread, so evidence for finding A would drop an unaddressed finding B out of the readiness denominator and let the merge gate pass over it. | | `classify.independent-refuses-an-unknown-finding-count` | isResolved=False, botOnly=True, isOutdated=True, humanDeferred=False, severityFlagged=False, findingCount=None | autonomous=False, only_outdated=False, include_human=False, independent=True | `skipped-multi-finding-thread` | `babysit_resolve_thread.py::classify` | The multi-finding guard fails CLOSED: the projection reports an unknown count (None) when a truncated comment page could be hiding another finding, and an unknown count refuses exactly as a known multi-finding count does. | | `classify.multi-finding-guard-is-independent-only` | isResolved=False, botOnly=True, isOutdated=True, humanDeferred=False, severityFlagged=False, findingCount=4 | autonomous=True, only_outdated=False, include_human=False | `eligible` | `babysit_resolve_thread.py::classify` | The multi-finding guard is scoped to --independent-resolver. --autonomous rests on isOutdated, which GitHub computes for the thread as a whole rather than per finding, so there is no per-finding claim there to under-cover: a multi-finding outdated bot thread stays eligible. | +| `classify.own-pr-requires-a-lane-answer` | isResolved=False, botOnly=True, isOutdated=True, humanDeferred=False, severityFlagged=False | autonomous=True, only_outdated=False, include_human=False, own_pr=True | `skipped-own-pr-not-lane-answered` | `babysit_resolve_thread.py::classify` | On a PR whose author is a self login, an outdated clean bot thread is still refused unless a lane login answered it: the resolver is the PR's author, so the 'handled' claim must come from another actor. An absent signal fails closed. | +| `classify.own-pr-keeps-the-severity-line` | isResolved=False, botOnly=True, isOutdated=True, humanDeferred=False, severityFlagged=True, laneAnswered=True | autonomous=True, only_outdated=False, include_human=False, own_pr=True | `skipped-severity-marked` | `babysit_resolve_thread.py::classify` | A lane answer never buys past the severity line: a CRITICAL or security thread on an own PR stays with a person. | +| `classify.own-pr-lane-answered-is-eligible` | isResolved=False, botOnly=True, isOutdated=True, humanDeferred=False, severityFlagged=False, laneAnswered=True | autonomous=True, only_outdated=False, include_human=False, own_pr=True | `eligible` | `babysit_resolve_thread.py::classify` | The own-PR exception admits exactly an outdated, bot-opened, unflagged thread a lane already answered. | | `classify.already-resolved-precedes-every-guard` | isResolved=True, botOnly=False, isOutdated=False, humanDeferred=False | autonomous=True, only_outdated=False, include_human=False | `skipped-already-resolved` | `babysit_resolve_thread.py::classify` | An already-resolved thread is skipped before any other guard is consulted, so a re-run never re-resolves and never reports a human thread it declined. | ## Effects: what reaches disk diff --git a/plugins/source-control/skills/babysit-prs/reference/orchestration.md b/plugins/source-control/skills/babysit-prs/reference/orchestration.md index 8dd7189e4b..573924fb9f 100644 --- a/plugins/source-control/skills/babysit-prs/reference/orchestration.md +++ b/plugins/source-control/skills/babysit-prs/reference/orchestration.md @@ -884,7 +884,7 @@ Each worker must: - **auto-resolve only pre-push-outdated threads.** A worker may resolve a review thread only when that thread was already `isOutdated` in the pre-push snapshot it was dispatched with, and only through `bash "/scripts/source-control-babysit-resolve-thread" owner/repo#42 --allowed-owners - --extra-bot-logins --self-logins @me, --autonomous --resolve` pinned with `--thread-id`, `--expected-comment-count`, and + --extra-bot-logins --self-logins @me, --own-pr-lane-logins --autonomous --resolve` pinned with `--thread-id`, `--expected-comment-count`, and `--expected-last-updated` taken from that same snapshot (`safety.md`, thread-pin pair rule). A thread that became outdated only because of the worker's own push has not thereby been addressed, so it is never auto-resolved on that basis. The push moving the diff under a finding does not @@ -977,7 +977,7 @@ GitHub settings, or auto-fix human-authored feedback. Classify, reply with evide surface human items instead. You may resolve a review thread only if it appears in the pre-push outdated-thread list above, via bash "/scripts/source-control-babysit-resolve-thread" owner/repo#42 --allowed-owners --extra-bot-logins --self-logins -@me, --autonomous --resolve --thread-id --expected-comment-count --expected-last-updated , with the pins +@me, --own-pr-lane-logins --autonomous --resolve --thread-id --expected-comment-count --expected-last-updated , with the pins taken from that list; a thread that becomes outdated only because of your own push is not addressed by that push. Leave it. If you address a finding on a CURRENT bot thread, whether with counter-evidence for an INCORRECT, a grounded diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index 6920f5995b..b9edcd32eb 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -598,6 +598,23 @@ auto-mode safety classifier and blocks the call before the wrapper runs. merely displaced (`isOutdated` flipped while both comment pins still match) is still resolvable by the script. Under `--resolve --include-human` the script still cannot merge, post replies, or dismiss reviews. +- **`--own-pr-lane-logins ` rides on every worker `--autonomous` resolve form** when + `babysit_lane_logins` is set; omit it when unset. Unset, the resolver takes the lane login from + the repository's `merge.agent-identity` in `docs/conventions/pr-pipeline.yaml`, only when the PR's + base commit and the default branch head declare the same one, never from the PR head: a person + merges that file, so a repository value that widens what the worker resolves stays reviewed + ([ADR 0039](https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/adr/0039-keep-babysit-identity-keys-in-userconfig-and-move-repository-keys-to-the-cascade.md)). The setting wins when set; with neither, the own-PR refusal holds. `--autonomous` refuses a PR whose author is a + self login, because the resolver would be the actor whose permission slip it is. This flag is + the one exception: on such a PR it admits a thread only when the latest reply from a lane login + other than the opener is address-feedback's fixed disposition (`LANE_FIXED_HEADER` in the + script), so the claim that the finding is handled comes from the lane. A left-open disposition + (`INCORRECT`, `UNCERTAIN`, `VALID (defer)`) is no answer. Every other + `--autonomous` guard still holds (bot opener, `isOutdated`, the severity line that keeps a + CRITICAL or security thread with a person, one pinned thread). A comment by a self login in the + thread refuses it too: on an own PR that login is the person who opened the PR. Any other thread + reports `skipped-own-pr-not-lane-answered`. The flag is refused outside `--autonomous`, and + `--independent-resolver` keeps the own-PR refusal. No permission is added: the resolve runs under + the PR author's own login ([GitHub](../../../reference/providers/github/babysit-prs/safety.md#resolve-identity-on-a-person-opened-pr)). - **`--independent-resolver` is a third mode, not a widening of `--autonomous`.** `--autonomous` admits only `isOutdated` threads, and `isOutdated` means the referenced code MOVED, so on a prose or documentation PR, where a finding is normally addressed by rewriting elsewhere in the @@ -1355,10 +1372,10 @@ assessment. Pin each vetted thread individually (the wrapper accepts exactly one per invocation; issue one pinned command per thread) with the thread-pin pair rule above: ```text -bash "/scripts/source-control-babysit-resolve-thread" owner/repo#42 --allowed-owners --extra-bot-logins --self-logins @me, --autonomous --resolve --thread-id --expected-comment-count --expected-last-updated +bash "/scripts/source-control-babysit-resolve-thread" owner/repo#42 --allowed-owners --extra-bot-logins --self-logins @me, --own-pr-lane-logins --autonomous --resolve --thread-id --expected-comment-count --expected-last-updated ``` -for the unattended-worker case, or +for the unattended-worker case (`--own-pr-lane-logins` omitted when `babysit_lane_logins` is unset), or ```text bash "/scripts/source-control-babysit-resolve-thread" owner/repo#42 --allowed-owners --extra-bot-logins --self-logins @me, --resolve --include-human --thread-id --expected-comment-count --expected-last-updated diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py index 0c5d5a3f47..744e15ab03 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py @@ -1488,10 +1488,13 @@ def discover_prs( def fetch_pull_request_author(repo: str, number: int) -> str: """The PR author's login from `gh pr view --json author`.""" repo = repo.casefold() - author = gh_json(["pr", "view", str(number), "--repo", repo, "--json", "author"]) - if not is_json_object(author): + payload = gh_json(["pr", "view", str(number), "--repo", repo, "--json", "author"]) + author = payload.get("author") if is_json_object(payload) else None + login = author.get("login") if is_json_object(author) else None + if not isinstance(login, str) or not login: + # Fail closed: an unread author cannot be shown not to be a self login. raise RuntimeError(f"Unexpected gh pr view author for {repo}#{number}") - return str(author.get("login") or "") + return login def view_pr_fields( diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index 174d525ebf..fe07702bc4 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -1203,6 +1203,32 @@ def with_default_branch( when it cannot be parsed or the agent identities differ.""" main = _default_branch_config(repo, branch) return base if main.base_sha == base.base_sha else stricter_config(base, main) +def lane_identity(repo: str, base_sha: str) -> tuple[str | None, str]: + """`merge.agent-identity` when the pull request's base commit and the + default branch head both declare the same one, with where it was read. + Never the head branch: a person merges `CONFIG_PATH`, so a value that + widens what the worker may resolve comes only from reviewed config. + None, with the reason, when either side is unset, they differ, or a read + fails, so the caller keeps its refusal.""" + try: + base = config_at(repo, base_sha) + if base.error: + return None, f"{CONFIG_PATH} at the base {base_sha}: {base.error}" + main = _default_branch_config(repo) + except PolicyReadError as exc: + return None, str(exc) + keys = {(c.agent_identity or "").strip().casefold() for c in (base, main)} + if "" in keys or len(keys) != 1: + return None, ( + f"merge.agent-identity is {base.agent_identity!r} at the base {base_sha} " + f"and {main.agent_identity!r} at the default branch head {main.base_sha}" + ) + return base.agent_identity, ( + f"merge.agent-identity in {CONFIG_PATH} at the base {base_sha} and the " + f"default branch head {main.base_sha}" + ) + + # --- Effective rung --------------------------------------------------------------- diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py index 50f29044d1..a63f2723f5 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py @@ -66,9 +66,22 @@ - `--autonomous` and `--independent-resolver` refuse when the PR author is a configured self login (`--self-logins`, including `@me`). Resolving threads on the caller's own PR is the same actor signing its own permission slip the - unattended guards exist to prevent; there is no autonomous override (mirroring - the merge gate's human-only `--allow-unprotected` pattern). Interactive modes - are unaffected. + unattended guards exist to prevent. Interactive modes are unaffected. +- `--own-pr-lane-logins` is the one narrow exception, for `--autonomous` only: + on a PR whose author is a self login, it admits a thread a pipeline lane has + already answered, so the claim that the finding is handled comes from the + lane, not from the resolver. Every other `--autonomous` guard still applies + (bot opener, `isOutdated`, the severity line, one pinned thread), and the + thread's latest reply from one of the named lane logins other than the + opener must be a fixed-disposition reply (`LANE_FIXED_HEADER`), with no + comment by a self login (on an own PR, the + self login is the person who opened it) and no truncated comment page. + Anything else is `skipped-own-pr-not-lane-answered`. Without the flag, or + with an empty value, the lane login is the repository's `merge.agent-identity` + when the PR's base commit and the default branch head both declare the same + one (`repo_lane_identity`), never read from the PR head; with neither, the + own-PR refusal above is unchanged. The flag wins when set. + `--independent-resolver` keeps the own-PR refusal with no exception. - `--independent-resolver` is a THIRD mode, parallel to `--autonomous` and not a relaxation of it. `--autonomous`'s `isOutdated` requirement is right about the merging worker, but `isOutdated` means "the referenced code moved" -- on a prose @@ -188,6 +201,7 @@ from babysit_classify import ( is_bot, is_self_login, + normalize_login_set, normalize_self_logins, severity_occurrences, strip_classification_rows, @@ -196,6 +210,7 @@ fetch_pull_request_author, fetch_review_threads, gh_capture, + gh_json, gh_http_status as http_status_from_text, is_owner_repo_pair, parse_repo_number, @@ -289,6 +304,13 @@ def append_resolve_thread_audit(record: dict[str, object]) -> None: ) +# The first line address-feedback's `renderReply` +# (.github/scripts/pr-unblock/publish-feedback.mjs) writes for a fixed finding. +# Its other dispositions leave the thread open, so only this one counts as an +# answer, and only on the lane's latest reply. +LANE_FIXED_HEADER = "**pr-unblock / address-feedback: VALID, fixed**" + + def _has_severity_marker(body: str) -> bool: """True for the forbidden class only: structured P0/P1 markers (badge, bracket, or declaration prefix), the word CRITICAL, or the word @@ -345,9 +367,18 @@ def project_thread( *, extra_bot_logins: frozenset[str] = frozenset(), self_logins: frozenset[str] = frozenset(), + lane_logins: frozenset[str] = frozenset(), ) -> dict[str, object]: """Map one shared-paginator record to this CLI's thread shape. + `laneAnswered` is the own-PR exception's signal (`--own-pr-lane-logins`): + every comment is a bot's (a self login counts against it here, because on + an own PR the self login is the person who opened the PR), and the latest + reply from a `lane_logins` login other than the opener's opens with + `LANE_FIXED_HEADER`; a left-open disposition is not an answer. + Logins compare casefolded with any `[bot]` suffix removed, since the API + reports a Bot's login without it. It fails closed on a truncated page. + `botOnly` is TWO conditions, both required. The OPENING comment must be bot-authored -- the canonical rule (`reference/review-discipline.md` D7.5) scopes resolution to bot-opened threads and forbids resolving a human's or @@ -404,6 +435,25 @@ def _author_is_self(comment: object) -> bool: ) total_count = record.get("comments_total_count") + def _bare(login: object) -> str: + return str(login or "").casefold().removesuffix("[bot]") + + opener_bare = _bare(first_author.get("login")) + lane_replies = [ + c + for c in comments[1:] + if (login := _bare(_comment_author(c).get("login"))) in lane_logins + and login != opener_bare + ] + last_lane_body = lane_replies[-1].get("body") if lane_replies else None + lane_answered = ( + bot_only + and bool(opener_bare) + and not any(_author_is_self(c) for c in comments) + and isinstance(last_lane_body, str) + and last_lane_body.split("\n", 1)[0].strip() == LANE_FIXED_HEADER + ) + def _severity_scan_body(comment: object) -> str | None: body = comment.get("body") if is_json_object(comment) else None if not isinstance(body, str): @@ -475,6 +525,7 @@ def _reply_body(comment: object) -> str | None: "author": first_author.get("login"), "authorType": first_author.get("__typename"), "botOnly": bot_only, + "laneAnswered": lane_answered, "severityFlagged": severity_flagged, "findingCount": finding_count, # Live count of ALL comments (not just the fetched page) -- the TOCTOU @@ -516,12 +567,16 @@ def fetch_threads( *, extra_bot_logins: frozenset[str] = frozenset(), self_logins: frozenset[str] = frozenset(), + lane_logins: frozenset[str] = frozenset(), ) -> list[dict[str, object]]: # include_resolved so the caller can see (and skip) already-resolved threads; # comments_first=100 to inspect every participant for the bot-only test. def _project(record: dict[str, Any]) -> dict[str, object]: return project_thread( - record, extra_bot_logins=extra_bot_logins, self_logins=self_logins + record, + extra_bot_logins=extra_bot_logins, + self_logins=self_logins, + lane_logins=lane_logins, ) return [ @@ -575,6 +630,7 @@ def classify( include_human: bool = False, independent: bool = False, pinned_thread: bool = False, + own_pr: bool = False, ) -> str: """Deterministic action label under the active guards. @@ -606,6 +662,11 @@ def classify( # unattended: never a security or P1 thread. Default True fails closed # when the projection did not compute the signal. return "skipped-severity-marked" + if own_pr and not thread.get("laneAnswered", False): + # The own-PR exception admits only a thread a lane already answered: + # the resolver is the PR's author, so the "handled" claim must come + # from another actor. Default False fails closed. + return "skipped-own-pr-not-lane-answered" if independent and not _is_single_finding(thread): # Resolution is a THREAD-level act while a disposition is a claim about ONE # finding, so a thread carrying several findings cannot be cleared on one @@ -892,6 +953,25 @@ def verify_disposition( return False, "refused-evidence-unverifiable" +def repo_lane_identity(repo: str, number: int) -> tuple[str | None, str]: + """The repository's `merge.agent-identity` for the own-PR exception when + `--own-pr-lane-logins` is unset, read at the PR's base commit and the + default branch head (`babysit_merge_policy.lane_identity`), never the PR + head. None, with the reason, when it cannot be established.""" + try: + payload = gh_json( + ["pr", "view", str(number), "--repo", repo, "--json", "baseRefOid"] + ) + base_sha = payload.get("baseRefOid") if is_json_object(payload) else None + if not isinstance(base_sha, str) or not base_sha: + return None, f"no base commit read for {repo}#{number}" + import babysit_merge_policy # noqa: PLC0415 -- heavy; only this path needs it + + return babysit_merge_policy.lane_identity(repo, base_sha) + except (RuntimeError, ValueError, json.JSONDecodeError) as exc: + return None, f"{type(exc).__name__}: {exc}" + + def parse_extra_bot_logins(raw: str | None) -> frozenset[str]: return frozenset(parse_csv_set(raw)) @@ -1040,6 +1120,15 @@ def main() -> int: "your gh login (ships empty)" ), ) + parser.add_argument( + "--own-pr-lane-logins", + default=None, + help=( + "--autonomous only: comma-separated pipeline lane logins. On a PR " + "whose author is a self login, admit a thread one of these lanes " + "already answered instead of refusing the PR; empty keeps the refusal" + ), + ) args = parser.parse_args() def _refuse(message: str, code: int = 2, **envelope: object) -> int: @@ -1179,6 +1268,13 @@ def _refuse(message: str, code: int = 2, **envelope: object) -> int: "a fix in this PR is the fixed disposition (--fix-commit)" ) + lane_logins = normalize_login_set(parse_csv_set(args.own_pr_lane_logins)) + if args.own_pr_lane_logins is not None and not args.autonomous: + return _refuse( + "--own-pr-lane-logins requires --autonomous; it is the worker " + "tier's own-PR exception and widens no other mode" + ) + if args.resolve and args.autonomous and not args.thread_id: return _refuse( "--autonomous --resolve requires a single pinned " @@ -1243,18 +1339,27 @@ def _refuse(message: str, code: int = 2, **envelope: object) -> int: if token.strip().casefold() != "@me" ) + own_pr = False + lane_logins_source = "--own-pr-lane-logins" if lane_logins else None if args.autonomous or args.independent_resolver: try: pr_author = fetch_pull_request_author(repo, number) except (RuntimeError, ValueError, json.JSONDecodeError) as exc: return _refuse(f"{type(exc).__name__}: {exc}") - if is_self_login(pr_author, self_logins): + own_pr = is_self_login(pr_author, self_logins) + if own_pr and args.autonomous and not lane_logins: + # babysit_lane_logins unset: fall back to the repository's own + # lane identity, read only where a person merges it. + identity, lane_logins_source = repo_lane_identity(repo, number) + lane_logins = normalize_login_set([identity] if identity else []) + if own_pr and not (args.autonomous and lane_logins): print( json.dumps( { "pr": args.pr, "prAuthor": pr_author, "skippedOwnPr": True, + "laneLoginsSource": lane_logins_source, "eligibleCount": 0, "humanThreadsActed": 0, "threads": [], @@ -1271,7 +1376,11 @@ def _refuse(message: str, code: int = 2, **envelope: object) -> int: try: threads = fetch_threads( - repo, number, extra_bot_logins=extra_bot_logins, self_logins=self_logins + repo, + number, + extra_bot_logins=extra_bot_logins, + self_logins=self_logins, + lane_logins=lane_logins, ) except (RuntimeError, ValueError, json.JSONDecodeError) as exc: return _refuse(f"{type(exc).__name__}: {exc}") @@ -1297,6 +1406,7 @@ def _refuse(message: str, code: int = 2, **envelope: object) -> int: include_human=args.include_human, independent=args.independent_resolver, pinned_thread=bool(args.thread_id), + own_pr=own_pr, ) entry: dict[str, object] = { "id": thread["id"], @@ -1305,6 +1415,7 @@ def _refuse(message: str, code: int = 2, **envelope: object) -> int: "path": thread["path"], "isOutdated": thread["isOutdated"], "botOnly": thread["botOnly"], + "laneAnswered": thread.get("laneAnswered"), "severityFlagged": thread.get("severityFlagged"), "findingCount": thread.get("findingCount"), "commentCount": thread.get("commentCount"), @@ -1374,6 +1485,7 @@ def _refuse(message: str, code: int = 2, **envelope: object) -> int: "expected_last_updated": args.expected_last_updated, "is_outdated": thread.get("isOutdated"), "severity_flagged": thread.get("severityFlagged"), + "own_pr": own_pr, } ) results.append(entry) @@ -1392,6 +1504,8 @@ def acted(*actions: str) -> int: "action": "resolve" if args.resolve else "list", "onlyOutdated": args.only_outdated, "includeHuman": args.include_human, + "ownPr": own_pr, + "laneLoginsSource": lane_logins_source if own_pr else None, # Count only threads whose OPENING author is human. `botOnly` # (every comment in the thread is a bot) is the wrong gate: a # bot-opened thread carrying a later human reply is `botOnly: @@ -1410,6 +1524,9 @@ def acted(*actions: str) -> int: "skippedNotOutdated": acted("skipped-not-outdated"), "skippedSeverityMarked": acted("skipped-severity-marked"), "skippedMultiFinding": acted("skipped-multi-finding-thread"), + "skippedOwnPrNotLaneAnswered": acted( + "skipped-own-pr-not-lane-answered" + ), # Independent-resolver refusals, rolled up alongside the other # skip counters so a caller sees "the evidence did not hold" # without re-deriving it from the per-thread action values. diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py b/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py index c28d29d7ad..ab6a6d8410 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py @@ -640,6 +640,26 @@ class DocCommandSource: refused_by=PYTHON_CLI, enforced_at="babysit_resolve_thread.py::main", ), + Refusal( + id="resolve.own-pr-lane-logins-requires-autonomous", + claim=( + "--own-pr-lane-logins is the worker tier's own-PR exception and nothing " + "else: outside --autonomous it is refused at exit 2, so it can never widen " + "the explicit or independent-resolver modes." + ), + entry_point=RESOLVE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--own-pr-lane-logins", + "lane", + ), + exit_code=2, + error_contains=("--autonomous",), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), Refusal( id="resolve.wrapper-reaches-failclosed-cli", claim=( @@ -890,6 +910,7 @@ def _thread( severity_flagged: bool | None = None, finding_count: object = _OMITTED, human_deferred: bool = False, + lane_answered: bool | None = None, ) -> dict[str, object]: thread: dict[str, object] = { "isResolved": resolved, @@ -907,6 +928,10 @@ def _thread( # MEANINGFUL value here (the projection reports it for a truncated # comment page), which is why omission needs its own sentinel. thread["findingCount"] = finding_count + if lane_answered is not None: + # Omitted by default, so the own-PR rows keep exercising the guard's + # fail-closed `.get(..., False)` default. + thread["laneAnswered"] = lane_answered return thread @@ -1145,6 +1170,70 @@ def _thread( flags={"autonomous": True, "only_outdated": False, "include_human": False}, expected="eligible", ), + Predicate( + id="classify.own-pr-requires-a-lane-answer", + claim=( + "On a PR whose author is a self login, an outdated clean bot thread is " + "still refused unless a lane login answered it: the resolver is the PR's " + "author, so the 'handled' claim must come from another actor. An absent " + "signal fails closed." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread( + resolved=False, bot_only=True, outdated=True, severity_flagged=False + ), + flags={ + "autonomous": True, + "only_outdated": False, + "include_human": False, + "own_pr": True, + }, + expected="skipped-own-pr-not-lane-answered", + ), + Predicate( + id="classify.own-pr-keeps-the-severity-line", + claim=( + "A lane answer never buys past the severity line: a CRITICAL or security " + "thread on an own PR stays with a person." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread( + resolved=False, + bot_only=True, + outdated=True, + severity_flagged=True, + lane_answered=True, + ), + flags={ + "autonomous": True, + "only_outdated": False, + "include_human": False, + "own_pr": True, + }, + expected="skipped-severity-marked", + ), + Predicate( + id="classify.own-pr-lane-answered-is-eligible", + claim=( + "The own-PR exception admits exactly an outdated, bot-opened, unflagged " + "thread a lane already answered." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread( + resolved=False, + bot_only=True, + outdated=True, + severity_flagged=False, + lane_answered=True, + ), + flags={ + "autonomous": True, + "only_outdated": False, + "include_human": False, + "own_pr": True, + }, + expected="eligible", + ), Predicate( id="classify.already-resolved-precedes-every-guard", claim=( diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py index 20a5d47fa2..fdce10b40d 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py @@ -1320,5 +1320,23 @@ def test_with_no_token_every_read_keeps_the_process_token(self) -> None: self.assertEqual([env for _, env in reads], [None] * 7) +class FetchPullRequestAuthorTests(unittest.TestCase): + def test_reads_the_login_nested_under_author(self) -> None: + # `gh pr view --json author` nests the login one level down; reading it + # at the top level returned "" and the own-PR refusal never fired. + payload = {"author": {"login": "kyle-sexton", "is_bot": False}} + with mock.patch.object(gh, "gh_json", return_value=payload): + self.assertEqual(gh.fetch_pull_request_author("o/r", 1), "kyle-sexton") + + def test_missing_login_fails_closed(self) -> None: + for payload in ({"author": {}}, {"login": "kyle-sexton"}, {"author": None}): + with ( + self.subTest(payload=payload), + mock.patch.object(gh, "gh_json", return_value=payload), + self.assertRaises(RuntimeError), + ): + gh.fetch_pull_request_author("o/r", 1) + + if __name__ == "__main__": unittest.main() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py index 59ffcb0528..10a93b4a7c 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_policy.py @@ -9209,5 +9209,46 @@ def test_a_read_error_is_carried_for_the_scorer(self) -> None: self.assertIn("502", out[0].contents_error) +class LaneIdentity(unittest.TestCase): + """`lane_identity` names the lane only when the base and the default branch + head agree, and never reads the pull request's head (#10763).""" + + def _run(self, base_id, main_id, *, base_error=None, main_raises=False): + base = policy.PipelineConfig( + SHA, "C2", None, base_id, None, (), None, base_error + ) + main = policy.PipelineConfig(MAIN_SHA, "C2", None, main_id, None, (), None) + with ( + mock.patch.object(policy, "config_at", return_value=base) as at, + mock.patch.object( + policy, + "_default_branch_config", + side_effect=policy.PolicyReadError("head unreadable") + if main_raises + else None, + return_value=main, + ), + ): + result = policy.lane_identity("owner/repo", SHA) + at.assert_called_once_with("owner/repo", SHA) + return result + + def test_agreeing_base_and_default_branch_name_the_lane(self) -> None: + identity, source = self._run(IDENTITY, IDENTITY.upper()) + self.assertEqual(identity, IDENTITY) + self.assertIn(SHA, source) + + def test_unset_on_either_side_names_none(self) -> None: + self.assertIsNone(self._run(IDENTITY, None)[0]) + self.assertIsNone(self._run(None, IDENTITY)[0]) + self.assertIsNone(self._run(None, None)[0]) + + def test_differing_identities_name_none(self) -> None: + self.assertIsNone(self._run(IDENTITY, "other-lane[bot]")[0]) + + def test_a_failed_read_names_none(self) -> None: + self.assertIsNone(self._run(IDENTITY, IDENTITY, base_error="bad yaml")[0]) + self.assertIsNone(self._run(IDENTITY, IDENTITY, main_raises=True)[0]) + if __name__ == "__main__": unittest.main() diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py index bd03f5dd8b..8140835c22 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_resolve_thread.py @@ -670,6 +670,7 @@ def test_own_pr_is_refused_in_autonomous_mode(self) -> None: mock.patch.object( rt, "fetch_pull_request_author", return_value="worker-bot" ), + mock.patch.object(rt, "repo_lane_identity", return_value=(None, "unset")), mock.patch.object( sys, "argv", @@ -698,6 +699,7 @@ def test_own_pr_resolve_exits_2(self) -> None: mock.patch.object( rt, "fetch_pull_request_author", return_value="worker-bot" ), + mock.patch.object(rt, "repo_lane_identity", return_value=(None, "unset")), mock.patch.object( sys, "argv", @@ -2131,5 +2133,321 @@ def test_explicit_mode_reports_no_disposition(self) -> None: self.assertIsNone(result["disposition"]) +LANE = "melodic-automation-lanes" +FIXED = "**pr-unblock / address-feedback: VALID, fixed**\n\nEvidence: `a.py:1`\nFix: abc1234" +OWNER = "owner-person" + + +def _own_pr_record(*comments: dict[str, object], outdated: bool = True) -> dict: + return { + "id": "T1", + "isResolved": False, + "isOutdated": outdated, + "comments": list(comments), + "comments_total_count": len(comments), + "comments_truncated": False, + } + + +def _own_pr_run( + record: dict[str, object], + *extra: str, + resolve: bool = False, + repo_identity: str | None = None, +) -> tuple[int, dict[str, object], mock.MagicMock]: + """Drive `main` on an own PR (author == self login) through the REAL + `project_thread`, so the projection and the own-PR branch both run.""" + argv = [ + "owner/repo#1", + "--allowed-owners", + "owner", + "--self-logins", + OWNER, + "--autonomous", + *extra, + ] + if resolve: + argv += [ + "--resolve", + "--thread-id", + "T1", + "--expected-comment-count", + str(record["comments_total_count"]), + "--expected-last-updated", + "2026-10-28T00:00:00Z", + ] + for comment in cast(list[dict[str, object]], record["comments"]): + comment["updatedAt"] = "2026-10-28T00:00:00Z" + buffer = io.StringIO() + with ( + mock.patch.object( + rt, + "fetch_review_threads", + side_effect=lambda repo, number, **kw: [kw["projection"](record)], + ), + mock.patch.object(rt, "fetch_pull_request_author", return_value=OWNER), + mock.patch.object( + rt, + "repo_lane_identity", + return_value=(repo_identity, "merge.agent-identity (test)"), + ), + mock.patch.object(rt, "resolve_thread", return_value=(True, "")) as resolver, + mock.patch.object(rt, "append_resolve_thread_audit"), + mock.patch.object(sys, "argv", ["babysit_resolve_thread.py", *argv]), + redirect_stdout(buffer), + ): + code = rt.main() + return code, json.loads(buffer.getvalue()), resolver + + +class OwnPrLaneAnsweredException(unittest.TestCase): + """#10763: on an owner-login PR the worker may resolve an outdated thread a + lane already answered; never a CRITICAL security-lane thread, never a + thread a person started or joined.""" + + LANES = ("--own-pr-lane-logins", f"{LANE}[bot]") + + def test_answered_lane_thread_is_resolved(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + ) + code, result, resolver = _own_pr_run(record, *self.LANES, resolve=True) + self.assertEqual(result["threads"][0]["action"], "resolved") + self.assertTrue(result["ownPr"]) + resolver.assert_called_once_with("T1") + self.assertEqual(code, 0) + + def test_unanswered_thread_is_not_resolved(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper") + ) + code, result, resolver = _own_pr_run(record, *self.LANES, resolve=True) + self.assertEqual( + result["threads"][0]["action"], "skipped-own-pr-not-lane-answered" + ) + self.assertEqual(result["skippedOwnPrNotLaneAnswered"], 1) + resolver.assert_not_called() + self.assertEqual(code, 10) + + def test_reply_from_a_bot_that_is_not_a_lane_does_not_count(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment("codex", "Bot", "Still applies."), + ) + _, result, resolver = _own_pr_run(record, *self.LANES, resolve=True) + self.assertEqual( + result["threads"][0]["action"], "skipped-own-pr-not-lane-answered" + ) + resolver.assert_not_called() + + def test_the_lane_replying_to_its_own_finding_does_not_count(self) -> None: + record = _own_pr_record( + _comment(LANE, "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + ) + _, result, _ = _own_pr_run(record, *self.LANES) + self.assertEqual( + result["threads"][0]["action"], "skipped-own-pr-not-lane-answered" + ) + + def test_a_left_open_lane_reply_is_not_an_answer(self) -> None: + for label in ("INCORRECT", "UNCERTAIN", "VALID (defer)"): + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment( + LANE, + "Bot", + f"**pr-unblock / address-feedback: {label}, left open**", + ), + ) + with self.subTest(label=label): + _, result, resolver = _own_pr_run(record, *self.LANES, resolve=True) + self.assertEqual( + result["threads"][0]["action"], "skipped-own-pr-not-lane-answered" + ) + resolver.assert_not_called() + + def test_a_later_left_open_reply_overrides_an_earlier_fix(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + _comment( + LANE, "Bot", "**pr-unblock / address-feedback: UNCERTAIN, left open**" + ), + ) + _, result, _ = _own_pr_run(record, *self.LANES) + self.assertEqual( + result["threads"][0]["action"], "skipped-own-pr-not-lane-answered" + ) + + def test_the_fixed_header_must_open_the_reply(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", "Still applies.\n" + FIXED), + ) + _, result, _ = _own_pr_run(record, *self.LANES) + self.assertEqual( + result["threads"][0]["action"], "skipped-own-pr-not-lane-answered" + ) + + def test_critical_security_lane_thread_is_not_resolved(self) -> None: + record = _own_pr_record( + _comment( + "github-actions", + "Bot", + "CRITICAL: token reaches the untrusted job", + ), + _comment(LANE, "Bot", FIXED), + ) + _, result, resolver = _own_pr_run(record, *self.LANES, resolve=True) + self.assertEqual(result["threads"][0]["action"], "skipped-severity-marked") + resolver.assert_not_called() + + def test_person_started_thread_is_not_resolved(self) -> None: + record = _own_pr_record( + _comment("a-reviewer", "User", "Please rename this helper."), + _comment(LANE, "Bot", FIXED), + ) + _, result, resolver = _own_pr_run(record, *self.LANES, resolve=True) + self.assertEqual(result["threads"][0]["action"], "skipped-human-thread") + resolver.assert_not_called() + + def test_owner_thread_is_not_resolved(self) -> None: + record = _own_pr_record( + _comment(OWNER, "User", "Rename this helper."), + _comment(LANE, "Bot", FIXED), + ) + _, result, resolver = _own_pr_run(record, *self.LANES, resolve=True) + self.assertEqual(result["threads"][0]["action"], "skipped-human-thread") + resolver.assert_not_called() + + def test_owner_reply_inside_a_bot_thread_blocks_the_resolve(self) -> None: + # The self login is neutral as a reply elsewhere, but on an own PR it + # is the person who opened the PR, so their reply keeps the thread. + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + _comment(OWNER, "User", "Not fixed yet."), + ) + _, result, resolver = _own_pr_run(record, *self.LANES, resolve=True) + self.assertEqual( + result["threads"][0]["action"], "skipped-own-pr-not-lane-answered" + ) + resolver.assert_not_called() + + def test_current_thread_is_not_resolved(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + outdated=False, + ) + _, result, _ = _own_pr_run(record, *self.LANES) + self.assertEqual(result["threads"][0]["action"], "skipped-not-outdated") + + def test_without_the_flag_the_own_pr_refusal_is_unchanged(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + ) + code, result, resolver = _own_pr_run(record, resolve=True) + self.assertTrue(result["skippedOwnPr"]) + resolver.assert_not_called() + self.assertEqual(code, 2) + + def test_an_empty_flag_keeps_the_own_pr_refusal(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + ) + _, result, _ = _own_pr_run(record, "--own-pr-lane-logins", "") + self.assertTrue(result["skippedOwnPr"]) + + def test_repo_agent_identity_stands_in_when_the_setting_is_unset(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + ) + code, result, resolver = _own_pr_run( + record, resolve=True, repo_identity=f"{LANE}[bot]" + ) + self.assertEqual(result["threads"][0]["action"], "resolved") + self.assertEqual(result["laneLoginsSource"], "merge.agent-identity (test)") + resolver.assert_called_once_with("T1") + self.assertEqual(code, 0) + + def test_both_unset_keeps_the_own_pr_refusal(self) -> None: + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + ) + code, result, resolver = _own_pr_run(record, resolve=True, repo_identity=None) + self.assertTrue(result["skippedOwnPr"]) + resolver.assert_not_called() + self.assertEqual(code, 2) + + def test_the_user_setting_overrides_the_repo_identity(self) -> None: + # The setting names another lane, so the repo identity's reply does not + # count, and the repo identity is never read. + record = _own_pr_record( + _comment("github-actions", "Bot", "P2: rename this helper"), + _comment(LANE, "Bot", FIXED), + ) + with mock.patch.object(rt, "repo_lane_identity") as repo_read: + buffer = io.StringIO() + with ( + mock.patch.object( + rt, + "fetch_review_threads", + side_effect=lambda repo, number, **kw: [kw["projection"](record)], + ), + mock.patch.object(rt, "fetch_pull_request_author", return_value=OWNER), + mock.patch.object( + sys, + "argv", + [ + "babysit_resolve_thread.py", + "owner/repo#1", + "--allowed-owners", + "owner", + "--self-logins", + OWNER, + "--autonomous", + "--own-pr-lane-logins", + "other-lane[bot]", + ], + ), + redirect_stdout(buffer), + ): + rt.main() + result = json.loads(buffer.getvalue()) + repo_read.assert_not_called() + self.assertEqual(result["laneLoginsSource"], "--own-pr-lane-logins") + self.assertEqual( + result["threads"][0]["action"], "skipped-own-pr-not-lane-answered" + ) + + def test_flag_without_autonomous_is_refused(self) -> None: + buffer = io.StringIO() + with ( + mock.patch.object( + sys, + "argv", + [ + "babysit_resolve_thread.py", + "owner/repo#1", + "--allowed-owners", + "owner", + "--own-pr-lane-logins", + LANE, + ], + ), + redirect_stdout(buffer), + ): + code = rt.main() + self.assertEqual(code, 2) + self.assertIn("requires --autonomous", json.loads(buffer.getvalue())["error"]) + + if __name__ == "__main__": unittest.main()