Repository navigation
test(orchestration): backfill the tool-loop suite and raise its floors - #283
Conversation
The loop's helpers were tested one in six, every loop case drove one tool call per round, and the gates that end a turn were never observed from outside. Fourteen tests now cover all six helpers with the whole truth table of shouldContinueToolLoop; a two-call round running both executors and pairing two tool_use with two tool_result items in order; one suppressing executor among two yielding one suppress_output after the tool_result; two upserts taking sequential indices and landing in the session; a round returning no client messages yielding no tool_result; no pending call leaking across three rounds; and the three gates: no registry, end_turn beside a tool call, a stream with no stop. The mock-driven cases live at the root, where the layering rule lets a test reach both the orchestrator and the exported mock. The orchestration, tools and global coverage floors ratchet to the measured numbers. Closes #80. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Lore is reviewing this PR — 2b23ab64-e382-4286-8aa0-4c9b27073472. Submit a review that requests changes to have Lore push a fix for it. Comment |
🔍 Lore Spec Impact — advisoryThis PR touches 43 statement(s) across 5 spec(s), and changes the validating tests alongside every one of them. Architecture · 5 statement(s)
Whether a round continues
validated by Whether a round continues
validated by Whether a round continues
validated by Whether a round continues
validated by Across rounds
validated by Weaker signals (38) — linked by a spec, not proven by a test runArchitecture · 26 statement(s)
OrchestratorHooks
validated by OrchestratorHooks
validated by OrchestratorHooks
validated by OrchestratorHooks
validated by OrchestratorHooks
validated by OrchestratorHooks
validated by OrchestratorHooks
validated by OrchestratorHooks
validated by OrchestratorHooks
validated by OrchestratorHooks
validated by …and 28 more statement(s). 18 new statement(s) have no test link yet. Deterministic · graph @ |
![lore-agent[bot]](https://avatars.githubusercontent.com/u/141229953?s=60&v=4){kind=small}
There was a problem hiding this comment.
Lore review — Changes suggested
gatedTurn registers first/second while TOOL_CALL invokes lookup, and src/orchestration/toolLoopTestSupport.ts fails check-changelog.sh in CI and inflates src/orchestration/ coverage thresholds
Must fix (2) · Consider (0) · Nits (0)
Reviewed by gemini-4-argon
Submit a review that requests changes to have Lore push a fix for it. Comment @lore review to re-run the review.
Posted by floor, visit 98456201-e727-4c5b-bb1d-7430536ea06f.
…upport out of src gatedTurn registered first and second while TOOL_CALL names lookup, so its "zero executions" was unobservable; it now registers lookup with a recording executor. The two-tool helper lives in its only consumer, the root-level suite, so no non-test file sits under src/ for the changelog check or the orchestration coverage floor to see; that floor is re-measured without it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@lore review |
|
Lore is reviewing this PR — 75a43a6d-487c-4179-b72d-9a1e6438dfd6. Submit a review that requests changes to have Lore push a fix for it. Comment |
There was a problem hiding this comment.
Lore review — Approved
No issues found.
Must fix (0) · Consider (0) · Nits (0)
Reviewed by gemini-4-argon
Submit a review that requests changes to have Lore push a fix for it. Comment @lore review to re-run the review.
Posted by floor, visit 36e7f14b-c055-4f6e-a492-c4907df64ff2.
Why
The tool loop is the engine's central feature and its suite was uneven:
orchestratorHelpers.test.tscovered one of six exported helpers, every loop case drove exactly one tool call per round, and the loop's negative gates (no registry, anend_turnstop beside tool calls, a stream with nostopchunk) were never observed from outside. Closes #80.What Changed
Test-only, plus the coverage floors; no
src/change outside*.test.ts.src/orchestration/orchestratorHelpers.test.ts: all six exports, with the eight-row truth table ofshouldContinueToolLoopin one assertion.src/toolLoop.test.ts(root level, see below): the cases the package's exportedcreateMockProvidercan drive. A two-call round runs both executors and hands the next round one assistant message with twotool_useitems and one user message with two matchingtool_resultitems, in script order; one suppressing executor among two yields exactly onesuppress_outputafter thetool_result; twoentry_upsertmessages take sequential indices from the session length at call time and are readable from the session afterwards; a round returning no client messages yields notool_result; three rounds leave two assistant messages with onetool_useeach, so no pending call leaks between rounds; a scriptedtool_useturn with no registry ends after one provider call.src/orchestration/chatOrchestrator.test.ts: the two gates the exported mock cannot express, because it derives the stop reason from whether a turn called a tool: anend_turnstop beside a tool call, and a stream ending without astopchunk. Both reuse the file's existingscriptedProvider(a second scripted round that must never stream proves no second provider call), adding no new double, and their registry holds the toolTOOL_CALLnames,lookup, with a recording executor, so "zero executions" is observed rather than assumed.specs/hal-engine-architecture/spec.md, § Tool Execution Loop: a### A round with more than one callsubsection and extra gating and helper statements, fourteen in all, each citing its test; the line shifts inchatOrchestrator.test.tsrepointed 33 existing anchors.jest.config.js: thesrc/orchestration/,src/orchestration/tools/andglobalfloors raised to the re-measured numbers.src/orchestration/is measured over production files alone (97.02 / 93.42 / 97.72 / 97.14). One thing worth knowing for the next ratchet: Jest holdsglobalto the files no path floor claims, not to the "All files" row, so its numbers (95.05 / 90.7 / 93.75 / 95.74) are lower than that row; a comment on the line now says so.Where this departs from #80's text, and why
chatOrchestrator.test.ts.layers.yaml(added after the issue) forbidsorchestrationfrom importingproviders, so a test there cannot construct the exportedcreateMockProviderthe issue requires. The mock-driven cases live in a root-level file, whereconfig.test.tsalready composes both layers, with the two-tool registry helper local to it; the twoscriptedProvidercases stay in place.scriptedProvider, not the exported mock.MockTurnhas no stop-reason field; it emitstool_usewhen a turn has calls andend_turnotherwise, and always emits astop. The issue's "no further hand-rolled double" is kept: nothing new was written, the file's existing double is reused.Nothing here asserts the round ceiling, the throwing-executor path or
ToolContext's field list, so the three sibling issues merge in any order.Alternatives Considered
chatOrchestrator.test.tswithscriptedProvideronly: possible, but the issue asks for the exported mock on purpose, and the root-level file honours both that and the layering rule.ADRs & Architecture
No architectural change. The tests pin
specs/hal-engine-architecture/spec.md§ Tool Execution Loop; the layering they respect islayers.yaml.Testing
Fourteen new tests, each cited from the spec; the touched suites pass,
scripts/check-changelog.sh origin/mainpasses (no non-test file undersrc/changed), andnpm run verifypasses with the raised floors. The second commit answers the review:gatedTurnnow observes the tool it names, and the helper that briefly lived undersrc/orchestration/as a non-test file is folded into its only consumer, the floors re-measured without it.Code Quality Checklist
npm run eslintpasses with no warningsnpm run typecheckpasses (strict mode)npm testpasses (new tests added for new code)npm run prettier:checkornpm run prettier)specs/,adrs/ordocs/logfromshared/logger.jsinsteadCHANGELOG.mdentry under## [Unreleased], or the PR carries theno-changeloglabelRelated Issues
Closes #80.
🤖 Generated with Claude Code