fix(cli): stop status, diff and reconcile hanging on a non-regular destination - #240
Merged
Merged
Conversation
A FIFO at a managed destination hangs agentsync. os.ReadFile on a FIFO does not fail, it BLOCKS in the open waiting for a writer that never comes, so the read's own error path never runs and the command never returns. Measured at f6aa686, with the destination swapped for a 0600 FIFO after a clean apply: whole-file dest diff, reconcile --auto-safe hang; status clean key-merge dest status, diff, reconcile hang status was already safe for the whole-file shape because hashFile applies render.IsRegularOrAbsent — and its comment claims it "shares render's predicate so the destination-read guards cannot disagree about what is safe to read". That was true of the hash and false of every other destination read. docs/components.md:394 made the same claim about internal/cli as a whole; it was written ahead of the code. All seven destination reads in internal/cli now go through readDestBytes, which applies the same predicate before the open. An ABSENT path is deliberately let through to os.ReadFile, whose ENOENT is the truthful answer every caller already handles; manufacturing a shape error for a file that is not there would name the wrong problem. Four of the seven are measured hangs, each pinned by a timeout-bounded test — this class does not fail, it hangs, so an unbounded test would wedge CI with no diagnostic rather than report in 5-8s: - readDestFile, the key-merge read ALL FOUR drift walks share - diff's per-op whole-file read - reconcile's per-op whole-file read - writeBackFileItem, which a user reaches one keystroke LATER: with only the classification reads guarded, a FIFO dest classifies as drift, the user presses [w], and the hang lands there instead The other three are import's state-seeding reads. They are NOT proven hangs: no fixture was found that reaches them with a non-regular destination, and with every guard removed `agentsync import` still returned. They were routed through the gate anyway, because three structurally identical unguarded reads beside four guarded ones invite the question "why those and not these", and "nobody found a fixture yet" is not an answer. TestEveryDestinationReadGoesThroughTheGate keeps that decision from rotting: a new bare os.ReadFile(op.Path) under internal/cli fails even where no hang can be demonstrated. Its proof-of-life arm earned its keep immediately — the first draft used os.ReadDir(".") and silently scanned zero files. Deliberately NOT touched: internal/render and the adapter Apply paths also read op.Path directly, but those are the write path with their own upstream handling (render.isRegularOrAbsent's doc names apply's pre-delete read). Unaudited here; the guard is scoped to internal/cli and claims nothing about them. Break-verified: removing the gate fails all four measured sites at their timeouts with the right diagnostic, and the guard test names the offending file when a bare read is planted back into diff.go. Groundwork for #229, which unifies these four drift walks; the guard has to land first so the shared walk inherits one dest-read policy instead of four. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
This was referenced Sep 1, 2026
… fixed Closes round-1 review findings on PR #240. Three of four lenses converged on the same two blockers, and both were mine. 1. The e2e import rows were VACUOUS, and the claim they backed was wrong. `import` is cobra.ExactArgs(1); the rows passed no agent selector, so cobra rejected them before RunE and they executed zero lines of import code. They passed identically with the fix reverted. That broken measurement is what the previous commit's "import's three sites are unproven hangs" rested on. A real `agentsync import claude` DOES hang, at d4fb233, with the gate in place — the read is upstream in the adapter Ingest paths, which this gate never covered. Rows removed rather than fixed, because asserting them would be asserting a bug. runBounded now fails any invocation cobra rejects, so this class cannot recur silently. 2. `apply` and `apply --dry-run` hang on the same fixture (measured rc=124), through render.Writer.Write's convergence read. `apply --dry-run` is advertised read-only. Not fixed here: between internal/render and the adapter Ingest paths that is ~60 sites across eleven packages, which is a sweep of its own, not a prerequisite bugfix. Filed as #241 and #242 with the measurements and the suggested shape. Consequently the PR's claims are narrowed to what it does: the drift-read path in internal/cli. The CHANGELOG, docs/components.md, destread.go's doc comment and the guard test's stated invariant all said or implied more. 3. hashFile was an EIGHTH destination read holding a second copy of the policy (guarded, so not a hang — but "the guards cannot disagree" was a coincidence rather than a property, and #229's shared walk would have inherited two policies since all four walks call it). It now goes through readDestBytes, which also gives errDestNotRegular its first production errors.Is consumer. Behavior-preserving: the existing TestHashFile_FIFODoesNotBlock catches the sentinel collapsing. 4. The symlink axis: destread.go claimed "Same predicate, so the statement is now true of all of them". False — hashFile Lstats and refuses a symlink, readDestBytes does not, so status calls a symlinked dest drifted while diff reads through it. Pre-existing (the old bare reads followed links too) and deliberately unchanged, because AGENTSYNC_ALLOW_SYMLINK_DEST=1 is a documented setup where apply writes through the link. Documented as a known divergence and left to #229's behavior pass. Also: the guard test gained the synthetic negative control both prior guards in this package run every time, driving the real matcher over planted sources in both directions; its LIMITS now name what it cannot see (readDestFile's own `os.ReadFile(path)`) and that it says nothing about apply/import. writeBackFileItem's refusal names a next step like its peers, and a test pins both the path wrap and the guidance — dropping the wrap previously left the suite green, so the pathless sentinel's rationale was untested. Break-verified: dropping the wrap fails on both arms; collapsing hashFile's sentinel fails TestHashFile_FIFODoesNotBlock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
spxrogers
changed the title
…a hang Closes round-2 review findings on PR #240. Both blockers were mine, and both are the same defect as round 1's, re-made inside the fix for it. 1. The anti-vacuity check added last round could NEVER FIRE. NewRoot sets SilenceErrors, so cobra prints nothing when it rejects an invocation — the text exists only in Execute()'s returned error, which runBounded discarded with `_ = root.Execute()`. Measured: `import` -> buffer "", error "accepts 1 arg(s), received 0". So the guard written to stop a vacuous row was itself vacuous, and `runBounded(t, 8s, "import")` still passed silently. Worse, the buffer carries ordinary stdout, so the substring match was live against user content: a rendered file containing the words "unknown command" failed a plain `diff`. Inert against its target, false-positive against real output. It now reads the returned error through isCobraRejection, and TestRunBoundedRejectsAnInvocationCobraRefuses is the positive control — the check only ever fires on a broken invocation, so without one nothing in a green suite proves it still works. That absence is exactly how the first version shipped. 2. `reconcile --auto-override` HANGS (measured rc=124): [o] re-applies through render.Writer.Write, whose convergence read is the unguarded one #241 covers. Two consequences, both introduced by this branch: - the CHANGELOG headline claimed `reconcile` was fixed, while the e2e only ever exercised --auto-safe, which never reaches that branch. Test and claim agreed with each other rather than with the code. - the guidance added LAST round told the user to "use [o]verride", walking them out of a clean refusal into an unbounded wedge. It now names the remedy that works, and a test asserts the message does NOT recommend [o] — the peer refusals in that file all do, so symmetry would quietly restore it. 3. Stale prose, instance #6: the guard test still said "with every guard removed `agentsync import` still returned", the sentence resting on the no-selector measurement — contradicted by its own LIMITS fifteen lines below. Rewritten: import's hang is real but upstream in the adapter Ingest reads (#242), so guarding these three sites neither fixed it nor could have. Instance #7: render.IsRegularOrAbsent's doc still named hashFile as its outside consumer. 4. Two behaviors were UNPINNED — the suite stayed green with hashFile returning the shape sentinel for every error (absent included, a branch its own suite takes 43 times) and green again with the symlink arm deleted, despite destread.go asserting that divergence in prose. TestHashFileSentinels pins all three sentinels and both halves of the symlink asymmetry. 5. The guard's negative control was tautological: built from the same slice it checked, so it could not notice a pattern being dropped. It now plants literals; break-verified by removing a pattern. Known-hanging commands are now SKIPPED rows rather than absent ones — greppable, visible in -v, and whoever closes #241/#242 deletes one line to inherit the assertion. Asserting the hang would cost 8s a row and fail when it is fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Closes round-3 review findings on PR #240. 1. The anti-vacuity check was still inferring. It matched substrings against cobra's error prose, which covers only the arg/flag layer — anything rejecting later scored as "it ran", including this repo's own enforceScopeStance, a PersistentPreRunE refusal that never reaches RunE. runBounded now WRAPS the resolved command's RunE, so "did the body start" is observed rather than deduced, and cannot drift with cobra's wording. Third version of this check; the first two were unable to fire at all. It is also falsifiable now. runBoundedE reports instead of failing, so TestRunBoundedDetectsACommandThatNeverRan can assert ran==false for a missing argument, an unknown command AND a PersistentPreRunE refusal — the case the substring list structurally could not catch — plus ran==true for a command that does run. Previously nothing failed if the check were reverted. 2. Subtest fixture bleed. The cleanup unlinked the FIFO but never restored the applied file, so the key-merge subtest ran against a home whose whole-file destination was missing, contradicting the test's own "a real applied home". Measured: with the skips deleted, `import claude` and `reconcile --auto-override` PASSED in a full run and HUNG in isolation — whoever closes #241/#242 would have inherited a row green for the wrong reason. The destination is now restored, and after the fix the row hangs both ways. 3. writeBackFileItem appended "remove or replace the non-regular file at that path" to EVERY read failure, including ENOENT. Deleting a managed file is itself drift and offers [w], so the common path produced "no such file or directory — remove or replace the non-regular file at that path": advice for a situation the user is not in. Gated on errors.Is(err, errDestNotRegular), with a test for the absent case. Third round running that this one message has been the site of a new defect. 4. Prose, instances #8 and #9. The CHANGELOG headline said a "directory" no longer hangs — a directory never hung (os.ReadFile fails it in ~18us with EISDIR); only the diagnosis changes. And destread.go justified leaving the symlink split by claiming AGENTSYNC_ALLOW_SYMLINK_DEST=1 would break, but that variable is read only in internal/iox, on the WRITE path, so a read gate cannot affect it. The real reason is that changing it changes what diff and reconcile have always reported (#229) — and the real consequence, now named, is that under that supported setup `status` reports drift no apply can clear. 5. The hash row computed its expectation with the function under test; salting hashContent left it green. Pinned to a literal digest, break-verified. Also: docs/components.md's "Enforced, not asserted" overstated a two-spelling text matcher whose own LIMITS exempt a read; softened, and the review-audit parenthetical that had survived into a website-mirrored contract page is gone. destread.go now documents that `diff` and readDestFile swallow the refusal and render a refused destination as empty — a poorer diagnosis than it deserves, left to #229 because fixing it changes what those commands print. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…oduced Closes round-4 review findings on PR #240. Three of the four are defects that round 3's fixes introduced. 1. The fixture restore added last round could write INTO the FIFO it was meant to replace. os.WriteFile opens the destination; opening a FIFO blocks until the other end appears, and t.Cleanup has no per-row timeout, so with no reader it wedges the suite outright. With a reader — which is what a timed-out row leaves parked in open(2) — it SUCCEEDS, drains the bytes into the pipe, returns nil, and leaves the FIFO in place, so the error check never fires and the destination is silently not restored. restoreDest now renames, which never opens the target, and TestRestoreDestReplacesAFIFOEvenWithAReaderAttached parks a reader and measures it. Break-verified: writing directly fails with "destination is p--------- after restore". 2. That restore's comment also carried a measurement I did not make. It claimed un-skipped rows PASSED in a full run and HUNG in isolation; eight configurations could not reproduce it. What actually varies is goroutines left parked by PRECEDING timed-out rows, which is nondeterministic and orthogonal to the restore. The structural reason for restoring stands on its own and is all the comment now claims. This is the round-1 error repeated: relaying a reviewer's measurement as established fact. 3. destread.go claimed "status, doctor and reconcile's write-back all name the shape correctly". Measured false for two of three: doctor performs NO destination read at all and reports "all checks passed" over a FIFO, and status maps the refusal to an opaque hash sentinel that statusItem never carries, so the user sees a bare "drift". Only reconcile names it. That sentence was the contrast justifying leaving diff's silent swallow alone, and the truth argues the other way; the comment now lists what each surface actually does, and the gap is still #229's to close. 4. Withholding [o]verride was over-corrected. It is unsafe only for a NON-REGULAR destination, where it hangs (#241). For an ABSENT one — the common case, a deleted managed file — Writer.Write's convergence read gets ENOENT and falls through to the write, so [o] is safe and is the fix. The two arms now match their failure, both pinned. Also: runBounded/runBoundedE returned an output string no caller read; dropped. runBoundedE's doc said it "returns rather than failing" while still failing on the timeout path; it reports the vacuity verdict, and now says so. destread.go lost the round-3 retraction framing (keeping the fact), states that its stat is racy against a reshape rather than implying atomicity, and the CHANGELOG's directory parenthetical no longer credits a change to the decode path that never happened there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…t errors Closes round-5 review findings on PR #240. 1. BLOCKER, and it was my own test: TestRestoreDestReplacesAFIFOEvenWith AReaderAttached was RACY and usually hung. The reader goroutine did a blocking os.Open on the FIFO and raced restoreDest — if the rename landed first the open returned instantly on a regular file and the test passed; if the reader won it parked forever, because restoreDest's whole purpose is never to open the target. So the test written to prevent an unbounded hang reproduced one, and my single green run had simply won the race. Two lenses lost it; so did every run since. The reader now attaches with syscall.O_RDONLY|O_NONBLOCK, which returns immediately and leaves a genuinely attached reader instead of a racing one, and restoreDest is called on the test goroutine (its t.Errorf no longer fires off-goroutine). Deterministic both ways: 3/3 pass, 3/3 fail when reverted to writing the path. 2. errDestNotRegular was returned for stat failures that are not absence. render.IsRegularOrAbsent answers false for EACCES-on-a-parent and ELOOP too, so a symlink loop reached reconcile's refusal and told the user to "remove or replace the non-regular file at that path" — a false statement about their destination, with the real errno discarded. Those now report as themselves. render.IsRegularOrAbsent is deliberately still the authority on SHAPE, at the cost of one extra stat on an error path: inlining the predicate would have falsified docs/components.md's claim that it is shared with this package's destination reads. Pinned with an ELOOP fixture (root in the container makes an EACCES fixture unenforceable). 3. Prose #13: "doctor reads no destination at all" is false. Its plugin check reaches one through claude.IngestPlugins -> a bare os.ReadFile of settings.json, a managed destination. A reviewer measured doctor hanging there; a second fixture exited 1 because another issue was reported first, so the comment records the read as unguarded by inspection and leaves reachability to #242, which now lists doctor in scope. 4. The guard test cited render.isRegularOrAbsent's doc as authority for the write path being handled. That doc says the predicate is what stops `apply --dry-run` hanging, which is false — Writer.Write never calls it and apply --dry-run still hangs (#241). The comment now says so rather than leaning on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Closes the rest of round 5. The deadlocking test and the lying sentinel went in 7dd8d9f; these are the two remaining findings. 1. restoreDest's chmod was UNPINNED — no test asserted the restored mode, so dropping it left the suite green. os.WriteFile's mode argument is masked by umask on create, so without the chmod a restored destination can come back more restrictive than the one captured. Now asserted; break-verified by dropping the chmod under `umask 0077`, which fails with "restored mode = 0600, want 0644". 2. Proportionality. The test-rigor lens' verdict was that the behavior tests are proportionate but the prose is not — 68 comment lines to 13 code lines in destread.go, with each round adding scaffolding and then scaffolding for the scaffolding. Acted on: the doc is 52/16, and the biggest block, a catalogue of what every OTHER command does with a refused destination, is gone from here entirely. It documented other packages' behavior in the wrong place and it is only actionable in the shared walk, so it now lives on #229 with the measurements, alongside the symlink split and the EACCES-classifies-as-Orphan case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
Closes round-6 review findings on PR #240. All four lenses reported: two CLEAN, two holding on the same item. 1. The chmod pin was VACUOUS in CI. Three lenses caught it. At the ambient umask 0022, os.WriteFile(_, _, 0o644) already yields 0644, so deleting restoreDest's chmod left the assertion green — and my own break-verification had "passed" only because I ran it under `umask 0077` in a subshell. I verified my shell, not the test. The fixture now captures 0o666, which masks down to 0644 on create, so only the chmod can produce it; break-verified at the DEFAULT umask, no process-global umask manipulation needed. 2. A base-parity regression I introduced in round 5. The stat-error arm moved hashFile's answer for an unstattable destination (parent ENOTDIR/ELOOP/ EACCES) from "not-a-regular-file" to "", which drift.Classify reads as absent — turning ForeignCollision into New when nothing was applied, and New is SafeForAutoApply. The naive fix breaks a different case: hashFile cannot tell a stat failure from a read failure by errno alone, and EACCES on the FILE answered "" at base. So the gate now has TWO sentinels. errDestNotRegular means the shape is wrong; errDestUnstattable wraps a real stat errno. reconcile keys on the first alone, so its "remove or replace the non-regular file" line stays truthful for a permission problem, while hashFile — whose sentinels are opaque tokens compared only for equality — maps both alike and regains exact base parity. Pinned by a new unstattable row; the fix had been UNPINNED, which is the same defect it was fixing. 3. Inverting the stat order fixed two findings at once. render.IsRegularOrAbsent is asked FIRST, so the ordinary read costs one stat and only the refusal path pays a second to tell shape from stat failure. That makes the comment's cost claim true rather than merely reworded — three lenses flagged it as false, since the stat had been unconditional. 4. Prose #15: my round-5 CORRECTION was over-broad. The guard test said render.isRegularOrAbsent's `apply --dry-run` claim "is false"; it is true of the orphan-delete read that predicate guards (writer.go:346, reached via OrphanDeleteWillProceed) and false only of Writer.Write's convergence read. Scoped. 5. doctor is a measured hang, not merely "exposed": mkfifo ~/.claude/settings.json then `doctor` wedges at rc=124 after printing "Plugins", via claude.IngestPlugins. My two earlier failures to reproduce were fixture bugs — .claude did not exist, so mkfifo itself failed and I measured a run with no FIFO in it. Stated as measured, and added to CHANGELOG's not-fixed list. No e2e row: neither existing destination shape is settings.json, so a row would pass vacuously. Also: restoreDest no longer leaves its temp file behind on a failure path (the contamination it exists to prevent); `explain` joins the CHANGELOG's fixed list (it reads destinations through readDestFile too); hashFile's doc describes what it now returns; and reconcile.go is back to the base count of over-long lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
…path Closes round-7 review findings on PR #240. Two lenses returned CLEAN; the one real finding is that a fix I reported as done was not in the tree. 1. `43f4464`'s message said prose #15 was "Scoped." It was not. That commit does not touch destread_guard_internal_test.go at all — my script applied three replacements to dest_fifo_e2e_unix_test.go, but the third pattern lives in the guard test, and str.replace() silently no-ops on a missing pattern. Every other edit in that script asserted count==1; that one did not, so a silent miss became a claim of work done. Caught only by `git show --name-only`. Now actually applied: render.isRegularOrAbsent's `apply --dry-run` claim is TRUE of the orphan-delete read it guards and false only of Writer.Write's convergence read. Every edit in this commit went through an assertion, and a post-hoc audit greps the tree for each one. 2. The second sentinel violated the first's stated principle. errDestNotRegular is deliberately pathless because callers wrap it with the path; but errDestUnstattable wrapped a *fs.PathError, so reconcile printed "read dest X: cannot stat destination: stat X: not a directory". It now unwraps to the bare errno via pathlessStatErr, mirroring secrets.pathlessErr, and errors.Is still matches both the sentinel and the underlying syscall error. Pinned by counting path occurrences rather than matching a literal, so a reworded message will not break it; break-verified. 3. `internal/render/writer.go`'s doc claimed the predicate is what stops `apply --dry-run` hanging on a FIFO — flagged by two lenses and disproved by this PR's own skipped rows. It is true of the pre-delete read it guards and false of the convergence read; the sentence now says exactly that rather than being left for #241 to correct later. Also: "Two deliberate limits:" headed three bullets (the second was added in round 6 without bumping the count, and is a design statement rather than a limit); a reflow had orphaned "// whose" on its own line; and docs/components.md's not-fixed list omitted `doctor`, which the CHANGELOG and destread.go already named. Correctness validated base parity empirically rather than by reading: a verbatim copy of f6aa686's hashFile compared against head across 13 input classes, run as root AND as an unprivileged uid so the EACCES rows genuinely denied. All 13 identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG
This was referenced Sep 3, 2026
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.
Summary
Groundwork for #229.
os.ReadFileon a FIFO does not fail — it blocks in the open waiting for a writer that never comes — so the read's own error path never runs and the command never returns.Measured at
f6aa686, destination swapped for a0600FIFO after a clean apply:SKILL.md)diff,reconcile --auto-safestatus~/.claude.json)status,diff,reconcile --auto-safestatusis advertised as read-only and wedges on the second row.Scope — read this first
This PR fixes the drift-read path in
internal/cli, and nothing else.apply,apply --dry-run,reconcile --auto-override,import <agent>anddoctorstill hang on the same fixture; their reads live ininternal/renderand the adapterIngestpaths — ~60 call sites across eleven packages, a sweep of its own rather than a prerequisite bugfix. Both are filed with measurements and a suggested shape:apply/apply --dry-run/reconcile --auto-overridehang viarender.Writer.Write's convergence read.import <agent>anddoctorhang viaadapter.ReadFileOptional(27 callers) plus 31 direct reads across the adapters.Those commands are present in the end-to-end table as
t.Skiprows citing their issues, verified to fail with the right diagnostic when un-skipped. Whoever closes #241 or #242 deletes oneskip:string to inherit a working assertion.The change
Every destination read in
internal/cligoes throughreadDestBytes(internal/cli/destread.go), which stats before opening. That includeshashFile, which previously held a second copy of the rule — guarded, so never a hang, but it made "the guards cannot disagree" a coincidence rather than a property, and #229's shared walk would have inherited two policies since all four walks call it.The gate distinguishes two facts, because they are different claims and one of them reaches a user:
errDestNotRegular— present, wrong shape.errDestUnstattable— shape unknown; wraps the real errno, pathless so callers can supply the path without doubling it.reconcile's write-back keys on the first alone, so its refusal never tells someone with a permission problem to "remove or replace the non-regular file".hashFile, whose sentinels are opaque tokens compared only for equality, maps both alike — which is what preserves exact parity withf6aa686for every input class.An absent path is not refused:
os.ReadFileruns and itsENOENTreaches callers unchanged.Known divergence, deliberately unchanged
hashFileLstats and refuses a symlink outright;readDestBytesdoes not, so a read that reachesos.ReadFilefollows the link.statustherefore calls a symlinked destination drifted whilediffreads through it and compares the target.The reads this gate replaced followed links too, so changing it would change what
diffandreconcilehave always reported — a behaviour decision that belongs with the drift-walk unification in #229, where all four walks can change together. #229 also carries the consequence worth knowing: under the documentedAGENTSYNC_ALLOW_SYMLINK_DEST=1, whereapplywrites through the link,statusreports drift noapplycan clear.Type of change
Test plan
Full
-race -count=1suite green;gofmt -s,go mod tidy(no diff),golangci-lint(pinnedGOTOOLCHAIN=go1.26.2) → 0 issues.-race -count=2and-shufflegreen with no ordering dependence.Every test here is timeout-bounded, and that is the design rather than caution: this defect class does not fail, it hangs.
runCLIexecutes in-process, so an unguarded read would wedge the whole test binary until the package timeout killed it with a stack dump and no useful diagnostic.TestReadDestBytesShape/…ReportsAStatFailureAsItself— the gate: FIFO and directory refused; a symlink loop reports as itself and carries no path; absent still yieldsENOENT; an ordinary file still read.TestHashFileSentinels— all three sentinels plus both halves of the symlink split, and the parity row for an unstattable destination.TestKeyMergeAndWriteBackReadsAreGuarded/…MessageMatchesTheFailure— the two callers not reachable end-to-end, and that each remedy matches its failure.TestCommandsDoNotHangOnNonRegularDestination— end-to-end against a real applied home, both destination shapes.TestRunBoundedDetectsACommandThatNeverRan— the positive control for the anti-vacuity check.TestRestoreDestReplacesAFIFOEvenWithAReaderAttached— the fixture restore, with a reader attached.TestEveryDestinationReadGoesThroughTheGate— the invariant, with a synthetic negative control run every time in both directions.What review cost, and what it bought
Seven review rounds on a ~40-line production change. Almost every finding was in prose or test scaffolding rather than in the shipped code, and the recurring defect was a claim broader than its measurement. Some worth recording, because the tests that exist now exist because of them:
importrows were vacuous:importiscobra.ExactArgs(1)and the rows passed no selector, so cobra rejected them beforeRunE— they executed zero lines of the code they named and passed identically with the fix reverted. That broken measurement was the sole evidence for an earlier claim in this PR.runBoundednow observes whether the command body ran, by wrapping the resolvedRunE, rather than inferring it from cobra's error text — the third implementation of that check; the first two could not fire at all.TestRestoreDest…was itself racy, and its two outcomes were "hang" and "pass vacuously" — reproducing, inside the test written to prevent it, the failure mode this PR exists to fix. Its reader now attaches withO_NONBLOCK.chmodassertion was vacuous at CI's umask; the fixture captures0o666so only thechmodcan produce it.ForeignCollision→New, andNewisSafeForAutoApply. That is why the gate has two sentinels. Parity was then validated empirically: a verbatim copy off6aa686'shashFilecompared against head across 13 input classes, run as root and as an unprivileged uid so theEACCESrows genuinely denied. All 13 identical.A final mutation sweep found no shipped behaviour whose deletion breaks no test.
just test-releaseis green (the release bar) — not run locally:justis not installable in this environment (the proxy blocksjust.systems), so the recipes were run directly. CI is the check here.just lintis clean — run as its underlying recipe with theGOTOOLCHAIN=go1.26.2pin CLAUDE.md documents.Checklist
docs/components.md,CHANGELOG.md, and theisRegularOrAbsentdoc ininternal/render/writer.gowhoseapply --dry-runclaim was false.Related
agentsync.toml's own read path.🤖 Generated with Claude Code
https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG