Add Docker containerization and skillbench enhancements#9
Merged
Conversation
…gement - Introduced a Dockerfile to build a lightweight image for running skillbench. - Added a Makefile with targets for building the Docker image and collecting data. - Implemented a script to dynamically generate Docker bind-mount arguments for workspace folders. - Updated README.md with Docker quick start instructions for ease of use.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Lite Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds Docker-based workflows: a new Dockerfile and Make targets, a helper script to emit workspace bind-mounts, README instructions, and updates to skillbench.py for platform-aware DB discovery, git-config fallback, and a new Changes
Sequence DiagramsequenceDiagram
actor User
participant Make as "Make target"
participant MountScript as "skillbench_docker_mounts.py"
participant Parser as "session_parser"
participant Docker as "Docker daemon / Container"
participant CLI as "skillbench.py (collect)"
User->>Make: run `make docker-collect`
Make->>MountScript: python3 scripts/skillbench_docker_mounts.py
MountScript->>Parser: call scan_sessions(verbose=False)
Parser-->>MountScript: workspace summaries
MountScript->>Make: emit `-v host:host:ro` mount args
Make->>Docker: docker run with mounts, env, image
Docker->>CLI: start `python3 skillbench.py collect --user [--include-excluded?]`
CLI->>CLI: discover CASS DB, parse git config, scan repos
CLI-->>Docker: produce collection output
Docker-->>User: write outputs to host `dist/`
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Comment |
coderabbitai
bot
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d44c44f15e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1409
to
+1411
| if not allowed_paths and include_excluded: | ||
| # Explicit opt-in: proceed with excluded workspaces too. | ||
| allowed_paths = [e["path"] for e in (included + excluded)] |
There was a problem hiding this comment.
The --include-excluded flag currently takes effect only when included is empty, because excluded paths are appended inside if not allowed_paths and include_excluded. In mixed cases (some public/OSS repos plus some excluded repos), an explicit opt-in still exports only the included set, so workflows like make docker-collect-all silently omit the excluded workspaces users asked to include.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
Makefile (2)
19-25: AGENT_MOUNTS may fail if directories don't exist on the host.The mounts for
.codex,.codex-cli, and.openai-codexwill cause Docker errors if these directories don't exist on the host system. Most users won't have all three Codex variants.Consider either:
- Creating missing directories before mounting, or
- Making mounts conditional, or
- Using a script to filter existing directories
♻️ Option 1: Create directories in docker-collect
docker-collect: docker-build `@mkdir` -p dist + `@mkdir` -p "$(HOME)/.claude" "$(HOME)/.gemini" "$(HOME)/.codex" "$(HOME)/.codex-cli" "$(HOME)/.openai-codex" 2>/dev/null || true docker run --rm \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 19 - 25, AGENT_MOUNTS currently includes fixed host paths that will cause Docker to fail if those host directories (e.g., $(HOME)/.codex, $(HOME)/.codex-cli, $(HOME)/.openai-codex) don't exist; change the Makefile to construct AGENT_MOUNTS dynamically by testing each host directory's existence and only appending the corresponding mount (using the existing CONTAINER_HOME variable for the container side) or create missing directories beforehand in a helper target (e.g., a docker-collect or prepare-mounts target) so Docker sees only valid mounts; modify the AGENT_MOUNTS definition to use a shell snippet or phony target that filters/creates the three codex-related directories before running the docker command.
57-70:docker-collect-verbosedoesn't supportINCLUDE_EXCLUDEDflag.Unlike
docker-collect, the verbose target doesn't include$(COLLECT_FLAGS), soINCLUDE_EXCLUDED=1 make docker-collect-verbosewon't work as expected.♻️ Pass COLLECT_FLAGS to verbose target (without -y for interactivity)
+# Verbose flags: same as COLLECT_FLAGS but without -y for interactive mode +COLLECT_FLAGS_VERBOSE := +ifeq ($(INCLUDE_EXCLUDED),1) +COLLECT_FLAGS_VERBOSE += --include-excluded +endif + docker-collect-verbose: docker-build `@mkdir` -p dist docker run --rm $(DOCKER_INTERACTIVE) \ ... "$(IMAGE)" \ - python3 skillbench.py collect + python3 skillbench.py collect $(COLLECT_FLAGS_VERBOSE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 57 - 70, The verbose Makefile target docker-collect-verbose currently omits $(COLLECT_FLAGS) so INCLUDE_EXCLUDED (and other flags) are ignored; update the command that runs python3 skillbench.py collect in docker-collect-verbose to append the collection flags using $(filter-out -y,$(COLLECT_FLAGS)) so interactivity is preserved while still passing INCLUDE_EXCLUDED and other flags (mirror how docker-collect uses $(COLLECT_FLAGS) but remove the -y flag for interactivity).scripts/skillbench_docker_mounts.py (1)
45-47: Minor: Substring check may have false positives.The check
if gemini_tmp in wsuses substring matching, which could incorrectly skip paths like/path/to/.gemini/tmp_backupor/home/user/.gemini/tmp2/project. Consider using path prefix or exact component matching.♻️ Optional: Use path-based check
- if gemini_tmp in ws: + # Skip paths under ~/.gemini/tmp/ (Gemini hash folders) + if ws.startswith(gemini_tmp + "/") or ws == gemini_tmp: continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/skillbench_docker_mounts.py` around lines 45 - 47, The substring check `if gemini_tmp in ws` can produce false positives; replace it with a path-aware check using pathlib so you only skip mounts that are actually under the gemini_tmp directory (e.g., resolve both paths and use Path.is_relative_to or compare the leading path parts) when evaluating `ws` vs `gemini_tmp` in the mount loop in scripts/skillbench_docker_mounts.py.Dockerfile (2)
38-47: Consider adding tarfile extraction filter for Python 3.12+ compatibility.Python 3.12+ deprecates extraction without an explicit
filterparameter due to security concerns with path traversal. While the risk is low here (trusted source, single file extraction), adding the filter future-proofs the build.♻️ Optional: Add extraction filter
tf = tarfile.open(fileobj=io.BytesIO(blob), mode="r:gz") +# Python 3.12+ requires explicit filter; 'data' is the safe default +if hasattr(tarfile, 'data_filter'): + tf.extraction_filter = tarfile.data_filter gh_member = next((m for m in tf.getmembers() if m.name.endswith("/bin/gh")), None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 38 - 47, The tar extraction should use an explicit extraction filter to satisfy Python 3.12+ requirements; replace the current tf.extractfile(gh_member) usage by calling tf.extract(...) (or tf.extractall with a filter) and provide a filter callable that validates/whitelists the member (e.g., accepts only the gh_member where gh_member.name.endswith("/bin/gh")) before extraction so you explicitly allow that path; update the code paths that reference tf.getmembers and gh_member and ensure you extract to out_path (/usr/local/bin/gh) with correct permissions after the filtered extraction.
35-48: Consider adding error handling for network failures.The
urllib.request.urlopencalls lack explicit error handling. If the GitHub API or release download fails (network issues, rate limits, 404), the build will fail with an unclear Python traceback.♻️ Optional: Add error handling
+try: with urllib.request.urlopen(url) as r: blob = r.read() +except urllib.error.URLError as e: + raise SystemExit(f"Failed to download gh: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 35 - 48, Wrap the network and archive operations in explicit error handling: around urllib.request.urlopen(url) catch urllib.error.HTTPError, urllib.error.URLError and socket.timeout and implement a small retry/backoff or at least raise a clear SystemExit with the HTTP status/error details; also catch tarfile.ReadError around tarfile.open(fileobj=...) and handle the case where tf.extractfile(gh_member) returns None with a descriptive SystemExit; ensure any failure logs the URL and out_path and exits non‑zero so the Docker build fails with a clear message (refer to urllib.request.urlopen, tarfile.open, tf.getmembers, tf.extractfile, out_path, os.chmod).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile`:
- Around line 38-47: The tar extraction should use an explicit extraction filter
to satisfy Python 3.12+ requirements; replace the current
tf.extractfile(gh_member) usage by calling tf.extract(...) (or tf.extractall
with a filter) and provide a filter callable that validates/whitelists the
member (e.g., accepts only the gh_member where
gh_member.name.endswith("/bin/gh")) before extraction so you explicitly allow
that path; update the code paths that reference tf.getmembers and gh_member and
ensure you extract to out_path (/usr/local/bin/gh) with correct permissions
after the filtered extraction.
- Around line 35-48: Wrap the network and archive operations in explicit error
handling: around urllib.request.urlopen(url) catch urllib.error.HTTPError,
urllib.error.URLError and socket.timeout and implement a small retry/backoff or
at least raise a clear SystemExit with the HTTP status/error details; also catch
tarfile.ReadError around tarfile.open(fileobj=...) and handle the case where
tf.extractfile(gh_member) returns None with a descriptive SystemExit; ensure any
failure logs the URL and out_path and exits non‑zero so the Docker build fails
with a clear message (refer to urllib.request.urlopen, tarfile.open,
tf.getmembers, tf.extractfile, out_path, os.chmod).
In `@Makefile`:
- Around line 19-25: AGENT_MOUNTS currently includes fixed host paths that will
cause Docker to fail if those host directories (e.g., $(HOME)/.codex,
$(HOME)/.codex-cli, $(HOME)/.openai-codex) don't exist; change the Makefile to
construct AGENT_MOUNTS dynamically by testing each host directory's existence
and only appending the corresponding mount (using the existing CONTAINER_HOME
variable for the container side) or create missing directories beforehand in a
helper target (e.g., a docker-collect or prepare-mounts target) so Docker sees
only valid mounts; modify the AGENT_MOUNTS definition to use a shell snippet or
phony target that filters/creates the three codex-related directories before
running the docker command.
- Around line 57-70: The verbose Makefile target docker-collect-verbose
currently omits $(COLLECT_FLAGS) so INCLUDE_EXCLUDED (and other flags) are
ignored; update the command that runs python3 skillbench.py collect in
docker-collect-verbose to append the collection flags using $(filter-out
-y,$(COLLECT_FLAGS)) so interactivity is preserved while still passing
INCLUDE_EXCLUDED and other flags (mirror how docker-collect uses
$(COLLECT_FLAGS) but remove the -y flag for interactivity).
In `@scripts/skillbench_docker_mounts.py`:
- Around line 45-47: The substring check `if gemini_tmp in ws` can produce false
positives; replace it with a path-aware check using pathlib so you only skip
mounts that are actually under the gemini_tmp directory (e.g., resolve both
paths and use Path.is_relative_to or compare the leading path parts) when
evaluating `ws` vs `gemini_tmp` in the mount loop in
scripts/skillbench_docker_mounts.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Lite
Run ID: 2a7f2041-40ac-4fad-ba3b-59ec0f438500
📒 Files selected for processing (5)
DockerfileMakefileREADME.mdscripts/skillbench_docker_mounts.pyskillbench.py
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
3 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
Documentation