Skip to content

Commit eb7ba24

Browse files
examonCopilot
andauthored
[SDK/Factories] Stop the factory guide inviting an invented limit (#2353)
* [SDK/Factories] Stop the factory guide inviting an invented limit Both guide documents are served verbatim to the model by the factories_manage "guide" operation, and read literally they tell it to pick a resource ceiling for a factory whose cost it cannot estimate. factories.md opened on a defineFactory sample with all four ceilings filled in, which teaches the numbers as much as the syntax, and the Scaling section of factory-patterns.md answered "there is no built-in concurrency cap" with "so declare one before fanning out widely". A guessed ceiling does not make a run safer: it stops a healthy run partway with factory_limit_reached, after that run has already taken the user's approval and spent credits, and the caller then raises the ceiling a step at a time. Drop limits from the opening sample and document the declaration syntax in the Resource limits section instead, where the rule sits next to it. State that a ceiling comes from real knowledge of the cost or from the user, and that omitting limits does not remove oversight, since a run still needs an approval whose prompt shows the effective limits. Redirect the Scaling remedy from declaring a cap to bounding the fan-out with the factory's own counters. The API documentation is unchanged: every limit keeps its own description, and limits stay declarable for an author who does know the cost profile. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(factories): fix limit fallback and approval claims in Resource limits Address review feedback on the Resource limits section: - "an omitted limit leaves that dimension unbounded" contradicted the documented `maxConcurrentSubagents` -> `maxTotalSubagents` fallback in factory-patterns.md. Note the exception so a declared total cap is not read as leaving concurrency unbounded. - "the run still needs an approval" is only true for the model-initiated `run_factory` path. SDK-initiated `run` and `resume` do not request permission (src/factory.ts:292, and the Run and resume section says the same), so qualify the claim instead of promising SDK callers a safeguard that does not exist. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: examon <examon@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 4472fcb commit eb7ba24

3 files changed

Lines changed: 37 additions & 8 deletions

File tree

nodejs/docs/factories.md

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,6 @@ const reviewChanged = defineFactory({
2323
files: { type: "array", items: { type: "string" } },
2424
},
2525
},
26-
limits: {
27-
maxConcurrentSubagents: 3,
28-
maxTotalSubagents: 10,
29-
timeoutSeconds: 90.5,
30-
maxAiCredits: 5,
31-
},
3226
},
3327
run: async (ctx) => {
3428
ctx.phase("Review");
@@ -121,7 +115,14 @@ See [factory-patterns.md](./factory-patterns.md) for composable orchestration pa
121115

122116
## Resource limits
123117

124-
Limits may be declared in `meta.limits` and overridden per invocation. All limits must be positive when present.
118+
Limits may be declared in `meta.limits` and overridden per invocation. Every limit is optional and must be positive when present; an omitted limit leaves that dimension unbounded, except that an omitted `maxConcurrentSubagents` falls back to `maxTotalSubagents`, so a declared total cap also bounds concurrency.
119+
120+
Set a ceiling only from real knowledge of what the factory costs, or because the user named one. A guessed ceiling does not make a run safer: it stops a healthy run partway with `factory_limit_reached`, after that run has already spent credits. An agent authoring or invoking a factory on the user's behalf has no basis for estimating a number, so it should leave `limits` unset and bound the work with the factory's own counters instead. Omitting limits does not remove oversight of a model-initiated run: `run_factory` requests permission first, and that prompt shows the effective limits. SDK-initiated `run` and `resume` do not request permission, so an SDK caller that wants a ceiling sets it deliberately, from the cost it already knows.
121+
122+
```js
123+
// Only when the cost profile is known, or the user asked for this ceiling.
124+
limits: { maxTotalSubagents: 10 },
125+
```
125126

126127
- `maxConcurrentSubagents`: Positive integer concurrent-subagent cap. Additional subagents wait in a queue. Queueing applies backpressure and does not fail the run.
127128
- `maxTotalSubagents`: Positive integer cumulative admission cap. An attempted subagent beyond the cap ends the attempt with failure kind `maxTotalSubagents`.

nodejs/docs/factory-patterns.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,6 @@ Compose these freely.
189189
190190
Match the orchestration to what was asked. A quick check wants a couple of subagents and single-vote verification; a request to be thorough or comprehensive wants a larger finder pool, a three-to-five vote adversarial pass, and a synthesis stage.
191191
192-
There is no in-script budget object. Scale with your own counters, as in the loop patterns above, and treat the declared limits as the safety ceiling rather than the control mechanism. Only `agent()` spawns are throttled, by `maxConcurrentSubagents` falling back to `maxTotalSubagents`; with neither declared there is no built-in concurrency cap, so declare one before fanning out widely. `parallel` itself is `Promise.all`, so non-agent work in a thunk runs fully concurrently regardless.
192+
There is no in-script budget object. Scale with your own counters, as in the loop patterns above, and treat any declared limits as the safety ceiling rather than the control mechanism. Only `agent()` spawns are throttled, by `maxConcurrentSubagents` falling back to `maxTotalSubagents`; with neither declared there is no built-in concurrency cap, so bound a wide fan-out with the factory's own counters. Do not invent a ceiling to compensate, and see [Resource limits](./factories.md#resource-limits) for when declaring one is appropriate. `parallel` itself is `Promise.all`, so non-agent work in a thunk runs fully concurrently regardless.
193193
194194
These patterns are not exhaustive. Compose novel harnesses — tournament brackets, self-repair loops, staged escalation — when the task calls for it.

nodejs/test/factory.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,34 @@ describe("factories", () => {
403403
expect(generatedRpc).toContain("timeoutSeconds?: number;");
404404
});
405405

406+
// A guessed ceiling does not make a run safer: it stops a healthy run partway
407+
// with `factory_limit_reached`, after that run has already taken the user's
408+
// approval and spent credits. Both documents are handed to the model verbatim
409+
// by the `factories_manage` guide, so neither may read as an invitation to
410+
// invent one.
411+
it("documents limits as opt-in rather than inviting an invented ceiling", () => {
412+
const guide = readFileSync(new URL("../docs/factories.md", import.meta.url), "utf8");
413+
const patterns = readFileSync(
414+
new URL("../docs/factory-patterns.md", import.meta.url),
415+
"utf8"
416+
);
417+
418+
expect(guide).toContain(
419+
"Set a ceiling only from real knowledge of what the factory costs, or because the user named one"
420+
);
421+
expect(guide).toContain("no basis for estimating a number");
422+
423+
// The opening `defineFactory` sample is the shape an author copies. Filling
424+
// all four ceilings in there taught the numbers as much as the syntax.
425+
const openingSample = guide.slice(0, guide.indexOf("## Declaring an argument shape"));
426+
expect(openingSample).not.toContain("limits: {");
427+
428+
// The Scaling section used to answer "there is no built-in concurrency cap"
429+
// with "so declare one before fanning out widely".
430+
expect(patterns).not.toContain("declare one before fanning out widely");
431+
expect(patterns).toContain("bound a wide fan-out with the factory's own counters");
432+
});
433+
406434
it("documents factory invocation and list paging behavior accurately", () => {
407435
const guide = readFileSync(new URL("../docs/factories.md", import.meta.url), "utf8");
408436
const publicApi = readFileSync(new URL("../src/factory.ts", import.meta.url), "utf8");

0 commit comments

Comments
 (0)