Agent benchmark generation - #41
Conversation
📝 WalkthroughWalkthroughSynBench adds schemas, domain loading, task generation and verification, configurable LLM clients, single- and multi-agent execution, scoring, fixtures, retail benchmark data, tests, and executable notebooks. ChangesSynBench platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Generator as TrajectoryGenerator
participant Sampler as ConstraintSampler
participant LLM as LLMClient
participant Verifier as verify_draft
participant Environment as Environment
participant Writer as GenerationRun
Generator->>Sampler: Sample task constraints
Sampler-->>Generator: Return entity and FSM metadata
Generator->>LLM: Request task JSON
LLM-->>Generator: Return generated task
Generator->>Verifier: Verify draft task
Verifier->>Environment: Replay oracle actions
Environment-->>Verifier: Return database state
Verifier-->>Generator: Return verification result
Generator->>Writer: Persist verified tasks
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
aieng-synthetic-data/tests/synbench/integration/test_generation_run.py-12-12 (1)
12-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the registered integration marker.
aieng-synthetic-data/pyproject.tomlregistersintegration_test, but this test usesintegration.pytest -m integration_testwill not select this real-provider test.--strict-markerswill also fail collection.-@pytest.mark.integration +@pytest.mark.integration_test🤖 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 `@aieng-synthetic-data/tests/synbench/integration/test_generation_run.py` at line 12, Replace the integration marker on this test with the registered integration_test marker so pytest -m integration_test selects it and strict marker validation succeeds.aieng-synthetic-data/tests/synbench/integration/test_generation_run.py-17-22 (1)
17-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire at least one verified task.
run_and_verify()can return an emptyverified_taskslist with all drafts rejected. Every current assertion then passes and writes an emptytasks.json. Require at least one verified task so this test validates the stated workflow.Proposed fix
verified_tasks, _ = run.run_and_verify(n=3) + assert verified_tasks, "Expected at least one verified generated task" tasks_path = run.write_tasks(verified_tasks, tmp_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 `@aieng-synthetic-data/tests/synbench/integration/test_generation_run.py` around lines 17 - 22, Add an assertion after run_and_verify() requiring verified_tasks to contain at least one Task, while preserving the existing type and upper-bound checks and task-writing flow.aieng-synthetic-data/tests/synbench/integration/test_call_llm_json.py-21-25 (1)
21-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not add missing model fields in this test.
setdefaultlets a response withoutidortask_typepass validation. Remove these mutations so the test detects violations of the requested JSON shape. If the generation pipeline owns these fields, test that pipeline instead ofcall_llm_jsondirectly.Proposed fix
assert isinstance(data, dict) - data.setdefault("id", "integration_test_draft") - data.setdefault("task_type", constraints.task_type) draft = Task.model_validate(data)🤖 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 `@aieng-synthetic-data/tests/synbench/integration/test_call_llm_json.py` around lines 21 - 25, Remove the data.setdefault calls for "id" and "task_type" before Task.model_validate in the test, leaving the raw call_llm_json response unchanged so missing required fields cause validation to fail.aieng-synthetic-data/tests/synbench/test_pipeline_roles.py-24-26 (1)
24-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck the first role directly.
The current assertion passes if
executoroccurs anywhere insession.role_trace. It does not ensure that the first role is valid.Proposed fix
- assert session.role_trace[0] == "planner" or "executor" in session.role_trace + assert session.role_trace[0] in {"planner", "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 `@aieng-synthetic-data/tests/synbench/test_pipeline_roles.py` around lines 24 - 26, Update the role assertion in the test around session.role_trace so it directly validates the first role, requiring session.role_trace[0] to be either "planner" or "executor" rather than checking for "executor" anywhere in the trace. Preserve the existing user message assertion.implementations/agent_benchmark_generation/README.md-9-13 (2)
9-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the environment-file copy command.
The stated working directory is
implementations/agent_benchmark_generation/. From that directory,cp implementations/agent_benchmark_generation/.env.example .envresolves to a nested path that does not exist. Usecp .env.example .env, or change the stated working directory.🤖 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 `@implementations/agent_benchmark_generation/README.md` around lines 9 - 13, Correct the README setup command so it matches the stated working directory: replace the prefixed source path with the local .env.example path while preserving the destination .env and API-key configuration guidance.
9-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the configured live-LLM credential.
GEMINI_API_KEYis not used by the configured SynBench backend; live requests run through the OpenAI-compatible config and readOPENAI_API_KEYorSYNBENCH_API_KEY. Update the “Mock vs live LLM” note and all repeated comments in the notebooks to mention one of those keys, notGEMINI_API_KEY.🤖 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 `@implementations/agent_benchmark_generation/README.md` around lines 9 - 13, Update the agent benchmark README’s “Mock vs live LLM” guidance and every repeated notebook comment to reference the configured live-LLM credentials OPENAI_API_KEY or SYNBENCH_API_KEY instead of GEMINI_API_KEY, while preserving the existing OpenAI-compatible backend instructions.implementations/agent_benchmark_generation/README.md-24-35 (1)
24-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the component paths to the checked-in package layout.
The table points readers to
src/synbench/.... The supplied implementation is located underaieng-synthetic-data/aieng/syn_data/synbench/.... Update these paths, or label them as import-module paths instead of repository locations.🤖 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 `@implementations/agent_benchmark_generation/README.md` around lines 24 - 35, Update the component table in the README to use the checked-in repository locations under aieng-synthetic-data/aieng/syn_data/synbench for each currently listed src/synbench entry, or explicitly label those entries as import-module paths if retaining the existing notation. Keep the component roles and referenced symbols unchanged.implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb-674-676 (1)
674-676: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRenumber this heading to Step 5.
The notebook goes from "Step 4 — Multi-agent pipeline" at line 309 to "Step 6 — Batch metrics (pass@1)" at line 674. No Step 5 exists, so readers look for a missing section.
🤖 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 `@implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb` around lines 674 - 676, Change the markdown heading in the notebook from “Step 6 — Batch metrics (pass@1)” to “Step 5 — Batch metrics (pass@1)”, leaving the MetricsCollector description unchanged.implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb-216-216 (1)
216-216: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo notebooks document a stale default model name. Both Step 3 markdown cells state that the real client uses "default model:
gemini/gemini-2.0-flash". The stored output of the same cell printsgemini-3.1-flash-lite-preview, and.env.examplesetsSYNBENCH_MODEL=gemini-3.1-flash-lite-preview. Update the text to nameSYNBENCH_MODELas the source of the model id instead of repeating a hardcoded default that can drift again.
implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb#L216: replace the hardcoded default with a reference toSYNBENCH_MODEL.implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb#L242: apply the same correction.🤖 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 `@implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb` at line 216, Update the Step 3 markdown text in implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb at lines 216-216 and implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb at lines 242-242 to identify SYNBENCH_MODEL as the source of the real client’s model ID, removing the hardcoded default model name while leaving the surrounding get_client() documentation unchanged.implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb-13-15 (1)
13-15: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winStored notebook outputs embed the author's local absolute paths. All three notebooks commit execution outputs that contain a personal home directory path, which discloses the author's username and local layout and creates noisy diffs on every re-run. Notebook 4 also proves the outputs are stale: line 29 and line 468 print two different domain paths, so the stored cells come from separate runs against different checkouts. Clear the outputs before commit, or add an
nbstripout-style hook to.pre-commit-config.yaml.
implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb#L13-L15: clear the stored outputs of the setup cell, and also the path in the write confirmation at line 491.implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb#L21-L23: clear the stored outputs of the setup cell, and also the session table path at line 359.implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb#L28-L30: clear the stored outputs of the setup cell, and also the stale mismatched path at line 468.🤖 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 `@implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb` around lines 13 - 15, Remove all committed notebook outputs containing local absolute paths. In implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb lines 13-15 and 491, implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb lines 21-23 and 359, and implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb lines 28-30 and 468, clear the specified cell outputs, including the stale mismatched path in the multi-agent notebook; optionally enforce this with an nbstripout-style pre-commit hook.aieng-synthetic-data/tests/synbench/test_tool_loop.py-10-16 (1)
10-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet
MOCK_LLMwithmonkeypatchto avoid cross-test leakage.Line 12 mutates
os.environwithout teardown. The value stays set for every later test in the same session, so any test that callsget_client()afterwards silently runs in mock mode. The loop here already receives an explicitMockLLMClient, so the global change is not needed.🧪 Proposed fix using the pytest `monkeypatch` fixture
-def test_tool_loop_collects_actions(mock_retail_path): +def test_tool_loop_collects_actions(mock_retail_path, monkeypatch): """The loop records each dispatched tool call in order.""" - os.environ["MOCK_LLM"] = "1" + monkeypatch.setenv("MOCK_LLM", "1") domain = load_domain(mock_retail_path)Then remove the now unused
import osat line 3.🤖 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 `@aieng-synthetic-data/tests/synbench/test_tool_loop.py` around lines 10 - 16, Update test_tool_loop_collects_actions to accept pytest’s monkeypatch fixture and set MOCK_LLM through monkeypatch instead of mutating os.environ directly. Remove the now-unused os import, while preserving the explicit MockLLMClient setup and test behavior.implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb-680-681 (1)
680-681: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the hash slices against
None.
target_db_hashandpredicted_db_hashcan beNone. Notebook 2 already guardsreport.target_db_hashbefore slicing at line 629. If either hash isNonehere, the cell raisesTypeErrorand the run stops before the metrics steps.🐛 Proposed fix for the cell source
-print(f" target_db_hash: {score.target_db_hash[:24]}...") -print(f" predicted_db_hash: {score.predicted_db_hash[:24]}...") +print(f" target_db_hash: {(score.target_db_hash or '')[:24]}...") +print(f" predicted_db_hash: {(score.predicted_db_hash or '')[:24]}...")🤖 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 `@implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb` around lines 680 - 681, Update the score-reporting print statements for score.target_db_hash and score.predicted_db_hash to guard each value before slicing, matching the existing None-safe behavior used for report.target_db_hash in Notebook 2. Preserve the current truncated hash output when a value is present and print a safe fallback when either hash is None.implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb-34-35 (1)
34-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe offline-mode comment contradicts the
MOCK_LLMvalue in all three notebooks. Each setup cell says "Offline mode: no API calls (CI-safe)" and then setsMOCK_LLMto"0", which selects the live client. The comment also namesGEMINI_API_KEY, butaieng/syn_data/synbench/llm/config.pyreadsOPENAI_API_KEYwith aSYNBENCH_API_KEYfallback. A reader who trusts the comment issues paid API calls. Notebook 2 line 286 reinforces the confusion, because it states that fixture responses apply "WithMOCK_LLM=1".
implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb#L34-L35: state that"0"selects live mode and"1"selects fixture mode, and replaceGEMINI_API_KEYwithOPENAI_API_KEY.implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb#L50-L51: apply the same corrected comment.implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb#L55-L56: apply the same corrected comment.📝 Proposed comment text for each setup cell
-# Offline mode: no API calls (CI-safe). Set to "0" + GEMINI_API_KEY for live LLM. +# Live mode: "0" calls the configured endpoint and needs OPENAI_API_KEY. +# Set "1" for offline fixture responses (CI-safe, no network). os.environ["MOCK_LLM"] = "0"🤖 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 `@implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb` around lines 34 - 35, Correct the setup-cell comments describing MOCK_LLM and the API key: in implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb lines 34-35, implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb lines 50-51, and implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb lines 55-56, state that MOCK_LLM="0" selects live mode, MOCK_LLM="1" selects fixture/offline mode, and reference OPENAI_API_KEY instead of GEMINI_API_KEY. Also update the fixture-response note at notebook 2 line 286 to remain consistent with MOCK_LLM="1".aieng-synthetic-data/aieng/syn_data/synbench/verification/pipeline.py-84-84 (1)
84-84: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve an empty supplied fingerprint set.
Line 84 creates a new set when
seen_hashesis empty. The caller-owned set then remains empty after the first batch. A later call that reuses that set does not filter duplicates from the first batch.Proposed fix
- seen = seen_hashes or set() + seen = seen_hashes if seen_hashes is not None else set()🤖 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 `@aieng-synthetic-data/aieng/syn_data/synbench/verification/pipeline.py` at line 84, Update the initialization in the verification pipeline to distinguish an omitted seen_hashes value from an explicitly supplied empty set. Reuse the caller-provided set unchanged, including when it is empty, while creating a new set only when seen_hashes is absent so duplicate filtering persists across calls.
🧹 Nitpick comments (3)
implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb (1)
41-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
ROOTbelow the imports to drop thenoqa: E402.Line 41 assigns
ROOTbefore the import at line 43, which forces the# noqa: E402suppression.ROOTis first used at line 51, so it can move after the imports.♻️ Proposed cell reordering
import json import os from pathlib import Path +from aieng.syn_data.synbench.display import ( + show_actions, + show_pipeline_messages, + show_session, + show_task, +) -ROOT = Path.cwd() - -from aieng.syn_data.synbench.display import ( # noqa: E402 - show_actions, - show_pipeline_messages, - show_session, - show_task, -) - +ROOT = Path.cwd() DOMAIN_PATH = ROOT / "domains" / "mock_retail"🤖 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 `@implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb` around lines 41 - 48, Move the ROOT assignment below the aieng.syn_data.synbench.display imports and remove the unnecessary # noqa: E402 suppression, keeping ROOT defined before its first use.aieng-synthetic-data/tests/synbench/test_tool_loop.py (1)
32-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert on public state instead of
client._sessions.The test reads and writes the private
_sessionsdict. Any rename insideMockLLMClientbreaks this test even when behavior is unchanged. Consider exposing a small public accessor, for exampleturn_index(task_id, role), and assert against it.🤖 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 `@aieng-synthetic-data/tests/synbench/test_tool_loop.py` around lines 32 - 45, Update test_mock_ensure_task_does_not_reset_mid_run to stop reading and mutating the private client._sessions dictionary. Use an existing public state API if available; otherwise add a small public MockLLMClient accessor such as turn_index(task_id, role), then use it to advance and assert the executor turn state while preserving the existing ensure_task and register_task behavior checks.implementations/agent_benchmark_generation/1-check_access_to_model.ipynb (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead the endpoint and model from the environment.
The notebook hardcodes
base_urlandmodel, but.env.examplealready definesSYNBENCH_BASE_URLandSYNBENCH_MODEL, andaieng/syn_data/synbench/llm/config.pyreads both. A user who changes.envstill gets the hardcoded values here, so this access check does not validate the configuration the rest of the pipeline uses.♻️ Proposed change to the cell source
-client = OpenAI(base_url="https://proxy.vectorinstitute.ai/v1", api_key=os.environ.get("OPENAI_API_KEY")) +client = OpenAI( + base_url=os.environ.get("SYNBENCH_BASE_URL", "https://proxy.vectorinstitute.ai/v1"), + api_key=os.environ.get("OPENAI_API_KEY"), +) stream = client.chat.completions.create( - model="gemini-3.1-flash-lite-preview", # see inference.vectorinstitute.ai for available models + # see inference.vectorinstitute.ai for available models + model=os.environ.get("SYNBENCH_MODEL", "gemini-3.1-flash-lite-preview"), messages=[{"role": "user", "content": "Hi, how are you?"}], stream=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 `@implementations/agent_benchmark_generation/1-check_access_to_model.ipynb` around lines 28 - 31, Update the notebook’s OpenAI client and chat completion setup to read the endpoint from SYNBENCH_BASE_URL and the model from SYNBENCH_MODEL, using the same environment-backed configuration names consumed by aieng/syn_data/synbench/llm/config.py. Remove the hardcoded URL and model so the access check validates the user’s configured pipeline values.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/synbench/agents/dialogue.py`:
- Around line 107-122: Extend the snapshot/rollback logic in the dialogue retry
loop to restore all execution state, not only session message and action lists.
Capture and restore session.env plus the retry-local MockLLMClient turn/counter
state before each critic retry, or rebuild both by replaying only the retained
actions, so the retry and final scoring execute from equivalent state.
In `@aieng-synthetic-data/aieng/syn_data/synbench/agents/planner.py`:
- Around line 18-28: Update the message construction in the planner flow so
prior assistant entries from session.messages are appended before the current
user_message. Keep the system prompt first, then add the current user message
last immediately before self.client.complete(messages), ensuring the planner
responds to the latest request.
In `@aieng-synthetic-data/aieng/syn_data/synbench/domain/loader.py`:
- Around line 246-249: Update the validation flow around load_domain so expected
malformed-file failures—including JSON/YAML parsing, seed-task validation, and
tools.py import errors—are converted to DomainLoadError or caught at this
boundary and returned as diagnostic strings. Preserve the existing successful
bundle validation path and the current handling of DomainLoadError in
validate_domain.
In `@aieng-synthetic-data/aieng/syn_data/synbench/environment/communicate.py`:
- Around line 12-18: The communication scoring logic in the verifier should not
treat raw substring matches in combined agent messages as success. Replace the
`missing` check in the required-criteria path with structured expected-outcome
validation or an intent-aware verifier that distinguishes negated or
contradictory statements, while preserving the existing full-credit,
zero-credit, and no-requirements return contract.
In `@aieng-synthetic-data/aieng/syn_data/synbench/evaluation/metrics.py`:
- Around line 26-58: Update the metrics collector’s add() and aggregation
behavior so repeated task_id values cannot inflate benchmark results: either
reject duplicates in add() or retain attempts separately while computing
pass_at_1() and n_tasks from the first attempt for each task. Preserve the
existing per-run metrics output and full-reward threshold for unique tasks.
In `@aieng-synthetic-data/aieng/syn_data/synbench/generation/llm.py`:
- Around line 20-23: Update the fixture root construction in the mock-generation
function containing fixtures so it resolves from the repository root rather than
aieng. Point it to tests/synbench/fixtures/llm_responses, then continue
appending name for the requested fixture file.
In `@aieng-synthetic-data/aieng/syn_data/synbench/generation/prompt.py`:
- Around line 41-45: Update the write guidance construction in the
prompt-generation flow to derive requirements from the FSM `path` tags rather
than only `allow_write` and `n`. Inspect each `lookup` and `mutate` action,
requiring a final write only when the path requires one, while preserving
read-only guidance for paths such as `[mutate]` and mandatory-write guidance for
paths such as `[lookup, lookup]` with writes allowed.
In `@aieng-synthetic-data/aieng/syn_data/synbench/llm/config.py`:
- Around line 26-36: Update get_base_url to remove the hardcoded third-party
fallback and require SYNBENCH_BASE_URL or BASE_URL to be configured. Raise a
clear configuration error when both variables are absent, before ChatClient can
use get_api_key or send credentials.
In `@aieng-synthetic-data/aieng/syn_data/synbench/llm/mock_client.py`:
- Around line 14-18: Fix fixture path resolution in the mock client: update the
FIXTURES, AGENT_TRACES, and LLM_RESPONSES setup near lines 14-18 to resolve
tests/synbench/fixtures from the project root or accept injected paths, and
update the seed-task fallback near lines 92-94 to resolve
implementations/agent_benchmark_generation/domains/mock_retail/tasks.seed.json
from the repository root or remove that repository-local fallback.
In `@aieng-synthetic-data/aieng/syn_data/synbench/schemas/actions.py`:
- Around line 35-38: Update normalize_action to sort dictionary keys while
preserving primitive string and numeric values exactly as provided; remove the
conversions that turn digit-only strings or integral floats into integers.
Ensure normalization does not attempt int conversion on non-finite floats or
otherwise raise for valid primitive values.
In `@aieng-synthetic-data/pyproject.toml`:
- Around line 19-25: Move SynBench’s required runtime libraries from the
synbench dependency group into [project].dependencies so installed package
metadata includes them, while preserving dependencies = [] only if SynBench
remains intentionally opt-in. Ensure the runtime set covers pydantic, openai,
httpx, and the distribution providing the imported dotenv module; retain an
optional group only for explicitly optional SynBench installation.
In `@aieng-synthetic-data/tests/synbench/conftest.py`:
- Around line 8-11: Resolve ROOT relative to conftest.py’s location rather than
the pytest working directory, and update project_root to return the documented
package root. Adjust MOCK_RETAIL and any related fixture paths to use this
file-based root while preserving their existing domain subpaths.
In `@implementations/agent_benchmark_generation/.env.example`:
- Around line 5-7: Remove the committed credential from the environment template
and replace OPENAI_API_KEY with an empty placeholder; revoke and rotate the
exposed key with the proxy provider, then purge the original value from Git
history. Preserve SYNBENCH_BASE_URL and the existing alias comment.
In
`@implementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.json`:
- Around line 36-52: Add the missing get_order action to the evaluation oracle
between find_user_id and cancel_order, using order ID ord_1001, so the actions
verify identity, confirm order details, then cancel the order. Keep the task
instruction unchanged.
In
`@implementations/agent_benchmark_generation/domains/mock_retail/tasks.seed.json`:
- Around line 22-26: Add the expected inquiry status, such as "pending", to
communicate_info for each affected task:
implementations/agent_benchmark_generation/domains/mock_retail/tasks.seed.json:22-26,
implementations/agent_benchmark_generation/data/benchmarks/mock_retail/notebook_walkthrough/tasks.json:22-26
for ord_1001, and
implementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.json:22-26
for ord_1001 and 82-86 for ord_2001. Ensure COMMUNICATE scoring has a required
status response at all four sites.
In `@implementations/agent_benchmark_generation/domains/mock_retail/tools.py`:
- Around line 85-124: Update get_order, cancel_order, and update_shipping to
require a user_id argument and verify it matches the order’s user_id before
returning or mutating the order; reject mismatches using the existing validation
pattern. Propagate the new argument through ToolKit, ToolSpec definitions, task
actions, and agent prompts so every affected tool call supplies the requester
identity.
---
Minor comments:
In `@aieng-synthetic-data/aieng/syn_data/synbench/verification/pipeline.py`:
- Line 84: Update the initialization in the verification pipeline to distinguish
an omitted seen_hashes value from an explicitly supplied empty set. Reuse the
caller-provided set unchanged, including when it is empty, while creating a new
set only when seen_hashes is absent so duplicate filtering persists across
calls.
In `@aieng-synthetic-data/tests/synbench/integration/test_call_llm_json.py`:
- Around line 21-25: Remove the data.setdefault calls for "id" and "task_type"
before Task.model_validate in the test, leaving the raw call_llm_json response
unchanged so missing required fields cause validation to fail.
In `@aieng-synthetic-data/tests/synbench/integration/test_generation_run.py`:
- Line 12: Replace the integration marker on this test with the registered
integration_test marker so pytest -m integration_test selects it and strict
marker validation succeeds.
- Around line 17-22: Add an assertion after run_and_verify() requiring
verified_tasks to contain at least one Task, while preserving the existing type
and upper-bound checks and task-writing flow.
In `@aieng-synthetic-data/tests/synbench/test_pipeline_roles.py`:
- Around line 24-26: Update the role assertion in the test around
session.role_trace so it directly validates the first role, requiring
session.role_trace[0] to be either "planner" or "executor" rather than checking
for "executor" anywhere in the trace. Preserve the existing user message
assertion.
In `@aieng-synthetic-data/tests/synbench/test_tool_loop.py`:
- Around line 10-16: Update test_tool_loop_collects_actions to accept pytest’s
monkeypatch fixture and set MOCK_LLM through monkeypatch instead of mutating
os.environ directly. Remove the now-unused os import, while preserving the
explicit MockLLMClient setup and test behavior.
In
`@implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb`:
- Around line 13-15: Remove all committed notebook outputs containing local
absolute paths. In
implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb
lines 13-15 and 491,
implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb lines
21-23 and 359, and
implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb
lines 28-30 and 468, clear the specified cell outputs, including the stale
mismatched path in the multi-agent notebook; optionally enforce this with an
nbstripout-style pre-commit hook.
- Around line 34-35: Correct the setup-cell comments describing MOCK_LLM and the
API key: in
implementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynb
lines 34-35,
implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb lines
50-51, and
implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb
lines 55-56, state that MOCK_LLM="0" selects live mode, MOCK_LLM="1" selects
fixture/offline mode, and reference OPENAI_API_KEY instead of GEMINI_API_KEY.
Also update the fixture-response note at notebook 2 line 286 to remain
consistent with MOCK_LLM="1".
In `@implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb`:
- Line 216: Update the Step 3 markdown text in
implementations/agent_benchmark_generation/3-single_agent_evaluation.ipynb at
lines 216-216 and
implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb
at lines 242-242 to identify SYNBENCH_MODEL as the source of the real client’s
model ID, removing the hardcoded default model name while leaving the
surrounding get_client() documentation unchanged.
- Around line 680-681: Update the score-reporting print statements for
score.target_db_hash and score.predicted_db_hash to guard each value before
slicing, matching the existing None-safe behavior used for report.target_db_hash
in Notebook 2. Preserve the current truncated hash output when a value is
present and print a safe fallback when either hash is None.
In
`@implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb`:
- Around line 674-676: Change the markdown heading in the notebook from “Step 6
— Batch metrics (pass@1)” to “Step 5 — Batch metrics (pass@1)”, leaving the
MetricsCollector description unchanged.
In `@implementations/agent_benchmark_generation/README.md`:
- Around line 9-13: Correct the README setup command so it matches the stated
working directory: replace the prefixed source path with the local .env.example
path while preserving the destination .env and API-key configuration guidance.
- Around line 9-13: Update the agent benchmark README’s “Mock vs live LLM”
guidance and every repeated notebook comment to reference the configured
live-LLM credentials OPENAI_API_KEY or SYNBENCH_API_KEY instead of
GEMINI_API_KEY, while preserving the existing OpenAI-compatible backend
instructions.
- Around line 24-35: Update the component table in the README to use the
checked-in repository locations under
aieng-synthetic-data/aieng/syn_data/synbench for each currently listed
src/synbench entry, or explicitly label those entries as import-module paths if
retaining the existing notation. Keep the component roles and referenced symbols
unchanged.
---
Nitpick comments:
In `@aieng-synthetic-data/tests/synbench/test_tool_loop.py`:
- Around line 32-45: Update test_mock_ensure_task_does_not_reset_mid_run to stop
reading and mutating the private client._sessions dictionary. Use an existing
public state API if available; otherwise add a small public MockLLMClient
accessor such as turn_index(task_id, role), then use it to advance and assert
the executor turn state while preserving the existing ensure_task and
register_task behavior checks.
In `@implementations/agent_benchmark_generation/1-check_access_to_model.ipynb`:
- Around line 28-31: Update the notebook’s OpenAI client and chat completion
setup to read the endpoint from SYNBENCH_BASE_URL and the model from
SYNBENCH_MODEL, using the same environment-backed configuration names consumed
by aieng/syn_data/synbench/llm/config.py. Remove the hardcoded URL and model so
the access check validates the user’s configured pipeline values.
In
`@implementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynb`:
- Around line 41-48: Move the ROOT assignment below the
aieng.syn_data.synbench.display imports and remove the unnecessary # noqa: E402
suppression, keeping ROOT defined before its first use.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 390a88c7-0675-4f45-8414-4871331cba1e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (99)
.pre-commit-config.yamlaieng-synthetic-data/aieng/syn_data/image/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/critic.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/dialogue.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/llm_agent.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/loop.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/pipeline.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/planner.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/prompts.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/session.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/single.pyaieng-synthetic-data/aieng/syn_data/synbench/agents/user_sim.pyaieng-synthetic-data/aieng/syn_data/synbench/display.pyaieng-synthetic-data/aieng/syn_data/synbench/domain/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/domain/loader.pyaieng-synthetic-data/aieng/syn_data/synbench/environment/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/environment/communicate.pyaieng-synthetic-data/aieng/syn_data/synbench/environment/core.pyaieng-synthetic-data/aieng/syn_data/synbench/environment/hashing.pyaieng-synthetic-data/aieng/syn_data/synbench/evaluation/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/evaluation/metrics.pyaieng-synthetic-data/aieng/syn_data/synbench/evaluation/scoring.pyaieng-synthetic-data/aieng/syn_data/synbench/fsm/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/fsm/validator.pyaieng-synthetic-data/aieng/syn_data/synbench/generation/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/generation/generator.pyaieng-synthetic-data/aieng/syn_data/synbench/generation/llm.pyaieng-synthetic-data/aieng/syn_data/synbench/generation/prompt.pyaieng-synthetic-data/aieng/syn_data/synbench/generation/sampler.pyaieng-synthetic-data/aieng/syn_data/synbench/llm/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/llm/chat_client.pyaieng-synthetic-data/aieng/syn_data/synbench/llm/client.pyaieng-synthetic-data/aieng/syn_data/synbench/llm/config.pyaieng-synthetic-data/aieng/syn_data/synbench/llm/mock_client.pyaieng-synthetic-data/aieng/syn_data/synbench/llm/parsing.pyaieng-synthetic-data/aieng/syn_data/synbench/llm/tools.pyaieng-synthetic-data/aieng/syn_data/synbench/schemas/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/schemas/actions.pyaieng-synthetic-data/aieng/syn_data/synbench/schemas/domain.pyaieng-synthetic-data/aieng/syn_data/synbench/schemas/generation.pyaieng-synthetic-data/aieng/syn_data/synbench/schemas/tasks.pyaieng-synthetic-data/aieng/syn_data/synbench/schemas/tools.pyaieng-synthetic-data/aieng/syn_data/synbench/schemas/verification.pyaieng-synthetic-data/aieng/syn_data/synbench/verification/__init__.pyaieng-synthetic-data/aieng/syn_data/synbench/verification/domain_checks.pyaieng-synthetic-data/aieng/syn_data/synbench/verification/errors.pyaieng-synthetic-data/aieng/syn_data/synbench/verification/pipeline.pyaieng-synthetic-data/pyproject.tomlaieng-synthetic-data/tests/synbench/__init__.pyaieng-synthetic-data/tests/synbench/conftest.pyaieng-synthetic-data/tests/synbench/fixtures/agent_traces/seed_cancel.jsonaieng-synthetic-data/tests/synbench/fixtures/agent_traces/seed_inquiry.jsonaieng-synthetic-data/tests/synbench/fixtures/agent_traces/seed_refuse_cancel.jsonaieng-synthetic-data/tests/synbench/fixtures/llm_responses/draft_cancel.jsonaieng-synthetic-data/tests/synbench/fixtures/tasks_invalid_fsm.jsonaieng-synthetic-data/tests/synbench/fixtures/tasks_invalid_replay.jsonaieng-synthetic-data/tests/synbench/fixtures/tasks_valid.jsonaieng-synthetic-data/tests/synbench/integration/__init__.pyaieng-synthetic-data/tests/synbench/integration/conftest.pyaieng-synthetic-data/tests/synbench/integration/test_call_llm_json.pyaieng-synthetic-data/tests/synbench/integration/test_generation_run.pyaieng-synthetic-data/tests/synbench/test_environment_tool_dispatch.pyaieng-synthetic-data/tests/synbench/test_evaluation_metrics.pyaieng-synthetic-data/tests/synbench/test_fsm_invalid_path.pyaieng-synthetic-data/tests/synbench/test_fsm_valid_path.pyaieng-synthetic-data/tests/synbench/test_generation_config.pyaieng-synthetic-data/tests/synbench/test_generation_dedup.pyaieng-synthetic-data/tests/synbench/test_pipeline_roles.pyaieng-synthetic-data/tests/synbench/test_prompt_builder_snapshot.pyaieng-synthetic-data/tests/synbench/test_schemas_action_compare.pyaieng-synthetic-data/tests/synbench/test_schemas_task_roundtrip.pyaieng-synthetic-data/tests/synbench/test_single_agent_mock.pyaieng-synthetic-data/tests/synbench/test_tool_loop.pyaieng-synthetic-data/tests/synbench/test_user_sim_perspective.pyaieng-synthetic-data/tests/synbench/test_verifier_policy_rules.pyaieng-synthetic-data/tests/synbench/test_verifier_replay_failure.pyaieng-synthetic-data/tests/synbench/test_verifier_replay_success.pyaieng-synthetic-data/tests/test_smoke.pyimplementations/agent_benchmark_generation/.env.exampleimplementations/agent_benchmark_generation/1-check_access_to_model.ipynbimplementations/agent_benchmark_generation/2-generate_and_verify_tasks.ipynbimplementations/agent_benchmark_generation/3-single_agent_evaluation.ipynbimplementations/agent_benchmark_generation/4-multi_agent_pipeline_evaluation.ipynbimplementations/agent_benchmark_generation/README.mdimplementations/agent_benchmark_generation/data/benchmarks/mock_retail/notebook_walkthrough/tasks.jsonimplementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.jsonimplementations/agent_benchmark_generation/domains/mock_retail/db.jsonimplementations/agent_benchmark_generation/domains/mock_retail/generation.yamlimplementations/agent_benchmark_generation/domains/mock_retail/policy.mdimplementations/agent_benchmark_generation/domains/mock_retail/state_machine.yamlimplementations/agent_benchmark_generation/domains/mock_retail/tasks.seed.jsonimplementations/agent_benchmark_generation/domains/mock_retail/tools.pyimplementations/agent_benchmark_generation/domains/mock_retail/user_simulator.yamlimplementations/agent_benchmark_generation/domains/mock_retail/verify.pyimplementations/implementation_b/README.mdimplementations/implementation_b/topic_b_a.ipynbpyproject.toml
💤 Files with no reviewable changes (4)
- implementations/implementation_b/topic_b_a.ipynb
- aieng-synthetic-data/aieng/syn_data/image/init.py
- implementations/implementation_b/README.md
- aieng-synthetic-data/tests/test_smoke.py
| # Snapshot so a critic reject can rewind a failed attempt. | ||
| snap_messages = len(session.messages) | ||
| snap_actions = len(session.agent_actions) | ||
| snap_agent_msgs = len(session.agent_messages) | ||
| critic_notes = "" | ||
| max_attempts = 2 if critic is not None else 1 | ||
|
|
||
| for attempt in range(max_attempts): | ||
| if attempt > 0: | ||
| session.messages = session.messages[:snap_messages] | ||
| session.agent_actions = session.agent_actions[:snap_actions] | ||
| session.agent_messages = session.agent_messages[:snap_agent_msgs] | ||
|
|
||
| extra = plan | ||
| if critic_notes: | ||
| extra = f"{plan}\n\nCritic revision:\n{critic_notes}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Restore execution state before a critic retry.
The rollback restores only transcript and trace lists. Rejected tool calls remain applied to session.env, so the retry runs against modified state. MockLLMClient turn counters also remain advanced. The final agent_actions can then score differently on the fresh scoring environment than the live retry executed.
Snapshot and restore the environment state and retry-local client state, or rebuild them by replaying only the retained actions before each retry.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/synbench/agents/dialogue.py` around lines
107 - 122, Extend the snapshot/rollback logic in the dialogue retry loop to
restore all execution state, not only session message and action lists. Capture
and restore session.env plus the retry-local MockLLMClient turn/counter state
before each critic retry, or rebuild both by replaying only the retained
actions, so the retry and final scoring execute from equivalent state.
| messages = [ | ||
| { | ||
| "role": "system", | ||
| "content": planner_system_prompt(session.domain, session.task), | ||
| }, | ||
| {"role": "user", "content": user_message}, | ||
| ] | ||
| for m in session.messages: | ||
| if m.get("role") == "assistant" and m.get("content"): | ||
| messages.append({"role": "assistant", "content": m["content"]}) | ||
| response = self.client.complete(messages) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Put the current user message after the conversation history.
On later dialogue turns, this code appends prior assistant replies after user_message. The planner then receives an assistant message as the latest message and can continue that reply instead of planning for the current user request. Append the history first, then append user_message.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/synbench/agents/planner.py` around lines
18 - 28, Update the message construction in the planner flow so prior assistant
entries from session.messages are appended before the current user_message. Keep
the system prompt first, then add the current user message last immediately
before self.client.complete(messages), ensuring the planner responds to the
latest request.
| try: | ||
| bundle = load_domain(path) | ||
| except DomainLoadError as e: | ||
| return [str(e)] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return validation errors for malformed domain files.
validate_domain only catches DomainLoadError. Invalid JSON, YAML, seed-task validation, or tools.py import errors can escape from load_domain and terminate synbench domain validate.
Normalize expected load and parse failures to DomainLoadError, or catch them at this boundary and append a diagnostic error.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/synbench/domain/loader.py` around lines
246 - 249, Update the validation flow around load_domain so expected
malformed-file failures—including JSON/YAML parsing, seed-task validation, and
tools.py import errors—are converted to DomainLoadError or caught at this
boundary and returned as diagnostic strings. Preserve the existing successful
bundle validation path and the current handling of DomainLoadError in
validate_domain.
| if not required: | ||
| return 1.0, [] | ||
| combined = " ".join(agent_messages).lower() | ||
| missing = [s for s in required if s.lower() not in combined] | ||
| if missing: | ||
| return 0.0, missing | ||
| return 1.0, [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not score substring presence as successful communication.
A message such as "Your order was not cancelled" satisfies a required string of "cancelled". The trajectory can receive full communication credit for the opposite outcome.
Use a structured expected outcome or an intent-aware verifier for communication criteria. Do not use raw substring presence as the success condition.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/synbench/environment/communicate.py`
around lines 12 - 18, The communication scoring logic in the verifier should not
treat raw substring matches in combined agent messages as success. Replace the
`missing` check in the required-criteria path with structured expected-outcome
validation or an intent-aware verifier that distinguishes negated or
contradictory statements, while preserving the existing full-credit,
zero-credit, and no-requirements return contract.
| runs: list[RunMetrics] = field(default_factory=list) | ||
|
|
||
| def add(self, task_id: str, score: ScoreResult) -> None: | ||
| """Record the score for one task run.""" | ||
| self.runs.append( | ||
| RunMetrics( | ||
| task_id=task_id, | ||
| reward=score.reward, | ||
| db_reward=score.db_reward, | ||
| communicate_reward=score.communicate_reward, | ||
| partial_action_match=score.partial_action_match, | ||
| ) | ||
| ) | ||
|
|
||
| def pass_at_1(self) -> float: | ||
| """Fraction of recorded runs that earned full reward.""" | ||
| if not self.runs: | ||
| return 0.0 | ||
| return sum(1 for r in self.runs if r.reward >= 1.0) / len(self.runs) | ||
|
|
||
| def summary(self) -> dict[str, Any]: | ||
| """Return aggregate metrics plus the individual run records.""" | ||
| return { | ||
| "n_tasks": len(self.runs), | ||
| "pass_at_1": self.pass_at_1(), | ||
| "mean_db_reward": sum(r.db_reward for r in self.runs) | ||
| / max(len(self.runs), 1), | ||
| "mean_communicate_reward": sum(r.communicate_reward for r in self.runs) | ||
| / max(len(self.runs), 1), | ||
| "mean_partial_action_match": sum(r.partial_action_match for r in self.runs) | ||
| / max(len(self.runs), 1), | ||
| "runs": [r.__dict__ for r in self.runs], | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent duplicate task IDs from changing pass@1.
If a caller retries one task, add() records another run with the same task_id. pass_at_1() and n_tasks then count retries as separate tasks. This can inflate a benchmark result.
Reject duplicate task_id values, or store attempts separately and calculate pass@1 from the first attempt for each task.
🤖 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 `@aieng-synthetic-data/aieng/syn_data/synbench/evaluation/metrics.py` around
lines 26 - 58, Update the metrics collector’s add() and aggregation behavior so
repeated task_id values cannot inflate benchmark results: either reject
duplicates in add() or retain attempts separately while computing pass_at_1()
and n_tasks from the first attempt for each task. Preserve the existing per-run
metrics output and full-reward threshold for unique tasks.
| SYNBENCH_BASE_URL=https://proxy.vectorinstitute.ai/v1 | ||
| OPENAI_API_KEY=vp_db93cca8.78d617ab90cdfc8f5ff0700d210ade4043fcf89322f463418e0872a046bb9c43 | ||
| # SYNBENCH_API_KEY= # alias for OPENAI_API_KEY |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Remove the committed API key and rotate it.
Line 6 contains a concrete credential value, not a placeholder, and line 5 points at a live proxy endpoint. Treat this key as compromised.
Required actions:
- Revoke and rotate the key at the proxy provider.
- Replace the value with an empty placeholder in this template.
- Purge the value from git history, because deleting it in a later commit does not remove it from earlier commits.
🔐 Proposed fix for the template
# OpenAI-compatible API (proxy, OpenAI, vLLM, etc.)
SYNBENCH_BASE_URL=https://proxy.vectorinstitute.ai/v1
-OPENAI_API_KEY=vp_db93cca8.78d617ab90cdfc8f5ff0700d210ade4043fcf89322f463418e0872a046bb9c43
+OPENAI_API_KEY=
# SYNBENCH_API_KEY= # alias for OPENAI_API_KEY📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SYNBENCH_BASE_URL=https://proxy.vectorinstitute.ai/v1 | |
| OPENAI_API_KEY=vp_db93cca8.78d617ab90cdfc8f5ff0700d210ade4043fcf89322f463418e0872a046bb9c43 | |
| # SYNBENCH_API_KEY= # alias for OPENAI_API_KEY | |
| SYNBENCH_BASE_URL=https://proxy.vectorinstitute.ai/v1 | |
| OPENAI_API_KEY= | |
| # SYNBENCH_API_KEY= # alias for OPENAI_API_KEY |
🧰 Tools
🪛 Betterleaks (1.7.3)
[high] 6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 dotenv-linter (4.0.0)
[warning] 6-6: [UnorderedKey] The OPENAI_API_KEY key should go before the SYNBENCH_BASE_URL key
(UnorderedKey)
🤖 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 `@implementations/agent_benchmark_generation/.env.example` around lines 5 - 7,
Remove the committed credential from the environment template and replace
OPENAI_API_KEY with an empty placeholder; revoke and rotate the exposed key with
the proxy provider, then purge the original value from Git history. Preserve
SYNBENCH_BASE_URL and the existing alias comment.
Source: Linters/SAST tools
There was a problem hiding this comment.
I was going to say the same! hopefully that is not your real api key?
There was a problem hiding this comment.
@rjavadi, I cannot verify whether the key is active. It has the form of a real credential, so it must be treated as exposed.
Please revoke or rotate it, replace it with OPENAI_API_KEY=, and remove the value from the Git history.
You are interacting with an AI system.
| "instructions": "The user wants to cancel their pending order ord_1001. The user's name is Alice Chen. The agent must verify identity and confirm the order details before proceeding with the cancellation.", | ||
| "initial_message": "Oh no, I've been feeling so worried about my recent purchase! Everything feels like it's going wrong \u2014 is there any way I could please, please cancel my order before it ships? I'm just so nervous that it's already too late!" | ||
| }, | ||
| "evaluation_criteria": { | ||
| "actions": [ | ||
| { | ||
| "name": "find_user_id", | ||
| "arguments": { | ||
| "name": "Alice Chen" | ||
| } | ||
| }, | ||
| { | ||
| "name": "cancel_order", | ||
| "arguments": { | ||
| "order_id": "ord_1001" | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the cancellation oracle match the task instruction.
The instruction requires identity verification and order confirmation. The oracle actions call find_user_id and then cancel_order, but omit get_order. Add the order lookup before cancellation, or remove the order-confirmation requirement from the instruction.
🤖 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
`@implementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.json`
around lines 36 - 52, Add the missing get_order action to the evaluation oracle
between find_user_id and cancel_order, using order ID ord_1001, so the actions
verify identity, confirm order details, then cancel the order. Keep the task
instruction unchanged.
| "communicate_info": [], | ||
| "reward_basis": [ | ||
| "DB", | ||
| "COMMUNICATE" | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require status content for inquiry communication scoring.
Each inquiry task enables COMMUNICATE scoring with an empty communicate_info list. The benchmark therefore has no required status response to evaluate.
implementations/agent_benchmark_generation/domains/mock_retail/tasks.seed.json#L22-L26: add the expected status, such as"pending".implementations/agent_benchmark_generation/data/benchmarks/mock_retail/notebook_walkthrough/tasks.json#L22-L26: add the expected status forord_1001.implementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.json#L22-L26: add the expected status forord_1001.implementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.json#L82-L86: add the expected status forord_2001.
📍 Affects 3 files
implementations/agent_benchmark_generation/domains/mock_retail/tasks.seed.json#L22-L26(this comment)implementations/agent_benchmark_generation/data/benchmarks/mock_retail/notebook_walkthrough/tasks.json#L22-L26implementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.json#L22-L26implementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.json#L82-L86
🤖 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
`@implementations/agent_benchmark_generation/domains/mock_retail/tasks.seed.json`
around lines 22 - 26, Add the expected inquiry status, such as "pending", to
communicate_info for each affected task:
implementations/agent_benchmark_generation/domains/mock_retail/tasks.seed.json:22-26,
implementations/agent_benchmark_generation/data/benchmarks/mock_retail/notebook_walkthrough/tasks.json:22-26
for ord_1001, and
implementations/agent_benchmark_generation/data/benchmarks/mock_retail/tasks.json:22-26
for ord_1001 and 82-86 for ord_2001. Ensure COMMUNICATE scoring has a required
status response at all four sites.
| def find_user_id(self, name: str) -> dict[str, Any]: | ||
| """Look up a customer by full name.""" | ||
| needle = name.strip().lower() | ||
| for user in self.db["users"].values(): | ||
| if str(user.get("name", "")).strip().lower() == needle: | ||
| return { | ||
| "user_id": user["user_id"], | ||
| "name": user["name"], | ||
| "email": user.get("email"), | ||
| } | ||
| raise ValueError(f"User not found for name: {name}") | ||
|
|
||
| def get_order(self, order_id: str) -> dict[str, Any]: | ||
| """Return the order with ``order_id``.""" | ||
| order = self.db["orders"].get(order_id) | ||
| if order is None: | ||
| raise ValueError(f"Order not found: {order_id}") | ||
| return copy.deepcopy(order) | ||
|
|
||
| def list_orders(self, user_id: str) -> list[dict[str, Any]]: | ||
| """Return every order belonging to ``user_id``.""" | ||
| return [copy.deepcopy(o) for o in self.db["orders"].values() if o["user_id"] == user_id] | ||
|
|
||
| def cancel_order(self, order_id: str) -> dict[str, Any]: | ||
| """Cancel ``order_id``, which must still be pending.""" | ||
| order = self.db["orders"].get(order_id) | ||
| if order is None: | ||
| raise ValueError(f"Order not found: {order_id}") | ||
| if order["status"] != "pending": | ||
| raise ValueError(f"Cannot cancel order with status: {order['status']}") | ||
| order["status"] = "cancelled" | ||
| return {"order_id": order_id, "status": "cancelled"} | ||
|
|
||
| def update_shipping(self, order_id: str, shipping_address: str) -> dict[str, Any]: | ||
| """Set a new shipping address on ``order_id``.""" | ||
| order = self.db["orders"].get(order_id) | ||
| if order is None: | ||
| raise ValueError(f"Order not found: {order_id}") | ||
| order["shipping_address"] = shipping_address | ||
| return {"order_id": order_id, "shipping_address": shipping_address} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size and relevant excerpt =="
wc -l implementations/agent_benchmark_generation/domains/mock_retail/tools.py
sed -n '1,180p' implementations/agent_benchmark_generation/domains/mock_retail/tools.py
echo "== tool/action/spec references =="
rg -n "RetailTool|cancel_order|update_shipping|get_order|list_orders|find_user_id|Order" implementations/agent_benchmark_generation.domains.mock_retail -S || true
rg -n "cancel_order|update_shipping|get_order|list_orders|find_user_id|ToolSpec|ToolSpecs" implementations/agent_benchmark_generation -S || true
echo "== changed files/status =="
git diff --stat || true
git status --short || trueRepository: VectorInstitute/synthetic-data-bootcamp
Length of output: 17443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
tool_path = Path("implementations/agent_benchmark_generation/domains/mock_retail/tools.py")
tree = ast.parse(tool_path.read_text())
methods = {fn.name: fn for fn in tree.body[-1].body if isinstance(fn, ast.FunctionDef)}
specs = ast.get_source_segment(tool_path.read_text(), tree.body[1])
def arg_names(fn):
return [arg.arg for a in fn.args.args for arg in ([a] if isinstance(a, ast.arg) else [])]
print("ToolKit methods and arguments:")
for name in ["find_user_id", "get_order", "list_orders", "cancel_order", "update_shipping"]:
fn = methods[name]
print(f"{name}({', '.join(arg_names(fn))})")
print("\nRelevant ToolSpec argument names:")
for name in ["get_order", "cancel_order", "update_shipping"]:
print(name)
start = tree.body[1].body[0].lineno if isinstance(tree.body[1], ast.FunctionDef) else tree.body[1].lineno
PYRepository: VectorInstitute/synthetic-data-bootcamp
Length of output: 439
Require and verify user_id for order access and mutation.
get_order, cancel_order, and update_shipping accept only order_id in both ToolSpec and ToolKit. These methods can read or modify any order because no requester identity is checked against order["user_id"].
Add a verified user_id argument to the tool API and reject access when it does not match the order owner. Update the affected ToolSpec definitions, task actions, and agent prompts.
🤖 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 `@implementations/agent_benchmark_generation/domains/mock_retail/tools.py`
around lines 85 - 124, Update get_order, cancel_order, and update_shipping to
require a user_id argument and verify it matches the order’s user_id before
returning or mutating the order; reject mismatches using the existing validation
pattern. Propagate the new argument through ToolKit, ToolSpec definitions, task
actions, and agent prompts so every affected tool call supplies the requester
identity.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
implementations/agent_benchmark_generation/README.md (1)
123-126: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the API key variable actually consumed by the shared LLM client.
The README instructions and
.env.examplesetOPENAI_API_KEY, but the live-model section says to setGEMINI_API_KEY. The shared LLM client readsOPENAI_API_KEYorSYNBENCH_API_KEY, so live-mode setup can fail after following the initial setup step. Update this section and the matching notebook comments, or document both provider-specific key paths and precedence.🤖 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 `@implementations/agent_benchmark_generation/README.md` around lines 123 - 126, Update the “Mock vs live LLM” section and matching notebook comments to use the API key variables consumed by the shared LLM client, prioritizing OPENAI_API_KEY or SYNBENCH_API_KEY instead of GEMINI_API_KEY alone. Document provider-specific alternatives and their precedence if GEMINI_API_KEY remains supported, keeping the setup instructions consistent with .env.example.
🤖 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 `@implementations/agent_benchmark_generation/README.md`:
- Around line 133-146: Update the “Codebase components (map)” entries to
reference the actual repository-relative
aieng-synthetic-data/aieng/syn_data/synbench/... locations, or consistently use
valid package import paths. Preserve each component’s existing role and ensure
every listed module points readers to an implementation that exists.
- Around line 32-36: Update the README heading hierarchy by changing the
notebook name headings, including 1-check_access_to_model.ipynb and the headings
at the referenced sections, from h3 to h2 so they directly follow the #
Notebooks parent heading and eliminate the MD001 warning.
- Line 97: Update the detailed domain-bundle file list in the README to include
verify.py, matching its required status in the overview list. Keep the
documentation consistent so domain authors know to provide the domain-specific
verification rules.
---
Outside diff comments:
In `@implementations/agent_benchmark_generation/README.md`:
- Around line 123-126: Update the “Mock vs live LLM” section and matching
notebook comments to use the API key variables consumed by the shared LLM
client, prioritizing OPENAI_API_KEY or SYNBENCH_API_KEY instead of
GEMINI_API_KEY alone. Document provider-specific alternatives and their
precedence if GEMINI_API_KEY remains supported, keeping the setup instructions
consistent with .env.example.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28fb498d-844c-4ab6-b122-5e06bfd081f3
📒 Files selected for processing (1)
implementations/agent_benchmark_generation/README.md
| 4. `state_machine.yaml` — `task_types` with `path`, `allow_write` | ||
| 5. `user_simulator.yaml` — personas and goal templates | ||
| 6. `tasks.seed.json` — 2–3 hand-verified seed tasks | ||
| 7. `verify.py` - domain-specific rules to verify the generated tasks with. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the domain-bundle lists consistent.
Line 97 makes verify.py a required domain file, but the detailed file list at Lines 100-107 omits it. Add verify.py to that list, or mark it optional in both places. Otherwise, new domain authors may omit the domain-specific verification rules.
🤖 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 `@implementations/agent_benchmark_generation/README.md` at line 97, Update the
detailed domain-bundle file list in the README to include verify.py, matching
its required status in the overview list. Keep the documentation consistent so
domain authors know to provide the domain-specific verification rules.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
implementations/agent_benchmark_generation/README.md (1)
126-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a credential variable supported by the runtime.
Line 126 documents
GEMINI_API_KEY, butaieng-synthetic-data/aieng/syn_data/synbench/llm/config.pyreads onlyOPENAI_API_KEYorSYNBENCH_API_KEY. Users who follow this instruction leave the API key empty and live calls fail. Use a supported variable or update the runtime configuration and tests.Proposed documentation fix
-- `MOCK_LLM=0` + `GEMINI_API_KEY` +- `MOCK_LLM=0` + `OPENAI_API_KEY` (or `SYNBENCH_API_KEY`)🤖 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 `@implementations/agent_benchmark_generation/README.md` at line 126, Update the README’s MOCK_LLM=0 credential guidance to reference a variable supported by the runtime configuration in config.py, preferably OPENAI_API_KEY or SYNBENCH_API_KEY, and keep the documented live-call behavior unchanged.
🤖 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 `@pyproject.toml`:
- Line 11: Replace the dotenv dependency declaration in pyproject.toml with
python-dotenv, preserving the intended version constraint, and regenerate or
update uv.lock to remove stale dotenv entries and resolve the python-dotenv
package instead.
---
Outside diff comments:
In `@implementations/agent_benchmark_generation/README.md`:
- Line 126: Update the README’s MOCK_LLM=0 credential guidance to reference a
variable supported by the runtime configuration in config.py, preferably
OPENAI_API_KEY or SYNBENCH_API_KEY, and keep the documented live-call behavior
unchanged.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e7242bc-bcee-4fb4-b5a0-622ce4c27821
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
aieng-synthetic-data/tests/synbench/conftest.pyimplementations/agent_benchmark_generation/README.mdpyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- aieng-synthetic-data/tests/synbench/conftest.py
| dependencies = [ | ||
| "aieng-synthetic-data", | ||
| "bleach==6.4.0", | ||
| "dotenv>=0.9.9", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n '"(dotenv|python-dotenv)' pyproject.toml
python - <<'PY'
import json
import urllib.request
for distribution in ("dotenv", "python-dotenv"):
with urllib.request.urlopen(
f"https://pypi.org/pypi/{distribution}/json"
) as response:
info = json.load(response)["info"]
print(distribution, info["version"], info.get("summary", ""))
PYRepository: VectorInstitute/synthetic-data-bootcamp
Length of output: 2830
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'pyproject references:\n'
rg -n 'dotenv|python-dotenv|requires-python|python\s*=' pyproject.toml || true
printf '\nLock / env files:\n'
git ls-files | rg '(^|/)(poetry.lock|uv.lock|requirements.*\.txt|Pipfile.lock|pyproject\.toml)$' || true
printf '\nDependency files snippets:\n'
for f in $(git ls-files | rg '(^|/)(poetry.lock|uv.lock|requirements.*\.txt|Pipfile.lock|pyproject\.toml)$' || true); do
printf '\n--- %s ---\n' "$f"
rg -n 'dotenv|python-dotenv|requires-python' "$f" || true
done
printf '\nImports/usages of dotenv:\n'
rg -n 'from dotenv import|import dotenv|python-dotenv|dotenv' --glob '!*.lock' --glob '!poetry.lock' --glob '!uv.lock' . || trueRepository: VectorInstitute/synthetic-data-bootcamp
Length of output: 2092
🌐 Web query:
PyPI dotenv python-dotenv packages python-dotenv from dotenv import
💡 Result:
The correct package to install for handling.env files in Python is python-dotenv [1][2]. It is common in the Python ecosystem for the PyPI distribution name to differ from the name used to import the library in code [3]. To use the library, you install the distribution package via pip: pip install python-dotenv Once installed, you import the library using the name dotenv [1][2]. A common usage pattern involves loading variables from a.env file into the system environment using the load_dotenv function [1][4]: from dotenv import load_dotenv # Load variables from.env file into os.environ load_dotenv # Access environment variables as usual import os value = os.getenv("MY_VARIABLE") Alternatively, if you prefer to load the values into a dictionary instead of the system environment, you can use dotenv_values [1][4]: from dotenv import dotenv_values config = dotenv_values(".env") print(config["MY_VARIABLE"]) It is important to ensure you install the distribution package named python-dotenv, as other packages with similar names exist on PyPI and may not provide the same functionality [2]. If you encounter a ModuleNotFoundError: No module named 'dotenv' after installing, ensure that the package was installed in the same Python environment (e.g., virtual environment or conda environment) where your script is being executed [2].
Citations:
- 1: https://pypi.org/project/python-dotenv/
- 2: https://stackoverflow.com/questions/78779210/how-to-avoid-no-module-named-dotenv-error-after-installing-python-dotenv-in
- 3: https://discuss.python.org/t/installing-dotenv-vs-load-dotenv-why/51024/1
- 4: https://bbc2.github.io/python-dotenv/
Use python-dotenv instead of dotenv.
dotenv==0.9.9 is a separate package; the docs and from dotenv import load_dotenv use are from python-dotenv. Update pyproject.toml and remove/update the stale uv.lock refs so dependency resolution installs the maintained package.
Proposed dependency fix
- "dotenv>=0.9.9",
+ "python-dotenv>=1.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "dotenv>=0.9.9", | |
| "python-dotenv>=1.0", |
🤖 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 `@pyproject.toml` at line 11, Replace the dotenv dependency declaration in
pyproject.toml with python-dotenv, preserving the intended version constraint,
and regenerate or update uv.lock to remove stale dotenv entries and resolve the
python-dotenv package instead.
Source: MCP tools
There was a problem hiding this comment.
In cell 1 I see domain dir is: /Users/royajavadi/projects/synthetic-data-bootcamp/domains/mock_retail
But in cell 2 I get validation error becasue it expects it to be in root.
2 participants
Summary
Clickup Ticket(s): Link(s) if applicable.
https://app.clickup.com/t/10524786/868kerrr8
https://app.clickup.com/t/10524786/868kerrd0
Type of Change
Changes Made
Testing
uv run pytest tests/)uv run mypy <src_dir>)uv run ruff check src_dir/)Manual testing is performed by running notebooks and extensively inspecting the steps.
Manual testing details:
Screenshots/Recordings
Related Issues
Deployment Notes
Checklist
Summary by CodeRabbit
New Features
Documentation