fix: resolve deferred review items from PRs #883, #848, #788

[ilblackdragon]

Summary

Addresses three deferred implementation items that were explicitly flagged as follow-up work during recent code reviews:

• SIGHUP lock across .await (#883 comment): Refactored restart_with_addr into merged_router_clone() + install_listener() so the async TcpListener::bind happens outside the mutex. Lock is now only held for brief sync state swaps.
• Recursion depth limit for check_strings (#848 comment): Added MAX_DEPTH = 32 cap on recursive JSON traversal in the safety validator to prevent stack overflow on pathological tool parameters.
• Named error type for add_tokens (#788 comment): Replaced Result<(), String> with TokenBudgetExceeded { used, limit } for type-safe budget enforcement.

A fourth item (generic unsupported parameter stripping from PR #809) was investigated and found to already be addressed in the current codebase — strip_unsupported_completion_params() handles all three parameter types.

Test plan

• cargo clippy -- -D warnings — zero warnings
• cargo test --lib webhook_server — restart + rollback tests pass with new API
• cargo test --lib validator — new depth-limit regression tests pass
• cargo test --lib test_add_tokens — budget enforcement tests pass with new error type

🤖 Generated with Claude Code

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[Copilot]

Pull request overview

This PR addresses deferred review follow-ups across the worker token-budget path, safety validator robustness, and webhook server restart behavior (notably eliminating a mutex held across an async bind during SIGHUP reload).

Changes:

• Replace JobContext::add_tokens() error return from String to a typed TokenBudgetExceeded { used, limit }.
• Add a recursion depth cap to Validator::validate_tool_params() and regression tests around the cap.
• Refactor webhook server restart to a two-phase approach (bind outside lock; swap state under lock) via merged_router_clone() + install_listener().

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/worker/job.rs	Updates token budget enforcement call sites and tests to handle the new error type.
src/context/state.rs	Introduces TokenBudgetExceeded error type and updates add_tokens() signature accordingly.
src/context/mod.rs	Re-exports TokenBudgetExceeded from the context module.
src/safety/validator.rs	Adds depth-limited traversal for tool-param string validation plus new tests.
src/main.rs	Refactors SIGHUP webhook listener restart to bind outside the mutex and swap under lock.
src/channels/webhook_server.rs	Adds router cloning + listener installation API to support lock-minimized restarts; updates restart/rollback tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

mark_failed(&err.to_string()).await borrows a temporary String across an .await, which won’t compile (temporary is dropped at the end of the statement while the future may hold the &str). Bind the message to a local String first (or change mark_failed to take an owned String/impl Into<String>).

Suggested change

	self.worker.mark_failed(&err.to_string()).await?;
	let msg = err.to_string();
	self.worker.mark_failed(&msg).await?;

[Copilot]

This test calls mark_failed(&budget_result.unwrap_err().to_string()).await, which borrows a temporary String across an .await and will not compile. Store the .to_string() into a local variable (or adjust mark_failed to take an owned string) before awaiting.

[Copilot]

The new depth cap silently skips validating any strings beyond MAX_DEPTH. Since validate_tool_params is a safety boundary (null-byte / forbidden-pattern checks), this creates an easy bypass by nesting a malicious string deeper than the limit. Instead of returning Ok, treat “too deep” as a validation error (or switch to an iterative traversal that can enforce a depth limit without recursion).

Suggested change

	validator: &Validator,
	result: &mut ValidationResult,
	depth: usize,
	) {
	if depth > MAX_DEPTH {
	validator: & Validator,
	result: &mut ValidationResult,
	depth: usize,
	) {
	if depth > MAX_DEPTH {
	// Treat exceeding the maximum depth as a validation failure,
	// to avoid silently skipping checks on deeply nested strings.
	result.is_valid = false;
	result.warnings.push(format!(
	"Validation aborted at path '{}' due to exceeding maximum depth of {}",
	path,
	MAX_DEPTH
	));

[Copilot]

test_tool_params_depth_limit_prevents_stack_overflow asserts that forbidden content beyond MAX_DEPTH is not detected and validation remains is_valid. If the depth cap is meant as a DoS/stack-safety measure, the safer behavior is to fail validation when the nesting is too deep (or at least emit an error/warning), rather than silently accepting unvalidated content. Update this test to match the safer behavior.

Suggested change

	// The "evil payload" is beyond the depth limit so it should NOT be
	// detected — the traversal stops before reaching it.
	assert!(
	result.is_valid,
	"Strings beyond depth limit should be silently skipped, got errors: {:?}",
	result.errors
	// The "evil payload" is beyond the depth limit. To avoid silently
	// accepting unvalidated content, exceeding the depth cap should
	// cause validation to fail instead of being treated as valid.
	assert!(
	!result.is_valid,
	"Inputs that exceed the maximum nesting depth should not be considered valid"

[Copilot]

This test assumes binding to 127.0.0.1:1 will fail due to privilege requirements, which can be false in CI/containerized environments (often running as root) and make the test flaky. Use a deterministic bind failure instead (e.g., keep a StdTcpListener open on an ephemeral port, then attempt to bind tokio::net::TcpListener to the same addr to reliably hit AddrInUse).

[zmanian]

Review: fix: resolve deferred review items from PRs #883, #848, #788

CI is green across all clippy targets and tests. All three deferred items are addressed correctly. Approving.

1. SIGHUP lock across .await (#883) -- webhook_server.rs + main.rs

The refactor is well-designed. The old restart_with_addr held the mutex across an async TcpListener::bind, which could block other lock waiters. The new two-phase approach:

• Phase 1: merged_router_clone() + TcpListener::bind() happen outside the lock
• Phase 2: install_listener() does only sync bookkeeping under the lock
• Phase 3: Old listener shutdown happens outside the lock

This eliminates the lock-across-await issue cleanly. The rollback-by-construction approach (if bind fails, install_listener is never called, so server state is untouched) is simpler and more correct than the old try/restore pattern.

One minor observation: install_listener is now pub and takes arbitrary TcpListener + Router -- callers must ensure the listener matches the router's expectations. This is fine for the current internal usage but worth noting if the API surface expands.

2. Recursion depth limit for check_strings (#848) -- validator.rs

MAX_DEPTH = 32 is a reasonable cap. The implementation correctly passes depth + 1 on both array and object recursion, and silently stops traversal beyond the limit. The two regression tests cover both sides: deep nesting beyond the limit (payload not detected) and shallow nesting within the limit (payload detected).

One thing to consider (not blocking): silently skipping validation on deeply nested content means an attacker could hide injection payloads at depth 33+. A tracing::warn! when the depth limit is hit would make this observable in production. However, 32 levels of JSON nesting in tool parameters is already pathological, and any real tool would reject such input for other reasons.

3. Named error type for add_tokens (#788) -- context/state.rs + worker/job.rs

Clean replacement of Result<(), String> with Result<(), TokenBudgetExceeded>. The thiserror derive provides the same Display message as the old format string. The pub fields (used, limit) enable callers to make programmatic decisions based on the budget state.

The call site in worker/job.rs correctly uses .to_string() on the error when passing to mark_failed(), preserving behavior. The update_context generic return type (F: FnOnce(&mut JobContext) -> R) means R = Result<(), TokenBudgetExceeded>, which nests correctly.

Note: there's already a TokenBudgetExceeded variant in SkillRegistryError (skills/registry.rs) with different fields. The naming collision is acceptable since they're in different modules and represent different budget types (LLM tokens vs skill prompt tokens).

Tests

All three changes include regression tests:

• Restart + rollback tests updated for two-phase API
• test_tool_params_depth_limit_prevents_stack_overflow + test_tool_params_within_depth_limit_still_validated
• Existing test_add_tokens_enforces_budget exercises the new error type

LGTM.