mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
Replace bare unwrap() with documented expect() across production runtim… (#415)
Audit and remediation pass enforcing the project's
no-panic-in-production policy. Every `unwrap()` on a mutex/RwLock in
reachable runtime code is replaced with `expect()` carrying a message
that explains *why* the lock cannot be poisoned (no code panics while
holding it). Bare `unreachable!()` and `panic!()` calls are updated with
messages that name the invariant being asserted. One genuine bug is
fixed in the process.
## What changed
**`unwrap()` → `expect()` on locks** (`fabro-core`, `fabro-oauth`,
`fabro-util`, `fabro-workflow/*`, `fabro-server`): Every
`Mutex`/`RwLock` `.unwrap()` in production paths now carries the
standard justification pattern: `"<name> mutex/RwLock should not be
poisoned: no code panics while holding this lock"`.
**`unreachable!()` and `panic!()` message quality**: Bare
`unreachable!()` calls in `subagent.rs`, `wait.rs`, `condition.rs`,
`event/convert.rs`, and `server.rs` now name the structural invariant
(e.g. "outer match arm already verified…"). The `panic!` in `tools.rs`
now includes the offending name and the expected format, making it
actionable.
**`sha_newtype` / `short_sha_newtype` in `run_files.rs` — actual bug
fix**: These helpers previously called `unwrap_or_else(|e| panic!(…))`
on git output, meaning a malformed SHA from a real git subprocess would
panic in a request handler. They now return `Result<T, ApiError>` and
propagate errors to callers, which in turn propagate with `?`. This is
the only change that alters observable behavior under failure.
**Demo-only panics in `fabro-server/src/demo/mod.rs`**: Panic messages
updated to clarify that these paths operate on hardcoded compile-time
constants, so the panic is a programming-error guard rather than a
runtime failure guard.
## Design note
The lock-poisoning `expect` messages all follow a single template so
reviewers can quickly verify the claim: if you ever add code that can
panic inside a lock guard scope, the message becomes a lie and that must
be caught in review. The uniformity is intentional.
### Fabro Details
<details>
<summary>Ran 0 stages in 64m 54s for $14.28</summary>
| Stage | Duration | Cost | Retries |
|---|---|---|---|
| **Total** | **64m 54s** | **$14.28** | **0** |
</details>
<details>
<summary>Ran <code>Goal.fabro</code> (4 nodes and 5 edges)</summary>
```dot
digraph Goal {
graph [
goal="Complete the user-provided goal",
rankdir=LR,
max_node_visits=30
]
start [shape=Mdiamond, label="Start"]
exit [shape=Msquare, label="Exit"]
work [
label="Work",
thread_id="goal",
fidelity="full",
max_visits=12,
prompt="@prompts/continue.md"
]
audit [
label="Completion Audit",
thread_id="goal",
fidelity="full",
goal_gate=true,
retry_target="work",
output_schema="routing",
output_retries=2,
max_visits=12,
prompt="@prompts/audit.md"
]
start -> work -> audit
audit -> exit [label="Done", condition="outcome=succeeded"]
audit -> work [label="Continue", condition="outcome=failed || preferred_label=Continue"]
audit -> work [label="No clear verdict"]
}
```
</details>
⚒️ Generated with [Fabro](https://fabro.sh)
---------
Co-authored-by: Fabro <noreply@fabro.sh>
Co-authored-by: Bryan Helmkamp <bryan@brynary.com>
This commit is contained in:
parent
71c06c1bc4
commit
01892185ff
22 changed files with 274 additions and 91 deletions
|
|
@ -234,7 +234,7 @@ impl SubAgentManager {
|
|||
|
||||
match &agent.status {
|
||||
SubAgentStatus::Finished(result) => result.clone(),
|
||||
_ => unreachable!(),
|
||||
_ => unreachable!("agent status was just assigned to Finished on the line above"),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1030,10 +1030,23 @@ async fn setup_github_app(
|
|||
.route(
|
||||
"/callback",
|
||||
get(move |Query(params): Query<CallbackParams>| async move {
|
||||
if let Some(tx) = code_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = code_tx
|
||||
.lock()
|
||||
.expect(
|
||||
"code_tx mutex is never poisoned: no code panics while holding this lock",
|
||||
)
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(params.code);
|
||||
}
|
||||
if let Some(tx) = shutdown_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = shutdown_tx
|
||||
.lock()
|
||||
.expect(
|
||||
"shutdown_tx mutex is never poisoned: no code panics while holding this \
|
||||
lock",
|
||||
)
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(());
|
||||
}
|
||||
Html(r#"<!DOCTYPE html>
|
||||
|
|
|
|||
|
|
@ -107,7 +107,9 @@ fn print_human_output(
|
|||
RunStatus::Failed { .. } => (&styles.bold_red, "Failed"),
|
||||
RunStatus::Dead => (&styles.bold_red, "Dead"),
|
||||
// Poll loop only breaks on is_terminal() which is the three arms above.
|
||||
_ => unreachable!(),
|
||||
_ => unreachable!(
|
||||
"print_human_output is only called after is_terminal(), which returns true only for Succeeded, Failed, and Dead"
|
||||
),
|
||||
};
|
||||
let status_display = style.apply_to(label);
|
||||
|
||||
|
|
|
|||
|
|
@ -20,11 +20,18 @@ impl Context {
|
|||
}
|
||||
|
||||
pub fn set(&self, key: impl Into<String>, value: Value) {
|
||||
self.values.write().unwrap().insert(key.into(), value);
|
||||
self.values
|
||||
.write()
|
||||
.expect("context RwLock should not be poisoned: no code panics while holding this lock")
|
||||
.insert(key.into(), value);
|
||||
}
|
||||
|
||||
pub fn get(&self, key: &str) -> Option<Value> {
|
||||
self.values.read().unwrap().get(key).cloned()
|
||||
self.values
|
||||
.read()
|
||||
.expect("context RwLock should not be poisoned: no code panics while holding this lock")
|
||||
.get(key)
|
||||
.cloned()
|
||||
}
|
||||
|
||||
pub fn get_string(&self, key: &str, default: &str) -> String {
|
||||
|
|
@ -34,14 +41,19 @@ impl Context {
|
|||
}
|
||||
|
||||
pub fn apply_updates(&self, updates: &HashMap<String, Value>) {
|
||||
let mut values = self.values.write().unwrap();
|
||||
let mut values = self.values.write().expect(
|
||||
"context RwLock should not be poisoned: no code panics while holding this lock",
|
||||
);
|
||||
for (k, v) in updates {
|
||||
values.insert(k.clone(), v.clone());
|
||||
}
|
||||
}
|
||||
|
||||
pub fn snapshot(&self) -> HashMap<String, Value> {
|
||||
self.values.read().unwrap().clone()
|
||||
self.values
|
||||
.read()
|
||||
.expect("context RwLock should not be poisoned: no code panics while holding this lock")
|
||||
.clone()
|
||||
}
|
||||
|
||||
/// Deep copy for parallel branch isolation.
|
||||
|
|
|
|||
|
|
@ -301,7 +301,9 @@ fn translate_messages(messages: &[Message]) -> Vec<ChatMessage> {
|
|||
Role::System | Role::Developer => "system",
|
||||
Role::User => "user",
|
||||
Role::Assistant => "assistant",
|
||||
Role::Tool => unreachable!(),
|
||||
Role::Tool => unreachable!(
|
||||
"Role::Tool is handled in the early-return branch above this match"
|
||||
),
|
||||
};
|
||||
|
||||
let mut tool_calls: Vec<ChatToolCall> = Vec::new();
|
||||
|
|
|
|||
|
|
@ -39,10 +39,15 @@ impl Tool {
|
|||
/// # Panics
|
||||
///
|
||||
/// Panics if the tool name is invalid (see [`validate_tool_name`]).
|
||||
/// Tool names are always hardcoded string literals in this codebase; this
|
||||
/// guards against programming errors where a constant would fail
|
||||
/// validation.
|
||||
#[must_use]
|
||||
pub fn passive(name: &str, description: &str, parameters: serde_json::Value) -> Self {
|
||||
if let Err(e) = validate_tool_name(name) {
|
||||
panic!("Invalid tool name: {e}");
|
||||
panic!(
|
||||
"tool name `{name}` must be a valid identifier ([a-zA-Z][a-zA-Z0-9_]*, ≤64 chars): {e}"
|
||||
);
|
||||
}
|
||||
Self {
|
||||
definition: ToolDefinition {
|
||||
|
|
@ -59,6 +64,9 @@ impl Tool {
|
|||
/// # Panics
|
||||
///
|
||||
/// Panics if the tool name is invalid (see [`validate_tool_name`]).
|
||||
/// Tool names are always hardcoded string literals in this codebase; this
|
||||
/// guards against programming errors where a constant would fail
|
||||
/// validation.
|
||||
pub fn active<F, Fut>(
|
||||
name: &str,
|
||||
description: &str,
|
||||
|
|
@ -70,7 +78,9 @@ impl Tool {
|
|||
Fut: Future<Output = Result<serde_json::Value, String>> + Send + 'static,
|
||||
{
|
||||
if let Err(e) = validate_tool_name(name) {
|
||||
panic!("Invalid tool name: {e}");
|
||||
panic!(
|
||||
"tool name `{name}` must be a valid identifier ([a-zA-Z][a-zA-Z0-9_]*, ≤64 chars): {e}"
|
||||
);
|
||||
}
|
||||
Self {
|
||||
definition: ToolDefinition {
|
||||
|
|
@ -319,7 +329,7 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
#[should_panic(expected = "Invalid tool name")]
|
||||
#[should_panic(expected = "must be a valid identifier")]
|
||||
fn passive_tool_panics_on_invalid_name() {
|
||||
let _ = Tool::passive(
|
||||
"1invalid",
|
||||
|
|
@ -329,7 +339,7 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
#[should_panic(expected = "Invalid tool name")]
|
||||
#[should_panic(expected = "must be a valid identifier")]
|
||||
fn active_tool_panics_on_invalid_name() {
|
||||
Tool::active(
|
||||
"my-tool",
|
||||
|
|
|
|||
|
|
@ -293,7 +293,10 @@ impl CallbackHandle {
|
|||
}
|
||||
|
||||
pub fn shutdown(&self) {
|
||||
if let Some(tx) = self.shutdown_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = self.shutdown_tx.lock()
|
||||
.expect("oauth shutdown_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(());
|
||||
}
|
||||
}
|
||||
|
|
@ -486,10 +489,16 @@ pub async fn start_callback_server(
|
|||
let desc = params
|
||||
.error_description
|
||||
.unwrap_or_else(|| error.clone());
|
||||
if let Some(tx) = code_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = code_tx.lock()
|
||||
.expect("oauth code_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(Err(desc.clone()));
|
||||
}
|
||||
if let Some(tx) = shutdown_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = shutdown_tx.lock()
|
||||
.expect("oauth shutdown_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(());
|
||||
}
|
||||
return (
|
||||
|
|
@ -499,10 +508,16 @@ pub async fn start_callback_server(
|
|||
}
|
||||
|
||||
let Some(code) = params.code else {
|
||||
if let Some(tx) = code_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = code_tx.lock()
|
||||
.expect("oauth code_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(Err("No authorization code received".to_string()));
|
||||
}
|
||||
if let Some(tx) = shutdown_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = shutdown_tx.lock()
|
||||
.expect("oauth shutdown_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(());
|
||||
}
|
||||
return (
|
||||
|
|
@ -513,10 +528,16 @@ pub async fn start_callback_server(
|
|||
);
|
||||
};
|
||||
|
||||
if let Some(tx) = code_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = code_tx.lock()
|
||||
.expect("oauth code_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(Ok(code));
|
||||
}
|
||||
if let Some(tx) = shutdown_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = shutdown_tx.lock()
|
||||
.expect("oauth shutdown_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(());
|
||||
}
|
||||
(StatusCode::OK, Html(callback_success_page()))
|
||||
|
|
@ -576,13 +597,19 @@ pub async fn start_callback_server_with_errors(
|
|||
let error_description = params
|
||||
.error_description
|
||||
.unwrap_or_else(|| error_code.clone());
|
||||
if let Some(tx) = callback_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = callback_tx.lock()
|
||||
.expect("oauth callback_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(Err(CallbackFailure {
|
||||
error_code: error_code.clone(),
|
||||
error_description: error_description.clone(),
|
||||
}));
|
||||
}
|
||||
if let Some(tx) = route_shutdown_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = route_shutdown_tx.lock()
|
||||
.expect("oauth route_shutdown_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(());
|
||||
}
|
||||
return (
|
||||
|
|
@ -592,13 +619,19 @@ pub async fn start_callback_server_with_errors(
|
|||
}
|
||||
|
||||
let Some(code) = params.code else {
|
||||
if let Some(tx) = callback_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = callback_tx.lock()
|
||||
.expect("oauth callback_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(Err(CallbackFailure {
|
||||
error_code: "invalid_request".to_string(),
|
||||
error_description: "No authorization code received".to_string(),
|
||||
}));
|
||||
}
|
||||
if let Some(tx) = route_shutdown_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = route_shutdown_tx.lock()
|
||||
.expect("oauth route_shutdown_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(());
|
||||
}
|
||||
return (
|
||||
|
|
@ -609,10 +642,16 @@ pub async fn start_callback_server_with_errors(
|
|||
);
|
||||
};
|
||||
|
||||
if let Some(tx) = callback_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = callback_tx.lock()
|
||||
.expect("oauth callback_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(Ok(CallbackSuccess { code }));
|
||||
}
|
||||
if let Some(tx) = route_shutdown_tx.lock().unwrap().take() {
|
||||
if let Some(tx) = route_shutdown_tx.lock()
|
||||
.expect("oauth route_shutdown_tx mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take()
|
||||
{
|
||||
let _ = tx.send(());
|
||||
}
|
||||
(StatusCode::OK, Html(callback_success_page()))
|
||||
|
|
|
|||
|
|
@ -243,7 +243,9 @@ where
|
|||
T: TryFrom<String>,
|
||||
<T as TryFrom<String>>::Error: std::fmt::Display,
|
||||
{
|
||||
T::try_from(sha.to_string()).unwrap_or_else(|err| panic!("invalid demo SHA `{sha}`: {err}"))
|
||||
T::try_from(sha.to_string()).unwrap_or_else(|err| {
|
||||
panic!("demo SHA `{sha}` is a hardcoded constant and must match the hex pattern: {err}")
|
||||
})
|
||||
}
|
||||
|
||||
fn short_sha_newtype<T>(sha: &str) -> T
|
||||
|
|
@ -252,8 +254,11 @@ where
|
|||
<T as TryFrom<String>>::Error: std::fmt::Display,
|
||||
{
|
||||
let short = sha.chars().take(7).collect::<String>();
|
||||
T::try_from(short.clone())
|
||||
.unwrap_or_else(|err| panic!("invalid demo short SHA `{short}`: {err}"))
|
||||
T::try_from(short.clone()).unwrap_or_else(|err| {
|
||||
panic!(
|
||||
"demo short SHA `{short}` is a hardcoded constant and must match the hex pattern: {err}"
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
fn demo_run_files() -> PaginatedRunFileList {
|
||||
|
|
@ -1128,7 +1133,7 @@ mod runs {
|
|||
labels: labels(entries),
|
||||
lifecycle: RunLifecycle {
|
||||
status: parse_run_status(status, status_reason)
|
||||
.unwrap_or_else(|| panic!("invalid demo run status: {status}")),
|
||||
.unwrap_or_else(|| panic!("demo run status `{status}` is a hardcoded constant and must be a valid RunStatus variant")),
|
||||
approval: None,
|
||||
pending_control,
|
||||
queue_position: None,
|
||||
|
|
|
|||
|
|
@ -345,8 +345,8 @@ async fn materialize_run_commits(
|
|||
data: commits,
|
||||
meta: RunCommitsMeta {
|
||||
source: RunCommitsMetaSource::Sandbox,
|
||||
base_sha: sha_newtype::<RunCommitsMetaBaseSha>(&base_sha),
|
||||
head_sha: sha_newtype::<RunCommitsMetaHeadSha>(&head_sha),
|
||||
base_sha: sha_newtype::<RunCommitsMetaBaseSha>(&base_sha)?,
|
||||
head_sha: sha_newtype::<RunCommitsMetaHeadSha>(&head_sha)?,
|
||||
limit: NonZeroU64::new(limit).expect("commit limit is non-zero"),
|
||||
total_returned,
|
||||
truncated,
|
||||
|
|
@ -410,15 +410,17 @@ fn parse_git_log_commit(record: &str) -> std::result::Result<RunCommit, ApiError
|
|||
let (subject, body) = split_commit_message(&message);
|
||||
let parents = parents
|
||||
.split_whitespace()
|
||||
.map(|parent| RunCommitParent {
|
||||
sha: sha_newtype::<RunCommitParentSha>(parent),
|
||||
short_sha: short_sha_newtype::<RunCommitParentShortSha>(parent),
|
||||
.map(|parent| {
|
||||
Ok(RunCommitParent {
|
||||
sha: sha_newtype::<RunCommitParentSha>(parent)?,
|
||||
short_sha: short_sha_newtype::<RunCommitParentShortSha>(parent)?,
|
||||
})
|
||||
})
|
||||
.collect();
|
||||
.collect::<std::result::Result<Vec<_>, ApiError>>()?;
|
||||
|
||||
Ok(RunCommit {
|
||||
sha: sha_newtype::<RunCommitSha>(sha),
|
||||
short_sha: short_sha_newtype::<RunCommitShortSha>(sha),
|
||||
sha: sha_newtype::<RunCommitSha>(sha)?,
|
||||
short_sha: short_sha_newtype::<RunCommitShortSha>(sha)?,
|
||||
parents,
|
||||
author: RunCommitPerson {
|
||||
name: author_name.to_string(),
|
||||
|
|
@ -434,7 +436,11 @@ fn parse_git_log_commit(record: &str) -> std::result::Result<RunCommit, ApiError
|
|||
body,
|
||||
message: message.clone(),
|
||||
trailers: parse_commit_trailers(&message),
|
||||
tree_sha: (!tree_sha.is_empty()).then(|| sha_newtype::<RunCommitTreeSha>(tree_sha)),
|
||||
tree_sha: if tree_sha.is_empty() {
|
||||
None
|
||||
} else {
|
||||
Some(sha_newtype::<RunCommitTreeSha>(tree_sha)?)
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
|
|
@ -472,23 +478,29 @@ fn parse_git_date(value: &str) -> Option<chrono::DateTime<chrono::Utc>> {
|
|||
.map(|d| d.with_timezone(&chrono::Utc))
|
||||
}
|
||||
|
||||
fn sha_newtype<T>(sha: &str) -> T
|
||||
fn sha_newtype<T>(sha: &str) -> std::result::Result<T, ApiError>
|
||||
where
|
||||
T: TryFrom<String>,
|
||||
<T as TryFrom<String>>::Error: std::fmt::Display,
|
||||
{
|
||||
T::try_from(sha.to_string())
|
||||
.unwrap_or_else(|err| panic!("invalid generated SHA `{sha}`: {err}"))
|
||||
T::try_from(sha.to_string()).map_err(|err| {
|
||||
ApiError::bad_request(format!(
|
||||
"git returned a SHA that did not match expected hex pattern: `{sha}`: {err}"
|
||||
))
|
||||
})
|
||||
}
|
||||
|
||||
fn short_sha_newtype<T>(sha: &str) -> T
|
||||
fn short_sha_newtype<T>(sha: &str) -> std::result::Result<T, ApiError>
|
||||
where
|
||||
T: TryFrom<String>,
|
||||
<T as TryFrom<String>>::Error: std::fmt::Display,
|
||||
{
|
||||
let short = sha.chars().take(7).collect::<String>();
|
||||
T::try_from(short.clone())
|
||||
.unwrap_or_else(|err| panic!("invalid generated short SHA `{short}`: {err}"))
|
||||
T::try_from(short.clone()).map_err(|err| {
|
||||
ApiError::bad_request(format!(
|
||||
"git returned a short SHA that did not match expected hex pattern: `{short}`: {err}"
|
||||
))
|
||||
})
|
||||
}
|
||||
|
||||
/// Materialize the response for `GET /runs/{id}/files`. Prefers the live
|
||||
|
|
|
|||
|
|
@ -3079,7 +3079,9 @@ fn update_live_run_from_event(state: &AppState, run_id: RunId, event: &RunEvent)
|
|||
EventBody::RunRemoving(_) => managed_run.status = RunStatus::Removing,
|
||||
EventBody::RunCompleted(_) => {
|
||||
let EventBody::RunCompleted(props) = &event.body else {
|
||||
unreachable!();
|
||||
unreachable!(
|
||||
"outer match arm already verified event.body is EventBody::RunCompleted"
|
||||
)
|
||||
};
|
||||
managed_run.status = RunStatus::Succeeded {
|
||||
reason: props.reason,
|
||||
|
|
|
|||
|
|
@ -20,7 +20,8 @@ macro_rules! warn_user {
|
|||
macro_rules! warn_user_once {
|
||||
($($arg:tt)*) => {{
|
||||
let message = format!($($arg)*);
|
||||
let mut set = $crate::WARNINGS.lock().unwrap();
|
||||
let mut set = $crate::WARNINGS.lock()
|
||||
.expect("WARNINGS mutex should not be poisoned: no code panics while holding this lock");
|
||||
if set.insert(message.clone()) {
|
||||
drop(set);
|
||||
$crate::warn_user!("{message}");
|
||||
|
|
|
|||
|
|
@ -114,7 +114,7 @@ fn eval_clause(clause: &Clause, outcome: &Outcome, context: &Context) -> bool {
|
|||
Op::Lt => lhs < rhs,
|
||||
Op::Gte => lhs >= rhs,
|
||||
Op::Lte => lhs <= rhs,
|
||||
_ => unreachable!(),
|
||||
_ => unreachable!("outer match arm already restricts to Gt, Lt, Gte, and Lte"),
|
||||
}
|
||||
}
|
||||
Op::Contains => {
|
||||
|
|
|
|||
|
|
@ -834,8 +834,9 @@ fn event_body_from_event(event: &Event) -> EventBody {
|
|||
| AgentEvent::ReasoningDelta { .. }
|
||||
| AgentEvent::ToolCallOutputDelta { .. }
|
||||
| AgentEvent::SessionStarted { .. }
|
||||
| AgentEvent::SessionEnded => panic!(
|
||||
"agent event should not be converted through the stage-scoped Event::Agent wrapper"
|
||||
| AgentEvent::SessionEnded => unreachable!(
|
||||
"streaming noise and session lifecycle events are filtered out before wrapping in \
|
||||
Event::Agent; if this is reached, the emitter has a routing bug"
|
||||
),
|
||||
},
|
||||
Event::SubgraphStarted { start_node, .. } => {
|
||||
|
|
|
|||
|
|
@ -482,14 +482,20 @@ fn track_file_event(event: &AgentEvent, state: &mut FileTracking) {
|
|||
fn file_tracking_snapshot(
|
||||
file_tracking: &Arc<Mutex<FileTracking>>,
|
||||
) -> (Vec<String>, Option<String>) {
|
||||
let state = file_tracking.lock().unwrap();
|
||||
let state = file_tracking
|
||||
.lock()
|
||||
.expect("file_tracking mutex is never poisoned: no code panics while holding this lock");
|
||||
let mut files: Vec<String> = state.touched.iter().cloned().collect();
|
||||
files.sort();
|
||||
(files, state.last.clone())
|
||||
}
|
||||
|
||||
fn last_touched_file(file_tracking: &Arc<Mutex<FileTracking>>) -> Option<String> {
|
||||
file_tracking.lock().unwrap().last.clone()
|
||||
file_tracking
|
||||
.lock()
|
||||
.expect("file_tracking mutex is never poisoned: no code panics while holding this lock")
|
||||
.last
|
||||
.clone()
|
||||
}
|
||||
|
||||
fn last_assistant_response(session: &Session) -> String {
|
||||
|
|
@ -540,7 +546,12 @@ fn spawn_event_forwarder(
|
|||
emitter.touch();
|
||||
|
||||
// Track file changes from tool calls (including sub-agent events)
|
||||
track_file_event(&event.event, &mut file_tracking.lock().unwrap());
|
||||
track_file_event(
|
||||
&event.event,
|
||||
&mut file_tracking.lock().expect(
|
||||
"file_tracking mutex is never poisoned: no code panics while holding this lock",
|
||||
),
|
||||
);
|
||||
|
||||
// Forward non-streaming agent events to pipeline
|
||||
if !event.event.is_streaming_noise()
|
||||
|
|
@ -888,7 +899,7 @@ impl AgentApiBackend {
|
|||
let sessions: Vec<Session> = self
|
||||
.sessions
|
||||
.lock()
|
||||
.unwrap()
|
||||
.expect("sessions mutex is never poisoned: no code panics while holding this lock")
|
||||
.drain()
|
||||
.map(|(_, s)| s)
|
||||
.collect();
|
||||
|
|
@ -1152,7 +1163,11 @@ impl CodergenBackend for AgentApiBackend {
|
|||
return Err(Error::Cancelled);
|
||||
}
|
||||
let (mut session, is_reused) = if let Some(ref key) = reuse_key {
|
||||
let existing = self.sessions.lock().unwrap().remove(key);
|
||||
let existing = self
|
||||
.sessions
|
||||
.lock()
|
||||
.expect("sessions mutex is never poisoned: no code panics while holding this lock")
|
||||
.remove(key);
|
||||
if let Some(s) = existing {
|
||||
(s, true)
|
||||
} else {
|
||||
|
|
@ -1521,7 +1536,10 @@ impl CodergenBackend for AgentApiBackend {
|
|||
// the cached session is not left wired to this run's cancel token.
|
||||
if let Some(key) = reuse_key {
|
||||
bridge.abort();
|
||||
self.sessions.lock().unwrap().insert(key, session);
|
||||
self.sessions
|
||||
.lock()
|
||||
.expect("sessions mutex is never poisoned: no code panics while holding this lock")
|
||||
.insert(key, session);
|
||||
} else {
|
||||
let session_id = session.id().to_string();
|
||||
if session.close() {
|
||||
|
|
|
|||
|
|
@ -73,7 +73,9 @@ impl ArtifactLifecycle {
|
|||
#[async_trait]
|
||||
impl RunLifecycle<WorkflowGraph> for ArtifactLifecycle {
|
||||
async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> {
|
||||
*self.attempt_start_epoch.lock().unwrap() = None;
|
||||
*self.attempt_start_epoch.lock().expect(
|
||||
"artifact mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = None;
|
||||
let ledger = self
|
||||
.rebuild_captured_artifact_ledger()
|
||||
.await
|
||||
|
|
@ -83,7 +85,9 @@ impl RunLifecycle<WorkflowGraph> for ArtifactLifecycle {
|
|||
"failed to rebuild captured artifact ledger: {rendered}"
|
||||
))
|
||||
})?;
|
||||
*self.captured_artifacts.lock().unwrap() = ledger;
|
||||
*self.captured_artifacts.lock().expect(
|
||||
"artifact mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = ledger;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
|
@ -96,7 +100,9 @@ impl RunLifecycle<WorkflowGraph> for ArtifactLifecycle {
|
|||
let epoch = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.map_or(0.0, |d| d.as_secs() as f64);
|
||||
*self.attempt_start_epoch.lock().unwrap() = Some(epoch);
|
||||
*self.attempt_start_epoch.lock().expect(
|
||||
"artifact mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = Some(epoch);
|
||||
Ok(NodeDecision::Continue)
|
||||
}
|
||||
|
||||
|
|
@ -108,7 +114,11 @@ impl RunLifecycle<WorkflowGraph> for ArtifactLifecycle {
|
|||
if self.artifact_globs.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
let epoch = self.attempt_start_epoch.lock().unwrap().unwrap_or(0.0);
|
||||
let epoch = self
|
||||
.attempt_start_epoch
|
||||
.lock()
|
||||
.expect("artifact mutex should not be poisoned: no code panics while holding this lock")
|
||||
.unwrap_or(0.0);
|
||||
let node_id = ctx.node.id();
|
||||
let visit = stage_visit(state, node_id);
|
||||
let node_slug = if visit <= 1 {
|
||||
|
|
@ -256,7 +266,9 @@ impl ArtifactLifecycle {
|
|||
}
|
||||
|
||||
fn new_captured_assets(&self, artifacts: &[ArtifactUpload]) -> Vec<ArtifactUpload> {
|
||||
let ledger = self.captured_artifacts.lock().unwrap();
|
||||
let ledger = self.captured_artifacts.lock().expect(
|
||||
"artifact mutex should not be poisoned: no code panics while holding this lock",
|
||||
);
|
||||
artifacts
|
||||
.iter()
|
||||
.filter(|artifact| !ledger.contains(&artifact_identity(artifact)))
|
||||
|
|
@ -265,7 +277,9 @@ impl ArtifactLifecycle {
|
|||
}
|
||||
|
||||
fn record_captured_assets(&self, artifacts: &[ArtifactUpload]) {
|
||||
let mut ledger = self.captured_artifacts.lock().unwrap();
|
||||
let mut ledger = self.captured_artifacts.lock().expect(
|
||||
"artifact mutex should not be poisoned: no code panics while holding this lock",
|
||||
);
|
||||
for artifact in artifacts {
|
||||
ledger.insert(artifact_identity(artifact));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -37,8 +37,12 @@ impl CircuitBreakerLifecycle {
|
|||
loop_sigs: HashMap<FailureSignature, usize>,
|
||||
restart_sigs: HashMap<FailureSignature, usize>,
|
||||
) {
|
||||
*self.loop_failure_signatures.lock().unwrap() = loop_sigs;
|
||||
*self.restart_failure_signatures.lock().unwrap() = restart_sigs;
|
||||
*self.loop_failure_signatures.lock().expect(
|
||||
"circuit breaker mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = loop_sigs;
|
||||
*self.restart_failure_signatures.lock().expect(
|
||||
"circuit breaker mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = restart_sigs;
|
||||
}
|
||||
|
||||
/// Snapshot current state for checkpoint building.
|
||||
|
|
@ -48,8 +52,12 @@ impl CircuitBreakerLifecycle {
|
|||
HashMap<FailureSignature, usize>,
|
||||
HashMap<FailureSignature, usize>,
|
||||
) {
|
||||
let loop_sigs = self.loop_failure_signatures.lock().unwrap().clone();
|
||||
let restart_sigs = self.restart_failure_signatures.lock().unwrap().clone();
|
||||
let loop_sigs = self.loop_failure_signatures.lock()
|
||||
.expect("circuit breaker mutex should not be poisoned: no code panics while holding this lock")
|
||||
.clone();
|
||||
let restart_sigs = self.restart_failure_signatures.lock()
|
||||
.expect("circuit breaker mutex should not be poisoned: no code panics while holding this lock")
|
||||
.clone();
|
||||
(loop_sigs, restart_sigs)
|
||||
}
|
||||
}
|
||||
|
|
@ -83,7 +91,8 @@ impl RunLifecycle<WorkflowGraph> for CircuitBreakerLifecycle {
|
|||
outcome.failure.as_ref().map(|f| f.message.as_str()),
|
||||
);
|
||||
if fc.is_signature_tracked() {
|
||||
let mut sigs = self.loop_failure_signatures.lock().unwrap();
|
||||
let mut sigs = self.loop_failure_signatures.lock()
|
||||
.expect("circuit breaker mutex should not be poisoned: no code panics while holding this lock");
|
||||
let count = sigs.entry(sig.clone()).or_insert(0);
|
||||
*count += 1;
|
||||
let limit = self.loop_restart_signature_limit;
|
||||
|
|
@ -133,7 +142,8 @@ impl RunLifecycle<WorkflowGraph> for CircuitBreakerLifecycle {
|
|||
Some(failure.message.as_str()),
|
||||
);
|
||||
if failure.category.is_signature_tracked() {
|
||||
let mut sigs = self.restart_failure_signatures.lock().unwrap();
|
||||
let mut sigs = self.restart_failure_signatures.lock()
|
||||
.expect("circuit breaker mutex should not be poisoned: no code panics while holding this lock");
|
||||
let count = sigs.entry(sig.clone()).or_insert(0);
|
||||
*count += 1;
|
||||
let limit = self.loop_restart_signature_limit;
|
||||
|
|
|
|||
|
|
@ -111,7 +111,8 @@ impl RunLifecycle<WorkflowGraph> for EventLifecycle {
|
|||
async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> {
|
||||
// If restarted_from is Some, emit LoopRestart and clear it
|
||||
{
|
||||
let mut restarted = self.restarted_from.lock().unwrap();
|
||||
let mut restarted = self.restarted_from.lock()
|
||||
.expect("event lifecycle mutex should not be poisoned: no code panics while holding this lock");
|
||||
if let Some((from_node, to_node)) = restarted.take() {
|
||||
self.emitter
|
||||
.emit(&Event::LoopRestart { from_node, to_node });
|
||||
|
|
@ -119,7 +120,9 @@ impl RunLifecycle<WorkflowGraph> for EventLifecycle {
|
|||
}
|
||||
|
||||
// Reset run_start for duration measurement
|
||||
*self.run_start.lock().unwrap() = Instant::now();
|
||||
*self.run_start.lock().expect(
|
||||
"event lifecycle mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = Instant::now();
|
||||
|
||||
// Emit RunStarted
|
||||
self.emitter.emit(&Event::WorkflowRunStarted {
|
||||
|
|
@ -377,7 +380,9 @@ impl RunLifecycle<WorkflowGraph> for EventLifecycle {
|
|||
let status = result.outcome.status.to_string();
|
||||
|
||||
// Read git checkpoint result (set by GitLifecycle)
|
||||
let git_result = self.checkpoint_git_result.lock().unwrap().clone();
|
||||
let git_result = self.checkpoint_git_result.lock()
|
||||
.expect("event lifecycle mutex should not be poisoned: no code panics while holding this lock")
|
||||
.clone();
|
||||
|
||||
let git_sha = git_result.as_ref().and_then(|r| r.commit_sha.clone());
|
||||
let diff = git_result.as_ref().and_then(|r| r.diff.clone());
|
||||
|
|
|
|||
|
|
@ -57,7 +57,9 @@ impl FidelityLifecycle {
|
|||
}
|
||||
|
||||
pub(crate) fn set_degrade_fidelity_on_resume(&self, flag: bool) {
|
||||
*self.degrade_fidelity_on_resume.lock().unwrap() = flag;
|
||||
*self.degrade_fidelity_on_resume.lock().expect(
|
||||
"fidelity mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = flag;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -65,7 +67,9 @@ impl FidelityLifecycle {
|
|||
impl RunLifecycle<WorkflowGraph> for FidelityLifecycle {
|
||||
async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> {
|
||||
// Clear incoming edge data (restart target must not inherit pre-restart edge)
|
||||
*self.incoming_edge_data.lock().unwrap() = None;
|
||||
*self.incoming_edge_data.lock().expect(
|
||||
"fidelity mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = None;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
|
@ -74,7 +78,11 @@ impl RunLifecycle<WorkflowGraph> for FidelityLifecycle {
|
|||
node: &WorkflowNode,
|
||||
state: &WfRunState,
|
||||
) -> CoreResult<WfNodeDecision> {
|
||||
let incoming = self.incoming_edge_data.lock().unwrap().take();
|
||||
let incoming = self
|
||||
.incoming_edge_data
|
||||
.lock()
|
||||
.expect("fidelity mutex should not be poisoned: no code panics while holding this lock")
|
||||
.take();
|
||||
let gv_node = node.inner();
|
||||
|
||||
// 1. Fidelity resolution via resolve_fidelity: edge → node → graph default →
|
||||
|
|
@ -84,7 +92,9 @@ impl RunLifecycle<WorkflowGraph> for FidelityLifecycle {
|
|||
|
||||
// 2. Fidelity degradation on resume (full → summary:high)
|
||||
let fidelity = {
|
||||
let mut degrade = self.degrade_fidelity_on_resume.lock().unwrap();
|
||||
let mut degrade = self.degrade_fidelity_on_resume.lock().expect(
|
||||
"fidelity mutex should not be poisoned: no code panics while holding this lock",
|
||||
);
|
||||
if *degrade {
|
||||
*degrade = false;
|
||||
fidelity.degraded()
|
||||
|
|
@ -180,7 +190,9 @@ impl RunLifecycle<WorkflowGraph> for FidelityLifecycle {
|
|||
let edge_data = IncomingEdgeData {
|
||||
edge: Arc::new(gv_edge.clone()),
|
||||
};
|
||||
*self.incoming_edge_data.lock().unwrap() = Some(edge_data);
|
||||
*self.incoming_edge_data.lock().expect(
|
||||
"fidelity mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = Some(edge_data);
|
||||
}
|
||||
Ok(EdgeDecision::Continue)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -94,8 +94,12 @@ pub(crate) struct GitLifecycle {
|
|||
impl RunLifecycle<WorkflowGraph> for GitLifecycle {
|
||||
async fn on_run_start(&self, _graph: &WorkflowGraph, _state: &WfRunState) -> CoreResult<()> {
|
||||
// Reset last_git_sha (diff base parity)
|
||||
*self.last_git_sha.lock().unwrap() = None;
|
||||
*self.checkpoint_git_result.lock().unwrap() = None;
|
||||
*self.last_git_sha.lock().expect(
|
||||
"git lifecycle mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = None;
|
||||
*self.checkpoint_git_result.lock().expect(
|
||||
"git lifecycle mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = None;
|
||||
if let Some(meta_branch) = self.metadata_branch().map(str::to_string) {
|
||||
if self.metadata_writer.is_none() || self.metadata_runtime.metadata_degraded() {
|
||||
return Ok(());
|
||||
|
|
@ -173,7 +177,8 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
|
|||
|
||||
// Skip git checkpoint for the start node (always empty) or if git disabled
|
||||
if self.start_node_id.as_deref() == Some(node_id) || self.run_options.git.is_none() {
|
||||
*self.checkpoint_git_result.lock().unwrap() = None;
|
||||
*self.checkpoint_git_result.lock()
|
||||
.expect("git lifecycle mutex should not be poisoned: no code panics while holding this lock") = None;
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
|
|
@ -330,7 +335,9 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
|
|||
}
|
||||
|
||||
// Save diff.patch
|
||||
let prev = self.last_git_sha.lock().unwrap().clone().or_else(|| {
|
||||
let prev = self.last_git_sha.lock()
|
||||
.expect("git lifecycle mutex should not be poisoned: no code panics while holding this lock")
|
||||
.clone().or_else(|| {
|
||||
self.run_options
|
||||
.git
|
||||
.as_ref()
|
||||
|
|
@ -386,8 +393,10 @@ impl RunLifecycle<WorkflowGraph> for GitLifecycle {
|
|||
}
|
||||
|
||||
// Update shared state
|
||||
*self.last_git_sha.lock().unwrap() = Some(sha);
|
||||
*self.checkpoint_git_result.lock().unwrap() = Some(git_result);
|
||||
*self.last_git_sha.lock()
|
||||
.expect("git lifecycle mutex should not be poisoned: no code panics while holding this lock") = Some(sha);
|
||||
*self.checkpoint_git_result.lock()
|
||||
.expect("git lifecycle mutex should not be poisoned: no code panics while holding this lock") = Some(git_result);
|
||||
}
|
||||
Err(e) => {
|
||||
let exec_output_tail = fabro_sandbox::default_redacted_output_tail(&e);
|
||||
|
|
|
|||
|
|
@ -391,7 +391,8 @@ impl RunLifecycle<WorkflowGraph> for WorkflowLifecycle {
|
|||
if matches!(decision, EdgeDecision::Continue) {
|
||||
if let Some(ref edge) = ctx.edge {
|
||||
if edge.inner().loop_restart() {
|
||||
*self.restarted_from.lock().unwrap() =
|
||||
*self.restarted_from.lock()
|
||||
.expect("lifecycle mutex should not be poisoned: no code panics while holding this lock") =
|
||||
Some((ctx.from.to_string(), ctx.to.to_string()));
|
||||
}
|
||||
}
|
||||
|
|
@ -419,7 +420,9 @@ impl RunLifecycle<WorkflowGraph> for WorkflowLifecycle {
|
|||
.on_checkpoint(node, result, next_node_id, state)
|
||||
.await?;
|
||||
// Clear checkpoint result for next checkpoint
|
||||
*self.checkpoint_git_result.lock().unwrap() = None;
|
||||
*self.checkpoint_git_result.lock().expect(
|
||||
"lifecycle mutex should not be poisoned: no code panics while holding this lock",
|
||||
) = None;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -804,27 +804,31 @@ impl RunSession {
|
|||
event if matches!(&event.body, EventBody::CheckpointCompleted(_)) => {
|
||||
if let EventBody::CheckpointCompleted(props) = &event.body {
|
||||
if let Some(sha) = props.git_commit_sha.as_ref() {
|
||||
*sha_clone.lock().unwrap() = Some(sha.clone());
|
||||
*sha_clone.lock()
|
||||
.expect("sha_clone mutex should not be poisoned: no code panics while holding this lock") = Some(sha.clone());
|
||||
}
|
||||
}
|
||||
}
|
||||
event if matches!(&event.body, EventBody::RunCompleted(_)) => {
|
||||
if let EventBody::RunCompleted(props) = &event.body {
|
||||
if let Some(sha) = props.final_git_commit_sha.as_ref() {
|
||||
*sha_clone.lock().unwrap() = Some(sha.clone());
|
||||
*sha_clone.lock()
|
||||
.expect("sha_clone mutex should not be poisoned: no code panics while holding this lock") = Some(sha.clone());
|
||||
}
|
||||
}
|
||||
}
|
||||
event if matches!(&event.body, EventBody::RunFailed(_)) => {
|
||||
if let EventBody::RunFailed(props) = &event.body {
|
||||
if let Some(sha) = props.final_git_commit_sha.as_ref() {
|
||||
*sha_clone.lock().unwrap() = Some(sha.clone());
|
||||
*sha_clone.lock()
|
||||
.expect("sha_clone mutex should not be poisoned: no code panics while holding this lock") = Some(sha.clone());
|
||||
}
|
||||
}
|
||||
}
|
||||
event if matches!(&event.body, EventBody::GitCommit(_)) => {
|
||||
if let EventBody::GitCommit(props) = &event.body {
|
||||
*sha_clone.lock().unwrap() = Some(props.sha.clone());
|
||||
*sha_clone.lock()
|
||||
.expect("sha_clone mutex should not be poisoned: no code panics while holding this lock") = Some(props.sha.clone());
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
|
|
@ -894,7 +898,9 @@ impl RunSession {
|
|||
workflow_name: executed.graph.name.clone(),
|
||||
preserve_sandbox: self.preserve_sandbox,
|
||||
stop_on_terminal: self.stop_on_terminal,
|
||||
last_git_sha: last_git_sha.lock().unwrap().clone(),
|
||||
last_git_sha: last_git_sha.lock()
|
||||
.expect("last_git_sha mutex should not be poisoned: no code panics while holding this lock")
|
||||
.clone(),
|
||||
};
|
||||
let pr_opts = PullRequestOptions {
|
||||
pr_config: self.pr_config,
|
||||
|
|
|
|||
|
|
@ -254,12 +254,19 @@ impl EngineServices {
|
|||
|
||||
/// Read the current git state (if any).
|
||||
pub fn git_state(&self) -> Option<Arc<GitState>> {
|
||||
self.git_state.read().unwrap().clone()
|
||||
self.git_state
|
||||
.read()
|
||||
.expect("git_state lock is never poisoned: no code panics while holding this lock")
|
||||
.clone()
|
||||
}
|
||||
|
||||
/// Set the git state for the current run.
|
||||
pub fn set_git_state(&self, state: Option<Arc<GitState>>) {
|
||||
*self.git_state.write().unwrap() = state;
|
||||
*self
|
||||
.git_state
|
||||
.write()
|
||||
.expect("git_state lock is never poisoned: no code panics while holding this lock") =
|
||||
state;
|
||||
}
|
||||
|
||||
/// Test-only default: empty registry and cross-phase services.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue