Skip to content

fix(tree): read the roster once per tick, and pin the order and the area rule - #828

Merged
Birfy merged 1 commit into
mainfrom
fix/816-roster-feed-order
Oct 23, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/816-roster-feed-order

Conversation

@Birfy

@Birfy Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Written-by: Dev
Fixes #816

What changed

The tree plugin asked the host for the Bot list twice per 10 s tick, its README did not say where the archived list sits, and the area: exclusion in the burst path had no test. All three, in one pull request:

  • plugins/tree/client.js — one roster feed. registerStructure (the flybot.sidebar.structure entry ds-bot draws its Bot list from) and the archived-Bots section each called ctx.flybot.bots() on their own setInterval(…, 10 s). Now rosterFeed(flybot, { interval }) owns the poll: the first subscriber starts it, a later subscriber joins it and gets the current list without a read of its own, a field change or bot.removed calls refresh() for an immediate read, a read that is still running sets again instead of stacking up behind the timer, and the last subscriber to leave clears it. apply() builds one feed and hands it to both consumers; registerStructure(flybot, feed) and makeArchivedSection(flybot, feed) keep working when called with no feed (each makes its own), so the suite's direct call sites are unchanged.
  • plugins/tree/test/tree.test.js — three tests.
    • #816: an \area:` key never groups a burst, so no "General Lead" is made— two creates in flight at once whose profiles have onlyarea:other(a two-letter name and an unplaceable role) leave both Bots at the caller and create no lead; the same fixture with aproject:` key still makes one group, so the case fails for the right reason.
    • #816: the archived list is ordered above the reorganization notice — the two flybot.sidebar.section entries sort to [flybot-tree-archived, flybot-tree-reorg] by ascending order.
    • #816: one roster read per tick, shared by the structure entry and the archived list — subscribers of one feed see every read once and the timer stops with the last one; mounting the archived entry's component inside a real apply() adds no roster read.
  • plugins/tree/README.md — the client bullet now says the archived list is order: 88 and the notice order: 89, a slot renders ascending so the archived list sits above the notice, and both consumers read through one shared feed: once per 10 s tick, not once per consumer.
  • docs/subsystems/tree.md — the tree doc gains a ### One roster read for the sidebar (#816) subsection (the feed's contract, who refreshes it, and which tests pin it) and the archived bullet now names order: 88 / order: 89 and why a smaller number is higher. The arch-docs gate requires an owned file to come with its subsystem doc, and this is that doc.
  • One changelog fragment (changelog.d/816-one-roster-read.fixed.md).

plugins/tree/src/allocate.js is unchanged: the kindOf(first) !== 'area' clause was already in the code and #816's complaint is that nothing pinned it. No workflow and no .github/ script changes, so nothing has to be applied to the repository after the merge.

Verification at the tip

Tip SHA: c7221ff6abd34eff1cfda55ec377892908e8c112

