fix(branchsync): settle wedged custody records at the kept local head - #1
Closed
hmcdaniel03 wants to merge 20 commits into
Closed
fix(branchsync): settle wedged custody records at the kept local head#1hmcdaniel03 wants to merge 20 commits into
hmcdaniel03 wants to merge 20 commits into
Conversation
…chenguid#877) * fix(pipeline): measure agent silence and bound a wedged auto-fix agent A pipeline agent invocation that exhausted its budget reported "agent timed out after 30m0s (agent silent for 30m0s)": the configured budget printed twice, with nothing measured, and on the shared agent-run seam the adapter's own error discarded. A wedged agent, a busy one, and a crashed one were therefore indistinguishable, so two real 30-minute timeouts on one task (one at Review, one at the CI fix step) left no evidence to act on. Three defects composed that failure. The claim was fabricated. classifyAgentRun, reviewAgentError, and testAgentError all restated their own budget as if it were an observation. agentActivity now measures the invocation and is the single owner of the evidence: whether output was ever seen, how long ago the last output was, and whether a subprocess launched. The evidence was discarded. For a native agent the adapter error carries the killed process's exit status and its captured stderr, which is the only account of what the process was doing, including an authentication prompt written to stderr that nothing else surfaces. It is now appended to every timeout diagnostic as "agent reported: ...", and testAgentError no longer replaces it with the bare context cause. Progress was unobservable. Adapters forward only assistant prose to OnChunk, and a tool-using turn emits none until the very end (verified against pi 0.84.3, which streams tool_execution_* and toolcall_* with no text_delta), so a healthy fix round looked identical to a wedge. nativeAgentPipe now raises a throttled LifecyclePhaseActivity for every non-empty read of a native subprocess's stdout or stderr. The executor records it as step activity so axi status can see a live agent, and never writes it to the step log. Subprocess start and exit deliberately do not count as output: start proves launch rather than work, and exit is the deadline's own consequence. The CI path also had no bounded outcome. A fix-agent timeout was logged as a warning and the identical request was re-issued on the next poll, up to auto_fix.ci attempts, each costing another full budget invisibly until ci_timeout ended the run hours later. It now parks at an ask-user gate carrying the measured diagnostic and, when the timed-out agent left uncommitted work, the run worktree holding it. The run and its worktree stay alive, so no commits are lost and no second run has to take custody of the branch, and a further attempt is the operator's to spend. Only a proven budget burn parks; other fix failures keep warn-and-retry. Review deliberately still fails the run rather than parking: the Push step commits leftover worktree changes, so an approved park would ship a half-finished, unreviewed fix. No timeout was raised, no retry added, no agent order or model pin touched. * no-mistakes(document): Document native agent timeout outcomes * no-mistakes(ci): Fixed Greptile's retry-lifecycle finding. Retry and unknown lifecycle metadata no longer count as measured agent output, while subprocess byte activity still does. Added a behavioral regression proving retry events remain forwarded but a retry-only invocation is diagnosed as silent. Verified with make lint and race-enabled pipeline and pipeline/steps tests * no-mistakes(review): Keep fallback notices out of agent activity measurements * no-mistakes(review): Measure subprocess silence from latest launch * no-mistakes(review): Redact adapter credentials from timeout findings * no-mistakes(review): Reset activity evidence between agent attempts * no-mistakes(review): Reset activity before fresh fixer sessions * no-mistakes(document): Document per-attempt timeout evidence * no-mistakes(ci): Fixed Greptile's finding by resetting the activity start timestamp at every retry or fallback boundary, preventing earlier attempts from inflating silence duration when the current attempt never launches. Added a behavioral regression covering a delayed failed attempt followed by a silent, never-launched fallback. Verified with make lint, race-enabled pipeline tests, repeated focused regressions, and the executable silent-agent axi e2e test * no-mistakes(review): Reset activity before OpenCode format fallback * no-mistakes(document): Document all fresh-attempt timeout resets
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
A terminal run whose recorded pipeline head can no longer be verified - missing from every reachable object store, or contradicted by the run's own recovery ref - left the branch pipeline_owned with no command that could settle it: every guarded recovery refused on the same unverifiable head, and abort of the already-terminal run was an idempotent no-op (kunchenguid#824). --recover --keep-local now settles that self-inconsistent record. It stays fail-closed: every reachable copy of the recorded head is pinned at refs/no-mistakes/recover-stranded/<run> first, and a head that still exists but cannot be pinned refuses rather than being stranded. The gate branch moves by the existing compare-and-swap, never a force, so a concurrent gate push still wins. Inspection names that exit as next_action.code: return_custody_keep_local instead of the dead-end manual-reconciliation pointer, and terminal-run abort responses carry the same command. The kunchenguid#814 polarity is preserved: an unverifiable record never advertises recover_custody. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four defects in the kunchenguid#824 keep-local settlement, all in edge sub-shapes rather than the primary flow: - selfInconsistentCustodyRecord advertised the settlement for a terminal run whose head was never verified, but Recover refuses that at its unverified-head guard before any keep-local interception - recreating the same "advertised action that always refuses" wedge. That shape is kunchenguid#707's scope, so the predicate now excludes it and status keeps its honest manual-reconciliation pointer. - A conflicting worktree recovery anchor on the locally reachable path refused without settlement while the predicate advertised it. It is now intercepted like the gate-side shapes; keeping a head that already contains the preserved commits can lose nothing. - The settlement treated any gate-branch rev-parse error as "branch missing" and stamped custody returned without the compare-and-swap. Only a branch proven absent via ExactRefTarget settles without one; an unreadable branch refuses like the sibling keep-local sites. - A refusal claimed "no files or refs were changed" after the pin loop may already have written the stranded anchor. It now names where that anchor exists. Each fix has a regression test that fails without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…document preserve-failed refusal
…help Three findings from the review of 391b3d7: - selfInconsistentCustodyRecord still treated every incompatible gate recovery ref as settleable, but SYMBOLIC evidence is refused before any keep-local interception. A dangling symref is invisible to for-each-ref while symbolic-ref still succeeds, so the predicate said "incompatible" while Recover's own probe saw no ref and then failed inside PreserveRecoveryHead's symbolic check - an un-intercepted refusal. A resolving symref reaches the ordinary keep-local path and refuses there. Symbolic gate-anchor evidence now disqualifies the record outright, keeping the honest manual-reconciliation pointer. - The bare-abort no-op emitted the branch's next action unconditionally, so on a released branch it answered an abort by prescribing `axi run` - telling the operator to launch a pipeline. Abort help is now allowlisted to custody-settlement actions via custodySettlementHelp, applied to all three abort sites so the --run siblings cannot drift into the same overreach. The structured branch_sync object still reports the branch's own next_action, which is state rather than a prescription. - cli.md claimed every listed refusal was settleable with --keep-local; the shapes refused before an interception are not. The predicate's invariant is now stated once: it may name the settlement only where Recover(keepLocal) actually reaches it. Each fix has a regression test that fails without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review findings, both narrowly scoped: - The pipeline_owned gate added to the bare-abort help was never applied to the two `axi abort --run <id>` sites. Their only guard was state.Pipeline.RunID != runID, which proves the branch resolves to that run, not that a run still holds it - so a released, diverged branch reached custodySettlementHelp and answered an abort with `git log` reconciliation advice the bare site had already been taught not to emit. Both conditions are now required, matching the bare site. - settlementAnchorsFree predicts whether PreserveRecoveryAnchor can write an anchor, but that write refuses any symbolic ref first while the probe used ExactRefTarget alone. A dangling symref is invisible to for-each-ref yet visible to symbolic-ref, so the probe reported "free" for a ref the settlement can never write, and inspection advertised a settlement that refuses every time. The probe now mirrors recoveryAnchorCompatible, covering both the stranded and gate anchors. Each fix has a regression test that fails without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes the merge blockers the maintainer named on kunchenguid#883, plus the three smaller published gaps. recoverKeepLocal wrote refs/no-mistakes/recover-gate/<run> before four refusals that each reported "no files or refs were changed" - and that ref is exactly what wedges every later attempt, so those refusals could recreate the condition this settlement exists to clear. The anchor guards one operation, the compare-and-swap that moves the gate branch off the displaced head, and nothing before that swap can strand it because refs/heads/<branch> still names it. So the write moved to immediately before the swap and every refusal that can precede it now leaves nothing behind by construction rather than by a cleanup that could itself fail. The read-only anchor conflict check stays first. One post-write refusal remains, the lost swap, deliberately: it failed because the gate moved, so that pin may be the only ref still naming the displaced head. The TUI keyed its u action on blocked_pipeline_owned_recoverable while a self-inconsistent record carries the settlement next action, so it was the one operator surface with no exit. It now reaches the same settlement as the CLI, keyed on the advertised next action so it cannot drift from the predicate deciding where the settlement can complete, behind its own confirmation because settling KEEPS the local head where recovery takes the preserved one. The settlement's data-safety argument is "nothing still has this head, so nothing can be lost", which only a proven absence supports. Its probe read any non-zero git exit as absent, so an unreadable store and a present non-commit object both settled the record while the head was still there. git.CommitPresence now treats only exit 1 as absence; everything else refuses, and settlementAnchorsFree mirrors it so the advertisement agrees with the write. finishRecover's stamp failure runs after the Git side already succeeded and nilled NextAction, leaving a moved gate branch with no named exit - the same shape one layer down. It now reports that the changes are applied and names the same recovery command, which completes because every Git step it repeats is idempotent once applied. Also: --keep-local help no longer claims the gate always compare-and-swaps, since the equal/ahead, user_owned, already-recovered and absent-gate-branch paths return first; and the two --run abort tests regain the fixture invariants that prove they exercise the StatePipelineOwned clause - their responses carry no branch_sync object, so they could not have failed even with that clause deleted. Refs kunchenguid#824. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VAsKnxZJbLEiUKMcUtCL2U
… from fixture check
…l settlement action
Owner
Author
|
Opened in error by the validation tooling against the wrong base; the real pull request for this work is kunchenguid#883. Closing. |
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
2 participants
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
Work the maintainer's requested revisions on the ALREADY-OPEN PR kunchenguid#883 (kunchenguid/no-mistakes), which fixes issue kunchenguid#824. This is a revision round on an existing PR, not a new change: the PR stays open, the branch stays fm/nm-824-custody-release, and no second PR is ever opened. Delivery must push to origin (our fork hmcdaniel03/no-mistakes) with --force-with-lease on fm/nm-824-custody-release only, so PR kunchenguid#883 updates in place against base kunchenguid/no-mistakes:main.
The maintainer's binding request was: rebase/resolve onto latest main, close known-gap (1) fail-closed, preferably also (2) so the TUI has the same exit as the CLI, then ping. He will not rebase or resolve conflicts himself while the safety gaps remain. The PR body's own "Known gaps" list (items 1-5) is our prior review's list and is what he was quoting.
WHAT THIS ROUND DID.
Rebase: fm/nm-824-custody-release was rebased onto upstream/main (13cdf1a). Only AGENTS.md conflicted, because main extracted that section into .agents/skills/; the resolution takes main's AGENTS.md verbatim (it is now byte-identical to main's) and re-homes our prose into .agents/skills/branch-sync-and-push-safety/SKILL.md, the file that now owns it. No upstream content was dropped to win a conflict.
Gap (1), the merge blocker. In internal/branchsync/sync.go, recoverKeepLocal wrote refs/no-mistakes/recover-gate/ BEFORE four refusals that each reported "no files or refs were changed". That ref is exactly what wedges every later attempt (settlementAnchorsFree stops advertising the settlement once it names a commit the gate has moved off, PreserveRecoveryAnchor then refuses to retarget it, and nothing retires it), so those refusals could recreate the condition this settlement exists to clear while claiming to have changed nothing. DECISION: fixed by reordering the write, not by cleaning up on the refusal path. Rationale: the anchor guards exactly one operation, the compare-and-swap that moves the gate branch off the displaced head, and nothing before that swap can strand the head because refs/heads/ still names it. So the anchor write moved to immediately before the CAS, which makes the no-change claim true BY CONSTRUCTION rather than by a cleanup that could itself fail and leave the same false claim. The read-only anchor CONFLICT check deliberately stays first, since it is the cheapest refusal and must not be reached only after a staging ref exists.
Counts, which MUST be stated explicitly in the PR body because the maintainer read our earlier "four" as an admission: at the pre-fix tip, recoverKeepLocal had 8 refusal returns, 5 of them in a post-anchor-write window, and 4 of those 5 made the untrue "no files or refs were changed" claim. The 5th (the lost compare-and-swap) says "no LOCAL files or refs were changed", names the anchor, and prescribes reconciling it, so it was already honest. Both our "four" and an auditor's "eight/five" were correct about different things; the PR body must say so rather than leave it reading as an error.
The honest form of the post-fix claim, which must NOT be compressed back into "exactly one": there is now one post-write REFUSAL by design (the lost CAS, which deliberately keeps its anchor because that swap failed because the gate moved, so the pin may be the only ref still naming the displaced head) PLUS one post-success FAILURE (the custody stamp, below). The earlier "exactly one post-write window" phrasing was scoped to refusals without saying so.
Gap (2), the TUI dead end. recoverableBranchSync keyed on safety blocked_pipeline_owned_recoverable while a self-inconsistent record carries the settlement next action, so the TUI was the one operator surface with no exit. It now reaches the same settlement the CLI offers. DECISION: the new predicate keys on the advertised next_action.code return_custody_keep_local rather than on a safety code, deliberately, so it cannot drift from the branchsync predicate that decides where the settlement can actually complete; the service never advertises that action for a record the settlement would only refuse. It gets its own confirmation box rather than reusing the recovery one, because settling KEEPS the local head where recovery TAKES the preserved head, and the CLI makes that an explicit --keep-local choice.
Gap (3). recoverSettleInconsistent's pin loop used a probe that read any non-zero git exit as "absent", in the one place the settlement's data-safety argument depends on that distinction. This was NOT cosmetic: verified that with the old probe the settlement COMPLETED (custody_returned, Recovered:true) without ever proving the recorded head absent. New git.CommitPresence treats ONLY git's exit 1 (the store was read and the object is not there) as an absence. Two shapes now fail closed instead of being read as absent: an unreadable store, and a present-but-wrong-type object, since
git cat-file -eexits 0 for a tree, blob or tag and collapsing that into "absent" would assert proven absence about an object that is right there. settlementAnchorsFree mirrors the same probe so the advertisement agrees with the write and a record that can only refuse is never offered the settlement. KNOWN TRADEOFF, deliberate and worth stating in the PR body: a recorded head that git cannot resolve at all now fails closed to manual reconciliation rather than settling, because absence cannot be proven for it.Additional fix found this round, not in the published list. finishRecover's stamp failure runs AFTER the Git side already succeeded - a keep-local settlement has moved the gate branch and written the recovery anchor - and it explicitly set NextAction to nil, so a failed database write left refs changed and named no exit at all. That is the kunchenguid#824 shape one layer down, and it had zero test coverage. It now reports that the recovery's Git changes are already applied (instead of implying nothing happened) and names the SAME recovery command again via recoveryRetryAction. The PR body should say why that is legitimate in R5 terms: naming the same command is only honest because every Git step it repeats is idempotent once applied - the gate now equals the kept head so recoverKeepLocal skips its whole move, and an already-advanced worktree takes the equal/ahead path - and the tests assert the prescribed retry ACTUALLY COMPLETES rather than merely that a next action exists.
Gap (4). The --keep-local flag help claimed unconditionally that the gate compare-and-swaps onto the kept head, but the equal/ahead, user_owned, already-recovered and absent-gate-branch paths return before recoverKeepLocal is reached. Help and docs now say the CAS happens where the gate branch still names a different head, and that custody otherwise returns without changing any ref.
Gap (5). The two --run abort tests had dropped the two fixture-invariant assertions their bare-abort sibling carries. This was worse than the PR body said: a --run abort response carries no branch_sync object at all, so those tests could not have failed even with the StatePipelineOwned clause deleted - verified by deleting it and watching them still pass. They now assert the fixture's classification through the surface that does report it, and were verified to fail when that clause is removed.
TEST DISCIPLINE USED. Every fix was proven by reverting it and watching the new test fail, then restoring it: the four (now five) recover-gate refusal tests, the undetermined-absence test, the present-but-wrong-type test, the two stamp-failure tests, and the two --run abort tests. Two narrow service seams were added following the file's existing seam convention (absPathFn, stampCustodyReturnedFn) because filepath.Abs failure and the post-success database failure cannot otherwise be induced portably - chmod is a no-op on Windows and this package runs on the Windows CI leg.
CONSTRAINTS AND EXCLUSIONS THAT STILL BIND.
Do NOT implement the structural blockedPlan change offered in the PR body's "Structural note" (inverting the nil-NextAction default). It is a separate, much larger change the maintainer did not ask for; mentioning it in the body is fine, implementing it here is out of scope.
Preserve every behavior the maintainer already accepted: default --recover behavior unchanged, compare-and-swap and never a blind force for gate moves, and fail-closed refusal whenever unique unpublished pipeline commits cannot be anchored. This settlement is a shortcut for self-inconsistent records only, never for records protecting unique unpublished content.
Do not re-litigate which refusal sites are "in theme"; the maintainer's comment and the five published gaps are the scope.
The failing "PR must be raised via no-mistakes" check is expected and is NOT ours to fix - the maintainer stated it is ignored when that check is not in the main ruleset, and live HEAD mergeability governs.
DELIVERABLE STATE. The PR body's "Known gaps" section must be rewritten so it no longer advertises defects that are now fixed, and must carry the explicit counts and the one-post-write-refusal-plus-one-post-success-failure accounting described above. make lint, go test -race ./..., and make e2e were all green locally before this run.
What Changed
--recover --keep-localnow settles a self-inconsistent custody record — a terminal run whose recorded pipeline head can no longer be verified — instead of leaving the branch with no command that can complete.branchsyncadvertisesnext_action.code: return_custody_keep_localonly where the settlement can actually finish, pins any surviving copy of the recorded head underrefs/no-mistakes/recover-stranded/<run>before moving anything, and moves the gate branch by the same compare-and-swap, so a concurrent gate push still wins. Newgit.CommitPresencebacks both the advertisement and the write with one probe that treats onlycat-file -eexit 1 as a proven absence, so an unreadable object store or a present-but-wrong-type object fails closed to manual reconciliation rather than being read as "absent".refs/no-mistakes/recover-gate/<run>is now written immediately before the compare-and-swap it guards. At the pre-fix tiprecoverKeepLocalhad 8 refusal returns, 5 of them after the anchor write, and 4 of those 5 asserted "no files or refs were changed" while having written that ref; those 4 now precede the write, making the claim true by construction. What remains is one post-write refusal by design — the lost compare-and-swap, which deliberately keeps its anchor because it may be the only ref still naming the displaced head, and which reports the narrower "no local files or refs" claim and names the ref plus the gate directory — plus one post-success failure, the custody stamp, which now reports that the recovery's Git changes are already applied and re-names the same recovery command (idempotent once applied) instead of returning a nil next action.uoffers "settle custody at local head" with its own confirmation naming the kept head and the unverifiable recorded head, keyed on the advertisedreturn_custody_keep_localaction rather than a safety code so it cannot drift from the service predicate.axi aborton an already-terminal run keeps its no-op but now names the settlement command (allowlisted custody actions only, never a fresh-run launch), and the--keep-localflag help, CLI/TUI/agent docs, and published agent guidance now say the gate compare-and-swap happens only where the gate branch still names a different head.Risk Assessment
Testing
Ran the targeted unit tests covering all five published gaps (branchsync settlement/keep-local refusals/stamp failure, the TUI exit, the fail-closed presence probe, the flag help, and the abort fixture invariants) plus full package runs of internal/branchsync, internal/cli, internal/tui and internal/git — all green. Because nothing exercised the settlement through the command surface an operator actually types, I added a focused CLI journey test that builds the #824 record on real Git repos and the real state database and walks the whole road, asserting persisted state rather than just output: worktree HEAD untouched, gate branch compare-and-swapped onto the kept head, displaced gate head still anchored at refs/no-mistakes/recover-gate/<run>, and custody_returned_at written. I proved the regression both ways by swapping internal/branchsync/sync.go back to upstream main (13cdf1a) and watching the journey fail with every surface pointing at inspect_and_reconcile_manually, then restoring it and confirming a clean worktree. For the TUI I captured the real renderer's ANSI output on both revisions and turned it into reviewer-visible before/after screenshots, showingugoing from bound-to-nothing to a settlement confirmation distinct from the recovery one. The only worktree change left behind is the new test file; temporary capture harnesses were deleted./Users/huntermcdaniel/.no-mistakes/evidence/01M15F2V70W27JBB75S43SRN87/settlement-evidence.png)/Users/huntermcdaniel/.no-mistakes/evidence/01M15F2V70W27JBB75S43SRN87/tui-settlement.png)Evidence: Full CLI transcript, upstream main vs this branch, identical fixture
Evidence: Rendered evidence page (HTML)
/Users/huntermcdaniel/.no-mistakes/evidence/01M15F2V70W27JBB75S43SRN87/tui-settlement.html)Evidence: Key CLI diff: the advertised exit now completes
Evidence: Rendered --keep-local help on both CLI surfaces (gap 4)
--keep-local with --recover: keep the current local head; the worktree is never touched, surviving preserved commits stay anchored, and where the gate branch still names a different head it compare-and-swaps onto the kept head; otherwise custody returns without moving any branch ref. Also settles a record whose preserved head can no longer be verified/Users/huntermcdaniel/.no-mistakes/evidence/01M15F2V70W27JBB75S43SRN87/raw-before.txt)Pipeline
Updates from git push no-mistakes
... (5 earlier update rounds omitted to keep the PR body within GitHub's 65536-char limit; full history is in the run log.)
Narrowest fix: thread the
pinned/strandedRefpair intorecoverKeepLocalso its refusals append the sameanchoredElsewheresuffix the settlement already builds; the write ordering, the CAS, and the advertisement predicate all stay as they are. This is user-visible refusal text and challenges the author's explicit accounting, so it is your call whether to correct the messages or narrow the invariant claims instead.docs/src/content/docs/reference/cli.md:239- The gap-(4) sentence added this round overstates one of the three no-op paths it enumerates: "...and on the paths that return before the gate is reached at all, such as a releaseduser_ownedbranch, an already-returned custody record, and a local head that already contains the preserved one - custody returns without changing any ref."The first two are exact -
Recoverreturns at sync.go:590 and :598 before touching anything. The third is not. "A local head that already contains the preserved one" is the branch at sync.go:651, which callsanchorReachablePreserved(sync.go:1192), and that unconditionally callscustody.PreserveRecoveryHeadon the invoking worktree, creatingrefs/no-mistakes/recover/<run>there. Terminalization pins that ref in the GATE, not in the operator's worktree, so on a first--keep-localfor this shape the ref is genuinely created rather than found. A ref is changed.The same wording appears one clause earlier for "where the gate branch already names the kept head": reaching
recoverKeepLocalwithgateHead == state.Local.Headvia the behind/diverged switch meansFetchRemoteRefplusPreserveRecoveryAnchoralready ran on the worktree, and via the settlement means the stranded pin may already exist.The flag help in
internal/cli/sync.gois fine ("surviving preserved commits stay anchored" acknowledges it); only this reference sentence claims no ref at all. The intended meaning is clearly "no BRANCH ref moves", which the surrounding paragraph says correctly elsewhere - so the narrowest fix is to say "without moving any branch ref" here. Flagging rather than fixing because it is published product wording in the exact class of claim this round is auditing.🔧 Fix: make keep-local refusals name the anchor they wrote
2 issues (1 error, 1 warning) still open:
internal/cli/axi_abort_custody_test.go:440-assertDivergedReleasedFixtureInvariantsreads the fixture's classification by running the BAREaxi abort, and its doc comment requires that it be called "before any fake daemon is started". But the bare abort path is the one abort path that requires a live daemon:runAxiAbort(internal/cli/axi_drive.go:947) callsopenAxiDaemonEnv→openAxiEnvWithOptions{ensureDaemonConn: true}→daemon.EnsureDaemon(p).Concrete sequence:
wedgedCustodyAbortFixturesetsNM_HOMEto a fresh temp dir and never starts a daemon, sodaemonIsRunningViaIPCstats a missing socket and returns(false, nil);EnsureDaemonthen callsdaemonStart→Start→ (managed service bypassed undergo test, internal/daemon/service.go:270) →startDetachedDaemon, which re-execs the TEST BINARY as<testbin> daemon run --root <NM_HOME>. That child hitshelpers_test.go:60, seesNM_TEST_START_DAEMONunset (onlysetupTestReposets it,helpers_test.go:140; this fixture does not), returns frominit(), and falls intoTestMain/m.Run()— so it runs the cli test suite instead of serving the socket.waitForDaemonStartWithProcesspolls health until the default 45sNM_TEST_DAEMON_START_TIMEOUT, then returnsdaemon launched but did not become ready.axi abortemits that as an error,executeCmdreturns non-nil, and the helper'st.Fatalf("reading the fixture classification must not fail")fires.Both callers are affected:
TestRunScopedAbortNoOpEmitsNoHelpForOrdinaryDivergence(line 355) andTestRunScopedAbortNoOpEmitsNoHelpForOrdinaryDivergenceWithDaemon(line 371) stall ~45s each and then fail, and each leaves a detached test-binary child re-running the package.The two sibling bare-abort tests in this same file start
startNoActiveRunDaemonfirst (lines 217/317), andTestAxiRunReportsInvalidGlobalConfig(internal/cli/axi_test.go:797) clampsNM_TEST_DAEMON_START_TIMEOUTto 100ms specifically because a daemon cannot come up in this package — both confirm the requirement.Narrowest fix: read the classification without the CLI/daemon at all - open the DB,
findRepo, and callbranchsync.Service{...}.InspectCached(ctx)directly (the same readterminalRunCustodyHelpWithDBperforms), assertingState == StateDiverged/Safety == blocked_divergedand theinspect_and_reconcile_manuallynext action. That keeps both fixture invariants without needing a daemon, and works for theWithDaemonvariant too, wherestartInactiveAbortDaemondoes not even serveMethodGetActiveRun.internal/branchsync/sync.go:1013-keepLocalNoChangeClauseDISCARDS the blanket string whenever an anchor note is present, and it is applied to the lost-compare-and-swap refusal too. That refusal's blanket is"no local files or refs were changed"- deliberately scoped to local because on that path the attempt has just writtenrefs/no-mistakes/recover-gate/<run>in the gate. When the caller supplies an anchor note (the settlement path, wherepinnedis non-empty), the message becomes"... ; no branch, worktree, or file changes were made; the recorded head is now anchored at refs/no-mistakes/recover-stranded/<run> in ...", dropping thelocalqualifier in the one branch where a non-local ref file was written by this very attempt.Concrete reachable state: conflicting gate recovery anchor, recorded head still present in the gate (the
TestRecoverKeepLocalSettlesConflictingGateAnchorAndPinsPreservedHeadfixture), gate branch at G with G != local head and G != recorded head.recoverSettleInconsistentpins the stranded ref, delegates with a non-empty note,recoverKeepLocaltakeswriteGateAnchor = true, writesrecover-gate/<run>, and a concurrent gate push then loses the CAS - producing a refusal that names the anchor it wrote and, in the same sentence, claims no file changes were made.It also contradicts documentation written in the same commit:
docs/src/content/docs/reference/cli.md:239still ends "That refusal names the ref and the gate directory, and reports that no local files or refs were changed", which is now false for the settlement path. No test covers it -wedgedCustodyFixturepins nothing, soanchoredNoteis empty inTestRecoverKeepLocalSettlementLosesConcurrentGatePushCleanly.Narrowest fix: on the two gate-race sites, APPEND the anchor note to the existing local-scoped blanket instead of replacing it (i.e.
"no local files or refs were changed" + anchoredNote), leavingkeepLocalNoChangeClausefor the genuinely pre-write refusals. This is user-visible refusal wording in the exact claim class this round is auditing, so it is your call whether to correct the message or the cli.md sentence.🔧 Fix: keep lost-swap refusal local-scoped; drop daemon from fixture check
2 issues (1 warning, 1 info) still open:
internal/cli/sync.go:97- Gap (4) is closed on three of four help surfaces but not on theaxi sync --keep-localflag help, which still makes the exact unconditional claim the gap was raised to correct.The intent states gap (4) as: "The --keep-local flag help claimed unconditionally that the gate compare-and-swaps onto the kept head, but the equal/ahead, user_owned, already-recovered and absent-gate-branch paths return before recoverKeepLocal is reached. Help and docs now say the CAS happens where the gate branch still names a different head, and that custody otherwise returns without changing any ref."
The human
syncflag help was corrected (sync.go:61): "...and where the gate branch still names a different head it compare-and-swaps onto the kept head". Theaxi syncLong text was corrected too (sync.go:78-80). But sync.go:97 still reads "surviving preserved commits stay anchored and the gate branch compare-and-swaps onto the kept head" - no conditional.That is false for every path that returns before
recoverKeepLocal:Recoverstamps custody viafinishRecoveron the equal/ahead branch (sync.go:670) without touching the gate at all,recoverSettleInconsistentreturns early when the gate branch is proven absent (sync.go:859) orgateDiris empty (sync.go:847), andrecoverKeepLocalitself skips its whole block whengateHead == state.Local.Head(sync.go:955).axi sync --helpprints the flag line, so an agent reading only that surface gets the overstatement. No test pins any of the four help strings, so nothing catches the drift.Narrowest fix: bring sync.go:97 in line with sync.go:61 - e.g. "...surviving preserved commits stay anchored, and where the gate branch still names a different head it compare-and-swaps onto the kept head; otherwise custody returns without moving any branch ref".
docs/src/content/docs/reference/cli.md:239- The closing sentence of the--keep-localparagraph attributes to one refusal properties that belong to two different refusals, so on either reading part of it is false. The trailing clause added in the latest fix round (24c96a4) is what forces the ambiguity into an outright contradiction.The sentence: "...later attempts refuse on the anchor conflict until you reconcile that ref in the gate by hand. That refusal names the ref and the gate directory, and reports that no local files or refs were changed - it keeps that narrower claim precisely because it has just written a ref in the gate..."
The two candidate referents behave differently:
gateHead,gateAnchor, ands.GateDir, so it does name the ref and the gate directory - but it closes withkeepLocalNoChangeClause("no files or branch refs were changed", anchoredNote), never "no local files or refs", and it is a read-only refusal that has written nothing, so "it has just written a ref in the gate" is false of it.custody.RecoveryGateRef(run.ID);s.GateDirnever appears in it, so "and the gate directory" is false of it.The grammatical antecedent is the anchor-conflict refusal, while the new trailing clause describes the lost-CAS refusal. The
.agents/skills/branch-sync-and-push-safety/SKILL.mdversion of the same claim is correct - it says the lost CAS "names the ref to reconcile" with no mention of the gate directory - so only cli.md is drifted. The round-2 decision explicitly asked that cli.md be kept in step with whatever the code ends up saying, which is why this is worth naming rather than leaving.Narrowest fix: split the two, e.g. "That later anchor-conflict refusal names the ref and the gate directory. The lost compare-and-swap itself reports that no local files or refs were changed and names the anchor to reconcile - it keeps that narrower claim precisely because it has just written a ref in the gate, and where the attempt had already anchored the preserved head it appends the same anchor note rather than dropping the
localqualifier."🔧 Fix: condition axi sync keep-local flag help on gate state
1 info still open:
internal/branchsync/sync.go:1253-finishRecover's stamp-failure branch replaced the old neutral wording with a positive claim - "the recovery's Git changes are already applied" - but three of its callers reach it having applied nothing at all, so on those paths the new message asserts a mutation that never happened.Concrete reachable state (one step, and the seam to induce it already exists): the wedged custody record with the gate branch deleted.
recoverSettleInconsistent's pin loop finds the recorded head in no store, sopinnedstays empty and nothing is written;git.ExactRefTargetthen reports the gate branch absent and sync.go:860 returnss.finishRecover(ctx, run, false, true)with zero refs, files, or objects touched. IfstampCustodyReturnedfails there, the operator is told the recovery's Git changes are already applied.TestRecoverKeepLocalSettlementTreatsAnUnreadableGateBranchAsUnknown's "absent gate branch settles without a gate move" subtest builds exactly that fixture, andstampCustodyReturnedFnis the seam that fails the write. The same holds for sync.go:848 (gateDir == "") and sync.go:1025 (recoverKeepLocalskipping its whole block becausegateHead == state.Local.Head).The two tests that pin this message both use fixtures where the gate really did move (
TestKeepLocalStampFailureAfterTheGateMovedNamesACompletableRetryasserts the moved gate branch and the writtenrecover-gateanchor;TestDefaultRecoveryStampFailureNamesItsOwnRetryuses the plain fixture where the worktree moves), so neither covers a no-op settlement.No safety consequence: the prescribed
return_custody_keep_localretry is still correct and completable, and nothing was left behind to reconcile. It matters only because this branch's whole standard is that a state's message must be true of what it actually did, and this is the one claim added by the fix rounds that runs the other way - overstating instead of understating. Narrowest correction is to scope the sentence to what is known, e.g. "any Git changes this recovery makes are already applied, but the custody return could not be recorded; re-run the same recovery to complete the record", leaving the safety code, therecoveryRetryActionnext action, and both existing tests untouched. This is user-visible wording, so it is your call whether to correct it or accept the looser reading.internal/cli/axi_settle_custody_test.go- new test file written by agent: internal/cli/axi_settle_custody_test.gogo test -race ./internal/branchsync/ -run 'Settle|Settlement|KeepLocal|Stamp|Wedged|RefusalsNameAnExit|AdvertiseSettlement|LeavesNoGateAnchor|AnchorItAlreadyWrote|AnchorTheAttemptWrote'(33 tests, all pass)go test -race ./internal/tui/ -run 'Wedged|Settlement|Recover'— covers TestWedgedCustodyRecordReachesTheSameSettlementExitAsTheCLI and TestPipelineOwnedStateWithoutASettlementActionOffersNoSettlementgo test -race ./internal/cli/ -run 'Custody|Abort|Settlement|Guidance'— abort-surface settlement help and the gap-5 fixture-invariant assertionsAdded and rango test -race ./internal/cli/ -run TestWedgedCustodyRecordSettlesThroughTheCLI— new end-to-end journey:axi sync --check->axi sync --recover(refuses) ->axi sync --recover --keep-local(settles) ->axi sync --check(released), plus live-Git and DB assertionsRegression proof: replacedinternal/branchsync/sync.gowith the 13cdf1a version and re-ran the journey test — it fails (settlement refuses, every next_action isinspect_and_reconcile_manually); restored the file and verified a cleangit statusTUI before/after capture: renderedrenderLocalBranchStatusand theukeypress against the wedged state on both 13cdf1a (branch_sync.go/keys.go/view.goswapped) and HEAD, then restored all filesgo test -race ./internal/cli/ ./internal/tui/ ./internal/git/ -count=1andgo test -race ./internal/branchsync/ -count=1— all four packages greenCaptured renderedno-mistakes sync --helpandno-mistakes axi sync --helpto confirm the gap-4 conditional--keep-localwording on both surfaces✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.