fix(harness): streamline system prompt handling and enhance message selection logic - #284
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 50 minutes and 51 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughMoves system-prompt assembly from the console into a new ChangesHarness system-prompt module and console migration
Session messages pagination
iii-sdk v0.19.4 binary-worker docs update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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 |
skill-check — worker0 verified, 22 skipped (no docs/).
Four for four. Nicely done. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/sops/binary-worker.md (2)
694-731:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
descriptionin the manifest test too.Both manifest examples stop at
name,version,default_config, andsupported_targets. That lets a manifest with an empty or missingdescriptionpass even though section 5 says the registry requires it.♻️ Add the missing check in both snippets
assert_eq!(manifest["name"], env!("CARGO_PKG_NAME")); assert_eq!(manifest["version"], env!("CARGO_PKG_VERSION")); + assert!( + manifest["description"] + .as_str() + .map_or(false, |s| !s.is_empty()), + "description must not be empty" + ); assert!( !manifest["default_config"].is_object(), "default_config must be an object" );Also applies to: 1317-1344
🤖 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 `@docs/sops/binary-worker.md` around lines 694 - 731, The manifest_subcommand_emits_valid_json test is missing validation for the description field, which the registry requires according to section 5. Add an assertion after the existing manifest field checks to verify that the description field exists and is not empty, similar to how name, version, default_config, and supported_targets are validated. This same assertion needs to be added to the other manifest test snippet referenced in the "Also applies to" section (lines 1317-1344).
138-160:⚠️ Potential issue | 🟠 MajorConfirm the SDK version bump and API shape before finalizing this scaffold.
The docs pin
iii-sdk = "=0.19.4", but the locked versions across workers show0.19.2(harness, session-manager, provider-openai, provider-anthropic, llm-router, lsp, context-manager, approval-gate),0.19.1-next.1(shell, codex),0.19.0(storage), and0.16.0-next.2(database, coder, console, acp, email, mcp, iii-directory, image-resize). Additionally, the existingshell/src/main.rsstill constructsInitOptions { otel: Some(OtelConfig::default()), ..Default::default() }, while the doc examples useInitOptions { metadata: Some(WorkerMetadata { ... }), ..InitOptions::default() }. If 0.19.4 and the new API shape are not yet released, this scaffold will diverge from the actual pinned SDK versions. Ensure the version and constructor signatures match before treating this as the single source of truth.🤖 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 `@docs/sops/binary-worker.md` around lines 138 - 160, Verify the actual pinned iii-sdk version across all workers in the repository (check Cargo.lock files in harness, session-manager, provider-openai, provider-anthropic, llm-router, lsp, context-manager, approval-gate, shell, codex, storage, database, coder, console, acp, email, mcp, iii-directory, and image-resize) and confirm the current API shape for the InitOptions constructor pattern used in existing workers like shell/src/main.rs. Update the Cargo.toml pinned version and the InitOptions constructor examples in the documentation to match the actual versions and API signatures in use across the codebase, ensuring all documentation reflects the true current state rather than a future state.
🤖 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 `@context-manager/src/core/selection.rs`:
- Around line 136-144: The condition `if start > 0` on line 138 prevents the
valid case of `start == 0` from returning early, causing the code to fall
through and unnecessarily apply compaction. The fix is to remove the nested `if
start > 0` guard entirely so that the Selection struct with `head_len: start`
and `tail_start_index: Some(start)` is returned immediately whenever the outer
`if let Some(start) = keep` condition is true, allowing the case where start
equals 0 to preserve the full verbatim history without forced summarization.
In `@harness/architecture/integration.md`:
- Around line 275-277: Clarify the documentation for the system_prompt field to
explicitly state that both omitting the field and providing an empty string ("")
result in the same behavior: the built-in identity prompt will be used. Update
the comment or description to make callers aware that resolve_system_prompt
treats these two cases identically, preventing confusion when an empty string is
serialized and unexpectedly falls back to the default prompt. This applies to
the system_prompt field and any other similar fields mentioned in the related
sections.
In `@harness/prompts/anthropic.txt`:
- Around line 84-90: The rule statement "BEFORE you call ANY function, fetch its
contract" is too absolute and creates a logical contradiction with the
documented exceptions for discovery and registry calls mentioned later in the
prompt. Revise the opening of RULE 2 to clarify that the contract-fetch
requirement applies to regular function calls but explicitly excludes the
discovery/registry operations like engine::functions::list,
engine::triggers::list, and registry lookup calls. Make it clear that these
specific calls are exempt from the contract-fetching requirement, so the rule is
internally consistent.
In `@harness/prompts/gpt.txt`:
- Around line 53-59: The opening rule statement "BEFORE you call ANY function,
fetch its contract" is too broad and creates a logical conflict with the
documented exemptions for discovery/registry calls like engine::functions::list,
engine::triggers::list, and registry lookup calls. Revise the rule to explicitly
carve out these exemptions upfront, making it clear that contract fetching is
required for general function calls but not for the specified discovery and
registry calls. This should remove the apparent contradiction between the broad
opening statement and the later documented exceptions.
In `@harness/src/clients/session.rs`:
- Around line 206-233: The pagination loop in the messages method trusts the
next_cursor unconditionally, which could cause an infinite loop if the upstream
API returns the same non-empty cursor repeatedly. Add a guard mechanism to track
previously-seen cursors before the loop starts, then check if the new cursor
from next_cursor has already been visited before updating the cursor variable
and continuing the loop. If the cursor has been seen before, break out of the
loop to prevent infinite pagination. This ensures the loop makes progress or
exits rather than spinning on the same cursor value indefinitely.
In `@provider-anthropic/src/stream_fn.rs`:
- Around line 105-120: The empty messages validation check in the `if
input.messages.is_empty()` block is currently positioned after token loading and
configuration metadata work, which wastes resources for invalid inputs. Move
this entire validation block to execute immediately after line 54 in the
function stream_fn, before any token loading or router/config/model metadata
calls occur, so that empty message inputs are rejected early via the
synthetic_error_event without incurring unnecessary work.
---
Outside diff comments:
In `@docs/sops/binary-worker.md`:
- Around line 694-731: The manifest_subcommand_emits_valid_json test is missing
validation for the description field, which the registry requires according to
section 5. Add an assertion after the existing manifest field checks to verify
that the description field exists and is not empty, similar to how name,
version, default_config, and supported_targets are validated. This same
assertion needs to be added to the other manifest test snippet referenced in the
"Also applies to" section (lines 1317-1344).
- Around line 138-160: Verify the actual pinned iii-sdk version across all
workers in the repository (check Cargo.lock files in harness, session-manager,
provider-openai, provider-anthropic, llm-router, lsp, context-manager,
approval-gate, shell, codex, storage, database, coder, console, acp, email, mcp,
iii-directory, and image-resize) and confirm the current API shape for the
InitOptions constructor pattern used in existing workers like shell/src/main.rs.
Update the Cargo.toml pinned version and the InitOptions constructor examples in
the documentation to match the actual versions and API signatures in use across
the codebase, ensuring all documentation reflects the true current state rather
than a future state.
🪄 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: 8ee1e4ab-84ae-440e-ba4f-38618fabb235
📒 Files selected for processing (31)
console/web/README.mdconsole/web/src/lib/backend/harness-send.tsconsole/web/src/lib/backend/real.tsconsole/web/src/lib/backend/system-prompt.tscontext-manager/src/core/selection.rscontext-manager/tests/features/assemble.featurecontext-manager/tests/steps/invariant_steps.rsdocs/sops/binary-worker.mdharness/README.mdharness/architecture/integration.mdharness/prompts/anthropic.txtharness/prompts/cli.txtharness/prompts/default.txtharness/prompts/gpt.txtharness/prompts/kimi.txtharness/src/clients/session.rsharness/src/functions/send.rsharness/src/functions/spawn.rsharness/src/lib.rsharness/src/prompt/family.rsharness/src/prompt/mod.rsharness/src/prompt/mode.rsharness/src/prompt/tests.rsharness/src/prompt/variants.rsharness/src/subagent.rsharness/src/turn_loop.rsharness/src/types/turn.rsharness/tests/golden/schemas/harness.run.jsonharness/tests/golden/schemas/harness.send.jsonharness/tests/golden/schemas/harness.spawn.jsonprovider-anthropic/src/stream_fn.rs
💤 Files with no reviewable changes (2)
- console/web/src/lib/backend/system-prompt.ts
- console/web/README.md
| RULE 2 — BEFORE you call ANY function, fetch its contract from the engine by passing that | ||
| function's id as `function_id` to `engine::functions::info`. A one-line description from | ||
| `engine::functions::list` is a HINT, not the contract — `info` is the contract. Shape your | ||
| `payload` to match that schema EXACTLY: every required field, the right value formats (single | ||
| binary vs argv array, inline string vs base64, "K=V" entries), and NO field the schema does not | ||
| define. Guessing or remembering field names burns turns on retries and can put workers into | ||
| degraded states. A contract you already fetched this turn does not need refetching. |
There was a problem hiding this comment.
Clarify the contract-fetch rule for documented discovery/registry calls.
BEFORE you call ANY function conflicts with the later exceptions for engine::functions::list, engine::triggers::list, and the registry lookup calls in this same prompt. As written, the model is told to fetch a contract for calls you explicitly exempt below.
Suggested rewrite
- RULE 2 — BEFORE you call ANY function, fetch its contract from the engine by passing that
- function's id as `function_id` to `engine::functions::info`.
+ RULE 2 — BEFORE you call any non-discovery function, fetch its contract from the engine by
+ passing that function's id as `function_id` to `engine::functions::info`.Also applies to: 135-149
🤖 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 `@harness/prompts/anthropic.txt` around lines 84 - 90, The rule statement
"BEFORE you call ANY function, fetch its contract" is too absolute and creates a
logical contradiction with the documented exceptions for discovery and registry
calls mentioned later in the prompt. Revise the opening of RULE 2 to clarify
that the contract-fetch requirement applies to regular function calls but
explicitly excludes the discovery/registry operations like
engine::functions::list, engine::triggers::list, and registry lookup calls. Make
it clear that these specific calls are exempt from the contract-fetching
requirement, so the rule is internally consistent.
| Two rules govern every call. BEFORE you call ANY function, fetch its contract by passing its id | ||
| as `function_id` to `engine::functions::info` — a one-line `list` description is a hint, | ||
| not the contract. Then shape the payload to that schema exactly: every required field, the | ||
| right value formats (single binary vs argv array, inline string vs base64, "K=V" entries), no | ||
| field the schema does not define. Guessing field names burns turns on retries and can put | ||
| workers into degraded states. A contract you already fetched this turn does not need | ||
| refetching. |
There was a problem hiding this comment.
Clarify the contract-fetch rule for documented discovery/registry calls.
BEFORE you call ANY function conflicts with the later carve-outs for engine::functions::list, engine::triggers::list, and the registry lookup calls. The prompt should not require a contract fetch for calls it explicitly says are exempt.
Suggested rewrite
- Two rules govern every call. BEFORE you call ANY function, fetch its contract by passing its id
- as `function_id` to `engine::functions::info` — a one-line `list` description is a hint,
- not the contract.
+ Two rules govern every non-discovery call. BEFORE you call it, fetch its contract by passing
+ its id as `function_id` to `engine::functions::info` — a one-line `list` description is a hint,
+ not the contract.Also applies to: 118-130
🤖 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 `@harness/prompts/gpt.txt` around lines 53 - 59, The opening rule statement
"BEFORE you call ANY function, fetch its contract" is too broad and creates a
logical conflict with the documented exemptions for discovery/registry calls
like engine::functions::list, engine::triggers::list, and registry lookup calls.
Revise the rule to explicitly carve out these exemptions upfront, making it
clear that contract fetching is required for general function calls but not for
the specified discovery and registry calls. This should remove the apparent
contradiction between the broad opening statement and the later documented
exceptions.
| let mut cursor: Option<String> = None; | ||
| loop { | ||
| let mut payload = json!({ | ||
| "session_id": session_id, | ||
| "include_custom": include_custom, | ||
| "limit": PAGE_LIMIT, | ||
| }); | ||
| if let Some(c) = &cursor { | ||
| payload["cursor"] = json!(c); | ||
| } | ||
| let resp = self.call("session::messages", payload).await?; | ||
| let arr = resp | ||
| .get("messages") | ||
| .and_then(Value::as_array) | ||
| .cloned() | ||
| .unwrap_or_default(); | ||
| for item in arr { | ||
| match serde_json::from_value::<LoadedEntry>(item) { | ||
| Ok(entry) => out.push(entry), | ||
| Err(e) => { | ||
| tracing::warn!(session_id, error = %e, "skipping unparseable session entry") | ||
| } | ||
| } | ||
| } | ||
| match resp.get("next_cursor").and_then(Value::as_str) { | ||
| Some(next) if !next.is_empty() => cursor = Some(next.to_string()), | ||
| _ => break, | ||
| } |
There was a problem hiding this comment.
Add a non-advancing cursor guard to prevent infinite pagination loops.
At Line 230–233, the loop trusts next_cursor unconditionally. If upstream ever returns the same non-empty cursor repeatedly, messages() can spin forever and stall turn execution. Add a progress check (or visited-cursor set) before continuing.
Proposed fix
pub async fn messages(
&self,
session_id: &str,
include_custom: bool,
) -> Result<Vec<LoadedEntry>, HarnessError> {
const PAGE_LIMIT: u64 = 500;
let mut out: Vec<LoadedEntry> = Vec::new();
let mut cursor: Option<String> = None;
+ let mut last_cursor: Option<String> = None;
loop {
let mut payload = json!({
"session_id": session_id,
"include_custom": include_custom,
"limit": PAGE_LIMIT,
});
if let Some(c) = &cursor {
payload["cursor"] = json!(c);
}
let resp = self.call("session::messages", payload).await?;
@@
}
match resp.get("next_cursor").and_then(Value::as_str) {
- Some(next) if !next.is_empty() => cursor = Some(next.to_string()),
+ Some(next) if !next.is_empty() => {
+ if last_cursor.as_deref() == Some(next) {
+ tracing::warn!(
+ session_id,
+ cursor = next,
+ "session::messages returned a non-advancing cursor; stopping pagination"
+ );
+ break;
+ }
+ last_cursor = Some(next.to_string());
+ cursor = Some(next.to_string());
+ }
_ => break,
}
}
Ok(out)
}📝 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.
| let mut cursor: Option<String> = None; | |
| loop { | |
| let mut payload = json!({ | |
| "session_id": session_id, | |
| "include_custom": include_custom, | |
| "limit": PAGE_LIMIT, | |
| }); | |
| if let Some(c) = &cursor { | |
| payload["cursor"] = json!(c); | |
| } | |
| let resp = self.call("session::messages", payload).await?; | |
| let arr = resp | |
| .get("messages") | |
| .and_then(Value::as_array) | |
| .cloned() | |
| .unwrap_or_default(); | |
| for item in arr { | |
| match serde_json::from_value::<LoadedEntry>(item) { | |
| Ok(entry) => out.push(entry), | |
| Err(e) => { | |
| tracing::warn!(session_id, error = %e, "skipping unparseable session entry") | |
| } | |
| } | |
| } | |
| match resp.get("next_cursor").and_then(Value::as_str) { | |
| Some(next) if !next.is_empty() => cursor = Some(next.to_string()), | |
| _ => break, | |
| } | |
| let mut cursor: Option<String> = None; | |
| let mut last_cursor: Option<String> = None; | |
| loop { | |
| let mut payload = json!({ | |
| "session_id": session_id, | |
| "include_custom": include_custom, | |
| "limit": PAGE_LIMIT, | |
| }); | |
| if let Some(c) = &cursor { | |
| payload["cursor"] = json!(c); | |
| } | |
| let resp = self.call("session::messages", payload).await?; | |
| let arr = resp | |
| .get("messages") | |
| .and_then(Value::as_array) | |
| .cloned() | |
| .unwrap_or_default(); | |
| for item in arr { | |
| match serde_json::from_value::<LoadedEntry>(item) { | |
| Ok(entry) => out.push(entry), | |
| Err(e) => { | |
| tracing::warn!(session_id, error = %e, "skipping unparseable session entry") | |
| } | |
| } | |
| } | |
| match resp.get("next_cursor").and_then(Value::as_str) { | |
| Some(next) if !next.is_empty() => { | |
| if last_cursor.as_deref() == Some(next) { | |
| tracing::warn!( | |
| session_id, | |
| cursor = next, | |
| "session::messages returned a non-advancing cursor; stopping pagination" | |
| ); | |
| break; | |
| } | |
| last_cursor = Some(next.to_string()); | |
| cursor = Some(next.to_string()); | |
| } | |
| _ => break, | |
| } | |
| } |
🤖 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 `@harness/src/clients/session.rs` around lines 206 - 233, The pagination loop
in the messages method trusts the next_cursor unconditionally, which could cause
an infinite loop if the upstream API returns the same non-empty cursor
repeatedly. Add a guard mechanism to track previously-seen cursors before the
loop starts, then check if the new cursor from next_cursor has already been
visited before updating the cursor variable and continuing the loop. If the
cursor has been seen before, break out of the loop to prevent infinite
pagination. This ensures the loop makes progress or exits rather than spinning
on the same cursor value indefinitely.
| // Defense in depth: never POST an empty messages array — Anthropic rejects | ||
| // it with a 400 ("messages: at least one message is required"). Surface a | ||
| // clear provider error frame instead of a cryptic upstream failure. The | ||
| // harness/context-manager guards make this unreachable in practice. | ||
| if input.messages.is_empty() { | ||
| let _ = send_event( | ||
| sink, | ||
| &synthetic_error_event( | ||
| "refusing to call anthropic with an empty messages array \ | ||
| (messages: at least one message is required)", | ||
| &model, | ||
| ErrorKind::Permanent, | ||
| ), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Move empty-message validation before resolve/config work.
Right now (Line 109) the guard runs after token load + router/config/model metadata calls. For empty input, that can produce unrelated errors first and adds avoidable dependency work. Short-circuit this check immediately after Line 54.
Suggested fix
async fn run_stream_call(
@@
) {
let model = input.model.clone();
+
+ if input.messages.is_empty() {
+ let _ = send_event(
+ sink,
+ &synthetic_error_event(
+ "refusing to call anthropic with an empty messages array \
+ (messages: at least one message is required)",
+ &model,
+ ErrorKind::Permanent,
+ ),
+ );
+ return;
+ }
let mut warnings = Vec::new();
@@
- if input.messages.is_empty() {
- let _ = send_event(
- sink,
- &synthetic_error_event(
- "refusing to call anthropic with an empty messages array \
- (messages: at least one message is required)",
- &model,
- ErrorKind::Permanent,
- ),
- );
- return;
- }
-
let body = build_body(&BodyArgs {🤖 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 `@provider-anthropic/src/stream_fn.rs` around lines 105 - 120, The empty
messages validation check in the `if input.messages.is_empty()` block is
currently positioned after token loading and configuration metadata work, which
wastes resources for invalid inputs. Move this entire validation block to
execute immediately after line 54 in the function stream_fn, before any token
loading or router/config/model metadata calls occur, so that empty message
inputs are rejected early via the synthetic_error_event without incurring
unnecessary work.
…election logic - Removed the obsolete `system-prompt.ts` file, consolidating system prompt assembly directly within the harness. - Introduced a new `HarnessSendMode` type to manage operating modes (`plan`, `ask`, `agent`) for message handling. - Updated the `real.ts` file to utilize the new mode handling, ensuring the harness constructs identity prompts dynamically based on the provided mode. - Enhanced the message selection logic in `selection.rs` to prevent empty message arrays when summarizing history, ensuring at least the last turn is retained. - Added tests to verify the new behavior of message selection and system prompt handling, ensuring compliance with the updated logic.
301b461 to
bc7da36
Compare
Summary
system-prompt.tsfile, consolidating system prompt assembly directly within the harness.HarnessSendModetype to manage operating modes (plan,ask,agent) for message handling.real.tsfile to utilize the new mode handling, ensuring the harness constructs identity prompts dynamically based on the provided mode.selection.rsto prevent empty message arrays when summarizing history, ensuring at least the last turn is retained.Move system-prompt assembly into the harness. The console now sends
mode(plan|ask|agent) instead of building prompts client-side; the harness selects a provider-specific identity prompt (anthropic,openai,kimi, default) and prepends the mode paragraph whensystem_promptis omitted.Fixes related context/turn failures: context-manager no longer compacts history into an empty tail, session messages are fully paginated, and empty-message guards are added in the turn loop and Anthropic provider.
Test plan
system_promptoverrideharness.send/harness.spawnwith explicitsystem_prompt— override still winscargo test -p harness -p context-manager -p provider-anthropicSummary by CodeRabbit
Release Notes
New Features
plan,ask,agent) to control request execution behaviorBug Fixes
Documentation