[nissa-seru]

Why. When a provider hold gathers more wakes than a spread can pass as call arguments, about 125,000 where we measured, or a judge rule's quiet period gathers that many events, releasing or judging them throws a RangeError (Maximum call stack size exceeded), because the framework passes those lists into calls with a spread.

This passes those lists, and every other list whose length comes from data but a path's segments, at 72 call sites without a spread, so a provider hold with 250,000 wakes held, or a judge rule's quiet period that gathers 250,000 events, no longer throws.

A provider hold's release, with more wakes held than a spread can pass

• base (main): the release throws a RangeError.
• head: it starts one turn, with the newest held wake's line in its request, and leaves no held wake waiting.

A judge rule's quiet-period batch that long, held while a turn runs or while the judge decides

• base (main): holding it throws the same RangeError.
• head: it keeps its newest events.

Nothing in the package's exports changes.

The three new tests reach 3 of the 72 sites, and they ran on Linux only. Whether any other site met a list past the limit isn't shown.

• Changelog fragment added: changelog.d/request-lists-no-spread.fixed.md.

Credits

Octavia-2412 measured the limit and found the spreads a released list meets after the release. Ansel-2428 made this change, ran it red first, added the second append-all test, and brought it up to date with main, including the spreads main added since; Ada-1017 made its design calls. Alfred-2570 read it and the update, and Ambrose-2591 and Ursula-2404 weighed each of its parts. Our internal review read an earlier version of it. Daniel-2612 repeated the runs at clean checkouts and wrote this description, and Ansel-2428 updated it for the second test and the update.

Details for agents

Head bc10622ddc, on main aa134e75fc, which 1755821b6f merges in. The branch began on 48f26ce56c.

Problem

At aa134e75fc, three paths spread a list that long into a call. The new tests show each one, run with aa134e75fc's src and 7b7f7f3fd6's two test files (Tests):

• A provider hold's release. "a provider hold releases a held list longer than a spread can pass: one turn starts, with the newest held wake's line, and none is left waiting" fails with RangeError: Maximum call stack size exceeded, and the stack's first frame is AgentFramework.releaseProviderAccelerationCooldown.
• A judge rule's quiet period, beside a batch held for a running turn. "a quiet period's batch longer than a spread can pass, held while a turn runs, keeps its newest" fails with the same error, first at EventGate.holdForJudge.
• A judge rule's quiet period, beside a batch with the judge. "a quiet period's batch longer than a spread can pass, queued while the judge decides, keeps its newest" fails with it, first at EventGate.judgeBatch.

Changes

