fix: shard subprocesses inherit CWD and profile for correct backend selection - #467
Open
rysweet wants to merge 4 commits into
Open
fix: shard subprocesses inherit CWD and profile for correct backend selection#467rysweet wants to merge 4 commits into
rysweet wants to merge 4 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Removes the explicit 5-minute job timeout that was cancelling the PR benchmark check before it could complete. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…election spawn_shard was missing two things that caused Copilot to be selected instead of Azure AI Foundry during benchmark eval runs: 1. No .current_dir(): shards inherited the parent process CWD. If skwaq gym eval was invoked from a directory other than the repo root, Config::load() in the shard subprocess could not find skwaq.toml and fell back to Config::default() which has reasoning = "copilot". Fix: set .current_dir(skwaq_root) so Config::load() always finds the repo-root skwaq.toml regardless of invocation directory. 2. No --profile forwarding: when gym eval --profile azure was used, the profile's LLM backend override was applied in the parent process but the shard subprocesses were spawned without --profile, so they used the raw skwaq.toml config rather than the profile-merged config. Fix: forward --profile <name> to shards when a profile is active. Both call sites (initial spawn and retry-on-early-death) are updated. Root-cause analysis of Copilot vs Azure AI Foundry selection: - Config::load() searches: skwaq.toml (CWD) -> .skwaq/config.toml (CWD) -> ~/.skwaq/config.toml - default_llm_backend() returns "copilot" (compile-time default) - If no config file is found, Config::default() selects Copilot - Shard subprocesses are the code path where CWD-dependency mattered Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…on patch fallback
- Add SOURCE_LAYOUT_DIRS constant listing conventional project source
directories (src, lib, include, source, Sources, core, main, libs, includes)
- single_child_dir_without_sources() now returns None when the sole child
dir is a SOURCE_LAYOUT_DIR, keeping source_tree_root() at the project
root instead of descending into the source tree
- Emit tracing::warn when patch_affected_files / paired_case_affected_files
both fail and the shallow fallback scan is used, making false-negatives
observable in gym logs
- Add three regression tests:
test_source_tree_root_stops_at_project_root_not_src
test_source_tree_root_stops_at_conventional_lib
test_patch_affected_files_resolves_src_relative_paths
Fixes false negatives where patch-relative paths like 'src/parser.c' failed
to resolve because source_tree_root() had descended into the 'src/' child
instead of stopping at the project root wrapper directory.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Superseded by #468. This older branch/PR path picked up unrelated scope and workflow residue; the focused replacement PR carries the backend-selection + CyberGym fixes with QA/docs evidence. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR contains two independent fixes to
skwaq gym evalcorrectness.Fix 1: Shard subprocesses inherit CWD and profile for correct backend selection
Root Cause
During
skwaq gym eval, shard subprocesses were selecting the Copilot backend instead of Azure AI Foundry. Traced to two bugs inspawn_shard()incrates/cli/src/commands/gym_cmd.rs.Config loading code path
Config::load()→find_config_file()searches:skwaq.tomlin current working directory.skwaq/config.tomlin CWD~/.skwaq/config.tomlIf no file is found →
Config::default()→default_llm_backend()returns"copilot".Bug 1: Missing
.current_dir(skwaq_root)spawn_sharddid not call.current_dir(), so shard subprocesses inherited the parent's CWD. Ifskwaq gym evalwas invoked from a directory other than the repo root,Config::load()in each shard would not findskwaq.tomland fall back toConfig::default()— selecting Copilot regardless of what the config file specifies.Fix: Set
.current_dir(skwaq_root)so shards always load config from the repo root.Bug 2:
--profilenot forwarded to shardsWhen running
gym eval --profile azure, the profile's LLM backend override (reasoning = "azure") was applied in the parent process but not forwarded to shard subprocesses. Each shard calledConfig::load()directly fromskwaq.tomlwithout the profile overlay, potentially selecting a different backend.Fix: Forward
--profile <name>to shard subprocess args when a profile is active.Changes (Fix 1)
crates/cli/src/commands/gym_cmd.rs:spawn_shardgains two new parameters (skwaq_root,profile); both call sites updated (initial spawn + retry-on-early-death).Fix 2: CyberGym adapter — prevent
source_tree_root()over-descent; WARN on patch fallbackRoot Cause
CyberGym cases are often packaged as
case_dir/<project-1.2.3>/src/vuln.c. The helpersource_tree_root()was supposed to strip the packaging wrapper (project-1.2.3/) to find the project root. However,single_child_dir_without_sources()would descend one more level intosrc/because it had no guard against conventional source-layout directories. This caused patch-relative paths likesrc/parser.cto fail resolution, producing false negatives.Additionally, when both
patch_affected_filesandpaired_case_affected_filesfailed, the shallow fallback scan ran silently with no log output, making false negatives invisible.Changes (Fix 2)
crates/gym/src/adapters/cybergym.rs:SOURCE_LAYOUT_DIRSconstant listing conventional source directories (src,lib,include,source,Sources,core,main,libs,includes).single_child_dir_without_sources()now returnsNonewhen the sole child directory name is inSOURCE_LAYOUT_DIRS, keepingsource_tree_root()at the project root instead of descending into the source tree.test_source_tree_root_stops_at_project_root_not_srctest_source_tree_root_stops_at_conventional_libtest_patch_affected_files_resolves_src_relative_pathsVerification
Fix 1
cargo build -p skwaq✅cargo clippy -p skwaq --tests -- -D warnings✅Fix 2
cargo test -q -p skwaq-gym✅ (all 85 tests pass including 3 new regression tests)cargo clippy -q -p skwaq-gym --tests -- -D warnings✅ (zero warnings)CI
CI is triggered on push. Any CI failures will be documented here as root causes are identified.