feat(verifier): implement local directory support#37
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds local filesystem verification as an alternative to S3-backed verification. ChangesLocal Filesystem Verifier Mode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
internal/orchestrator/verifier_render_test.go (1)
120-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
data-initstartup gate in this render test.The local verifier path depends on
data-initcompleting before the subject starts, but this test would still pass if that service or dependency were removed. Assert both the service anddepends_on.data-init.condition.Suggested test addition
subject := services["subject"].(map[string]any) if !hasSharedDataMount(subject) { t.Errorf("subject missing shared-data:/data mount for local verifier:\n%s", out) } + + if _, ok := services["data-init"].(map[string]any); !ok { + t.Fatalf("data-init service not rendered for local verifier:\n%s", out) + } + deps, _ := subject["depends_on"].(map[string]any) + dataInitDep, _ := deps["data-init"].(map[string]any) + if dataInitDep["condition"] != "service_completed_successfully" { + t.Errorf("subject missing data-init completion dependency:\n%s", out) + } }🤖 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 `@internal/orchestrator/verifier_render_test.go` around lines 120 - 145, The local verifier render test only checks mounts and env vars, so it can miss regressions where the startup gate is removed. Update TestComposeRendersLocalVerifier in verifier_render_test.go to also assert that the rendered services include data-init and that the subject service’s depends_on entry for data-init uses the expected condition, alongside the existing shared-data mount checks.
🤖 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 `@containers/verifier/main.go`:
- Around line 181-195: The local-mode glob built in the path selection logic for
the verifier misses files directly under cfg.LocalDir because the current
recursive pattern only matches subdirectories. Update the glob construction in
the verifier’s reader helper to include both top-level and recursive matches for
local directories (for example by combining patterns before sqlQuote is
applied), while keeping the S3 branch unchanged and preserving the existing
read_parquet/read_avro selection.
In `@internal/config/case.go`:
- Around line 1904-1916: The verifier validation in tc.Verifier needs to reject
local_dir values outside the shared /data mount, since the current
mutual-exclusion checks in the case validation path only verify presence and
exclusivity. Update the validation logic around tc.Verifier.IsLocal() to require
a cleaned container path rooted under /data for local_dir, and return a clear
error from the same case-validation flow when the path is outside that mount.
In `@internal/orchestrator/docker.go`:
- Around line 179-184: The clustered subject service block in docker generation
is missing the same local /data wiring that the singular subject branch uses.
Update the service template logic around the subject rendering path so clustered
subjects either also depend on data-init and mount shared-data:/data, or are
rejected during validation when Verifier.LocalDir is enabled. Use the existing
docker template branches for .Subjects, subject, data-init, and VerifierLocalDir
to locate and mirror the current singular behavior.
- Line 596: The data-init command in docker.go is directly templating
VerifierLocalDir into a root shell command, which allows shell metacharacters to
alter execution. Update the Docker command construction used by the relevant
init/container setup to pass VerifierLocalDir via an environment variable
instead of interpolating it into /bin/sh -c, and ensure the shell reference is
quoted/safely expanded when used in the command string.
---
Nitpick comments:
In `@internal/orchestrator/verifier_render_test.go`:
- Around line 120-145: The local verifier render test only checks mounts and env
vars, so it can miss regressions where the startup gate is removed. Update
TestComposeRendersLocalVerifier in verifier_render_test.go to also assert that
the rendered services include data-init and that the subject service’s
depends_on entry for data-init uses the expected condition, alongside the
existing shared-data mount checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80f0eccc-cd89-42b2-9bc9-f6582747270d
📒 Files selected for processing (6)
containers/verifier/main.gocontainers/verifier/main_test.gointernal/config/case.gointernal/config/verifier_test.gointernal/orchestrator/docker.gointernal/orchestrator/verifier_render_test.go
Deploying pipebench with
|
| Latest commit: |
f3ce343
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9bc10105.pipebench.pages.dev |
| Branch Preview URL: | https://dt-819.pipebench.pages.dev |
yusufozturk
approved these changes
Jun 30, 2026
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 by CodeRabbit
New Features
Bug Fixes
Tests