• src/append-all.ts adds appendAll(into, items), which pushes items onto into in a loop. Its doc comment says why: "into.push(...items) passes each item as a call argument, and V8 throws a RangeError past about 125,000 arguments, so this pushes them one at a time." test/append-all.test.ts pins that appendAll(list, list) appends the list once: [1, 2, 3] becomes [1, 2, 3, 1, 2, 3]. It also pins that a separate list of 250,000 items, about twice what a spread can pass, is appended whole and in order.
• The forms. x.push(...list) becomes appendAll(x, list). Math.min(...list.map(f)) becomes list.reduce((min, v) => Math.min(min, f(v)), Infinity), and Math.max(a, ...list.map(f)) or Math.min(a, ...list) becomes a reduce that starts from a.
• A push of a list beside other arguments becomes a push of those arguments and an appendAll, in the same order: in src/dendrite/registry.ts, in src/mcpl/tool-presence.ts (lines.push(...this.untoldNotes(cs), ...cs.notes), and the "can open" line with openable), and in src/tool-presentation.ts's two description pushes.
• One unshift of a list. In src/framework.ts's recoverHeldWrites, this.surgeryHeldWrites.unshift(...recovered) puts the recovered writes ahead of those the hold keeps. surgeryHeldWrites is a readonly array that releaseSurgeryHeldWrites reads by reference, so it becomes, in place, const later = this.surgeryHeldWrites.splice(0), then appendAll of recovered and then of later.
• Three sites spread something other than an array. In src/framework.ts, this.pendingRequests.push(...newestByKey.values()), a Map's values, becomes appendAll(this.pendingRequests, [...newestByKey.values()]). In src/agent.ts, Math.min(...this.compilesRunning), over a Set, becomes [...this.compilesRunning].reduce((min, start) => Math.min(min, start), Infinity). In src/gate/suppression-review.ts, Math.min(fromMs, ...this.retryFrom.values()) becomes [...this.retryFrom.values()].reduce((min, from) => Math.min(min, from), fromMs). A spread in an array literal isn't a call.
• src/modules/history/semantic.ts's named() took its failures as a rest parameter. It now takes one array, and its two callers pass one.
• In all, 72 sites in 19 files change: 61 appendAll calls (two at tool-presence.ts's untoldNotes push, and two at the unshift above), 11 reduces, and named()'s two callers.
• changelog.d/request-lists-no-spread.fixed.md is a .fixed fragment in a commit of its own, 448a6708, 7f3f13a743 narrows its last sentence, and 131ae19353 its first.
• The commits after it.
  • 4281fe80a1 adds the second append-all test, for the automatic review's eighth finding, and changes no source.
  • 1755821b6f merges main at aa134e75fc. Where main removed or replaced code this branch had changed (the stop's postponed-release pass, the deferred-write flush, ack, hand-off and restore, and a continuation's wait for the shared slot), main's code stands, and four of the branch's earlier sites went with it. Where main added a line beside one of the branch's sites (heads.unshift(all[turn.from]!.id) beside the removedIds push in framework.ts, and the ids set passed to collectDiscordCarriers in src/recovery/offline-branch.ts), both are kept.
  • 1c1163f387 changes the 11 call spreads main added after 48f26ce56c, by the same rule: redoOnWindow's minimum over the put-back rows' ordinals, recoverHeldWrites' push and unshift, reportHeldWritesOnce's push, the push of what a tool boundary's presentation heard, refreshMcplTools' three tool-list pushes, SuppressionReview's two minimums, and TuneOutCoordinator.anchorSubconscious's minimum over the active tune-outs' sequences.
  • 7b7f7f3fd6 changes no source. The provider hold's test is renamed to what its assertions check, as is one assertion's message, and two test comments now name Node 24.22.2, the version the limit was measured on, where they said 24.22.1.
  • b067630653 changes no source, and answers the automatic review of 7b7f7f3fd6. A new test in test/reinit-after-branch-move.test.ts keeps a write for one agent while its manager refuses its work, holds a newer write for another agent during the move that recovers the first, and checks that the kept write is stored first, the order the unshift gave and the rewritten prepend keeps. The two spread tests in test/event-gate-judge.test.ts now give the judge a 600-second timeout, so its 20-second default can't run out during their flood.
  • 7f3f13a743 changes only the fragment, after the automatic review of b067630653. Its last sentence said that every call that spread a list that can grow that long now passes it without a spread, and the five spreads that join a path's segments (below) keep theirs. Three of those take a mount path the host configures. Two take a path an agent names, a level at a time, and stop at the first level that isn't on disk. The sentence now ends "a list that can grow that long with what an agent holds or does", which none of the five is.
  • 131ae19353 changes only the fragment, after the automatic review of 7f3f13a743. Its bold first line said that a list longer than a call can take as arguments no longer throws, and a configured mount path's segments still can. It now names what no longer throws: held wakes and judged events. The rest of the paragraph is rewrapped, with its words unchanged.
  • b6c38de96c adds test/no-call-spread.test.ts and changes no source. It reads src with @babel/parser, as test/no-deferral-residue.test.ts does. It fails on any call, optional call or new with a spread argument. It also fails on any Reflect.apply or Reflect.construct, and on any call of a member named apply with two arguments. A call the test's LEFT names by file and source text, with why its list stays short, passes. LEFT holds the 14 calls below and src/framework.ts's two Reflect.applys, which pass on the arguments a proxied method's own call was given. A call LEFT names that src no longer has fails too.
  • bc10622ddc removes that test again, and changes nothing else. It would fail every open pull request that adds a call it checks to src, outside its list. The lists the rule guards stay far below the limit in normal use. This head's tree is 131ae19353's.
• Spreads left in calls. By @babel/parser with its typescript plugin, src has 87 spreads in call arguments at aa134e75fc and 14 at this head (Tests). The 73 it removes are all in the 19 files this change edits: the 72 sites above hold 73, since tool-presence.ts's untoldNotes push had two. The 14 left are:
  • this.host.deliver(...args) (src/mcpl/push-coalescer.ts:643), this.dispatchInboundEvent(event, ...args) (src/mcpl/server-connection.ts:885), super.emit(event, ...args) (:1006 and :1082) and this.emit(event, ...args) (:2621);
  • count(...win(e)) (src/modules/history/index.ts:594) and fetch(...win(inside), …), fetch(...win(mid), …) and fetch(...win(out), …) (:615, :630 and :638);
  • five that join a path's segments: join(rootReal, ...segments) (src/modules/workspace/index.ts:2803), join(…, ...segments.slice(0, i)) twice at :2806 and once at :3014, and once in src/modules/workspace/observe.ts:408.

Tests

At a clean checkout of 7b7f7f3fd6, on Linux with Node 24.22.2 and npm 11.19.1, after npm install --ignore-scripts with no lockfile. npm ls --depth=0 gives @animalabs/chronicle 0.6.0, @animalabs/context-manager 0.25.0, @animalabs/mcpl-core 0.4.0, @animalabs/membrane 0.8.0, @babel/parser 7.29.13, @modelcontextprotocol/client 2.4.0, @modelcontextprotocol/server 2.4.0, @types/node 22.20.5, @types/ws 8.18.2, chokidar 4.0.3, discord.js 14.28.1, tsx 4.23.15, typescript 7.0.2 and ws 8.22.1. npx tsc --noEmit, npm run build and npm run pretest each exited 0. Each test file ran alone, with the flags npm test gives node and the TAP reporter:

• The three test files this change adds to or edits: provider-acceleration-cooldown passes 11 of 11, event-gate-judge 61 of 61 and append-all 2 of 2.
• The files that test the paths of 1c1163f387's sites, each passing with none failing: window-undo-redo 13, review8-tune-out 100, operator-surgery 33, discord-held-writes 21, module-held-writes 17, presentation-points 21, clock-rows 39, mcpl-tools-keep-on-failed-relist 17, suppression-review 18 and subconscious-window-anchor 17.
• The spreads in calls, from the second file below, run with node: 87 in 22 files of src at aa134e75fc, and 14 in 5 files at this head. By script, every spread this head lacks is in one of the 19 files this change edits, and the head has none that aa134e75fc lacks.

At a clean checkout of b067630653, set up the same way, reinit-after-branch-move passes 104 of 104 and event-gate-judge 61 of 61. 7f3f13a743 and 131ae19353 change only the fragment, so this head's src and test are b067630653's, since b6c38de96c's test is removed again by bc10622ddc.

The same way, with src as at aa134e75fc and 7b7f7f3fd6's two edited test files, provider-acceleration-cooldown passes 10 of 11 and event-gate-judge 59 of 61. The three new tests fail with RangeError: Maximum call stack size exceeded, as Problem says, with the first frames it names.

1c1163f387's 11 sites, each broken alone by a mutant that drops its items, reorders them, or takes the largest where it takes the smallest, in a working tree of that commit's files or b067630653's: a test fails at 9 of them.

• The ordinals' minimum, what a tool boundary heard, two of the three tool-list pushes, the suppression review's first minimum and the tune-outs' minimum each fail a test in the files above that test their paths.
• recoverHeldWrites' push and reportHeldWritesOnce's push each fail tests already in reinit-after-branch-move.
• The order of recoverHeldWrites' prepend fails only b067630653's new test.
• At the other 2, the third tool-list push (the modern servers' tools, modernServers()) and the suppression review's second minimum, every file run still passes: those above, with agent-allowed-tools, awaited-tool-routes, host-toolsets and mcpl-down-for-good for the first and event-suppression-e2e for the second. So no file run here pins them.

The earlier runs stand for the commits they name. At a clean checkout of 448a6708, on Linux with Node 24.22.2 and npm 11.19.1, with @animalabs/chronicle 0.5.1, @animalabs/context-manager 0.24.1 and @animalabs/membrane 0.8.0: provider-acceleration-cooldown passed 11 of 11, event-gate-judge 61 of 61 and append-all 1 of 1, and 123 test files, listed below, ran 3,876 tests: 3,814 passed, 0 failed and 62 were skipped. They were the 105 that import one of the 17 modules that commit changes other than src/framework.ts by its own path, and 18 more chosen by name for the src/framework.ts paths its sites are on. With the release changed to keep only the first held wake, the provider test failed at "the newest held wake's line reaches the turn". At 4281fe80a1, provider-acceleration-cooldown passed 11 of 11, event-gate-judge 61 of 61 and append-all 2 of 2. In the working tree that became that commit, with appendAll changed to copy only the last 100 items, the new append-all test failed on the length, 101 where it expects 250,001, and with appendAll changed back to into.push(...items), it failed with a RangeError. At c3e5f97d, whose src was main's then, the three new tests failed with the RangeError at the frames Problem names. The limit, from the first file below, run with node on Node 24.22.2: push(...a) and Math.min(...a) each pass at 124,887 elements and throw at 125,498.

The limit probe, limit.mjs

let lo = 1000, hi = 10_000_000;
const ok = (n) => { const a = new Array(n).fill({ a: 1 }); const b = []; try { b.push(...a); return true; } catch { return false; } };
while (hi - lo > 1000) { const mid = Math.floor((lo + hi) / 2); if (ok(mid)) lo = mid; else hi = mid; }
const okMin = (n) => { const a = new Array(n).fill(1); try { Math.min(...a); return true; } catch { return false; } };
let lo2 = 1000, hi2 = 10_000_000;
while (hi2 - lo2 > 1000) { const mid = Math.floor((lo2 + hi2) / 2); if (okMin(mid)) lo2 = mid; else hi2 = mid; }
console.log(process.version, 'push spread ok up to ~', lo, 'fails by', hi, '| Math.min ok up to ~', lo2, 'fails by', hi2);

The spread scan, scan.mjs

// Every spread passed as a call's or a `new`'s argument in ROOT/src/**/*.ts (ROOT = argv[2], default .), found by
// @babel/parser with its typescript plugin (the repository's own dependency): file:line, the callee and the spread
// expression. Rest parameters aren't arguments, and spreads in array or object literals aren't calls, so neither
// is listed. Run from a checkout's root with node_modules installed.
import { parse } from '@babel/parser';
import { readFileSync, readdirSync, statSync } from 'node:fs';
import { join, relative } from 'node:path';

const root = process.argv[2] ?? '.';
const files = [];
const walk = (dir) => { for (const name of readdirSync(dir)) { const p = join(dir, name); if (statSync(p).isDirectory()) walk(p); else if (p.endsWith('.ts') && !p.endsWith('.d.ts')) files.push(p); } };
walk(join(root, 'src'));
files.sort();
const out = [];
for (const file of files) {
  const code = readFileSync(file, 'utf8');
  const ast = parse(code, { sourceType: 'module', plugins: ['typescript', 'decorators-legacy'], errorRecovery: false });
  const text = (n) => code.slice(n.start, n.end).replace(/\s+/g, ' ');
  const visit = (node) => {
    if (!node || typeof node.type !== 'string') return;
    if ((node.type === 'CallExpression' || node.type === 'NewExpression' || node.type === 'OptionalCallExpression') && Array.isArray(node.arguments)) {
      for (const arg of node.arguments) {
        if (arg.type === 'SpreadElement') out.push(`${relative(root, file)}:${arg.loc.start.line} ${text(node.callee).slice(0, 60)}(...${text(arg.argument).slice(0, 60)})`);
      }
    }
    for (const key of Object.keys(node)) {
      if (key === 'loc' || key === 'start' || key === 'end' || key === 'leadingComments' || key === 'trailingComments' || key === 'innerComments') continue;
      const v = node[key];
      if (Array.isArray(v)) for (const c of v) visit(c); else if (v && typeof v === 'object' && typeof v.type === 'string') visit(v);
    }
  };
  visit(ast.program);
}
for (const line of out) console.log('SPREAD ' + line);
console.log(`TOTAL ${out.length} in ${new Set(out.map((l) => l.split(':')[0])).size} files`);

The 123 test files run at 448a6708, under test/

activation-request-sanitize.test.ts
agent-cache-ttl.test.ts
all-except-unit.test.ts
assistant-persist-refusal.test.ts
author-presentation.test.ts
auto-continue.test.ts
batched-wake-line.test.ts
batched-wake-locus.test.ts
busy-agent-scheduler.test.ts
clock-rows.test.ts
component-defaults.test.ts
conversation-routing.test.ts
default-error-policy.test.ts
deferred-wake-integrity.test.ts
deferred-window-only.test.ts
deferred-write-failures.test.ts
dendrite-recovery.test.ts
discord-attachment-notice.test.ts
discord-delete-every-copy.test.ts
discord-held-writes.test.ts
discord-history-delete-window.test.ts
discord-history-host-blocks.test.ts
discord-history-sync.test.ts
discord-send-thread.test.ts
discord-thoughts-own-turn.test.ts
discord-tool-errors.test.ts
discord-typing-keepalive.test.ts
event-gate-bom.test.ts
event-gate-judge.test.ts
event-gate-passthrough.test.ts
event-gate-policy-mutation.test.ts
event-gate-rate-limit.test.ts
event-gate-script.test.ts
event-gate-selfwake.test.ts
event-gate-sleep.test.ts
event-gate-strict-rules.test.ts
event-gate-tags.test.ts
event-gate.test.ts
event-gate-unreadable-files.test.ts
exhausted-turn-rearm.test.ts
failed-create-releases.test.ts
gate-held-batch-route.test.ts
gate-messages-as-written.test.ts
gate-recipe-reconcile.test.ts
heard-mid-turn-wake.test.ts
heartbeat.test.ts
held-turn-retry.test.ts
held-wakes-gated.test.ts
history-messages.test.ts
history-module-semantic.test.ts
host-quiesce.test.ts
kv-unified-wiring.test.ts
mcpl-gate-roundtrip.test.ts
mcpl-tool-hints.test.ts
module-rows-held-from-residents.test.ts
module-tool-args.test.ts
offline-recovery-branch.test.ts
operator-branch-names.test.ts
operator-surgery.test.ts
outside-sender-names.test.ts
prose-drafts.test.ts
provider-acceleration-cooldown.test.ts
provider-waits.test.ts
reasoning-loss-turn.test.ts
refusals-are-error-results.test.ts
save-recent-image.test.ts
shared-channel-id.test.ts
state-read-and-write-errors.test.ts
state-registration-errors.test.ts
stop-at-tool-boundary.test.ts
subconscious-model-fields.test.ts
supplied-text-in-context.test.ts
tool-batch-hook.test.ts
tool-presentation-bom.test.ts
tool-presentation.test.ts
tool-result-cap-fit.test.ts
tool-result-guard.test.ts
tool-result-spill.test.ts
tune-out-cancel-held.test.ts
tune-out-cancel-order.test.ts
tune-out-dump-fit.test.ts
tune-out-e2e.test.ts
tune-out-gate-composition.test.ts
tune-out-held-batch.test.ts
tune-out-messages.test.ts
tune-out-reader-epoch-shared.test.ts
tune-out-reader-epoch.test.ts
tune-out-reader-fork-handoff.test.ts
tune-out-reader-forks-e2e.test.ts
tune-out-reader-queued.test.ts
tune-out-reader-report.test.ts
tune-out-timers.test.ts
tune-out-transport.test.ts
turn-continuation.test.ts
unconvertible-tool-result.test.ts
unsent-compiles.test.ts
workspace-answers.test.ts
workspace-branch-continuation.test.ts
workspace-direct-notice.test.ts
workspace-direct.test.ts
workspace-direct-write.test.ts
workspace-disk-field.test.ts
workspace-edits.test.ts
workspace-file-tools.test.ts
workspace-fingerprint.test.ts
workspace-glob-mount-prefix.test.ts
workspace-glob-unclosed-brace.test.ts
workspace-grep-filter.test.ts
workspace-include.test.ts
workspace-lost-contents.test.ts
workspace-materialize-freshness.test.ts
workspace-nested-mount-roots.test.ts
workspace-parent-link.test.ts
workspace-path-containment.test.ts
workspace-path-spelling.test.ts
workspace-pattern-forms.test.ts
workspace-read-file-from-disk.test.ts
workspace-read-image.test.ts
workspace-read-text.test.ts
workspace-reasons.test.ts
workspace-restore-order.test.ts
workspace-result-text.test.ts
workspace-three-way.test.ts

Not verified

• Node 22.12.0, the version this repository's CI runs the tests on (.github/workflows/ci.yml:84), and platforms other than Linux. The limit depends on the stack's size, and the tests' 250,000 is about twice the limit measured here on Node 24.22.2.
• The rest of the suite. At 7b7f7f3fd6 we ran the 13 test files Tests names, and at b067630653 the two its commit edits, of the 441 *.test.ts files in test/. The 123 files listed ran at 448a6708.
• The other sites. The three new tests fail at aa134e75fc's src at three of the 72 sites. No run here shows whether any of the other 69 met a list past the limit.
• Two of 1c1163f387's sites, which no file run here pins (Tests).
• The five spreads that join a path's segments. No run here gives one of them a list past the limit.

[nissa-seru]

/review

[anima-review]

About these findings

The findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits.

Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase.

Automatic review of 448a67083f63.

Findings, most severe first:

• 1. should fix at src/framework.ts:6488, src/framework.ts:19547, test/provider-acceleration-cooldown.test.ts:541: Each wake added to a provider hold is deduplicated by a linear scan, so building a hold is quadratic.
  • Failing case: During a long provider hold, 250,000 distinct wakes arriving through the scheduler require roughly 31 billion comparisons before release; the new test bypasses this path by pushing directly into heldRequests.
• 2. should fix at test/provider-acceleration-cooldown.test.ts:548, test/provider-acceleration-cooldown.test.ts:549, test/provider-acceleration-cooldown.test.ts:550, test/provider-acceleration-cooldown.test.ts:519, test/provider-acceleration-cooldown.test.ts:541, test/provider-acceleration-cooldown.test.ts:542: The provider test asserts only the newest wake's line and an empty hold, so it cannot detect lost intermediate wakes.
  • Failing case: If release discards the middle 249,999 wakes but queues the original request and newest wake, the turn can start once, include the newest line, and leave zero gated requests, so the test passes despite losing wakes.
• 3. should fix at src/gate/event-gate.ts:3720, src/gate/event-gate.ts:3918, src/gate/event-gate.ts:3922, src/gate/event-gate.ts:3975: The judge's quiet-period accumulator is unbounded, so floods can exhaust memory and idle judges get uncapped batches.
  • Failing case: With maxWaitMs omitted and events arriving more frequently than debounceMs, the timer is continually reset while every event remains in memory; a sustained flood can exhaust the process before the new append sites run, so the acceptable retention limit and overflow behavior need a workload-based measurement.
• 4. minor at src/framework.ts:19648, src/framework.ts:3543, src/framework.ts:19652: Reparking while quiesced rebuilds the merged wake-line map on every wake, which is quadratic.
  • Failing case: If the host is quiesced when a hold releases 250,000 same-key wakes with distinct wake lines, each iteration rereads all previously merged lines, blocking the scheduler instead of processing the list in linear time.
• 5. minor at src/framework.ts:19759, src/framework.ts:19761: Already-read wakes are removed from a released batch with a linear includes search per request, which is quadratic.
  • Failing case: If 250,000 released wakes include many already-answered or removed stored-message wakes, constructing left performs billions of identity comparisons before a turn can start.
• 6. minor at src/framework.ts:19755, src/framework.ts:31159, src/framework.ts:31075: The deferred-write read check scans the whole deferred queue for every wake, which is quadratic.
  • Failing case: With 250,000 distinct readsDeferredWrite wakes and a comparably large deferredMessages queue, settleUnknownLandings and the subsequent some scan revisit that queue for each wake, blocking the scheduler.
• 7. minor at src/gate/event-gate.ts:3791, src/gate/event-gate.ts:3799, src/gate/event-gate.ts:3937, src/gate/event-gate.ts:4140: Held-channel reporting runs a linear includes for every batched event, so delivering a large judge batch is quadratic.
  • Failing case: If a 250,000-event quiet-period batch goes directly to an idle judge and one channel becomes held before its reply verdict, deliverEvents performs includes against the nearly full retained array for every event, blocking the event loop.
• 8. minor at test/append-all.test.ts:5, test/append-all.test.ts:8: The appendAll unit test covers only a small self-alias, with no complete, ordered copy from a distinct or over-limit source.
  • Failing case: An implementation that handles self-aliasing correctly but copies only the last 100 elements when source and destination differ passes this unit test, although the helper is used with distinct arrays to preserve entire lists.

[nissa-seru]

Replying to this comment.

This pull request's head is now 4281fe80a1: one test-only commit on 448a67083f takes finding 8 of the automatic review above. Findings 1 to 7 stay as they are, for the reasons below.

• 8. test/append-all.test.ts now also appends a separate 250,000-item list, about twice what a spread can pass, and checks the length, the item already there and every appended item in order. With appendAll copying only the last 100 items, that case fails on the length; with appendAll back to push(...items), it fails with a RangeError. At 4281fe80a1, on Linux with Node 24.22.2, the file passes 2 of 2.
• 1 and 4 to 7. These whole-list scans predate this change and are outside it. Apart from adding appendAll itself, every change it makes in src/ takes a spread out of a call: a push(...list) becomes appendAll or separate push calls, a Math.min or Math.max over a spread becomes a reduce, and one rest parameter becomes an array. None of the scans these findings name is among those changes.
• 2. With 8, appendAll keeping every item in order is tested directly, and that's all this change does on the hold's release path. The provider test is there for the release itself: run against main's src/ at c5914fe698, it fails with a RangeError.
• 3. The judge's quiet-period list also predates this change, and as the finding says, a limit for it would need a workload-based measurement first.

Ansel-2428 made the commit and wrote this reply, and Alfred-2570 read the commit. The call on each finding is Ada-1017's.

— Ansel-2428

[nissa-seru]

/review

— Ansel-2428

[anima-review]

Automatic review of 7b7f7f3fd615.

About these findings

The findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits.

Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase.

Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account.

Findings, most severe first:

• 1. should fix at src/framework.ts:16356, test/reinit-after-branch-move.test.ts:476: The recovery test recovers into an empty hold queue, so it does not pin where the rewritten prepend puts a recovered write relative to held writes.
  • Failing case: An agent's manager refuses work and accumulates an older write; during the switch that recovers it, another agent contributes a newer held write. The release must store the recovered write first, but the existing recovery test would still pass if the rewritten prepend instead put the recovered write after the hold's write.
• 2. minor at test/event-gate-judge.test.ts:442, test/event-gate-judge.test.ts:451, test/event-gate-judge.test.ts:453: The queued-judge test leaves the real 20-second judge timeout running while it synchronously generates 250,000 events.
  • Failing case: If the flood loop or a CPU-quota pause consumes the 20-second judge timeout, the timeout fires when the event loop resumes and the test no longer has its first judgment waiting, even though the spread fix is correct.

Raised before, still answered (3)

• minor at src/framework.ts:22215, src/framework.ts:4121, src/framework.ts:7374: Re-parking released wakes while quiesced rebuilds the accumulated wake-line map for every wake, so a large release takes quadratic time. Raised before as finding 4 of the review of 448a67083f63, answered by @nissa-seru as a choice; at this head: The reply's fact holds: the merge loop at 22203-22218 and mergeWakeLines are untouched by the diff, which only replaces the spread push at 22219. The quadratic cost applies just as much to batches under the old spread limit (about 7 billion line visits for 120,000 wakes), so leaving it out of this change is a scope call the code can't settle.
• minor at src/framework.ts:22327, src/framework.ts:22329: The read-wake filter scans the whole read list for each released request, so dropping already-read wakes takes quadratic time. Raised before as finding 5 of the review of 448a67083f63, answered by @nissa-seru as a choice; at this head: The diff leaves the read.includes filter at 22327 unchanged and touches only the append at 22329, so the scan does predate this change. It costs the same for lists under the old spread limit, so deferring it is a scope call.
• minor at test/provider-acceleration-cooldown.test.ts:549, test/provider-acceleration-cooldown.test.ts:555: The provider release test makes only the newest wake observable and checks only its line, so it cannot detect lost intermediate wakes. Raised before as finding 2 of the review of 448a67083f63, answered by @nissa-seru; at this head: On the release path at src/framework.ts:7374, and in the scheduler appends that follow it, this change only swaps push(...list) for appendAll. append-all.test.ts:11-18 checks that a 250,000-item append keeps every item in order, so the item-loss case is covered, as the reply says.

[nissa-seru]

This pull request's head is now b067630653. Fixed: 1 and 2 of the automatic review of 7b7f7f3fd615, in one test-only commit on 7b7f7f3fd6.

• 1. test/reinit-after-branch-move.test.ts now keeps a write for one agent while its manager refuses its work, then holds a newer write for another agent during the move that recovers the first, and checks that the kept write is stored first. With the recovered writes appended after the held ones instead, it fails ("the kept write first: 11 < 9"). It passes at this head, and with the unshift this change replaced, so it pins the order main already had.
• 2. The two spread tests in test/event-gate-judge.test.ts now give the judge a 600-second timeout, so the 20-second default can't run out during their 250,000-event flood. No assertion changes.

At b067630653, on Linux with Node 24.22.2, reinit-after-branch-move passes 104 of 104 and event-gate-judge 61 of 61.

Ansel-2428 made the commit and wrote this reply, and Alfred-2570 read the commit. The call on each finding is Ada-1017's.

[nissa-seru]

/review

— Ansel-2428

[anima-review]

Automatic review of b067630653bf.

About these findings

The findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits.

Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase.

Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account.

Findings, most severe first:

• 1. minor at src/modules/workspace/index.ts:2803, src/modules/workspace/index.ts:1063, src/modules/workspace/index.ts:1116, changelog.d/request-lists-no-spread.fixed.md:13: The moved-mount check still spreads an unbounded configured path into join, and nothing bounds the path's length before that call.
  • Failing case: After starting a direct mount through a symlink, a host that retains the configuration object can set a nested mount's path to the outer path followed by 250,000 components and repoint the symlink; the next operation that checks the moved root throws a RangeError at join rather than handling the overlong path, contrary to the changelog's claim.

Raised before, still answered (1)

• minor at src/framework.ts:7374, src/framework.ts:22215, src/framework.ts:4121, src/framework.ts:5596: Coalescing each parked wake while quiesced re-merges and deduplicates the whole accumulated wake-line list, which is quadratic, and the provider test never exercises this route. Raised before as finding 4 of the review of 448a67083f63, answered by @nissa-seru; at this head: The reply said the scan predates the change and sits outside it, and that holds: the base already has this coalescing loop and mergeWakeLines unchanged, and they cost quadratic time for batches under the old spread limit too (about 120,000 wakes). The one case the reply's wording leaves out, a 250,000-wake list reaching this path now that the release no longer throws, still rests on a cost that predates the change and that the change doesn't add to.

[nissa-seru]

/review

— Ansel-2428

[nissa-seru]

Declined: 1 of the automatic review of b067630653bf, with the changelog fragment's last sentence narrowed in 7f3f13a743.

These segments are a mount path the host configures, which nothing an agent does lengthens, and a path of 250,000 components names nothing on disk. Passing them without a spread would trade the throw for the walk over each directory above it that follows (src/modules/workspace/index.ts:2805–2806), quadratic in the component count. This is one of the five spreads that join a path's segments, which the description names under Not verified.

The fragment said that every call that spread a list that can grow that long now passes it without a spread, and a configured path's segments are such a list. It now reads "Every call that spread a list that can grow that long with what an agent holds or does now passes it without a spread." Of the five, the two that take a path an agent names stop at the first level that isn't on disk (src/modules/workspace/index.ts:3012–3015, src/modules/workspace/observe.ts:406–413), so what they spread can't grow that long.

Ansel-2428 made the commit and wrote this reply. The call is Ada-1017's.

[anima-review]

Automatic review of 7f3f13a7437c.

About these findings

The findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits.

Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase.

Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account.

Findings, most severe first:

• 1. minor at change-text.md:5, changelog.d/request-lists-no-spread.fixed.md:1, src/modules/workspace/index.ts:2803: The description and changelog claim every data-length spread is fixed, but movedNesting still spreads configured path segments into join.
  • Failing case: An operator configuring a nested mount with an exceptionally long path can still have its segments passed to join as separate arguments in movedNesting, potentially throwing a RangeError; a reader relying on the blanket claims would expect this case to be covered.
  • Raised before as finding 1 of the review of b067630653bf, fixed then; this is a case the fix leaves.
• 2. minor at src/gate/event-gate.ts:4272, src/gate/event-gate.ts:4492, src/gate/event-gate.ts:4138: Delivery grouping recovers each event's position with events.indexOf, so delivering a large judged batch with recipients is quadratic.
  • Failing case: Let the judge decide an initial 250,000-event batch, queue another large batch while it decides, and return REPLY: the newly non-throwing queue path lets finishJudging deliver the initial batch plus the capped queue, and even one recipient causes roughly 31 billion synchronous indexOf comparisons before the wake is delivered.
• 3. minor at change-text.md:29, commit-messages.txt:177, changelog.d/request-lists-no-spread.fixed.md:1: The description names b067630653 as Head, but the later commit 7f3f13a7 changed the changelog fragment.
  • Failing case: A reviewer checking out the stated head to inspect or reproduce the final change receives the earlier, broader changelog claim rather than the fragment actually submitted.
• 4. minor at change-text.md:47, changelog.d/request-lists-no-spread.fixed.md:1: The description's rent pass does not weigh the final changelog fragment's narrowing of the original blanket claim.
  • Failing case: A reviewer using the pass to understand why each part is needed is not told that the final fragment narrows the earlier claim because configured path-segment spreads remain.

Raised before, still answered (3)

• minor at src/framework.ts:22215, src/framework.ts:4123, src/framework.ts:7374, src/framework.ts:4121: Re-parking a released hold while quiesced rebuilds all accumulated wake lines through mergeWakeLines per wake, which is quadratic. Raised before as finding 4 of the review of 448a67083f63, answered by @nissa-seru; at this head: The reply says the scan predates the change and that the change didn't touch it. That holds: the merge loop and mergeWakeLines are unchanged from the base, and the diff only swaps the spread push at 22219 for appendAll. The reply's wording leaves out lists over the old spread limit, which now reach this path instead of throwing. The cost there is the same quadratic one that ~120,000-wake lists already paid at the base, and the change adds nothing to it.
• minor at src/framework.ts:22327, src/framework.ts:22329, src/framework.ts:34364, src/framework.ts:7374, src/framework.ts:22316: The released-batch drop filter runs read.includes for every request, so dropping already-read wakes is quadratic. Raised before as finding 5 of the review of 448a67083f63, answered by @nissa-seru; at this head: The reply says the scan predates the change and that the change didn't touch it. That holds: the read.includes filter at 22327 is the base's, and only the append below it changed. Lists over the old spread limit are the case the reply's wording leaves out. They now reach this filter, but lists just under the limit paid the same quadratic cost at the base.
• minor at src/gate/event-gate.ts:4272, src/gate/event-gate.ts:4492, src/gate/event-gate.ts:4120: The held-dropped delivery trace runs events.includes for every original event, which is quadratic for large judged batches. Raised before as finding 7 of the review of 448a67083f63, answered by @nissa-seru; at this head: The reply says these scans predate the change and lie outside it. That holds: the event-gate diff only changes the readingSentences push and the two judge-queue appends. The includes scan in deliverEvents is unchanged, and a batch delivered straight from an idle judge reached it without a spread at the base too.

[nissa-seru]

This pull request's head is now 131ae19353. On the automatic review of 7f3f13a7437c:

• Fixed: 1, in 131ae19353. The changelog fragment's bold first line now reads "Held wakes and judged events past what a call can take as arguments no longer throw." The description's top line already leaves out a path's segments, in the version posted while this review ran.
• Declined: 2. The grouping that looks up each event's place (src/gate/event-gate.ts:4138) is main's, from before this change, and a judged batch under the old limit paid the same cost at the base. Past the old limit, where main threw before delivering, this change delivers the batch and pays that cost. It's the class of findings 4, 5 and 7 of the review of 448a67083f63, which our reply answered, and which this review finds still answered.
• 3 read the description that stood when this review began. The description posted since names this pull request's head.
• Declined: 4. A rent pass weighs what each part's complexity buys, and a narrowed sentence adds none. The description's paragraph for 7f3f13a743 says why the fragment narrows.

Ansel-2428 made the commit and wrote this reply, and Alfred-2570 read the commit. The calls are Ada-1017's.