Conversation
Detect newly conflicted open PRs from registry-derived repos, dedupe by head SHA, poll lazy GitHub mergeability safely, and arm through the standard registered check path.
…errun, record pruning
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the current discovery-completeness tracking and pruning guards address the previously reported empty, partial, and missing-registry dedupe-state loss paths. Reviews (5): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
Owner
|
Speaking as Kun's firstmate: this account has been flagged as attempting malicious activity and can no longer contribute to any of Kun's repos. Closing this pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Build automatic detection and routing of pull-request merge conflicts across every repository this fleet works in. Add a registered watcher check that polls open PRs for repos derived from data/projects.md clones, data/secondmates.md ownership, and the firstmate checkout origin (no hardcoded repo list). Route each wake with owner-team from secondmates.md (firstmate repo -> main). Dedupe on repo+number+head SHA so the same conflict wakes once and a force-updated head wakes again. Poll GitHub lazy mergeability: never treat UNKNOWN as clean or conflicted. Print one wake line only when firstmate should wake; stay silent otherwise; finish inside FM_CHECK_TIMEOUT; use gh-axi; include drafts; register with fm-check-register.sh via arm/disarm. Detection only - no conflict resolution, rebase, or update-branch. One script, one registration. Prove with fixture tests covering newly conflicted wakes, same-head silence, new-head re-wake, unknown never reported, and empty output when clean. Document what this does NOT solve (safety net, not cure).
What Changed
bin/fm-pr-conflict-watch.sh, a registered watcher check (check/arm/disarmviafm-check-register.sh) that sweeps open GitHub PRs - drafts included - across repositories derived fromdata/projects.mdclone origins and this firstmate checkout's own origin, routes each wake to anowner-teamresolved fromdata/secondmates.md(firstmate repo and unmapped projects fall back tomain), and prints a singlepr-conflict:line only when a new conflict appears. It polls lazymergeablestate so a persistentUNKNOWNis never reported as clean or conflicted, dedupes on repo + PR number + head SHA so a force-updated head wakes again, bounds each probe and the whole sweep insideFM_CHECK_TIMEOUT, discloses conflicts cut by the line cap asN more omittedwithout recording them, and prunes the dedupe record back to the conflicts each sweep still observes. Detection and routing only - no rebase, update-branch, or conflict resolution.owner/repoparse out ofbin/fm-bearings-snapshot.shinto a new sharedbin/fm-repo-slug-lib.sh(fm_repo_slug) so the bearings snapshot and the new watcher name the same repository the same way.tests/fm-pr-conflict-watch.test.sh(12 fixture cases covering new-conflict wakes, same-head silence, new-head re-wake, unknown never reported, unknown-reread head attribution and deadline, line-cap deferral, record pruning, draft reporting, clean-fleet silence, andarmregistration), and documented the watcher's routing, dedupe, tunables, and explicit non-goals indocs/configuration.md,docs/scripts.md, andAGENTS.md.Risk Assessment
Testing
I ran the change's own fixture suite (12 cases, twice, no flakiness) plus the bearings-snapshot suite that shares the newly extracted repo-slug library, then built and ran an end-to-end demo that drives the real
bin/fm-watch.shagainst an armedpr-conflict-watch.check.shin a throwaway FM_HOME with only GitHub faked, so repository discovery, owner routing, the trust-bound check sweep, the durable wake queue, and the drain are all production code. The captured transcript shows a clean fleet producing no wake at all, three conflicts across two project clones and the firstmate repo surfacing as one wake line routed to team-a, team-b and main with the draft flagged, silence on subsequent sweeps with unchanged heads, a fresh wake after a force-updated head, an UNKNOWN pull request re-read and never reported either way, and disarm leaving no artifacts behind. The change is a shell/CLI watcher with no rendered surface, so the reviewer-visible evidence is the operator transcript rather than a screenshot. Everything passed and I found no issues.Evidence: End-to-end operator transcript: arm, eight watcher sweeps, drained wake lines, disarm
Source: End-to-end operator transcript: arm, eight watcher sweeps, drained wake lines, disarm
=== 3. main moves: two PRs conflict (one a draft) plus one in the firstmate repo === watcher exited to wake firstmate. what firstmate reads out of the durable wake queue: | check: <home>/state/pr-conflict-watch.check.sh: pr-conflict: owner-team=team-a repo=acme/alpha number=7 head=1111111111111111111111111111111111111111 draft=no url=https://github.com/acme/alpha/pull/7 title=Add retry budget to the worker; owner-team=team-b repo=acme/beta number=3 head=1111111111111111111111111111111111111111 draft=yes url=https://github.com/acme/beta/pull/3 title=WIP: split the ingest queue; owner-team=main repo=kunchenguid/firstmate number=2942 head=1111111111111111111111111111111111111111 draft=no url=#2942 title=bound remote job worker supervisor restarts === 4. the same heads are still conflicting: the fleet stays quiet === no wake: the watcher kept supervising for 12s and printed nothing. === 5. PR 7's head is force-updated and still conflicts: wake again === | check: <home>/state/pr-conflict-watch.check.sh: pr-conflict: owner-team=team-a repo=acme/alpha number=7 head=2222222222222222222222222222222222222222 draft=no url=https://github.com/acme/alpha/pull/7 title=Add retry budget to the worker === 6. GitHub has not computed mergeability yet (UNKNOWN): never guessed either way === no wake: the watcher kept supervising for 12s and printed nothing. | 11x gh-axi pr view --repo acme/alpha 9Evidence: Reproducible end-to-end script (fake GitHub only; real watcher, wake queue and drain)
Source: Reproducible end-to-end script (fake GitHub only; real watcher, wake queue and drain)
Evidence: Fixture-test transcript naming the behaviours the intent requires
Source: Fixture-test transcript naming the behaviours the intent requires
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-pr-conflict-watch.sh:469- Every emitted finding is recorded as reported (record_add_key) even when it never made it into the printed line.emit_findingconcatenates all findings into one string andaction_checkcuts it to MAX_LINE=1000 withfm_cap_line_var, but nothing ties the recorded keys to what survived the cut. Concrete sequence: main moves and 8 open PRs go CONFLICTING in one sweep. A realistic finding is ~220 chars (owner-team=main repo=kunchenguid/firstmate number=2942 head=<40 hex> draft=no url=https://github.com/kunchenguid/firstmate/pull/2942 title=...), so after the 13-charpr-conflict:prefix only ~4 findings fit; findings 5-8 are replaced by[truncated]. All 8 keys are still persisted byrecord_write, so on the next sweeprecord_has_keyskips them and those 4 conflicts never wake firstmate at all - the heads are unchanged, so nothing ever re-triggers them. This defeats the intent's "the same conflict wakes once" for exactly the mass-conflict scenario the watcher exists for. Fix: only record keys for findings actually present in the printed line, or cap the finding count and disclose "N more omitted" while leaving the omitted keys unrecorded (the siblingbin/fm-tool-update-check.sh:735handles the same hazard by comparing the whole finding set rather than the cut line).bin/fm-pr-conflict-watch.sh:500- When the sweep runs out of budget the loop just breaks, and repos never reached are indistinguishable from clean in the output.discover_reposemits a fixed order (projects.md order, with the firstmate repo appended last), so the same tail repos starve on every sweep, forever. With BUDGET_SECS=20 and onegh pr listper repo (plus up to UNKNOWN_ATTEMPTS extrapr viewcalls per UNKNOWN PR, and PR_LIMIT=30 PRs per repo), a fleet with more than ~15 repos - or one busy repo early in the list - permanently silences everything after the cut point, including the firstmate repo itself. Intent requires detection "across every repository this fleet works in"; the established sibling contract inbin/fm-tool-update-check.sh:216(budget_allows) emits "check incomplete: the time budget ran out before <name>" precisely so an unfinished sweep says so instead of reading as clean. Same applies to the other silent-failure paths that return without a word:gh pr listfailure or timeout (:432), missing jq (:477), missing gh-axi (:480). Since the fix adds a user-visible line, confirm the desired shape.bin/fm-pr-conflict-watch.sh:399-pr_mergeable_resolvednever checksbudget_exhaustedand its total cost is not bounded by DEADLINE. Once the deadline passes,probe_boundfloors at PROBE_MIN_SECS=1, so the loop can still spend UNKNOWN_ATTEMPTS gh calls plus (UNKNOWN_ATTEMPTS-1) sleeps of UNKNOWN_WAIT past the deadline. BUDGET_MAX only reserves PROBE_MIN_SECS+CLOCK_ROUNDING+KILL_GRACE = 3s of headroom. Concrete: FM_PR_CONFLICT_UNKNOWN_ATTEMPTS=10 and FM_PR_CONFLICT_UNKNOWN_WAIT=5 are both inside the documented valid ranges (:121-143) and yield up to 101 + 95 = 55s of overshoot on top of the 20s budget; FM_PR_CONFLICT_BUDGET_SECS=30 with the default FM_CHECK_TIMEOUT=30 gives 27+6=33s at default knobs. Either exceeds the watcher's CHECK_TIMEOUT (bin/fm-watch.sh:153), which kills the process group - and because the script only prints and records at the very end (:509-512), a killed run prints nothing and records nothing, so the same sweep dies identically on every poll and no conflict is ever reported. Fix: cut the retry loop short onbudget_exhausted, and/or fold UNKNOWN_ATTEMPTS*(PROBE_MIN_SECS+UNKNOWN_WAIT) into the BUDGET_MAX reserve.bin/fm-pr-conflict-watch.sh:492- The budget-cut notice is pushed into FINDINGS on every sweep but is never deduped against the record (REPORTED_KEYS only holds conflict keys), so a non-empty line is printed every cadence even when no PR is conflicted. Concrete: FM_PR_CONFLICT_BUDGET_SECS=30 passes validation (1..120 at :109-119); with the default FM_CHECK_TIMEOUT=30, BUDGET_MAX=27, so BUDGET_CUT_FROM is set and every poll printspr-conflict: sweep budget 30s cut to 27s to stay inside the watcher check timeout of 30s, waking firstmate every FM_CHECK_INTERVAL indefinitely with no new information. This contradicts the intent's "Print one wake line only when firstmate should wake; stay silent otherwise" and "empty output when clean". The sibling avoids this by gating the print on[ "$FINDINGS" != "$RECORD_REPORTED" ](bin/fm-tool-update-check.sh:735). Fix: report the cut once (record it), or only attach it to a line that already carries a conflict.tests/fm-pr-conflict-watch.test.sh:133-write_view_sequencewrites the three view payloads as newline-delimited JSON objects, but the fake gh-axi reads them withjq -c ".[$((idx - 1))]" "$file"(:98), which is array indexing. I verified:printf '%s\n' '{"a":1}' '{"a":2}' | jq -c '.[0]'fails with "Cannot index object with number" and exit 5. The fake then falls through toexit 0with empty stdout, sogh_bounded pr viewsucceeds with no output,json_fieldon empty input exits 4, andpr_mergeable_resolvedtakes itsreturn 2probe-failure branch at :402 - the UNKNOWN retry loop, the attempt counter, and the "persistent UNKNOWN stays unknown" decision at :406-413 are never executed.test_unknown_never_reportedtherefore passes because empty gh output is silent, not because UNKNOWN is handled, leaving the intent-required "never treat UNKNOWN as clean or conflicted" behavior unproven (and the wholepr_mergeable_resolvedpath, including UNKNOWN -> CONFLICTING resolution, untested). Fix: write the sequence as a single JSON array sojq '.[i]'resolves, and add a case where the retries settle on CONFLICTING.bin/fm-pr-conflict-watch.sh:378-IFS=';' read -r -a _parts <<< "$REPORTED_KEYS"followed byfor part in "${_parts[@]}"expands a declared-but-empty array underset -u(:29). On bash before 4.4 (including macOS's system bash 3.2, which this repo supports elsewhere - see the gtimeout/shasum-first fallbacks in bin/fm-timeout-lib.sh and bin/fm-check-lib.sh) that is an "unbound variable" fatal error and the non-interactive shell exits, so the check dies before printing or recording anything. Reachable on the most common path: a clean fleet on the first run has REPORTED_KEYS empty, a MERGEABLE PR callsrecord_remove_key(:449), and the array is empty. Same pattern at :282 inowner_for_projectwhen a secondmate'sprojects:field is empty. The repo already uses the guard idiom for exactly this ("${arr[@]+\"${arr[@]}\"}"in bin/fm-pr-merge.sh:254, bin/fm-test-run.sh:664); apply it here or return early when the source string is empty.bin/fm-pr-conflict-watch.sh:366- REPORTED_KEYS only ever shrinks when an open PR is observed MERGEABLE (:449). Keys for PRs that are merged, closed, or drop out of the PR_LIMIT window are never removed, and keys for repos the sweep no longer visits persist forever, sostate/.pr-conflict-watchgrows without bound (~60 bytes per conflicted head, forever). Unlike the sibling, whose record is the current finding set rebuilt each sweep, this record is accumulate-only. Consider dropping keys whose repo was fully evaluated and whose PR no longer appeared, or aging entries out via the recorded epoch.bin/fm-pr-conflict-watch.sh:217-repo_slugis a byte-for-byte copy ofrepo_slugin bin/fm-bearings-snapshot.sh:197-199, including the identical two-stage sed pipeline. Two independent copies of the same GitHub URL -> owner/repo parser will drift (e.g. when GHES hosts or a new URL form need handling), and the repo's own convention for shared shape is a single owner (see the "ONE OWNER" headers in bin/fm-timeout-lib.sh and bin/fm-line-cap-lib.sh). Move it into a sourced lib - bin/fm-pr-lib.sh already owns provider/path parsing - and have both callers use it.bin/fm-pr-conflict-watch.sh:295-owner_for_repois called once per repo inside the sweep loop, and each call re-runsfirstmate_repo_slug(one git invocation) plusresolve_project_repofor every project until it matches (one git invocation each), re-deriving workdiscover_reposalready did. For N projects that is O(N^2) git spawns per sweep, all charged against the same BUDGET_SECS the GitHub probes need, and none of it is bounded byfm_run_timed. Build the repo->owner map once while discovering repos and look it up, which also removes the secondfirstmate_repo_slugcall site.bin/fm-pr-conflict-watch.sh:455- On the UNKNOWN path the dedupe key and the reportedhead=come from thepr listresponse, but the resolution re-reads the PR withpr viewand already requestsheadRefOid(:401) without using it. If the head is force-updated between the list and the view, the mergeability that gets reported belongs to the new head while the wake line labels it with the old SHA, and the key is recorded against the old head - a wronghead=value routed to the owning team, with no error. Use the view'sheadRefOidwhen the view is what resolved the state.docs/configuration.md:422- The new configuration section is thorough and does document what this does NOT solve, but the repo's two maintained inventories were not updated for the new artifacts: AGENTS.md's state/ catalog has a line for the sibling (tool-updates.check.sh ... its report record .tool-updates ...at AGENTS.md:113) and needs the equivalent forpr-conflict-watch.check.sh, its.check-trustbinding, and the.pr-conflict-watchrecord; docs/scripts.md's bin/ table has a row forfm-tool-update-check.sh(docs/scripts.md:111) and none forfm-pr-conflict-watch.sh.🔧 Fix: fix PR conflict watch cap suppression, budget overrun, record pruning
5 infos still open:
bin/fm-pr-conflict-watch.sh:639-SWEPT_REPOSonly gains a repo whencount < PR_LIMIT, so pruning is disabled entirely for any repo whose open-PR page filled the limit, andrecord_prunethen keeps every one of that repo's keys (the repo is still in DISCOVERED_REPOS, so the else branch preserves them). Concrete: a repo with 30+ open PRs (default FM_PR_CONFLICT_PR_LIMIT=30); PR ci: enforce contributor guardrails #5 conflicts at head A -> keyowner/repo#5#Arecorded; the author force-pushes, it conflicts again at head B -> key#5#Brecorded; A is never removed, and every subsequent force-push adds another permanent key.state/.pr-conflict-watchgrows one line-entry per historical head forever, which contradicts the claim the fix round added at bin/fm-pr-conflict-watch.sh:24-25 ("cannot grow without bound") and docs/configuration.md:445 ("stays the size of the live conflict set"). The conservative gate is right for absence-based pruning, but a per-number rule is safe even on a truncated page: for any repo+number the page did return, a recorded key with the same repo+number and a different head is definitionally stale (a PR has exactly one head), so those can be dropped regardless of whether the page was complete.bin/fm-pr-conflict-watch.sh:508- Whendiscover_reposresolves no repositories, thefor repo in $DISCOVERED_REPOSloop body never runs, soSWEEP_COMPLETEstays 1 whileSWEPT_REPOSandSEEN_KEYSare both empty.record_prunethen sends every recorded key down the else branch, finds its repo absent from the emptyDISCOVERED_REPOS, and drops it - the whole dedupe record is wiped and rewritten asreported=. The next sweep re-wakes every still-conflicted PR in the fleet, contradicting the intent's "the same conflict wakes once". Reachable whenever discovery fails wholesale rather than per repo:action_checkprobes onlyjq(:647) and$GH_AXI(:650), notgit, so a watcher PATH withoutgitmakesfirstmate_repo_slugand everyresolve_project_reporeturn 1; likewise if$PROJECTSis transiently unavailable (stale mount, home being relocated). An unread fleet is not a resolved fleet - gate the DISCOVERED_REPOS-based drop on[ -n "$DISCOVERED_REPOS" ], or setSWEEP_COMPLETE=0when discovery yields nothing.tests/fm-pr-conflict-watch.test.sh:324-test_unknown_rereads_stop_at_the_sweep_deadlinesets FM_PR_CONFLICT_BUDGET_SECS=1, butDEADLINEis computed at bin/fm-pr-conflict-watch.sh:661 beforediscover_reposruns, anddiscover_reposspawns several unboundedgit remote get-urlcalls plus two file reads. If that takes over one second (plausible on a loaded CI box),budget_exhaustedfires at the top of the repo loop,evaluate_repois never entered, the reread loop is never reached, and the test's only assertions -elapsed < 15and empty output - both hold even if the entire deadline guard were reverted. Unliketest_unknown_never_reported, which pins behaviour with the fake's.seqcursor, this test has no evidence the loop was entered at all. Usewrite_view_sequencehere too and assert the cursor advanced but stayed below FM_PR_CONFLICT_UNKNOWN_ATTEMPTS=10, so the test proves the loop ran and was cut short by the deadline rather than never running.bin/fm-pr-conflict-watch.sh:472-record_remove_keyis called for every MERGEABLE PR (:629) and unconditionally runswhile ... done < <(list_parts "$REPORTED_KEYS" ';'), which forks a subshell even whenREPORTED_KEYSis empty - the normal state of a clean fleet. With the default PR_LIMIT=30 across a multi-repo fleet that is one wasted fork per open PR on every sweep, charged against the sameBUDGET_SECSthe GitHub probes need and inside the deadline the fix round just tightened.record_has_key "$key" || return 0at the top makes the no-op case free and leaves behaviour identical.bin/fm-pr-conflict-watch.sh:302-DEADLINEis set at :661 anddiscover_reposruns after it, but thegit -C "$dir" remote get-url origincall it makes per project (and the one infirstmate_repo_slug, :289) is the only external command in the sweep not wrapped infm_run_timed, and there is nobudget_exhaustedcheck between projects. The fix round's own rationale for bounding the UNKNOWN loop applies identically here: this check prints and records only at the very end (:699-702), so a discovery phase that stalls past FM_CHECK_TIMEOUT gets the process group killed and the sweep loses everything, repeating identically on every poll. A clone on a hung or stale mount is enough. The script already sources bin/fm-timeout-lib.sh, so wrapping the two origin reads infm_run_timed "$(probe_bound)"(and breaking discovery onbudget_exhausted) closes it with no new machinery.✅ **Test** - passed
✅ No issues found.
bash tests/fm-pr-conflict-watch.test.sh(12 fixture cases: newly conflicted wake with routing fields, same-head silence, new-head re-wake, persistent UNKNOWN never reported, UNKNOWN settling on CONFLICTING, reread-head labelling/dedupe, line-cap conflicts waking on a later sweep, record pruning, UNKNOWN rereads stopping at the sweep deadline, clean-fleet silence, draft conflicts, arm registration) - run twice to check for flakiness, all pass both timesbash tests/fm-bearings-snapshot.test.sh(31 cases) - the other consumer of the newly extractedbin/fm-repo-slug-lib.sh, all passManual end-to-end:/home/rich/.no-mistakes/evidence/01M0VCRA29HFS9SR262BP9YW00/pr-conflict-watch-e2e.sh- arms the check in a throwaway FM_HOME, then runs the realbin/fm-watch.shcheck sweep against a fakegh-axiover eight scenarios (clean fleet, three routed conflicts including a draft and the firstmate repo, same-head silence, force-updated head, UNKNOWN, resolved, disarm), draining each wake throughbin/fm-wake-drain.shFM_STATE_OVERRIDE=<home>/state bin/fm-check-register.sh pr-conflict-watch- verified the armed shim bytes against the trust bindingdocs/fm-test-portable-shards.md:76- This change adds tests/fm-pr-conflict-watch.test.sh, which lands in the portable serial lane (it appears in portable-serial-2of4) with no duration hint, so it takes PORTABLE_SERIAL_DEFAULT_WEIGHT_MS. The doc's shard table still reads 29/28/30/30 scripts while the lanes now hold 30/32/32/32, and the doc's own rule says "Refresh the hints whenever the serial lane gains scripts". I did not fix it: the table was already stale by eight scripts at the base commit (this change accounts for one of them), and a correct refresh needs measured per-script timings downloaded from a green CI run plus an edit to the portable_serial_weight_hints table in bin/fm-test-run.sh, which is executable code this documentation phase must not touch. Coverage is unaffected (the coverage guard keeps the partition complete and disjoint); only shard balance drifts. Worth a follow-up that runs the refresh procedure the doc already spells out.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.