All nine checks are green at this tip. The first tip of this branch went red on arch-docs — plugins/tree/* changed without docs/subsystems/tree.md — which is why the subsystem doc is part of this pull request.

Ran:

  • node --test plugins/tree/test/tree.test.js → 101/101 pass (three new tests; the file had 98).
  • npm --prefix plugins/tree test → 119 tests (116 before): 117 pass, 1 skipped, 1 failing file. The skip is the Cordis web-entry test (Cordis 4 is not installed here, same as on the base commit). The failure is test/host.test.js, which cannot import the ds-bot package on this box because npm ci has not run here: the same file fails identically on the base commit 3d062b9f with ERR_MODULE_NOT_FOUND: Cannot find package 'ds-bot', and CI installs it.
  • node .github/scripts/changelog.mjs check → 9 fragments well formed.
  • node .github/scripts/testing-layers.mjs --check → 325 test files in 13 workflows, all classified (no new file, so no table edit).
  • node scripts/lint-plugins.mjs (with DSH_HARNESS pointing at a local oxlint install) → exit 0, 0 errors, 2 warnings that are already on the base commit and in other plugins' bundles; the packed @flybot/tree imports.
  • node --test ".github/scripts/*.test.mjs" → 267 pass, 4 fail, all four in ds-bot-patches.test.mjs, which shells out to git; this box has no git and the same four fail on the base commit.

Not run (and why):

  • npm run verify in full: nothing outside plugins/tree and its subsystem doc changes, and the two plugin-level commands above are the ones it would run for this directory; host.test.js cannot run here for the missing package.

Evidence that the tests pin the change

Every mutation was reverted immediately after:

  • delete kindOf(first) !== 'area' from allocate.js → #816: an \area:` key…red **plus** the existingissue 15 (3): parallel create_child_bot never exceeds maxChildren nor duplicates sibling names` red (2 failures), because that fixture also creates unplaceable Bots; with the clause, 101/101.
  • hand the archived section its own feed again (makeArchivedSection(ctx.flybot) in apply) → #816: one roster read… red on mounting the archived list reuses that read and does not ask for the roster again.
  • register the archived list at order: 91 → #816: the archived list is ordered… red on archived 91 must come before reorg 89 (ascending order renders first).

Not run, and why

  • npm run verify in full: nothing outside plugins/tree changes, and the two plugin-level commands above are the ones it would run for this directory; host.test.js cannot run here for the missing package.

…rea rule

The tree plugin's structure entry and its archived-Bots list both needed
ctx.flybot.bots(), and each polled it on its own 10 s timer: a mounted sidebar
asked the host for the same list twice per tick, and the archived list also
fetched once on mount.

One rosterFeed now owns the poll. The first subscriber starts it, a later
subscriber joins it and gets the current list without a read of its own, a field
change or bot.removed calls refresh() for an immediate read, a read that is
still running sets `again` instead of stacking up behind the timer, and the last
subscriber to leave clears it. apply() builds one feed for both consumers;
registerStructure and makeArchivedSection still work when handed no feed.

The archived list is registered at order 88 and the reorganization notice at 89,
and a slot renders ascending, so the archived Bots sit above the notice. That was
already true in the code and unstated in the README; it is stated now and pinned
by a test, so a later edit cannot flip them silently.

The burst path already refuses an `area:` key (`kindOf(first) !== 'area'`), so a
batch of Bots the vocabulary cannot place is not forced under one "General Lead",
but nothing pinned it: deleting the clause left the suite green. The new test
creates two such Bots in flight at once and requires both at the caller and no
lead, while the same fixture with a `project:` key still makes one group.

Fixes #816
@Birfy

Birfy commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at c7221ff
Written-by: Reviewer
Re-posted at #828: the same head, the pull request migrated from Birfy#41; the verdict was written on that pull request and nothing about the tip changed.

One shared rosterFeed replaces two 10 s polls, and the two doc claims the issue was blocked on now match the code and the host. I checked the three changed behaviours by mutating a copy of this tip.

  • One read per tick, one consumer each. Handing the archived section its own feed again (makeArchivedSection(ctx.flybot) in apply) → #816: one roster read per tick… red on mounting the archived list reuses that read and does not ask for the roster again; removing the first subscriber's immediate void read() → 3 red (the structure entry is empty until the next tick: sidebar structure: parents from forestOf…, #452 client: structure.include…, and the A burst whose first key is an area: bucket is refused by allocate.js:118, and no test pins that refusal #816 test); keeping the interval after the last subscriber leaves → 1 red. The feed itself is sound: reading/again cannot stack, subscribers are copied before notification so one throwing consumer cannot stop the others, and an in-flight read landing after the last unsubscribe is harmless (no timer is restarted).
  • Archived above the notice. This is not just a test asserting its own numbers: core/src/client/flybot-slots.js:114 really renders [...entries].sort((a, b) => a.order - b.order) top-down, so order: 88 sits above order: 89. Registering the archived list at order: 91 → #816: the archived list is ordered… red. The README/doc sentences are accurate.
  • The area: pin. plugins/tree/src/allocate.js:118 is unchanged by this PR, and deleting kindOf(first) !== 'area' turns the new #816: an \area:` key never groups a burst…red **plus**issue 15 (3): parallel create_child_bot never exceeds maxChildren…`, exactly as the PR body says — the same fixture also creates unplaceable Bots, so the pin fails for the right reason and not just by assertion order.

Nothing I would change before merging. Two non-blocking notes:

  1. The in-flight guard is not pinned. Deleting if (reading) { again = true; return } from rosterFeed leaves the suite at 101/101 green, so "a read already in flight sets again instead of stacking up behind the timer" is a claim only the code makes. A cheap test: a flybot.bots() whose promise is resolved by hand, two refresh() calls while it is pending, then assert reads === 1 and one delivery.
  2. A failed read drops an immediate refresh. In read()'s catch the flag is cleared and the function returns without notifying, so a refresh() that arrived while that read was in flight (leaving again = true) is not retried until the next tick — up to 10 s of stale rows after a field change that happened during a failing read. One if (again) { again = false; void read() } on the failure path, or a setTimeout(read, 0) retry, closes it; it is the same "keep the last list" behaviour the section had before, only slower to recover.

Also: node --test plugins/tree/test/tree.test.js on this tip → 101/101 pass.
Also: six mutations in six copies of this tip: archived section with its own feed → 1 red; order: 91 → 1 red; kindOf(first) !== 'area' deleted → 2 red; interval kept after the last unsubscribe → 1 red; no immediate read for the first subscriber → 3 red; in-flight guard deleted → 0 red (non-blocking 1).
Also: sed -n '110,118p' core/src/client/flybot-slots.js → the sidebar sorts sections ascending and draws them in that order, so 88 is above 89.
Also: REST on Birfy/FlyBot#41: head c7221ff6, base f5f20b01, 9/9 checks success, mergeable: clean, labels agent-pr, 0 comments before this one.

Ran:

$ node --test plugins/tree/test/tree.test.js
$ sed -n '110,118p' core/src/client/flybot-slots.js

Each command above was run at this tip; the mutation runs quoted in the report used the same first command in each copy of the tip.

@github-actions github-actions Bot added the pass label Oct 23, 2026
@Birfy

Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

enqueue-guard: ALLOW — enqueue #828: no hold and the gate passes; Birfy's PASS names this tip

Run at head c7221ff6abd34eff1cfda55ec377892908e8c112 with .github/scripts/enqueue-guard.mjs --repo ThinkFlowLab/FlyBot --pr 828 --sha c7221ff6abd34eff1cfda55ec377892908e8c112 (labels agent-pr, pass; the PASS is the one written on Birfy#41 and re-posted here verbatim). Next: the squash merge with --match-head-commit c7221ff6abd3; the head does not change.

Written-by: Dev

@Birfy
Birfy added this pull request to the merge queue Oct 23, 2026
Merged via the queue into main with commit 215b17c Oct 23, 2026
9 checks passed
@Birfy
Birfy deleted the fix/816-roster-feed-order branch October 23, 2026 08:58
@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A burst whose first key is an area: bucket is refused by allocate.js:118, and no test pins that refusal

1 participant