749 lines
40 KiB
Markdown
749 lines
40 KiB
Markdown
---
|
||
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: <iso8601>
|
||
# repo: <owner/name>
|
||
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; <branch>, <n>, <sha>,
|
||
<path> 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:"<name>")
|
||
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/<auditor>.md — the path is a spawn-time decision, not a
|
||
placeholder. Reports go to team_send_message per batch.
|
||
|
||
FIX git worktree add <path> -b fix/<slug> $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 <n> --body-file <f> # 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 <n> --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:"<member>", 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/<o>/<r>/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' <sha>`.
|
||
|
||
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 -- <domain>`. 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 <field/type at file:line> -> <value or offset> -> <the read or
|
||
write it enables> -> <the exact input that produces it>
|
||
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
|
||
<id> is FALSE: <the evidence that killed it, in plain language>
|
||
|
||
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: <sha> # the frozen $base from §0, recorded
|
||
date: <iso8601>
|
||
repo: <owner/name>
|
||
```
|
||
|
||
**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 <id> <status: hypothesis|confirmed|false|fixed|merged>
|
||
- 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: <exact command>
|
||
exit=<code> at=<iso8601> by=<who/where>
|
||
<key output lines — the value that only this path could produce>
|
||
- ci: run=<id> job=<id> head-sha=<sha> step="<name>" = <conclusion>
|
||
- fix: branch=<name> commits=<sha[,sha]> pr=<n>
|
||
- merge: merge-commit=<sha> master-before=<sha> master-after=<sha>
|
||
```
|
||
|
||
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": "<project>-hardening",
|
||
"members": [
|
||
{ "name": "auditor-core", "kind": "category", "category": "deep",
|
||
"prompt": "<role> <exclusive domain> <method, in order> <report path>
|
||
<report format §2.1 verbatim> <prohibitions>" },
|
||
{ "name": "auditor-datapath", "kind": "category", "category": "deep",
|
||
"prompt": "<same, disjoint domain>" },
|
||
{ "name": "confirmer", "kind": "category", "category": "deep",
|
||
"prompt": "<read-only> 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 <report path> and the source;
|
||
never edit. The lead hands you settled verdicts to avoid redoing
|
||
them." },
|
||
{ "name": "implementer-a", "kind": "category", "category": "deep",
|
||
"prompt": "<fix assigned defects only; minimal diffs; atomic commits;
|
||
push + gh pr create; honest 'could not verify' reporting>" },
|
||
{ "name": "implementer-b", "kind": "category", "category": "deep",
|
||
"prompt": "<same, own worktree>" },
|
||
{ "name": "pr-reviewer", "kind": "category", "category": "ultrabrain",
|
||
"prompt": "<review every PR; caller-classification first; verdict
|
||
MERGEABLE or CHANGES REQUIRED>" }
|
||
]
|
||
}
|
||
```
|
||
|
||
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 <path> -b fix/<slug> $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/<slug>
|
||
gh pr create --base master --title '<feature>: <subject>' --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 <that-branch>`. After the base merges,
|
||
retarget with `gh api -X PATCH repos/<o>/<r>/pulls/<n> -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 <stacked-branch>
|
||
git rebase --onto $M $B <stacked-branch>
|
||
git push --force-with-lease origin <stacked-branch>
|
||
gh api -X PATCH repos/<o>/<r>/pulls/<n> -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 <stacked-branch>
|
||
```
|
||
|
||
Verify the outcome either way — this is the check that catches a botched rewrite:
|
||
|
||
```bash
|
||
gh pr diff <n> --name-only # should list ONLY the stacked branch's files
|
||
gh pr view <n> --json mergeable,baseRefName
|
||
```
|
||
|
||
Two traps, both verified against a scratch repo:
|
||
|
||
- **`git log --no-merges M..<branch>` 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...<branch>` 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..<branch>`) 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 <n> --body-file <f>` 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 <n> --json headRefOid --jq .headRefOid)
|
||
RUN=$(gh run list --commit "$HEAD" --workflow "<workflow-name>" \
|
||
--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: <token>" 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/<o>/<r>/issues/<n>/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=="<job>")|.databaseId')
|
||
[ -n "$JOB" ] || { echo "job <job> not found in run $RUN" >&2; exit 1; }
|
||
gh api repos/<o>/<r>/actions/jobs/"$JOB" \
|
||
--jq '.steps[] | select(.name=="<step>") | .conclusion' | grep -qx success \
|
||
|| { echo "step <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 <n> --squash --delete-branch
|
||
state=""
|
||
for i in $(seq 1 30); do
|
||
state=$(gh pr view <n> --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 <master-sha> <branch> \
|
||
|| echo "branch predates it"
|
||
|
||
# 2. The branch's own patch, relative to the merge base (three-dot):
|
||
git diff --name-status <master-sha>...<branch>
|
||
|
||
# 3. Two final trees compared (whole-tree divergence):
|
||
git diff --name-status origin/master <branch>
|
||
|
||
# 4. The ACTUAL merge result, without merging (authoritative):
|
||
git merge-tree --write-tree origin/master <branch> # tree oid, or conflict report
|
||
git ls-tree -r --name-only <tree-oid> # 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 <file>` 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 <tree>
|
||
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 <branch> && 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 <n> --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: <sha>
|
||
merged: <PR list with commit shas>
|
||
open: <PR list, each with its exact blocker>
|
||
left: <findings recorded but not tasked, with file:line and fix direction>
|
||
verify: <what is proven, what is unproven, and how to re-check>
|
||
next: <exact commands to continue>
|
||
```
|
||
|
||
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/<wf>.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 <branch>`; 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 <branch>` 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 <base>..<head>` |
|
||
| **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
|