commit a690547e8ce6206164aaad413c52298d3b90c508 Author: Bo Xu Date: Wed Sep 23 03:37:23 2026 +0800 first commit diff --git a/SKILL.md b/SKILL.md new file mode 100644 index 0000000..d47bf0c --- /dev/null +++ b/SKILL.md @@ -0,0 +1,748 @@ +--- +name: find-and-fix +description: | + Run a multi-agent team that audits a codebase, verifies each finding, fixes the survivors, reviews the patches, and merges them — with every claim gated on reproducible evidence. Use when asked to hunt for bugs/vulnerabilities across a codebase and land fixes for them, or when the user says "find and fix", "audit and fix", "hardening pass", "review the codebase and fix what you find", or wants a team of auditors plus implementers plus a reviewer working a bug backlog end to end. +--- + +# find-and-fix + +Partition the tree → audit in parallel → **verify before you fix** → fix in +isolated worktrees → review → merge. The hard parts are refusing to fix findings +that are not real, and refusing to merge changes you have not actually tested. + +Do **not** use this for a single known defect (just fix it) or pure exploration +with no intent to land changes. + +--- + +## 0. The contract — read this first + +This is the executable core. Everything after it is rationale and detail. + +The **git** semantics asserted in §6 and §7 are verified by `verify-claims.sh` next +to this file — run it after editing those sections. Its scope is deliberate: it +proves local git behaviour only, **not** GitHub CLI behaviour, team-tool +behaviour, or platform limits. A claim in this skill that is not covered by the +script is unverified and should be labelled as operational knowledge rather than +fact — which is the exact standard this document applies to everything else. + +``` +FREEZE git fetch origin --prune # WITHOUT this, origin/master may be stale + base=$(git rev-parse origin/master) # one SHA, used everywhere + mkdir -p .omo/audits # the ledger dir must exist first + # Write the ledger HEADER (see §4) and record the base in it. Do not + # append a bare line before the header, or §4's contract is violated: + # base-sha: $base + # date: + # repo: + Every branch and every diff uses $base. When a step needs "current master" + later (the post-merge check), resolve a NEW sha, record it as + master-after, and label it — never silently re-resolve $base. + Placeholder convention: $base is this frozen SHA; , , , + are literal arguments you substitute. + +SPAWN team_create({ inline_spec }) # see §5 for the shape + One team_task_create per auditor, confirmer, implementer, and reviewer — + not just auditors. Record each returned task id in the ledger. + team_send_message({ teamRunId, to, body }) # assignment; include the task id + "Work starts on dispatch" means the ASSIGNMENT starts the member; if all + parallel slots are busy, the work queues — so check capacity first. + +TASKS Task state machine is strict: pending → claimed → in_progress → completed + Direct pending→completed and claimed→completed are REJECTED. + So: team_task_update(status:"claimed", owner:"") + team_task_update(status:"in_progress") + team_task_update(status:"completed") + Readback after each transition: team_task_list — confirm the state changed. + +EVIDENCE Every command whose output you rely on goes in the ledger file + (see §4). Not in chat. The ledger is the audit trail. + +REPORTS Auditors write to a gitignored path YOU assign before spawning, e.g. + .omo/audits/.md — the path is a spawn-time decision, not a + placeholder. Reports go to team_send_message per batch. + +FIX git worktree add -b fix/ $base + implementer pushes, then: gh pr create --base master --title ... --body-file ... + A PR body has three sections: What Problem is / What changes / Not fixed. + +REVIEW pr-reviewer posts a verdict: MERGEABLE or CHANGES REQUIRED. + gh pr comment --body-file # NOT gh pr review — see §6 + +MERGE Only when BOTH hold: + 1. CI green AND the step list shows the relevant step EXECUTED + 2. review verdict MERGEABLE + gh pr merge --squash --delete-branch + Then confirm state=MERGED and that origin/master advanced, before §7. + +CLOSE All tasks terminal → team_shutdown_request + team_approve_shutdown per + member → team_delete. Same turn. Write the handover (§7) at a path you name. +``` + +### 0.1 The tool calls — shapes verified by use + +``` +team_create ({ inline_spec: { name, members:[...] } }) # §5 for members +team_task_create ({ teamRunId, subject, description }) # returns a task id +team_task_update ({ teamRunId, taskId, status, owner }) # status per §0 TASKS +team_task_list ({ teamRunId }) # readback + closure check +team_send_message({ teamRunId, to:"", body:"..." }) # delivery is automatic +team_status ({ teamRunId }) # who is active / idle +team_shutdown_request({ teamRunId, targetMemberName }) +team_approve_shutdown({ teamRunId, memberName }) +team_delete ({ teamRunId }) +``` + +Notes that cost real time when unknown: + +- `team_send_message` **delivers automatically** as a new turn; a member that goes + idle after replying is normal, not stalled. +- The task board rejects `pending→completed` and `claimed→completed`; you must + pass through `in_progress` (§0 TASKS). Readback with `team_task_list` after each + transition — that is how you *observe* the change rather than assume it. +- Record each returned task id in the ledger next to its finding id, so "which + task tracked which finding" is answerable later. +- Closure requires every task terminal **and** no shutdown request outstanding. + Read `team_task_list` before you try to close; if it errors on `team_delete`, + re-read `team_status` rather than forcing. + +Everything below this line is rationale and detail. **§0 plus §0.1 is the +control skeleton, not a self-contained procedure** — it tells you the sequence and +the invariants, but you must read §2 (team shape and prompts), §3 (audit and +confirmation), §4 (ledger), §5 (team spec), §6 (fix/review/merge commands) and §7 +(post-merge checks) to actually execute it. Do not hand §0 to an agent and expect a +complete run. + +Two gates, and they are different gates — do not collapse them: + +- **Claim gate.** A finding is actionable when its mechanism is derived with + concrete values: type widths, offsets, index values, and *who writes each + shared field*, with `file:line`. Otherwise it is a hypothesis. +- **Execution gate.** A verification counts when there is evidence the relevant + path actually ran. A green check whose path never executed proves nothing + (§3.1, *vacuous verification*). + +--- + +## 1. Phase 0 — Recon before you spawn + +Cheap here, expensive later. Do it yourself, with tools. + +```bash +git remote -v; git rev-parse --abbrev-ref HEAD; git log --oneline -10 +git rev-list --count $base..HEAD +gh repo view --json viewerPermission,visibility # permission, not proof +gh api repos///branches/master/protection # see caveat below +gh run list --limit 10 # does CI actually run? +rg -n '^[[:space:]]*(pull_request|push)[[:space:]]*:' .github/workflows --glob '*.{yml,yaml}' +``` + +**Do not read a 404 from the protection endpoint as "unprotected".** It is +ambiguous: the repo or branch name may be wrong, the token may lack access, or the +endpoint may be unavailable. Confirm the repo and branch resolve first (e.g. the +`gh repo view` above succeeds) before drawing any conclusion, and treat the answer +as advisory — it decides whether force-push and direct merges are safe, so get it +right rather than inferring from a status code. + +- **Does CI run at all?** A run stuck queued with no runner may mean a missing + self-hosted runner — or just capacity, labels, or an outage. Investigate; do + not conclude. The `rg` above finds event keys, but it is reconnaissance: it + misses `on: [pull_request]` list syntax and comments it out nothing. If nothing + triggers on `pull_request`, you have no PR-level verification: **build it + before auditing** (that outranks every bug), or find another gate. +- **What can be verified locally?** Build trees, compile database, whether LSP + tooling can see the tree (test it — some reject paths outside their own cwd), + and the pinned toolchain versions. +- **Conventions.** Read `AGENTS.md`/`CONTRIBUTING.md`/`MAINTAINERS`. Get the + commit-subject format, the feature-id list, the style checker, and a real + accepted commit body: `git log -1 --format='%s%n%b' `. + +If the project has no working verification, that becomes task 1 and every fix +waits on it. Give it an owner, a deliverable, and an exit condition — a phase +with none of those is a dead branch. + +## 2. Phase 1 — Partition and spawn + +Split into **non-overlapping file domains**. A workable shape: + +| Member | Scope | +|--------|-------| +| auditor-core | core libs, memory, IPC, API layers | +| auditor-datapath | dataplane, plugins, drivers, tests | +| implementer-a / -b | fixes (assigned by you) | +| pr-reviewer | every PR | +| **confirmer** | the independent second pass (§3) | + +Two things people forget, both of which cause stalls: + +- **Staff the confirmer.** The verification gate needs a second agent, and the + team bounds cap parallelism (observed: 8 members, 4 concurrent workers). Decide + up front whether the confirmer is a dedicated member, a rotated role, or the + reviewer — and if the reviewers are saturated, the confirmation work queues. + Say which. +- **Cap open PRs.** CI is usually the bottleneck, not the agents. Decide how many + PRs may be open at once, and merge before opening more. + +Standing-prompt rules: + +- **Explicit, exclusive path list** per member; tell each what a peer owns. + Cross-domain leads go to the lead as *leads*, not as findings. +- **Diff-first** if the tree is a fork or has a hardening history: + `git diff --stat $base...HEAD -- `. In an already-hardened tree the + highest-value defects are *incomplete* guards — hardening applied to some call + sites and not others. +- **Auditors are read-only**: no edits, no commits, no builds. Outputs are one + markdown report at a path you name, plus `team_send_message` per batch. +- **If local builds are banned, tell the implementers up front** and say + verification comes from CI. Otherwise they burn hours trying to compile. + +### 2.1 Producer contract — state this once, verbatim + +Applies to every producer of claims (auditors, confirmers, reviewers). + +``` +Per finding: + Location file:line, plus every other call site of the symbol + Mechanism -> -> -> + Reachability dataplane / API / CLI / config — and whether proven or assumed + Ownership for each shared field (counters, head/tail, refcounts, + generations): which side writes it, with file:line + Severity crash / DoS / memory safety / leak / robustness + Fix shape smallest correct change, direction only, not implemented + Confidence HIGH / MEDIUM / LOW + one line why (unmarked is treated LOW) + VERIFY FIRST yes/no; if yes, the missing fact and what would settle it + +Report must also end with: + ## Retracted + is FALSE: + +Self-correction is a duty, not a failure. Re-derive your own earlier findings and +retract with the evidence. A retraction section is a deliverable, and a growing +one is a POSITIVE signal about the producer. An agent that marks confidence and +flags what it cannot prove is trusted and kept; an agent that reports everything +at equal volume gets its whole batch re-verified by someone else. +``` + +That last paragraph is the highest-leverage thing in the prompt: it is the +producer's *calibration*, not its recall, that decides how much of the batch +survives. + +## 3. Phase 2 — Audit, then verify + +Auditors report **incrementally, per batch** — define what "batch" means +(e.g. "every ~5 findings or when a file is finished"), because the lead must be +able to tell "still working" from "done". Speed of reporting beats completeness +of sweep. + +The gate, in order: + +1. **Spot-check the top findings yourself.** Read the source at the cited lines. + Reproduce the arithmetic. Do not accept a finding you cannot see. +2. **Send an independent confirmation pass** for survivors. Give it your settled + verdicts so it does not redo them, and ask for `CONFIRMED / FALSE / + UNRESOLVED` plus a derivation, and for confirmed items the minimal fix shape + and the caller blast radius. +3. **Correct the producer.** Send back what was killed and the evidence, and + require the report file to be corrected so it does not stand as a list of live + defects. + +Tell the confirmers: **killing a finding is a success**, and a well-derived FALSE +counts as much as a CONFIRMED. + +Two failure modes recur; name them in the prompt: + +- **Assumed ownership** — the finding assumes a peer or callee can move a value + the local side (or another owner) actually controls. Require the write-owner + with `file:line` for every shared field. +- **Whole-file reasoning** — conclusions from running a tool over entire files + rather than the changed hunks. Gate scripts are usually hunk-scoped. + +### 3.1 Vacuous verification — the failure that looks like success + +A check "passes" because its precondition never fired: the loop iterated zero +times, the branch was never taken, the test step was skipped. Formal verification +calls this *vacuous satisfaction*. + +It is uniquely dangerous because **a normal defect shows up as failure; this +shows up as success.** The information you need is not in the source — it is in +*which branch executed*. That is why re-reading the code never finds it. Note +"never" is not absolute: source review *can* spot a missing event trigger or a +filter that matches nothing. But you cannot rely on it; you need execution +evidence. + +A worked example (one run, but the shape generalises): a CI workflow carried +three defects on a single path, none findable by re-reading the YAML. Steps ran +under `sh` instead of `bash` and died with `Bad substitution`; fixing that let the +step complete and emit a test filter with no matching module; and the probe added +to exercise the path produced zero runs because its branch was already merged. +Those were **layers on one unexecuted path**, each hiding the next — so expect +layers, not a single bug, on any path that has never actually run. + +**The criterion: do not ask "did it pass?" Ask "did it execute?"** + +1. **Demand log evidence the branch ran.** Good evidence is a value only the + executed path could produce — and a *result*, not just a banner. A line like + `Running TEST suites:nat64` proves the step completed and selected something + non-empty; it does not by itself prove the tests ran or passed, so pair it + with the test-result output. Contrast `conclusion=success` with the test step + `skipped`: nothing was proven. +2. **Require a negative control where you can: show the gate can fail.** A gate + that has never failed is *weakly* validated, not automatically invalid — some + gates cannot be safely broken, and static proof carries some claims. But treat + an unexercised-failure gate as weaker evidence and label it that way. +3. **Mutation testing is the systematic form of this.** Mutate the code under + test and confirm the test fails. A surviving mutant points at a coverage or + assertion gap — *unless* it is equivalent or deliberately out of contract. + +**When you cannot construct the input, say so and treat the run as +non-evidence.** "I could not make the acceptance run exercise this path" is a +finding. "The run was green" is not, when the path never ran. + +## 4. The evidence ledger + +"Show the command and its output" is not enough if the output lives in chat and +scrolls away. Keep one file, e.g. `.omo/audits/LEDGER.md` (gitignored). + +**Header, once, before anything else:** + +``` +base-sha: # the frozen $base from §0, recorded +date: +repo: +``` + +**One block per finding.** The `evidence` field must carry *provenance*, not a +bare output line — a line copied from the wrong run or wrong job satisfies a +one-line schema and proves nothing: + +``` +LEDGER-FINDING +- claim: one line +- derivation: the arithmetic / ownership facts, with file:line +- confidence: HIGH|MEDIUM|LOW (from the producer; VERIFY FIRST if flagged) +- verdict: who confirmed it, how, and what settled it +- evidence: + exit= at= by= + +- ci: run= job= head-sha= step="" = +- fix: branch= commits= pr= +- merge: merge-commit= master-before= master-after= +``` + +Two fields earn their keep by being easy to omit: **`exit=`** (a command that +failed and printed something plausible is not evidence) and **`head-sha=`** (the +run you inspected must be the run for the commit you tested — a branch can have +several). If you find yourself unable to fill `head-sha=` or `step=`, you have not +actually verified anything. + +This is what makes the run auditable and what you hand over. It is also how you +survive context exhaustion: raw logs go to files, the ledger holds conclusions. + +## 5. Team spec + +```json +{ + "name": "-hardening", + "members": [ + { "name": "auditor-core", "kind": "category", "category": "deep", + "prompt": " + " }, + { "name": "auditor-datapath", "kind": "category", "category": "deep", + "prompt": "" }, + { "name": "confirmer", "kind": "category", "category": "deep", + "prompt": " Verify findings handed to you by the lead: verdict + CONFIRMED/FALSE/UNRESOLVED with a derivation, plus the minimal + fix shape and caller blast radius for confirmed items. Killing a + finding is a success. Work from and the source; + never edit. The lead hands you settled verdicts to avoid redoing + them." }, + { "name": "implementer-a", "kind": "category", "category": "deep", + "prompt": "" }, + { "name": "implementer-b", "kind": "category", "category": "deep", + "prompt": "" }, + { "name": "pr-reviewer", "kind": "category", "category": "ultrabrain", + "prompt": "" } + ] +} +``` + +Six members is deliberate and fits the observed bounds (8 members, 4 concurrent +workers). **If you have fewer slots, do not drop the confirmer** — the +verification gate is the point of the pipeline. Instead drop one auditor and give +its domain to the other, or make `pr-reviewer` also the confirmer and accept that +review work queues behind confirmation. Say which you chose. A team spec with no +confirmer contradicts §2 and silently removes the gate that makes findings +trustworthy. + +Prompts must be **self-contained** — a member cannot see your audit +conversation, so a brief that says "fix the bug we discussed" is unusable. +Every prompt carries: role, scope, method, deliverable path, report format, and +prohibitions. Vague prompts produce vague work. + +## 6. Phases 3–5 — fix, review, merge + +**Fix.** Create the worktree and branch yourself, from the frozen base: +`git worktree add -b fix/ $base`. Do not let two implementers share +one, and do not let them create their own (they collide or pick a stale base). +The brief carries the derivation, the required shape, an **explicit +out-of-scope list**, and the acceptance criteria. Constrain it: minimal, no +refactor, no drive-by reformatting. Atomic commits, one per independent logical +unit, verified against the project's own gate script. Then: + +```bash +git push -u origin fix/ +gh pr create --base master --title ': ' --body-file /tmp/body.md +``` + +The body has three sections: `## What Problem is`, `## What changes`, +`## Not fixed`. The last is where honesty lives. + +If a fix depends on an unmerged PR's new symbol, **stack it**: branch from that +PR's branch and `gh pr create --base `. After the base merges, +retarget with `gh api -X PATCH repos///pulls/ -f base=master` +(`gh pr edit` is unreliable on some repos). + +**Never rebase a pushed branch by default** — but that is a *policy* default, not +a law. After a base PR is squash-merged, a stacked branch often needs rebasing or +recreation to drop the base's pre-squash commits; a merge commit can leave +duplicate logical commits and a confusing diff. + +Concrete procedure once the base is merged. Record the base branch's tip **before** +it merges — call it `B`; call the base's merge commit on master `M`: + +```bash +git fetch origin + +# Option A (preferred where history rewriting is allowed): rebase the stacked +# branch's own commits onto M, dropping the base's pre-squash commits. +git switch +git rebase --onto $M $B +git push --force-with-lease origin +gh api -X PATCH repos///pulls/ -f base=master # retarget + +# Option B (where force-push is forbidden): merge master in, and review the result. +git merge origin/master +git diff --name-status HEAD^..HEAD # confirm nothing reverted +git push origin +``` + +Verify the outcome either way — this is the check that catches a botched rewrite: + +```bash +gh pr diff --name-only # should list ONLY the stacked branch's files +gh pr view --json mergeable,baseRefName +``` + +Two traps, both verified against a scratch repo: + +- **`git log --no-merges M..` does not isolate "the real work".** After a + squash merge it lists the base's original commits *as well*, because the squash + produced a different SHA — the base's work appears twice (once as `M`, once as + the branch's own commit). Do not use that range to decide what to cherry-pick. +- **`git diff M...` likewise shows the base's files too**, for the same + reason: `M` is not an ancestor of the branch, so the merge base is still the old + branch point. Only `git rebase --onto $M $B` (or an equivalent explicit + cherry-pick of commits in `$B..`) yields a branch whose diff against `M` + is just its own work. + +If a `D` appears for a file the branch never touched, see §7 — it may be staleness, +not a revert. State in the PR which option you used and why. + +**Review.** One reviewer, every PR, verdict exactly `MERGEABLE` or +`CHANGES REQUIRED`, with per-finding `file:line`, mechanism, required change. + +Require the reviewer to emit the verdict as a **bare token on its own line, in a +fixed prefix form**, so it can be read back mechanically: + +``` +VERDICT: MERGEABLE +``` + +not prose containing the word ("I would not call this MERGEABLE yet" defeats a +`grep`-based readback — the §6 check greps for the token and would extract the +wrong one). The reviewer must also name the head SHA it reviewed, so a verdict +issued before a later push is visibly stale. + +This is what the merge gate reads in §6 step 2b; without the fixed form the +readback is a guess. + +First check, and it is the one that catches this whole pipeline's defect class: + +> **Enumerate every reference of the symbol being changed, then classify which +> paths the fix protects.** Not "every site must appear in the diff" — a +> root-level accessor fix or a wrapper change correctly protects callers without +> touching them. And grep misses macro-expanded, generated, and indirect +> references. Require: *every vulnerable path is covered*, and say how many +> sites exist and how many the fix reaches. + +Then: does it remove the defect at the root for every reachable path (a guard +validating the wrong thing, or sitting after the dereference, is a rejection); +is the value genuinely untrusted there; does the test fail before and pass after +(a test that cannot distinguish is a rejection); scope honesty; project style and +commit gates. + +Operational notes: **`gh pr review` is rejected when reviewer and author share an +account** — use `gh pr comment --body-file ` and say so, or a reader will +think no review exists. `gh pr edit` may fail with a GraphQL Projects-classic +error; use the `gh api -X PATCH` form. + +**Merge.** Two conditions, both required: + +1. **CI green, read from the step list** — and the relevant step *executed*. +2. Review verdict `MERGEABLE`. + +Every check below **fails closed**: it exits non-zero, so you cannot slide past a +missing run or an unmerged PR. That matters because the obvious forms do not — +`jq 'select(...)'` emits nothing and exits 0 when nothing matches, `last` on an +empty array yields `null`, and `[ cond ] || echo "warning"` returns 0. Verified: + +``` +$ echo '{"a":1}' | jq 'select(.b=="x")'; echo $? # no output, 0 +$ echo '[]' | jq 'sort_by(.x)|last|.y'; echo $? # null, 0 +$ bash -c '[ 1 = 2 ] || { echo warn; }'; echo $? # warn, 0 <- does NOT fail closed +``` + +```bash +set -euo pipefail + +# 0. Record the master SHA before merging, so you can prove it advanced. +# §7 reuses this as $BEFORE; keep it in the ledger as master-before. +BEFORE=$(git rev-parse origin/master) + +# 1. Select the run deterministically: pin commit AND workflow, filter to the +# pull_request event, take the newest. A commit can have several runs. +HEAD=$(gh pr view --json headRefOid --jq .headRefOid) +RUN=$(gh run list --commit "$HEAD" --workflow "" \ + --json databaseId,event \ + --jq '[.[] | select(.event=="pull_request")] | sort_by(.databaseId) | last | .databaseId') +[ -n "$RUN" ] && [ "$RUN" != "null" ] || { echo "no run for $HEAD — do NOT merge" >&2; exit 1; } + +# 2. Assert THAT run finished successfully and belongs to this head. +# 'select' emits nothing on no match, so assert the output is non-empty. +MATCH=$(gh run view "$RUN" --json status,conclusion,headSha \ + --jq 'select(.headSha=="'"$HEAD"'" and .status=="completed" and .conclusion=="success")') +[ -n "$MATCH" ] || { echo "run $RUN is not a completed success for $HEAD" >&2; exit 1; } + +# 2b. Read back the review verdict instead of remembering it. The reviewer emits +# "VERDICT: " on its own line (see the Review section), so anchor the +# grep to that prefix — a bare token search would match prose like +# "I would not call this MERGEABLE yet" and extract the wrong verdict. +VERDICT=$(gh api repos///issues//comments --jq '[.[].body] | join("\n")' \ + | grep -oE '^VERDICT: (MERGEABLE|CHANGES REQUIRED)$' | tail -1 \ + | sed 's/^VERDICT: //') +[ "$VERDICT" = "MERGEABLE" ] || { echo "review verdict is '$VERDICT', not MERGEABLE — do not merge" >&2; exit 1; } +# Also confirm the verdict postdates the head: if the reviewer named a head SHA, +# it must equal $HEAD, or the verdict is stale and the PR needs a re-review. + +# 3. Assert the dependent step ran and succeeded. grep -qx fails on "skipped", +# on "failure", and on a step that does not exist — that is the point. +JOB=$(gh run view "$RUN" --json jobs --jq '.jobs[]|select(.name=="")|.databaseId') +[ -n "$JOB" ] || { echo "job not found in run $RUN" >&2; exit 1; } +gh api repos///actions/jobs/"$JOB" \ + --jq '.steps[] | select(.name=="") | .conclusion' | grep -qx success \ + || { echo "step did not succeed (skipped/failed/missing)" >&2; exit 1; } +# Record run id, job id, step and its conclusion in the ledger. + +# 4. Merge, then poll until it is ACTUALLY merged (a queue or auto-merge returns +# before master moves). Exit non-zero if it never merges. +gh pr merge --squash --delete-branch +state="" +for i in $(seq 1 30); do + state=$(gh pr view --json state --jq .state) + [ "$state" = "MERGED" ] && break + sleep 10 +done +[ "$state" = "MERGED" ] || { echo "not MERGED after 5min — stop here" >&2; exit 1; } + +# 5. Prove master advanced, rather than assuming. +git fetch origin +AFTER=$(git rev-parse origin/master) +[ "$BEFORE" != "$AFTER" ] || { echo "master did NOT advance — the merge did not land" >&2; exit 1; } +git log --oneline -3 origin/master +``` + +Step 3 is the one that enforces this document's whole thesis: it asserts the +step's conclusion instead of inviting the reader to eyeball it, so a `skipped` +test step fails loudly rather than merging silently. + +An agent can satisfy "inspect the steps" while inspecting the wrong run's job, so +pin by commit **and** workflow, take the newest matching run, and record the ids. +Note step 3 asserts the step's conclusion rather than eyeballing it: a `skipped` +test step is the exact failure this document exists to prevent, so make the check +fail loudly instead of relying on the reader to notice. + +A skipped test step is **disqualifying when that test is the claimed behavioural +evidence**, and acceptable only for changes whose correctness is independently +established and whose scope does not need it. Say which clause you applied. + +Do not merge to unblock. If the gate is broken, fix the gate — merging "the one +that passes" while the rest are blocked by your own defect hides the defect and +teaches everyone to ignore red. + +## 7. After the merge — check the merged tree + +Merging is not finished when the command returns. Check that master actually moved +and that the merged tree contains what you intended. Two traps live here: the +squash-captures-a-stale-head trap, and the two-tree-diff trap. + +So after merging: + +```bash +git fetch origin && git log --oneline origin/master -3 +git diff --name-status $BEFORE origin/master # $BEFORE is the pre-merge master from §6 +``` + +And for any PR whose branch predates a recent master merge, know what each +command does and does not tell you before trusting `MERGEABLE` (`MERGEABLE` means +"no conflict", not "changes only what it says"): + +```bash +# 1. Does the branch contain the recent master state? +git merge-base --is-ancestor \ + || echo "branch predates it" + +# 2. The branch's own patch, relative to the merge base (three-dot): +git diff --name-status ... + +# 3. Two final trees compared (whole-tree divergence): +git diff --name-status origin/master + +# 4. The ACTUAL merge result, without merging (authoritative): +git merge-tree --write-tree origin/master # tree oid, or conflict report +git ls-tree -r --name-only # what the merged tree contains +``` + +**Command 3 does NOT predict what a merge will do. This is a trap worth naming.** +A branch cut before a file was added to master shows `D ` in command 3 — +yet a three-way merge **preserves** that file, because the branch never deleted +it. Verified: + +``` +$ git diff --name-status master feature +D master-only.txt # looks like a deletion + +$ git merge-tree --write-tree master feature +$ git ls-tree -r --name-only +base.txt +master-only.txt # actually preserved +``` + +So do not read a `D` as "the merge will delete this". Use command 4 for the merge +result, and command 2 to see what the branch itself changed. A genuine revert is +the branch *modifying* a path that master changed — visible in command 2, not as a +bare `D` in command 3. (A `D` in command 3 is still worth *looking at*: it means +the branch lacks something master has, which may be staleness or may be a real +deletion. Command 2 distinguishes them.) + +`git merge-tree --write-tree` needs Git ≥ 2.38. On older Git, fall back to a +throwaway worktree: `git worktree add /tmp/probe && git -C /tmp/probe +merge --no-commit --no-ff origin/master` and inspect, then remove it. + +After merging, confirm the merge actually happened before checking the tree — +`gh pr merge` can enter a merge queue or enable auto-merge rather than completing: + +```bash +gh pr view --json state,mergeCommit # state must be MERGED +git fetch origin && git log --oneline origin/master -3 +git diff --name-status $BEFORE origin/master # $BEFORE is the pre-merge master from §6 +``` + +A squash merge captures **the head SHA actually selected and merged**. A commit +pushed to the branch *after* that head was chosen is not included — so a cleanup +commit can silently miss the merge. (Say "after the selected head", not "after +the PR was queued": a queued or auto-merge PR may be revalidated against a later +head, in which case the later commit *is* included.) + +Real case: a probe file was added so a PR's own diff would exercise a code path; +the removal commit was pushed seconds after the head was selected and never +entered the merge. The probe file landed on master. (Note the naive explanation +"squash resurrected a deleted file" is wrong — squash cannot do that. And note +that this is a *different* phenomenon from the `D`-in-command-3 trap above; do not +conflate the two.) + +Clean up, and **verify the cleanup** rather than assuming it: + +```bash +git worktree list # only the main checkout should remain +git worktree prune +git branch -a --list 'fix/*' # merged branches should be gone +git ls-remote --heads origin 'fix/*' # including on the remote +``` + +Then close the team: all tasks terminal → `team_shutdown_request` + +`team_approve_shutdown` per member → `team_delete`, in the same turn as the last +task completes. Read `team_task_list` first: if anything is still +`pending`/`claimed`/`in_progress`, the closure contract is not met. + +**Handover** (write it to a path you name, e.g. `.omo/audits/HANDOVER.md`) — the +next session reads this, not your chat: + +``` +master: +merged: +open: +left: +verify: +next: +``` + +Every claim in it needs a command that reproduces it. A handover that says +"verified" without the command is the same vacuous evidence the rest of this +document warns about. + +## 8. Handling the things that go wrong + +| Situation | Do this | +|---|---| +| **Member goes idle with no report** | Idle is normal after a turn, not an error. But a member that never reports *and* never completes is stalled: ask it directly, with a bounded question. If it produces nothing twice, reassign the work — do not wait on the wall clock. | +| **Audit finds nothing** | That is a valid outcome, but it needs an artifact: a zero-findings report listing what was covered and what was not. "Nothing found" must not look like an abandoned run. | +| **Context filling up** | Push conclusions to the ledger (§4), raw logs to files. Summarise and discard. A long multi-agent run will exhaust the lead's context if everything stays in chat. | +| **Wall-clock budget** | Team runs have a wall-clock limit (observed: 120 min). Budget it: recon, audit, fix, CI waits. CI is the long pole — start runs early, keep several in flight, and do not serialise behind one build. | +| **CI slow or flaky** | Distinguish: PR defect / infra failure / flaky test / queued runner / missing required check. Merge independent fast-checked fixes first; cancel superseded runs. Do not merge on a flake — re-run and see it twice. | +| **Two findings touch the same file** | Non-overlapping audit domains do not imply non-overlapping fixes. Combine them into one branch, or serialise them — never two open PRs editing the same lines. | +| **A probe produces no run** | Verify the run *exists* before trusting it — but first read the workflow's `on:` block and its filters, because the trigger is conditional. `reopened` fires only if the workflow lists it in `types:`; a branch push fires only if the workflow file exists on that ref and its `branches:`/`paths:` filters match. Check `rg -n -A6 '^on:' .github/workflows/.yml`, then confirm a new run id appears. | +| **Empty or absent report** | Treat "no report, no completion signal" as unfinished, not as clean. Ask, then escalate. | + +## 9. Anti-patterns + +Observed examples from one run; the fix is the general rule. + +| Anti-pattern | Looks like | Do instead | +|---|---|---| +| **Vacuous verification** | A check passes because its precondition never fired — loop ran zero times, test step `skipped`. Looks like success, so re-reading code cannot find it | Ask "did it execute?" not "did it pass?" (§3.1) | +| **Layered masking** | Fixing one defect on an untested path reveals the next it was hiding; looks like a run of independent mistakes | Expect layers on any path that never ran; treat each green as provisional until the whole path has executed | +| **Vacuous probe** | A commit added to exercise a path produces zero runs (branch can't fire an event) — the verification attempt is itself unverified | Check the run id exists before trusting it | +| **Uniform-confidence reporting** | Every finding equally certain; no confidence marks, no `VERIFY FIRST`, no retractions | Put calibration duties in the prompt at spawn (§2.1) | +| **Accepting a workflow on a run that could not exercise it** | A CI change merged on a green run whose diff never reached the new path | Make the acceptance run touch a path that exercises the change, or treat the run as non-evidence | +| **Whole-file tool reasoning** | Running a formatter over a whole file, seeing hundreds of diffs, concluding "the tool is unreliable" and dismissing a correct finding | Run the project's own gate script, hunk-scoped, with the pinned version; calibrate a local tool against a file the change does not touch | +| **Merging on a green conclusion** | `conclusion=success` while `Run the tests` is `skipped` | Read the step list | +| **Trusting `MERGEABLE` alone** | A PR reports no conflict and builds fine, but its branch predates a master merge and the merge would revert a file | `git diff --name-status origin/master `; check ancestry with `git merge-base --is-ancestor` | +| **Commit pushed after the merge queued** | A cleanup commit sits on the branch but is absent from master, because squash captured the earlier head | Check the merged tree after every merge (§7) | +| **Dispatch trigger that cannot fire** | `gh workflow run --ref ` returns 422 because the branch predates the workflow file | Close and reopen the PR to fire `reopened`; confirm a new run id | +| **Merge commit failing the commit gate** | Contributor merges the base branch in; the gate rejects `Merge remote-tracking branch ...` for having no feature id | Iterate the PR range with `git rev-list --reverse --no-merges ..` | +| **Sentinel collision** | A "not set" sentinel of `0` colliding with a legitimate `open()` returning `0` | Use `-1`, or force the descriptor above 0 | +| **A check that can never fire** | `if (!ptr)` right after a raw accessor that never returns NULL — the wild dereference already happened | Validate inside the accessor, or promote a checked accessor and adopt it at trust boundaries | +| **Overclaiming in a PR body** | "The existing test would fail before this change" when the test never reaches the changed function | Verify against the call chain, or state the gap | + +## 10. Checklist + +- [ ] Recon: remotes, permission, **does CI run**, local verification, conventions +- [ ] Base SHA frozen and recorded once; every branch made from it +- [ ] Domains partitioned, no overlap, each member told what a peer owns +- [ ] Confirmer staffed; open-PR cap decided +- [ ] Standing prompts carry scope, report path, format, and calibration duties +- [ ] Auditors read-only; reporting per batch with a defined batch size +- [ ] Evidence ledger started; commands and outputs recorded there +- [ ] Every finding spot-checked, then independently confirmed; producers corrected +- [ ] Every green gate confirmed to have **executed** the path it claims to check +- [ ] Worktree + branch created by the lead per confirmed finding +- [ ] Briefs self-contained, with out-of-scope and honest-verification clauses +- [ ] PRs created with `gh pr create`; body has all three sections +- [ ] Every PR reviewed with the caller-classification check first +- [ ] Merged only on executed-CI-green + MERGEABLE; merged tree checked afterwards +- [ ] Zero-finding or abandoned-domain outcomes have an artifact +- [ ] Worktrees and branches cleaned; team closed in the same turn as the last task +- [ ] Handover written: what merged, what is left, exact next commands diff --git a/verify-claims.sh b/verify-claims.sh new file mode 100755 index 0000000..ca8a1d7 --- /dev/null +++ b/verify-claims.sh @@ -0,0 +1,123 @@ +#!/usr/bin/env bash +# Verify the Git semantics that SKILL.md asserts, by experiment. +# +# Scope: this script proves the GIT behaviours exercised below. It does NOT prove +# GitHub CLI behaviour (gh pr merge, run selection, merge queues) — those are +# verified procedurally in SKILL.md §6, not here. Do not read a green run as +# "every claim in the skill is verified"; read it as "these specific git claims +# hold". Adding a claim to the skill without adding a check here leaves it +# unverified, which is the failure the skill itself warns about. +set -euo pipefail + +pass=0; fail=0 +chk() { # chk "description" expected actual + if [ "$2" = "$3" ]; then printf ' PASS %s\n' "$1"; pass=$((pass+1)); + else printf ' FAIL %s\n expected=[%s] actual=[%s]\n' "$1" "$2" "$3"; fail=$((fail+1)); fi +} +newrepo() { + T=$(mktemp -d); cd "$T" + git init -q -b master .; git config user.email t@t; git config user.name t + echo base > base.txt; git add .; git commit -qm base +} +cleanup() { cd /; rm -rf "$T"; } + +echo "--- §7: two-tree diff vs the actual merge result (the D trap) ---" +newrepo +git checkout -qb feature +git checkout -q master; echo m > master-only.txt; git add .; git commit -qm "master adds file" +chk "two-tree diff reports D for a file the branch merely predates" \ + "D master-only.txt" "$(git diff --name-status master feature)" +TREE=$(git merge-tree --write-tree master feature | head -1) +chk "merge-tree PRESERVES it -> a D does NOT mean the merge deletes it" \ + "1" "$(git ls-tree -r --name-only "$TREE" | grep -c '^master-only.txt$')" +chk "three-dot diff is empty for a stale-only branch" "" "$(git diff --name-status master...feature)" +if git merge-base --is-ancestor master feature; then rc=0; else rc=1; fi +chk "is-ancestor exits 1 when the branch predates master" "1" "$rc" +cleanup + +echo "--- §7: a genuine branch-side deletion IS visible in the three-dot diff ---" +newrepo +git checkout -q master; echo shared > shared.txt; git add .; git commit -qm "add shared" +git checkout -qb feature; git merge -q master; git rm -q shared.txt; git commit -qm "branch deletes shared" +chk "three-dot diff shows the real branch-side deletion" \ + "D shared.txt" "$(git diff --name-status master...feature)" +cleanup + +echo "--- §6: --no-merges excludes the merge commit from a commit-message gate ---" +newrepo +git checkout -qb f; echo x > x.txt; git add .; git commit -qm x +git checkout -q master; git merge -q --no-ff -m "a merge commit" f +MERGE=$(git rev-parse master) +chk "the merge commit exists and is found by --merges" "$MERGE" "$(git rev-list --merges master)" +# The real assertion: the merge sha is ABSENT from the --no-merges enumeration. +# (An earlier version of this script tested `--no-merges --merges`, which is a +# contradiction: it returns nothing for ANY repo and so proved nothing.) +chk "the merge sha is absent from the --no-merges list" \ + "0" "$(git rev-list --no-merges master | grep -c "^$MERGE$" || true)" +chk "non-merge commits are still listed" \ + "2" "$(git rev-list --no-merges master | wc -l | tr -d ' ')" +cleanup + +echo "--- §7: squash captures the head AT MERGE TIME ---" +newrepo +git checkout -qb work; echo a > a.txt; git add .; git commit -qm work +EARLY=$(git rev-parse work) +echo b > b.txt; git add .; git commit -qm "later commit, pushed after the head was selected" +git checkout -q master; git merge -q --squash "$EARLY" >/dev/null; git commit -qm "squash work (#1)" +chk "the selected head is in the merge" "1" "$(git ls-tree -r --name-only HEAD | grep -c '^a.txt$')" +chk "the later commit is NOT in the merge" "0" "$(git ls-tree -r --name-only HEAD | grep -c '^b.txt$')" +cleanup + +echo "--- §6: stacked-branch recovery after a base squash-merge ---" +newrepo +git checkout -qb feat-base; echo b1 > b1.txt; git add .; git commit -qm "base: work" +B=$(git rev-parse feat-base) # base tip recorded BEFORE the merge +git checkout -qb feat-stack; echo s1 > s1.txt; git add .; git commit -qm "stack: work" +git checkout -q master; git merge -q --squash feat-base >/dev/null; git commit -qm "base: work (#1)" +M=$(git rev-parse master) +chk "log --no-merges M..branch does NOT isolate the real work (lists base too)" \ + "2" "$(git log --oneline --no-merges $M..feat-stack | wc -l | tr -d ' ')" +chk "diff M...branch shows the base's file too (M is not an ancestor)" \ + "2" "$(git diff --name-status $M...feat-stack | wc -l | tr -d ' ')" +git switch -q feat-stack +git rebase -q --onto $M $B feat-stack +chk "after rebase --onto M B, the branch's diff is ONLY its own work" \ + "A s1.txt" "$(git diff --name-status $M...feat-stack)" +cleanup + +echo "--- §7: merge-tree reports conflicts instead of silently succeeding ---" +newrepo +git checkout -qb c1; echo one > c.txt; git add .; git commit -qm one +git checkout -q master; echo two > c.txt; git add .; git commit -qm two +if git merge-tree --write-tree master c1 >/dev/null 2>&1; then rc=0; else rc=1; fi +chk "a conflicting merge makes merge-tree exit non-zero" "1" "$rc" +cleanup + + +echo "--- §6: the merge guards fail closed (not just warn) ---" +# These are the forms that DO NOT fail closed, so the skill's assertions matter. +set +e +out=$(echo '{"a":1}' | jq 'select(.b=="x")' 2>/dev/null); rc=$? +set -e +chk "jq select with no match emits nothing and exits 0 (the trap)" "0" "$rc" +chk "...and emits no output, so an assertion on its output is required" "" "$out" +set +e +out=$(echo '[]' | jq 'sort_by(.x) | last | .y' 2>/dev/null); rc=$? +set -e +chk "last on an empty array yields the literal null" "null" "$out" +set +e; bash -c '[ 1 = 2 ] || { echo warn; }' >/dev/null 2>&1; rc=$?; set -e +chk "a bare '|| echo' warning still exits 0 (why guards must exit 1)" "0" "$rc" + +guard() { # guard ; emulate the skill's guard shape + if "$@" >/dev/null 2>&1; then return 0; else return 1; fi +} +chk "empty RUN is rejected" "1" "$(guard bash -c '[ -n "" ] && [ "" != null ]' ; echo $?)" +chk "null RUN is rejected" "1" "$(guard bash -c '[ -n null ] && [ null != null ]' ; echo $?)" +chk "valid RUN is accepted" "0" "$(guard bash -c '[ -n 123 ] && [ 123 != null ]' ; echo $?)" +chk "empty MATCH is rejected" "1" "$(guard bash -c '[ -n "" ]' ; echo $?)" +chk "skipped step is rejected" "1" "$(guard bash -c 'echo skipped | grep -qx success' ; echo $?)" +chk "success step is accepted" "0" "$(guard bash -c 'echo success | grep -qx success' ; echo $?)" +chk "missing step is rejected" "1" "$(guard bash -c 'echo "" | grep -qx success' ; echo $?)" + +printf '\n%d passed, %d failed\n' "$pass" "$fail" +[ "$fail" -eq 0 ]