Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .codewith/skills/merge-pr/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <pr> --match-head-commit <head_sha>` 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 both `--lock-owner registered-agent/session-id` and the stable exact Todos identity as `--agent-id <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 <pr> --match-head-commit <head_sha>` and must never include `--admin`.

## Workflow

Expand All @@ -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
Expand Down Expand Up @@ -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, 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`.
5 changes: 3 additions & 2 deletions .codewith/skills/merge-pr/references/merge-safety.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ 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 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 <pr> --match-head-commit <head_sha>`. This protects against merging a changed PR head after review.
- `--match-head-commit` pins only the PR head. GitHub's supported merge input exposes no expected base OID, so the branch lock serializes participating merge-pr executors but cannot exclude a nonparticipating external actor. Postverify must continue comparing the merged commit's first parent and tree with the reviewed base and tree, and must report any mismatch rather than treating the merge command's success as completion.
- `--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.
70 changes: 65 additions & 5 deletions .codewith/skills/merge-pr/scripts/merge_pr_execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"


Expand Down Expand Up @@ -49,17 +50,54 @@ 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()]
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: <registered-name> trailer")
if sum(1 for line in nonempty if line.startswith("Agent: ")) != 1:
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(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", agent_id])
if error:
raise ValueError(
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 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


def agent_trailer_blockers(message: str, expected_agent: str) -> list[str]:
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 = []
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(
Expand Down Expand Up @@ -206,13 +244,17 @@ 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 {
"target": target or "",
"merge_commit": resolved_merge or "",
"parent": parent or "",
"tree": tree or "",
"message": message or "",
}, None


Expand Down Expand Up @@ -298,12 +340,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.agent_id, 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:
Expand Down Expand Up @@ -641,6 +689,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",
Expand All @@ -655,6 +706,8 @@ 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,
expected_agent_id=args.agent_id,
blocking_reasons=post_blockers,
)
return 6
Expand All @@ -672,6 +725,8 @@ 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,
agent_identity_id=args.agent_id,
merge_command=merge_command,
)
return 0
Expand Down Expand Up @@ -722,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,
Expand Down
Loading
Loading