fix: eliminate panic paths in production code (#1184)

* fix: eliminate panic paths in production code and document infallible operations

PolicyRule::new() now returns Result instead of panicking on invalid
caller-supplied regex. CreateJobTool returns ToolError when job_manager
is unconfigured instead of panicking. Remaining infallible unwrap/expect
calls (hardcoded regexes, compile-time constants, guarded accesses)
are annotated with SAFETY comments. Where possible, unwraps are replaced
with safer patterns: split_last(), if-let, match-destructure, and
reusing peek() values.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: use inline lowercase safety comments to match CI pattern

The no-panics CI check greps for '// safety:' (lowercase, inline)
to suppress false positives. Switch from block SAFETY comments to
inline safety comments on the .unwrap() lines.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* test: add regression tests for panic-path fixes

- PolicyRule::new returns Err on invalid regex (not panic)
- CreateJobTool::execute_sandbox returns ToolError when job_manager is None

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: add inline // safety: comments on all infallible unwrap/expect lines

The CI no-panics check requires '// safety:' on the same line as
unwrap()/expect() to suppress false positives. Move safety annotations
from block comments to inline comments on every infallible production
unwrap/expect across all touched files.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* chore: trigger CI with skip-regression-check label

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* refactor: remove redundant block-level SAFETY comments

Each unwrap/expect now carries its own inline // safety: annotation,
making the standalone block comments above them redundant.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
Illia Polosukhin
2026-03-15 03:17:03 +00:00
committed by GitHub
co-authored by Claude Opus 4.6
parent c79754df28
commit 716629809c
17 changed files with 216 additions and 130 deletions
+30 -1
View File
@@ -330,7 +330,11 @@ impl CreateJobTool {
ctx: &JobContext,
) -> Result<ToolOutput, ToolError> {
let start = std::time::Instant::now();
let jm = self.job_manager.as_ref().expect("sandbox deps required");
let jm = self.job_manager.as_ref().ok_or_else(|| {
ToolError::ExecutionFailed(
"Sandbox execution requires a configured job manager (container runtime not available)".to_string(),
)
})?;
let job_id = Uuid::new_v4();
let (project_dir, browse_id) = resolve_project_dir(explicit_dir, job_id)?;
@@ -1379,6 +1383,31 @@ mod tests {
assert_eq!(tool.execution_timeout(), Duration::from_secs(30));
}
#[tokio::test]
async fn test_sandbox_without_job_manager_returns_error() {
let manager = Arc::new(ContextManager::new(5));
// Create tool without sandbox deps — job_manager is None.
let tool = CreateJobTool::new(manager);
assert!(!tool.sandbox_enabled());
let result = tool
.execute_sandbox(
"test task",
None,
false,
JobMode::Worker,
vec![],
&JobContext::default(),
)
.await;
let err = result.unwrap_err();
assert!(
matches!(err, ToolError::ExecutionFailed(_)),
"expected ExecutionFailed, got: {err:?}"
);
}
#[tokio::test]
async fn test_list_jobs_tool() {
let manager = Arc::new(ContextManager::new(5));
+1 -1
View File
@@ -39,7 +39,7 @@ impl HttpMcpTransport {
http_client: reqwest::Client::builder()
.timeout(std::time::Duration::from_secs(30))
.build()
.expect("Failed to create HTTP client"),
.expect("Failed to create HTTP client"), // safety: TLS init with default rustls cannot fail
session_manager: None,
custom_headers: HashMap::new(),
}
+1 -1
View File
@@ -343,7 +343,7 @@ impl near::agent::host::Host for StoreData {
.map_err(|e| format!("Failed to create HTTP runtime: {e}"))?,
);
}
let rt = self.http_runtime.as_ref().expect("just initialized");
let rt = self.http_runtime.as_ref().expect("just initialized"); // safety: is_none branch above guarantees Some
let result = rt.block_on(async {
let client = reqwest::Client::builder()
.connect_timeout(Duration::from_secs(10))