review classifier: every ACP run gets one shared, empty cwd
opencode writes the session's cwd into the system message's environment
block ("Working directory: …", "Workspace root folder: …"), ahead of the
PR's user message. Each ACP classification ran with its per-call scratch
directory, /tmp/review-classifier-<pid>-<counter>, as cwd, so every call
sent a different system message and the provider's prompt cache could not
match past it.
The session's cwd is now one fixed directory, <tmp>/review-classifier-acp-cwd,
shared by every run. HOME (the config, system.md and opencode's own state)
stays a directory per run, removed after it, as before. With no tool and
no project config nothing writes into the cwd: a live opencode 1.15.10 run
through initialize, session/new and a prompt left it empty, with its state
all under HOME. So concurrent runs can share it, and no state carries from
one run to the next.
Refs #5245
This commit is contained in:
parent
345c6e3731
commit
2fb6b39f9f
4 changed files with 70 additions and 11 deletions
|
|
@ -124,7 +124,11 @@ resume, no tools (`--tools ""`), no MCP server (`--strict-mcp-config` and no
|
||||||
config) and an empty directory of its own.
|
config) and an empty directory of its own.
|
||||||
|
|
||||||
On the ACP runtime, each classification is one prompt on a new `opencode acp`
|
On the ACP runtime, each classification is one prompt on a new `opencode acp`
|
||||||
session in an empty directory of its own. The service drives it through
|
session. Its config and opencode's state live in a directory of its own,
|
||||||
|
removed after it. Its working directory is one empty directory that every
|
||||||
|
classification shares: opencode writes that path into the system message, so
|
||||||
|
one path keeps the system message the same from one classification to the
|
||||||
|
next, and the provider's prompt cache can hold. The service drives it through
|
||||||
[`hive-runtime`](../../hive-runtime/README.md)'s `AcpRuntime`, as the subagent
|
[`hive-runtime`](../../hive-runtime/README.md)'s `AcpRuntime`, as the subagent
|
||||||
daemon does a one-run subagent, and stops the agent when the classification
|
daemon does a one-run subagent, and stops the agent when the classification
|
||||||
ends. The agent has no tool. opencode runs some tools, `read` among them,
|
ends. The agent has no tool. opencode runs some tools, `read` among them,
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,8 @@
|
||||||
//! One classification on the ACP runtime: `opencode acp`, driven through
|
//! One classification on the ACP runtime: `opencode acp`, driven through
|
||||||
//! [`hive_runtime::AcpRuntime`] for one prompt on a new session, in a
|
//! [`hive_runtime::AcpRuntime`] for one prompt on a new session. Its config,
|
||||||
//! directory made for it and removed after it.
|
//! system prompt and opencode's state live in a directory made for it and
|
||||||
|
//! removed after it, its `HOME`; its working directory is [`Acp::cwd`],
|
||||||
|
//! shared by every run.
|
||||||
//!
|
//!
|
||||||
//! The agent has no tool at all. opencode runs some tools, `read` among them,
|
//! The agent has no tool at all. opencode runs some tools, `read` among them,
|
||||||
//! without asking, so a deny-all permission policy alone would never see them:
|
//! without asking, so a deny-all permission policy alone would never see them:
|
||||||
|
|
@ -68,6 +70,11 @@ pub struct Acp<S> {
|
||||||
/// [`CANCEL_WAIT`] outside the tests.
|
/// [`CANCEL_WAIT`] outside the tests.
|
||||||
pub cancel_wait: Duration,
|
pub cancel_wait: Duration,
|
||||||
pub cost: S,
|
pub cost: S,
|
||||||
|
/// The empty working directory every run's session gets. opencode writes
|
||||||
|
/// it into the system message, so one path for every run keeps that
|
||||||
|
/// message the same and the provider's prompt cache usable; with no tool
|
||||||
|
/// and no project config, nothing writes into it.
|
||||||
|
pub cwd: PathBuf,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl<S: CostSink> crate::run::Model for Acp<S> {
|
impl<S: CostSink> crate::run::Model for Acp<S> {
|
||||||
|
|
@ -97,6 +104,7 @@ impl<S: CostSink> Acp<S> {
|
||||||
let config_file = dir.join("opencode.json");
|
let config_file = dir.join("opencode.json");
|
||||||
tokio::fs::create_dir_all(dir)
|
tokio::fs::create_dir_all(dir)
|
||||||
.await
|
.await
|
||||||
|
.and(tokio::fs::create_dir_all(&self.cwd).await)
|
||||||
.and(tokio::fs::write(&system, crate::prompt::SYSTEM).await)
|
.and(tokio::fs::write(&system, crate::prompt::SYSTEM).await)
|
||||||
.and(tokio::fs::write(&config_file, opencode_config(provider).to_string()).await)
|
.and(tokio::fs::write(&config_file, opencode_config(provider).to_string()).await)
|
||||||
.map_err(|e| format!("preparing the run's directory: {e}"))?;
|
.map_err(|e| format!("preparing the run's directory: {e}"))?;
|
||||||
|
|
@ -112,7 +120,7 @@ impl<S: CostSink> Acp<S> {
|
||||||
.ok_or("the ACP runtime offers no cancel handle")?;
|
.ok_or("the ACP runtime offers no cancel handle")?;
|
||||||
let config = Config {
|
let config = Config {
|
||||||
effort: run.effort.map(|e| e.as_str().to_owned()),
|
effort: run.effort.map(|e| e.as_str().to_owned()),
|
||||||
cwd: Some(dir.to_owned()),
|
cwd: Some(self.cwd.clone()),
|
||||||
idle_timeout: Some(IDLE_TIMEOUT),
|
idle_timeout: Some(IDLE_TIMEOUT),
|
||||||
..Config::default()
|
..Config::default()
|
||||||
};
|
};
|
||||||
|
|
@ -271,8 +279,10 @@ mod tests {
|
||||||
|
|
||||||
/// An ACP agent in plain `sh`, logging to `<log>.*`. It writes its
|
/// An ACP agent in plain `sh`, logging to `<log>.*`. It writes its
|
||||||
/// environment and copies of its config and the system prompt file
|
/// environment and copies of its config and the system prompt file
|
||||||
/// beside it at start, appends each method it is sent to
|
/// beside it and appends its `$HOME` to `<log>.homes` at start, appends
|
||||||
/// `<log>.methods` and each prompt request to `<log>.prompts`, and answers a prompt with a text chunk
|
/// each method it is sent to `<log>.methods`, each `session/new` request
|
||||||
|
/// to `<log>.sessions` and each prompt request to `<log>.prompts`, and
|
||||||
|
/// answers a prompt with a text chunk
|
||||||
/// holding a verdict, then `end_turn` with 1000 input and 200 output
|
/// holding a verdict, then `end_turn` with 1000 input and 200 output
|
||||||
/// tokens. A prompt holding `ASK-READ` first asks permission for a
|
/// tokens. A prompt holding `ASK-READ` first asks permission for a
|
||||||
/// `read` and appends the reply to `<log>.permission`; one holding
|
/// `read` and appends the reply to `<log>.permission`; one holding
|
||||||
|
|
@ -283,6 +293,7 @@ log="$1"
|
||||||
env > "$log.env"
|
env > "$log.env"
|
||||||
cp "$OPENCODE_CONFIG" "$log.config"
|
cp "$OPENCODE_CONFIG" "$log.config"
|
||||||
cp "${OPENCODE_CONFIG%/*}/system.md" "$log.system"
|
cp "${OPENCODE_CONFIG%/*}/system.md" "$log.system"
|
||||||
|
printf '%s\n' "$HOME" >> "$log.homes"
|
||||||
stalled=
|
stalled=
|
||||||
while IFS= read -r line; do
|
while IFS= read -r line; do
|
||||||
id=${line#*\"id\":}; id=${id%%[,\}]*}
|
id=${line#*\"id\":}; id=${id%%[,\}]*}
|
||||||
|
|
@ -292,6 +303,7 @@ while IFS= read -r line; do
|
||||||
initialize)
|
initialize)
|
||||||
printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{"mcpCapabilities":{"http":true}}}}\n' "$id" ;;
|
printf '{"jsonrpc":"2.0","id":%s,"result":{"protocolVersion":1,"agentCapabilities":{"mcpCapabilities":{"http":true}}}}\n' "$id" ;;
|
||||||
session/new)
|
session/new)
|
||||||
|
printf '%s\n' "$line" >> "$log.sessions"
|
||||||
printf '{"jsonrpc":"2.0","id":%s,"result":{"sessionId":"s1"}}\n' "$id" ;;
|
printf '{"jsonrpc":"2.0","id":%s,"result":{"sessionId":"s1"}}\n' "$id" ;;
|
||||||
session/cancel)
|
session/cancel)
|
||||||
[ -n "$stalled" ] && printf '{"jsonrpc":"2.0","id":%s,"result":{"stopReason":"cancelled"}}\n' "$stalled" ;;
|
[ -n "$stalled" ] && printf '{"jsonrpc":"2.0","id":%s,"result":{"stopReason":"cancelled"}}\n' "$stalled" ;;
|
||||||
|
|
@ -338,6 +350,7 @@ done
|
||||||
deadline,
|
deadline,
|
||||||
cancel_wait: Duration::from_millis(500),
|
cancel_wait: Duration::from_millis(500),
|
||||||
cost: Arc::clone(&costs),
|
cost: Arc::clone(&costs),
|
||||||
|
cwd: dir.join("cwd"),
|
||||||
};
|
};
|
||||||
(acp, costs)
|
(acp, costs)
|
||||||
}
|
}
|
||||||
|
|
@ -507,4 +520,45 @@ done
|
||||||
assert!(acp.ask("classify", "p", &claude).await.is_err());
|
assert!(acp.ask("classify", "p", &claude).await.is_err());
|
||||||
assert_eq!(read(dir.path(), "methods"), "", "no agent started");
|
assert_eq!(read(dir.path(), "methods"), "", "no agent started");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The cwd of each session the agent was asked for, in order.
|
||||||
|
fn cwds(dir: &Path) -> Vec<String> {
|
||||||
|
read(dir, "sessions")
|
||||||
|
.lines()
|
||||||
|
.map(|l| {
|
||||||
|
let request: Value = serde_json::from_str(l).expect("JSON");
|
||||||
|
request["params"]["cwd"].as_str().expect("a cwd").to_owned()
|
||||||
|
})
|
||||||
|
.collect()
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn every_run_gets_the_same_cwd_even_at_once() {
|
||||||
|
let dir = tempfile::tempdir().expect("a dir");
|
||||||
|
let (acp, _) = acp(dir.path(), Duration::from_mins(1));
|
||||||
|
|
||||||
|
let (run_one, run_two) = (run(), run());
|
||||||
|
let (one, two) = tokio::join!(acp.ask("one", "p", &run_one), acp.ask("two", "p", &run_two));
|
||||||
|
one.expect("an answer");
|
||||||
|
two.expect("an answer");
|
||||||
|
acp.ask("three", "p", &run()).await.expect("an answer");
|
||||||
|
let cwd = dir.path().join("cwd").display().to_string();
|
||||||
|
assert_eq!(cwds(dir.path()), [cwd.clone(), cwd.clone(), cwd]);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn each_run_has_a_home_of_its_own_removed_after_it() {
|
||||||
|
let dir = tempfile::tempdir().expect("a dir");
|
||||||
|
let (acp, _) = acp(dir.path(), Duration::from_mins(1));
|
||||||
|
|
||||||
|
acp.ask("one", "p", &run()).await.expect("an answer");
|
||||||
|
acp.ask("two", "p", &run()).await.expect("an answer");
|
||||||
|
let homes = read(dir.path(), "homes");
|
||||||
|
let homes: Vec<&str> = homes.lines().collect();
|
||||||
|
assert_eq!(homes.len(), 2, "{homes:?}");
|
||||||
|
assert_ne!(homes[0], homes[1]);
|
||||||
|
for home in homes {
|
||||||
|
assert!(!Path::new(home).exists(), "{home}");
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -70,6 +70,7 @@ async fn serve() -> Result<()> {
|
||||||
deadline: swarm_queue_client::review_classifier::CLASSIFICATION_DEADLINE,
|
deadline: swarm_queue_client::review_classifier::CLASSIFICATION_DEADLINE,
|
||||||
cancel_wait: acp::CANCEL_WAIT,
|
cancel_wait: acp::CANCEL_WAIT,
|
||||||
cost: cost::Otel,
|
cost: cost::Otel,
|
||||||
|
cwd: std::env::temp_dir().join("review-classifier-acp-cwd"),
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
let secret = client_secret_with_retry(&client_id).await?;
|
let secret = client_secret_with_retry(&client_id).await?;
|
||||||
|
|
|
||||||
|
|
@ -3,11 +3,11 @@
|
||||||
//!
|
//!
|
||||||
//! On the API key runtime each classification is one `claude --print` with
|
//! On the API key runtime each classification is one `claude --print` with
|
||||||
//! no session, no tools and no MCP server; on the ACP runtime it is one
|
//! no session, no tools and no MCP server; on the ACP runtime it is one
|
||||||
//! prompt on a new ACP session ([`crate::acp`]). Either way it runs in a
|
//! prompt on a new ACP session ([`crate::acp`]). Either way its files live
|
||||||
//! directory made for it and removed after it, so nothing carries from one
|
//! in a directory made for it and removed after it, so nothing carries from
|
||||||
//! run to the next. The key is read from the store for every run and reaches
|
//! one run to the next. The key is read from the store for every run and
|
||||||
//! only the child's environment, which keeps nothing of the daemon's beyond
|
//! reaches only the child's environment, which keeps nothing of the daemon's
|
||||||
//! [`crate::child_env`]'s allowlist.
|
//! beyond [`crate::child_env`]'s allowlist.
|
||||||
|
|
||||||
use std::future::Future;
|
use std::future::Future;
|
||||||
use std::path::{Path, PathBuf};
|
use std::path::{Path, PathBuf};
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue