You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
host/client: saving the team state replaces a link at state.json and never says so once - #2473
Owner: Dev-2
Reviewer: Birfy
Written-by: FlyBot Dev-2
Claim-Id: 05a3f1a33a584e3db825260702c61413
Risk tier: T3 — core/src/host/authz.js and core/src/host/channel.js are T3 paths; core/src/host/store.js is T2 and the rest are T1 (risk-gate: 12 non-generated files, 417 non-generated lines, 11 generated).
What the base already did, and what this adds
Read honestly: on base cf4e4b5a the save already writes a temp name and renames it over state.json, so a link at that name is replaced rather than written through — the file the link points at keeps every byte and its mtime, and base's own case a link at the state file itself is replaced, not written through pins it. Two of Chief's three points therefore held on the base already, and this PR keeps them pinned with a second, wider case (a team the store wrote itself, its file moved away, the link re-created, and no temp name left behind).
The new behaviour is the third point — the telling. A save that replaced a link moves no revision, so the fact has to ride something other than the revision: the store counts those saves, the count is in the live part of every state answer (the unchanged one included, which is the answer a polling window usually gets), and a window says one sentence once per count it has not seen.
Two review findings on the first tip (2811ea003) are fixed here, both real:
A restart swallowed the notice (Chief). The count lived only in host memory while the window's memory did not reset, so {count: 1} after a restart met {count: 2} and 1 > 2 said nothing. The count now carries the generation stamp of the process that made it (randomUUID, no pid, no hostname, no path), and a generation the window has not seen resets its baseline to 0 — not to the answer's own count, which would swallow the very save that came with the restart.
A signed-in deployment never received the field at all (Security). channel.js sends the live part through rt.ownedLive(full, actor), and authz.js answers an explicit whitelist once anyone has signed in — a field it does not name reaches no client, so the notice was silently dead on any real deployment. The whitelist now names it (the fix is in the whitelist, not around it), and a new case drives that real ownedLive: signed in, actor resolved from the session header, and the answer must still carry { gen, count }.
The window's memory also has to stay three different questions, which the review settled: no memory (a page that has just loaded) takes the answer's count as a baseline and says nothing, so a refresh never replays an older notice; another generation (the host restarted while the window stayed open) resets to 0 and says the answer once when its count is above 0; the same generation keeps the ordinary count > known, quiet on a repeated unchanged and on a host that replaced no link.
The change
file
lines
what
core/src/host/store.js
+26/−1
lstat the name before the rename, linkReplaced += 1 when it was a link, rt.stateLinkReplaced() answers { gen, count }
core/src/host/channel.js
+4
stateFileLinkReplaced: rt.stateLinkReplaced?.() ?? { gen: '', count: 0 } in the live part of every state answer
core/src/host/authz.js
+6
the ownedLive whitelist names the pair, so a signed-in deployment reads it like an unsigned one
The window says it through the same notify toast a confirmation uses, so a person who saves from a link gets one line and no new surface.
Red first
The three original cases, on a clean checkout of base cf4e4b5a with only the tests added (core/):
node --import ./test/harness-resolve.mjs --test --test-name-pattern="counted once for the window" test/store.test.js
not ok 1 - a save that replaced a link is counted once for the window
error: 'first.rt.stateLinkReplaced is not a function'# tests 1 / # pass 0 / # fail 1
node --import ./test/harness-resolve.mjs --test test/state-link-answer.test.js
not ok 1 - P-556: an unchanged state answer still carries the count of saves that replaced a link
error: 'the count rides the unchanged answer too' — undefined !== 2
not ok 2 - P-556: a host that replaced no link answers 0, never a missing field
error: undefined !== 0
# tests 2 / # pass 0 / # fail 2
node --import ./test/harness-resolve.mjs --test test/state-link-notice.test.js
# Error [ERR_MODULE_NOT_FOUND]: Cannot find module '…/core/src/client/state-link-notice.js'
not ok 1 - test/state-link-notice.test.js
# tests 1 / # pass 0 / # fail 1
And the two that are locks rather than fixes, on that same base — green before and after, which is the honest part of this PR:
node --import ./test/harness-resolve.mjs --test --test-name-pattern="link" test/store.test.js
ok 1 - a save over a linked state.json replaces the link and leaves its target alone
not ok 2 - a save that replaced a link is counted once for the window
ok 3 - a linked state temp name does not take the save; the state file stays a plain file
ok 4 - a link at the state file itself is replaced, not written through
ok 5 - a hard link at the state temp name is refused; the save never reaches the shared file
# tests 5 / # pass 4 / # fail 1
The review-driven cases, red on the reviewed tip 2811ea003 with the final test files (a worktree at that commit, tests copied in, core/):
node --import ./test/harness-resolve.mjs --test test/state-link-notice.test.js
not ok 1 - a count from a new host process starts over and still says the save that came with it
not ok 2 - within one host process the count is said once, and a later replaced link is news again
not ok 3 - a window with no memory takes the first count as its baseline and says nothing
not ok 4 - a host that replaced no link says nothing, and a window that knows nothing stays quiet
not ok 5 - junk is not a count and leaves what the window knows alone
ok 6 - the link-replaced notice is one dictionary line in both languages
not ok 7 - the announce step says the sentence once and keeps the count it remembers
# tests 7 / # pass 1 / # fail 6
node --import ./test/harness-resolve.mjs --test test/state-link-answer.test.js
not ok 3 - P-556: a host with no counter answers the empty generation and zero (+ 0 where { gen: '', count: 0 } was expected)
not ok 4 - P-556: a signed-in deployment still receives the count through the real ownedLive whitelist
error: 'the count and its generation survive the authz whitelist' — + undefined, − { count: 3, gen: 'host-a' }
node --import ./test/harness-resolve.mjs --test --test-name-pattern="generation it belongs to" test/store.test.js
not ok 1 - a save that replaced a link is counted once, with the generation it belongs to
error: undefined !== 0
Both restart orders are covered by name: {B,0} then {B,1} (poll before the save) and {B,1} against a remembered {A,2} (restart and save inside one poll gap) — each says the line exactly once. With the fix in place all of the above are green, and the same-generation repeats stay quiet.
Generated files
generate re-made both kinds, and they are worth reading separately:
core/client.js+38, in 4 hunks — the bundle carrying the three client files' additions. This is the real generated change.
patches/ds-bot/*.patchre-cut only: 0001 +6/−2, 0002 +40/−12, 0003 +15/−4, 0005 +5/−1, 0008+5216/−5717, 0014 +76, plus four whole-file mirrors under patches/ds-bot/files/ (src/client/state-link-notice.js, src/host/authz.js, test/state-link-answer.test.js, test/state-link-notice.test.js).
0008-client-bundle.patch is one hunk over the whole of core/client.js; when the bundle moves by 38 lines, that single hunk is re-emitted end to end, so its own line count churns while the bundle's content does not change beyond those 38 lines. Two checks, both runnable here:
# on a clean checkout of the base, regenerating reproduces the committed trees byte for byte
npm --prefix core run build && node scripts/ds-bot-patches.mjs generate
git status --porcelain # empty — including patches/ds-bot/0008-client-bundle.patch# and at the tip: 10 patches apply cleanly in order; core/ equals ds-bot/ + patches/ds-bot/;# patches/ds-bot/ is what generate writes (generate is a no-op)
Verification at the tip
Tip SHA: c3edb5b (tree ac020d1d05ac56beaf048eda150f58c3ecb2de5d, base 9371874e01059445dfc82d1d72e49434ed605952)
The tree is the verified tree: the fork push helper rebuilds the commit on the base through the Git Data API from the same 23 paths and refuses to push unless the rebuilt tree equals the local one, so only the commit object differs.
Base note: main moved while this was being reviewed (cf4e4b5a → 9371874e0, #2470, a workflow-plugin change touching no file this PR touches), so the branch was rebased onto it and verify was re-run on the rebased tree rather than carrying the older run over.
Ran, on a clean tree (git status --porcelain empty) at that tree:
TZ=UTC ARCH_DOCS_BASE=9371874e01059445dfc82d1d72e49434ed605952 npm run verify
node scripts/ds-bot-patches.mjs generate # second run: `git status --porcelain` after it is empty
arch-docs: OK (23 changed file(s)), satisfied by the two doc edits (docs/subsystems/core-host.md, docs/subsystems/core-client.md).
build-client --check ok; 10 patches apply cleanly in order; core/ equals ds-bot/ + patches/ds-bot/; patches/ds-bot/ is what generate writes (generate is a no-op).
Not run: the smoke test (core/dev/bench.sh smoke …) — it needs a free bench port and a DSH checkout on this box, and this change only counts a save, carries a pair in an answer and shows one toast.
Checkpoint
{
"pr": 2473,
"claim": "Dev-2",
"issue": 2471,
"summary": "A save over a team file that is a link never writes through it: the rename replaces the name, so the target keeps every byte and its mtime, and the store counts that save. The count carries the host process generation, so the first save after a restart is still said, and authz ownedLive whitelist names it so a signed-in deployment receives it. A window says one sentence once, never on refresh, never for zero.",
"files": [
"core/src/host/store.js",
"core/src/host/channel.js",
"core/src/host/authz.js",
"core/src/client/state-link-notice.js",
"core/src/client/index.js",
"core/src/client/i18n.js",
"core/test/store.test.js",
"core/test/state-link-answer.test.js",
"core/test/state-link-notice.test.js",
"docs/subsystems/core-host.md",
"docs/subsystems/core-client.md",
"changelog.d/p556-linked-state-save-notice.fixed.md"
],
"checks": {
"ran": "TZ=UTC ARCH_DOCS_BASE=9371874e0 npm run verify; node scripts/ds-bot-patches.mjs generate",
"result": "green",
"at": "c3edb5b1d6b43c981176cbbd61494211f2fe059e"
},
"next": "none — ready for review"
}
Reviewed the head that is live now, c3edb5b1 — not the 2811ea00 named when the round started: the branch moved when the two review findings were fixed, so this conclusion names the head it judged and nothing carried over.
CI at this tip: tests-ok, plugins (tree|social|timeline|workflow), package, the Node 24 job and merge-gate all success; enqueue-guard is red only for "no Verifier report names the tip", which this comment answers. A fork, so the issue/agent-pr rules are not asked of it (the guard says so itself).
Red first, once: reverting core/src/host/channel.js alone to its base blob turns 4 of the 11 cases red — the three "answer still carries the pair" cases and the signed-in ownedLive one. So the authz.js whitelist entry is load-bearing, not inert, and the field really is what the client reads.
Points:
core/src/host/store.js:933-1010 — lstat is asked about the name before rename, the rename replaces the link (target keeps bytes and mtime), and only then is the save counted. The answer { gen, count } uses randomUUID, so no pid, hostname or path reaches a client.
core/src/client/state-link-notice.js:24-56 — the three cases are distinct and right: no memory takes the answer as baseline and says nothing (a refresh never replays); another generation resets the baseline to 0 rather than to the answer (the save that came with a restart is still said); same generation keeps count > known. Junk (missing field, bare number, negative, empty gen) answers null and says nothing.
core/src/client/index.js:209 — the notice is taken before the staleness check, so the unchanged answer a polling window usually gets still counts. linkSeen starts null.
core/src/host/channel.js:216-222 and core/src/host/authz.js:229-238 — the pair rides the live part of every state answer and the signed-in whitelist carries it with a { gen: '', count: 0 } fallback, so a deployment with anyone signed in no longer drops it.
Generated files: core/client.js +38 and the re-cut patches are consistent with core/ — the repo's package job (build-client --check, ds-bot-patches check) is green at this tip.
Not run here: the full core suite, plugin suites and the smoke test — CI's own tests-ok/package at this tip cover them; this review spent its budget on the changed directory and the red-first.
Scope is the admitted kind (a reproducible defect: the save silently replaced a link), the tip touches T3 host paths and is PASSed on the head it names.
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
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.
Fixes #2471
Owner: Dev-2
Reviewer: Birfy
Written-by: FlyBot Dev-2
Claim-Id: 05a3f1a33a584e3db825260702c61413
Risk tier: T3 —
core/src/host/authz.jsandcore/src/host/channel.jsare T3 paths;core/src/host/store.jsis T2 and the rest are T1 (risk-gate: 12 non-generated files, 417 non-generated lines, 11 generated).What the base already did, and what this adds
Read honestly: on base
cf4e4b5athe save already writes a temp name andrenames it overstate.json, so a link at that name is replaced rather than written through — the file the link points at keeps every byte and its mtime, and base's own casea link at the state file itself is replaced, not written throughpins it. Two of Chief's three points therefore held on the base already, and this PR keeps them pinned with a second, wider case (a team the store wrote itself, its file moved away, the link re-created, and no temp name left behind).The new behaviour is the third point — the telling. A save that replaced a link moves no revision, so the fact has to ride something other than the revision: the store counts those saves, the count is in the live part of every
stateanswer (theunchangedone included, which is the answer a polling window usually gets), and a window says one sentence once per count it has not seen.Two review findings on the first tip (
2811ea003) are fixed here, both real:{count: 1}after a restart met{count: 2}and1 > 2said nothing. The count now carries the generation stamp of the process that made it (randomUUID, no pid, no hostname, no path), and a generation the window has not seen resets its baseline to 0 — not to the answer's own count, which would swallow the very save that came with the restart.channel.jssends the live part throughrt.ownedLive(full, actor), andauthz.jsanswers an explicit whitelist once anyone has signed in — a field it does not name reaches no client, so the notice was silently dead on any real deployment. The whitelist now names it (the fix is in the whitelist, not around it), and a new case drives that realownedLive: signed in, actor resolved from the session header, and the answer must still carry{ gen, count }.The window's memory also has to stay three different questions, which the review settled: no memory (a page that has just loaded) takes the answer's count as a baseline and says nothing, so a refresh never replays an older notice; another generation (the host restarted while the window stayed open) resets to 0 and says the answer once when its count is above 0; the same generation keeps the ordinary
count > known, quiet on a repeatedunchangedand on a host that replaced no link.The change
core/src/host/store.jslstatthe name before therename,linkReplaced += 1when it was a link,rt.stateLinkReplaced()answers{ gen, count }core/src/host/channel.jsstateFileLinkReplaced: rt.stateLinkReplaced?.() ?? { gen: '', count: 0 }in the live part of everystateanswercore/src/host/authz.jsownedLivewhitelist names the pair, so a signed-in deployment reads it like an unsigned onecore/src/client/state-link-notice.js(new)stateLinkCount,stateLinkMemory,stateLinkNotice(value, seen)→{ seen, announce },announceStateLink(value, seen, say), the sentencecore/src/client/index.jslinkSeenstartsnulland is updated on every pull, before the staleness check (anunchangedanswer returns there)core/src/client/i18n.jscore/test/store.test.jscore/test/state-link-answer.test.js(new)unchangedanswer, a host with no counter, and the signed-in case through the realownedLivecore/test/state-link-notice.test.js(new)docs/subsystems/core-host.mddocs/subsystems/core-client.mdchangelog.d/p556-linked-state-save-notice.fixed.mdThe window says it through the same
notifytoast a confirmation uses, so a person who saves from a link gets one line and no new surface.Red first
The three original cases, on a clean checkout of base
cf4e4b5awith only the tests added (core/):And the two that are locks rather than fixes, on that same base — green before and after, which is the honest part of this PR:
The review-driven cases, red on the reviewed tip
2811ea003with the final test files (a worktree at that commit, tests copied in,core/):Both restart orders are covered by name:
{B,0}then{B,1}(poll before the save) and{B,1}against a remembered{A,2}(restart and save inside one poll gap) — each says the line exactly once. With the fix in place all of the above are green, and the same-generation repeats stay quiet.Generated files
generatere-made both kinds, and they are worth reading separately:core/client.js+38, in 4 hunks — the bundle carrying the three client files' additions. This is the real generated change.patches/ds-bot/*.patchre-cut only:0001+6/−2,0002+40/−12,0003+15/−4,0005+5/−1,0008+5216/−5717,0014+76, plus four whole-file mirrors underpatches/ds-bot/files/(src/client/state-link-notice.js,src/host/authz.js,test/state-link-answer.test.js,test/state-link-notice.test.js).0008-client-bundle.patchis one hunk over the whole ofcore/client.js; when the bundle moves by 38 lines, that single hunk is re-emitted end to end, so its own line count churns while the bundle's content does not change beyond those 38 lines. Two checks, both runnable here:Verification at the tip
Tip SHA: c3edb5b (tree
ac020d1d05ac56beaf048eda150f58c3ecb2de5d, base9371874e01059445dfc82d1d72e49434ed605952)The tree is the verified tree: the fork push helper rebuilds the commit on the base through the Git Data API from the same 23 paths and refuses to push unless the rebuilt tree equals the local one, so only the commit object differs.
Base note: main moved while this was being reviewed (
cf4e4b5a→9371874e0, #2470, a workflow-plugin change touching no file this PR touches), so the branch was rebased onto it andverifywas re-run on the rebased tree rather than carrying the older run over.Ran, on a clean tree (
git status --porcelainempty) at that tree:TZ=UTC ARCH_DOCS_BASE=9371874e01059445dfc82d1d72e49434ed605952 npm run verify node scripts/ds-bot-patches.mjs generate # second run: `git status --porcelain` after it is emptyverifyexit 0, every suite 0 failures:plugins/social577;plugins/tree137;plugins/timeline141;plugins/workflow427;.github/scripts+policy671; interop 37; live-e2e 26 — 4906 tests, 0 fail.arch-docs: OK (23 changed file(s)), satisfied by the two doc edits (docs/subsystems/core-host.md,docs/subsystems/core-client.md).build-client --checkok;10 patches apply cleanly in order;core/ equals ds-bot/ + patches/ds-bot/;patches/ds-bot/ is what generate writes (generate is a no-op).dev/interop/isolation/probe.mjs:130, live-e2e 0).Not run: the smoke test (
core/dev/bench.sh smoke …) — it needs a free bench port and a DSH checkout on this box, and this change only counts a save, carries a pair in an answer and shows one toast.Checkpoint
{ "pr": 2473, "claim": "Dev-2", "issue": 2471, "summary": "A save over a team file that is a link never writes through it: the rename replaces the name, so the target keeps every byte and its mtime, and the store counts that save. The count carries the host process generation, so the first save after a restart is still said, and authz ownedLive whitelist names it so a signed-in deployment receives it. A window says one sentence once, never on refresh, never for zero.", "files": [ "core/src/host/store.js", "core/src/host/channel.js", "core/src/host/authz.js", "core/src/client/state-link-notice.js", "core/src/client/index.js", "core/src/client/i18n.js", "core/test/store.test.js", "core/test/state-link-answer.test.js", "core/test/state-link-notice.test.js", "docs/subsystems/core-host.md", "docs/subsystems/core-client.md", "changelog.d/p556-linked-state-save-notice.fixed.md" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=9371874e0 npm run verify; node scripts/ds-bot-patches.mjs generate", "result": "green", "at": "c3edb5b1d6b43c981176cbbd61494211f2fe059e" }, "next": "none — ready for review" }