Skip to content

Commit 2a749d3

Browse files
authored
fix: automatically start Crosscheck reviews on PR registration (#373)
* fix: start crosscheck on PR registration * no-mistakes(review): Fix Crosscheck retirement handoff and live merge detection * no-mistakes(review): Preserve Crosscheck registration order and completion state * no-mistakes(document): Replace stale registration description with authoritative documentation pointer * no-mistakes: apply CI fixes * no-mistakes: apply CI fixes
1 parent 692fa15 commit 2a749d3

16 files changed

Lines changed: 1757 additions & 35 deletions

bin/fm-crosscheck-autostart.py

Lines changed: 848 additions & 0 deletions
Large diffs are not rendered by default.

bin/fm-crosscheck.py

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#!/usr/bin/env python3
2-
"""Fail-closed independent review ledger bound to an exact pull-request head."""
2+
"""Fail-closed independent review ledger bound to an exact pull-request head.
3+
4+
The public `run TASK URL` surface resolves the live head itself.
5+
The PR-registration coordinator additionally passes `--expected-head SHA` so a head change between registration and launch refuses before reviewer or Azure spending.
6+
"""
37

48
from __future__ import annotations
59

@@ -6954,7 +6958,13 @@ def write_ledger(path: Path, ledger: dict[str, Any]) -> None:
69546958
atomic_write(path, encoded)
69556959

69566960

6957-
def run_crosscheck(root: Path, home: Path, task_id: str, url: str) -> int:
6961+
def run_crosscheck(
6962+
root: Path,
6963+
home: Path,
6964+
task_id: str,
6965+
url: str,
6966+
expected_head: str | None = None,
6967+
) -> int:
69586968
# C1 (docs/azure-requirements.md): the invocation's clock starts here, so
69596969
# the recorded `total` covers everything the caller waits for, including
69606970
# the unattributed gaps between the named phases.
@@ -6985,6 +6995,11 @@ def run_crosscheck(root: Path, home: Path, task_id: str, url: str) -> int:
69856995
snapshot_value = github_snapshot(root, url)
69866996
except CrosscheckError as exc:
69876997
tool_fail(f"GitHub snapshot preflight failed: {exc}")
6998+
if expected_head is not None and snapshot_value["head_sha"] != expected_head:
6999+
tool_fail(
7000+
"registered PR head changed before Crosscheck launch: expected "
7001+
f"{expected_head}, observed {snapshot_value['head_sha']}"
7002+
)
69887003
with timer.phase("ledger"):
69897004
try:
69907005
ledger = load_ledger(ledger_path, task_id, url)
@@ -7796,6 +7811,8 @@ def build_parser() -> argparse.ArgumentParser:
77967811
command = subparsers.add_parser(name)
77977812
command.add_argument("task_id")
77987813
command.add_argument("pr_url")
7814+
if name == "run":
7815+
command.add_argument("--expected-head")
77997816
timings = subparsers.add_parser("timings")
78007817
timings.add_argument("task_id")
78017818
economics = subparsers.add_parser("economics")
@@ -7904,7 +7921,16 @@ def main() -> int:
79047921
except BlockingIOError:
79057922
tool_fail("another crosscheck operation already owns this task")
79067923
if args.command == "run":
7907-
return run_crosscheck(root, home, args.task_id, args.pr_url)
7924+
expected_head = args.expected_head
7925+
if expected_head is not None and SHA_RE.fullmatch(expected_head) is None:
7926+
tool_fail("expected registered PR head must be one 40-hex SHA")
7927+
return run_crosscheck(
7928+
root,
7929+
home,
7930+
args.task_id,
7931+
args.pr_url,
7932+
expected_head,
7933+
)
79087934
if args.command == "verify":
79097935
return verify_crosscheck(root, home, args.task_id, args.pr_url)
79107936
return merge_crosschecked(

bin/fm-crosscheck.sh

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,17 @@
22
# Run or verify the independent exact-head crosscheck ledger for a task PR.
33
#
44
# Usage:
5-
# fm-crosscheck.sh run <task-id> <full GitHub PR URL>
5+
# fm-crosscheck.sh run <task-id> <full GitHub PR URL> [--expected-head <SHA>]
66
# fm-crosscheck.sh verify <task-id> <full GitHub PR URL>
77
# fm-crosscheck.sh status
88
# fm-crosscheck.sh timings <task-id>
99
# fm-crosscheck.sh economics <task-id>
1010
# fm-crosscheck.sh merge <task-id> <full GitHub PR URL> <reviewed SHA> <method> [--allow-queue]
1111
#
1212
# `run` is intentionally independent of no-mistakes so both reviews can be in
13-
# flight together once a PR exists. `verify` is the merge-gate operation: it
13+
# flight together once a PR exists. The task-local PR-registration coordinator
14+
# uses `--expected-head` to refuse a moved head before reviewer or Azure spend.
15+
# `verify` is the merge-gate operation: it
1416
# re-reads live GitHub state, requires the latest attempt for that exact head
1517
# and claims document to be clear, and prints only the reviewed SHA.
1618
# `timings` is the read-only C1 breakdown: it prints the per-phase duration

bin/fm-pr-check.sh

Lines changed: 66 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
#!/usr/bin/env bash
2-
# Record a PR-ready task: appends pr=<url> and GitHub's pr_head=<sha> to
3-
# state/<id>.meta when available, then arms the watcher's merge poll by writing
4-
# state/<id>.check.sh, which prints one line when the PR is merged or its lookup
5-
# fails (the watcher's check contract: output = wake, silence = keep sleeping).
6-
# With central Slack config installed, then binds the live PR head to the signed
7-
# launch record created before the task agent started. Issuance failure exits
8-
# nonzero after poll setup.
2+
# Register a PR-ready task: append pr=<url> and GitHub's pr_head=<sha> to
3+
# state/<id>.meta, arm the watcher's merge poll, and asynchronously start the
4+
# independent exact-head Crosscheck review. Registration returns after the
5+
# task-local coordinator is requested; review latency never parks the caller.
6+
# Matching active or CLEAR heads deduplicate, failed/dead coordinators remain
7+
# visible and retryable, and unrelated tasks never share a launcher lock.
8+
# A task-local registration lock orders head capture and publication without
9+
# holding the account metadata lock across the remote lookup.
10+
# With central Slack config installed, binds the live PR head to the signed
11+
# launch record created before the task agent started before requesting review.
12+
# Issuance failure exits nonzero after poll setup.
913
# Usage: fm-pr-check.sh <task-id> <pr-url>
1014
set -eu
1115

@@ -26,6 +30,32 @@ META="$STATE/$ID.meta"
2630
LOOKUP_WT=
2731
LOOKUP_GENERATION=
2832
PR_HEAD=
33+
CROSSCHECK_AUTOSTART="$SCRIPT_DIR/fm-crosscheck-autostart.py"
34+
CROSSCHECK_AUTOSTART_ENABLED=1
35+
case "${FM_CROSSCHECK_AUTOSTART_TEST_DISABLE:-}" in
36+
'') ;;
37+
firstmate-pr-check-nonautostart-test-v1)
38+
[ "${FM_TEST_RUNNER_ACTIVE:-}" = firstmate-test-runner-v1 ] || {
39+
echo "error: the Crosscheck autostart test bypass is available only inside the sealed behavior-test runner" >&2
40+
exit 1
41+
}
42+
CROSSCHECK_AUTOSTART_ENABLED=0
43+
;;
44+
*)
45+
echo "error: invalid FM_CROSSCHECK_AUTOSTART_TEST_DISABLE value" >&2
46+
exit 1
47+
;;
48+
esac
49+
REGISTRATION_LOCK=$(fm_account_lock_acquire "$STATE" "$ID" pr-registration \
50+
"PR registration" "${FM_ACCOUNT_META_LOCK_WAIT_SECONDS:-10}") || exit 1
51+
META_LOCK=
52+
release_meta_lock() {
53+
if [ -n "$META_LOCK" ]; then
54+
fm_account_meta_lock_release "$META_LOCK" >/dev/null 2>&1 || true
55+
fi
56+
fm_account_meta_lock_release "$REGISTRATION_LOCK" >/dev/null 2>&1 || true
57+
}
58+
trap release_meta_lock EXIT
2959
META_LOCK=$(fm_account_meta_lock_acquire "$STATE" "$ID") || exit 1
3060
if [ ! -f "$META" ]; then
3161
fm_account_meta_lock_release "$META_LOCK"
@@ -54,18 +84,17 @@ if [ -z "$LOOKUP_GENERATION" ]; then
5484
exit 1
5585
fi
5686
fi
87+
# Serialize head capture/publication only against other registrations, not
88+
# account-session updates or task retirement during a remote lookup.
5789
fm_account_meta_lock_release "$META_LOCK"
90+
META_LOCK=
5891
if ! PR_HEAD_LOOKUP=$("$FM_ROOT/bin/fm-github-pr.py" head "$URL" 2>&1); then
5992
PR_HEAD_DIAGNOSTIC=$(printf '%s' "$PR_HEAD_LOOKUP" | tr '\r\n' ' ')
6093
printf 'UNREVIEWED: PR head lookup failed: %.500s\n' "$PR_HEAD_DIAGNOSTIC" >&2
6194
exit 1
6295
fi
6396
PR_HEAD=$PR_HEAD_LOOKUP
6497
META_LOCK=$(fm_account_meta_lock_acquire "$STATE" "$ID") || exit 1
65-
release_meta_lock() {
66-
fm_account_meta_lock_release "$META_LOCK" >/dev/null 2>&1 || true
67-
}
68-
trap release_meta_lock EXIT
6998
if [ -f "$META" ]; then
7099
CURRENT_WT=$(fm_account_meta_value "$META" worktree)
71100
CURRENT_GENERATION=$(fm_account_meta_value "$META" generation_id)
@@ -85,13 +114,26 @@ else
85114
fi
86115
CHECK_TMP=$(mktemp "$STATE/.$ID.check.XXXXXX") || exit 1
87116
printf -v PR_ADAPTER_Q '%q' "$FM_ROOT/bin/fm-github-pr.py"
117+
printf -v CROSSCHECK_AUTOSTART_Q '%q' "$CROSSCHECK_AUTOSTART"
118+
printf -v ID_Q '%q' "$ID"
88119
printf -v URL_Q '%q' "$URL"
120+
printf -v PR_HEAD_Q '%q' "$PR_HEAD"
121+
printf -v GENERATION_Q '%q' "$LOOKUP_GENERATION"
89122
cat > "$CHECK_TMP" <<EOF
90123
if ! state=\$($PR_ADAPTER_Q state $URL_Q 2>&1); then
91124
diagnostic=\$(printf '%s' "\$state" | tr '\r\n' ' ')
92125
printf 'UNREVIEWED: PR state lookup failed: %.500s\n' "\$diagnostic"
93126
exit 0
94127
fi
128+
if [ "\$state" = MERGED ]; then
129+
echo "merged"
130+
exit 0
131+
fi
132+
if ! crosscheck_state=\$($CROSSCHECK_AUTOSTART_Q status $ID_Q $URL_Q $PR_HEAD_Q $GENERATION_Q 2>&1); then
133+
diagnostic=\$(printf '%s' "\$crosscheck_state" | tr '\r\n' ' ')
134+
printf 'UNREVIEWED: Crosscheck autostart failed: %.500s\n' "\$diagnostic"
135+
exit 0
136+
fi
95137
case "\$state" in
96138
OPEN) ;;
97139
MERGED) echo "merged" ;;
@@ -100,8 +142,6 @@ esac
100142
EOF
101143
chmod +x "$CHECK_TMP"
102144
mv "$CHECK_TMP" "$STATE/$ID.check.sh"
103-
fm_account_meta_lock_release "$META_LOCK"
104-
trap - EXIT
105145
SLACK_CONFIG=${FM_CROSSCHECK_SLACK_CONFIG:-$FM_HOME/config/crosscheck-slack.json}
106146
if [ -f "$SLACK_CONFIG" ]; then
107147
"$FM_ROOT/bin/fm-crosscheck-slack.sh" attest-task \
@@ -111,3 +151,16 @@ if [ -f "$SLACK_CONFIG" ]; then
111151
}
112152
fi
113153
echo "armed: state/$ID.check.sh polls $URL"
154+
if [ "$CROSSCHECK_AUTOSTART_ENABLED" = 1 ]; then
155+
if CROSSCHECK_AUTOSTART_OUT=$("$CROSSCHECK_AUTOSTART" start \
156+
"$ID" "$URL" "$PR_HEAD" "$LOOKUP_GENERATION" 2>&1); then
157+
printf '%s\n' "$CROSSCHECK_AUTOSTART_OUT"
158+
else
159+
CROSSCHECK_AUTOSTART_DIAGNOSTIC=$(printf '%s' "$CROSSCHECK_AUTOSTART_OUT" | tr '\r\n' ' ')
160+
printf 'UNREVIEWED: Crosscheck autostart launcher failed: %.500s\n' \
161+
"$CROSSCHECK_AUTOSTART_DIAGNOSTIC" >&2
162+
fi
163+
fi
164+
fm_account_meta_lock_release "$META_LOCK"
165+
fm_account_meta_lock_release "$REGISTRATION_LOCK"
166+
trap - EXIT

