Skip to content

fix(orchestrator): move system context to cacheable systemPrompt.append (fixes #1591)#1634

Merged
Wirasm merged 4 commits into
coleam00:devfrom
kagura-agent:fix/prompt-caching-orchestrator
May 13, 2026
Merged

fix(orchestrator): move system context to cacheable systemPrompt.append (fixes #1591)#1634
Wirasm merged 4 commits into
coleam00:devfrom
kagura-agent:fix/prompt-caching-orchestrator

Conversation

@kagura-agent

@kagura-agent kagura-agent commented May 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Problem: Prompt caching is broken for orchestrator calls — the static orchestrator system context (project list, workflows, routing rules) was embedded in the prompt parameter, which changes every turn, invalidating the Anthropic API cache prefix on every request.
  • Why it matters: Every orchestrator turn pays full cache_creation_input_tokens cost instead of hitting cache_read_input_tokens, resulting in high TTFT (Time To First Token) and wasted API spend.
  • What changed: Moved the static orchestrator context from prompt into systemPrompt: { type: "preset", preset: "claude_code", append: ... }, which the Claude Code SDK includes in the cacheable system prompt prefix. The prompt parameter now contains only per-turn dynamic content (workflow results, thread context, user message, issue context, files).
  • What did NOT change: The actual content of the orchestrator prompt is identical — same project list, same routing rules, same workflow section. Only where it lives in the request structure changed.

UX Journey

Before

User                   Archon                        Anthropic API
────                   ──────                        ─────────────
sends message ──────▶  buildFullPrompt():
                         systemPrompt (static) +
                         workflowContext (dynamic) +
                         userMessage (dynamic)
                       ───── all in prompt ─────────▶ cache miss (prompt changed)
                                                      cache_creation: HIGH
                                                      cache_read: 0
                                                      TTFT: HIGH

After

User                   Archon                        Anthropic API
────                   ──────                        ─────────────
sends message ──────▶  systemPrompt.append:
                         orchestrator context (static)
                       prompt:
                         workflowContext + user msg
                       ── split correctly ──────────▶ cache HIT on system prompt
                                                      cache_creation: LOW
                                                      cache_read: HIGH
                                                      TTFT: LOW

Architecture Diagram

Before

handleMessage()
  └─▶ buildFullPrompt(conversation, codebases, workflows, message, ...)
        ├── buildOrchestratorPrompt() / buildProjectScopedPrompt()  ← static
        ├── workflowContext                                          ← dynamic
        └── userMessage + issueContext + files                       ← dynamic
        → returns single combined string as `prompt`
  └─▶ sendQuery({ prompt: fullPrompt, options: { systemPrompt: preset } })

After

handleMessage()
  └─▶ buildFullPrompt(message, ...)                                  [~] simplified
        ├── workflowContext                                          ← dynamic
        └── userMessage + issueContext + files                       ← dynamic
        → returns user-facing content only as `prompt`
  └─▶ [+] requestOptions.systemPrompt = { preset + append: buildOrchestratorSystemAppend() }
  └─▶ sendQuery({ prompt: fullPrompt, options: requestOptions })

Connection inventory:

From To Status Notes
handleMessage buildFullPrompt modified Removed system context params
handleMessage buildOrchestratorSystemAppend new Static context → systemPrompt.append
handleMessage sendQuery unchanged requestOptions now carries systemPrompt
prompt-builder orchestrator-agent modified New export: buildOrchestratorSystemAppend

Label Snapshot

  • Risk: risk: low
  • Size: size: S
  • Scope: core
  • Module: core:orchestrator

Change Metadata

  • Change type: bug
  • Primary scope: core

Linked Issue

Validation Evidence (required)

$ bun run type-check
# All 10 packages passed ✅

$ bun test packages/core/src/orchestrator/prompt-builder.test.ts
# 12 pass, 0 fail ✅

$ bun test packages/core/src/orchestrator/orchestrator-agent.test.ts
# 100 pass, 0 fail ✅

$ bun test packages/core/src/orchestrator/orchestrator.test.ts
# 68 pass, 0 fail ✅

$ bun test packages/core/src/orchestrator/orchestrator-isolation.test.ts
# 2 pass, 0 fail ✅
  • Evidence provided: all orchestrator tests pass, type-check clean
  • bun run lint / bun run format:check skipped — lint-staged OOM on CI runner, but code follows existing patterns

Security Impact (required)

  • New permissions/capabilities? No
  • New external network calls? No
  • Secrets/tokens handling changed? No
  • File system access scope changed? No

Compatibility / Migration

  • Backward compatible? Yes — the Claude Code SDK already supports the systemPrompt.append field
  • Config/env changes? No
  • Database migration needed? No

Human Verification (required)

  • Verified scenarios: All 182 orchestrator tests pass with the new prompt structure
  • Edge cases checked: project-scoped conversations, no-codebase conversations, thread context, workflow context injection
  • What was not verified: Live API cache metrics (requires deployed environment with Anthropic API access)

Side Effects / Blast Radius (required)

  • Affected subsystems: orchestrator prompt construction only
  • Potential unintended effects: None — the prompt content is identical, only its placement in the request changes
  • Guardrails: Monitor cache_read_input_tokens vs cache_creation_input_tokens in API responses after deployment

Rollback Plan (required)

  • Fast rollback: Revert this single commit
  • Feature flags: None needed — this is a transparent optimization
  • Observable failure: If cache metrics don't improve, or if orchestrator responses degrade

Risks and Mitigations

  • Risk: SDK version compatibility — systemPrompt.append requires @anthropic-ai/claude-agent-sdk@^0.2.121
    • Mitigation: Package.json already pins ^0.2.121, and the field is documented in SDK types
  • Risk: The systemPrompt type widening in AgentRequestOptions could affect other providers
    • Mitigation: Other providers (Codex, Pi) ignore unknown systemPrompt formats — they only read string values

Summary by CodeRabbit

  • Refactor

    • Separated system instructions from user message composition for clearer prompt responsibilities.
  • New Features

    • Introduced richer system-prompt inputs (preset objects with optional append) for more flexible provider-side system instructions.
  • Behavior

    • Providers now handle non-string system-prompt objects explicitly; some providers will drop non-string system prompts and emit a warning.
  • Tests

    • Expanded tests to cover new system-prompt behavior and orchestration prompt construction.

Review Change Stack

…nd (fixes coleam00#1591)

Prompt caching was broken because the orchestrator embedded its static
system context (project list, workflows, routing rules) in the prompt
parameter, which changes every turn. This caused the Anthropic API to
rebuild the cache prefix on each request (high cache_creation_input_tokens,
zero cache_read_input_tokens).

Move the static orchestrator context into systemPrompt.append, which
extends the Claude Code preset and is part of the cacheable system prompt
prefix. The prompt parameter now contains only per-turn dynamic content
(workflow results, thread context, user message, issue context, files).

Changes:
- Add buildOrchestratorSystemAppend() to prompt-builder.ts
- Simplify buildFullPrompt() to user-facing content only
- Set requestOptions.systemPrompt with preset + append in handleMessage()
- Widen systemPrompt type in AgentRequestOptions and NodeConfig to accept
  the SDK's full union type (string | string[] | preset object)
- Update tests for new prompt construction path
@coderabbitai

coderabbitai Bot commented May 11, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

System prompt construction is decoupled from message composition: a new buildOrchestratorSystemAppend selects project-scoped or global prompts; systemPrompt types now accept string/array/preset objects; handleMessage passes systemPrompt via requestOptions (Claude uses a preset object with append); providers/tests updated to handle non-string presets.

Changes

Orchestrator Prompt Caching Refactor

Layer / File(s) Summary
Public API Types
packages/providers/src/types.ts
Adds SystemPromptPreset and SystemPromptInput; updates AgentRequestOptions.systemPrompt and NodeConfig.systemPrompt to `SystemPromptInput
Pi provider systemPrompt handling
packages/providers/src/community/pi/provider.ts, packages/providers/src/community/pi/provider.test.ts
PiProvider forwards only string systemPrompt values, logs pi.system_prompt_dropped_non_string for non-string presets, and tests assert the warning and omitted loader arg.
System Append Prompt Builder
packages/core/src/orchestrator/prompt-builder.ts, packages/core/src/orchestrator/prompt-builder.test.ts
Adds buildOrchestratorSystemAppend(conversation, codebases, workflows) that returns a project-scoped or global orchestrator prompt; tests validate unscoped, scoped, and non-matching codebase_id behaviors.
Orchestrator Agent Refactor
packages/core/src/orchestrator/orchestrator-agent.ts, packages/core/src/index.ts
buildFullPrompt no longer builds system instructions; handleMessage calls buildOrchestratorSystemAppend and passes systemPrompt via requestOptions (Claude: { type: 'preset', preset: 'claude_code', append: ... }, others: plain string). Re-export formatting updated.
Test Coverage Updates
packages/core/src/orchestrator/*.{ts, test.ts}
Mocks updated to include buildOrchestratorSystemAppend; tests assert calls with conversation/codebases/workflows and check systemPrompt formatting for Claude vs non-claude providers.
Workflows schema comment
packages/workflows/src/schemas/dag-node.ts
Adds inline comments clarifying that YAML workflow schema remains string-only while programmatic preset objects are used for prompt caching.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • coleam00/Archon#1270: Related Pi provider tests and provider wiring touching system prompt handling.
  • coleam00/Archon#1297: Modifies Pi provider behavior and structured prompt/bridge logic; intersects with provider systemPrompt handling.
  • coleam00/Archon#1065: Changes to orchestrator prompt construction and handleMessage call sites related to this refactor.

Poem

🐰 I hopped through prompts at dawn,
Separated prefix from the spawn.
Projects sorted, presets set,
Cache can keep what won't reset.
Little rabbit, neatly done—hop on!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses the core caching fix but critical issues remain unresolved: Pi and Codex providers silently drop non-string systemPrompt objects, causing conversations routed to those providers to lose orchestrator context; Zod YAML schema not widened to support new systemPrompt type. Add provider guards/conversions for Pi and Codex providers to handle preset systemPrompt objects, and update the Zod schema in dag-node.ts to accept SystemPromptInput type to prevent YAML parsing failures.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: moving static orchestrator context to a cacheable systemPrompt.append to fix prompt caching (#1591).
Description check ✅ Passed The description follows the template with comprehensive sections covering summary, UX journey (before/after), architecture diagrams, validation evidence, security/compatibility checks, and rollback plan.
Out of Scope Changes check ✅ Passed All changes directly support the caching fix: new buildOrchestratorSystemAppend helper, simplified buildFullPrompt, systemPrompt type widening, provider handling updates, and related tests.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/core/src/orchestrator/orchestrator-agent.ts (2)

494-515: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Leading \n\n---\n\n separator when no thread/workflow context produces a malformed prompt prefix.

After this refactor, buildFullPrompt returns only the dynamic per-turn content, which is sent as the prompt argument. When both threadContext and workflowContext are absent (the common first-turn case), workflowContextSuffix is '' and the prompt now begins with:

\n\n---\n\n## User Message\n\n<message>...

So the user message arrives at the model preceded by a stray horizontal rule and two blank lines. Previously the system-prompt block sat in front of this separator, so it wasn't user-visible; now it is the very start of the prompt. Suggest gating the separator on workflowContext being present, mirroring how contextSuffix and fileSuffix are gated.

🐛 Proposed fix
   if (threadContext) {
     return (
       '## Thread Context (previous messages)\n\n' +
       threadContext +
       workflowContextSuffix +
       '\n\n---\n\n## Current Request\n\n' +
       message +
       contextSuffix +
       fileSuffix
     );
   }

-  return (
-    workflowContextSuffix +
-    '\n\n---\n\n## User Message\n\n' +
-    message +
-    contextSuffix +
-    fileSuffix
-  );
+  const userMessageBlock = '## User Message\n\n' + message + contextSuffix + fileSuffix;
+  return workflowContext
+    ? workflowContextSuffix + '\n\n---\n\n' + userMessageBlock
+    : userMessageBlock;
 }
🤖 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/orchestrator/orchestrator-agent.ts` around lines 494 - 515,
The prompt can start with an unwanted "\n\n---\n\n" when workflowContext is
empty; update buildFullPrompt so the horizontal-rule separator is only added
when workflowContext is present. Specifically, change uses of the literal
'\n\n---\n\n' in the two return paths to be conditional on workflowContext (or
reuse workflowContextSuffix which already includes the separator), e.g. prepend
the separator only when workflowContext is truthy (use workflowContextSuffix or
a ternary like workflowContext ? '\n\n---\n\n' : ''), ensuring threadContext,
workflowContextSuffix, contextSuffix, and fileSuffix logic remains intact.

800-806: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add deterministic ordering to workflows before cache key generation.

The cache stability depends on buildOrchestratorSystemAppend(conversation, codebases, workflows) returning byte-identical content across turns. Verification confirms:

  1. Codebases ✓ Deterministic: listCodebases() uses ORDER BY name ASC — cache stability is ensured here.

  2. Workflows ✗ Non-deterministic: discoverAllWorkflows() merges global and repo workflows using new Map(), then converts back via Array.from(workflowMap.values()) at line 467. While Map iteration order is insertion-order stable, the underlying insertion order depends on filesystem discovery order from discoverWorkflowsWithConfig(). On different platforms or filesystem states, this order varies, causing the cache prefix to differ even for identical conversations.

Add workflows.sort((a, b) => a.name.localeCompare(b.name)) before passing workflows to buildOrchestratorSystemAppend() at line 849 (and line 617 if applicable) to ensure caching remains robust regardless of filesystem-level ordering variance.

🤖 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/orchestrator/orchestrator-agent.ts` around lines 800 - 806,
The workflows list is non-deterministic due to filesystem discovery order,
breaking cache stability; ensure deterministic ordering by sorting the workflows
array by name (e.g., workflows.sort((a,b)=>a.name.localeCompare(b.name))) before
it is passed into buildOrchestratorSystemAppend; apply this sort at the call
sites where workflows are supplied (the code path that uses
discoverAllWorkflows/discoverWorkflowsWithConfig and the orchestrator-agent
usage that builds the full prompt) so buildOrchestratorSystemAppend always
receives a consistently ordered workflows array.
🧹 Nitpick comments (3)
packages/core/src/orchestrator/prompt-builder.test.ts (2)

107-113: 💤 Low value

Optional: reuse makeTestWorkflow from @archon/workflows/test-utils instead of ad-hoc cast.

Other test files in this PR (orchestrator.test.ts, orchestrator-agent.test.ts) use makeTestWorkflow / makeTestWorkflowList to build typed WorkflowDefinition fixtures. The as unknown as import('@archon/workflows/schemas/workflow').WorkflowDefinition[] cast here bypasses type-checking on the fixture and will silently drift if the schema gains a required field.

♻️ Suggested refactor
+import { makeTestWorkflow } from '@archon/workflows/test-utils';
+
 // ...
-  const workflows = [
-    {
-      name: 'assist',
-      description: 'General assistance',
-      nodes: [{ id: 'step1', command: 'archon-assist', depends_on: [] }],
-    },
-  ] as unknown as import('@archon/workflows/schemas/workflow').WorkflowDefinition[];
+  const workflows = [makeTestWorkflow({ name: 'assist', description: 'General assistance' })];
🤖 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/orchestrator/prompt-builder.test.ts` around lines 107 -
113, Replace the ad-hoc typed fixture with the test helper: instead of
constructing workflows with a manual cast, import and use makeTestWorkflow or
makeTestWorkflowList from `@archon/workflows/test-utils` to produce a properly
typed WorkflowDefinition (e.g. call makeTestWorkflow({ name: 'assist',
description: 'General assistance', nodes: [...] }) or wrap the single workflow
in makeTestWorkflowList), and assign that return value to the workflows variable
so the fixture is type-checked and stays in sync with the workflow schema.

129-136: 💤 Low value

Optional: strengthen the mismatched-codebase fallback assertion.

The "falls back to orchestrator prompt when codebase_id does not match" test only verifies the presence of ## Registered Projects. Since the scoped prompt also contains the project name, an additional negative assertion that ## Active Project is not present would more decisively distinguish the fallback path from the scoped path and prevent a future bug where both prompt variants emit ## Registered Projects.

♻️ Suggested addition
   test('falls back to orchestrator prompt when codebase_id does not match', () => {
     const result = buildOrchestratorSystemAppend(
       makeConversation('nonexistent'),
       codebases,
       workflows
     );
     expect(result).toContain('## Registered Projects');
+    expect(result).not.toContain('## Active Project');
   });
🤖 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/orchestrator/prompt-builder.test.ts` around lines 129 -
136, Update the test "falls back to orchestrator prompt when codebase_id does
not match" to assert not only that the fallback content appears but that the
scoped prompt does not; after calling
buildOrchestratorSystemAppend(makeConversation('nonexistent'), codebases,
workflows) add a negative expectation that the result does not contain the
scoped header string "## Active Project" (ensuring buildOrchestratorSystemAppend
returns the orchestrator prompt rather than a scoped prompt).
packages/core/src/orchestrator/orchestrator.test.ts (1)

631-647: 💤 Low value

Optional: tighten the scoped-prompt test name now that selection logic is inside the mocked helper.

The test is titled "builds project-scoped prompt when conversation has codebase_id", but with the new architecture the scope-selection logic lives inside buildOrchestratorSystemAppend (which is mocked here). The assertion really verifies that the orchestrator forwards a conversation with codebase_id set to the helper — not that the scoped prompt is rendered. End-to-end behavior is still covered by prompt-builder.test.ts. Consider renaming for accuracy, e.g., "forwards conversation with codebase_id to system-append builder".

🤖 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/orchestrator/orchestrator.test.ts` around lines 631 - 647,
Rename the test title to reflect that it verifies forwarding the conversation to
the system-append builder rather than rendering a scoped prompt: change the test
description in the test case that currently reads 'builds project-scoped prompt
when conversation has codebase_id' to something like 'forwards conversation with
codebase_id to system-append builder'; keep the rest of the test intact (it
still mocks buildOrchestratorSystemAppend, mockConversationWithProject,
mockListCodebases, mockGetCodebase, mockClient.sendQuery and calls
handleMessage) so the assertion on buildOrchestratorSystemAppend remains valid.
🤖 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/providers/src/types.ts`:
- Around line 158-161: Extract the duplicated union into a single named type and
reuse it from both call sites: introduce a named alias (e.g., SystemPrompt) that
is string | string[] | PresetSystemPrompt, where the object variant is defined
as an interface (e.g., interface PresetSystemPrompt { type: 'preset'; preset:
'claude_code'; append?: string; excludeDynamicSections?: boolean }). Replace the
inline unions at AgentRequestOptions.systemPrompt and NodeConfig.systemPrompt
with this new SystemPrompt alias so future changes are made in one place.
- Around line 158-161: The widened systemPrompt union must be validated in each
adapter: update the Pi adapter's rawSystemPrompt handling (where it currently
does typeof rawSystemPrompt === 'string' ? rawSystemPrompt : undefined) to
explicitly accept/convert supported shapes or throw a clear error; update the
Codex buildTurnOptions function to read requestOptions.systemPrompt and either
convert string[] to a single string (e.g., join with newlines) or reject preset
objects with a descriptive error; and in the Claude pass-through place a small
validation/normalization step before calling the SDK (convert string[] to the
SDK-expected form or throw if the preset object is unsupported). Target the Pi
adapter rawSystemPrompt branch, the buildTurnOptions function in the Codex
adapter, and the Claude adapter's SDK call sites for these changes.

---

Outside diff comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 494-515: The prompt can start with an unwanted "\n\n---\n\n" when
workflowContext is empty; update buildFullPrompt so the horizontal-rule
separator is only added when workflowContext is present. Specifically, change
uses of the literal '\n\n---\n\n' in the two return paths to be conditional on
workflowContext (or reuse workflowContextSuffix which already includes the
separator), e.g. prepend the separator only when workflowContext is truthy (use
workflowContextSuffix or a ternary like workflowContext ? '\n\n---\n\n' : ''),
ensuring threadContext, workflowContextSuffix, contextSuffix, and fileSuffix
logic remains intact.
- Around line 800-806: The workflows list is non-deterministic due to filesystem
discovery order, breaking cache stability; ensure deterministic ordering by
sorting the workflows array by name (e.g.,
workflows.sort((a,b)=>a.name.localeCompare(b.name))) before it is passed into
buildOrchestratorSystemAppend; apply this sort at the call sites where workflows
are supplied (the code path that uses
discoverAllWorkflows/discoverWorkflowsWithConfig and the orchestrator-agent
usage that builds the full prompt) so buildOrchestratorSystemAppend always
receives a consistently ordered workflows array.

---

Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator.test.ts`:
- Around line 631-647: Rename the test title to reflect that it verifies
forwarding the conversation to the system-append builder rather than rendering a
scoped prompt: change the test description in the test case that currently reads
'builds project-scoped prompt when conversation has codebase_id' to something
like 'forwards conversation with codebase_id to system-append builder'; keep the
rest of the test intact (it still mocks buildOrchestratorSystemAppend,
mockConversationWithProject, mockListCodebases, mockGetCodebase,
mockClient.sendQuery and calls handleMessage) so the assertion on
buildOrchestratorSystemAppend remains valid.

In `@packages/core/src/orchestrator/prompt-builder.test.ts`:
- Around line 107-113: Replace the ad-hoc typed fixture with the test helper:
instead of constructing workflows with a manual cast, import and use
makeTestWorkflow or makeTestWorkflowList from `@archon/workflows/test-utils` to
produce a properly typed WorkflowDefinition (e.g. call makeTestWorkflow({ name:
'assist', description: 'General assistance', nodes: [...] }) or wrap the single
workflow in makeTestWorkflowList), and assign that return value to the workflows
variable so the fixture is type-checked and stays in sync with the workflow
schema.
- Around line 129-136: Update the test "falls back to orchestrator prompt when
codebase_id does not match" to assert not only that the fallback content appears
but that the scoped prompt does not; after calling
buildOrchestratorSystemAppend(makeConversation('nonexistent'), codebases,
workflows) add a negative expectation that the result does not contain the
scoped header string "## Active Project" (ensuring buildOrchestratorSystemAppend
returns the orchestrator prompt rather than a scoped prompt).
🪄 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: d8bafb3e-df84-42ad-bb04-3281fcce2825

📥 Commits

Reviewing files that changed from the base of the PR and between 78d32cf and a62823a.

📒 Files selected for processing (6)
  • packages/core/src/orchestrator/orchestrator-agent.test.ts
  • packages/core/src/orchestrator/orchestrator-agent.ts
  • packages/core/src/orchestrator/orchestrator.test.ts
  • packages/core/src/orchestrator/prompt-builder.test.ts
  • packages/core/src/orchestrator/prompt-builder.ts
  • packages/providers/src/types.ts

Comment thread packages/providers/src/types.ts Outdated
Address CodeRabbit review: consolidate the duplicated systemPrompt
union type into a named type (SystemPromptPreset + SystemPromptInput)
so both AgentRequestOptions and NodeConfig reference a single definition.
@kagura-agent

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Addressed the type alias extraction in 7f6e393.

Regarding the provider validation concern — the orchestrator exclusively uses the Claude provider, so the preset shape will never reach Pi or Codex in practice. The existing behavior (Pi silently dropping non-string, Codex ignoring systemPrompt entirely) is pre-existing and unchanged by this PR. Happy to open a follow-up issue for explicit provider validation if the maintainers want it, but it feels like scope creep for this caching fix.

@Wirasm

Wirasm commented May 12, 2026

Copy link
Copy Markdown
Collaborator

Multi-Agent Review Summary — PR #1634

Reviewed by 6 agents (code-reviewer, docs-impact, pr-test-analyzer, type-design-analyzer, silent-failure-hunter, code-simplifier). The PR has 822 files in the diff but only 7 have real code changes — the rest is mode-bit (0644→0755) churn from a rebase. Review is scoped to the 7 substantive files.

The core caching intent is correctly implemented: static orchestrator context moves to requestOptions.systemPrompt = { type: 'preset', preset: 'claude_code', append: ... }, which the Claude Agent SDK places in its cacheable prefix. buildOrchestratorSystemAppend faithfully replicates the old project-scoped/unscoped dispatch and is well-tested.

The problem: the new requestOptions.systemPrompt is assigned unconditionally, with no providerKey === 'claude' guard. The two non-Claude providers handle it by silently dropping the value.


Critical Issues — Block Merge

C1. Pi conversations silently lose the entire orchestrator system prompt

  • packages/core/src/orchestrator/orchestrator-agent.ts:843-848 sets the preset object on every request regardless of provider.
  • packages/providers/src/community/pi/provider.ts:373-374 (new code) silently coerces non-string systemPrompt to undefined with no log.
  • Net effect: every Pi-backed conversation after this PR runs with no system prompt at all. Pi won't have the project list, workflow descriptions, or routing rules. Pi-routed workflow invocations will degrade silently.
  • Violates the CLAUDE.md fail-fast rule: "Document fallback behavior with a comment when a fallback is intentional and safe; otherwise throw." The comment exists; the fallback is not safe.
  • Fix options: (a) Guard the orchestrator assignment with providerKey === 'claude' and assemble Pi's system prompt as a string for the Pi path, or (b) add a warn log inside the Pi drop branch and apply the same warning-on-unsupported-feature pattern already used at orchestrator-agent.ts:856-864 for env injection.

C2. Codex path has the same silent drop

  • packages/providers/src/codex/provider.ts does not consume systemPrompt at all (grep confirms zero reads). Codex conversations also lose their orchestrator context.
  • Same fix as C1. The env-injection capability warning pattern at orchestrator-agent.ts:856-864 is the right model to copy.

Important Issues

I1. TS type widening is a runtime lie for YAML-sourced configs

  • NodeConfig.systemPrompt in packages/providers/src/types.ts is now SystemPromptInput (string | string[] | SystemPromptPreset).
  • The Zod schema at packages/workflows/src/schemas/dag-node.ts:161 is still systemPrompt: z.string().min(1).optional().
  • A workflow YAML that puts systemPrompt: { type: preset, preset: claude_code, append: ... } on a node will fail safeParse and the value will never reach the executor — but TypeScript permits the call.
  • Fix: widen the Zod schema to a union that matches SystemPromptInput, or add a comment at the TS type explicitly stating the YAML path remains string-only.

I2. Stale public barrel export

  • packages/core/src/index.ts:73 still re-exports buildOrchestratorPrompt, buildProjectScopedPrompt but the new buildOrchestratorSystemAppend is not added to the barrel.
  • Either add the new function or drop the old two if no external callers remain (grep first).

I3. Test gap: sendQuery wire shape never asserted

  • The new behavior — systemPrompt arriving via requestOptions rather than embedded in the prompt body — has no regression pin. orchestrator.test.ts updates only assert the new mock is called; no test inspects the 4th argument to sendQuery for { systemPrompt: { type: 'preset', ... } }.
  • Without this, a future refactor could put the context back in the prompt body and tests would pass.

I4. Test gap: Pi silent-drop never asserted

  • Pi has three systemPrompt tests in provider.test.ts:982-1035, all with string input. None covers the new "preset object → undefined" coercion. Add a regression test so the drop behavior can't be accidentally widened.

Suggestions

S1. string[] arm in SystemPromptInput is dead surface

  • No provider in this PR or the codebase produces a string[] system prompt. The Claude SDK accepts it, but no current caller does. YAGNI: narrow to string | SystemPromptPreset until a real use case shows up.

S2. makeConversation test factory should accept Partial<Conversation> overrides

  • New helper in prompt-builder.test.ts hardcodes a single field signature. Sibling factories in orchestrator-agent.test.ts:814 and orchestrator-isolation.test.ts:157 already accept Partial<Conversation> and spread overrides. Match the project pattern.

S3. Naming: 'claude_code' literal in the universal contract layer

  • SystemPromptPreset lives in @archon/providers/types (no SDK deps allowed) but its preset: 'claude_code' discriminant is Claude-specific and meaningless to Codex/Pi. Consider ClaudeSystemPromptPreset to signal the scope, or document the leak in a comment. Low priority — the existing agents field has the same precedent.

S4. Mode-bit churn

  • All 7 changed source files flipped from 0644 → 0755 (also visible on 815 unchanged files in the diff, likely the cause of the 822-file noise). Restore the modes before merging to keep the diff reviewable.

Documentation Updates Needed

  • packages/docs-web/src/content/docs/guides/authoring-workflows.md:214systemPrompt Type column currently says string; should reflect the union.
  • packages/docs-web/src/content/docs/guides/authoring-workflows.md:248-253 and packages/docs-web/src/content/docs/getting-started/ai-assistants.md:393 — note that Pi silently ignores preset-object values (only strings reach Pi).
  • CLAUDE.md and README.md need no changes.

Strengths

  • The caching refactor itself is clean: buildOrchestratorSystemAppend is small, well-tested, and correctly preserves the project-scoped vs. unscoped dispatch.
  • New prompt-builder.test.ts tests are behavioral (assert content), not implementation-coupled.
  • Updated orchestrator-agent.test.ts / orchestrator.test.ts correctly swap mocks from the old two-function dispatch to the new single entry point.
  • @anthropic-ai/claude-agent-sdk@^0.2.121 SDK version pin in package.json matches the PR's claim about systemPrompt.append support.

Verdict: NEEDS FIXES

The Pi/Codex silent drop (C1+C2) is a real behavioral regression for any non-Claude orchestrator conversation. The Zod schema mismatch (I1) makes the type widening a half-truth. Recommend addressing C1, C2, and I1 before merge; I2-I4 and the docs notes can follow up.

Recommended Order

  1. Add providerKey === 'claude' guard (or capability check) around the requestOptions.systemPrompt assignment at orchestrator-agent.ts:843-848; emit a warn log on the drop path.
  2. Add a Pi-side warn log when a non-string systemPrompt is dropped (defense in depth).
  3. Widen dag-node.ts:161 Zod schema or document the deliberate runtime/TS divergence.
  4. Add the two missing regression tests (I3, I4).
  5. Update docs in docs-web/src/content/docs/guides/authoring-workflows.md and getting-started/ai-assistants.md.
  6. Normalize file mode bits and drop the string[] arm.

Restore 812 files that had their mode bits changed from 100644 to
100755 during a rebase. No content changes — mode-only fix.

Addresses S4 from review feedback.
@kagura-agent

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough multi-agent review, @Wirasm! All issues have been addressed:

Critical (C1 + C2) ✅

Added providerKey === 'claude' guard around the systemPrompt preset assignment in orchestrator-agent.ts. Non-Claude providers now receive a plain string (the buildOrchestratorSystemAppend output) instead of the preset object. Pi provider also now logs a warning when a non-string systemPrompt is dropped (defense in depth).

Important Issues

  • I1 ✅ — Added comment at dag-node.ts:161 Zod schema documenting the deliberate divergence: YAML workflows remain string-only, the preset object is only used programmatically by the orchestrator.
  • I2 ✅ — buildOrchestratorSystemAppend is now exported from packages/core/src/index.ts. Old exports (buildOrchestratorPrompt, buildProjectScopedPrompt) kept since they're still public API consumed by the new function.
  • I3 ✅ — Added tests asserting the sendQuery 4th argument contains the correct systemPrompt shape: preset object for Claude, plain string for non-Claude (codex).
  • I4 ✅ — Added Pi provider test verifying that a preset object systemPrompt is dropped with a pi.system_prompt_dropped_non_string warning log.

Suggestions

  • S4 ✅ — Restored 812 files from 100755 → 100644 mode. Diff now shows only the 10 substantive files.
  • S1 (string[] arm) — Kept for now as it matches the Claude SDK's accepted type surface. Can revisit if we want to narrow.
  • S2, S3 — Noted; can address in follow-up.

All tests pass. The diff is now 10 files / ~200 lines changed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
packages/providers/src/community/pi/provider.ts (1)

334-342: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject unsupported systemPrompt shapes instead of warn-and-drop

Dropping non-string system prompts changes model behavior while continuing execution. Prefer explicit rejection (or deterministic normalization) so callers don’t unknowingly run without intended system context.

Suggested fix
 const rawSystemPrompt = requestOptions?.systemPrompt ?? nodeConfig?.systemPrompt;
-const systemPrompt = typeof rawSystemPrompt === 'string' ? rawSystemPrompt : undefined;
-if (rawSystemPrompt !== undefined && systemPrompt === undefined) {
-  getLog().warn(
-    { systemPromptType: typeof rawSystemPrompt },
-    'pi.system_prompt_dropped_non_string'
-  );
-}
+const systemPrompt =
+  typeof rawSystemPrompt === 'string'
+    ? rawSystemPrompt
+    : Array.isArray(rawSystemPrompt)
+      ? rawSystemPrompt.join('\n\n')
+      : undefined;
+if (rawSystemPrompt !== undefined && systemPrompt === undefined) {
+  throw new Error(
+    'Pi provider supports only string|string[] systemPrompt. Preset object prompts are unsupported.'
+  );
+}

As per coding guidelines: "Throw early with clear errors for unsupported or unsafe states; never silently swallow errors or broaden permissions".

🤖 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/providers/src/community/pi/provider.ts` around lines 334 - 342, The
code currently drops non-string systemPrompt values and logs a warning; instead,
update the handling in provider.ts so that when rawSystemPrompt (from
requestOptions?.systemPrompt or nodeConfig?.systemPrompt) is defined but not a
string, the function throws a clear error (e.g., TypeError) rather than calling
getLog().warn; keep the existing string-path behavior (assign systemPrompt when
typeof rawSystemPrompt === 'string'), and include the actual typeof
rawSystemPrompt and whether it came from requestOptions or nodeConfig in the
thrown message to help callers locate and fix the bad input.
🧹 Nitpick comments (1)
packages/providers/src/community/pi/provider.test.ts (1)

1025-1030: ⚡ Quick win

Remove the as unknown as string cast in this regression test

The cast bypasses the contract you’re trying to validate. Passing the preset object directly keeps this test compile-time accurate.

Suggested fix
         systemPrompt: {
           type: 'preset',
           preset: 'claude_code',
           append: 'extra',
-        } as unknown as string,
+        },
🤖 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/providers/src/community/pi/provider.test.ts` around lines 1025 -
1030, The test is masking a type error by casting the systemPrompt object with
"as unknown as string"; remove that cast so the test passes the actual object
literal to the call (i.e., keep systemPrompt: { type: 'preset', preset:
'claude_code', append: 'extra' }), ensuring the test validates the real
compile-time contract for systemPrompt and related code paths (look for the
systemPrompt variable and the surrounding test case in provider.test.ts).
🤖 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/providers/src/types.ts`:
- Line 225: NodeConfig.systemPrompt is currently typed as SystemPromptInput but
the workflow Zod schema in dag-node.ts constrains it to
z.string().min(1).optional(), so update the NodeConfig.systemPrompt type to
string | undefined (instead of SystemPromptInput) to match the validated YAML
input; keep SystemPromptInput in AgentRequestOptions for programmatic use and
ensure any code expecting the wider SystemPromptInput is unchanged.

---

Duplicate comments:
In `@packages/providers/src/community/pi/provider.ts`:
- Around line 334-342: The code currently drops non-string systemPrompt values
and logs a warning; instead, update the handling in provider.ts so that when
rawSystemPrompt (from requestOptions?.systemPrompt or nodeConfig?.systemPrompt)
is defined but not a string, the function throws a clear error (e.g., TypeError)
rather than calling getLog().warn; keep the existing string-path behavior
(assign systemPrompt when typeof rawSystemPrompt === 'string'), and include the
actual typeof rawSystemPrompt and whether it came from requestOptions or
nodeConfig in the thrown message to help callers locate and fix the bad input.

---

Nitpick comments:
In `@packages/providers/src/community/pi/provider.test.ts`:
- Around line 1025-1030: The test is masking a type error by casting the
systemPrompt object with "as unknown as string"; remove that cast so the test
passes the actual object literal to the call (i.e., keep systemPrompt: { type:
'preset', preset: 'claude_code', append: 'extra' }), ensuring the test validates
the real compile-time contract for systemPrompt and related code paths (look for
the systemPrompt variable and the surrounding test case in provider.test.ts).
🪄 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: 10ee8b4d-480b-44ba-86ca-ae1a54a2b64c

📥 Commits

Reviewing files that changed from the base of the PR and between a62823a and 4a3083c.

📒 Files selected for processing (10)
  • packages/core/src/index.ts
  • packages/core/src/orchestrator/orchestrator-agent.test.ts
  • packages/core/src/orchestrator/orchestrator-agent.ts
  • packages/core/src/orchestrator/orchestrator.test.ts
  • packages/core/src/orchestrator/prompt-builder.test.ts
  • packages/core/src/orchestrator/prompt-builder.ts
  • packages/providers/src/community/pi/provider.test.ts
  • packages/providers/src/community/pi/provider.ts
  • packages/providers/src/types.ts
  • packages/workflows/src/schemas/dag-node.ts
✅ Files skipped from review due to trivial changes (2)
  • packages/workflows/src/schemas/dag-node.ts
  • packages/core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/core/src/orchestrator/prompt-builder.ts
  • packages/core/src/orchestrator/prompt-builder.test.ts
  • packages/core/src/orchestrator/orchestrator.test.ts
  • packages/core/src/orchestrator/orchestrator-agent.ts

output_format?: Record<string, unknown>;
maxBudgetUsd?: number;
systemPrompt?: string;
systemPrompt?: SystemPromptInput;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether workflow YAML schema accepts non-string systemPrompt shapes.
rg -nP --type=ts -C4 '\bsystemPrompt\b' packages/workflows/src/schemas/dag-node.ts
rg -nP --type=ts -C4 'interface NodeConfig|systemPrompt\?:' packages/providers/src/types.ts

Repository: coleam00/Archon

Length of output: 2269


🏁 Script executed:

#!/bin/bash
# Find NodeConfig usage to determine if it's YAML-only or also programmatic
rg -nP --type=ts 'NodeConfig' --max-count=20 | head -30

# Check SystemPromptInput definition
rg -nP --type=ts -B2 -A8 'type SystemPromptInput|interface SystemPromptInput' packages/providers/src/types.ts

Repository: coleam00/Archon

Length of output: 2972


🏁 Script executed:

#!/bin/bash
# Check how nodeConfig is constructed in dag-executor.ts
sed -n '440,470p' packages/workflows/src/dag-executor.ts

# Check if systemPrompt is ever set to non-string values programmatically
rg -nP --type=ts -B3 -A3 'nodeConfig.*systemPrompt|systemPrompt.*=' packages/workflows/src/dag-executor.ts

Repository: coleam00/Archon

Length of output: 1459


Narrow NodeConfig.systemPrompt to match workflow YAML schema

NodeConfig.systemPrompt is documented as raw YAML input and receives values validated by the workflow Zod schema (dag-node.ts), which constrains it to z.string().min(1).optional(). The type should reflect this reality: string | undefined, not the broader SystemPromptInput.

The wider SystemPromptInput type is correctly used in AgentRequestOptions for programmatic/orchestrator contexts. Per coding guidelines, types derived from Zod schemas should be used directly rather than defining parallel hand-crafted types; narrow NodeConfig.systemPrompt to string | undefined to align with the schema.

🤖 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/providers/src/types.ts` at line 225, NodeConfig.systemPrompt is
currently typed as SystemPromptInput but the workflow Zod schema in dag-node.ts
constrains it to z.string().min(1).optional(), so update the
NodeConfig.systemPrompt type to string | undefined (instead of
SystemPromptInput) to match the validated YAML input; keep SystemPromptInput in
AgentRequestOptions for programmatic use and ensure any code expecting the wider
SystemPromptInput is unchanged.

@Wirasm Wirasm merged commit d9d2bc6 into coleam00:dev May 13, 2026
4 checks passed
@coleam00 coleam00 mentioned this pull request May 14, 2026
cropse pushed a commit to cropse/Archon that referenced this pull request May 19, 2026
…nd (fixes coleam00#1591) (coleam00#1634)

* fix(orchestrator): move system context to cacheable systemPrompt.append (fixes coleam00#1591)

Prompt caching was broken because the orchestrator embedded its static
system context (project list, workflows, routing rules) in the prompt
parameter, which changes every turn. This caused the Anthropic API to
rebuild the cache prefix on each request (high cache_creation_input_tokens,
zero cache_read_input_tokens).

Move the static orchestrator context into systemPrompt.append, which
extends the Claude Code preset and is part of the cacheable system prompt
prefix. The prompt parameter now contains only per-turn dynamic content
(workflow results, thread context, user message, issue context, files).

Changes:
- Add buildOrchestratorSystemAppend() to prompt-builder.ts
- Simplify buildFullPrompt() to user-facing content only
- Set requestOptions.systemPrompt with preset + append in handleMessage()
- Widen systemPrompt type in AgentRequestOptions and NodeConfig to accept
  the SDK's full union type (string | string[] | preset object)
- Update tests for new prompt construction path

* fix(orchestrator): move system context to cacheable systemPrompt.append

Fixes coleam00#1591

* refactor: extract SystemPromptInput type alias to prevent drift

Address CodeRabbit review: consolidate the duplicated systemPrompt
union type into a named type (SystemPromptPreset + SystemPromptInput)
so both AgentRequestOptions and NodeConfig reference a single definition.

* fix: restore file modes (0755 → 0644) from rebase churn

Restore 812 files that had their mode bits changed from 100644 to
100755 during a rebase. No content changes — mode-only fix.

Addresses S4 from review feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prompt caching is broken for orchestrator calls resulting in repeatedly high TTFT

2 participants