feat: HumanEval coding benchmark for TRINITY training + retry/timeout reliability fixes - #3
feat: HumanEval coding benchmark for TRINITY training + retry/timeout reliability fixes#3dangeReis wants to merge 10 commits into
Conversation
mini.py now loads the HF-published materialized safetensors checkpoints (embedder, decoder blocks, ffn, lm head, router head) instead of the now-missing model_iter_60.npy CMA-ES vector, via FUGU_CHECKPOINT_DIR. Handles the Elixir/Bumblebee "kernel" tensor transposition vs PyTorch's nn.Linear convention. Self-test: 36/37 = 97% agent / 97% role accuracy against real weights, reconfirmed live 2026-07-12. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ath invocation serve.py loads mini via sys.path.insert + bare `import mini`, which breaks mini's `from .materialized import ...` relative import (no known parent package). Only affected the FUGU_CHECKPOINT_DIR path (serve.py's default), so `python -m openfugu.mini --self-test` masked it. Fall back to an absolute import when the relative one fails. Verified: self-test still 97% agent/role match; serve.py now boots and routes a real request through 7 NIM-backed litellm slots end to end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… fixes GSM8K saturated at one worker hitting 100%, giving sep-CMA-ES nothing to learn from. Added train_trinity_coding.py using HumanEval (auto-graded via real subprocess execution) as a more discriminating benchmark — baseline per-worker solve rates ranged 0.75-0.95, and the trained coordinator hit 1.000, beating every single worker. Also fixes two real bugs found while validating train_trinity_real.py: - litellm.RateLimitError (429) was silently scored as a wrong answer with no retry, corrupting the reward signal under NIM rate limiting. - litellm.completion() had no timeout, so a stalled connection could hang the whole run indefinitely with no exception to catch. Both scripts now retry 429s with exponential backoff and use timeout=45. Verified: full live run against the NIM worker pool (NVIDIA_API_KEY_2) completed successfully — PASS, coordinator >= best single worker. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code review flagged that a call which exhausted retries (RateLimitError) or hit a non-rate-limit exception (e.g. litellm.Timeout) was still being cached as ok=0.0 under solve_cache[(worker, task)] — permanently marking that pair "unsolved" for the rest of the run, even though no answer was ever actually graded. This is the same class of silent reward-signal corruption the earlier retry/timeout fix was meant to eliminate. Now a call that never succeeds returns 0.0 for that single fitness evaluation but is not cached, so a later iteration can retry it for real. Also scrub API-key env vars from the subprocess environment used to execute candidate code in train_trinity_coding.py, since it inherits the full environment by default and candidate code is untrusted model output. Re-verified against the live NIM pool: nvidia/openai/gpt-oss-20b was hitting a sustained outage (10/10 consecutive timeouts) during this verification pass, so the confirming run used the remaining 3-worker pool (nemotron-3-super-120b-a12b, diffusiongemma-26b-a4b-it, nemotron-3-nano-omni-30b-a3b-reasoning) — PASS, coordinator (0.925) >= best single worker (0.925). gpt-oss-20b remains a valid --slot-models option; exclude it only while NIM reports it unhealthy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ining feat: HumanEval coding benchmark for TRINITY training + retry/timeout fixes
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideAdds a new TRINITY HumanEval-based coding training script, introduces a materialized-weights loader path for FuguRouter, and hardens LLM worker calls with retries, timeouts, and safer caching/environment handling. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 3 other issues
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="train/train_trinity_coding.py" line_range="125" />
<code_context>
+ call_succeeded = False
+ # Candidate code runs via subprocess in the parent's env; scrub API keys so
+ # hallucinated/adversarial completions can't read or exfiltrate them.
+ run_env = {k: v for k, v in os.environ.items() if "KEY" not in k.upper()}
+ for attempt in range(5):
+ try:
</code_context>
<issue_to_address>
**🚨 issue (security):** Environment scrubbing for untrusted code is too narrow and may leave other secrets accessible.
Filtering only env vars containing "KEY" still leaves many sensitive values (tokens, passwords, org/tenant IDs, etc.) visible to untrusted code. Given this runs arbitrary completions, consider starting from an empty env and explicitly allowlisting only what’s required (e.g. PATH, PYTHONPATH if needed), rather than copying almost everything and subtracting keys with "KEY" in the name.
</issue_to_address>
### Comment 2
<location path="openfugu/materialized.py" line_range="41-48" />
<code_context>
+ if not os.path.exists(abs_chk_path):
+ raise FileNotFoundError(f"Checkpoint file not found: {abs_chk_path}")
+
+ tensors_dict = load_file(abs_chk_path)
+ if path_key not in tensors_dict:
+ # fallback to checking if any key in tensors_dict contains the name
+ matching_keys = [k for k in tensors_dict if path_key in k]
+ if matching_keys:
+ tensor = tensors_dict[matching_keys[0]]
+ else:
+ tensor = list(tensors_dict.values())[0]
+ else:
+ tensor = tensors_dict[path_key]
</code_context>
<issue_to_address>
**issue (bug_risk):** Fallback to the first tensor when `path_key` is missing is risky and can silently load wrong weights.
When `path_key` is missing and no partial match exists, this falls back to `list(tensors_dict.values())[0]`, which can bind an unrelated tensor to `source_name` and corrupt the model in a non-obvious way. Instead of guessing, consider raising an error that includes `path_key` and the available keys.
</issue_to_address>
### Comment 3
<location path="openfugu/materialized.py" line_range="49-53" />
<code_context>
+ else:
+ tensor = tensors_dict[path_key]
+
+ expected_shape = expected_shapes[source_name]
+ if source_name in [
+ "model.layers.26.self_attn.k_proj.weight",
</code_context>
<issue_to_address>
**suggestion:** Indexing `expected_shapes[source_name]` without validation can throw on manifest/key drift.
This assumes every `source_name` in the manifest exists in `expected_shapes`, so any new/renamed tensor will raise a bare `KeyError`. Consider checking membership first and raising a clearer error that explains the unexpected `source_name` and the mismatch between the checkpoint and the hardcoded expectations.
```suggestion
else:
tensor = tensors_dict[path_key]
if source_name not in expected_shapes:
available_expected = ", ".join(sorted(expected_shapes.keys()))
available_tensors = ", ".join(sorted(tensors_dict.keys()))
raise KeyError(
f"Unexpected tensor source_name {source_name!r} when looking up expected_shapes. "
f"This usually indicates drift between the checkpoint/manifest and the "
f"hardcoded expected_shapes. "
f"Available expected_shapes keys: {available_expected}. "
f"Available checkpoint tensor keys: {available_tensors}."
)
expected_shape = expected_shapes[source_name]
if source_name in [
```
</issue_to_address>
### Comment 4
<location path="train/train_trinity_coding.py" line_range="139" />
<code_context>
res = subprocess.run([sys.executable, tmp_path], capture_output=True, timeout=10, env=run_env)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try: | ||
| tmp_file.write(full_code) | ||
| tmp_file.close() | ||
| res = subprocess.run([sys.executable, tmp_path], capture_output=True, timeout=10, env=run_env) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Sourcery flagged (on the upstream PR) that scrubbing only vars matching "KEY" from the subprocess environment used to execute untrusted candidate code still leaves everything else (tokens, org IDs, etc.) exposed. Switch to an explicit allowlist (PATH, PYTHONPATH) instead of a denylist that has to be kept in sync with whatever secrets happen to be in the parent env. Also documented why the subprocess call itself isn't a command-injection risk (no shell=True, args passed as a list) — executing candidate code is the actual point of HumanEval-style grading, not something to route around; that part of the finding is a static-analysis pattern match, not a real vulnerability in this shape of call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fix: allowlist subprocess env in HumanEval grading instead of denylisting secrets
Sourcery flagged this on the upstream PR (trotsky1997#3), in code that predates this training work (openfugu/materialized.py, from the materialized-weights loader feature). Two silent-failure paths in load_materialized(): - When a manifest tensor's path_key had no exact or substring match in the checkpoint file, it silently fell back to list(tensors_dict.values())[0] — an arbitrary tensor. If that tensor happened to have the expected shape (plausible in single/few-tensor checkpoint files), this would load the wrong weights into the model with no error at all. - expected_shapes[source_name] indexed the dict directly, raising a bare KeyError with no context if a manifest entry didn't have a registered expected shape. Both now raise a clear, specific error instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…llback fix: fail loudly instead of silently loading wrong tensor in materialized.py
|
Re: the remaining opengrep finding on `train/train_trinity_coding.py:141` (`subprocess.run` "without a static string" / possible command injection): This one's a static-analysis pattern match rather than an actual injection vector in this call shape — there's no `shell=True`, arguments are passed as a list (`[sys.executable, tmp_path]`), so there's no shell to escape into. Running the candidate completion as a real Python process is the intended behavior — that's how HumanEval-style grading works (same pattern as the reference HumanEval eval harness): the model's candidate solution has to actually execute against the test cases to produce a pass/fail reward. The genuine risk here isn't shell injection, it's "this executes untrusted model output" in general — which is inherent to any code-benchmark grader, not something `shlex.escape()` addresses (there's nothing to escape, no shell is invoked). What is mitigated in this PR: the subprocess environment is allowlisted (PATH/PYTHONPATH only, verified live) rather than inheriting the full parent env, so a malicious/hallucinated completion can't read secrets from the environment. Happy to add a sandbox (container/seccomp) if that's wanted for this repo, but wanted to flag that as a separate, bigger scope-of-work decision rather than block this PR on a semgrep pattern that doesn't apply to this call shape. |
Summary
train/train_trinity_coding.py, using HumanEval (auto-graded via real subprocess execution of candidate code) as a more discriminating benchmark.train/train_trinity_real.pyend-to-end against a real NIM worker pool:litellm.RateLimitError(429) was silently scored as a wrong answer, corrupting the reward signal under normal rate limiting.litellm.completion()had no timeout, so a stalled connection could hang the whole run indefinitely.train_trinity_coding.py.Both scripts now retry 429s with exponential backoff and use
timeout=45on the LLM call.Test plan
train_trinity_real.py) against a real NIM worker pool: PASS, coordinator (1.00) matched best single worker.openai_humanevalproblem + canonical solution (including round-trip through the fenced-code extractor) — confirmed valid, executable Python.🤖 Generated with Claude Code
Summary by Sourcery
Introduce a HumanEval-based TRINITY training script for coding tasks, add support for loading materialized coordinator weights in the router, and harden LLM worker calls against rate limits, timeouts, and cache corruption.
New Features:
Bug Fixes:
Enhancements:
Documentation: