fix(samplemath): make environment runnable end-to-end (closes #360) - #363
Merged
Conversation
…culator in env.py number_converter is not defined in tools.py; use the existing percentage_calculator instead so MathEnvironment can be instantiated. Refs #360
…culator in env_subtask.py Mirror the env.py fix: number_converter does not exist in tools.py. Map percentage_calculator into the subtask tool registry instead. Refs #360
The old git+https://github.com/lamalab-org/mat-agent-bench.git URL references the pre-rename repository, so 'uv sync' resolves a different codebase than the surrounding source. Switch to the editable path dependency on '../..' that the other task envs (catalyst, ml) already use. Refs #360
…t API Environment.__init__ now requires base_work_dir; the standalone MathEnvironment was still constructed without one and crashed on startup. Thread base_work_dir through MathEnvironment and read CORRAL_WORK_DIR (with a tempdir fallback) in __main__, matching the subtask environment's contract. Also tighten setuptools package discovery so 'uv sync' does not bail on the flat layout containing both 'samplemath' and 'environments', and refresh uv.lock to drop the stale mat-agent-bench source. Refs #360
The bundled environments/level_1/{tasks_json,subtasks_json}/task_1.json
files use the standardized list-of-entries schema (each entry carrying
its own 'id'), but load_tasks_from_json still iterated as a dict and
crashed with AttributeError. Accept both shapes so the documented
'python samplemath/env_subtask.py environments/...' invocation works.
Refs #360
|
Warning Review limit reached
More reviews will be available in 1 minute and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSwitch corral to a local editable path and add setuptools package discovery; thread an optional base_work_dir through MathEnvironment and main; replace missing number_converter with percentage_calculator across env files; and update task JSON loading to iterate per-task entries and build TaskDefinition objects. ChangesEnvironment setup and tool registration fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Reviewer's GuideMakes the samplemath environment runnable end-to-end by wiring it to the current corral APIs, fixing tool registrations, aligning JSON task loading with the standardized schema, and updating packaging/dependency configuration for monorepo usage. Flow diagram for load_tasks_from_json with standardized entry listflowchart LR
A["load_tasks_from_json(task_json_path, work_dir)"] --> B["json.load"]
B --> C["for entry in task_data"]
C --> D["task_id = entry['id']"]
D --> E["scoring_fn_name = entry.get('scoring_function', 'default')"]
E --> F["get_scoring_function(scoring_fn_name, scoring_params)"]
F --> G["initial_input = entry.get('initial_input', {}).copy()"]
G --> H{"work_dir in initial_input?"}
H -- no --> I["initial_input['work_dir'] = work_dir"]
H -- yes --> J["keep existing work_dir"]
I --> K["TaskDefinition(..., tools=entry.get('tools', []), initial_input=initial_input)"]
J --> K
K --> L["tasks[task_id] = TaskDefinition"]
L --> M["return tasks"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The bundled task JSON files (and the shared loader in src/corral/utils/task_loader.py) all use the list-of-entries schema, so the dict fallback was speculative dead code. Iterate the list directly. Refs #360
n0w0f
marked this pull request as ready for review
June 4, 2026 04:31
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
MathEnvironment.__init__, consider makingbase_work_dira required argument or defaulting toNoneand validating it rather than using an empty string, so that any new call sites that forget to pass it fail fast instead of silently inheriting a potentially invalid work directory. - In
load_tasks_from_json, you now assume a list of entries with required keys likeid,name, anddescription; adding a quick schema/shape check with a clear error message would make failures on malformed task JSONs easier to debug than the currentKeyError/TypeErrorbehavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MathEnvironment.__init__`, consider making `base_work_dir` a required argument or defaulting to `None` and validating it rather than using an empty string, so that any new call sites that forget to pass it fail fast instead of silently inheriting a potentially invalid work directory.
- In `load_tasks_from_json`, you now assume a list of entries with required keys like `id`, `name`, and `description`; adding a quick schema/shape check with a clear error message would make failures on malformed task JSONs easier to debug than the current `KeyError`/`TypeError` behavior.
## Individual Comments
### Comment 1
<location path="tasks/samplemath/samplemath/env.py" line_range="36-37" />
<code_context>
if __name__ == "__main__":
+ base_work_dir = os.environ.get(
+ "CORRAL_WORK_DIR", tempfile.mkdtemp(prefix="samplemath_")
+ )
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `tempfile.mkdtemp` here can leave behind uncleaned temporary directories.
The fallback creates a persistent directory that is never removed, so repeated runs can accumulate many `samplemath_*` dirs. Either use a deterministic path under a known temp root, or add cleanup at process exit (e.g., via `atexit` or by delegating lifecycle management to `Environment`).
Suggested implementation:
```python
if __name__ == "__main__":
base_work_dir = os.environ.get(
"CORRAL_WORK_DIR",
os.path.join(tempfile.gettempdir(), "samplemath_workdir"),
)
```
Ensure the module imports `os` and `tempfile` at the top of the file:
1. Add (or confirm) `import os`.
2. Add (or confirm) `import tempfile`.
No further lifecycle management is required with this change because the fallback is now a single deterministic directory, avoiding accumulation of many `samplemath_*` directories across runs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@tasks/samplemath/samplemath/env_subtask.py`:
- Around line 160-164: The fallback value "default" for scoring_function is
invalid and causes get_scoring_function to fail; update the task loading loop to
use a real default such as "addition_score" (or "multiplication_score") instead
of "default", or validate and raise a clear schema error when scoring_function
is missing; change the line that sets scoring_fn_name in the for-loop (task_data
/ scoring_function) to a valid key from SCORING_FUNCTIONS (addition_score or
multiplication_score) or add an explicit check that raises a descriptive error
before calling get_scoring_function.
- Line 8: Change package-relative imports in env_subtask.py (and corresponding
import in env.py) from "from tools import calculator, percentage_calculator" to
a package-qualified import such as "from samplemath.tools import calculator,
percentage_calculator" so the module works when installed as the samplemath
package. In load_tasks_from_json, stop defaulting scoring_function to "default"
(which isn't in SCORING_FUNCTIONS) — either change the default to a valid key
like "addition_score" or update the code to map unknown/missing scoring_function
values to a safe fallback (e.g., scoring = scoring_function if scoring_function
in SCORING_FUNCTIONS else "addition_score" or use
SCORING_FUNCTIONS.get(scoring_function, SCORING_FUNCTIONS["addition_score"])) to
avoid raising ValueError; reference the SCORING_FUNCTIONS dict and the
load_tasks_from_json function when making this change.
In `@tasks/samplemath/samplemath/env.py`:
- Around line 36-38: The current assignment to base_work_dir calls
tempfile.mkdtemp(...) regardless of CORRAL_WORK_DIR which creates a leftover
temp dir; change it to first read env_var = os.environ.get("CORRAL_WORK_DIR")
and only call tempfile.mkdtemp(prefix="samplemath_") when env_var is falsy, then
assign base_work_dir = env_var or the newly created temp dir (update this logic
inside env.py where base_work_dir is defined).
- Line 4: Replace the top-level import and eager temp-dir creation in env.py:
change the import from "from tools import UnitConverterTool, calculator,
percentage_calculator" to a package-relative import (e.g., "from .tools import
UnitConverterTool, calculator, percentage_calculator") so the module works when
installed as a package, and avoid calling tempfile.mkdtemp() unconditionally for
the CORRAL_WORK_DIR fallback — only call tempfile.mkdtemp() when
os.environ.get("CORRAL_WORK_DIR") is None (e.g., check the env var first and
create the temp dir only in that branch) so the temp directory is not allocated
when CORRAL_WORK_DIR is already set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbc8fe6a-0550-48dc-9ea7-fd716ecb692d
⛔ Files ignored due to path filters (1)
tasks/samplemath/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
tasks/samplemath/pyproject.tomltasks/samplemath/samplemath/env.pytasks/samplemath/samplemath/env_subtask.py
The mkdtemp fallback leaked a new samplemath_* temp dir on every run. Match the convention used by sibling tasks (corral_md, spectra_elucidation) with a deterministic relative path and drop the unused tempfile import.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tasks/samplemath/samplemath/env.py (1)
3-3:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse package-relative import for tools.
The import statement
from tools import ...was flagged in a previous review and remains unaddressed. Whensamplemathis installed as a package, this absolute import will fail becausetoolswon't be directly onsys.path.📦 Proposed fix
-from tools import UnitConverterTool, calculator, percentage_calculator +from .tools import UnitConverterTool, calculator, percentage_calculator🤖 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/samplemath/samplemath/env.py` at line 3, The import in env.py uses a top-level module name "tools" which breaks when samplemath is installed; change it to a package-relative import so Python resolves the module inside the samplemath package (e.g. import the three symbols from the local tools module using a relative import), ensuring UnitConverterTool, calculator and percentage_calculator are imported from the package-local tools module.
🤖 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.
Inline comments:
In `@tasks/samplemath/samplemath/env.py`:
- Around line 35-37: The fallback base_work_dir uses a fragile relative path;
update the assignment of base_work_dir in env.py (the base_work_dir variable) to
use a module-relative path (e.g., based on __file__ /
Path(__file__).resolve().parent) instead of "../CORRAL_WORK_DIR/samplemath", and
after determining the path call mkdir(parents=True, exist_ok=True) to ensure the
directory exists before Environment or trial workspace creation; mirror the
approach used in env_subtask.py for determining and creating the directory.
---
Duplicate comments:
In `@tasks/samplemath/samplemath/env.py`:
- Line 3: The import in env.py uses a top-level module name "tools" which breaks
when samplemath is installed; change it to a package-relative import so Python
resolves the module inside the samplemath package (e.g. import the three symbols
from the local tools module using a relative import), ensuring
UnitConverterTool, calculator and percentage_calculator are imported from the
package-local tools module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 516a7d25-ee64-4a4b-97dc-09be7a4c0e91
📒 Files selected for processing (1)
tasks/samplemath/samplemath/env.py
Comment on lines
+35
to
+37
| base_work_dir = os.environ.get( | ||
| "CORRAL_WORK_DIR", "../CORRAL_WORK_DIR/samplemath" | ||
| ) |
There was a problem hiding this comment.
Hardcoded relative path is fragile and may not exist.
The fallback "../CORRAL_WORK_DIR/samplemath" is relative to the current working directory at runtime, making it unpredictable. Additionally, there's no mkdir() call to ensure the directory exists before Environment tries to create trial workspaces under it (see context snippet 2).
Following the pattern in env_subtask.py, use a module-relative path and ensure it exists.
🛠️ Proposed fix
+from pathlib import Path
+
import os
-from tools import UnitConverterTool, calculator, percentage_calculator
+from .tools import UnitConverterTool, calculator, percentage_calculator if __name__ == "__main__":
- base_work_dir = os.environ.get(
- "CORRAL_WORK_DIR", "../CORRAL_WORK_DIR/samplemath"
- )
+ base_work_dir = os.environ.get("CORRAL_WORK_DIR")
+ if not base_work_dir:
+ base_work_dir = str(Path(__file__).parent / "work_dir")
+ Path(base_work_dir).mkdir(parents=True, exist_ok=True)🤖 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/samplemath/samplemath/env.py` around lines 35 - 37, The fallback
base_work_dir uses a fragile relative path; update the assignment of
base_work_dir in env.py (the base_work_dir variable) to use a module-relative
path (e.g., based on __file__ / Path(__file__).resolve().parent) instead of
"../CORRAL_WORK_DIR/samplemath", and after determining the path call
mkdir(parents=True, exist_ok=True) to ensure the directory exists before
Environment or trial workspace creation; mirror the approach used in
env_subtask.py for determining and creating the directory.
- Import tools via samplemath.tools so the package works when installed, not only when run as a script with its dir on sys.path (matches catalyst). - Make scoring_function required in load_tasks_from_json; the old 'default' fallback pointed at a key absent from SCORING_FUNCTIONS, raising a cryptic error. Now fails with the offending task id and the valid options.
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
2 participants
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
Fixes #360. The
samplemathenvironment failed to start after the documented setup. Six commits address each layer of the problem:number_converterimport inenv.pyandenv_subtask.pywith the already-definedpercentage_calculator.corral@git+https://github.com/lamalab-org/mat-agent-bench.gitdependency frompyproject.tomland use the editable monorepo path (corral = { path = "../..", editable = true }) — same pattern as other environments.CORRAL_WORK_DIR(with a tempdir fallback) in__main__, sinceEnvironment.__init__now requiresbase_work_dir.load_tasks_from_jsonto iterate the standardized list-of-entries schema used by the bundledenvironments/level_1/{tasks,subtasks}_json/task_1.jsonfiles (each entry carries its ownid).[tool.setuptools.packages.find] include = ["samplemath*"]souv syncno longer trips on the flat layout (samplemath/+environments/).Test plan
uv syncresolves without falling back to the renamed remote.MathEnvironment(...)constructs and registerscalculator,percentage_calculator,unit_converter.create_environments('environments/level_1/subtasks_json/task_1.json')buildstask1..task4with their declared tools.grep -r "number_converter\|mat-agent-bench" tasks/samplemathis empty.python samplemath/env.py, port 8011):GET /tasks,/tasks/math_1/prompt,/tasks/math_1/tools,POST /tools/execute(calculator add 23+45 → 68),POST /submitanswer "68" → score 1.0.python samplemath/env_subtask.py environments/level_1/subtasks_json/task_1.json, port 8012):GET /tasksreturnstask1..task4,/tasks/task1/promptshows "First Addition / 3 + 5",POST /tools/execute(calculator add 3+5 → 8),POST /submitanswer "8" → score 1.0.Summary by Sourcery
Make the samplemath environment runnable end-to-end by aligning tooling, task loading, and project configuration with the current corral setup.
Bug Fixes:
Enhancements:
Build:
Summary by CodeRabbit
Configuration
Refactor