From 4bc91f84fc3e457fac92462350e1496ea6adc9d8 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Thu, 13 Aug 2026 14:16:08 +0300 Subject: [PATCH 1/3] fix(merge-pr): enforce agent attribution Resolve the merge executor identity through Todos, refuse placeholder or mismatched trailers, and verify the merged commit retains exactly one canonical Agent trailer. Agent: Hortensia-404 --- .codewith/skills/merge-pr/SKILL.md | 11 +- .../merge-pr/references/merge-safety.md | 5 +- .../merge-pr/scripts/merge_pr_execute.py | 53 +++++++- .../merge-pr/tests/test_merge_pr_execute.py | 117 +++++++++++++++++- 4 files changed, 173 insertions(+), 13 deletions(-) diff --git a/.codewith/skills/merge-pr/SKILL.md b/.codewith/skills/merge-pr/SKILL.md index 54691b9b7..b6681145a 100644 --- a/.codewith/skills/merge-pr/SKILL.md +++ b/.codewith/skills/merge-pr/SKILL.md @@ -24,8 +24,9 @@ Actual merge means `immediate-merge`, `auto-merge`, or `merge-queue`. 2. Require one independent reviewer artifact from the fixed reviewer run tied to the exact PR head SHA. Self-review is not an acceptable fallback for actual merge. If that independent artifact cannot be obtained, stop before merge. A coordinator may deliberately fix additional reviewers before review begins, but the skill never requires more than one; every supplied artifact must have a distinct reviewer identity or run id. 3. Each reviewer artifact must include: repository, PR number, exact head SHA, reviewer identity or run id, timestamp, verdict, checked risks summary, and blocking findings. Treat missing, invalid, future, or stale artifact timestamps as blockers; the helper default staleness window is 24 hours unless `--max-artifact-age-hours` is explicitly set. 4. The executor must re-fetch and re-check immediately before merge. A preflight JSON snapshot is advisory only and is not authority to merge. -5. Immediate squash merges must run through `scripts/merge_pr_execute.py`, which holds one exclusive `github-merge/OWNER/REPO/refs/heads/BASE` resource lock across authoritative recheck, exact base/head/tree validation, merge, and postverification. Pass `--lock-owner registered-agent/session-id`; the executable appends a unique invocation id so even parallel merges from one session cannot refresh the same ownership record. -6. The merge command must include `gh pr merge --match-head-commit ` and must never include `--admin`. +5. Immediate squash merges must run through `scripts/merge_pr_execute.py`, which holds one exclusive `github-merge/OWNER/REPO/refs/heads/BASE` resource lock across authoritative recheck, exact base/head/tree validation, merge, and postverification. Direct `gh pr merge` is not a supported substitute. Pass `--lock-owner registered-agent/session-id`; the executable appends a unique invocation id so even parallel merges from one session cannot refresh the same ownership record. +6. The executor resolves the agent portion of `--lock-owner` through the exact Todos agent lookup. The merge body must end with exactly one `Agent:` trailer using that canonical registered name. Wrong names and `unresolved-*` placeholder identities are refused before mutation. +7. The merge command must include `gh pr merge --match-head-commit ` and must never include `--admin`. ## Workflow @@ -46,6 +47,10 @@ Actual merge means `immediate-merge`, `auto-merge`, or `merge-queue`. - For queue branches, follow local `gh pr merge --help`: queue-required branches need no strategy; passed checks enqueue; pending checks enable merge-when-ready only under GitHub queue semantics and only when the user explicitly asked for delayed merge intent. 8. Execute once all gates pass, then postverify. +Postverification reads the merged commit message from Git and requires exactly +one final `Agent:` trailer with the same canonical registered identity. Parent, +tree, head, and GitHub state checks do not substitute for this provenance gate. + The stable lock serializes merge-pr executors that participate in this path, including parallel invocations from one session because the executable adds a unique lock-holder id. GitHub and `gh` expose an expected head OID but no expected base OID, so a nonparticipating external actor can still move the base after the final locked check and before GitHub commits the merge. The postverify parent/tree gate remains mandatory and reports that mismatch; the lock does not claim to make external GitHub mutation atomic. ## Preflight JSON @@ -89,4 +94,4 @@ Record the PR state, merged commit or queue state, target branch state, CI/check Validate the contract with static checks and fixtures: - Static checks: trigger text present, `scancommitpr` non-goal present, four modes present, one independent reviewer artifact required, no self-review fallback for actual merge, optional additional artifacts remain distinct, executor recheck required, stable branch lock required for immediate squash, exact base/head/tree required, `--match-head-commit` required, `--admin` forbidden, postverify fields present. -- Fixtures: green PR with one artifact, green PR with two distinct optional artifacts, no reviewer artifacts, malformed or stale artifact, wrong repository/PR/head artifact, duplicate reviewer identity, pending checks, failed checks, requested changes, draft PR, conflict PR, merge queue passed checks, merge queue pending checks, explicit merge-when-green, head SHA changes between preflight and executor, branch protection queue, unchanged-base immediate squash, two concurrent candidates on one repository/default branch, moved-base retry refusal, and no generated command containing `--admin`. +- Fixtures: green PR with one artifact, green PR with two distinct optional artifacts, no reviewer artifacts, malformed or stale artifact, wrong repository/PR/head artifact, duplicate reviewer identity, pending checks, failed checks, requested changes, draft PR, conflict PR, merge queue passed checks, merge queue pending checks, explicit merge-when-green, head SHA changes between preflight and executor, branch protection queue, unchanged-base immediate squash preserving one canonical registered `Agent:` trailer, missing postmerge trailer refusal, wrong or unresolved identity refusal before mutation, two concurrent candidates on one repository/default branch, moved-base retry refusal, and no generated command containing `--admin`. diff --git a/.codewith/skills/merge-pr/references/merge-safety.md b/.codewith/skills/merge-pr/references/merge-safety.md index 9c58d1bda..61ce779f7 100644 --- a/.codewith/skills/merge-pr/references/merge-safety.md +++ b/.codewith/skills/merge-pr/references/merge-safety.md @@ -5,7 +5,8 @@ These rules keep merge execution tied to fresh evidence rather than stale snapsh - Preflight is read-only and advisory. It may summarize PR state, checks, reviews, and reviewer artifacts, but it must not mutate GitHub or local git state. - Check summaries are decision evidence, not navigation evidence. Project GitHub check reads at the CLI boundary to `name,state,bucket`, retain only named decision fields, and never request or persist check links, `detailsUrl`, raw `statusCheckRollup`, or other URL-bearing check fields. - Executor recheck is authoritative. Immediately before any actual merge action, re-fetch and re-read PR state, head SHA, mergeability, checks, reviews, draft/conflict state, and queue/protection behavior. -- Immediate squash merges use the package-owned `merge_pr_execute.py` path. It acquires one exclusive lock keyed to `github-merge/OWNER/REPO/refs/heads/BASE` before authoritative recheck and holds it through exact base/head/tree validation, merge, and postverification. The caller supplies its registered agent/session identity and the executable appends a unique invocation id, so parallel work from one session cannot refresh the same ownership record. +- Immediate squash merges use the package-owned `merge_pr_execute.py` path; direct `gh pr merge` bypasses its provenance and candidate gates and is unsupported. The executor acquires one exclusive lock keyed to `github-merge/OWNER/REPO/refs/heads/BASE` before authoritative recheck and holds it through exact base/head/tree validation, merge, and postverification. The caller supplies its registered agent/session identity and the executable appends a unique invocation id, so parallel work from one session cannot refresh the same ownership record. +- The executor resolves the lock owner's agent portion through the exact Todos agent lookup and requires the body trailer to use the returned canonical name. `unresolved-*` placeholders and body/owner mismatches fail before mutation. - Actual merge requires one independent reviewer artifact from the reviewer fixed before review begins for the exact PR head SHA. A coordinator may deliberately fix additional reviewers before review begins, but the skill never requires more than one and every supplied artifact must identify a distinct reviewer run. Missing, duplicate-run, self-review, stale-head, blocking-verdict, or blocking-finding artifacts stop the merge. - Reviewer artifact freshness is part of merge safety. Missing, invalid, future, or stale timestamps are blockers; the helper default staleness window is 24 hours unless explicitly overridden for a run. - Every merge command must include `gh pr merge --match-head-commit `. This protects against merging a changed PR head after review. @@ -13,4 +14,4 @@ These rules keep merge execution tied to fresh evidence rather than stale snapsh - `--admin` is forbidden. Do not bypass branch protection, queue policy, required checks, or required reviews. - Use `--auto` only when the user explicitly asks for delayed intent such as "merge when green" or "enable auto-merge". - Merge queue is an execution mode, not a merge strategy. Follow local `gh pr merge --help`: queue-required branches need no strategy; passed checks enqueue; pending checks enable queue-when-ready only under GitHub queue semantics and explicit delayed user intent. -- Postverify must record PR state, merged commit or queue state, target branch state, CI/check state when available, command used excluding secrets, and final outcome. If execution stops, record the gate that stopped it. +- Postverify must read the merged commit message and require exactly one final `Agent:` trailer matching the canonical registered executor identity. It also records PR state, merged commit or queue state, target branch state, CI/check state when available, command used excluding secrets, and final outcome. If execution stops, record the gate that stopped it. diff --git a/.codewith/skills/merge-pr/scripts/merge_pr_execute.py b/.codewith/skills/merge-pr/scripts/merge_pr_execute.py index 3c3801214..a3c9b7817 100755 --- a/.codewith/skills/merge-pr/scripts/merge_pr_execute.py +++ b/.codewith/skills/merge-pr/scripts/merge_pr_execute.py @@ -16,6 +16,7 @@ SHA_PATTERN = re.compile(r"^[0-9a-f]{40}$") AGENT_TRAILER_PATTERN = re.compile(r"^Agent: \S+$") +UNRESOLVED_AGENT_PATTERN = re.compile(r"^unresolved(?:-|$)", re.IGNORECASE) LOCK_RESOURCE_TYPE = "github-merge" @@ -49,7 +50,7 @@ def validate_sha(value: str, name: str) -> None: raise ValueError(f"{name} must be a lowercase 40-character Git SHA") -def validate_body_file(path: Path) -> None: +def validate_body_file(path: Path) -> str: if not path.is_file(): raise ValueError(f"merge body file does not exist: {path}") lines = [line.strip() for line in path.read_text(encoding="utf-8").splitlines()] @@ -60,6 +61,37 @@ def validate_body_file(path: Path) -> None: raise ValueError("merge body must contain exactly one Agent: trailer") if any(line.lower().startswith("co-authored-by:") for line in nonempty): raise ValueError("merge body must not contain Co-Authored-By") + return nonempty[-1].removeprefix("Agent: ") + + +def resolve_registered_agent(lock_owner: str) -> str: + requested = lock_owner.split("/", 1)[0] + if not requested or UNRESOLVED_AGENT_PATTERN.match(requested): + raise ValueError("--lock-owner must name a registered agent identity") + payload, error = run_json(["todos", "--json", "agent", requested]) + if error: + raise ValueError( + f"--lock-owner agent does not resolve to a registered agent identity: {requested}" + ) + record = (payload or {}).get("agent") if isinstance(payload, dict) else None + canonical = str((record or {}).get("name") or "").strip() + if not canonical or UNRESOLVED_AGENT_PATTERN.match(canonical): + raise ValueError("--lock-owner must resolve to a registered agent identity") + return canonical + + +def agent_trailer_blockers(message: str, expected_agent: str) -> list[str]: + nonempty = [line.strip() for line in message.splitlines() if line.strip()] + trailers = [line for line in nonempty if line.startswith("Agent: ")] + expected = f"Agent: {expected_agent}" + blockers = [] + if len(trailers) != 1: + blockers.append("merged_agent_trailer_count_mismatch") + elif trailers[0] != expected: + blockers.append("merged_agent_trailer_identity_mismatch") + if not nonempty or nonempty[-1] != expected: + blockers.append("merged_agent_trailer_not_final") + return blockers def lock_command( @@ -206,6 +238,9 @@ def fetch_postmerge( git_dir, ["rev-parse", "--verify", f"{merge_commit}^{{tree}}"], ) + if error: + return None, error + message, error = git_output(git_dir, ["show", "-s", "--format=%B", merge_commit]) if error: return None, error return { @@ -213,6 +248,7 @@ def fetch_postmerge( "merge_commit": resolved_merge or "", "parent": parent or "", "tree": tree or "", + "message": message or "", }, None @@ -298,12 +334,18 @@ def execute(args: argparse.Namespace) -> int: try: for name, value in expected.items(): validate_sha(value, f"expected {name}") - body_file = Path(args.body_file).resolve() - validate_body_file(body_file) if "/" not in args.repo or args.repo.startswith("/") or args.repo.endswith("/"): raise ValueError("--repo must use OWNER/REPO") if "/" not in args.lock_owner: raise ValueError("--lock-owner must be unique per invocation, such as agent/session-id") + registered_agent = resolve_registered_agent(args.lock_owner) + body_file = Path(args.body_file).resolve() + body_agent = validate_body_file(body_file) + if body_agent != registered_agent: + raise ValueError( + "merge body Agent trailer must use registered agent identity " + f"{registered_agent}" + ) initial, error = current_pr(args.repo, args.pr) if error: @@ -641,6 +683,9 @@ def execute(args: argparse.Namespace) -> int: post_blockers.append("merged_tree_mismatch") if (post_pr or {}).get("headRefOid") != expected["head"]: post_blockers.append("merged_head_mismatch") + post_blockers.extend( + agent_trailer_blockers((post or {}).get("message") or "", registered_agent) + ) if post_blockers: emit( "postverify_failed", @@ -655,6 +700,7 @@ def execute(args: argparse.Namespace) -> int: actual_tree=(post or {}).get("tree"), merge_commit=merge_commit, target_branch_head=(post or {}).get("target"), + expected_agent=registered_agent, blocking_reasons=post_blockers, ) return 6 @@ -672,6 +718,7 @@ def execute(args: argparse.Namespace) -> int: merged_parent=(post or {}).get("parent"), merged_tree=(post or {}).get("tree"), target_branch_head=(post or {}).get("target"), + agent_identity=registered_agent, merge_command=merge_command, ) return 0 diff --git a/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py b/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py index 03a6d2886..5bd1140b1 100644 --- a/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py +++ b/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py @@ -94,6 +94,7 @@ def __init__(self, root: Path) -> None: encoding="utf-8", ) self._write_executable("conversations", self._conversations_script()) + self._write_executable("todos", self._todos_script()) self._write_executable("gh", self._gh_script()) def _write_executable(self, name: str, content: str) -> None: @@ -175,6 +176,25 @@ def _conversations_script(self) -> str: """ ) + def _todos_script(self) -> str: + return textwrap.dedent( + """\ + #!/usr/bin/env python3 + import json + import sys + + args = sys.argv[1:] + if args[:2] == ["--json", "agent"] and len(args) == 3: + if args[2].casefold() == "severianus": + print(json.dumps({"agent": {"name": "Severianus"}})) + raise SystemExit(0) + print(json.dumps({"error": "Agent not found"})) + raise SystemExit(1) + print("unexpected todos invocation: " + " ".join(args), file=sys.stderr) + raise SystemExit(64) + """ + ) + def _gh_script(self) -> str: return textwrap.dedent( """\ @@ -298,9 +318,18 @@ def git(*git_args): ) base = external_commit tree = git("merge-tree", "--write-tree", base, item["head"]) + subject = ( + args[args.index("--subject") + 1] + if "--subject" in args + else f"merge pr {pr}" + ) + body = Path(args[args.index("--body-file") + 1]).read_text(encoding="utf-8") + message = subject + "\\n\\n" + if os.environ.get("MERGE_FIXTURE_DROP_BODY") != "1": + message += body commit = subprocess.run( ["git", "--git-dir", remote, "commit-tree", tree, "-p", base], - input=f"merge pr {pr}\\n", + input=message, check=True, text=True, stdout=subprocess.PIPE, @@ -349,10 +378,10 @@ def artifact(self, pr_number: int) -> Path: ) return path - def body(self, pr_number: int) -> Path: + def body(self, pr_number: int, *, agent: str = "Severianus") -> Path: path = self.root / f"body-{pr_number}.txt" path.write_text( - f"Merge fixture PR {pr_number}.\n\nAgent: severianus\n", + f"Merge fixture PR {pr_number}.\n\nAgent: {agent}\n", encoding="utf-8", ) return path @@ -362,6 +391,7 @@ def env( *, block_first: bool = False, block_first_refresh: bool = False, + drop_body: bool = False, external_move: bool = False, ) -> dict[str, str]: return { @@ -371,6 +401,7 @@ def env( "MERGE_FIXTURE_ALLOW_FIRST_REFRESH": str(self.allow_first_refresh), "MERGE_FIXTURE_BLOCK_FIRST": "1" if block_first else "0", "MERGE_FIXTURE_BLOCK_FIRST_REFRESH": "1" if block_first_refresh else "0", + "MERGE_FIXTURE_DROP_BODY": "1" if drop_body else "0", "MERGE_FIXTURE_EXTERNAL_MOVE": "1" if external_move else "0", "MERGE_FIXTURE_FIRST_ENTERED": str(self.first_merge_entered), "MERGE_FIXTURE_FIRST_REFRESH_ENTERED": str(self.first_refresh_entered), @@ -381,7 +412,13 @@ def env( "MERGE_FIXTURE_STATE": str(self.state), } - def command(self, pr_number: int, lock_owner: str) -> list[str]: + def command( + self, + pr_number: int, + lock_owner: str, + *, + agent: str = "Severianus", + ) -> list[str]: return [ sys.executable, str(SCRIPT), @@ -397,7 +434,7 @@ def command(self, pr_number: int, lock_owner: str) -> list[str]: "--artifact", str(self.artifact(pr_number)), "--body-file", - str(self.body(pr_number)), + str(self.body(pr_number, agent=agent)), "--lock-owner", lock_owner, "--git-remote", @@ -420,6 +457,22 @@ def merge_call_rows(self) -> list[list[str]]: for line in self.merge_calls.read_text(encoding="utf-8").splitlines() ] + def merge_message(self, pr_number: int) -> str: + state = json.loads(self.state.read_text(encoding="utf-8")) + merge_commit = state["prs"][str(pr_number)]["merge_commit"] + return run( + [ + "git", + "--git-dir", + str(self.remote), + "show", + "-s", + "--format=%B", + merge_commit, + ], + cwd=self.root, + ) + class MergePrExecuteTests(unittest.TestCase): def test_unchanged_base_uses_exact_head_squash_path(self) -> None: @@ -448,6 +501,60 @@ def test_unchanged_base_uses_exact_head_squash_path(self) -> None: self.assertIn("--squash", merge_call) self.assertIn("--body-file", merge_call) self.assertNotIn("--admin", merge_call) + self.assertEqual( + [ + line + for line in fixture.merge_message(1).splitlines() + if line.startswith("Agent: ") + ], + ["Agent: Severianus"], + ) + + def test_missing_agent_trailer_fails_postverification(self) -> None: + with TemporaryDirectory() as directory: + fixture = MergeFixture(Path(directory)) + result = subprocess.run( + fixture.command(1, "severianus/session-a"), + cwd=fixture.root, + env=fixture.env(drop_body=True), + check=False, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + + self.assertEqual(result.returncode, 6, result.stderr) + output = json.loads(result.stdout) + self.assertEqual(output["status"], "postverify_failed") + self.assertIn( + "merged_agent_trailer_count_mismatch", + output["blocking_reasons"], + ) + + def test_wrong_or_unresolved_agent_identity_refuses_before_merge(self) -> None: + cases = ( + ("OtherAgent", "severianus/session-a"), + ("unresolved-account002", "unresolved-account002/session-a"), + ) + for agent, lock_owner in cases: + with self.subTest(agent=agent), TemporaryDirectory() as directory: + fixture = MergeFixture(Path(directory)) + result = subprocess.run( + fixture.command(1, lock_owner, agent=agent), + cwd=fixture.root, + env=fixture.env(), + check=False, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + + self.assertEqual(result.returncode, 64, result.stderr) + output = json.loads(result.stdout) + self.assertEqual(output["status"], "invalid_arguments") + self.assertIn("registered agent identity", output["error"]) + self.assertEqual(fixture.mutation_rows(), []) + self.assertEqual(fixture.merge_call_rows(), []) def test_two_candidates_cannot_enter_and_moved_base_retry_refuses(self) -> None: with TemporaryDirectory() as directory: From 1e6febb1e0ec7df6a328b7d8aa512778d7622591 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Thu, 13 Aug 2026 14:25:00 +0300 Subject: [PATCH 2/3] fix(merge-pr): pin executor agent identity Resolve the stable Todos agent ID, verify the exact returned record and lock-owner name, and cover duplicate display names without widening the merge change. Agent: Hortensia-404 --- .codewith/skills/merge-pr/SKILL.md | 6 +-- .../merge-pr/references/merge-safety.md | 4 +- .../merge-pr/scripts/merge_pr_execute.py | 25 +++++++++--- .../merge-pr/tests/test_merge_pr_execute.py | 39 ++++++++++++++++++- 4 files changed, 62 insertions(+), 12 deletions(-) diff --git a/.codewith/skills/merge-pr/SKILL.md b/.codewith/skills/merge-pr/SKILL.md index b6681145a..488477195 100644 --- a/.codewith/skills/merge-pr/SKILL.md +++ b/.codewith/skills/merge-pr/SKILL.md @@ -24,8 +24,8 @@ Actual merge means `immediate-merge`, `auto-merge`, or `merge-queue`. 2. Require one independent reviewer artifact from the fixed reviewer run tied to the exact PR head SHA. Self-review is not an acceptable fallback for actual merge. If that independent artifact cannot be obtained, stop before merge. A coordinator may deliberately fix additional reviewers before review begins, but the skill never requires more than one; every supplied artifact must have a distinct reviewer identity or run id. 3. Each reviewer artifact must include: repository, PR number, exact head SHA, reviewer identity or run id, timestamp, verdict, checked risks summary, and blocking findings. Treat missing, invalid, future, or stale artifact timestamps as blockers; the helper default staleness window is 24 hours unless `--max-artifact-age-hours` is explicitly set. 4. The executor must re-fetch and re-check immediately before merge. A preflight JSON snapshot is advisory only and is not authority to merge. -5. Immediate squash merges must run through `scripts/merge_pr_execute.py`, which holds one exclusive `github-merge/OWNER/REPO/refs/heads/BASE` resource lock across authoritative recheck, exact base/head/tree validation, merge, and postverification. Direct `gh pr merge` is not a supported substitute. Pass `--lock-owner registered-agent/session-id`; the executable appends a unique invocation id so even parallel merges from one session cannot refresh the same ownership record. -6. The executor resolves the agent portion of `--lock-owner` through the exact Todos agent lookup. The merge body must end with exactly one `Agent:` trailer using that canonical registered name. Wrong names and `unresolved-*` placeholder identities are refused before mutation. +5. Immediate squash merges must run through `scripts/merge_pr_execute.py`, which holds one exclusive `github-merge/OWNER/REPO/refs/heads/BASE` resource lock across authoritative recheck, exact base/head/tree validation, merge, and postverification. Direct `gh pr merge` is not a supported substitute. Pass both `--lock-owner registered-agent/session-id` and the stable exact Todos identity as `--agent-id `; the executable appends a unique invocation id so even parallel merges from one session cannot refresh the same ownership record. +6. The executor resolves `--agent-id` through the exact Todos agent lookup and verifies that the returned ID is identical and its canonical name matches the agent portion of `--lock-owner`. The merge body must end with exactly one `Agent:` trailer using that canonical registered name. Missing IDs, name mismatches, and `unresolved-*` placeholder identities are refused before mutation. 7. The merge command must include `gh pr merge --match-head-commit ` and must never include `--admin`. ## Workflow @@ -94,4 +94,4 @@ Record the PR state, merged commit or queue state, target branch state, CI/check Validate the contract with static checks and fixtures: - Static checks: trigger text present, `scancommitpr` non-goal present, four modes present, one independent reviewer artifact required, no self-review fallback for actual merge, optional additional artifacts remain distinct, executor recheck required, stable branch lock required for immediate squash, exact base/head/tree required, `--match-head-commit` required, `--admin` forbidden, postverify fields present. -- Fixtures: green PR with one artifact, green PR with two distinct optional artifacts, no reviewer artifacts, malformed or stale artifact, wrong repository/PR/head artifact, duplicate reviewer identity, pending checks, failed checks, requested changes, draft PR, conflict PR, merge queue passed checks, merge queue pending checks, explicit merge-when-green, head SHA changes between preflight and executor, branch protection queue, unchanged-base immediate squash preserving one canonical registered `Agent:` trailer, missing postmerge trailer refusal, wrong or unresolved identity refusal before mutation, two concurrent candidates on one repository/default branch, moved-base retry refusal, and no generated command containing `--admin`. +- Fixtures: green PR with one artifact, green PR with two distinct optional artifacts, no reviewer artifacts, malformed or stale artifact, wrong repository/PR/head artifact, duplicate reviewer identity, pending checks, failed checks, requested changes, draft PR, conflict PR, merge queue passed checks, merge queue pending checks, explicit merge-when-green, head SHA changes between preflight and executor, branch protection queue, unchanged-base immediate squash preserving one canonical registered `Agent:` trailer, duplicate display names resolved by the intended stable agent ID, missing postmerge trailer refusal, wrong or unresolved identity refusal before mutation, two concurrent candidates on one repository/default branch, moved-base retry refusal, and no generated command containing `--admin`. diff --git a/.codewith/skills/merge-pr/references/merge-safety.md b/.codewith/skills/merge-pr/references/merge-safety.md index 61ce779f7..53c5c6eff 100644 --- a/.codewith/skills/merge-pr/references/merge-safety.md +++ b/.codewith/skills/merge-pr/references/merge-safety.md @@ -5,8 +5,8 @@ These rules keep merge execution tied to fresh evidence rather than stale snapsh - Preflight is read-only and advisory. It may summarize PR state, checks, reviews, and reviewer artifacts, but it must not mutate GitHub or local git state. - Check summaries are decision evidence, not navigation evidence. Project GitHub check reads at the CLI boundary to `name,state,bucket`, retain only named decision fields, and never request or persist check links, `detailsUrl`, raw `statusCheckRollup`, or other URL-bearing check fields. - Executor recheck is authoritative. Immediately before any actual merge action, re-fetch and re-read PR state, head SHA, mergeability, checks, reviews, draft/conflict state, and queue/protection behavior. -- Immediate squash merges use the package-owned `merge_pr_execute.py` path; direct `gh pr merge` bypasses its provenance and candidate gates and is unsupported. The executor acquires one exclusive lock keyed to `github-merge/OWNER/REPO/refs/heads/BASE` before authoritative recheck and holds it through exact base/head/tree validation, merge, and postverification. The caller supplies its registered agent/session identity and the executable appends a unique invocation id, so parallel work from one session cannot refresh the same ownership record. -- The executor resolves the lock owner's agent portion through the exact Todos agent lookup and requires the body trailer to use the returned canonical name. `unresolved-*` placeholders and body/owner mismatches fail before mutation. +- Immediate squash merges use the package-owned `merge_pr_execute.py` path; direct `gh pr merge` bypasses its provenance and candidate gates and is unsupported. The executor acquires one exclusive lock keyed to `github-merge/OWNER/REPO/refs/heads/BASE` before authoritative recheck and holds it through exact base/head/tree validation, merge, and postverification. The caller supplies its registered agent/session identity plus its stable exact Todos agent ID, and the executable appends a unique invocation id so parallel work from one session cannot refresh the same ownership record. +- The executor resolves the exact agent ID, verifies the returned ID and canonical name against the lock owner's agent portion, and requires the body trailer to use that canonical name. Duplicate display names cannot redirect the lookup; missing IDs, `unresolved-*` placeholders, and body/owner mismatches fail before mutation. - Actual merge requires one independent reviewer artifact from the reviewer fixed before review begins for the exact PR head SHA. A coordinator may deliberately fix additional reviewers before review begins, but the skill never requires more than one and every supplied artifact must identify a distinct reviewer run. Missing, duplicate-run, self-review, stale-head, blocking-verdict, or blocking-finding artifacts stop the merge. - Reviewer artifact freshness is part of merge safety. Missing, invalid, future, or stale timestamps are blockers; the helper default staleness window is 24 hours unless explicitly overridden for a run. - Every merge command must include `gh pr merge --match-head-commit `. This protects against merging a changed PR head after review. diff --git a/.codewith/skills/merge-pr/scripts/merge_pr_execute.py b/.codewith/skills/merge-pr/scripts/merge_pr_execute.py index a3c9b7817..5268e99aa 100755 --- a/.codewith/skills/merge-pr/scripts/merge_pr_execute.py +++ b/.codewith/skills/merge-pr/scripts/merge_pr_execute.py @@ -64,19 +64,25 @@ def validate_body_file(path: Path) -> str: return nonempty[-1].removeprefix("Agent: ") -def resolve_registered_agent(lock_owner: str) -> str: +def resolve_registered_agent(agent_id: str, lock_owner: str) -> str: requested = lock_owner.split("/", 1)[0] if not requested or UNRESOLVED_AGENT_PATTERN.match(requested): raise ValueError("--lock-owner must name a registered agent identity") - payload, error = run_json(["todos", "--json", "agent", requested]) + payload, error = run_json(["todos", "--json", "agent", agent_id]) if error: raise ValueError( - f"--lock-owner agent does not resolve to a registered agent identity: {requested}" + f"--agent-id does not resolve to a registered agent identity: {agent_id}" ) record = (payload or {}).get("agent") if isinstance(payload, dict) else None + resolved_id = str((record or {}).get("id") or "").strip() canonical = str((record or {}).get("name") or "").strip() - if not canonical or UNRESOLVED_AGENT_PATTERN.match(canonical): - raise ValueError("--lock-owner must resolve to a registered agent identity") + if resolved_id != agent_id or not canonical or UNRESOLVED_AGENT_PATTERN.match(canonical): + raise ValueError("--agent-id must resolve to the exact registered agent identity") + if requested.casefold() != canonical.casefold(): + raise ValueError( + "--lock-owner agent must match the identity resolved by --agent-id: " + f"{canonical}" + ) return canonical @@ -338,7 +344,7 @@ def execute(args: argparse.Namespace) -> int: raise ValueError("--repo must use OWNER/REPO") if "/" not in args.lock_owner: raise ValueError("--lock-owner must be unique per invocation, such as agent/session-id") - registered_agent = resolve_registered_agent(args.lock_owner) + registered_agent = resolve_registered_agent(args.agent_id, args.lock_owner) body_file = Path(args.body_file).resolve() body_agent = validate_body_file(body_file) if body_agent != registered_agent: @@ -701,6 +707,7 @@ def execute(args: argparse.Namespace) -> int: merge_commit=merge_commit, target_branch_head=(post or {}).get("target"), expected_agent=registered_agent, + expected_agent_id=args.agent_id, blocking_reasons=post_blockers, ) return 6 @@ -719,6 +726,7 @@ def execute(args: argparse.Namespace) -> int: merged_tree=(post or {}).get("tree"), target_branch_head=(post or {}).get("target"), agent_identity=registered_agent, + agent_identity_id=args.agent_id, merge_command=merge_command, ) return 0 @@ -769,6 +777,11 @@ def main() -> int: required=True, help="Unique invocation owner, for example registered-agent/session-id", ) + parser.add_argument( + "--agent-id", + required=True, + help="Stable Todos agent ID for the agent named by --lock-owner", + ) parser.add_argument( "--lock-ttl", type=int, diff --git a/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py b/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py index 5bd1140b1..5508576a2 100644 --- a/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py +++ b/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py @@ -185,8 +185,19 @@ def _todos_script(self) -> str: args = sys.argv[1:] if args[:2] == ["--json", "agent"] and len(args) == 3: + if args[2] == "agent-severianus": + print( + json.dumps( + {"agent": {"id": "agent-severianus", "name": "Severianus"}} + ) + ) + raise SystemExit(0) if args[2].casefold() == "severianus": - print(json.dumps({"agent": {"name": "Severianus"}})) + print( + json.dumps( + {"agent": {"id": "agent-collision", "name": "severianus"}} + ) + ) raise SystemExit(0) print(json.dumps({"error": "Agent not found"})) raise SystemExit(1) @@ -418,6 +429,7 @@ def command( lock_owner: str, *, agent: str = "Severianus", + agent_id: str = "agent-severianus", ) -> list[str]: return [ sys.executable, @@ -437,6 +449,8 @@ def command( str(self.body(pr_number, agent=agent)), "--lock-owner", lock_owner, + "--agent-id", + agent_id, "--git-remote", str(self.remote), ] @@ -531,6 +545,29 @@ def test_missing_agent_trailer_fails_postverification(self) -> None: output["blocking_reasons"], ) + def test_duplicate_display_names_use_exact_agent_id(self) -> None: + with TemporaryDirectory() as directory: + fixture = MergeFixture(Path(directory)) + result = subprocess.run( + fixture.command( + 1, + "severianus/session-a", + agent_id="agent-severianus", + ), + cwd=fixture.root, + env=fixture.env(), + check=False, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + + self.assertEqual(result.returncode, 0, result.stderr) + output = json.loads(result.stdout) + self.assertEqual(output["status"], "merged_verified") + self.assertEqual(output["agent_identity_id"], "agent-severianus") + self.assertEqual(output["agent_identity"], "Severianus") + def test_wrong_or_unresolved_agent_identity_refuses_before_merge(self) -> None: cases = ( ("OtherAgent", "severianus/session-a"), From 532645168b2dccfd37dd24b72726ca857608bed4 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Thu, 13 Aug 2026 15:33:07 +0300 Subject: [PATCH 3/3] fix(merge-pr): reject indented agent trailers Preserve nonblank merge-body lines during attribution validation so only an exact final Agent trailer can pass pre- and postverification. Agent: unresolved-account002 --- .../merge-pr/scripts/merge_pr_execute.py | 6 ++--- .../merge-pr/tests/test_merge_pr_execute.py | 26 +++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/.codewith/skills/merge-pr/scripts/merge_pr_execute.py b/.codewith/skills/merge-pr/scripts/merge_pr_execute.py index 5268e99aa..a632c7de4 100755 --- a/.codewith/skills/merge-pr/scripts/merge_pr_execute.py +++ b/.codewith/skills/merge-pr/scripts/merge_pr_execute.py @@ -53,8 +53,8 @@ def validate_sha(value: str, name: str) -> None: def validate_body_file(path: Path) -> str: if not path.is_file(): raise ValueError(f"merge body file does not exist: {path}") - lines = [line.strip() for line in path.read_text(encoding="utf-8").splitlines()] - nonempty = [line for line in lines if line] + lines = path.read_text(encoding="utf-8").splitlines() + nonempty = [line for line in lines if line.strip()] if not nonempty or not AGENT_TRAILER_PATTERN.fullmatch(nonempty[-1]): raise ValueError("merge body must end with one Agent: trailer") if sum(1 for line in nonempty if line.startswith("Agent: ")) != 1: @@ -87,7 +87,7 @@ def resolve_registered_agent(agent_id: str, lock_owner: str) -> str: def agent_trailer_blockers(message: str, expected_agent: str) -> list[str]: - nonempty = [line.strip() for line in message.splitlines() if line.strip()] + nonempty = [line for line in message.splitlines() if line.strip()] trailers = [line for line in nonempty if line.startswith("Agent: ")] expected = f"Agent: {expected_agent}" blockers = [] diff --git a/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py b/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py index 5508576a2..3fa7c650d 100644 --- a/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py +++ b/.codewith/skills/merge-pr/tests/test_merge_pr_execute.py @@ -545,6 +545,32 @@ def test_missing_agent_trailer_fails_postverification(self) -> None: output["blocking_reasons"], ) + def test_indented_agent_trailer_refuses_before_merge(self) -> None: + with TemporaryDirectory() as directory: + fixture = MergeFixture(Path(directory)) + command = fixture.command(1, "severianus/session-a") + body_file = Path(command[command.index("--body-file") + 1]) + body_file.write_text( + "Merge fixture PR 1.\n\n Agent: Severianus\n", + encoding="utf-8", + ) + result = subprocess.run( + command, + cwd=fixture.root, + env=fixture.env(), + check=False, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + + self.assertEqual(result.returncode, 64, result.stderr) + output = json.loads(result.stdout) + self.assertEqual(output["status"], "invalid_arguments") + self.assertIn("merge body must end", output["error"]) + self.assertEqual(fixture.mutation_rows(), []) + self.assertEqual(fixture.merge_call_rows(), []) + def test_duplicate_display_names_use_exact_agent_id(self) -> None: with TemporaryDirectory() as directory: fixture = MergeFixture(Path(directory))