docs/crosscheck.md

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,40 @@
11
# Crosscheck
22

3-
Crosscheck is an on-demand, exact-head PR reviewer. It is independent of
4-
Firstmate task orchestration: any agent or operator that can run the supported
5-
wrapper and read the configured Firstmate home can use it.
3+
Crosscheck is an exact-head PR reviewer that starts automatically when Firstmate registers a PR-ready task and remains available on demand.
4+
It is independent of Firstmate task orchestration: any agent or operator that can run the supported wrapper and read the configured Firstmate home can use it.
65

7-
Crosscheck does one job. It reviews the current PR head, returns `CLEAR` or
8-
`BLOCKING`, and records cited findings and suspicions against that exact SHA.
6+
Crosscheck does one job.
7+
It reviews the current PR head, returns `CLEAR` or `BLOCKING`, and records cited findings and suspicions against that exact SHA.
98
It does not rerun CI, manufacture proof scripts, or launch verifier VMs.
109

1110
## Run it
1211

13-
Use a unique task ID and the full public GitHub PR URL:
12+
The normal Firstmate path is PR-ready registration:
13+
14+
```sh
15+
FM_HOME=/Users/dongkeun/firstmate-home \
16+
bin/fm-pr-check.sh <task-id> \
17+
https://github.com/OWNER/REPO/pull/NUMBER
18+
```
19+
20+
Registration records the live PR head, arms the merge poll, durably requests Crosscheck, starts one task-local coordinator, and returns without waiting for the review.
21+
A matching active request is reused, and a matching exact-head and exact-claims `CLEAR` result is verified without another review.
22+
Registering a new head replaces the queued request so the coordinator reviews that head next.
23+
Registration holds the task metadata lock from head capture through poll emission and request publication, so an older capture cannot replace a newer registration.
24+
A short task-local handoff lock couples request publication, status reconciliation, and coordinator retirement; it is never held during review execution.
25+
A dead or failed coordinator releases its task-local lock and retries when the same registration command runs again.
26+
The merge poll observes live GitHub merge state before reporting launcher failures, so manual completion and merge still trigger cleanup without granting merge authorization.
27+
Unrelated task coordinators share no launcher lock, so the Azure lane-capacity and cost-admission controls remain the only review spending authority.
28+
29+
Before launching a review, the coordinator loads the authoritative operator-private fleet environment from `~/.fm-azure/fleet.env` by default.
30+
`FM_CROSSCHECK_FLEET_ENV` may select another absolute file.
31+
The launcher opens that file without following symlinks and requires a current-operator-owned regular file that is not group or world writable.
32+
It sources the already-open file only inside the Crosscheck child, suppresses output from the source operation, and never copies environment values into argv, prompts, logs, repository files, or launcher records.
33+
34+
Missing, unsafe, or incomplete fleet configuration does not undo or fail PR registration.
35+
The task remains honestly uncleared, the actionable failure is recorded in `state/<task-id>.crosscheck-autostart.json` and `state/<task-id>.crosscheck-autostart.log`, and the task check surfaces it for repair and retry.
36+
37+
For an explicit on-demand run, use a unique task ID and the full public GitHub PR URL:
1438

