diff --git a/install.sh b/install.sh index 9bb587c3..2a42a277 100755 --- a/install.sh +++ b/install.sh @@ -145,7 +145,7 @@ copy_file() { fi } -for script in super-board-run.sh super-board-gh-guard.sh super-board-status.py super-board-wave-plan.sh super-board-deps.sh super-board-preflight.sh super-board-merge-gate.sh super-board-merge-policy.py super-board-env-check.sh super-board-agents-md.py super-board-settings.py super-board-setup.py super-board-usage.sh super-board-pr-body.sh super-review-file-refactor.sh super-qa-file-bug.sh super-board-stop.sh; do +for script in super-board-run.sh super-board-gh-guard.sh super-board-status.py super-board-wave-plan.sh super-board-deps.sh super-board-preflight.sh super-board-merge-gate.sh super-board-merge-policy.py super-board-approval.py super-board-env-check.sh super-board-agents-md.py super-board-settings.py super-board-setup.py super-board-usage.sh super-board-pr-body.sh super-review-file-refactor.sh super-qa-file-bug.sh super-board-stop.sh; do if [ -f "$REPO_ROOT/scripts/$script" ]; then copy_file "$REPO_ROOT/scripts/$script" "$TARGET/.claude/bin/$script" chmod +x "$TARGET/.claude/bin/$script" diff --git a/scripts/super-board-approval.py b/scripts/super-board-approval.py new file mode 100644 index 00000000..c48ce87e --- /dev/null +++ b/scripts/super-board-approval.py @@ -0,0 +1,248 @@ +#!/usr/bin/env python3 +"""Read-only, head-bound human approval shared by the planner and merge gate. + +A trusted request pins the code and human steps BEFORE a human replies `done`. +Labels only describe UI state. They never authorize a merge. No GitHub writes. +""" +from __future__ import annotations + +import argparse +from datetime import datetime +import hashlib +import json +import re +import subprocess +import sys + +PREFIX = "approval-request: " +SHA = re.compile(r"[0-9a-f]{40}\Z") +SCOPE = re.compile(r"[0-9a-f]{64}\Z") +BLOCK = re.compile(r"Reason tag:[^\n]*๐Ÿ™‹|^approval-request:", re.M) +DONE = re.compile(r"\s*done[.!]?\s*\Z", re.I) + + +def scope_for(plan): + # Exclude UI-only `done`; a label change must not change what was approved. + relevant = {key: plan.get(key, []) for key in ("human", "migrations", "run", "needs_you")} + return hashlib.sha256(json.dumps(relevant, sort_keys=True, separators=(",", ":")).encode()).hexdigest() + + +def request_for(repo, pr, head, scope): + return {"repo": repo.lower(), "pr": int(pr), "head": head, "scope": scope} + + +def marker(request): + return PREFIX + json.dumps(request, sort_keys=True, separators=(",", ":")) + + +def parse_request(comment): + lines = [line for line in (comment.get("body") or "").splitlines() if line.startswith(PREFIX)] + if len(lines) != 1: + return None + try: + value = json.loads(lines[0][len(PREFIX):]) + if (not isinstance(value, dict) or not isinstance(value.get("repo"), str) + or not re.fullmatch(r"[^/\s]+/[^/\s]+", value["repo"]) + or type(value.get("pr")) is not int or value["pr"] < 1 + or not SHA.fullmatch(value.get("head", "")) + or not SCOPE.fullmatch(value.get("scope", ""))): + return None + return value + except (ValueError, TypeError): + return None + + +def timestamp(value): + if not isinstance(value, str): + raise ValueError("missing comment timestamp") + result = datetime.fromisoformat(value.replace("Z", "+00:00")) + if result.tzinfo is None: + raise ValueError("comment timestamp lacks timezone") + return result + + +def trusted_blocks(comments, permission): + result = [] + for block in comments: + if not BLOCK.search(block.get("body") or ""): + continue + user = block.get("user") or {} + if not user.get("login") or user.get("type") not in ("User", "Bot"): + raise ValueError("requester identity could not be verified") + if permission(user["login"]) in ("write", "maintain", "admin"): + result.append(block) + return result + + +def validate(comments, expected, permission): + """Pure verdict; permission(login) is a current repository permission lookup.""" + def hold(why): + return {"approved": False, "why": why} + + if not SHA.fullmatch(expected.get("head", "")): + return hold("current PR head is unavailable") + try: + blocks = trusted_blocks(comments, permission) + if not blocks: + return hold("no request from a trusted repository collaborator") + # Any later human block supersedes the old request, including one with + # missing/malformed metadata. Equal-second requests are ambiguous: hold. + latest_time = max(timestamp(c.get("created_at")) for c in blocks) + latest = [c for c in blocks if timestamp(c.get("created_at")) == latest_time] + if len(latest) != 1: + return hold("ambiguous newest human request") + request_comment = latest[0] + request = parse_request(request_comment) + if request is None or any(request.get(k) != v for k, v in expected.items()): + return hold("the latest request does not cover the current code and human steps") + if timestamp(request_comment.get("updated_at")) != latest_time: + return hold("approval request was edited; post a fresh request") + requester = request_comment.get("user") or {} + if not requester.get("login") or permission(requester["login"]) not in ("write", "maintain", "admin"): + return hold("requester authority could not be verified") + if not isinstance(request_comment.get("source"), int) or not request_comment.get("id"): + return hold("request identity is missing") + for comment in comments: + if (not comment.get("id") or not DONE.fullmatch(comment.get("body") or "") + or comment.get("source") != request_comment.get("source")): + continue + created = timestamp(comment.get("created_at")) + if created <= latest_time or timestamp(comment.get("updated_at")) != created: + continue + author = comment.get("user") or {} + if (author.get("type") == "User" and author.get("login") + and permission(author["login"]) in ("write", "maintain", "admin")): + return {"approved": True, "why": "trusted human confirmed the current request", + "request": request, "requestId": request_comment.get("id"), + "approvalId": comment.get("id"), "approver": author["login"]} + return hold("waiting for a trusted human to comment done on the current request") + except (ValueError, TypeError, KeyError): + return hold("approval evidence is incomplete or unreadable") + + +class GitHub: + def __init__(self, repo): + if not re.fullmatch(r"[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+", repo): + raise ValueError("missing or invalid repository") + self.repo = repo.lower() + self.permissions = {} + + @staticmethod + def read(*args): + result = subprocess.run(["gh", *args], capture_output=True, text=True, timeout=30) + if result.returncode: + raise ValueError("GitHub evidence could not be read") + return json.loads(result.stdout) + + def permission(self, login): + if not re.fullmatch(r"[A-Za-z0-9_-]+(?:\[bot\])?", login): + return None + if login not in self.permissions: + value = self.read("api", f"repos/{self.repo}/collaborators/{login}/permission") + self.permissions[login] = value.get("permission") + return self.permissions[login] + + def comments(self, number): + pages = self.read("api", "--paginate", "--slurp", + f"repos/{self.repo}/issues/{number}/comments?per_page=100") + if not isinstance(pages, list) or not pages or any(not isinstance(p, list) for p in pages): + raise ValueError("unreadable comments") + if any(not isinstance(c, dict) for page in pages for c in page): + raise ValueError("unreadable comment") + return [dict(comment, source=number) for page in pages for comment in page] + + def pr(self, number): + # Paginate linked issues too: an omitted newer request must never make + # an older approval authoritative. Remote-repository links are ignored. + query = '''query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){ + repository(owner:$owner,name:$repo){pullRequest(number:$pr){headRefOid state + closingIssuesReferences(first:100,after:$endCursor){ + pageInfo{hasNextPage endCursor} nodes{number repository{nameWithOwner}} + }}}}''' + owner, name = self.repo.split("/") + pages = self.read("api", "graphql", "--paginate", "--slurp", "-f", f"query={query}", + "-F", f"owner={owner}", "-F", f"repo={name}", "-F", f"pr={number}") + heads, issues = set(), set() + if not isinstance(pages, list) or not pages: + raise ValueError("unreadable PR") + for page in pages: + if page.get("errors"): + raise ValueError("incomplete PR response") + pr = page["data"]["repository"]["pullRequest"] + if pr["state"] != "OPEN": + raise ValueError("PR is not open") + heads.add(pr["headRefOid"]) + links = pr["closingIssuesReferences"] + for issue in links["nodes"]: + if issue["repository"]["nameWithOwner"].lower() == self.repo: + issues.add(issue["number"]) + if links["pageInfo"]["hasNextPage"] or len(heads) != 1: + raise ValueError("PR changed or issue links are incomplete") + head = heads.pop() + if not SHA.fullmatch(head): + raise ValueError("unreadable PR head") + return head, issues + + def check(self, number, head=None, scope=None, issue=None): + current_head, issues = self.pr(number) + if issue is not None and issue not in issues: + return {"approved": False, "why": "request issue is not linked to this PR"} + if head is not None and current_head != head: + return {"approved": False, "headChanged": True, "why": "PR head changed after review"} + comments = [] + for source in sorted(issues | {number}): + comments.extend(self.comments(source)) + expected = {"repo": self.repo, "pr": number, "head": current_head} + if scope is not None: + expected["scope"] = scope + return validate(comments, expected, self.permission) + + +def main(): + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("--repo", required=True) + ap.add_argument("--pr", type=int) + ap.add_argument("--head") + ap.add_argument("--plan", help="JSON plan from merge-policy") + ap.add_argument("--request", action="store_true", help="print a pinned request; no network or writes") + ap.add_argument("--deps", action="store_true", help="enrich the dependency graph on stdin") + args = ap.parse_args() + if args.request: + if not args.pr or not args.head or not SHA.fullmatch(args.head) or not args.plan: + ap.error("request requires --pr, full --head and --plan") + print(marker(request_for(args.repo, args.pr, args.head, scope_for(json.loads(args.plan))))) + return + if args.deps: + graph = json.load(sys.stdin) + github = GitHub(args.repo) if args.repo else None + for entry in graph.values(): + entry["needsYouDone"] = False + entry["approvalRefresh"] = False + if not entry.get("needsYou") or github is None: + continue + try: + comments = github.comments(entry["number"]) + blocks = trusted_blocks(comments, github.permission) + latest = max(blocks, key=lambda c: timestamp(c.get("created_at"))) if blocks else {} + request = parse_request(latest) + if request is None or request["repo"] != github.repo: + continue + result = github.check(request["pr"], head=request["head"], issue=entry["number"]) + entry["needsYouDone"] = result["approved"] + entry["approvalRefresh"] = result.get("headChanged", False) + entry["approvalWhy"] = result["why"] + except (ValueError, KeyError, TypeError, OSError, subprocess.SubprocessError): + entry["approvalWhy"] = "approval evidence could not be verified" + print(json.dumps(graph)) + return + if not args.pr or not args.head or not args.plan: + ap.error("check requires --pr, --head and --plan") + try: + result = GitHub(args.repo).check(args.pr, args.head, scope_for(json.loads(args.plan))) + except (ValueError, KeyError, TypeError, OSError, subprocess.SubprocessError): + result = {"approved": False, "why": "approval evidence could not be verified"} + print(json.dumps(result)) + + +if __name__ == "__main__": + main() diff --git a/scripts/super-board-deps.sh b/scripts/super-board-deps.sh index c30eff3b..523e9611 100755 --- a/scripts/super-board-deps.sh +++ b/scripts/super-board-deps.sh @@ -48,9 +48,9 @@ # # `needsYou` โ€” the newest Block comment carries the ๐Ÿ™‹ reason tag, or the issue # has the `needs-you` label: a human-only command is waiting (block-template.md). -# `needsYouDone` โ€” the human said it is done: the issue has the `needs-you:done` -# label, or a comment AFTER that ๐Ÿ™‹ comment reads just "done". The wave planner -# reports these in `resume`; they go back to Review and the merge gate re-runs. +# `needsYouDone` โ€” verified by super-board-approval.py: a trusted human replied +# done after a pinned request for the current PR head. Labels are never authority. +# Saved --from payloads stay offline and cannot assert verified human approval. # A ๐Ÿ™‹ card stays humanGated either way โ€” only the resume path moves it. # # `runnable` is true only when the line parses, no blocker is still open, AND the @@ -93,6 +93,8 @@ else echo "could not read issues for $REPO" >&2; exit 69; } fi +APPROVAL_REPO="$REPO" +[ -z "$FROM" ] || APPROVAL_REPO="" echo "$ISSUES" | jq --arg only "$ONLY" ' # ---- the parser --------------------------------------------------------- # Everything after the LAST "## Blocked by" heading, stopping at the next @@ -144,10 +146,7 @@ echo "$ISSUES" | jq --arg only "$ONLY" ' def needs_at: ( bodies | to_entries | map(select(.value | test("Reason tag:[^\n]*๐Ÿ™‹"))) | last | .key ) // null; def needs_you: (label_names | index("needs-you") != null) or (needs_at != null); - def needs_you_done: - (label_names | index("needs-you:done") != null) - or ( needs_at as $at | $at != null - and ( bodies[($at + 1):] | any(.[]; test("^[ \t\n]*done[.!]?[ \t\n]*$"; "i")) ) ); + ( [ .[] | .number ] ) as $open | ( if $only == "" then null else ($only / "," | map(tonumber)) end ) as $filter @@ -190,7 +189,7 @@ echo "$ISSUES" | jq --arg only "$ONLY" ' runnable: ($p.parseable and ($p.human_gated | not) and (($stillOpen | length) == 0)), why: $p.why, needsYou: ($i | needs_you), - needsYouDone: (($i | needs_you) and ($i | needs_you_done)) } + needsYouDone: false } ] | ( if $filter == null then . else map(select(.number as $n | $filter | index($n))) end ) - | INDEX(.number | tostring)' + | INDEX(.number | tostring)' | python3 "$(dirname "${BASH_SOURCE[0]}")/super-board-approval.py" --deps --repo "$APPROVAL_REPO" diff --git a/scripts/super-board-merge-gate.sh b/scripts/super-board-merge-gate.sh index 91d46a01..63e145d8 100755 --- a/scripts/super-board-merge-gate.sh +++ b/scripts/super-board-merge-gate.sh @@ -81,14 +81,14 @@ # review" โ€” or merge_policy.default "human"). Stdout lists each # `human-gate: โ€” `. Nothing ran, nothing merged; the # card โ†’ Blocked with the ๐Ÿ™‹ template: the human reviews and merges it, or -# comments "done" (label needs-you:done) to approve โ€” the next wave re-runs +# comments "done" after its pinned request to approve โ€” the next wave re-runs # the gate, which then skips the policy check and merges. # 8 ๐Ÿ™‹ needs you โ€” the PR has migrations for a database the robot may not # touch (merge_policy โ†’ migrations.allowed_envs), an allowed migrate command # failed, or a human-only step is declared (`needs-you:` line in the PR body, # or migrations.human_steps). Stdout lists the exact commands as # `needs-you: ` lines. Card โ†’ Blocked with the ๐Ÿ™‹ template; once the -# PR carries the `needs-you:done` label the next wave re-runs the gate and it +# current request has verified trusted-human approval, the gate re-runs and # merges. # # MERGE POLICY AND MIGRATIONS (config, all optional โ€” defaults shown in @@ -218,14 +218,27 @@ PLAN=$(python3 "$HERE/super-board-merge-policy.py" --config "$CONFIG" --meta "$M echo "human-gate: policy โ€” could not classify the PR (unreadable metadata)" say "merge policy could not be evaluated โ€” a human merges this one"; exit 7; } +# The label is display-only. A request posted before `done` pins the exact head +# and policy/commands. Re-read trusted approval evidence; never mint it here. +APPROVED=false +approval_check() { + APPROVAL=$(python3 "$HERE/super-board-approval.py" --repo "$REPO" --pr "$PR" \ + --head "$HEAD_SHA" --plan "$PLAN") || APPROVAL='{"approved":false}' + APPROVED=$(echo "$APPROVAL" | jq -r '.approved // false') +} +approval_request() { + python3 "$HERE/super-board-approval.py" --request --repo "$REPO" --pr "$PR" \ + --head "$HEAD_SHA" --plan "$PLAN" +} HUMAN=$(echo "$PLAN" | jq -r '.human[] | "human-gate: \(.category) โ€” \(.why)"') -if [ -n "$HUMAN" ] && [ "$(echo "$PLAN" | jq -r '.done')" = "true" ]; then - say "needs-you:done is on the PR โ€” a human approved the policy gate ($(echo "$PLAN" | jq -r '[.human[].category] | join(", ")'))" - HUMAN="" +if [ -n "$HUMAN" ] || [ "$(echo "$PLAN" | jq '.needs_you | length')" -gt 0 ]; then + approval_check fi -if [ -n "$HUMAN" ]; then +if [ -n "$HUMAN" ] && [ "$APPROVED" != true ]; then echo "$HUMAN" - say "merge policy: a human merges this PR"; exit 7 + echo "$PLAN" | jq -r '.needs_you[] | "needs-you: " + .' + approval_request + say "merge policy: waiting for a trusted human to approve this exact head"; exit 7 fi git -C "$REPO_PATH" fetch origin "$HEAD_REF" "$BASE" --quiet @@ -275,15 +288,32 @@ if [ "$(echo "$PLAN" | jq '.migrations | length')" -gt 0 ]; then done < <(echo "$PLAN" | jq -r '.run[] | [.env, .cmd] | @tsv') fi if [ "${#NEEDS[@]}" -gt 0 ]; then - if [ "$(echo "$PLAN" | jq -r '.done')" = "true" ] && [ "$(echo "$PLAN" | jq '.needs_you | length')" -eq "${#NEEDS[@]}" ]; then - say "needs-you:done is on the PR โ€” the human steps are confirmed; merging" + if [ "$APPROVED" = true ] && [ "$(echo "$PLAN" | jq '.needs_you | length')" -eq "${#NEEDS[@]}" ]; then + say "a trusted human confirmed the current head and human steps" else for n in "${NEEDS[@]}"; do echo "needs-you: $n"; done + approval_request say "๐Ÿ™‹ needs you before this merges โ€” card โ†’ Blocked with the commands above" exit 8 fi fi +# An approval can be superseded while verification/migrations run. Re-read just +# before merging; GitHub independently pins the code with --match-head-commit. +if [ "$APPROVED" = true ]; then + # Code remains pinned; body-declared human steps and config can change without + # a commit. Reclassify them too instead of reusing the old approval scope. + gh pr view "$PR" ${REPO:+--repo "$REPO"} --json labels,files,additions,deletions,body > "$META" 2>/dev/null || echo '{}' > "$META" + PLAN=$(python3 "$HERE/super-board-merge-policy.py" --config "$CONFIG" --meta "$META" --diff "$DIFF") || { + say "human approval scope could not be refreshed"; exit 7; } + approval_check + if [ "$APPROVED" != true ]; then + approval_request + say "human approval changed during verification; keep the card Blocked" + if [ -n "$HUMAN" ]; then exit 7; else exit 8; fi + fi +fi + # ---- the merge ------------------------------------------------------------- if [ "$DRY" -eq 1 ]; then say "dry run: would squash-merge PR #${PR}" diff --git a/scripts/super-board-merge-policy.py b/scripts/super-board-merge-policy.py index 52484a42..167a71eb 100755 --- a/scripts/super-board-merge-policy.py +++ b/scripts/super-board-merge-policy.py @@ -17,7 +17,8 @@ "done": false } `human` non-empty โ†’ the gate exits 7 (a human merges). `needs_you` non-empty โ†’ -exit 8 unless `done` (the PR carries the `needs-you:done` label). Exit 2 on +exit 8 unless the gate verifies a head-bound approval. `done` stays false for +compatibility: labels never supply authority. Exit 2 on unreadable metadata, so the gate fails safe to "human". Rules, from references/config-schema.json โ†’ merge_policy / migrations: @@ -68,7 +69,6 @@ "**/migrations/*.sql", "db/migrate/**", "alembic/versions/**", ] DEFAULT_ALLOWED_ENVS = ["test", "staging"] -DONE_LABEL = "needs-you:done" DEFAULT_AUTO_MAX_LINES = 400 DEFAULT_SIZE_EXCLUDE = [ # lockfiles @@ -193,7 +193,7 @@ def main() -> int: needs.append(m.group(1).strip("`")) print(json.dumps({"human": human, "migrations": mig_files, "run": run, - "needs_you": needs, "done": DONE_LABEL in labels})) + "needs_you": needs, "done": False})) return 0 diff --git a/scripts/super-board-setup.py b/scripts/super-board-setup.py index c6935fd8..880d70b1 100755 --- a/scripts/super-board-setup.py +++ b/scripts/super-board-setup.py @@ -65,7 +65,7 @@ OLD_SKILL_DIRS = ["super-refine", "cleanup-wt", "arch-loop"] BIN = ["super-board-run.sh", "super-board-gh-guard.sh", "super-board-status.py", "super-board-wave-plan.sh", "super-board-deps.sh", "super-board-preflight.sh", "super-board-merge-gate.sh", - "super-board-merge-policy.py", "super-board-env-check.sh", "super-board-agents-md.py", + "super-board-merge-policy.py", "super-board-approval.py", "super-board-env-check.sh", "super-board-agents-md.py", "super-board-settings.py", "super-board-setup.py", "super-board-usage.sh", "super-board-pr-body.sh", "super-review-file-refactor.sh", "super-qa-file-bug.sh", "super-board-stop.sh"] WORKFLOWS = ["super-board-wave.js", "ui-refine-loop.js"] diff --git a/scripts/super-board-wave-plan.sh b/scripts/super-board-wave-plan.sh index 426a1761..35881fb6 100755 --- a/scripts/super-board-wave-plan.sh +++ b/scripts/super-board-wave-plan.sh @@ -67,6 +67,7 @@ # { "cards": [ {"number":10,"status":"Review","title":"โ€ฆ","lane":"review","labels":[]} ], # "sweep": [ {"number":37,"title":"โ€ฆ","clearedBy":[32]} ], # "resume": [ {"number":51,"title":"โ€ฆ"} ], +# "refreshApproval": [ {"number":52,"title":"โ€ฆ","why":"PR head changed after review"} ], # "flag": [ {"number":82,"title":"โ€ฆ","why":"โ€ฆ"} ], # "stranded": [ {"number":44,"title":"โ€ฆ"} ] } set -euo pipefail @@ -166,15 +167,19 @@ echo "$ITEMS" | jq --argjson cols "$COLUMNS" --argjson cap "$MAX_WORKERS" --argj | { number, title, clearedBy: (dep(.number) | .blockers) } ], - # ๐Ÿ™‹ Blocked cards whose human step is confirmed done (needs-you:done - # label, or a "done" comment after the ๐Ÿ™‹ block). The orchestrator moves - # them to Review and labels the PR needs-you:done; the Reviewer re-runs - # the merge gate, which re-verifies and merges. + # Verified current-head human approval from the dependency helper. The + # Reviewer independently rechecks it; labels are only UI state. resume: [ $all[] | select(.status == "Blocked") | select(dep(.number) != null and (dep(.number).needsYouDone // false)) | { number, title } ], + # Changed code while Blocked needs a new review/request, never approval. + refreshApproval: [ $all[] + | select(.status == "Blocked") + | select(dep(.number).approvalRefresh // false) + | { number, title, why: dep(.number).approvalWhy } ], + # Cards the graph could not read. Left where they are, reported so the # orchestrator can ask for the line to be fixed. flag: [ $all[] diff --git a/skills/super-board/references/block-template.md b/skills/super-board/references/block-template.md index 4edadfc8..49af3e61 100644 --- a/skills/super-board/references/block-template.md +++ b/skills/super-board/references/block-template.md @@ -20,7 +20,7 @@ record of what they were waiting for was English prose in a comment nobody re-re ## Required Block comment template (mandatory on every transition into Blocked) -The bot must write a structured comment on **both the issue and the PR** (if a PR exists) explaining *why* it moved the card and *what it couldn't safely decide*. Format: +The bot must write a structured comment on **both the issue and the PR** (if a PR exists) explaining *why* it moved the card and *what it couldn't safely decide*. Exception: a ๐Ÿ™‹ merge approval uses one canonical issue request below; the PR links to it without duplicating its machine lines. Format: ``` [] [blocker] ๐Ÿ›‘ blocked ยท @@ -101,14 +101,15 @@ an allowed migrate command that failed, a declared human step), or any lane that Checklist first: the person sees what to do before why. The why and the evidence fold away. Merge gate exit 7 (a human merges) has one item before `done`: `- [ ] Review and merge PR #

