fix(sync): say which condition failed, and which field an exit came from - #793
Merged
fujibee merged 3 commits intoAug 14, 2026
Merged
Conversation
Two reports, one file, one shape: a message that states something the run never established. Kept as one commit because both live in `remote-sync.mjs` and cannot be staged apart. == #781, the condition that was never checked == A reader on Windows was told a file was not private, went looking for permissions, and there were none to look at: the mode test is guarded by `process.platform !== "win32"`, so on that platform the only conditions left were a missing file, a symlink, or an oversized one. The same message on Linux had been a real `0664` left by `umask 0002`, which is what made the wrong reading credible. Five throws said it, in three files, and the report named two. The set was derived rather than listed -- every `process.platform !== "win32"` under `scripts/`: remote-sync.mjs the bounded authority read, the retained checkpoint, and AGMSG_SYNC_TRUST_DIR rename-sync-config.mjs the config it is about to rename sync-cipher.mjs the age identity Two sites in the same sweep are NOT changed, and the reason is the same rule: the post-open re-check says "changed while it was being opened", which is true on every platform, and the identity check at the end of remote-sync.mjs is guarded in full, so on win32 it throws nothing rather than something wrong. The fix is an ordering, not a wording. `authorityFileFault` returns the condition that actually failed, and the mode test is the LAST thing it consults -- so no message above it can be about permissions, on any platform. Callers name their own subject and add the path, which the old messages omitted: they named a property without naming what had it, on machines that hold several teams. `readNativeAgeIdentity` needed more than a reworded throw. Its own `catch` replaced every failure with "age identity is not securely readable", so the specific reason never reached anyone -- a missing file and an unparseable one both arrived as a claim about security. The reasons are raised as `CipherStateError` now, which that `catch` passes through. The test that covered the first site accepted one sentence for all three of its cases. It is replaced by one that requires each case to name its own condition, including the negative: a symlink must not be reported as a permission problem. The unit test around `authorityFileFault` pins the ordering directly, because the integration case cannot -- a symlink's mode on this platform does not trip the mask being tested, so it would stay green with the order reversed. == #782, the field the number came from == storage sync prepare failed (3840) Three things missing from one line. Which field produced 3840, which team it was about, and where to look. `signal ?? code` prints one number and hides which of the two it was. On Windows/Git Bash a driver that ends to a signal can arrive through `code` as a raw wait status, so the operator gets a number that reads like an exit status and is not one -- with nothing on the line to tell them apart. Both fields are named now, and a `code` outside the 0-255 an exit status can take is additionally shown DECOMPOSED. Not decoded: under the POSIX encoding 3840 is `WIFEXITED` with status 15, while the report that raised it read it as signal 15 -- and a death by signal puts the number in the LOW bits, with a `bash` in the middle reporting 143 rather than passing a wait status along at all. Nobody reproducing this has the platform to settle which layer produced the number, so both components are printed and neither is asserted. Choosing one would put a second wrong sentence exactly where the first one was. The fallback diagnostic said "inspect its team storage and binding" and named neither, on a machine that may hold several teams. `runDriver` is handed the team and the binding path rather than reading them back out of `args`, where they sit behind a positional index -- the report's own description miscounted it, placing the team at `args[1]` where the operation is. The path is resolved defensively: `teamConfigPath` throws without a connection root, and a caller that has none still has to run. A value that exists for a sentence in a failure message must not become a new way to fail -- which it briefly was here, breaking three tests that had nothing to do with the change. Refs #781 Refs #782
…y catch The previous commit's message and the comment it added both said the old `readNativeAgeIdentity` collapsed an UNPARSEABLE identity into "age identity is not securely readable". That is false, and review caught it. The parser fails through `malformed()`, which raises a `CipherStateError`, and that catch has always begun `if (error instanceof CipherStateError) throw error`. So a malformed identity kept its own reason on the base and keeps it now. The risk this change carried was BREAKING that pass-through, not restoring it -- the opposite of what the message claimed -- and a test now pins it. What the catch actually collapsed is a plain `Error`: the stat and read failures. A missing file, a directory, an unreadable one, and a mode win32 never checks all arrived as a claim about security. The first version of this fix named three of those and left the rest -- EACCES and EPERM still reached the generic sentence, so the comment's "every failure" was wider than the code. Every stat and read failure names itself now, with its code and the path, so the sentence and the behaviour say the same thing. AND THE TWO FILES OUTSIDE THE REPORT ARE NOW BOUND AT THEIR ENTRY POINTS. Reverting either `sync-cipher.mjs` or `rename-sync-config.mjs` whole left every test green, so "five sites fixed" was a claim the suite did not carry. The identity reader is now driven for a missing file, a directory, a loose mode and a malformed key; the rename script is RUN, as a script, for a symlink, a directory and a loose mode -- each asserting its own condition, the path, and the absence of the other conditions' words. Reverting either file turns its own case red and leaves the other green; both were measured. Refs #781
`bindingPathIfKnown` caught a failure and returned `undefined`, so a diagnostic
that could not name the binding said "and its binding" and stopped -- with
nothing anywhere saying why. Found while checking this branch against a pattern
being collected elsewhere; the fourth instance of it, and the second to appear
inside work whose subject is messages that say what actually happened.
Swallowing is right here and stays. `teamConfigPath` throws without a
connection root, and a caller that has none still has to run: a value that
exists for a sentence in a failure message must not become a new way to fail.
Being non-fatal was never the defect. Being silent was.
So the reason travels with the fallback -- `{ path }` or `{ unavailable }` --
and the diagnostic says which it got.
Driven at the production entry rather than beside it: the new case calls
`driver()`, which resolves the path itself, once with a connection root (the
message names the file) and once without (the message names the reason). Both
halves assert that the run still reached the driver and still reported its
exit, because continuing is the property that must not change. Reverting the
catch to `undefined` turns the second half red.
Refs #781
Refs #802
fujibee
force-pushed
the
fix/781-782-name-the-condition-that-failed
branch
from
August 14, 2026 18:55
a5b1ce2 to
33f2287
Compare
This was referenced Aug 14, 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
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.
Declared reviewers: 1
Refs #781/Refs #782, notCloses— a closing keyword does not fire offthe default branch, and this lands on
integration/remote. Both issues needclosing by hand afterwards.
Head:
33f2287554a1b9c039d235ea06f16a28c4e235af, rebased ontointegration/remoteat
5d3cc9eba4bbaf22ef94e2d652722ff552c2fe13. Every number below is from thathead.
This body carried a false sentence about the old behaviour, and a correction
is below rather than a quiet edit. See "What the first commit got wrong".
Two reports, one shape: a message that states something the run never
established. One names a permission problem on a platform where permissions
were not consulted; the other prints a number without saying which field it came
from, for a team it does not name.
#781 — the set is five throws, in three files
The report named two. Deriving the set instead — every
process.platform !== "win32"underscripts/— gives nine guards on thebase, of which five throw a message about a condition that guard has already
excluded. The whole set is listed, including what is deliberately untouched,
because a count with no list is the shape that lost the three extra sites in the
first place:
remote-sync.mjsbounded authority readmust be a private / non-writable regular fileremote-sync.mjsretained checkpointmust be a private regular fileremote-sync.mjsAGMSG_SYNC_TRUST_DIRmust be a private directoryrename-sync-config.mjsmust be a private regular filesync-cipher.mjsage identityidentity file is not privateremote-sync.mjspost-open re-checkchanged while it was being openedremote-sync.mjsage identity, end of fileremote-sync.mjsdirectoryfsyncguardrename-sync-config.mjsdirectoryfsyncguardThe fix is an ordering, not a wording.
authorityFileFaultreturns thecondition that actually failed, and the mode test is the last thing it
consults — so no message above it can be about permissions, on any platform.
That is what makes the win32 case safe rather than merely reworded. Callers add
the path, which the old messages omitted: they named a property without naming
what had it, on machines that hold several teams.
readNativeAgeIdentityneeded more than a reworded throw. Itscatchreplacesa plain
Errorwithage identity is not securely readable, so the statand read failures lost their reason. Each of them names itself now — with its
codeand the path — includingEACCESandEPERM, which an earlier versionof this branch left on the generic sentence while claiming to have covered
"every failure".
#782 — decomposed, not decoded
signal ?? codeprinted one number and hid which field produced it.Both fields are named now, and a
codeoutside the 0–255 an exit status cantake is additionally shown decomposed. Not decoded, and the report's
reading is corrected: under the POSIX encoding
3840isWIFEXITEDwithstatus 15, not "terminated by signal 15" — a signal death puts the number in the
low bits, and a
bashin the middle reports143rather than passing await status along at all. Nobody reproducing this has the platform to settle
which layer produced the number, so both components are printed and neither is
asserted.
The fallback said "inspect its team storage and binding" and named neither.
runDriveris handed the team and the binding path rather than reading themback out of
args— where they sit behind a positional index the report itselfmiscounted, placing the team at
args[1]where the operation is. Bothcorrections are recorded on the issues.
What the first commit got wrong
4a804f4's message, and the source comment it added, said the oldreadNativeAgeIdentitycollapsed an unparseable identity into the privacysentence. That is false. The parser fails through
malformed(), which is aCipherStateError, and thatcatchhas always begunif (error instanceof CipherStateError) throw error— so a malformed identitykept its own reason on the base and keeps it now. The risk this change carried
was breaking that pass-through, not restoring it, which is the opposite of
what the message claimed. A test pins it.
The correction is in
e7941d1's message rather than in a rewritten4a804f4.(Both were
2a6a9ecanda5b1ce2before this branch was rebased onto thelanded base; the rebase renamed them, and this body names the SHAs that exist.)
git commit --amendis refused here — "fix it in the next commit; only ask forhelp when the message itself must change" — and rebuilding the branch would be
the same operation wearing a different name. So the wrong sentence stays in the
history with its correction beside it, touching the same lines, where
git log -pandgit blameboth show them together.What was claimed and not carried
The first head asserted "five sites fixed" while
rename-sync-config.mjsandsync-cipher.mjscould be reverted whole with every test still green. Reviewfound that; it was not measured before the request.
Both are bound at their real entry points now, not beside them:
readNativeAgeIdentityis called for a missing file, a directory, a loosemode, and a malformed key — the last pinning what must not change.
rename-sync-config.mjsis run, as a script, for a symlink, a directoryand a loose mode.
Each case asserts its own condition, the path, and the absence of the other
conditions' words.
A fourth silent failure, found here and fixed here
While checking this branch against a pattern being collected elsewhere, the
helper it added for #782 turned out to be an instance of it:
teamConfigPaththrows without a connection root, and a caller that has nonestill has to run — swallowing is right here and stays. What was wrong was
dropping the reason, so a diagnostic that could not name the binding said "and
its binding" and stopped. Being non-fatal was never the defect. Being silent
was.
The fallback carries it now —
{ path }or{ unavailable }— and thediagnostic says which it got. Filed as #802 with the other three instances,
including the line between them: two
|| trueleft in a diagnostic dump arecorrect and are deliberately not changed.
Driven at the production entry, not beside it: the new case calls
driver(), which resolves the path itself — once with a connection root (themessage names the file) and once without (the message names the reason). Both
halves also assert the run still reached the driver and still reported its
exit, because continuing is the property that must not change.
Tests, on this head
tests/remote_sync_engine.test.mjs: 87 pass, 0 fail (local, this head).For bats, CI on this head is the stronger statement — it runs every shard on
three platforms. Every one is green except
bats (ubuntu-latest 4/4)and thebatsaggregate that depends on it:The test covering the first site accepted one sentence for all three of its
cases — it was pinning the defect. It now requires each case to name its own
condition, including the negative: a symlink must not be reported as a
permission problem.
describeChildExitback tosignal ?? codesync-cipher.mjsreverted to baserename-sync-config.mjsreverted to basereturn undefinedThe last two failing one each is itself the check: a control that went red
for both reverts would not be binding the file it names.
One honest limit. The integration symlink case does not discriminate on
this platform: a symlink's mode here is
0o755, which does not trip the0o022mask, so it would stay green with the order reversed. The unit test around
authorityFileFaultis what pins the ordering, with a case built to be both asymlink and group-writable.
Windows itself is not measured — nobody reproducing this has the platform. What
is claimed here is what the code does on the branch
process.platformtakesthere, read from the source and exercised where the guard allows.
check-private-names: clean, 412 files — and it fires, controlled with aseat-shaped name injected into a tracked file (1 finding), removed again.
The one red, and why it is not this branch
not ok 265 watch: relaunch with the SAME instance id replaces the previous watcher (#66 preserved), failing at_wait_pidfile "$pf" "$w2"(
tests/test_watch.bats:442). Confirmed on this exact head, not carried over.#595 was opened
2026-08-01T20:25:42Zand its body names this test andthis assertion; its title is "bats suite flakes across the process-lifecycle
tests:
mainitself fails ~half its runs, a different test each time". Thisbranch's first commit is
2026-08-14T11:18:42Z. The signature is older thanthe branch, and it is recorded against
mainitself.Reachability, re-measured on this head because the head moved:
remote-sync.mjs,rename-sync-config.mjs,sync-cipher.mjs, and the engine testscripts/watch.shandtests/test_watch.batsreference none of themwatch.shdoes reachsqlite-sync.sh—lib/storage.sh→agmsg_storage_load→storage.sh:382→sqlite.sh:510— and that filenames
sync-cipher.mjs. But the assignment sits insidestorage_sync_prepare_push, and the spawn is inside the same function;the failing test never calls it, and this branch changes no line of
sqlite-sync.shThat is "reached, but the failing test does not execute the changed hunk" —
not "no direct reference".