mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-08 17:10:50 -10:00
wip: unfinished review fix-up (interrupted by the session usage limit)
Uncommitted edits from the fix-up stage, saved so nothing is lost. Not compiled or tested after these edits; the next pass reviews and finishes them. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01R9rJEoMuUSRWznU6QjkE7h
This commit is contained in:
CodeWhale BotandClaude Opus 5.5 committed
1 parent
01c6da2445
commit
fa8058ce85
8 files changed
+422
-54
No files matched your search
@@ -74,6 +74,24 @@ quieter, and Fleet runs can be checked before they spend anything.
|
||||
honours `NO_COLOR` ([#5846](https://github.com/Hmbown/Codewhale/issues/5846)).
|
||||
- `/cache`, `/stash`, `/config`, session prune, `metrics --since` and the
|
||||
`lane start`/`lane stop --json` flags handle their edge cases.
|
||||
- Plain `codewhale exec` (no `--auto`) runs one Engine turn with the same
|
||||
system prompt as every other run, so `--json` no longer changes the model's
|
||||
instructions, and the run is recorded as a session. It offers no tools
|
||||
unless a flag grants them (`--auto`, `--yolo`, `--allowed-tools`, or
|
||||
resuming a session): `--max-turns`, `--disallowed-tools`,
|
||||
`--append-system-prompt`, `--sandbox` and `--output-format stream-json` no
|
||||
longer turn a chat call into a tool-using agent. The `--json` one-shot
|
||||
receipt drops `stop_reason` and gains the agent receipt's fields. A reply
|
||||
cut off at the provider's output limit is continued instead of failing the
|
||||
run ([#6510](https://github.com/Hmbown/Codewhale/issues/6510)).
|
||||
- `codewhale review` of a plain diff uses the same review prompt as
|
||||
`review --pr` and prints the structured review as Markdown, or the model's
|
||||
prose when it ignores the JSON format
|
||||
([#6510](https://github.com/Hmbown/Codewhale/issues/6510)).
|
||||
- A recursive `rlm_query` that runs out of rounds returns its last answer
|
||||
marked `[rlm_query incomplete: …]` instead of an empty string, its model
|
||||
calls appear in the parent turn's record, and its history is no longer
|
||||
trimmed ([#6511](https://github.com/Hmbown/Codewhale/issues/6511)).
|
||||
|
||||
### Experience
|
||||
|
||||
|
||||
@@ -74,6 +74,24 @@ quieter, and Fleet runs can be checked before they spend anything.
|
||||
honours `NO_COLOR` ([#5846](https://github.com/Hmbown/Codewhale/issues/5846)).
|
||||
- `/cache`, `/stash`, `/config`, session prune, `metrics --since` and the
|
||||
`lane start`/`lane stop --json` flags handle their edge cases.
|
||||
- Plain `codewhale exec` (no `--auto`) runs one Engine turn with the same
|
||||
system prompt as every other run, so `--json` no longer changes the model's
|
||||
instructions, and the run is recorded as a session. It offers no tools
|
||||
unless a flag grants them (`--auto`, `--yolo`, `--allowed-tools`, or
|
||||
resuming a session): `--max-turns`, `--disallowed-tools`,
|
||||
`--append-system-prompt`, `--sandbox` and `--output-format stream-json` no
|
||||
longer turn a chat call into a tool-using agent. The `--json` one-shot
|
||||
receipt drops `stop_reason` and gains the agent receipt's fields. A reply
|
||||
cut off at the provider's output limit is continued instead of failing the
|
||||
run ([#6510](https://github.com/Hmbown/Codewhale/issues/6510)).
|
||||
- `codewhale review` of a plain diff uses the same review prompt as
|
||||
`review --pr` and prints the structured review as Markdown, or the model's
|
||||
prose when it ignores the JSON format
|
||||
([#6510](https://github.com/Hmbown/Codewhale/issues/6510)).
|
||||
- A recursive `rlm_query` that runs out of rounds returns its last answer
|
||||
marked `[rlm_query incomplete: …]` instead of an empty string, its model
|
||||
calls appear in the parent turn's record, and its history is no longer
|
||||
trimmed ([#6511](https://github.com/Hmbown/Codewhale/issues/6511)).
|
||||
|
||||
### Experience
|
||||
|
||||
|
||||
@@ -6502,6 +6502,56 @@ async fn tool_request_snapshot_matches_the_exact_mock_request_payload() {
|
||||
assert!(snapshot.delivery_status.starts_with("unknown"));
|
||||
}
|
||||
|
||||
/// #6510: the inline ```repl kernel runs model-written Python, so it answers
|
||||
/// to the `code_execution` gate. A surface that leaves `code_execution` out of
|
||||
/// its allowlist (plain `exec`'s zero-tool surface, or a narrowed
|
||||
/// `--allowed-tools`) or denies it must not execute a fence: the reply is the
|
||||
/// answer, no kernel starts, and no second model call is made.
|
||||
#[tokio::test]
|
||||
async fn repl_fence_does_not_run_when_code_execution_is_not_allowed() {
|
||||
use crate::llm_client::mock::{MockLlmClient, canned};
|
||||
use codewhale_models::{ContentBlock, Message};
|
||||
|
||||
let fence = "```repl\nprint('fence ran')\n```";
|
||||
for (allowed, disallowed) in [
|
||||
(Some(Vec::new()), None),
|
||||
(Some(vec!["read_file".to_string()]), None),
|
||||
(None, Some(vec!["code_execution".to_string()])),
|
||||
] {
|
||||
let workspace = tempdir().expect("tempdir");
|
||||
let mock = std::sync::Arc::new(MockLlmClient::new(vec![canned::simple_text_turn(fence)]));
|
||||
let client: crate::core::model_client::SharedModelClient = mock.clone();
|
||||
let config = EngineConfig {
|
||||
allowed_tools: allowed.clone(),
|
||||
disallowed_tools: disallowed.clone(),
|
||||
..deterministic_engine_config(workspace.path())
|
||||
};
|
||||
let (mut engine, _handle) =
|
||||
Engine::new_with_model_client(config, &Config::default(), client);
|
||||
engine.session.add_message(Message {
|
||||
role: Role::User,
|
||||
content: vec![ContentBlock::Text {
|
||||
text: "Answer.".to_string(),
|
||||
cache_control: None,
|
||||
}],
|
||||
});
|
||||
let registry = crate::tools::ToolRegistry::new(crate::tools::ToolContext::new(
|
||||
workspace.path().to_path_buf(),
|
||||
));
|
||||
let policy = test_tool_surface(&engine, registry, None, AppMode::Agent);
|
||||
let mut turn = crate::core::turn::TurnContext::new(4);
|
||||
let (status, error) = engine.run_turn(&mut turn, policy, None, None).await;
|
||||
|
||||
let case = format!("allowed={allowed:?} disallowed={disallowed:?}");
|
||||
assert_eq!(status, TurnOutcomeStatus::Completed, "{case}: {error:?}");
|
||||
assert!(
|
||||
engine.repl_kernel.is_none(),
|
||||
"{case}: kernel must not start"
|
||||
);
|
||||
assert_eq!(mock.call_count(), 1, "{case}: no follow-up model call");
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn normal_repl_kernel_persists_across_user_turns() {
|
||||
use crate::llm_client::mock::{MockLlmClient, canned};
|
||||
|
||||
+129
-22
@@ -429,7 +429,10 @@ Examples:
|
||||
|
||||
Plain `codewhale exec` is a one-shot model response: one Engine turn with the
|
||||
same system prompt as every other run, and no tools. Use `--auto` for
|
||||
non-interactive agent-with-tools execution. `--auto` does not change the
|
||||
non-interactive agent-with-tools execution. Tools are offered only with
|
||||
`--auto`, `--yolo`, `--allowed-tools`, or when resuming a session; limits such
|
||||
as `--max-turns`, `--disallowed-tools` or `--sandbox`, and the output format,
|
||||
never add tools. `--auto` does not change the
|
||||
sandbox posture or elevate a denied tool. Use `--sandbox danger-full-access`
|
||||
or `--allow-sandbox-elevation` to explicitly authorize sandbox elevation.
|
||||
")]
|
||||
@@ -601,6 +604,32 @@ fn shell_only_exec_allowed_tools() -> Vec<String> {
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// #6510: whether an `exec` run is offered tools. Plain exec is one Engine
|
||||
/// turn with no tools; only a flag that grants tool authority opens a
|
||||
/// surface: `--auto`/`--yolo`, an explicit `--allowed-tools` list, a Fleet
|
||||
/// authority envelope, or the launcher's tool-surface env. A resumed session
|
||||
/// keeps its surface, because its history can carry tool calls and results
|
||||
/// that a zero-tool request cannot replay.
|
||||
///
|
||||
/// Limits (`--max-turns`, `--max-tool-calls`, `--disallowed-tools`,
|
||||
/// `--sandbox`, `--allow-sandbox-elevation`), prompt and hook opt-ins
|
||||
/// (`--append-system-prompt`, `--hooks`) and the output format never grant
|
||||
/// tools. They used to, so `exec --max-turns 1 "hi"` silently became a
|
||||
/// tool-using agent.
|
||||
fn exec_grants_tool_surface(
|
||||
args: &ExecArgs,
|
||||
yolo: bool,
|
||||
resuming: bool,
|
||||
env_tool_surface: bool,
|
||||
) -> bool {
|
||||
args.auto
|
||||
|| yolo
|
||||
|| resuming
|
||||
|| args.allowed_tools.is_some()
|
||||
|| args.tool_authority_json.is_some()
|
||||
|| env_tool_surface
|
||||
}
|
||||
|
||||
fn resolve_exec_allowed_tools(
|
||||
cli_allowed_tools: Option<&[String]>,
|
||||
env_tool_surface: Option<ExecToolSurface>,
|
||||
@@ -2445,23 +2474,15 @@ async fn run_async_main_dispatch(
|
||||
let yolo = cli.yolo || config.yolo.unwrap_or(false);
|
||||
let env_tool_surface = exec_tool_surface_from_env();
|
||||
// #6510: every exec runs on the Engine — one turn loop, one
|
||||
// prompt authority (BASE_PROMPT, AGENTS.md, skills). These
|
||||
// flags ask for a tool surface; without any of them the run
|
||||
// is a one-shot answer on a zero-tool surface.
|
||||
let tool_surface_requested = args.auto
|
||||
|| yolo
|
||||
|| resume_session_id.is_some()
|
||||
|| args.output_format == ExecOutputFormat::StreamJson
|
||||
|| args.max_turns.is_some()
|
||||
|| args.max_tool_calls.is_some()
|
||||
|| args.allowed_tools.is_some()
|
||||
|| args.disallowed_tools.is_some()
|
||||
|| args.append_system_prompt.is_some()
|
||||
|| args.tool_authority_json.is_some()
|
||||
|| args.hooks
|
||||
|| args.sandbox.is_some()
|
||||
|| args.allow_sandbox_elevation
|
||||
|| env_tool_surface.is_some();
|
||||
// prompt authority (BASE_PROMPT, AGENTS.md, skills). Without
|
||||
// a tool grant the run is a one-shot answer on a zero-tool
|
||||
// surface.
|
||||
let tool_surface_requested = exec_grants_tool_surface(
|
||||
&args,
|
||||
yolo,
|
||||
resume_session_id.is_some(),
|
||||
env_tool_surface.is_some(),
|
||||
);
|
||||
{
|
||||
if args.parent_death_watch {
|
||||
spawn_parent_death_watch();
|
||||
@@ -8977,10 +8998,7 @@ async fn run_review(config: &Config, args: ReviewArgs) -> Result<()> {
|
||||
eprintln!("Review receipt written: {}", path.display());
|
||||
}
|
||||
} else {
|
||||
match structured.as_ref() {
|
||||
Some(review) => println!("{}", render_review_markdown(review, None)),
|
||||
None => println!("{output}"),
|
||||
}
|
||||
println!("{}", plain_diff_review_report(structured.as_ref(), &output));
|
||||
if let Some((path, _)) = receipt {
|
||||
eprintln!("Review receipt written: {}", path.display());
|
||||
}
|
||||
@@ -9672,6 +9690,20 @@ fn plan_inline_review_comments(
|
||||
plan
|
||||
}
|
||||
|
||||
/// The local report for a plain-diff review (#6510). The diff is reviewed
|
||||
/// under the one structured review prompt; when the model kept the JSON
|
||||
/// contract the report is rendered Markdown, otherwise its prose is printed
|
||||
/// verbatim, as before.
|
||||
fn plain_diff_review_report(
|
||||
structured: Option<&crate::tools::review::ReviewOutput>,
|
||||
output: &str,
|
||||
) -> String {
|
||||
structured.map_or_else(
|
||||
|| output.to_string(),
|
||||
|review| render_review_markdown(review, None),
|
||||
)
|
||||
}
|
||||
|
||||
/// Render a structured review as Markdown. `posted` names the PR the body is
|
||||
/// being published to; `None` is the local report (plain diffs and unposted
|
||||
/// PR reviews), where suggestion fences stay live.
|
||||
@@ -15796,6 +15828,58 @@ reasoning = "high"
|
||||
assert_eq!(client.base_url(), "http://127.0.0.1:18183/v1");
|
||||
}
|
||||
|
||||
/// #6510: only a flag that grants tool authority opens a tool surface.
|
||||
/// Limits, prompt/hook opt-ins and the output format keep plain exec a
|
||||
/// zero-tool one-shot; `--max-turns 1` used to make it a tool agent.
|
||||
#[test]
|
||||
fn exec_tool_surface_needs_an_explicit_grant() {
|
||||
let grants = |argv: &[&str], yolo: bool, resuming: bool, env: bool| {
|
||||
let mut full = vec!["codewhale", "exec"];
|
||||
full.extend_from_slice(argv);
|
||||
full.push("hi");
|
||||
let cli = parse_cli(&full);
|
||||
let Some(Commands::Exec(args)) = cli.command else {
|
||||
panic!("expected exec command");
|
||||
};
|
||||
exec_grants_tool_surface(&args, yolo, resuming, env)
|
||||
};
|
||||
|
||||
for zero_tool in [
|
||||
&[][..],
|
||||
&["--max-turns", "1"],
|
||||
&["--max-tool-calls", "3"],
|
||||
&["--disallowed-tools", "exec_shell"],
|
||||
&["--append-system-prompt", "be brief"],
|
||||
&["--hooks"],
|
||||
&["--sandbox", "read-only"],
|
||||
&["--allow-sandbox-elevation"],
|
||||
&["--output-format", "stream-json"],
|
||||
&["--json"],
|
||||
] {
|
||||
assert!(
|
||||
!grants(zero_tool, false, false, false),
|
||||
"{zero_tool:?} must not grant tools"
|
||||
);
|
||||
}
|
||||
|
||||
assert!(grants(&["--auto"], false, false, false));
|
||||
assert!(grants(
|
||||
&["--allowed-tools", "read_file"],
|
||||
false,
|
||||
false,
|
||||
false
|
||||
));
|
||||
assert!(grants(
|
||||
&["--tool-authority-json", "{}"],
|
||||
false,
|
||||
false,
|
||||
false
|
||||
));
|
||||
assert!(grants(&["--max-turns", "1"], true, false, false), "yolo");
|
||||
assert!(grants(&[], false, true, false), "resumed session");
|
||||
assert!(grants(&[], false, false, true), "launcher tool surface");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_accepts_split_prompt_words_for_windows_cmd_shims() {
|
||||
let cli = parse_cli(&["codewhale", "exec", "hello", "world"]);
|
||||
@@ -17131,6 +17215,29 @@ api_key = "test-only-key"
|
||||
assert!(posted.contains("```text"), "{posted}");
|
||||
}
|
||||
|
||||
/// #6510: a plain-diff review now asks for the one structured review
|
||||
/// contract. A reply that keeps it renders as the Markdown report; one
|
||||
/// that ignores it is printed verbatim, as the old prose path did.
|
||||
#[test]
|
||||
fn plain_diff_review_renders_structured_reply_and_keeps_prose() {
|
||||
let json = r#"{"summary":"One risky unwrap.","issues":[{"severity":"high","title":"Unchecked unwrap","description":"Panics on None.","path":"src/lib.rs","line":11}],"suggestions":[],"overall_assessment":"request changes"}"#;
|
||||
let structured = crate::tools::review::ReviewOutput::from_structured_str(json);
|
||||
let report = plain_diff_review_report(structured.as_ref(), json);
|
||||
assert!(report.starts_with("## Codewhale review"), "{report}");
|
||||
assert!(report.contains("One risky unwrap."), "{report}");
|
||||
assert!(report.contains("### Findings"), "{report}");
|
||||
assert!(report.contains("`src/lib.rs:11`"), "{report}");
|
||||
assert!(
|
||||
!report.contains("\"issues\""),
|
||||
"raw JSON must not leak: {report}"
|
||||
);
|
||||
|
||||
let prose = "Looks fine overall; consider a test for the empty case.";
|
||||
let structured = crate::tools::review::ReviewOutput::from_structured_str(prose);
|
||||
assert!(structured.is_none());
|
||||
assert_eq!(plain_diff_review_report(structured.as_ref(), prose), prose);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn oversized_review_suggestion_degrades_to_prose() {
|
||||
use crate::tools::review::MAX_COMMITTABLE_SUGGESTION_LINES;
|
||||
|
||||
@@ -678,13 +678,23 @@ def llm_query_batched(prompts, model=None, dependency_mode=None, safety_note=Non
|
||||
out.append(r.get("text",""))
|
||||
return out
|
||||
|
||||
def _rlm_child_text(r, err_label):
|
||||
# A sub-RLM can end with a partial answer AND an error (it ran out of
|
||||
# rounds before FINAL). Keep the text and mark it incomplete; only an
|
||||
# error with no text collapses to the error marker.
|
||||
text = r.get("text") or ""
|
||||
err = r.get("error")
|
||||
if not err:
|
||||
return text
|
||||
if text.strip():
|
||||
return f"{text}\n[rlm_query incomplete: {err}]"
|
||||
return f"[{err_label}: {err}]"
|
||||
|
||||
def rlm_query(prompt, model=None):
|
||||
"""Recursive sub-RLM. The model arg is accepted for compatibility but ignored by Rust."""
|
||||
resp = _rpc({"type":"rlm","prompt":str(prompt),"model":model})
|
||||
if isinstance(resp, dict) and resp.get("error"):
|
||||
return f"[rlm_query error: {resp['error']}]"
|
||||
if isinstance(resp, dict):
|
||||
return resp.get("text","")
|
||||
return _rlm_child_text(resp, "rlm_query error")
|
||||
return str(resp)
|
||||
|
||||
def rlm_query_batched(prompts, model=None, dependency_mode=None, safety_note=None):
|
||||
@@ -706,13 +716,7 @@ def rlm_query_batched(prompts, model=None, dependency_mode=None, safety_note=Non
|
||||
results = (resp or {}).get("results", []) if isinstance(resp, dict) else []
|
||||
if len(results) != len(prompts):
|
||||
return [f"[rlm_query_batched: size mismatch ({len(results)}/{len(prompts)})]" for _ in prompts]
|
||||
out = []
|
||||
for r in results:
|
||||
if r.get("error"):
|
||||
out.append(f"[child err: {r['error']}]")
|
||||
else:
|
||||
out.append(r.get("text",""))
|
||||
return out
|
||||
return [_rlm_child_text(r, "child err") for r in results]
|
||||
|
||||
def _slice_text(slice_value):
|
||||
if slice_value is None:
|
||||
@@ -1414,6 +1418,58 @@ mod tests {
|
||||
rt.shutdown().await;
|
||||
}
|
||||
|
||||
/// A sub-RLM that ran out of rounds returns its last answer AND an
|
||||
/// error. Answers every recursive request with that shape.
|
||||
struct ExhaustedRlmBridge;
|
||||
|
||||
impl RpcDispatcher for ExhaustedRlmBridge {
|
||||
fn dispatch<'a>(
|
||||
&'a self,
|
||||
req: RpcRequest,
|
||||
) -> std::pin::Pin<Box<dyn std::future::Future<Output = RpcResponse> + Send + 'a>> {
|
||||
Box::pin(async move {
|
||||
let partial = |i: usize| SingleResp {
|
||||
text: if i == 1 {
|
||||
String::new()
|
||||
} else {
|
||||
format!("partial answer {i}")
|
||||
},
|
||||
error: Some("RLM loop exhausted after 25 iterations without FINAL".to_string()),
|
||||
};
|
||||
match req {
|
||||
RpcRequest::RlmBatch { prompts, .. } => RpcResponse::Batch(BatchResp {
|
||||
results: (0..prompts.len()).map(partial).collect(),
|
||||
}),
|
||||
_ => RpcResponse::Single(partial(0)),
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
/// #6511: the Rust loop kept the exhausted sub-RLM's last answer, but the
|
||||
/// Python helpers returned only the error marker whenever `error` was
|
||||
/// set, so the calling code never saw the text.
|
||||
#[tokio::test]
|
||||
async fn rlm_query_keeps_partial_answer_from_an_exhausted_child() {
|
||||
let mut rt = PythonRuntime::new().await.expect("spawn");
|
||||
let round = rt
|
||||
.run(
|
||||
"print(rlm_query('go'))\nfor r in rlm_query_batched(['a', 'b'], dependency_mode='independent'):\n print(r)",
|
||||
Some(&ExhaustedRlmBridge),
|
||||
)
|
||||
.await
|
||||
.expect("execute");
|
||||
let out = &round.stdout;
|
||||
assert!(
|
||||
out.contains("partial answer 0\n[rlm_query incomplete: RLM loop exhausted"),
|
||||
"{out}"
|
||||
);
|
||||
// Batched: a child with text keeps it; one with none keeps the old marker.
|
||||
assert!(out.matches("partial answer 0").count() == 2, "{out}");
|
||||
assert!(out.contains("[child err: RLM loop exhausted"), "{out}");
|
||||
rt.shutdown().await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rpc_dispatcher_round_trips_sub_query_alias() {
|
||||
let bridge = StubBridge::new();
|
||||
|
||||
+24
-10
@@ -16,10 +16,14 @@
|
||||
//! history is bounded by [`MAX_RLM_ITERATIONS`] (two small metadata messages
|
||||
//! per round), not by silently dropping the middle.
|
||||
//! - **Never an empty answer without a reason.** On exhaustion the last root
|
||||
//! response is returned with the error; any other empty answer carries an
|
||||
//! error naming the termination.
|
||||
//! response is returned with the error, and the REPL's `rlm_query` hands
|
||||
//! that text to the caller marked `[rlm_query incomplete: …]`. Any other
|
||||
//! empty answer, except a deliberate `FINAL("")`, carries an error naming
|
||||
//! the termination.
|
||||
//! - Not bounded by wall clock: per-request cancellation comes from the
|
||||
//! parent turn; the iteration cap is the cost bound.
|
||||
//! parent turn; the iteration cap is the cost bound. `turn_timeout()` was
|
||||
//! deliberately made `None` (no fixed 180s cap on long RLM work), so the
|
||||
//! #6511 ask to bound by wall clock instead of a count is still open.
|
||||
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
@@ -743,9 +747,13 @@ async fn run_rlm_turn_impl(
|
||||
}
|
||||
|
||||
/// An empty answer is never returned silently: without an error of its own,
|
||||
/// it gets one naming how the loop ended.
|
||||
/// it gets one naming how the loop ended. A deliberate `FINAL("")` is the
|
||||
/// model's answer, not a failure, and stays an empty answer with no error.
|
||||
fn require_answer_or_error(mut result: RlmTurnResult) -> RlmTurnResult {
|
||||
if result.answer.trim().is_empty() && result.error.is_none() {
|
||||
if result.termination != RlmTermination::Final
|
||||
&& result.answer.trim().is_empty()
|
||||
&& result.error.is_none()
|
||||
{
|
||||
result.error = Some(format!(
|
||||
"RLM ended ({:?}) after {} iteration(s) with an empty answer",
|
||||
result.termination, result.iterations
|
||||
@@ -1238,7 +1246,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn empty_answer_is_never_returned_without_an_error() {
|
||||
let result = require_answer_or_error(RlmTurnResult {
|
||||
let empty = |termination| RlmTurnResult {
|
||||
answer: " ".to_string(),
|
||||
iterations: 2,
|
||||
duration: Duration::ZERO,
|
||||
@@ -1247,13 +1255,19 @@ mod tests {
|
||||
routed_usage: Vec::new(),
|
||||
routed_usage_drop_records: Vec::new(),
|
||||
routed_usage_dropped_records: 0,
|
||||
termination: RlmTermination::Final,
|
||||
termination,
|
||||
trace: Vec::new(),
|
||||
total_rpcs: 0,
|
||||
});
|
||||
let error = result.error.expect("empty answer needs a reason");
|
||||
};
|
||||
let error = require_answer_or_error(empty(RlmTermination::NoCode))
|
||||
.error
|
||||
.expect("empty answer needs a reason");
|
||||
assert!(error.contains("empty answer"), "{error}");
|
||||
assert!(error.contains("Final"), "{error}");
|
||||
assert!(error.contains("NoCode"), "{error}");
|
||||
|
||||
// `FINAL("")` is an answer the model chose; callers keep getting "".
|
||||
let deliberate = require_answer_or_error(empty(RlmTermination::Final));
|
||||
assert!(deliberate.error.is_none(), "{:?}", deliberate.error);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -140,10 +140,11 @@ Rules for \"suggestions\":\n\
|
||||
- Anchor a suggestion only to lines that appear in the diff you were given, and never to a deleted line. If you are not sure of the exact line numbers, omit \"replacement\".\n\
|
||||
- A wrong replacement is worse than no replacement: it is one click from being merged. When in doubt, omit it.";
|
||||
|
||||
/// The system prompt shared by every structured review path (`review`
|
||||
/// tool and `codewhale review --pr`). Callers parse the reply with
|
||||
/// [`ReviewOutput::from_str`], which falls back to freeform text when a
|
||||
/// model ignores the JSON contract.
|
||||
/// The one review system prompt (#6510), shared by every review path: the
|
||||
/// `review` tool, `codewhale review --pr`, and `codewhale review` of a plain
|
||||
/// diff. Callers parse the reply with [`ReviewOutput::from_str`] or
|
||||
/// [`ReviewOutput::from_structured_str`], which fall back to freeform text
|
||||
/// when a model ignores the JSON contract.
|
||||
#[must_use]
|
||||
pub fn review_system_prompt() -> &'static str {
|
||||
REVIEW_SYSTEM_PROMPT
|
||||
|
||||
@@ -400,14 +400,7 @@ async fn plain_exec_runs_one_engine_turn_under_one_prompt_authority() {
|
||||
"{receipt}"
|
||||
);
|
||||
|
||||
let bodies: Vec<Value> = server
|
||||
.received_requests()
|
||||
.await
|
||||
.expect("request recording")
|
||||
.into_iter()
|
||||
.filter(|request| request.url.path() == "/v1/chat/completions")
|
||||
.map(|request| serde_json::from_slice(&request.body).expect("request body JSON"))
|
||||
.collect();
|
||||
let bodies = chat_bodies(&server).await;
|
||||
assert_eq!(bodies.len(), 2, "one model call per run: {bodies:#?}");
|
||||
for body in &bodies {
|
||||
let messages = body["messages"].to_string();
|
||||
@@ -426,3 +419,114 @@ async fn plain_exec_runs_one_engine_turn_under_one_prompt_authority() {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Chat-completions bodies the mock received, in order.
|
||||
async fn chat_bodies(server: &MockServer) -> Vec<Value> {
|
||||
server
|
||||
.received_requests()
|
||||
.await
|
||||
.expect("request recording")
|
||||
.into_iter()
|
||||
.filter(|request| request.url.path() == "/v1/chat/completions")
|
||||
.map(|request| serde_json::from_slice(&request.body).expect("request body JSON"))
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// #6510: a limit is not a tool grant. `--max-turns`, `--disallowed-tools`
|
||||
/// and `--append-system-prompt` used to put plain exec on the full tool
|
||||
/// catalog, so `exec --max-turns 1 "hi"` became a tool-using agent.
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn plain_exec_limits_do_not_grant_tools() {
|
||||
let server = start_mock_llm(answer_sse_with_usage("pong")).await;
|
||||
|
||||
for flags in [
|
||||
&["--max-turns", "1"][..],
|
||||
&["--disallowed-tools", "exec_shell"],
|
||||
&["--append-system-prompt", "Be brief."],
|
||||
] {
|
||||
let mut args = flags.to_vec();
|
||||
args.extend(["--model", TEST_MODEL, "answer briefly"]);
|
||||
let text = run_exec(&server, &args);
|
||||
assert_eq!(text.trim(), "pong", "{flags:?}: {text}");
|
||||
}
|
||||
|
||||
let bodies = chat_bodies(&server).await;
|
||||
assert_eq!(bodies.len(), 3, "one model call per run: {bodies:#?}");
|
||||
for body in &bodies {
|
||||
assert!(
|
||||
body.get("tools")
|
||||
.is_none_or(|tools| tools.as_array().is_some_and(Vec::is_empty)),
|
||||
"a limit flag must not offer tools: {body}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// #6510: plain exec now runs on the Engine, so an output-limit stop follows
|
||||
/// the Engine's one policy: the partial answer is kept, the model is asked to
|
||||
/// continue, and the run succeeds with the whole answer. The old direct call
|
||||
/// printed the partial answer and failed.
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn plain_exec_continues_past_an_output_limit_stop() {
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/v1/models"))
|
||||
.respond_with(json_response(json!({
|
||||
"object": "list",
|
||||
"data": [{ "id": TEST_MODEL, "object": "model" }]
|
||||
})))
|
||||
.mount(&server)
|
||||
.await;
|
||||
let truncated = [
|
||||
sse_chunk(json!({
|
||||
"id": "chatcmpl-cut",
|
||||
"object": "chat.completion.chunk",
|
||||
"model": TEST_MODEL,
|
||||
"choices": [{"index": 0, "delta": {"content": "first half"}, "finish_reason": null}]
|
||||
})),
|
||||
sse_chunk(json!({
|
||||
"id": "chatcmpl-cut",
|
||||
"object": "chat.completion.chunk",
|
||||
"model": TEST_MODEL,
|
||||
"choices": [{"index": 0, "delta": {}, "finish_reason": "length"}]
|
||||
})),
|
||||
"data: [DONE]\n\n".to_string(),
|
||||
]
|
||||
.join("");
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/v1/chat/completions"))
|
||||
.respond_with(sse_response(truncated))
|
||||
.up_to_n_times(1)
|
||||
.with_priority(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/v1/chat/completions"))
|
||||
.respond_with(sse_response(answer_sse_without_usage(" second half")))
|
||||
.with_priority(2)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let json_stdout = run_exec(
|
||||
&server,
|
||||
&["--json", "--model", TEST_MODEL, "answer briefly"],
|
||||
);
|
||||
let receipt: Value = serde_json::from_str(&json_stdout)
|
||||
.unwrap_or_else(|err| panic!("--json receipt should parse: {err}\n{json_stdout}"));
|
||||
assert_eq!(receipt["mode"], "one-shot", "{receipt}");
|
||||
assert_eq!(receipt["success"], true, "{receipt}");
|
||||
let output = receipt["output"].as_str().unwrap_or_default();
|
||||
assert!(
|
||||
output.contains("first half") && output.contains("second half"),
|
||||
"{receipt}"
|
||||
);
|
||||
|
||||
let bodies = chat_bodies(&server).await;
|
||||
assert_eq!(bodies.len(), 2, "one continuation request: {bodies:#?}");
|
||||
assert!(
|
||||
bodies[1]["messages"]
|
||||
.to_string()
|
||||
.contains("stopped generation at its output limit"),
|
||||
"the continuation names the truncation: {}",
|
||||
bodies[1]["messages"]
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user