feat: Terminal Bench Workflow Agent + one shot ferment workflow - #940
feat: Terminal Bench Workflow Agent + one shot ferment workflow#940pmateusz wants to merge 1 commit into
Conversation
|
📚 No documentation changes were needed for this PR. |
Kimchi Code Review
Summary📊 Review Score: 80/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — The PR adds substantial test coverage: 🔒 Security concerns found: 📝 Found 4 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
3a33be2 to
ba76439
Compare
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 80/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — The PR adds substantial test coverage: agent_test.py guards the new Kimchi extension/seam hooks, workflow_agent_test.py covers WorkflowAgent construction, launch command, install/upload, stdin handling, and identity; workflow_extension_test.py covers npm/dir resolution, caching, and provenance; and conformance_test.py parameterises the contract across Kimchi and WorkflowAgent.
🔒 Security concerns found: workflow_extension.py runs npm install --omit=dev --omit=peer on an arbitrary npm: extension spec without --ignore-scripts, allowing a malicious or compromised package to execute arbitrary postinstall code on the benchmark host during resolution.
📝 Found 4 issue(s). See inline comments for details.
Address four review comments on PR #940: 1. Security — --ignore-scripts on npm install The host-side `npm install --omit=dev --omit=peer` ran without `--ignore-scripts`, allowing a compromised extension's lifecycle scripts to execute on the benchmark host (which holds KIMCHI_API_KEY). Added the flag and extracted the command into a variable so the error message cannot drift from the invocation. New test reaches past the injectable seam to verify the real implementation includes it — every other test fakes the seam, so without this the flag could silently disappear. 2. Concurrency — safe cache publish without in-place rmtree Replaced `shutil.rmtree(cache_dir)` + `staging.rename(cache_dir)` with `_publish_npm_cache_entry`: try rename first, reuse a complete entry if another process won the race, quarantine a stale leftover and rename-aside rather than deleting where a concurrent reader may be uploading. Two new tests cover the mid-install race and the stale-leftover replacement. 3. Bug — workflow guard scanned source tree, not staged upload The `glob("*.workflow.ts")` guard checked the source directory, which contains `node_modules/@kimchi-dev/kimchi-workflows/.../ create.workflow.ts` that the upload filter strips — a false positive. Replaced with `_assert_a_workflow_can_resolve` which scans the staged copy (what actually reaches the container) and distinguishes name-based vs path-based workflow references: a name only resolves at top level (discoverWorkflows is non-recursive), a .ts path reaches nested files. Three tests pin each case. 4. Maintainability — document intentional unfiltered dir: upload The `dir:` extension upload is unfiltered by design (the extension needs its node_modules/jiti). Added a comment explaining the trade-off and updated workflows/README.md to document that the workflows directory is filtered via WORKFLOWS_UPLOAD_IGNORE and that dev scaffolding never reaches the container.
96d4552 to
7683811
Compare
Runs a named kimchi-workflows workflow instead of kimchi's default chat loop, so workflow implementations can be benchmarked against stock kimchi under identical conditions. Extension specs resolve on the host and upload to the container, since task images have no Node toolchain. Kimchi gains three composition hooks; conformance_test.py pins both agents to the same result.json shape. Also updates retired default model ids.
7683811 to
9b221ca
Compare
1 participant
Linked issue
Closes #0
What does this PR do?
Adds a WorkflowAgent to the terminal-bench-2 harness that runs a named workflow via the kimchi-workflows extension instead of kimchi's default chat loop. This allows benchmarking different workflow implementations against stock kimchi under identical conditions.
Ferment-oneshot workflow (workflows/ferment-oneshot.workflow.ts)
Checklist
pnpm run test)pnpm run check)