1539
```sh
1640
set -a
@@ -21,15 +45,14 @@ FM_HOME=/Users/dongkeun/firstmate-home \
2145
https://github.com/OWNER/REPO/pull/NUMBER
2246
```
2347

24-
The fleet environment is operator-private Azure configuration. Load it into the
25-
process environment; never paste its values into a prompt or command.
48+
The fleet environment is operator-private Azure configuration.
49+
Load it into the process environment; never paste its values into a prompt or command.
2650

27-
A new task ID needs no pre-created metadata file. Existing state must match the
28-
same task and PR identity or the run fails closed.
51+
A new task ID needs no pre-created metadata file.
52+
Existing state must match the same task and PR identity or the run fails closed.
2953

30-
The command exits zero only for a valid `CLEAR` verdict on the live head. A
31-
finding, unresolved suspicion, stale head, provider failure, malformed verdict,
32-
or infrastructure failure exits nonzero and is never presented as clearance.
54+
The command exits zero only for a valid `CLEAR` verdict on the live head.
55+
A finding, unresolved suspicion, stale head, provider failure, malformed verdict, or infrastructure failure exits nonzero and is never presented as clearance.
3356

3457
Results are written to:
3558

docs/scripts.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ The shared no-mistakes gate refusal used by every directly invocable mutating co
106106
| `fm-crosscheck-slack.sh` | Run, preflight, selftest, or issue exact-head provenance for the Slack Crosscheck lane |
107107
| `fm-crosscheck-slack.py` | Serve allowlisted, metered, exact-head Slack reviews through the shared core lanes |
108108
| `fm-crosscheck-slack-service.sh` | Install and operate the credential-free macOS launchd wrapper for the central listener |
109-
| `fm-pr-check.sh` | Record `pr=` and `pr_head=` for a PR-ready task, then arm the watcher's merge poll |
109+
| `fm-pr-check.sh` | Register a PR-ready task; see [Crosscheck operator flow](crosscheck.md#run-it) |
110110
| `fm-pr-merge.sh` | Require exact-head crosscheck, record PR metadata, and atomically merge or enqueue the reviewed SHA |
111111
| `fm-promote.sh` | Promote a scout task in place to a protected ship task |
112112
| `fm-report-contract-lib.sh` | Render the shared ship completion-report contract inserted into briefs and continuation prompts |

tests/behavior-test-durations.tsv

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
4000 tests/fm-pi-refresh.test.sh
8989
3750 tests/fm-pi-retry-continuity.test.sh
9090
3170 tests/fm-pi-watch-extension.test.sh
91+
14000 tests/fm-pr-crosscheck-autostart.test.sh
9192
1574 tests/fm-pr-merge.test.sh
9293
6030 tests/fm-process-tree.test.sh
9394
802 tests/fm-prompt-exec.test.sh

tests/fm-crosscheck-slack.test.sh

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1315,6 +1315,38 @@ with Path({str(launch_log)!r}).open('a') as handle:
13151315
sys.exit(1 if sys.argv[1] == 'print' else 0)
13161316
""")
13171317
launchctl.chmod(0o755)
1318+
# Model the macOS plist editor on every host; the emitted plist is parsed below
1319+
# and its actual service command is executed with the emitted environment.
1320+
plutil = bin_dir / "plutil"
1321+
plutil.write_text(f"#!{sys.executable}\n" + """
1322+
import plistlib
1323+
from pathlib import Path
1324+
import sys
1325+
args = sys.argv[1:]
1326+
path = Path(args[-1])
1327+
if args[0] == '-create':
1328+
value = {}
1329+
else:
1330+
value = plistlib.loads(path.read_bytes())
1331+
parts = args[1].split('.')
1332+
parent = value
1333+
for part in parts[:-1]:
1334+
parent = parent[int(part)] if isinstance(parent, list) else parent[part]
1335+
kind = args[2]
1336+
item = {'-array': [], '-dictionary': {}}.get(kind)
1337+
if kind == '-string':
1338+
item = args[3]
1339+
elif kind == '-bool':
1340+
item = args[3] == 'true'
1341+
elif kind == '-integer':
1342+
item = int(args[3])
1343+
if isinstance(parent, list):
1344+
parent.insert(int(parts[-1]), item)
1345+
else:
1346+
parent[parts[-1]] = item
1347+
path.write_bytes(plistlib.dumps(value))
1348+
""")
1349+
plutil.chmod(0o755)
13181350
environment = dict(os.environ, HOME=str(home), FM_HOME=str(fm_home), FM_ROOT_OVERRIDE=str(fixture), FM_CROSSCHECK_SLACK_CONFIG=str(config_path), FM_CROSSCHECK_PYTHON=sys.executable, PATH=str(bin_dir) + os.pathsep + os.environ['PATH'])
13191351
for name in ('app_token_env', 'bot_token_env', 'github_token_env'):
13201352
environment[config[name]] = 'fixture-inherited-secret'

tests/fm-crosscheck.test.sh

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3462,6 +3462,30 @@ assert run["state"] == "clear"
34623462
pass "a pipeline-updated PR is reviewed at its exact remote head while the author worktree remains behind"
34633463
}
34643464

3465+
test_registered_expected_head_refuses_a_moved_head_before_spend() {
3466+
local record case_dir base head expected rc
3467+
record=$(make_case registered-head-moved)
3468+
IFS=$'\t' read -r case_dir base head <<< "$record"
3469+
expected=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
3470+
[ "$expected" != "$head" ] || fail "expected-head fixture did not move"
3471+
set +e
3472+
run_case "$case_dir" "$base" "$head" clear run --expected-head "$expected" \
3473+
> "$case_dir/out" 2> "$case_dir/err"
3474+
rc=$?
3475+
set -e
3476+
expect_code 1 "$rc" "moved registered head"
3477+
assert_grep "registered PR head changed before Crosscheck launch: expected $expected, observed $head" \
3478+
"$case_dir/err" \
3479+
"moved registered head did not produce the exact pre-spend diagnostic"
3480+
assert_absent "$case_dir/codex.log" \
3481+
"reviewer launched after the registered exact head changed"
3482+
assert_absent "$case_dir/pi.log" \
3483+
"Pi reviewer launched after the registered exact head changed"
3484+
assert_absent "$case_dir/data/task-x1/crosscheck-ledger.json" \
3485+
"head mismatch fabricated a durable review attempt"
3486+
pass "a moved registered head refuses before reviewer or Azure spending"
3487+
}
3488+
34653489
test_missing_pr_head_ref_fails_closed() {
34663490
local record case_dir base head rc
34673491
record=$(make_case missing-pr-head-ref)
@@ -6691,6 +6715,7 @@ if [ -n "${FM_TEST_CASE:-}" ]; then
66916715
test_missing_metadata_for_existing_task_fails_closed|\
66926716
test_existing_task_metadata_identity_collision_fails_closed|\
66936717
test_review_fetches_exact_pr_head_when_author_worktree_is_behind|\
6718+
test_registered_expected_head_refuses_a_moved_head_before_spend|\
66946719
test_missing_pr_head_ref_fails_closed|\
66956720
test_codex_reviewer_requires_bound_auth_and_clears_ambient_credentials|\
66966721
test_launcher_requires_supported_python|\
@@ -6845,6 +6870,7 @@ test_mismatched_state_without_metadata_fails_closed
68456870
test_missing_metadata_for_existing_task_fails_closed
68466871
test_existing_task_metadata_identity_collision_fails_closed
68476872
test_review_fetches_exact_pr_head_when_author_worktree_is_behind
6873+
test_registered_expected_head_refuses_a_moved_head_before_spend
68486874
test_missing_pr_head_ref_fails_closed
68496875
test_codex_reviewer_requires_bound_auth_and_clears_ambient_credentials
68506876
test_null_ledger_fails_without_normalization

0 commit comments

Comments
 (0)