fix(eval): add environment/ fallback to get_dockerfile_hash - #264
Open
baobaolaodie wants to merge 2 commits into
Open
fix(eval): add environment/ fallback to get_dockerfile_hash#264baobaolaodie wants to merge 2 commits into
baobaolaodie wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe evaluation Dockerfile hash lookup now falls back to ChangesEvaluation Dockerfile fallback
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Reviewer's GuideAligns Docker image hash calculation with existing build-time Dockerfile fallback while adding a regression test and bumping release metadata to version 0.4.0-beta.13. Sequence diagram for Dockerfile hash fallback in eval scaffoldsequenceDiagram
actor EvalRunner
participant get_image_name
participant get_dockerfile_hash
participant docker_build
EvalRunner->>get_image_name: get_image_name(dir)
get_image_name->>get_dockerfile_hash: get_dockerfile_hash(dir)
alt root Dockerfile exists
get_dockerfile_hash->>get_image_name: hash(Dockerfile)
else root Dockerfile missing
get_dockerfile_hash->>get_dockerfile_hash: [check dir/environment/Dockerfile]
alt environment Dockerfile exists
get_dockerfile_hash->>get_image_name: hash(environment/Dockerfile)
else environment Dockerfile missing
get_dockerfile_hash->>get_image_name: "" (error)
end
end
get_image_name->>docker_build: docker_build(dir)
docker_build->>docker_build: [uses same Dockerfile fallback]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
baobaolaodie
force-pushed
the
fix/eval-dockerfile-hash-fallback
branch
from
August 1, 2026 16:40
68e15e5 to
2e27f1e
Compare
baobaolaodie
marked this pull request as ready for review
August 1, 2026 16:45
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The Dockerfile path resolution logic is now duplicated between
get_dockerfile_hash()anddocker_build(); consider extracting a small helper to centralize the fallback behavior and keep these functions in sync more easily.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Dockerfile path resolution logic is now duplicated between `get_dockerfile_hash()` and `docker_build()`; consider extracting a small helper to centralize the fallback behavior and keep these functions in sync more easily.
## Individual Comments
### Comment 1
<location path="eval/local/tests/scaffold/test_utils.py" line_range="221-230" />
<code_context>
+def test_get_image_name_falls_back_to_environment_dockerfile(tmp_path: Path):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a complementary test for the case where both Dockerfile and environment/Dockerfile exist to assert priority
The current test covers the fallback when only `environment/Dockerfile` exists. To fully specify the behavior and protect against regressions, please add a test where both `dir/Dockerfile` and `dir/environment/Dockerfile` exist and assert that `get_image_name()` selects the top-level `Dockerfile`, not the environment fallback.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
Thanks for the review. On the suggestion to extract a shared Dockerfile-resolution helper: this fix intentionally mirrors the existing fallback in |
baobaolaodie
force-pushed
the
fix/eval-dockerfile-hash-fallback
branch
from
August 2, 2026 17:51
c0a4ee7 to
d9de717
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@eval/local/tests/scaffold/test_utils.py`:
- Around line 250-261: Strengthen
test_get_image_name_falls_back_to_environment_dockerfile by creating a second
workspace with the same environment/Dockerfile content and asserting the
complete _get_image_name results are equal. Keep the existing
environment/Dockerfile-only setup so the test verifies that this fallback file
participates in image hash computation rather than merely checking the
image=skillbench: prefix.
🪄 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 Plus
Run ID: d80874cd-333b-42d3-a21c-c5ce7c64be9f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.mdassets/manifest.jsoneval/local/tests/scaffold/test_utils.pyeval/scaffold/shell/docker.shpackage.jsontest/app/cli-help.test.tstest/repository/release-metadata.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- eval/scaffold/shell/docker.sh
- assets/manifest.json
- CHANGELOG.md
- test/repository/release-metadata.test.ts
- package.json
Comment on lines
+250
to
+261
| def test_get_image_name_falls_back_to_environment_dockerfile(tmp_path: Path): | ||
| """get_image_name() resolves environment/Dockerfile when dir/Dockerfile is absent. | ||
|
|
||
| docker_build() falls back to environment/Dockerfile, so get_image_name() must | ||
| apply the same rule; otherwise the fallback in docker_build() is dead code and | ||
| building a workspace that only carries an environment/Dockerfile always fails. | ||
| """ | ||
| env_dir = tmp_path / "environment" | ||
| env_dir.mkdir() | ||
| (env_dir / "Dockerfile").write_text("FROM python:3.11-slim\n", encoding="utf-8") | ||
|
|
||
| assert "image=skillbench:" in _get_image_name(tmp_path) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
断言必须验证环境 Dockerfile 已参与镜像哈希计算。
Line 261 只验证 image=skillbench: 前缀。未修复的实现如果返回空哈希，例如 image=skillbench:，此测试仍会通过。创建一个具有相同 Dockerfile 内容的根目录工作区，并断言两个完整镜像名相等。
建议修改
env_dir.mkdir()
(env_dir / "Dockerfile").write_text("FROM python:3.11-slim\n", encoding="utf-8")
- assert "image=skillbench:" in _get_image_name(tmp_path)
+ root_dir = tmp_path / "root"
+ root_dir.mkdir()
+ (root_dir / "Dockerfile").write_text("FROM python:3.11-slim\n", encoding="utf-8")
+
+ assert _get_image_name(tmp_path) == _get_image_name(root_dir)🤖 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 `@eval/local/tests/scaffold/test_utils.py` around lines 250 - 261, Strengthen
test_get_image_name_falls_back_to_environment_dockerfile by creating a second
workspace with the same environment/Dockerfile content and asserting the
complete _get_image_name results are equal. Keep the existing
environment/Dockerfile-only setup so the test verifies that this fallback file
participates in image hash computation rather than merely checking the
image=skillbench: prefix.
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
1 participant
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
get_dockerfile_hash()ineval/scaffold/shell/docker.shonly checked fordir/Dockerfile, even thoughdocker_build()already falls back todir/environment/Dockerfile(the eval layout where task environments live underenvironment/). Becauseget_image_name()callsget_dockerfile_hash()before the build, that fallback was dead code: passing a workspace that only carriesenvironment/Dockerfile(e.g. comet-any direct calls or validator scenarios) made image lookup fail before any Docker command ran.This PR applies the same fallback to
get_dockerfile_hash(), keeping the two functions consistent, and bumps the version to0.4.0-beta.15.🎯 Scope
init,status,doctor,update)assets/skills/,assets/skills-zh/)assets/skills/comet/scripts/)eval/scaffold/shell/docker.sh), release metadata🧪 Testing
pnpm install --frozen-lockfilepnpm buildpnpm lint(ESLint + architecture linter)pnpm format:check— every file changed by this PR passes; the repo-wide check only flags the knowndomains/dashboard/web/Windows CRLF boundary files, which are byte-identical toorigin/master(flagged identically onmaster, passes on CI's Linux checkout)pnpm test(full) — result identical tomaster: 8 files / 39 tests fail on Windows (pre-existing openspec/path-integration environment failures), 2991 pass; this PR introduces zero regressionsuv run pytest -q local/tests/scaffold local/tests/tasks/test_validation_scripts.py— 341 passed (one more thanmaster, the new regression test); the same 5 pre-existing failures asmasteruv run pytest local/tests/scaffold/test_utils.py::test_get_image_name_falls_back_to_environment_dockerfile(new regression test; red before the fix, green after)uv run pytest local/tests/scaffold/test_utils.py::test_get_image_name_prefers_root_dockerfile_over_environment(complementary priority test, added after review)uv run --extra dev ruff check local/tests/scaffold/test_utils.py✅ Checklist
CHANGELOG.mdCHANGELOG.mdis updated when behavior changesassets/manifest.jsonand relevant tests👀 Notes for Reviewers
The fix mirrors the existing fallback in
docker_build()(lines 222-226). This PR also bumps the version to0.4.0-beta.15per the one-version-ahead rule (master is0.4.0-beta.14), syncingpackage-lock.json,assets/manifest.json, and the version assertions intest/repository/release-metadata.test.tsandtest/app/cli-help.test.ts. The branch was rebased onto the latestmasterafter the0.4.0-beta.13/0.4.0-beta.14releases, resolving the version-field conflicts.The READMEs are intentionally unchanged: this is an internal eval-scaffold fix whose user-visible effect is recorded in the changelog, and the repo keeps README updates restrained. Known boundary:
get_dockerfile_hash()applies theenvironment/fallback toDockerfileonly, not torequirements.txt; no current eval task keepsrequirements.txtunderenvironment/, so this does not affect existing workloads.Summary by CodeRabbit
Bug Fixes
Tests
Release
0.4.0-beta.15.