feat: add abench method - #374
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @MrtinoRG, your pull request is larger than the review limit of 300000 diff characters
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds bounded synchronous and asynchronous benchmark concurrency, isolated trial runtimes with optional process workers, background tool jobs with MCP Tasks support, async router interoperability, agent execution updates, decorator-based tools, cost-aware metric parallelism, and extensive integration coverage. ChangesConcurrent benchmarking and runtime isolation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant AsyncCorralRouter
participant CorralRunner
participant TrialWorkerPool
participant TrialRuntime
Client->>AsyncCorralRouter: create trial
AsyncCorralRouter->>TrialWorkerPool: lease worker when process isolation is enabled
TrialWorkerPool->>TrialRuntime: open trial runtime
CorralRunner->>TrialRuntime: execute agent trial
TrialRuntime->>TrialRuntime: run tools and background jobs
CorralRunner->>TrialRuntime: submit result
TrialRuntime->>TrialWorkerPool: close and release worker
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (27)
src/corral/utils/tool_helpers.py-81-85 (1)
81-85: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winResolve relative answers against
base_dirbefore the process CWD.
Path(extracted_path).exists()can select a same-named file from the server CWD, bypassing trial isolation and scoring the wrong output.Proposed fix
extracted_path = extract_path_from_answer(input_path) + if base_dir: + base_path = Path(base_dir).resolve() + candidate = Path(extracted_path) + candidate = ( + candidate.resolve() + if candidate.is_absolute() + else (base_path / candidate).resolve() + ) + try: + candidate.relative_to(base_path) + except ValueError: + return find_file_by_name(candidate.name, str(base_path)) + if candidate.exists(): + return str(candidate) + return find_file_by_name(candidate.name, str(base_path)) + if Path(extracted_path).exists(): - return extracted_path # Use the extracted path directly - else: - # Search as fallback, scoped to base_dir when provided. - return find_file_by_name(Path(extracted_path).name, base_dir) + return extracted_path + return find_file_by_name(Path(extracted_path).name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/utils/tool_helpers.py` around lines 81 - 85, Update the extracted-path resolution logic around Path(extracted_path).exists() to resolve relative paths against base_dir first, using the process CWD only when base_dir is unavailable or the base_dir-relative candidate does not exist. Preserve absolute paths and the existing find_file_by_name fallback, ensuring trial-isolated files take precedence over same-named CWD files.src/corral/utils/io_tools.py-171-172 (1)
171-172: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound regex execution in
_grep_file.re.search()runs inline insidecall_tool(), so a catastrophic-backtracking pattern can pin the runtime’s worker thread and block the call until the outer harness times out. Add a hard timeout or move the match loop into a killable worker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/utils/io_tools.py` around lines 171 - 172, Update the regex matching loop in _grep_file so each re.search operation is subject to a hard timeout or runs in a killable worker, preventing catastrophic-backtracking patterns from blocking call_tool indefinitely; preserve the existing line iteration and match-result behavior.Source: Linters/SAST tools
tests/backend/test_work_dir_isolation.py-22-36 (1)
22-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCover the process-CWD path that still bypasses trial isolation.
smart_resolve_path()returns an existing relative path from the process cwd before consultingbase_dir. A cwd-levelanswer.jsoncan therefore override both trial workspaces. Add that collision to these tests and make relative paths resolve againstbase_dirfirst.Also applies to: 60-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/backend/test_work_dir_isolation.py` around lines 22 - 36, Update smart_resolve_path and test_smart_resolve_path_scopes_the_search_to_base_dir so relative paths are resolved against base_dir before checking the process CWD. Add a cwd-level answer.json collision in the test and assert each workspace still resolves its own file, preventing the CWD path from overriding trial isolation.tests/backend/test_job_executors.py-108-127 (1)
108-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnsure cancellation-test failures cannot hang the suite.
These non-daemon worker threads can run forever if cancellation regresses, while
shutdown()is skipped when an assertion fails. Make the threads daemonized and place cancellation plus non-blocking executor shutdown infinally.Also applies to: 182-206, 238-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/backend/test_job_executors.py` around lines 108 - 127, Update the cancellation tests around test_subprocess_executor_cancellation_kills_the_process and the additionally affected test blocks so worker threads are daemonized and cleanup cannot be skipped. Wrap cancellation, joining, assertions, and executor cleanup in a try/finally structure, set the cancellation event in finally, and invoke executor.shutdown(wait=False) there so regressions cannot leave the suite hanging.src/corral/run.py-108-127 (1)
108-127: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when concurrency metadata cannot be fetched.
Returning
{}treats every environment as thread-safe. A transient endpoint failure therefore permits unsafe overlap—the warning itself acknowledges possible corruption. Abort or serialize the selected tasks until their modes are known.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/run.py` around lines 108 - 127, Update _fetch_concurrency_modes so failures or missing get_concurrency_modes metadata do not return an empty map that marks all tasks as thread-safe. Fail closed by aborting or ensuring selected tasks are serialized until their declared modes are available, while preserving normal mode retrieval when acall(getter) succeeds.src/corral/run.py-2181-2190 (1)
2181-2190: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCommit independent results by trial index, not completion order.
With
per_task > 1, attempt 2 may be appended and checkpointed before attempt 1. Resume then useslen(trials) == 1, reruns index 1, and can duplicate attempt 2 while losing the intended trial set. Buffer completions per task/index and append only contiguous indices, or persist indexed completion state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/run.py` around lines 2181 - 2190, Update the collector’s task result handling so checkpointed trial state is keyed by trial index rather than completion order. Buffer out-of-order completions in task_results and append only the next contiguous trial indices before calling checkpoint_saver, ensuring resume logic cannot rerun or duplicate completed attempts when per_task is greater than one.src/corral/run.py-1456-1472 (1)
1456-1472: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
global_trials=1on the serial path.
per_task > 1cannot create overlap under a global limit of one, but these conditions still require a fresh agent, async orchestration, and trial-runtime support. Base concurrent-mode selection on actual possible overlap, or rejectper_task > global_trialsduring configuration validation.Also applies to: 1549-1580
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/run.py` around lines 1456 - 1472, Update the concurrent-mode checks surrounding self.abench and the corresponding logic at the referenced later branch so global_trials=1 remains on the serial path when per_task exceeds one. Base async/concurrent selection on whether the configured global limit permits overlap, or validate and reject per_task greater than global_trials during configuration validation; preserve concurrent behavior when actual overlap is possible.src/corral/run.py-152-186 (1)
152-186: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
"serial"environments can still overlap"thread"environments.Only stateful tasks acquire
serial_gate; ordinary tasks bypass it and can execute concurrently with a"serial"task. This contradicts the declared “never overlaps anything” contract. Use shared/exclusive admission or clamp all other execution while a serial task is active.Also applies to: 2147-2175, 2290-2306
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/run.py` around lines 152 - 186, The current _serial_lease logic only gates stateful tasks, allowing "thread" tasks to overlap "serial" environments despite the never-overlap contract. Update the admission logic around _serial_lease and its call sites (including the referenced execution paths) to use shared/exclusive coordination: serial tasks require exclusive access, while ordinary tasks acquire shared access, preserving existing limiter behavior and ensuring serial tasks overlap no other task.src/corral/run.py-2105-2110 (1)
2105-2110: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBound queued scheduler tasks, not only executing trials.
The scheduler materializes every work item and starts one AnyIO task per trial or episode; the infinite event streams add no backpressure. Large benchmarks therefore consume
O(total trials × DAG nodes)task memory despite the global limiter. Feed a bounded stream into a fixed worker pool and cap in-flight episodes similarly.Also applies to: 2151-2151, 2197-2205, 2254-2259, 2294-2294, 2356-2366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/run.py` around lines 2105 - 2110, Replace the materialized work_items and per-trial AnyIO task creation in the scheduler with a bounded producer/consumer stream and fixed worker pool, so queued tasks are capped rather than proportional to total trials. Apply the same bounded in-flight approach to episode execution and the corresponding scheduler paths around the referenced loops, while preserving resume filtering and the global concurrency limit.src/corral/concurrency.py-104-122 (1)
104-122: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFreeze the
per_modelmapping, not only the dataclass shell.A caller-provided
dictremains mutable after construction. Mutating a validated limit to0later bypasses__post_init__; the scheduler then creates a zero-capacity semaphore and those trials wait indefinitely. Snapshot and expose an immutable mapping during initialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/concurrency.py` around lines 104 - 122, Update the dataclass initialization around __post_init__ so per_model is copied into an immutable mapping after validation, preventing mutations to the caller’s dictionary from affecting scheduler limits. Preserve None as None and retain the existing validation behavior and per-model values.src/corral/agents/claude_code.py-884-938 (1)
884-938: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlways execute the
AFTER_TASKlifecycle hook.Both entry points invoke
BEFORE_TASKduring preparation but only clean upcwdafterward. This skips user cleanup and telemetry hooks on every outcome.Proposed fix
try: final_answer = _run_coroutine( self._run_harness( interface, task_id, sdk_prompt, tools, enable_surrender, cwd ), timeout=self.wall_clock_timeout_s, ) except ImportError: raise except Exception as e: return self._harness_failure_answer(e) + else: + return self._finalize_answer(final_answer, task_id, enable_surrender) finally: shutil.rmtree(cwd, ignore_errors=True) - - return self._finalize_answer(final_answer, task_id, enable_surrender) + self._execute_hooks(HookPoint.AFTER_TASK, interface, task_id) @@ try: final_answer = await self._arun_harness( interface, task_id, sdk_prompt, tools, enable_surrender, cwd ) except ImportError: raise except Exception as e: return self._harness_failure_answer(e) + else: + return self._finalize_answer(final_answer, task_id, enable_surrender) finally: shutil.rmtree(cwd, ignore_errors=True) - - return self._finalize_answer(final_answer, task_id, enable_surrender) + self._execute_hooks(HookPoint.AFTER_TASK, interface, task_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/agents/claude_code.py` around lines 884 - 938, Ensure both run and arun execute the AFTER_TASK lifecycle hook after harness completion, failure, or cancellation, while retaining cwd cleanup. Update the shared finalization or each entry point around _prepare_run/_aprepare_run and the existing finally blocks so the hook always runs once for every outcome, including exceptions and timeouts.src/corral/agents/base_agent.py-431-471 (1)
431-471: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCancellation is blocked until these worker threads finish
Both
to_thread.run_sync()calls use the defaultabandon_on_cancel=False, so a cancelled trial stays stuck inrun()or answer extraction until the worker returns. Use a cooperatively cancellable path or move the blocking work to a separately terminable worker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/agents/base_agent.py` around lines 431 - 471, Update BaseAgent.arun and the related to_thread.run_sync usage for answer extraction so cancellation does not wait for blocking worker threads to finish. Use a cooperatively cancellable execution path or a separately terminable worker, while preserving synchronous router adaptation through as_sync_interface and the existing run behavior.src/corral/router/routes.py-523-540 (1)
523-540: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep a finite timeout on the owned async client.
httpx.Timeout(None)disables all timeouts, so a stalled server can hold anabenchtrial and its concurrency slot indefinitely. Add a configurable timeout for the owned client; externally supplied clients can keep their own policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/router/routes.py` around lines 523 - 540, Update the router constructor’s owned-client branch in __init__ to create httpx.AsyncClient with a configurable finite timeout instead of httpx.Timeout(None). Preserve externally supplied clients and their timeout policies, and retain the existing ownership tracking via _owns_client.src/corral/backend/jobs.py-219-232 (1)
219-232: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEnforce
max_concurrencyacross all executor types.Each executor name receives its own pool of
_max_concurrencyworkers. Mixing thread, process, and subprocess tools therefore exceeds the advertised per-trial limit. Add a manager-level execution semaphore shared by every executor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/jobs.py` around lines 219 - 232, Update the manager-level execution flow around _resolve_executor to add and acquire a single semaphore shared by all executor types, limiting total active work to _max_concurrency across thread, process, and subprocess executors. Ensure the semaphore is released when each job finishes, including failure or cancellation paths, while preserving the existing per-name executor caching.src/corral/backend/jobs.py-339-345 (1)
339-345: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRedact hidden argument values from persisted and logged exceptions.
Raw
str(exc)is exposed throughJobRecord.errorand warning logs. A tool exception containing an injected secret defeats the documented hidden-argument redaction guarantee. Sanitize the message before both assignments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/jobs.py` around lines 339 - 345, Sanitize exception messages in the background job exception handler before assigning to JobRecord.error or emitting the warning log. Reuse the existing hidden-argument redaction utility, if available, so both persisted errors and logger.warning output from the job failure path redact injected secret values consistently.src/corral/backend/jobs.py-436-454 (1)
436-454: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize submission and shutdown behind a closed-state gate.
_executorsis mutated under_executors_guardbut iterated here under_lock. Concurrent submission can raise “dictionary changed size,” escape the snapshot, or start work after shutdown. Mark the manager closed, reject new submissions, and snapshot executors under the same lifecycle synchronization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/jobs.py` around lines 436 - 454, Update the lifecycle coordination around shutdown and submission: add a closed-state gate that shutdown marks before taking its executor snapshot, reject new submissions after closure, and use _executors_guard consistently when mutating or snapshotting _executors. Ensure concurrent submissions cannot start work after shutdown or escape the synchronized executor snapshot, while preserving the existing job cancellation and executor.shutdown behavior.src/corral/backend/background_tools.py-204-217 (1)
204-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreflight generated tool names instead of silently overwriting tools.
A user tool named
start_<tool>,cancel_job, or another control name is replaced without warning. Build all generated names first, reject collisions with a configuration error, then mutateenv.tools.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/background_tools.py` around lines 204 - 217, Update the background tool setup around _make_start_tool and _make_control_tools to precompute every generated tool name before modifying env.tools. Detect collisions with existing user tools or duplicate generated names and raise the established configuration error; only populate env.tools after validation succeeds, preventing silent overwrites.src/corral/backend/executors.py-281-285 (1)
281-285: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail instead of falling back to the server working directory.
When a specified workspace is missing,
cwd=Noneruns the isolated job in the server process directory. This can read or modify another trial’s files.Proposed fix
- cwd = ( - work.workspace - if work.workspace and Path(work.workspace).is_dir() - else None - ) + cwd = None + if work.workspace is not None: + workspace = Path(work.workspace) + if not workspace.is_dir(): + raise FileNotFoundError( + f"Job workspace does not exist: {work.workspace!r}" + ) + cwd = str(workspace)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/executors.py` around lines 281 - 285, Update the cwd selection in the executor path around work.workspace so that an explicitly specified workspace must exist and be a directory; otherwise fail the job instead of assigning None. Preserve the current None behavior only when no workspace was specified, and ensure the failure occurs before launching the isolated job.src/corral/backend/jobs.py-253-258 (1)
253-258: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeep-copy visible arguments before storing provenance.
The two shallow copies share nested lists and dictionaries. An in-process tool that mutates its arguments also rewrites the supposedly immutable reported arguments.
Proposed fix
+from copy import deepcopy ... - arguments=dict(visible_arguments), + arguments=deepcopy(visible_arguments),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/jobs.py` around lines 253 - 258, Update the JobContext construction in the job execution flow to deep-copy visible_arguments before assigning it to the arguments provenance field, preventing nested lists and dictionaries from sharing mutable references with tool inputs. Preserve the existing call_arguments handling and other JobContext fields.src/corral/backend/executors.py-183-191 (1)
183-191: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTerminate the whole process group on cancellation.
start_new_session=Trueputs the child in its own process group, but_terminate_process()only signals the leader. Grandchildren can survive the cancel and keep running after cleanup; send SIGTERM/SIGKILL to the group instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/executors.py` around lines 183 - 191, Update _terminate_process to signal the child’s entire process group, using the group created by start_new_session=True, rather than only calling methods on the leader process. Preserve the existing early-exit and grace-period escalation behavior: send group termination first, wait, then send group kill after subprocess.TimeoutExpired, while handling process-group signaling safely.src/corral/backend/schema.py-75-75 (1)
75-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate and cap
tool_jobs_per_trial.
This request model accepts any integer and forwards it intomax_job_concurrencyunchanged; negatives are only clamped later, and large values can still size an oversized pool. Addge=1and a reasonable upper bound here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/schema.py` at line 75, Update the request model field tool_jobs_per_trial to validate values as integers greater than or equal to 1 and enforce a reasonable maximum before forwarding it to max_job_concurrency. Preserve the optional None default.src/corral/backend/server.py-353-395 (1)
353-395: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep every task in an episode on one backend and dependency store.
A process task writes outputs to the worker-local
episode_stores, while a thread task with the sameepisode_idreads the server-sideepisode_registry. Mixed-concurrency dependency chains therefore cannot see outputs across that boundary.Choose the backend for the entire dependency component—routing every task in an episode through the pinned worker—or explicitly synchronize completed outputs across backends.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/server.py` around lines 353 - 395, Update the trial backend selection around _acquire_worker and episode_registry so every task in the same episode uses one backend and dependency store. For an episode containing process-backed work, route all its tasks through the pinned WorkerTrialRuntime rather than allowing thread tasks to use InProcessTrialRuntime with the server-side episode_registry. Preserve standalone trial behavior and ensure shared dependency outputs remain visible across the entire episode.src/corral/backend/env.py-667-695 (1)
667-695: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse a runtime-wide execution gate for
SERIALcalls and finalization.
_state_lockis not held while keyed or read-only tools execute. They can therefore overlap aSERIALtool, andsubmit_answer()can finalize while they are still running and later recording results.Use a global readers-writer gate:
SERIAL, submit, and surrender take the write side; concurrent tools take the read side before their resource-specific lock.Also applies to: 910-920
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/env.py` around lines 667 - 695, Update _execution_guard to use a runtime-wide readers-writer execution gate: acquire its write side for SERIAL tools and the read side for keyed, read-only, and other concurrent tools before any resource-specific lock. Update submit_answer() and surrender() finalization paths to acquire the same gate’s write side, ensuring finalization cannot overlap tool execution or result recording.src/corral/backend/env.py-329-331 (1)
329-331: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReset background-job state at the trial boundary.
The old manager is snapshotted after the new trial starts, and the new
jobsfield is never cleared.
src/corral/backend/env.py#L329-L331: shut down and snapshot the old manager beforestart_new_trial().src/corral/backend/env.py#L822-L846: attach the replacement manager without shutting down the previous one after reset.src/corral/backend/env.py#L904-L908: ensure this snapshot targets the trial that owned the manager.src/corral/backend/state.py#L93-L97: clearjobsafter archiving the completed trial.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/env.py` around lines 329 - 331, Reset background-job state at the trial boundary: in src/corral/backend/env.py lines 329-331, shut down and snapshot the existing JobManager before start_new_trial(); in lines 822-846, attach the replacement manager without shutting down the previous manager after reset; in lines 904-908, snapshot against the trial that owned the manager; and in src/corral/backend/state.py lines 93-97, clear jobs after archiving the completed trial. Use the existing background-manager and trial-management symbols to preserve ownership and lifecycle ordering.src/corral/backend/trial_worker.py-254-266 (1)
254-266: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a health-aware lifecycle for worker handles.
Failed or contaminated workers can currently be published, leaked, or returned to the idle queue.
src/corral/backend/trial_worker.py#L254-L266: enqueue handles only after a successful startup handshake.src/corral/backend/trial_worker.py#L98-L106: send a structured startup failure instead of silently exiting.src/corral/backend/trial_worker.py#L189-L192: remove failed-to-close runtimes from the child registry.src/corral/backend/trial_worker.py#L354-L362: mark handles unhealthy on RPC/EOF failure.src/corral/backend/trial_worker.py#L402-L408: retire/restart unhealthy handles instead of releasing them.src/corral/backend/server.py#L357-L373: clean up the lease or episode pin if opening the runtime fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/trial_worker.py` around lines 254 - 266, Implement health-aware worker lifecycle management: in src/corral/backend/trial_worker.py lines 254-266, perform a successful startup handshake before publishing handles to _handles and _idle; at lines 98-106, have _worker_loop send a structured startup failure instead of exiting silently; at lines 189-192, remove runtimes that fail to close from the child registry; at lines 354-362, mark handles unhealthy on RPC or EOF failures; at lines 402-408, retire and restart unhealthy handles rather than releasing them; and in src/corral/backend/server.py lines 357-373, clean up the lease or episode pin when opening a runtime fails.src/corral/backend/tool.py-74-94 (1)
74-94: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRequire
concurrency_keyfor keyed concurrency modes.
CONCURRENTandCONCURRENT_READwithout a key silently run with no execution lock, turning a configuration mistake into a data race. Reject these combinations during construction; callers wanting no lock should useREAD_ONLY.Proposed validation
- self.concurrency = ToolConcurrency(concurrency) + resolved_concurrency = ToolConcurrency(concurrency) + if resolved_concurrency in { + ToolConcurrency.CONCURRENT, + ToolConcurrency.CONCURRENT_READ, + } and not concurrency_key: + raise ValueError( + f"{resolved_concurrency.value} requires a concurrency_key" + ) + self.concurrency = resolved_concurrency🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/tool.py` around lines 74 - 94, Update the Tool constructor where self.concurrency is initialized to validate keyed modes: when concurrency is CONCURRENT or CONCURRENT_READ, require a non-empty concurrency_key and reject construction otherwise. Preserve existing behavior for SERIAL and READ_ONLY, with READ_ONLY remaining the mode for callers that do not want a lock.src/corral/backend/mcp_tasks.py-97-108 (1)
97-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn a consistent
Task.ttlacross the task lifecycle.Creation echoes the requested TTL, but the job record never stores or enforces it, and later get/list/cancel responses always return
None. Either returnNoneon creation too, or persist and surface the same TTL everywhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/backend/mcp_tasks.py` around lines 97 - 108, Make Task.ttl consistent across creation and all later task responses. Since job records do not persist or enforce TTL, update the task creation path to return None as well, matching _task_from_job and the get/list/cancel response paths; do not echo the requested TTL unless it is persisted and surfaced throughout the lifecycle.
🟡 Minor comments (7)
src/corral/utils/io_tools.py-340-344 (1)
340-344: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSurface file-inspection failures instead of reporting no matches.
Silently skipping every
file_infofailure can return"No matches found"when files were actually unreadable or unavailable.Proposed fix
try: if fs_manager.file_info(file_path)["type"] != "file": continue - except Exception: + except Exception as e: + all_matches.append( + { + "file": file_path, + "error": f"Error inspecting file: {e!s}", + } + ) continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/corral/utils/io_tools.py` around lines 340 - 344, The file filtering logic around fs_manager.file_info(file_path) currently suppresses inspection errors and can incorrectly produce no matches; surface these failures instead of continuing as though the file were absent. Update the surrounding search flow to propagate or report the exception while preserving the existing continue behavior for successfully inspected non-file entries.Source: Linters/SAST tools
tests/test_async_router.py-312-320 (1)
312-320: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose every owned
AsyncCorralRouterafter the assertion.These routers construct their own
httpx.AsyncClient, but none enters the router context or callsaclose(), potentially leaking transport resources during the test suite.
tests/test_async_router.py#L312-L320: closerouterin afinallyblock.tests/test_async_router.py#L323-L333: close the parentrouterafter checking the synchronous trial view.tests/test_async_router.py#L336-L343: retain the async router in a variable, convert it, then close it.tests/test_abench.py#L945-L963: create and useasync_routerinside an async context manager.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_async_router.py` around lines 312 - 320, Close every owned AsyncCorralRouter after use: in tests/test_async_router.py ranges 312-320, wrap the assertions in a finally block that closes router; in ranges 323-333, close the parent router after checking the synchronous trial view; and in ranges 336-343, retain the async router variable, convert it, then close it. In tests/test_abench.py range 945-963, create and use async_router within an async context manager.tests/test_abench.py-831-833 (1)
831-833: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a raw string for the exception regex.
Line 832 triggers Ruff RUF043 because
.*is a regex metacharacter in a normal string.- with pytest.raises(ValueError, match="agent.*agent_factory"): + with pytest.raises(ValueError, match=r"agent.*agent_factory"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_abench.py` around lines 831 - 833, Update the pytest.raises match argument in test_runner_requires_an_agent_source to use a raw string literal for the regex, preserving the existing pattern and assertion behavior.Source: Linters/SAST tools
tests/backend/test_mcp_tasks.py-235-278 (1)
235-278: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the worker before deleting the trial runtime.
Line 276 may wait for
gate.wait(timeout=5)because the running thread cannot be forcibly cancelled andgate.set()executes only afterward.Proposed fix
again = _call(client, rid, "tasks/cancel", {"taskId": task_id}, 3) assert again["error"]["code"] == INVALID_PARAMS + gate.set() assert client.delete(f"/trials/{rid}").status_code == 200 finally: gate.set() # release the worker thread🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/backend/test_mcp_tasks.py` around lines 235 - 278, Set the test-controlled gate before deleting the trial runtime, so the background worker exits before the client.delete call. Move the gate.set() cleanup from the outer finally into the active test flow immediately after the terminal cancellation assertions, while retaining finally cleanup as needed for failure paths.tests/backend/test_jobs.py-196-216 (1)
196-216: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSynchronize the jobs instead of relying on a 100 ms overlap window.
On a loaded runner, the second worker may start after the first sleep completes, producing a false failure. Use a two-party
threading.Barrierinside_workto prove both jobs entered execution concurrently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/backend/test_jobs.py` around lines 196 - 216, Update test_different_concurrency_keys_overlap to synchronize both jobs with a two-party threading.Barrier created before _work; have _work wait on that barrier after entering execution, then retain the result assertions and cleanup so the test proves concurrent execution without relying on sleep timing.tests/agents/test_codex.py-334-357 (1)
334-357: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest read-only and write-only tool sets separately.
Passing both
write_fileandread_filemeans this test still passes if the directive is mistakenly triggered byread_filealone.Proposed test adjustment
- with_write = agent._developer_instructions( - enable_surrender=False, tool_names=["write_file", "read_file"] + read_only = agent._developer_instructions( + enable_surrender=False, tool_names=["read_file"] ) + assert "RELATIVE path" not in read_only + + with_write = agent._developer_instructions( + enable_surrender=False, tool_names=["write_file"] + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents/test_codex.py` around lines 334 - 357, Update test_relative_path_directive_only_with_file_tools to test write_file and read_file independently: assert the directive is absent for a read-only tool set and present for a write-only tool set, while retaining the existing no-tools and non-file assertions and final-answer assertion.docs/documentation/how_to_guides.md-11-11 (1)
11-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required space after the heading marker.
###6is not a valid ATX heading and triggers MD018.Proposed fix
-###6. How to [run benchmarks concurrently (`abench`)](how_tos/concurrent_benchmarking.md) +### 6. How to [run benchmarks concurrently (`abench`)](how_tos/concurrent_benchmarking.md)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/documentation/how_to_guides.md` at line 11, Update the “How to run benchmarks concurrently” heading in how_to_guides.md to include a space between the Markdown heading markers and the heading text, changing the invalid ###6 form to valid ATX syntax while preserving the existing heading level and link.Source: Linters/SAST tools
🧹 Nitpick comments (2)
tests/test_tools.py (1)
523-524: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dangling tuple element.
The trailing
, namecreates a tuple that is immediately discarded. This appears to be a leftover from a previous assertion or debug statement and can be safely removed.♻️ Proposed fix
- validator_for(mcp_schema).check_schema(mcp_schema), name + validator_for(mcp_schema).check_schema(mcp_schema)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tools.py` around lines 523 - 524, Remove the trailing “, name” from the validator_for(mcp_schema).check_schema(mcp_schema) statement in the test, leaving only the schema validation call and preserving the surrounding MCP schema setup.tasks/catalyst/src/catalyst/tools.py (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer resolving
.envrelative to the module file.Using
load_dotenv("../../.env")resolves relative to the process's current working directory (CWD). If the module is executed or imported from a different directory (like the repository root), the path will fail to resolve correctly. Consider anchoring the path to__file__for robustness.♻️ Proposed refactor
- load_dotenv("../../.env") + from pathlib import Path + # Adjust the number of `.parent` calls to point to the correct directory + env_path = Path(__file__).resolve().parent.parent.parent / ".env" + load_dotenv(env_path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tasks/catalyst/src/catalyst/tools.py` at line 14, Update the module-level load_dotenv call to resolve the .env path relative to __file__ rather than the process CWD, preserving the existing environment-loading behavior regardless of where the module is executed or imported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Major comments:
In `@src/corral/agents/base_agent.py`:
- Around line 431-471: Update BaseAgent.arun and the related to_thread.run_sync
usage for answer extraction so cancellation does not wait for blocking worker
threads to finish. Use a cooperatively cancellable execution path or a
separately terminable worker, while preserving synchronous router adaptation
through as_sync_interface and the existing run behavior.
In `@src/corral/agents/claude_code.py`:
- Around line 884-938: Ensure both run and arun execute the AFTER_TASK lifecycle
hook after harness completion, failure, or cancellation, while retaining cwd
cleanup. Update the shared finalization or each entry point around
_prepare_run/_aprepare_run and the existing finally blocks so the hook always
runs once for every outcome, including exceptions and timeouts.
In `@src/corral/backend/background_tools.py`:
- Around line 204-217: Update the background tool setup around _make_start_tool
and _make_control_tools to precompute every generated tool name before modifying
env.tools. Detect collisions with existing user tools or duplicate generated
names and raise the established configuration error; only populate env.tools
after validation succeeds, preventing silent overwrites.
In `@src/corral/backend/env.py`:
- Around line 667-695: Update _execution_guard to use a runtime-wide
readers-writer execution gate: acquire its write side for SERIAL tools and the
read side for keyed, read-only, and other concurrent tools before any
resource-specific lock. Update submit_answer() and surrender() finalization
paths to acquire the same gate’s write side, ensuring finalization cannot
overlap tool execution or result recording.
- Around line 329-331: Reset background-job state at the trial boundary: in
src/corral/backend/env.py lines 329-331, shut down and snapshot the existing
JobManager before start_new_trial(); in lines 822-846, attach the replacement
manager without shutting down the previous manager after reset; in lines
904-908, snapshot against the trial that owned the manager; and in
src/corral/backend/state.py lines 93-97, clear jobs after archiving the
completed trial. Use the existing background-manager and trial-management
symbols to preserve ownership and lifecycle ordering.
In `@src/corral/backend/executors.py`:
- Around line 281-285: Update the cwd selection in the executor path around
work.workspace so that an explicitly specified workspace must exist and be a
directory; otherwise fail the job instead of assigning None. Preserve the
current None behavior only when no workspace was specified, and ensure the
failure occurs before launching the isolated job.
- Around line 183-191: Update _terminate_process to signal the child’s entire
process group, using the group created by start_new_session=True, rather than
only calling methods on the leader process. Preserve the existing early-exit and
grace-period escalation behavior: send group termination first, wait, then send
group kill after subprocess.TimeoutExpired, while handling process-group
signaling safely.
In `@src/corral/backend/jobs.py`:
- Around line 219-232: Update the manager-level execution flow around
_resolve_executor to add and acquire a single semaphore shared by all executor
types, limiting total active work to _max_concurrency across thread, process,
and subprocess executors. Ensure the semaphore is released when each job
finishes, including failure or cancellation paths, while preserving the existing
per-name executor caching.
- Around line 339-345: Sanitize exception messages in the background job
exception handler before assigning to JobRecord.error or emitting the warning
log. Reuse the existing hidden-argument redaction utility, if available, so both
persisted errors and logger.warning output from the job failure path redact
injected secret values consistently.
- Around line 436-454: Update the lifecycle coordination around shutdown and
submission: add a closed-state gate that shutdown marks before taking its
executor snapshot, reject new submissions after closure, and use
_executors_guard consistently when mutating or snapshotting _executors. Ensure
concurrent submissions cannot start work after shutdown or escape the
synchronized executor snapshot, while preserving the existing job cancellation
and executor.shutdown behavior.
- Around line 253-258: Update the JobContext construction in the job execution
flow to deep-copy visible_arguments before assigning it to the arguments
provenance field, preventing nested lists and dictionaries from sharing mutable
references with tool inputs. Preserve the existing call_arguments handling and
other JobContext fields.
In `@src/corral/backend/mcp_tasks.py`:
- Around line 97-108: Make Task.ttl consistent across creation and all later
task responses. Since job records do not persist or enforce TTL, update the task
creation path to return None as well, matching _task_from_job and the
get/list/cancel response paths; do not echo the requested TTL unless it is
persisted and surfaced throughout the lifecycle.
In `@src/corral/backend/schema.py`:
- Line 75: Update the request model field tool_jobs_per_trial to validate values
as integers greater than or equal to 1 and enforce a reasonable maximum before
forwarding it to max_job_concurrency. Preserve the optional None default.
In `@src/corral/backend/server.py`:
- Around line 353-395: Update the trial backend selection around _acquire_worker
and episode_registry so every task in the same episode uses one backend and
dependency store. For an episode containing process-backed work, route all its
tasks through the pinned WorkerTrialRuntime rather than allowing thread tasks to
use InProcessTrialRuntime with the server-side episode_registry. Preserve
standalone trial behavior and ensure shared dependency outputs remain visible
across the entire episode.
In `@src/corral/backend/tool.py`:
- Around line 74-94: Update the Tool constructor where self.concurrency is
initialized to validate keyed modes: when concurrency is CONCURRENT or
CONCURRENT_READ, require a non-empty concurrency_key and reject construction
otherwise. Preserve existing behavior for SERIAL and READ_ONLY, with READ_ONLY
remaining the mode for callers that do not want a lock.
In `@src/corral/backend/trial_worker.py`:
- Around line 254-266: Implement health-aware worker lifecycle management: in
src/corral/backend/trial_worker.py lines 254-266, perform a successful startup
handshake before publishing handles to _handles and _idle; at lines 98-106, have
_worker_loop send a structured startup failure instead of exiting silently; at
lines 189-192, remove runtimes that fail to close from the child registry; at
lines 354-362, mark handles unhealthy on RPC or EOF failures; at lines 402-408,
retire and restart unhealthy handles rather than releasing them; and in
src/corral/backend/server.py lines 357-373, clean up the lease or episode pin
when opening a runtime fails.
In `@src/corral/concurrency.py`:
- Around line 104-122: Update the dataclass initialization around __post_init__
so per_model is copied into an immutable mapping after validation, preventing
mutations to the caller’s dictionary from affecting scheduler limits. Preserve
None as None and retain the existing validation behavior and per-model values.
In `@src/corral/router/routes.py`:
- Around line 523-540: Update the router constructor’s owned-client branch in
__init__ to create httpx.AsyncClient with a configurable finite timeout instead
of httpx.Timeout(None). Preserve externally supplied clients and their timeout
policies, and retain the existing ownership tracking via _owns_client.
In `@src/corral/run.py`:
- Around line 108-127: Update _fetch_concurrency_modes so failures or missing
get_concurrency_modes metadata do not return an empty map that marks all tasks
as thread-safe. Fail closed by aborting or ensuring selected tasks are
serialized until their declared modes are available, while preserving normal
mode retrieval when acall(getter) succeeds.
- Around line 2181-2190: Update the collector’s task result handling so
checkpointed trial state is keyed by trial index rather than completion order.
Buffer out-of-order completions in task_results and append only the next
contiguous trial indices before calling checkpoint_saver, ensuring resume logic
cannot rerun or duplicate completed attempts when per_task is greater than one.
- Around line 1456-1472: Update the concurrent-mode checks surrounding
self.abench and the corresponding logic at the referenced later branch so
global_trials=1 remains on the serial path when per_task exceeds one. Base
async/concurrent selection on whether the configured global limit permits
overlap, or validate and reject per_task greater than global_trials during
configuration validation; preserve concurrent behavior when actual overlap is
possible.
- Around line 152-186: The current _serial_lease logic only gates stateful
tasks, allowing "thread" tasks to overlap "serial" environments despite the
never-overlap contract. Update the admission logic around _serial_lease and its
call sites (including the referenced execution paths) to use shared/exclusive
coordination: serial tasks require exclusive access, while ordinary tasks
acquire shared access, preserving existing limiter behavior and ensuring serial
tasks overlap no other task.
- Around line 2105-2110: Replace the materialized work_items and per-trial AnyIO
task creation in the scheduler with a bounded producer/consumer stream and fixed
worker pool, so queued tasks are capped rather than proportional to total
trials. Apply the same bounded in-flight approach to episode execution and the
corresponding scheduler paths around the referenced loops, while preserving
resume filtering and the global concurrency limit.
In `@src/corral/utils/io_tools.py`:
- Around line 171-172: Update the regex matching loop in _grep_file so each
re.search operation is subject to a hard timeout or runs in a killable worker,
preventing catastrophic-backtracking patterns from blocking call_tool
indefinitely; preserve the existing line iteration and match-result behavior.
In `@src/corral/utils/tool_helpers.py`:
- Around line 81-85: Update the extracted-path resolution logic around
Path(extracted_path).exists() to resolve relative paths against base_dir first,
using the process CWD only when base_dir is unavailable or the base_dir-relative
candidate does not exist. Preserve absolute paths and the existing
find_file_by_name fallback, ensuring trial-isolated files take precedence over
same-named CWD files.
In `@tests/backend/test_job_executors.py`:
- Around line 108-127: Update the cancellation tests around
test_subprocess_executor_cancellation_kills_the_process and the additionally
affected test blocks so worker threads are daemonized and cleanup cannot be
skipped. Wrap cancellation, joining, assertions, and executor cleanup in a
try/finally structure, set the cancellation event in finally, and invoke
executor.shutdown(wait=False) there so regressions cannot leave the suite
hanging.
In `@tests/backend/test_work_dir_isolation.py`:
- Around line 22-36: Update smart_resolve_path and
test_smart_resolve_path_scopes_the_search_to_base_dir so relative paths are
resolved against base_dir before checking the process CWD. Add a cwd-level
answer.json collision in the test and assert each workspace still resolves its
own file, preventing the CWD path from overriding trial isolation.
---
Minor comments:
In `@docs/documentation/how_to_guides.md`:
- Line 11: Update the “How to run benchmarks concurrently” heading in
how_to_guides.md to include a space between the Markdown heading markers and the
heading text, changing the invalid ###6 form to valid ATX syntax while
preserving the existing heading level and link.
In `@src/corral/utils/io_tools.py`:
- Around line 340-344: The file filtering logic around
fs_manager.file_info(file_path) currently suppresses inspection errors and can
incorrectly produce no matches; surface these failures instead of continuing as
though the file were absent. Update the surrounding search flow to propagate or
report the exception while preserving the existing continue behavior for
successfully inspected non-file entries.
In `@tests/agents/test_codex.py`:
- Around line 334-357: Update test_relative_path_directive_only_with_file_tools
to test write_file and read_file independently: assert the directive is absent
for a read-only tool set and present for a write-only tool set, while retaining
the existing no-tools and non-file assertions and final-answer assertion.
In `@tests/backend/test_jobs.py`:
- Around line 196-216: Update test_different_concurrency_keys_overlap to
synchronize both jobs with a two-party threading.Barrier created before _work;
have _work wait on that barrier after entering execution, then retain the result
assertions and cleanup so the test proves concurrent execution without relying
on sleep timing.
In `@tests/backend/test_mcp_tasks.py`:
- Around line 235-278: Set the test-controlled gate before deleting the trial
runtime, so the background worker exits before the client.delete call. Move the
gate.set() cleanup from the outer finally into the active test flow immediately
after the terminal cancellation assertions, while retaining finally cleanup as
needed for failure paths.
In `@tests/test_abench.py`:
- Around line 831-833: Update the pytest.raises match argument in
test_runner_requires_an_agent_source to use a raw string literal for the regex,
preserving the existing pattern and assertion behavior.
In `@tests/test_async_router.py`:
- Around line 312-320: Close every owned AsyncCorralRouter after use: in
tests/test_async_router.py ranges 312-320, wrap the assertions in a finally
block that closes router; in ranges 323-333, close the parent router after
checking the synchronous trial view; and in ranges 336-343, retain the async
router variable, convert it, then close it. In tests/test_abench.py range
945-963, create and use async_router within an async context manager.
---
Nitpick comments:
In `@tasks/catalyst/src/catalyst/tools.py`:
- Line 14: Update the module-level load_dotenv call to resolve the .env path
relative to __file__ rather than the process CWD, preserving the existing
environment-loading behavior regardless of where the module is executed or
imported.
In `@tests/test_tools.py`:
- Around line 523-524: Remove the trailing “, name” from the
validator_for(mcp_schema).check_schema(mcp_schema) statement in the test,
leaving only the schema validation call and preserving the surrounding MCP
schema setup.
Summary by CodeRabbit