Codex/local parity#1
Conversation
🤖 Augment PR SummarySummary: This PR moves the repo toward Codex/local parity by introducing deterministic agent-loop/runtime plumbing, layered .agents config parsing fixtures, and a unified verification gate. Changes:
Technical Notes: The server now uses deterministic backend dispatch based on prompt triggers ("inspect" and ".agents config"), and CI runs the same 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| let mut request_line = request.to_string(); | ||
| request_line.push('\n'); | ||
|
|
||
| let _ = state.stdin.write_all(request_line.as_bytes()); |
There was a problem hiding this comment.
send_stdio_session_jsonrpc_request ignores the write_all (and read_line) results, which can mask I/O failures and potentially block waiting for a response that will never arrive if the child didn’t receive the request. Consider surfacing these errors so failures are deterministic and diagnosable when the stdio-session process closes stdin/stdout early.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct RetryPolicy { | ||
| pub max_attempts: usize, |
There was a problem hiding this comment.
RetryPolicy exposes max_attempts publicly, but execute_with_retry_and_fallback later calls .expect("retry policy guarantees..."), which will panic if a caller constructs RetryPolicy { max_attempts: 0 }. This makes an invalid-but-publicly-constructible value a runtime crash instead of a regular error.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| }, | ||
| &inspect_tool_registry, | ||
| ) | ||
| .expect("default tool-adapter runtime prompt execution should not fail"); |
There was a problem hiding this comment.
This .expect("default tool-adapter runtime prompt execution should not fail") will panic the server if tool-adapter execution ever returns an error (e.g. once adapters become process/network-backed), instead of returning a 500 like the layering path does. That turns an execution error into a full-process crash on a user prompt.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| let addr = parse_bind_addr(Some("0.0.0.0:4000".into())).unwrap(); | ||
| assert_eq!(addr, "0.0.0.0:4000".parse().unwrap()); | ||
| assert_eq!( | ||
| server_addr_from_env().unwrap(), |
There was a problem hiding this comment.
This test assumes DOTAGENTS_BIND is unset; if it’s set in the environment running tests (CI, developer shell), server_addr_from_env() won’t return DEFAULT_BIND_ADDR and the test can fail. Similar environment coupling exists for tests using app() which reads DOTAGENTS_LAYERING_MCP_STDIO_SESSION_COMMAND* for the default prompt executor.
Severity: medium
Other Locations
crates/dotagents-server/src/lib.rs:697
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| let sessions = store.list_sessions().await; | ||
| assert_eq!(sessions.len(), 2); | ||
| assert_eq!(sessions[0].id, first.id); |
There was a problem hiding this comment.
This assertion relies on a deterministic ordering from list_sessions() which sorts by updated_at; if two sessions end up with identical timestamps, the sort order is unspecified and this test can become flaky. It may be safer for the test to avoid depending on ordering when keys can tie.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
1 participant
No description provided.