test(e2e): resolve dynamic task_id in continue_subagent spec via mock templating (#4517)#4701
Conversation
… templating (tinyhumansai#4517) Follow-up to tinyhumansai#4671 for the last chat tool-round E2E spec tracked by tinyhumansai#4517: `chat-harness-subagent-continue.spec.ts` covers the full orchestrator -> researcher clarification -> resume loop and was still failing after the sibling `chat-harness-subagent` conversion. Two issues on that spec: 1. The `llmForcedResponses` FIFO desyncs. Same failure mode as tinyhumansai#4671: the researcher runs its own agent-harness loop, so any ancillary call (title-gen, summariser, delegation-side model call) pops a scripted response and the scripted canary never renders. 2. The forced `continue_subagent` tool_call carried a literal `{{DYNAMIC_TASK_ID}}` placeholder. The real `task_id: sub-<uuid>` is generated at delegate time in src/openhuman/agent_orchestration/tools/dispatch.rs and is only observable to the orchestrator via the `[SUBAGENT_AWAITING_USER]` envelope in the message history. A keyword-rule swap alone leaves this broken; the sub-agent dispatch fails with "no checkpoint found for task_id '{{DYNAMIC_TASK_ID}}'". Fix: - `scripts/mock-api/routes/llm/shared.mjs`: add `renderDynamicPlaceholders(text, parsedBody)` that scans messages newest-first for the last `[SUBAGENT_AWAITING_USER]` block, extracts `task_id` / `agent_id` / `worker_thread_id`, and substitutes `{{DYNAMIC_TASK_ID}}` / `{{DYNAMIC_AGENT_ID}}` / `{{DYNAMIC_WORKER_THREAD_ID}}` in the response. Identity fast-path when text has no placeholders or no envelope is in history. `applyDynamicPlaceholdersToResponse` wraps it for `content` + `toolCalls[*].arguments` and never mutates the input (call-sites re-read the rule on every request). - `scripts/mock-api/routes/llm.mjs`: apply the wrapper at every script-entry site -- streaming forced-queue, streaming keyword rule, streaming requestRule fallback, non-streaming requestRule, non-streaming forced-queue, non-streaming keyword rule. No behaviour change when text has no placeholders. - `app/test/e2e/specs/chat-harness-subagent-continue.spec.ts`: swap `llmForcedResponses` for six `llmKeywordRules` keyed on disjoint tokens (`chromatophore-request`, `harlequin-token`, `citrine-cliff`, `wisteria-tag`, `zenith-beacon`, and the `user's answer to your clarification question` framing that continue_subagent.rs:193 prepends to the resumed sub-agent's history). Documented ordering constraint: the researcher-resumed rule must precede the orchestrator-continue rule because the researcher's probe contains both the framing AND the raw user answer text. - Six new node:test cases in llm.test.mjs exercise the helper (substitution, no-envelope no-op, identity fast-path, immutability regression across successive requests, end-to-end keyword-rule substitution of `continue_subagent` args). No product/runtime change. All 42 mock route tests pass locally (`node --test scripts/mock-api/routes/__tests__/*.test.mjs`).
📝 WalkthroughWalkthroughThis PR adds dynamic placeholder substitution ( ChangesMock LLM dynamic placeholders and keyword-rule e2e migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/mock-api/routes/llm.mjs (1)
605-638: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRender the streaming rule before recording history.
Whenstream: trueand a rule suppliescontent/toolCalls, this branch stores the raw placeholder values in thread history, whilehandleStreamingCompletion()renders the SSE payload later. That leavesthread.lastAssistantContent/lastToolCallsout of sync with what the client saw and can skew follow-up turns; match the non-streaming branch by recording the rendered rule instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mock-api/routes/llm.mjs` around lines 605 - 638, The streaming path in llm.mjs records placeholder rule data before the SSE response is rendered, so thread history can diverge from what the client actually received. Update the stream branch around recordTurn and handleStreamingCompletion so that when requestRule has content/toolCalls/streamScript, you first render the rule using the same logic as the non-streaming path and then call recordMockLlmTurn with the rendered content and toolCalls, keeping thread.lastAssistantContent and lastToolCalls in sync.
🧹 Nitpick comments (2)
scripts/mock-api/routes/__tests__/llm.test.mjs (1)
408-538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the streaming forced-queue/keyword-rule placeholder wiring.
All six new tests exercise only the non-streaming code path (no
stream: truein anyparsedBody, responses read synchronously fromctx.res.body). But this PR also wiresapplyDynamicPlaceholdersToResponseinto the streaming forced-queue and keyword-rule branches inllm.mjs(Lines 301-304, 318-321), which remain unverified by any test. The file already has a pattern for streaming assertions (see"streams request-rule scripts for root chat completions path"), so extending thecontinue_subagentintegration test (Lines 440-491) to astream: truevariant would be straightforward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mock-api/routes/__tests__/llm.test.mjs` around lines 408 - 538, The new placeholder coverage only exercises the synchronous completion path, so the streaming forced-queue and keyword-rule wiring in llm.mjs remains untested. Extend the existing continue_subagent integration coverage in applyDynamicPlaceholdersToResponse / llmKeywordRules to include a stream: true variant, and assert the streamed chunks still render DYNAMIC_TASK_ID and DYNAMIC_AGENT_ID correctly without leaving unresolved placeholders. Reuse the existing streaming test pattern already present in this suite so the branch that feeds forced-queue and keyword-rule responses through applyDynamicPlaceholdersToResponse is verified end to end.app/test/e2e/specs/chat-harness-subagent-continue.spec.ts (1)
69-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUndocumented keyword collision between the "zenith-beacon" and "wisteria-tag" rules.
RESEARCHER_FINAL_REPLY("In the wisteria-tag project I found the zenith-beacon marker.") contains both keywords. The final-synthesis probe (the tool result carrying this reply) will matchzenith-beacononly because that rule happens to be listed beforewisteria-tagin the array — the same first-match-wins mechanism the "ORDER MATTERS" comment (lines 72-77) already calls out for the rule1/rule3 pair. This second ordering dependency isn't mentioned, so a future reorder (e.g., alphabetizing, or adding rules) could silently misroute this turn.Consider extending the existing comment to document this second constraint, or reword
RESEARCHER_FINAL_REPLYso it doesn't embed thewisteria-tagtoken unnecessarily.📝 Example comment addition
// ORDER MATTERS: the researcher's resumed probe includes both the framing // `[User's answer to your clarification question]` prepended by // `src/openhuman/agent_orchestration/tools/continue_subagent.rs` AND the raw // user answer text (`wisteria-tag`). Its rule must appear BEFORE the // orchestrator-continue rule (keyed on `wisteria-tag`) so the researcher's // resumed call hits the researcher rule first. +// +// ALSO NOTE: `RESEARCHER_FINAL_REPLY` embeds `wisteria-tag` (it echoes the +// project name) in addition to `zenith-beacon`, so the `zenith-beacon` rule +// must also stay ordered before the `wisteria-tag` rule to win on the final +// synthesis turn. const KEYWORD_RULES = [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/test/e2e/specs/chat-harness-subagent-continue.spec.ts` around lines 69 - 116, The KEYWORD_RULES array has an undocumented first-match collision: RESEARCHER_FINAL_REPLY contains both "zenith-beacon" and "wisteria-tag", so the final-synthesis path in KEYWORD_RULES depends on the current ordering of the "zenith-beacon" and "wisteria-tag" entries. Fix this by either updating the existing ORDER MATTERS comment in chat-harness-subagent-continue.spec.ts to explicitly document this second ordering constraint, or by changing RESEARCHER_FINAL_REPLY so it no longer includes the unnecessary "wisteria-tag" token. Keep the behavior of the researcher/orchestrator probe matching intact by preserving the intended unique symbols used by the rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mock-api/routes/llm/shared.mjs`:
- Around line 160-181: applyDynamicPlaceholdersToResponse currently only
substitutes placeholders in toolCalls[*].arguments when arguments is a string,
so object-form arguments are left untouched. Update the logic in
applyDynamicPlaceholdersToResponse to also walk and render placeholders inside
plain object arguments (while preserving the shallow-copy, non-mutating
behavior), and make sure tool call entries still pass through unchanged when
they have no placeholders. Add a test near the existing render coverage in
llm.test.mjs for the documented object-literal arguments case (for example
continue_subagent with a placeholder task_id) to verify substitution happens.
---
Outside diff comments:
In `@scripts/mock-api/routes/llm.mjs`:
- Around line 605-638: The streaming path in llm.mjs records placeholder rule
data before the SSE response is rendered, so thread history can diverge from
what the client actually received. Update the stream branch around recordTurn
and handleStreamingCompletion so that when requestRule has
content/toolCalls/streamScript, you first render the rule using the same logic
as the non-streaming path and then call recordMockLlmTurn with the rendered
content and toolCalls, keeping thread.lastAssistantContent and lastToolCalls in
sync.
---
Nitpick comments:
In `@app/test/e2e/specs/chat-harness-subagent-continue.spec.ts`:
- Around line 69-116: The KEYWORD_RULES array has an undocumented first-match
collision: RESEARCHER_FINAL_REPLY contains both "zenith-beacon" and
"wisteria-tag", so the final-synthesis path in KEYWORD_RULES depends on the
current ordering of the "zenith-beacon" and "wisteria-tag" entries. Fix this by
either updating the existing ORDER MATTERS comment in
chat-harness-subagent-continue.spec.ts to explicitly document this second
ordering constraint, or by changing RESEARCHER_FINAL_REPLY so it no longer
includes the unnecessary "wisteria-tag" token. Keep the behavior of the
researcher/orchestrator probe matching intact by preserving the intended unique
symbols used by the rules.
In `@scripts/mock-api/routes/__tests__/llm.test.mjs`:
- Around line 408-538: The new placeholder coverage only exercises the
synchronous completion path, so the streaming forced-queue and keyword-rule
wiring in llm.mjs remains untested. Extend the existing continue_subagent
integration coverage in applyDynamicPlaceholdersToResponse / llmKeywordRules to
include a stream: true variant, and assert the streamed chunks still render
DYNAMIC_TASK_ID and DYNAMIC_AGENT_ID correctly without leaving unresolved
placeholders. Reuse the existing streaming test pattern already present in this
suite so the branch that feeds forced-queue and keyword-rule responses through
applyDynamicPlaceholdersToResponse is verified end to end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 265468af-cd31-4ec7-8a75-fde6f61298e6
📒 Files selected for processing (4)
app/test/e2e/specs/chat-harness-subagent-continue.spec.tsscripts/mock-api/routes/__tests__/llm.test.mjsscripts/mock-api/routes/llm.mjsscripts/mock-api/routes/llm/shared.mjs
| // Apply `renderDynamicPlaceholders` to a rule / forced-response's `content` and | ||
| // each `toolCalls[*].arguments` (when they're stringified JSON, which the mock | ||
| // forwards verbatim to the agent harness). No-op for entries that don't carry | ||
| // placeholders. Returns a shallow copy — never mutates the input. | ||
| export function applyDynamicPlaceholdersToResponse(response, parsedBody) { | ||
| if (!response || typeof response !== "object") return response; | ||
| const rendered = { ...response }; | ||
| if (typeof rendered.content === "string") { | ||
| rendered.content = renderDynamicPlaceholders(rendered.content, parsedBody); | ||
| } | ||
| if (Array.isArray(rendered.toolCalls)) { | ||
| rendered.toolCalls = rendered.toolCalls.map((tc) => { | ||
| if (!tc || typeof tc !== "object") return tc; | ||
| const next = { ...tc }; | ||
| if (typeof next.arguments === "string") { | ||
| next.arguments = renderDynamicPlaceholders(next.arguments, parsedBody); | ||
| } | ||
| return next; | ||
| }); | ||
| } | ||
| return rendered; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Object-form toolCalls[*].arguments silently skip placeholder substitution.
applyDynamicPlaceholdersToResponse only renders next.arguments when typeof next.arguments === "string" (Line 174). But llm.mjs's own keyword-rule JSDoc documents arguments as a plain object literal (e.g. "arguments": {"q": "rust"}), and after parseBehaviorJson parses the rule JSON, a config author who follows that documented convention for continue_subagent (e.g. arguments: { task_id: "{{DYNAMIC_TASK_ID}}" }) will get an untouched {{DYNAMIC_TASK_ID}} literal forwarded to the harness — the whole point of this PR's feature silently fails for that (documented) usage pattern.
🛠️ Proposed fix to also handle object-form arguments
if (typeof next.arguments === "string") {
next.arguments = renderDynamicPlaceholders(next.arguments, parsedBody);
+ } else if (next.arguments && typeof next.arguments === "object") {
+ const renderedArgs = renderDynamicPlaceholders(
+ JSON.stringify(next.arguments),
+ parsedBody,
+ );
+ try {
+ next.arguments = JSON.parse(renderedArgs);
+ } catch {
+ // leave as-is if somehow not valid JSON after substitution
+ }
}Consider adding a test alongside the existing "renders content and toolCalls arguments" case in llm.test.mjs covering object-form arguments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Apply `renderDynamicPlaceholders` to a rule / forced-response's `content` and | |
| // each `toolCalls[*].arguments` (when they're stringified JSON, which the mock | |
| // forwards verbatim to the agent harness). No-op for entries that don't carry | |
| // placeholders. Returns a shallow copy — never mutates the input. | |
| export function applyDynamicPlaceholdersToResponse(response, parsedBody) { | |
| if (!response || typeof response !== "object") return response; | |
| const rendered = { ...response }; | |
| if (typeof rendered.content === "string") { | |
| rendered.content = renderDynamicPlaceholders(rendered.content, parsedBody); | |
| } | |
| if (Array.isArray(rendered.toolCalls)) { | |
| rendered.toolCalls = rendered.toolCalls.map((tc) => { | |
| if (!tc || typeof tc !== "object") return tc; | |
| const next = { ...tc }; | |
| if (typeof next.arguments === "string") { | |
| next.arguments = renderDynamicPlaceholders(next.arguments, parsedBody); | |
| } | |
| return next; | |
| }); | |
| } | |
| return rendered; | |
| } | |
| // Apply `renderDynamicPlaceholders` to a rule / forced-response's `content` and | |
| // each `toolCalls[*].arguments` (when they're stringified JSON, which the mock | |
| // forwards verbatim to the agent harness). No-op for entries that don't carry | |
| // placeholders. Returns a shallow copy — never mutates the input. | |
| export function applyDynamicPlaceholdersToResponse(response, parsedBody) { | |
| if (!response || typeof response !== "object") return response; | |
| const rendered = { ...response }; | |
| if (typeof rendered.content === "string") { | |
| rendered.content = renderDynamicPlaceholders(rendered.content, parsedBody); | |
| } | |
| if (Array.isArray(rendered.toolCalls)) { | |
| rendered.toolCalls = rendered.toolCalls.map((tc) => { | |
| if (!tc || typeof tc !== "object") return tc; | |
| const next = { ...tc }; | |
| if (typeof next.arguments === "string") { | |
| next.arguments = renderDynamicPlaceholders(next.arguments, parsedBody); | |
| } else if (next.arguments && typeof next.arguments === "object") { | |
| const renderedArgs = renderDynamicPlaceholders( | |
| JSON.stringify(next.arguments), | |
| parsedBody, | |
| ); | |
| try { | |
| next.arguments = JSON.parse(renderedArgs); | |
| } catch { | |
| // leave as-is if somehow not valid JSON after substitution | |
| } | |
| } | |
| return next; | |
| }); | |
| } | |
| return rendered; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/mock-api/routes/llm/shared.mjs` around lines 160 - 181,
applyDynamicPlaceholdersToResponse currently only substitutes placeholders in
toolCalls[*].arguments when arguments is a string, so object-form arguments are
left untouched. Update the logic in applyDynamicPlaceholdersToResponse to also
walk and render placeholders inside plain object arguments (while preserving the
shallow-copy, non-mutating behavior), and make sure tool call entries still pass
through unchanged when they have no placeholders. Add a test near the existing
render coverage in llm.test.mjs for the documented object-literal arguments case
(for example continue_subagent with a placeholder task_id) to verify
substitution happens.
Summary
chat-harness-subagent-continue.spec.ts(orchestrator → researcher clarification → resume → final canary).{{DYNAMIC_*}}template substitution primitive so keyword-rule fixtures can reference runtime-generatedtask_id/agent_idextracted from the[SUBAGENT_AWAITING_USER]envelope in message history.llmForcedResponsesFIFO tollmKeywordRules(content-addressed, never depleted) — same pattern as the sibling PR test(e2e): convert chat-harness-subagent to llmKeywordRules (#4517) #4671.node:testcases cover the helper + end-to-end substitution + immutability regression.Problem
Two coupled failures on
chat-harness-subagent-continue.spec.ts:FIFO desync. The delegated researcher runs its own agent-harness loop; any ancillary call (title-gen from
threadSlice.ts, summariser, delegation-side model call) drains a scripted response and the scripted canary lands on the wrong turn or never renders. Same failure mode test(e2e): stop ancillary LLM calls draining the mock forced-response queue (#4517) #4519 mitigated globally and test(e2e): convert chat-harness-subagent to llmKeywordRules (#4517) #4671 fixed for the siblingchat-harness-subagentspec.Literal placeholder in scripted
continue_subagentargs. The spec's forced response Refactor testing scripts in package.json and update dependencies #4 declaredarguments: JSON.stringify({ task_id: '{{DYNAMIC_TASK_ID}}', agent_id: 'researcher', message: USER_ANSWER }). The realtask_id: sub-<uuid>is generated at delegate time insrc/openhuman/agent_orchestration/tools/dispatch.rs:97and only surfaces to the orchestrator inside the[SUBAGENT_AWAITING_USER]envelope (awaiting_user.rs:41). A keyword-rule swap alone leaves the placeholder literal;continue_subagentfails withno checkpoint found for task_id '{{DYNAMIC_TASK_ID}}'and the sub-agent never resumes.Follow-up called out in PR #4671's body:
Solution
Chose the message-history parse route — it's a small, reusable primitive.
scripts/mock-api/routes/llm/shared.mjsrenderDynamicPlaceholders(text, parsedBody)walksparsedBody.messagesnewest-first, finds the last[SUBAGENT_AWAITING_USER]…[/SUBAGENT_AWAITING_USER]block, parsestask_id:/agent_id:/worker_thread_id:lines, and substitutes{{DYNAMIC_TASK_ID}}/{{DYNAMIC_AGENT_ID}}/{{DYNAMIC_WORKER_THREAD_ID}}. Identity fast-path when text has no placeholders or no envelope in history.applyDynamicPlaceholdersToResponse(response, parsedBody)shallow-copies a rule/forced-response and renders bothcontentand eachtoolCalls[*].arguments. Never mutates the input (call-sites re-read the stored rule on every request; mutation would let a first substitution poison later turns without an envelope — verified by a regression test).scripts/mock-api/routes/llm.mjsrequestRulefallback, non-streamingrequestRule, non-streaming forced-queue, non-streaming keyword rule.app/test/e2e/specs/chat-harness-subagent-continue.spec.tsllmForcedResponses→ sixllmKeywordRuleskeyed on disjoint distinctive tokens carried by the six probes:pickProbeTextresult)chromatophore-requestTask:\n…, fromarchetype_delegation.rs::render_structured_handoff)harlequin-tokenrole: tool=[SUBAGENT_AWAITING_USER]envelopecitrine-cliffwisteria-tag[User's answer to your clarification question]\n<message>(prepended incontinue_subagent.rs:193)user's answer to your clarification questionrole: tool= researcher's resumed replyzenith-beaconwisteria-taguser-answer token as well as the framing. Other rules have disjoint keywords and can be ordered freely.{{DYNAMIC_TASK_ID}}/{{DYNAMIC_AGENT_ID}}, substituted at mock-time from the preceding envelope.scripts/mock-api/routes/__tests__/llm.test.mjs— six newnode:testcases:renderDynamicPlaceholderssubstitutes task/agent IDs from history.applyDynamicPlaceholdersToResponserenderscontent+toolCalls[*].argumentswithout mutating input.llmKeywordRulessubstitute{{DYNAMIC_TASK_ID}}incontinue_subagenttool_call args.{{DYNAMIC_TASK_ID}}unmutated (identity fast-path). Guards against a subtle mutation bug the wrapper's purity prevents.Submission Checklist
node:testcases inscripts/mock-api/routes/__tests__/llm.test.mjscover the helper's happy paths + immutability regression + end-to-end substitution. The E2E spec is itself the failure-path fixture (unresolved placeholder → dispatch error was the pre-fix failure).scripts/mock-api/andapp/test/e2e/, no productsrclines under coverage.## Related— N/A: no feature IDs affected.Closes #NNNin the## Relatedsection — see## Relatedbelow.Impact
chat-harness-subagent-continue.spec.tsunder the Release CI desktop chat shard. No runtime, product, or platform behaviour change. TherenderDynamicPlaceholdersprimitive is available for future specs that need to reference other runtime-generated IDs surfaced through structured envelopes.Related
chat-harness-subagentconverted tollmKeywordRules).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/4517-continue-spec-templating57f1df0d1Validation Run
pnpm --filter openhuman-app format:check—prettier --check test/e2e/specs/chat-harness-subagent-continue.spec.ts✓ (whole-app run OOM'd in this session; targeted check is definitive for the only app file touched)pnpm typecheck—tsc -p app/tsconfig.json --noEmit✓ exit 0node --test scripts/mock-api/routes/__tests__/*.test.mjs→ 42/42 pass (17/17 inllm.test.mjsincluding 6 new)Validation Blocked
command:pnpm debug e2e app/test/e2e/specs/chat-harness-subagent-continue.spec.tserror:Full WDIO/CEF chain needs a built Tauri bundle +tauri-driver(Linux) or Appium XCUITest (macOS); not runnable in this session's environment.impact:Runtime side of the fix (real agent harness resuming a paused sub-agent) will be exercised by Release CI on this PR; the in-processnode:testcases prove the mock's routing + substitution side.Behavior Changes
{{DYNAMIC_*}}substitution.Parity Contract
llmForcedResponses/llmKeywordRules/llmRequestRulesbehaviour is unchanged for every other spec. The wrapper is a pure function (verified by the immutability regression test), so successive requests see the same stored rule.Duplicate / Superseded PR Handling