Repository navigation
feat: harness 1.0 tech spec - #234
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comprehensive tech specs for an agentic chat backend split into four standalone workers: session-manager (durable store), context-manager (token-aware context assembly), llm-router (provider routing/streaming), and harness (durable orchestration, function dispatch, hooks). ChangesAgentic Chat Backend Specification
Sequence DiagramsequenceDiagram
participant Client
participant Harness as harness
participant SessionMgr as session-manager
participant ContextMgr as context-manager
participant Router as llm-router
participant Provider as provider-worker
Client->>Harness: harness::send (user message)
Harness->>SessionMgr: session::append (persist message)
Harness->>ContextMgr: context::assemble (build prompt)
ContextMgr->>Router: router::models::get (resolve limits)
Harness->>Router: router::chat (streaming request)
Router->>Provider: provider::<id>::stream (provider stream)
Provider-->>Router: AssistantMessageEvent frames
Router-->>Harness: assistant frames (deltas/done/error)
Harness->>SessionMgr: session::append / update_message (persist assistant)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
skill-check — worker0 verified, 14 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@tech-specs/2026-06-agentic/context-manager.md`:
- Around line 56-58: The fenced code block containing the budget formula "usable
= max(0, (input_limit || context_window - max_output_tokens) - reserved)" lacks
a language tag and triggers markdownlint MD040; update that fenced block to
include a language identifier (e.g., use "text") so the block becomes fenced
with a language tag and CI passes, locating the block by the formula variables
usable, input_limit, context_window, max_output_tokens, and reserved.
- Around line 127-137: The spec's context::assemble references an error when
fallback is "explicitly disabled" but AssembleRequest.options lacks such a flag;
add an explicit boolean option allow_fallback?: boolean to
AssembleRequest.options (default true) and update the context::assemble Errors
and behavior text to gate the fallback-disabled error on options.allow_fallback
=== false, or alternatively remove the fallback-disabled error path if you
prefer not to expose this control; ensure any mentions at lines ~161-163 are
updated to reference the new allow_fallback option.
In `@tech-specs/2026-06-agentic/harness.md`:
- Around line 114-124: The harness::on_steering handler is duplicating user
messages because session::message_added already persisted evt.message; in the
idle branch replace the call to harness::send (which appends) with a
non-persisting resume/enqueue flow: either trigger the bootstrap function
harness::turn (or another resume entrypoint) with the same payload so the model
runs without re-appending the user message, or add and use a skip_append flag on
harness::send and propagate skip_append: true from harness::on_steering; update
references to function_id "harness::send" in harness::on_steering to use
"harness::turn" or include skip_append to prevent double persistence.
In `@tech-specs/2026-06-agentic/README.md`:
- Around line 312-318: The shared AgentFunction type lacks the provider-exposed
flag prepare_arguments_supported (present on AgentFunctionSchema), so update the
AgentFunction contract by adding prepare_arguments_supported?: boolean to its
definition (alongside existing fields name, description, parameters, label?,
execution_mode?), ensuring consumers like router/harness can observe
provider/tool capabilities; update any related types/usages that construct or
validate AgentFunction to accept and propagate this new optional boolean.
- Around line 243-246: The SessionEntry type in README currently diverges from
the persisted shape; update the SessionEntry union so its discriminators and
variant names match the harness definitions: use the existing discriminator name
(e.g., type instead of kind), rename the custom variant to custom_message (or
the exact persisted variant name), and align payload fields (use
content/details/display instead of data if those are the persisted keys). Also
reintroduce the missing variants/fields such as branch_summary and compaction
with the same field names and shapes used in harness/src/session/tree/types.ts
so replay/branching/compaction semantics remain compatible; locate and edit the
SessionEntry declaration to mirror those exact keys and variant names.
In `@tech-specs/2026-06-agentic/session-manager.md`:
- Around line 215-220: CreateRequest declares an optional metadata field but
SessionMeta and the documented State shape never specify where or how metadata
is persisted and returned (creating an ambiguous cross-worker contract); update
the spec so the persistence/read contract is consistent by either adding a
metadata field to SessionMeta and the State section (documenting storage
location and how it appears in CreateResponse/Read APIs) or remove metadata from
CreateRequest; specifically modify the types and State description to align
CreateRequest, CreateResponse, and SessionMeta (and update examples at the other
mentioned ranges) so all references to metadata are clearly defined and
consistently returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 595f0ca3-b8e6-475e-9d07-4db233442e7d
📒 Files selected for processing (5)
tech-specs/2026-06-agentic/README.mdtech-specs/2026-06-agentic/context-manager.mdtech-specs/2026-06-agentic/harness.mdtech-specs/2026-06-agentic/llm-router.mdtech-specs/2026-06-agentic/session-manager.md
Comment on lines
+56
to
+58
| ``` | ||
| usable = max(0, (input_limit || context_window - max_output_tokens) - reserved) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced budget formula block
This fenced code block has no language identifier, which triggers markdownlint (MD040). Use a tag like text for consistency and CI cleanliness.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@tech-specs/2026-06-agentic/context-manager.md` around lines 56 - 58, The
fenced code block containing the budget formula "usable = max(0, (input_limit ||
context_window - max_output_tokens) - reserved)" lacks a language tag and
triggers markdownlint MD040; update that fenced block to include a language
identifier (e.g., use "text") so the block becomes fenced with a language tag
and CI passes, locating the block by the formula variables usable, input_limit,
context_window, max_output_tokens, and reserved.
Source: Linters/SAST tools
Comment on lines
+127
to
+137
| type AssembleRequest = { | ||
| messages: AgentMessage[]; // full candidate history, oldest first | ||
| model: ModelInput; | ||
| system_prompt?: string; // base system prompt to prepend/merge | ||
| options?: { | ||
| reserved_tokens?: number; // override the default reserve | ||
| tail_turns?: number; // user+assistant pairs always kept verbatim (default 2) | ||
| allow_compaction?: boolean; // default true | ||
| allow_prune?: boolean; // default true | ||
| protected_tools?: string[]; // function_ids whose outputs are never pruned | ||
| }; |
There was a problem hiding this comment.
context::assemble documents a fallback-disable error without a matching request field
The Errors section says fallback can be “explicitly disabled,” but AssembleRequest.options has no corresponding flag. Add an explicit option (e.g. allow_fallback?: boolean) or remove that error path from the contract.
Also applies to: 161-163
🤖 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 `@tech-specs/2026-06-agentic/context-manager.md` around lines 127 - 137, The
spec's context::assemble references an error when fallback is "explicitly
disabled" but AssembleRequest.options lacks such a flag; add an explicit boolean
option allow_fallback?: boolean to AssembleRequest.options (default true) and
update the context::assemble Errors and behavior text to gate the
fallback-disabled error on options.allow_fallback === false, or alternatively
remove the fallback-disabled error path if you prefer not to expose this
control; ensure any mentions at lines ~161-163 are updated to reference the new
allow_fallback option.
Comment on lines
+243
to
+246
| type SessionEntry = | ||
| | { kind: "message"; id: string; parent_id: string | null; timestamp: number; message: AgentMessage } | ||
| | { kind: "custom"; id: string; parent_id: string | null; timestamp: number; custom_type: string; data: unknown }; | ||
| ``` |
There was a problem hiding this comment.
SessionEntry contract currently diverges from the existing persisted shape
This shared contract changes discriminator/fields (kind vs type, custom vs custom_message, data vs content/details/display) and drops branch_summary/compaction variants that exist in harness/src/session/tree/types.ts. Since this README is the cross-worker baseline, this creates a cross-layer integration risk for replay/branching/compaction semantics.
Proposed contract-alignment diff
type SessionEntry =
- | { kind: "message"; id: string; parent_id: string | null; timestamp: number; message: AgentMessage }
- | { kind: "custom"; id: string; parent_id: string | null; timestamp: number; custom_type: string; data: unknown };
+ | { type: "message"; id: string; parent_id?: string | null; message: AgentMessage; timestamp: number }
+ | { type: "custom_message"; id: string; parent_id?: string | null; custom_type: string; content: unknown; display?: string; details?: unknown; timestamp: number }
+ | { type: "branch_summary"; id: string; parent_id?: string | null; summary: string; from_id: string; timestamp: number }
+ | { type: "compaction"; id: string; parent_id?: string | null; summary: string; tokens_before: number; tail_start_id?: string | null; details: unknown; timestamp: number };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type SessionEntry = | |
| | { kind: "message"; id: string; parent_id: string | null; timestamp: number; message: AgentMessage } | |
| | { kind: "custom"; id: string; parent_id: string | null; timestamp: number; custom_type: string; data: unknown }; | |
| ``` | |
| type SessionEntry = | |
| | { type: "message"; id: string; parent_id?: string | null; message: AgentMessage; timestamp: number } | |
| | { type: "custom_message"; id: string; parent_id?: string | null; custom_type: string; content: unknown; display?: string; details?: unknown; timestamp: number } | |
| | { type: "branch_summary"; id: string; parent_id?: string | null; summary: string; from_id: string; timestamp: number } | |
| | { type: "compaction"; id: string; parent_id?: string | null; summary: string; tokens_before: number; tail_start_id?: string | null; details: unknown; timestamp: number }; |
🤖 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 `@tech-specs/2026-06-agentic/README.md` around lines 243 - 246, The
SessionEntry type in README currently diverges from the persisted shape; update
the SessionEntry union so its discriminators and variant names match the harness
definitions: use the existing discriminator name (e.g., type instead of kind),
rename the custom variant to custom_message (or the exact persisted variant
name), and align payload fields (use content/details/display instead of data if
those are the persisted keys). Also reintroduce the missing variants/fields such
as branch_summary and compaction with the same field names and shapes used in
harness/src/session/tree/types.ts so replay/branching/compaction semantics
remain compatible; locate and edit the SessionEntry declaration to mirror those
exact keys and variant names.
Comment on lines
+312
to
+318
| type AgentFunction = { | ||
| name: string; // the iii function_id, exposed to the model as the tool name | ||
| description: string; | ||
| parameters: unknown; // JSON Schema of the arguments | ||
| label?: string; | ||
| execution_mode?: "parallel" | "sequential"; | ||
| }; |
There was a problem hiding this comment.
Tool schema is missing a provider-exposed capability flag
harness/src/types/provider.ts includes prepare_arguments_supported?: boolean on AgentFunctionSchema, but the shared AgentFunction contract omits it. This can hide provider/tool behavior from router/harness decisions.
Proposed contract-alignment diff
type AgentFunction = {
name: string; // the iii function_id, exposed to the model as the tool name
description: string;
parameters: unknown; // JSON Schema of the arguments
label?: string;
execution_mode?: "parallel" | "sequential";
+ prepare_arguments_supported?: boolean;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type AgentFunction = { | |
| name: string; // the iii function_id, exposed to the model as the tool name | |
| description: string; | |
| parameters: unknown; // JSON Schema of the arguments | |
| label?: string; | |
| execution_mode?: "parallel" | "sequential"; | |
| }; | |
| type AgentFunction = { | |
| name: string; // the iii function_id, exposed to the model as the tool name | |
| description: string; | |
| parameters: unknown; // JSON Schema of the arguments | |
| label?: string; | |
| execution_mode?: "parallel" | "sequential"; | |
| prepare_arguments_supported?: boolean; | |
| }; |
🤖 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 `@tech-specs/2026-06-agentic/README.md` around lines 312 - 318, The shared
AgentFunction type lacks the provider-exposed flag prepare_arguments_supported
(present on AgentFunctionSchema), so update the AgentFunction contract by adding
prepare_arguments_supported?: boolean to its definition (alongside existing
fields name, description, parameters, label?, execution_mode?), ensuring
consumers like router/harness can observe provider/tool capabilities; update any
related types/usages that construct or validate AgentFunction to accept and
propagate this new optional boolean.
rohitg00
reviewed
Jun 9, 2026
|
|
||
| ## Tools (the white box) | ||
|
|
||
| "Tools" are not a harness feature — they are the **iii substrate**. Any registered iii function is a |
Contributor
There was a problem hiding this comment.
do we want to call substrate here?
ytallo
reviewed
Jun 9, 2026
| [README.md § Cross-cutting contracts](README.md#cross-cutting-contracts). | ||
|
|
||
| ```typescript | ||
| type TurnStatus = "running" | "awaiting_tools" | "stopped" | "failed"; |
Contributor
Author
There was a problem hiding this comment.
yea let me add it
ytallo
reviewed
Jun 9, 2026
| the first `harness::turn` step, then returns immediately. The loop runs as durable enqueued steps so a | ||
| crash or restart resumes mid-turn. One `harness::turn` step does: | ||
|
|
||
| 1. Mark working: `session::set_status working` (first step of a turn). |
Contributor
There was a problem hiding this comment.
how can we lock it to process one message at time?
Contributor
Author
There was a problem hiding this comment.
great question, let me think how we could solve this
ytallo
reviewed
Jun 9, 2026
| the first `harness::turn` step, then returns immediately. The loop runs as durable enqueued steps so a | ||
| crash or restart resumes mid-turn. One `harness::turn` step does: | ||
|
|
||
| 1. Mark working: `session::set_status working` (first step of a turn). |
Contributor
There was a problem hiding this comment.
can we explicit the stop check?
ytallo
reviewed
Jun 9, 2026
| - `session::get` — Read one session's metadata. | ||
| - `session::list` — List sessions with pagination/ordering. | ||
| - `session::set_meta` — Update a session's `title`/`description` (e.g. an auto-generated title). | ||
| - `session::set_status` — Set status `idle`/`working`/`done`; fires `session::status_changed`. |
Contributor
There was a problem hiding this comment.
is it not missing a success/error flag?
Contributor
Author
There was a problem hiding this comment.
it shouldn't have error state, it's like when the AI finishes the work, it's done even if it couldn't fulfill the requirements
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tech-specs/2026-06-agentic/harness.md (1)
136-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
harness::on_steeringstill duplicates already-persisted user messages
session::message_addedmeansevt.messageis already in the transcript, but this branch callsharness::send, whose contract appends again. That creates duplicate persisted user entries when resuming from terminal/idle states.Suggested spec fix
- await iii.trigger({ - function_id: "harness::send", - payload: { session_id: evt.session_id, message: evt.message, model: "<model>" }, - }); + await iii.trigger({ + function_id: "harness::turn", + payload: { session_id: evt.session_id, turn_id: "<new_turn_id>", step: 0 }, + });Or document/use a
skip_append: truemode onharness::sendand require it here.🤖 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 `@tech-specs/2026-06-agentic/harness.md` around lines 136 - 145, In harness::on_steering, the session::message_added event's evt.message is already persisted, but the current branch calls harness::send which appends again; change the call so it does not duplicate storage—either invoke harness::send with a skip_append: true flag (or similar mode) in the payload, or replace the call with a non-appending notifier function; update the payload to include skip_append: true (or switch to the documented no-append API) when handling evt from session::message_added to prevent duplicate persisted user entries.
🤖 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.
Duplicate comments:
In `@tech-specs/2026-06-agentic/harness.md`:
- Around line 136-145: In harness::on_steering, the session::message_added
event's evt.message is already persisted, but the current branch calls
harness::send which appends again; change the call so it does not duplicate
storage—either invoke harness::send with a skip_append: true flag (or similar
mode) in the payload, or replace the call with a non-appending notifier
function; update the payload to include skip_append: true (or switch to the
documented no-append API) when handling evt from session::message_added to
prevent duplicate persisted user entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1abadb8-109b-49e8-9183-3232940a506e
📒 Files selected for processing (5)
tech-specs/2026-06-agentic/README.mdtech-specs/2026-06-agentic/context-manager.mdtech-specs/2026-06-agentic/harness.mdtech-specs/2026-06-agentic/llm-router.mdtech-specs/2026-06-agentic/session-manager.md
✅ Files skipped from review due to trivial changes (4)
- tech-specs/2026-06-agentic/context-manager.md
- tech-specs/2026-06-agentic/llm-router.md
- tech-specs/2026-06-agentic/README.md
- tech-specs/2026-06-agentic/session-manager.md
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tech-specs/2026-06-agentic/llm-router.md (4)
332-337: ⚖️ Poor tradeoffConsider surfacing catalog misses instead of silent degradation.
The spec notes that a missing catalog record "silently degrades a 200k model to the conservative 8k fallback budget." While this ensures robustness, silent degradation can make debugging difficult when a model's context window is unexpectedly limited. Consider one of:
- Log a warning when falling back to the conservative budget
- Emit an observability event/metric when catalog lookup fails
- Return an error at routing time if the model is not in the catalog
This would help operators detect provider registration issues or missing
modelsdeclarations before they degrade user experience.🤖 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 `@tech-specs/2026-06-agentic/llm-router.md` around lines 332 - 337, When catalog lookup in router::models::get falls back to the conservative budget, surface the miss instead of silently degrading: add a warning log and emit an observability event/metric at the fallback site (used by router::models::reconcile and provider::<id>::refresh_models flow) and optionally make routing return an error option when strict routing is required; update the code paths in router::models::get (and callers in context-manager) to record a metric like "catalog.miss" with model id and provider id and to log a warning that includes the model id and chosen fallback budget so operators can detect missing models.
454-456: ⚖️ Poor tradeoffConsider a priority mechanism for ambiguous model IDs.
The current design throws an error when multiple providers serve the same model ID, requiring the caller to pass an explicit
provider. While this is safer than silent selection, it may create friction when multiple providers offer the same models (e.g.,gpt-4on bothopenaiandazure-openai).Consider adding an optional provider priority/preference list in the
llm-routerconfig to automatically resolve ambiguity when the caller omitsprovider. This would preserve explicit disambiguation when needed while reducing verbosity for common cases.🤖 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 `@tech-specs/2026-06-agentic/llm-router.md` around lines 454 - 456, The router currently fails with "ambiguous model <id> (providers: ...)" when multiple providers serve the same model id; add an optional provider priority/preference list in the llm-router config (e.g., providerPriority: string[]) and update the resolution logic in the model-to-provider selection code (the branch that throws the ambiguous model error) to: if provider param is missing and multiple providers match, consult providerPriority and choose the highest-ranked provider present; if none match the priority list, keep failing with the same error; ensure explicit provider parameter still overrides priority. Also update any error messages and docs to mention the new config option and precedence rules.
489-490: ⚡ Quick winFix cross-reference to match section title.
The cross-reference says "liveness and cancellation" but the actual section title (line 90) is "Stream liveness and cancellation". While the anchor link works, the display text should match the section title for consistency.
📝 Proposed fix
- Honour [liveness and cancellation](`#stream-liveness-and-cancellation`): emit `ping` through silent + Honour [stream liveness and cancellation](`#stream-liveness-and-cancellation`): emit `ping` through silent🤖 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 `@tech-specs/2026-06-agentic/llm-router.md` around lines 489 - 490, The cross-reference display text should match the actual section title: replace the link text "[liveness and cancellation](`#stream-liveness-and-cancellation`)" with "Stream liveness and cancellation" while keeping the same anchor (`#stream-liveness-and-cancellation`); update the occurrence in the sentence that currently reads "Honour [liveness and cancellation](`#stream-liveness-and-cancellation`): emit `ping`..." so it becomes "Honour Stream liveness and cancellation (`#stream-liveness-and-cancellation`): emit `ping`..." to preserve link behavior and improve consistency with the section title.
84-84: ⚡ Quick winClarify the purpose of the
statusfield.The
ProviderStreamOutputincludes an optionalstatusfield, but the spec doesn't explain when or how it should be used. Should this indicate the outcome of the stream (success/failure summary), or is it for debugging? Please document the semantics or remove it if unused.🤖 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 `@tech-specs/2026-06-agentic/llm-router.md` at line 84, The ProviderStreamOutput's optional status field is ambiguous—update the spec to define its semantics and usage: state that status is a short, machine-parseable summary set when the stream ends or encounters an error (use a union of literal values like "success" | "error" | "partial" and allow an optional human-readable message elsewhere), document when to set each value (e.g., "success" on normal completion, "partial" when truncated, "error" on failure) and how consumers should interpret it, or remove the field if no code or consumers rely on it; reference the ProviderStreamOutput type to locate where to change the definition and add the corresponding usage examples in the spec.
🤖 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.
Nitpick comments:
In `@tech-specs/2026-06-agentic/llm-router.md`:
- Around line 332-337: When catalog lookup in router::models::get falls back to
the conservative budget, surface the miss instead of silently degrading: add a
warning log and emit an observability event/metric at the fallback site (used by
router::models::reconcile and provider::<id>::refresh_models flow) and
optionally make routing return an error option when strict routing is required;
update the code paths in router::models::get (and callers in context-manager) to
record a metric like "catalog.miss" with model id and provider id and to log a
warning that includes the model id and chosen fallback budget so operators can
detect missing models.
- Around line 454-456: The router currently fails with "ambiguous model <id>
(providers: ...)" when multiple providers serve the same model id; add an
optional provider priority/preference list in the llm-router config (e.g.,
providerPriority: string[]) and update the resolution logic in the
model-to-provider selection code (the branch that throws the ambiguous model
error) to: if provider param is missing and multiple providers match, consult
providerPriority and choose the highest-ranked provider present; if none match
the priority list, keep failing with the same error; ensure explicit provider
parameter still overrides priority. Also update any error messages and docs to
mention the new config option and precedence rules.
- Around line 489-490: The cross-reference display text should match the actual
section title: replace the link text "[liveness and
cancellation](`#stream-liveness-and-cancellation`)" with "Stream liveness and
cancellation" while keeping the same anchor (`#stream-liveness-and-cancellation`);
update the occurrence in the sentence that currently reads "Honour [liveness and
cancellation](`#stream-liveness-and-cancellation`): emit `ping`..." so it becomes
"Honour Stream liveness and cancellation (`#stream-liveness-and-cancellation`):
emit `ping`..." to preserve link behavior and improve consistency with the
section title.
- Line 84: The ProviderStreamOutput's optional status field is ambiguous—update
the spec to define its semantics and usage: state that status is a short,
machine-parseable summary set when the stream ends or encounters an error (use a
union of literal values like "success" | "error" | "partial" and allow an
optional human-readable message elsewhere), document when to set each value
(e.g., "success" on normal completion, "partial" when truncated, "error" on
failure) and how consumers should interpret it, or remove the field if no code
or consumers rely on it; reference the ProviderStreamOutput type to locate where
to change the definition and add the corresponding usage examples in the spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb2b7573-fb2f-43e0-aea8-9ea78e5ffb10
📒 Files selected for processing (5)
tech-specs/2026-06-agentic/README.mdtech-specs/2026-06-agentic/context-manager.mdtech-specs/2026-06-agentic/harness.mdtech-specs/2026-06-agentic/llm-router.mdtech-specs/2026-06-agentic/session-manager.md
✅ Files skipped from review due to trivial changes (3)
- tech-specs/2026-06-agentic/session-manager.md
- tech-specs/2026-06-agentic/context-manager.md
- tech-specs/2026-06-agentic/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tech-specs/2026-06-agentic/harness.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tech-specs/2026-06-agentic/harness.md (1)
650-666:⚠️ Potential issue | 🟠 MajorFix potential duplicate user-message append in
harness::on_steering(lines 650-666)
harness::on_steeringis bound tosession::message_added, soevt.messagehas already been persisted (the event includesentry_id).- The idle/terminal branch calls
harness::sendwithpayload: { session_id: evt.session_id, message: evt.message, ... }, andharness::send“append[s] the user message”.session::appendis idempotent only when the caller supplies anentry_id;harness::send(as specified) has no passthrough forevt.entry_id, so this likely appends a second user entry (duplicate transcript item).- Update the handler/API so steering can start the turn without re-appending the already-written message (e.g., add
entry_id/“use existing message” support toharness::send, or otherwise skip append when the incoming message already exists).🤖 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 `@tech-specs/2026-06-agentic/harness.md` around lines 650 - 666, harness::on_steering may re-append a user message because it calls harness::send which appends messages; fix by ensuring harness::send is told to use the existing entry instead of creating a new one—either add an entry_id field (pass evt.entry_id) or a use_existing_message/skip_append boolean in the payload when invoking harness::send from harness::on_steering; modify harness::send to accept and honor entry_id or the skip flag (and be idempotent when entry_id supplied) so the already-persisted session::message_added event is not duplicated.
🤖 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.
Outside diff comments:
In `@tech-specs/2026-06-agentic/harness.md`:
- Around line 650-666: harness::on_steering may re-append a user message because
it calls harness::send which appends messages; fix by ensuring harness::send is
told to use the existing entry instead of creating a new one—either add an
entry_id field (pass evt.entry_id) or a use_existing_message/skip_append boolean
in the payload when invoking harness::send from harness::on_steering; modify
harness::send to accept and honor entry_id or the skip flag (and be idempotent
when entry_id supplied) so the already-persisted session::message_added event is
not duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed6fe28e-b2b2-4a9e-a463-aa7385298350
📒 Files selected for processing (4)
tech-specs/2026-06-agentic/README.mdtech-specs/2026-06-agentic/harness.mdtech-specs/2026-06-agentic/llm-router.mdtech-specs/2026-06-agentic/session-manager.md
✅ Files skipped from review due to trivial changes (2)
- tech-specs/2026-06-agentic/session-manager.md
- tech-specs/2026-06-agentic/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tech-specs/2026-06-agentic/llm-router.md
…tails in documentation
… durable stream storage
This was referenced Jun 13, 2026
3 tasks done
This was referenced Jul 3, 2026
This branch was successfully deployed
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
3 participants
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.
Summary by CodeRabbit