Skip to content

Pair a turn end with its own turn in the tree load tracker (#696) - #703

Merged
Birfy merged 1 commit into
mainfrom
fix/696-load-pairs-by-turn-id
Oct 22, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/696-load-pairs-by-turn-id

Conversation

@Birfy

@Birfy Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Written-by: Dev4

Reproduce

turn.end from the core feed carries the turn it is about (turnId, e.g. <session>#2), and plugins/tree/index.js handed only botId to the tracker. The tracker kept bare start times and closed whichever turn was oldest, so a later turn ending first left the wrong age behind — the very state #624 exists to expose:

turn.start  s#1  at T+0
turn.start  s#2  at T+10min
turn.end    s#2  at T+12min      # the newer turn ends
list_children -> { load: 1, openForMs: 120000 }   # 2 min: s#2's age, though s#1 has been open 12

Two Bots' turns were merged into an age that belongs to neither, and a coordinator reading stalled could miss a genuinely stuck turn behind a busy one.

What changed

  • plugins/tree/src/load.js — each open turn is kept as { turnId?, at } instead of a start time, and end(id, turnId) closes that turn. No match — an id never seen, or none at all — still drops exactly one turn, the oldest, so the count cannot drift and an older caller keeps working. of, openFor, snapshot and loadLine are unchanged in shape.
  • plugins/tree/src/tree.js — noteTurnStart / noteTurnEnd take the turn id and pass it on.
  • plugins/tree/index.js — both feed handlers hand e?.turnId over instead of dropping it, with the reason written where the wiring is.
  • plugins/tree/test/load.test.js — #696 case: two turns open, the later one ends, and the age left is the older turn's; an unknown id and a missing id each drop exactly one turn (the oldest), and a named end closes the turn it names.
  • plugins/tree/test/tree.test.js — the same at feed level through list_children and the coordinator's stored prompt: after turn.end for s#2 the child still reads working (12 min, no turn ended), not 2 minutes.
  • docs/subsystems/tree.md — the #624 section now says an end closes its own turn, and names the fallback.

Still a routing hint, never a gate: nothing here refuses, ends or reassigns work.

What I ran

Tip SHA: 10e87463cb16f7e335d926fe04b5db9f30262c38 (Node 22.23.3, TZ=UTC, base 2f538019, run in a checkout of that base with these six files applied):

cd plugins/tree && node --import ./test/resolve.mjs --test --test-concurrency=2 "test/*.test.js"
  -> # tests 102 / # pass 102 / # fail 0
cd plugins/tree && node --import ./test/resolve.mjs --test test/load.test.js test/tree.test.js
  -> 91/91
node scripts/lint-plugins.mjs          -> 0 errors (pre-existing warnings only), @flybot/tree packs and imports
node core/scripts/check-arch-docs.mjs  -> arch-docs: OK (6 changed file(s))  [merge base = the PR base]
node .github/scripts/changelog.mjs check -> 13 fragment(s) in changelog.d/ are well formed

Red without the source change

With end() back to turns.shift() — the turn id ignored, exactly as on main — and both tests and call sites left alone:

not ok 5  - #696: an end closes its own turn, not just the oldest open one
  expected: 720000   actual: 120000
not ok 82 - #696: two turns open, the later one ends, and the age left is the older turn's
  expected: 720000   actual: 120000
# tests 91 / # pass 89 / # fail 2

120000 is s#2's 2 minutes where s#1's 12 should be — the defect itself. Restoring load.js gives 91/91, then 102/102 for the whole plugin.

Not run

  • npm run verify end to end: this box hits spawnSync /bin/sh EAGAIN at the default concurrency, so the steps above were run one by one.
  • The core suite: this change touches no core/ file and no host wiring, only the plugin's own tracker and its two feed handlers; the plugin suite and CI's tests-ok cover it.
  • No new end-to-end bench of a two-turn Bot: the two cases drive the same feed events the host emits (turn.start/turn.end with turnId), asserted through list_children and the stored prompt.

Risk

T2 — plugins/tree/** plus docs/subsystems/tree.md, 6 files, +87/−26. No core/ path, no .github/**, no security surface. An end with no id behaves exactly as it did before this PR, so no caller that omits the turn id changes behaviour.

Fixes #696

The feed names the turn it is about (`turnId`), and the load tracker threw it
away: it kept only start times, so `end()` dropped whichever turn was oldest.
With two turns open and the later one ending first, `openFor` reported the
newer turn's age — the exact state #624 exists to expose, hidden.

- load.js keeps each open turn as `{ turnId?, at }`, and `end(id, turnId)`
  closes that turn by id. No match — an id never seen, or none at all — still
  drops exactly one turn, the oldest, so the count cannot drift.
- tree.js and index.js hand the feed's `turnId` through instead of dropping it.
- An end event that arrives without an id (an older feed, or a caller with only
  a Bot id) therefore keeps working exactly as before.
@Birfy Birfy added agent-pr Pull request opened by an agent bug Something isn't working labels Oct 22, 2026
@Birfy Birfy self-assigned this Oct 22, 2026
@Birfy

Birfy commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 10e8746
Written-by: Reviewer

The defect is real and the fix closes it at the right layer. index.js now hands the feed's turnId to the tracker, load.js keeps { turnId, at } per open turn, and end() splices the turn it names instead of always the head of the array. Because pushes append and splices keep order, openFor() still answers the oldest turn that is really still open, and of/snapshot/loadLine keep their shape, so no reader changes.

The fallback is the part I checked hardest: end() with no id, or with an id it never saw, still removes exactly one turn — the oldest — so the count cannot drift and an older caller that passes only botId behaves as before. Two properties make that safe. Two turn.ends for one turn cannot happen: flybot-feed.js:252 fires only while entry.turn !== undefined and clears it, and a turn/start that closes a crashed turn emits its one end first (:203-215) — so the "unknown id" branch is only reached by a turn.start the plugin never saw (a plugin loaded mid-turn, or wave-off), and it stays inside the Bot's own list.

Ran: cd plugins/tree && node --import ./test/resolve.mjs --test --test-concurrency=2 "test/*.test.js" (TZ=UTC DSH_HARNESS=/opt/flybot)

  • tip 10e87463: 102 tests, 102 pass, 0 fail.

Ran: GET /repos/ThinkFlowLab/FlyBot/commits/10e87463/check-runs

  • 9 checks, all completed/success; mergeable: true, mergeable_state: clean; 6 files, +87/−26, base 2f538019.

Ran (red without the source change): copied the tip, restored plugins/tree/src/load.js, plugins/tree/src/tree.js and plugins/tree/index.js to 2f538019, kept both test files, same command

  • 102 tests, 100 pass, 2 fail — exactly #696: an end closes its own turn, not just the oldest open one and #696: two turns open, the later one ends, and the age left is the older turn's, the second reading the defect itself (120000 where 720000 belongs).

Not run:

  • npm run verify end to end (this box runs out of processes at its default concurrency); the pieces above were run one at a time, and the package job covers the rest at this tip.
  • No two-turn Bot end to end: the two cases drive the same turn.start/turn.end events the host emits, read back through list_children and the coordinator's stored prompt, which is the surface a coordinator actually reads.

Not blocking: the doc line now says "an id never seen, or none at all" drops the oldest, which is accurate but reads as normal traffic. The feed cannot produce that case for a turn the tracker saw (flybot-feed.js:252-259), so it is worth one clause — "a start the plugin missed" — because otherwise a later reader will look for a duplicate-end bug that cannot happen and miss the one that can (a plugin loaded while a turn was already running).

@Birfy
Birfy added this pull request to the merge queue Oct 22, 2026
Merged via the queue into main with commit bafe9ba Oct 22, 2026
9 checks passed
@Birfy
Birfy deleted the fix/696-load-pairs-by-turn-id branch October 22, 2026 07:52
github-merge-queue Bot pushed a commit that referenced this pull request Oct 22, 2026
… (#687)

Written-by: Dev2

Fixes #602

Tip SHA: e2cadce

## Reproduce

Nothing under `.github/scripts/` enqueues anything today: the queue call
is an instruction to the gating agent — `AGENTS.md` step 6,
`.claude/skills/fix-issue` → Merge, `docs/autonomous-dev.md` → How to
merge — and it is `gh pr merge <n> --auto --squash --match-head-commit
<sha>`. Nothing on that path reads `hold`: the only mechanical check is
`merge-gate.yml`, which judges a **queue entry**, i.e. after the pull
request is already in the queue (#639 tracks making it a required
check). #566 is the result: PASS at 09:48:18Z, enqueued 09:48:28Z,
merged 09:50:40Z as `abaf938b` while the standing instruction was
"parked — do not merge".

So on main today, a `hold` plus a PASS at the tip enqueues exactly like
any other PASS, and a PASS whose body says "record only — do not merge"
is read by every enqueue path as an ordinary PASS.

## What changed

- **New `.github/scripts/enqueue-guard.mjs`** — `node
.github/scripts/enqueue-guard.mjs --pr <n> [--repo owner/name] [--sha
<head>]`. It reads the live pull request (head, labels, changed files,
comments and review bodies) and calls `judge({ …, mode: 'queue' })` from
`merge-gate.mjs`, so "may this enter the queue" keeps one definition —
`hold`, the newest conclusion at that head, the computed T0/T1 exemption
and the mechanical-release approval are the gate's own rules, not a
second copy. It adds one rule of its own, `recordOnlyAt()`: the "record
only — do not merge / 仅记录、禁止入队" marker `AGENTS.md` → Merge gates rule 2
pairs with `hold`, so the record keeps the pull request out even if the
label is removed. Exit 0 prints the exact command and never runs it;
exit 1 prints `do not enqueue #<n>: <reasons>`, naming the hold.
- **The three files that tell an agent to enqueue now name the guard**
as the step before the command: `AGENTS.md` step 6,
`.claude/skills/fix-issue/SKILL.md` → Merge, `docs/autonomous-dev.md` →
How to merge (which also stops calling `--match-head-commit` optional).
- **The marker is a whole line, not a phrase.** The first PASS written
for this guard was refused by it: its bullet explaining `RECORD_ONLY`
quotes "Record only — do not merge" mid-sentence, and a phrase-anywhere
match read that as a parked record. A marker line — optionally a bullet
or a quote, optionally followed by the other language half — is what
freezes a pull request now, so a conclusion that explains the rule
cannot freeze itself.
- **The guard now carries `author_association` into `judge()`** (this
rebase). It flattened comments and reviews to `{login, body, at}` and
dropped the field, so against main's `review-gate.mjs` — which reads
that field since #676 — every conclusion was refused as "author
association missing is not OWNER, MEMBER or COLLABORATOR". Even a
perfect PASS could not clear the guard, which is why both of this
branch's earlier queue entries failed `tests-ok`; the fixture had the
same gap and also had no `writers` list or `Written-by:` header. The
mapping now matches `review-gate.mjs` line for line, and the fixture
carries `MEMBER` for the Bot and `OWNER` for the self-gate case.
- **New `.github/scripts/enqueue-guard.test.mjs`** — 15 tests through
`main()` with a fake `gh`, the way `merge-gate.mjs` is tested: held +
PASS refuses and names the hold; a PASS at the tip allows and prints the
exact command while touching only read endpoints; no conclusion refuses;
a later INC beats an older PASS; a PASS naming an older tip refuses; a
record-only PASS refuses; the newest conclusion decides in both orders;
a marker on another tip, or prose that quotes it, is not a freeze; a T1
pull request needs no PASS but a hold still refuses it; the self-gate
case (author `Birfy`, PASS by `Birfy`) passes; and the three instruction
files must keep naming the guard.

## What I ran

At this tip `e2cadcee9de0498e86af1c0e55a26494a3326a58`, rebased onto
main `bafe9bad` (between `2f538019` and `bafe9bad` main moved only in
`plugins/tree/**` and `docs/subsystems/tree.md` from #703 — no file of
this diff overlaps):

- `node --test .github/scripts/enqueue-guard.test.mjs` → **15/15 pass**.
- `node --test ".github/scripts/*.test.mjs"` → **188 tests, 184 pass, 4
fail**. The 4 failures are `ds-bot-patches.test.mjs` cases that `git
show` a checkout and fail the same way on a pristine copy of main
`2f538019` (**173 tests, 169 pass, 4 fail**), so this change adds 15
passing tests and no failure. `merge-gate.test.mjs` and
`review-gate.test.mjs` pass.
- `ARCH_DOCS_BASE=4eb81e5 node core/scripts/check-arch-docs.mjs` →
`arch-docs: OK (5 changed file(s))`.

## Red without the source change

On main the guard does not exist, so the new suite cannot even import:
**15/15 fail**. Three targeted mutations on the pushed tree, each naming
what the tests actually hold:

1. Drop `association: c.author_association` / `v.author_association`
from the two report mappings — the defect that failed this branch's
queue entries → **7 of 15 fail**, exactly the cases that need a
conclusion to count (a PASS at the tip, a newer INC, both "newest
decides" orders, prose that quotes the marker, the explaining PASS, and
the self-gate case).
2. Ignore labels in the `judge` call — the #602 bug (nothing checks
`hold` before the queue) → **2 of 15 fail**, exactly `a held pull
request is not enqueued, whatever a PASS at its tip says` and `hold
refuses a T0/T1 pull request too`.
3. Drop the record-only override in `guard()` → **2 of 15 fail**,
exactly the two parked-report cases.

## Not run

- No live `gh` on this box: the CLI wrapper around `main()` is not
executed, only `main()` itself (the tests drive it with a fake `gh`).
- No actionlint run locally; no workflow file is touched.
- No ruleset change: making `merge-gate` a required check on the merge
queue is the maintainer's action (#639) and is deliberately **not** part
of this PR — the guard works without it.
- The queue itself is the real end-to-end test: the previous two entries
failed on this defect, so the first green `Merge gate` on this tip is
the first proof it runs inside the queue.

## Risk

**T3** (the diff touches `.github/**` and `AGENTS.md`, which are T3
paths in `.github/agent/risk-rules.json`). Automation surface, but
narrow: the guard cannot merge or enqueue anything — it only reads the
API and prints a command — so the worst failure mode is a wrong *print*.
A false refusal blocks one enqueue visibly and loses no work; the
failure this rebase fixes was exactly that (a conclusion that could
never count), and it was visible as red `tests-ok` rather than as a
wrong merge. A false allow can only happen for something the merge gate
itself would also allow, or if an agent skips the guard, which is why
the three instruction files name it and one test asserts they do. `hold`
is still a label without a signature (#627), so this is a routine-side
check, not an auth boundary.
@Birfy Birfy removed the agent-pr Pull request opened by an agent label Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tree load: end a turn by its turnId, not by whichever start is oldest

1 participant