-(or comment done to approve it)`. +(or comment done to approve it)`. Include any `needs-you:` commands printed with +that policy hold too: the approval covers those human steps as well as the code. ``` [reviewer] [blocker] ๐Ÿ™‹ Your turn on # โ€” - [ ] Run `<exact command 1, copy-paste ready>` on the **<env>** database - [ ] <exact command 2, if any> - [ ] Comment `done` here -After `done`, the next wave moves the card to Review and merges it. +After a trusted human confirms this version, the next wave returns it to Review for verification. <details><summary>Why, and what I checked</summary> @@ -116,6 +117,7 @@ PR #<P> <one line โ€” e.g. "adds prisma/migrations/0042_add_plan">. <env> isn't Tests green on <base>@<sha>; migrated <test, staging>. Evidence: <the gate's `needs-you:` lines, verbatim> Reason tag: ๐Ÿ™‹ needs you ยท Owner: <Eric | repo admin> +approval-request: <copy the gate's exact JSON here> blocked-by: - </details> ``` @@ -126,7 +128,7 @@ Example (what the person sees on GitHub): > - [ ] Run `npx prisma migrate deploy` on the **live** database > - [ ] Comment `done` here > -> After `done`, the next wave moves the card to Review and merges it. +> After a trusted human confirms this version, the next wave returns it to Review for verification. > โ–ธ Why, and what I checked The `Reason tag:` and `blocked-by: -` lines stay (inside the fold): the planner reads them @@ -137,12 +139,35 @@ never paraphrase ("run the migration on prod"). A placeholder such as `<your liv means the config has no command for that env โ€” say so in the fold and name the config key (`migrations.commands.live`). -**Resume.** The wave planner's `resume` list carries every ๐Ÿ™‹ card whose human said `done` (a -comment after the ๐Ÿ™‹ block that reads just `done`, or the `needs-you:done` label). The orchestrator -moves it to **Review** and puts `needs-you:done` on the PR; the Reviewer re-runs the merge gate, -which re-checks the head, re-verifies against the base, skips the merge-policy check (the human -approved), re-runs the allowed migrations, and merges. -A command that still fails puts the card straight back here. +**Approval request.** Copy the gate's `approval-request:` line verbatim into this +issue comment. It records the full PR head and the exact policy/human steps being +confirmed. The issue must be linked by the PR's closing reference (`Closes #N`). +For a standalone PR, post the canonical request on the PR instead. Keep a single +request location: the PR's status comment links to the issue request and does not +repeat `Reason tag: ๐Ÿ™‹` or `approval-request:`. Never edit a request after posting; +post a new one when the code, steps or actual human question changes. Do not +repost the same pending request on every poll or retry. Generic credential or +product blocks without a PR remain manual; a bare `done` does not automate them. + +**Resume.** A human with current repository write, maintain or admin permission +comments `done` after the newest request, in the same thread. This supports a solo +owner approving their own PR. The planner verifies identity, permission, linked +PR and current full head before putting the card in `resume`; the Reviewer then +re-runs the merge gate. The gate also verifies the current policy and human steps, +re-verifies against the base, re-runs allowed migrations, and rechecks approval +just before merging. Changed code, a newer human block, edited evidence, missing +permissions or unreadable GitHub evidence keep it on hold. A still-failing command +also sends it back here. Old bare comments and `needs-you:done` labels never count; +legacy blocked cards need one fresh pinned request and a fresh human reply. Move +those legacy cards to Review once to generate it. If a PR changes while still +Blocked, the planner's `refreshApproval` sends it to Review to create the new +request automatically; it does not mark the new code approved. + +**Trust boundary.** GitHub Bot accounts cannot supply human approval. GitHub cannot +distinguish a person from an agent using that person's token; agents must never +write the human's `done` response. The final approval check is a fresh snapshot; +GitHub atomically protects the head at merge, but does not lock issue comments. +A human can still merge a PR manually under the repository's normal rules. ## Hard rule diff --git a/skills/super-board/references/config-schema.json b/skills/super-board/references/config-schema.json index eee5d395..fb0c0897 100644 --- a/skills/super-board/references/config-schema.json +++ b/skills/super-board/references/config-schema.json @@ -151,9 +151,9 @@ // (๐Ÿ™‹ needs you โ†’ Blocked, block-template.md) instead when: target_env is not // in allowed_envs, an allowed command fails, target_env is allowed but has no // command, or human_steps is non-empty. A `needs-you: <command>` line in the - // PR body does the same for any PR. The `needs-you:done` PR label (or a - // "done" comment, which the orchestrator turns into the label) clears the - // human steps; the next wave re-verifies and merges. + // PR body does the same for any PR. A trusted human comments "done" after a + // pinned approval request for the current head and steps (block-template.md). + // The gate rechecks that evidence; needs-you:done labels never authorize it. // In one line, for the user: the robot migrates the DBs you allow to test its // work; a live database, or a destructive schema change, waits for you. "migrations": { diff --git a/skills/super-board/references/run-workflow.md b/skills/super-board/references/run-workflow.md index 5a0e85d5..691bbaea 100644 --- a/skills/super-board/references/run-workflow.md +++ b/skills/super-board/references/run-workflow.md @@ -84,13 +84,16 @@ Repeat until a done condition or halt gate fires: cut off mid-lane. The legacy `claude-p` dispatcher does not run this guard. 2. **Plan the wave** โ€” `bash .claude/bin/super-board-wave-plan.sh --config <config-path>` โ†’ - The planner returns `cards`, `sweep`, `resume`, `flag` and `stranded`. **Act on - `sweep`, `resume`, `flag` and `stranded` BEFORE launching** (run.md โ†’ "The wave-start + The planner returns `cards`, `sweep`, `resume`, `refreshApproval`, `flag` and `stranded`. **Act on + `sweep`, `resume`, `refreshApproval`, `flag` and `stranded` BEFORE launching** (run.md โ†’ "The wave-start sweep"): move every swept card to `Ready` with a comment naming what cleared - it, move every `resume` card (๐Ÿ™‹ needs you, human said done) to `Review` and - label its PR `needs-you:done`, comment on every flagged card asking for its + it, move every `resume` card (verified human approval of the current pinned + request) to `Review`; `needs-you:done` is display-only. Move `refreshApproval` + cards to `Review` solely to review the changed head and post a fresh human request; + clear their stale `needs-you:done` display labels. They have no approval to merge. + Comment on flagged cards asking for their `## Blocked by` line to be fixed, and return every stranded Building card to - `Ready` (below). Swept, resumed and stranded cards are NOT in this pass's + `Ready` (below). Swept, resumed, approval-refresh and stranded cards are NOT in this pass's `cards`; they join the next wave. **Stranded Building cards.** A card in Building with no assignee between diff --git a/skills/super-board/references/run.md b/skills/super-board/references/run.md index 4219669c..4b703868 100644 --- a/skills/super-board/references/run.md +++ b/skills/super-board/references/run.md @@ -449,16 +449,18 @@ on the base branch**. category and evidence; `To unblock` = "[ ] review PR #<P> and merge it yourself" OR "[ ] comment `done` to approve โ€” the next wave merges it"; label `needs-you`, `blocked-by: -`. A human merge moves the card to Done the - usual way; a `done` brings it back through `resume`, and the gate skips the - policy check once the PR carries `needs-you:done`. + usual way; a `done` brings it back through `resume`, and the gate clears the + policy hold only with a verified trusted approval of the current request. 8 โ†’ ๐Ÿ™‹ needs you. Stdout carries one `needs-you: <command>` line per human step (migration for an env outside `migrations.allowed_envs`, an allowed migrate command that failed, a `needs-you:` line in the PR body, `migrations.human_steps`). Card Review โ†’ **Blocked** with the ๐Ÿ™‹ template (block-template.md โ†’ "๐Ÿ™‹ Needs you"), the commands copied verbatim into `To unblock`, label `needs-you` on issue and PR, `blocked-by: -`. When the - human comments `done` (or labels `needs-you:done`), the wave planner's - `resume` list brings it back to Review and this gate re-runs. + human comments `done` after the current pinned request, the wave planner + verifies their permission and the head before resuming Review. For exits + 7 and 8, copy the gate's `approval-request:` line into the canonical issue + block; link it from the PR without duplicating the request. โ†’ do NOT leave a card in Review on exit 2, 3, 5, 7 or 8. A card left in Review is re-picked next tick and re-reviewed forever, which is the re-dispatch waste tracked in issue #10. Exits 4 and 6 are the exceptions: 4 simply queued, and @@ -648,7 +650,7 @@ usual template โ€” and, per ยง4, a `blocked-by:` line. ### The wave-start sweep -Before planning any wave, `super-board-wave-plan.sh` reports four lists the orchestrator must act on +Before planning any wave, `super-board-wave-plan.sh` reports five lists the orchestrator must act on **before** launching: - **`sweep`** โ€” `Blocked` cards whose blockers have all closed. Move each to `Ready` and comment @@ -657,12 +659,17 @@ Before planning any wave, `super-board-wave-plan.sh` reports four lists the orch so a wave stopped mid-build leaves them there forever. Remove any leftover build worktree, keep the branch, move the card to `Ready`, and comment naming the branch. The legacy dispatcher does the same once at start (`reclaim_stranded_building`). -- **`resume`** โ€” ๐Ÿ™‹ `Blocked` cards (merge gate exit 7 or 8) whose human step is confirmed: the `needs-you:done` label, or a - comment after the ๐Ÿ™‹ block that reads just `done`. Move each to **Review** (not Ready โ€” the code - was already reviewed), add `needs-you:done` to its PR, and comment `โ†ฉ๏ธ back to Review โ€” human step - confirmed; the merge gate re-verifies and merges. Next: Reviewer.` The Reviewer re-runs the gate, - which re-checks the head and the build, re-runs the allowed migrations and merges, or sends it - straight back to Blocked if a command still fails. +- **`resume`** โ€” ๐Ÿ™‹ `Blocked` cards whose newest pinned request has a later `done` + from a human with verified write, maintain or admin permission, for the current + PR head. Move each to **Review**; `needs-you:done` may be kept as a display label + only. Do not create or copy approval evidence. The Reviewer re-runs the gate, + which independently checks the request, current policy/steps and head, verifies + the build, and runs allowed migrations. New code or a newer request needs fresh + human approval; a label or old bare comment cannot bypass that hold. +- **`refreshApproval`** โ€” a PR head changed while its card was still `Blocked`. + Move it to **Review** and remove stale `needs-you:done` display labels. The Reviewer + reviews the new code and lets the gate produce a new pinned request, then returns + it to **Blocked** for a fresh human reply. This is not an approved resume. - **`flag`** โ€” cards whose `## Blocked by` section could not be parsed. Leave them where they are and comment asking for the line to be fixed, quoting the `why`. Never guess: a card treated as free on an unreadable line gets built against a base that does not have what it needs. diff --git a/skills/super-board/references/writing-standard.md b/skills/super-board/references/writing-standard.md index 123fe614..17973968 100644 --- a/skills/super-board/references/writing-standard.md +++ b/skills/super-board/references/writing-standard.md @@ -345,3 +345,12 @@ Tables plus CAPS rules (NEVER / DON'T / ALWAYS). No prose paragraphs. The manage - Numbers over adjectives: "1.2 s", not "fast". - Active voice: "QA found", not "it was found". - No em-dash chains. No rhetorical questions. No closing summary line. + +## Human approval evidence + +A ๐Ÿ™‹ merge hold has one canonical request comment on the linked issue (or on the PR +when it has no linked issue). Copy the gate's `approval-request:` line verbatim into +that comment; it pins the repository, PR, full head and policy/human-step scope. +Other threads link to that request without repeating its machine lines. A trusted +human replies `done` in that same thread. Labels are status display, never approval. +Do not edit the request or approval comment; changed code or steps need a fresh request. diff --git a/skills/super-review/SKILL.md b/skills/super-review/SKILL.md index 3aba4e72..963bc6d3 100644 --- a/skills/super-review/SKILL.md +++ b/skills/super-review/SKILL.md @@ -349,8 +349,10 @@ two complete builds. In order, no shortcuts: human-only step. Card โ†’ **Blocked** with the ๐Ÿ™‹ template (`block-template.md` โ†’ "๐Ÿ™‹ Needs you"), the gate's `needs-you:` commands copied verbatim into `To unblock`, label `needs-you`, `blocked-by: -`. After the human - comments `done` (or labels `needs-you:done`), the next wave moves it back to Review - and you re-run the gate; it re-verifies and merges. + comments `done` after the current pinned request, the next wave verifies their + permission and the head, then moves it to Review. Re-run the gate; labels do not + authorize it. For exits 7 and 8, copy `approval-request:` into the canonical + issue block and link it from the PR (no duplicate request); see block-template.md. - The rule in one line: the robot migrates the databases it was allowed to test its work; live databases, money, auth and destructive schema changes wait for a person. 3. **Confirm the merge landed** โ€” never trust the merge command's exit code: diff --git a/tests/test-deps.sh b/tests/test-deps.sh index efefcf9d..235675ef 100755 --- a/tests/test-deps.sh +++ b/tests/test-deps.sh @@ -104,11 +104,11 @@ get 24 '.blockers == [2] and .runnable == false' || fail "#24: the body should s get 1 '.blockers == [2]' || fail "a payload without a comments key must still parse" # 18 โ€” ๐Ÿ™‹ needs you. A ๐Ÿ™‹ Block comment marks the card needsYou and keeps it -# human-gated; "done" in a LATER comment, or the needs-you:done label, -# marks it needsYouDone. A "done" written before the block is history. +# human-gated. Bare done comments and labels cannot prove head/authority; +# only the live approval helper can mark needsYouDone (test_approval.py). get 25 '.needsYou == true and .needsYouDone == false and .runnable == false' || fail "#25 waits on a human" -get 26 '.needsYouDone == true' || fail "#26: a later 'Done' comment confirms the human step" -get 27 '.needsYouDone == true' || fail "#27: the needs-you:done label confirms the human step" +get 26 '.needsYouDone == false' || fail "#26: a bare done with no verified head/request cannot authorize" +get 27 '.needsYouDone == false' || fail "#27: a label alone cannot authorize" get 28 '.needsYouDone == false' || fail "#28: a 'done' before the ๐Ÿ™‹ block must not count" get 26 '.runnable == false' || fail "#26: done goes through resume, never the Ready sweep" get 2 '.needsYou == false and .needsYouDone == false' || fail "an ordinary card is not needsYou" diff --git a/tests/test-merge-gate.sh b/tests/test-merge-gate.sh index 79ab847a..0ad16b71 100755 --- a/tests/test-merge-gate.sh +++ b/tests/test-merge-gate.sh @@ -110,12 +110,27 @@ printf '%s\n' "$*" >> "$GH_LOG" case "$1 $2" in "pr view") case "$*" in *labels*) + if [ -n "${STUB_META_AFTER:-}" ] && [ -f "$STUB_VERIFIED" ]; then printf '%s\n' "$STUB_META_AFTER"; exit 0; fi if [ -n "${STUB_META:-}" ]; then printf '%s\n' "$STUB_META"; else echo '{"files":[],"labels":[]}'; fi exit 0 ;; esac oid="$STUB_OID"; grep -q '^pr merge' "$GH_LOG" && oid="${STUB_OID_AFTER:-$STUB_OID}" echo "feat $oid" ;; "pr merge") exit "${STUB_MERGE_RC:-0}" ;; "pr diff") printf '%b' "${STUB_DIFF:-}" ;; + "api graphql") + [ "${STUB_APPROVAL:-0}" = 1 ] || exit 1 + jq -n --arg head "$STUB_OID" '[{data:{repository:{pullRequest:{headRefOid:$head,state:"OPEN",closingIssuesReferences:{pageInfo:{hasNextPage:false,endCursor:null},nodes:[{number:42,repository:{nameWithOwner:"x/y"}}]}}}}}]' ;; + "api --paginate") + [ "${STUB_APPROVAL:-0}" = 1 ] || exit 1 + case "$*" in + */issues/42/comments*) cat "$STUB_COMMENTS" ;; + */issues/1/comments*) echo '[[]]' ;; + *) exit 1 ;; + esac ;; + "api repos/x/y/collaborators/owner/permission") + [ "${STUB_PERMISSION_FAIL:-0}" = 0 ] || exit 1 + echo '{"permission":"admin"}' ;; + *) exit 1 ;; esac STUB chmod +x "$TMP/bin/gh" @@ -259,10 +274,81 @@ RC=0; STUB_OID="$SHA" STUB_META="$(meta "" db/x.sql)" STUB_DIFF='-DROP TABLE use [ "$RC" -eq 0 ] || fail "a removed DROP TABLE must not gate, got $RC" teardown -# 16b โ€” a human approved the policy gate (needs-you:done): the gate merges. +# 16b โ€” a stale UI label cannot approve a policy gate. head_setup RC=0; STUB_OID="$SHA" STUB_META="$(meta money,needs-you:done src/x.ts)" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? -[ "$RC" -eq 0 ] || fail "needs-you:done should clear the policy gate, got $RC" +[ "$RC" -eq 7 ] || fail "a needs-you:done label alone must not authorize a policy bypass, got $RC" +teardown + +# 16c โ€” a trusted done confirms the request emitted for the exact current head. +# The request exists before the human comment; processing never binds old done +# to whatever head happens to exist now. +head_setup +export STUB_APPROVAL=1 STUB_COMMENTS="$TMP/comments.json" +echo '[[]]' > "$STUB_COMMENTS" +OUT=$(STUB_OID="$SHA" STUB_META="$(meta money src/x.ts)" gate --expect-head "$SHA" 2>/dev/null || true) +REQUEST=$(echo "$OUT" | sed -n 's/^approval-request: //p') +[ -n "$REQUEST" ] || fail "a hold must emit a pinned request" +make_approval() { + jq -n --argjson request "$REQUEST" '[[ + {id:1,body:("Reason tag: ๐Ÿ™‹ needs you\napproval-request: " + ($request|tojson)),created_at:"2026-10-03T00:00:01Z",updated_at:"2026-10-03T00:00:01Z",user:{login:"owner",type:"User"}}, + {id:2,body:"done",created_at:"2026-10-03T00:00:02Z",updated_at:"2026-10-03T00:00:02Z",user:{login:"owner",type:"User"}} + ]]' > "$STUB_COMMENTS" +} +make_approval +RC=0; STUB_OID="$SHA" STUB_META="$(meta money src/x.ts)" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? +[ "$RC" -eq 0 ] || fail "current trusted approval must merge, got $RC" +: > "$GH_LOG" +# A later code version cannot reuse the same human's done. +REQUEST=$(echo "$REQUEST" | jq '.head = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"') +make_approval +RC=0; STUB_OID="$SHA" STUB_META="$(meta money,needs-you:done src/x.ts)" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? +[ "$RC" -eq 7 ] || fail "old-head approval must hold, got $RC" +grep -q '^pr merge' "$GH_LOG" && fail "old approval must never reach merge" +# Neither unreadable authority nor a newer human block can authorize. +REQUEST=$(echo "$REQUEST" | jq --arg head "$SHA" '.head=$head') +make_approval +RC=0; STUB_PERMISSION_FAIL=1 STUB_OID="$SHA" STUB_META="$(meta money src/x.ts)" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? +[ "$RC" -eq 7 ] || fail "unreadable authority must hold, got $RC" +jq '.[0] += [{id:3,body:"Reason tag: ๐Ÿ™‹ new decision",created_at:"2026-10-03T00:00:03Z",updated_at:"2026-10-03T00:00:03Z",user:{login:"owner",type:"User"}}]' "$STUB_COMMENTS" > "$TMP/new.json" +mv "$TMP/new.json" "$STUB_COMMENTS" +RC=0; STUB_OID="$SHA" STUB_META="$(meta money,needs-you:done src/x.ts)" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? +[ "$RC" -eq 7 ] || fail "new human block must revoke prior approval, got $RC" +# Verify takes time: a same-head body edit adds a new human step during it. +make_approval +export STUB_VERIFIED="$TMP/verified" +jq --arg cmd "touch $STUB_VERIFIED" '.verify_commands=[$cmd]' "$TMP/c.json" > "$TMP/new.json" +mv "$TMP/new.json" "$TMP/c.json" +RC=0; STUB_META_AFTER="$(meta money src/x.ts 'needs-you: run a new command')" STUB_OID="$SHA" STUB_META="$(meta money src/x.ts)" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? +[ "$RC" -eq 7 ] || fail "changed human steps during verify must revoke approval, got $RC" +grep -q '^pr merge' "$GH_LOG" && fail "stale approval after verification must not merge" +unset STUB_APPROVAL STUB_COMMENTS STUB_VERIFIED +teardown + +# 16d โ€” head-bound approval also covers a declared human-only command; label +# changes cannot grant it, and a later human request during verification revokes it. +head_setup +export STUB_APPROVAL=1 STUB_COMMENTS="$TMP/comments.json" +echo '[[]]' > "$STUB_COMMENTS" +OUT=$(STUB_OID="$SHA" STUB_META="$(meta '' src/x.ts 'needs-you: confirm live setting')" gate --expect-head "$SHA" 2>/dev/null || true) +REQUEST=$(echo "$OUT" | sed -n 's/^approval-request: //p') +make_approval +RC=0; STUB_OID="$SHA" STUB_META="$(meta '' src/x.ts 'needs-you: confirm live setting')" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? +[ "$RC" -eq 0 ] || fail "trusted confirmation of current human step must merge, got $RC" +: > "$GH_LOG" +cat > "$TMP/new-block.py" <<'NEWBLOCK' +import json, os +p=os.environ["STUB_COMMENTS"] +x=json.load(open(p)) +x[0].append({"id":3,"body":"Reason tag: ๐Ÿ™‹ a new human decision","created_at":"2026-10-03T00:00:03Z","updated_at":"2026-10-03T00:00:03Z","user":{"login":"owner","type":"User"}}) +with open(p,"w") as f: json.dump(x,f) +NEWBLOCK +jq --arg cmd "python3 $TMP/new-block.py" '.verify_commands=[$cmd]' "$TMP/c.json" > "$TMP/new.json" +mv "$TMP/new.json" "$TMP/c.json" +RC=0; STUB_OID="$SHA" STUB_META="$(meta '' src/x.ts 'needs-you: confirm live setting')" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? +[ "$RC" -eq 8 ] || fail "a newer human request during verification must hold, got $RC" +grep -q '^pr merge' "$GH_LOG" && fail "revoked human approval must not reach merge" +unset STUB_APPROVAL STUB_COMMENTS teardown # 17 โ€” custom categories replace the defaults: with always_human = {} a money @@ -295,12 +381,12 @@ echo "$OUT" | grep -q "^needs-you: npm run db:migrate:live" || fail "exit 8 must grep -q '^pr merge' "$GH_LOG" && fail "no merge while a human step is open" teardown -# 20 โ€” the human ran it and the PR carries needs-you:done: re-verify, merge. +# 20 โ€” a UI label cannot confirm that human-only steps ran. head_setup cfgset ".migrations = {allowed_envs: [\"staging\"], target_env: \"live\", commands: {staging: \"true\", live: \"npm run db:migrate:live\"}}" RC=0; STUB_OID="$SHA" STUB_META="$(meta needs-you:done prisma/migrations/2026/migration.sql)" gate --expect-head "$SHA" >/dev/null 2>&1 || RC=$? -[ "$RC" -eq 0 ] || fail "needs-you:done should let the gate merge, got $RC" +[ "$RC" -eq 8 ] || fail "a needs-you:done label alone must not confirm human steps, got $RC" teardown # 21 โ€” an allowed migrate command that FAILS is a needs-you, and needs-you:done @@ -344,4 +430,4 @@ pol "$(meta "" a/b/migrations/1.sql)" | jq -e '.migrations | length == 1' > pol "$(meta "" migrations/old/1.sql)" | jq -e '.migrations | length == 0' >/dev/null || fail "* must not cross a /" rm -rf "$T25" -echo "PASS: test-merge-gate.sh (27 scenarios)" +echo "PASS: test-merge-gate.sh (merge, policy and approval scenarios)" diff --git a/tests/test-wave-plan.sh b/tests/test-wave-plan.sh index 516e187f..fa5d1a81 100755 --- a/tests/test-wave-plan.sh +++ b/tests/test-wave-plan.sh @@ -148,4 +148,9 @@ echo "$OUT" | jq -e '[.resume[].number] == [20]' >/dev/null \ echo "$OUT" | jq -e '[.sweep[].number, .cards[].number] | (index(20) == null and index(21) == null)' >/dev/null \ || fail "๐Ÿ™‹ cards must not be swept to Ready or dispatched" -echo "PASS: test-wave-plan.sh (18 scenarios)" +# 19 โ€” a moved head while Blocked goes to request refresh, never approved resume. +OUT19=$("$PLAN" --config <(echo "$NOCAP") --items "$ITEMS" --deps <(jq '.["21"].approvalRefresh=true | .["21"].approvalWhy="PR head changed"' "$DEPS")) +echo "$OUT19" | jq -e '[.refreshApproval[].number] == [21]' >/dev/null || fail "stale approval must get a fresh request" +echo "$OUT19" | jq -e '[.resume[].number, .cards[].number, .sweep[].number] | index(21) == null' >/dev/null || fail "refresh is not permission to merge or build" + +echo "PASS: test-wave-plan.sh (19 scenarios)" diff --git a/tests/test_approval.py b/tests/test_approval.py new file mode 100644 index 00000000..9b4fcce6 --- /dev/null +++ b/tests/test_approval.py @@ -0,0 +1,217 @@ +#!/usr/bin/env python3 +"""Offline approval lifecycle tests: no board writes, workers or databases.""" +import importlib.util +import json +import os +from pathlib import Path +import subprocess +import tempfile +import unittest + +ROOT = Path(__file__).resolve().parents[1] +spec = importlib.util.spec_from_file_location("approval", ROOT / "scripts/super-board-approval.py") +approval = importlib.util.module_from_spec(spec) +spec.loader.exec_module(approval) +HEAD = "a" * 40 +PLAN = {"human": [{"category": "auth", "why": "login"}], "needs_you": ["run live migration"]} +REQUEST = approval.request_for("x/y", 9, HEAD, approval.scope_for(PLAN)) + + +def comment(body, second, login="owner", actor="User", source=3): + at = f"2026-10-03T00:00:{second:02d}Z" + return {"id": second, "body": body, "created_at": at, "updated_at": at, + "user": {"login": login, "type": actor}, "source": source} + + +def evidence(): + return [comment("Reason tag: ๐Ÿ™‹ needs you\n" + approval.marker(REQUEST), 1), comment("done", 2)] + + +class ApprovalTests(unittest.TestCase): + def check(self, comments=None, expected=None, permissions=None): + return approval.validate(comments if comments is not None else evidence(), expected or REQUEST, + lambda login: (permissions or {"owner": "admin"}).get(login)) + + def test_current_request_allows_solo_owner(self): + self.assertTrue(self.check()["approved"]) + + def test_old_approval_never_covers_new_head(self): + self.assertFalse(self.check(expected=dict(REQUEST, head="b" * 40))["approved"]) + + def test_done_before_processing_does_not_get_rebound_to_new_head(self): + records = evidence() + self.assertFalse(self.check(records, dict(REQUEST, head="c" * 40))["approved"]) + + def test_changed_human_steps_revoke_same_head(self): + newer = dict(REQUEST, scope=approval.scope_for(dict(PLAN, needs_you=["different operation"]))) + self.assertFalse(self.check(expected=newer)["approved"]) + + def test_label_only_or_bare_done_is_not_approval(self): + self.assertFalse(self.check([comment("done", 2)])["approved"]) + + def test_new_block_invalidates_old_done_even_without_marker(self): + self.assertFalse(self.check(evidence() + [comment("Reason tag: ๐Ÿ™‹ needs a new decision", 3)])["approved"]) + + def test_new_request_requires_new_done(self): + newer = evidence() + [comment(approval.marker(REQUEST), 3)] + self.assertFalse(self.check(newer)["approved"]) + newer.append(comment("Done!", 4)) + self.assertTrue(self.check(newer)["approved"]) + + def test_wrong_pr_or_repository_is_rejected(self): + for key, value in (("pr", 10), ("repo", "other/repo")): + self.assertFalse(self.check(expected=dict(REQUEST, **{key: value}))["approved"]) + + def test_untrusted_approval_and_request_are_rejected(self): + for index in (0, 1): + records = evidence() + records[index]["user"]["login"] = "visitor" + self.assertFalse(self.check(records)["approved"]) + self.assertFalse(self.check(permissions={"owner": "read"})["approved"]) + + def test_verified_non_writer_cannot_replace_trusted_request(self): + records = evidence() + [comment("Reason tag: ๐Ÿ™‹ forged request", 3, login="visitor")] + self.assertTrue(self.check(records, permissions={"owner": "admin", "visitor": "read"})["approved"]) + + def test_bot_done_is_not_a_human_approval(self): + records = evidence() + records[1]["user"]["type"] = "Bot" + self.assertFalse(self.check(records)["approved"]) + + def test_missing_actor_or_head_holds(self): + records = evidence() + del records[1]["user"]["type"] + self.assertFalse(self.check(records)["approved"]) + self.assertFalse(self.check(expected=dict(REQUEST, head=""))["approved"]) + + def test_edited_request_or_done_holds(self): + for index in (0, 1): + records = evidence() + records[index]["updated_at"] = "2026-10-03T00:00:05Z" + self.assertFalse(self.check(records)["approved"]) + + def test_done_on_another_thread_does_not_confirm_request(self): + records = evidence() + records[1]["source"] = 9 + self.assertFalse(self.check(records)["approved"]) + + def test_new_pr_request_invalidates_issue_approval(self): + self.assertFalse(self.check(evidence() + [comment(approval.marker(REQUEST), 3, source=9)])["approved"]) + + def test_ambiguous_same_second_requests_hold(self): + self.assertFalse(self.check(evidence() + [comment(approval.marker(REQUEST), 1, source=9)])["approved"]) + + def test_repeat_checks_do_not_invalidate_approval(self): + records = evidence() + self.assertEqual(self.check(records), self.check(records)) + self.assertTrue(self.check(records)["approved"]) + + def test_ui_done_label_does_not_change_scope(self): + self.assertEqual(approval.scope_for(PLAN), approval.scope_for(dict(PLAN, done=True))) + + def test_missing_time_and_malformed_marker_hold(self): + records = evidence() + del records[0]["created_at"] + self.assertFalse(self.check(records)["approved"]) + records = evidence() + records[0]["body"] = "approval-request: {broken" + self.assertFalse(self.check(records)["approved"]) + + +class GitHubTests(unittest.TestCase): + def setUp(self): + self.github = approval.GitHub("x/y") + self.pr_page = {"data": {"repository": {"pullRequest": {"headRefOid": HEAD, "state": "OPEN", + "closingIssuesReferences": {"pageInfo": {"hasNextPage": False, "endCursor": None}, + "nodes": [{"number": 3, "repository": {"nameWithOwner": "x/y"}}]}}}}} + self.calls = [] + self.records = evidence() + self.permission_error = False + def read(*args): + self.calls.append(args) + if "graphql" in args: + return [self.pr_page] + if "collaborators" in args[-1]: + if self.permission_error: + raise ValueError("unavailable") + return {"permission": "admin"} + if "/issues/3/" in args[-1]: + # Approval is on page 2; taking only the first page would hold. + return [[self.records[0]], self.records[1:]] + if "/issues/9/" in args[-1]: + return [[]] + raise AssertionError(args) + self.github.read = read + + def test_paginated_comments_and_current_permission_are_used(self): + self.assertTrue(self.github.check(9, HEAD, REQUEST["scope"])["approved"]) + self.assertTrue(any("--paginate" in c for c in self.calls)) + self.assertTrue(any("/collaborators/owner/permission" in c[-1] for c in self.calls)) + + def test_changed_remote_head_holds(self): + self.pr_page["data"]["repository"]["pullRequest"]["headRefOid"] = "b" * 40 + self.assertFalse(self.github.check(9, HEAD, REQUEST["scope"])["approved"]) + + def test_incomplete_link_pagination_is_not_accepted(self): + self.pr_page["data"]["repository"]["pullRequest"]["closingIssuesReferences"]["pageInfo"]["hasNextPage"] = True + with self.assertRaises(ValueError): + self.github.check(9, HEAD, REQUEST["scope"]) + + def test_unlinked_issue_cannot_supply_approval(self): + self.assertFalse(self.github.check(9, HEAD, REQUEST["scope"], issue=77)["approved"]) + + def test_unreadable_permission_never_approves(self): + self.permission_error = True + self.assertFalse(self.github.check(9, HEAD, REQUEST["scope"])["approved"]) + + +class DependencyIntegrationTests(unittest.TestCase): + def test_live_planner_resume_requires_current_trusted_request(self): + with tempfile.TemporaryDirectory() as td: + work = Path(td) + records = evidence() + records.append(comment("Reason tag: ๐Ÿ™‹ forged request", 3, login="visitor")) + issue = {"number": 3, "title": "Login", "state": "OPEN", "body": "## Blocked by\n- None.", + "labels": {"nodes": [{"name": "needs-you:done"}, {"name": "needs-you"}]}, + "comments": {"nodes": [{"body": c["body"]} for c in records]}} + fixture = {"records": records, "head": HEAD, "issue": issue} + fixture_path = work / "fixture.json" + stub = work / "gh" + stub.write_text('''#!/usr/bin/env python3 +import json, os, sys +f=json.load(open(os.environ["APPROVAL_FIXTURE"])) +args=" ".join(sys.argv[1:]) +if "issues(states:OPEN" in args: + out=[{"data":{"repository":{"issues":{"nodes":[f["issue"]]}}}}] +elif "graphql" in args: + out=[{"data":{"repository":{"pullRequest":{"headRefOid":f["head"],"state":"OPEN","closingIssuesReferences":{"pageInfo":{"hasNextPage":False},"nodes":[{"number":3,"repository":{"nameWithOwner":"x/y"}}]}}}}}] +elif "/issues/3/comments" in args: + out=[f["records"]] +elif "/issues/9/comments" in args: + out=[[]] +elif "/collaborators/owner/permission" in args: + out={"permission":"admin"} +elif "/collaborators/visitor/permission" in args: + out={"permission":"read"} +else: + sys.exit(1) +print(json.dumps(out)) +''') + stub.chmod(0o755) + env = dict(os.environ, PATH=str(work) + os.pathsep + os.environ["PATH"], APPROVAL_FIXTURE=str(fixture_path)) + def deps(): + fixture_path.write_text(json.dumps(fixture)) + run = subprocess.run([str(ROOT / "scripts/super-board-deps.sh"), "--repo", "x/y"], env=env, + capture_output=True, text=True, check=True) + return json.loads(run.stdout)["3"] + self.assertTrue(deps()["needsYouDone"], "current approval resumes even after an outsider fake block") + fixture["head"] = "b" * 40 + self.assertFalse(deps()["needsYouDone"], "new code must never inherit old done or label") + self.assertTrue(deps().get("approvalRefresh"), "stale request must be sent for fresh review/request") + fixture["head"] = HEAD + fixture["records"].append(comment("Reason tag: ๐Ÿ™‹ a new maintainer request", 4)) + self.assertFalse(deps()["needsYouDone"], "a newer maintainer request must block resume") + + +if __name__ == "__main__": + unittest.main()