Skip to content

Add docker-test skill for running commands in arcticdb Docker container#2947

Open
G-D-Petrov wants to merge 3 commits intomasterfrom
gpetrov/adb_docker_skill
Open

Add docker-test skill for running commands in arcticdb Docker container#2947
G-D-Petrov wants to merge 3 commits intomasterfrom
gpetrov/adb_docker_skill

Conversation

@G-D-Petrov
Copy link
Copy Markdown
Collaborator

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?


Run the user's command inside the container:
```bash
docker exec -w /workspace arc-dev $ARGUMENTS
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unquoted $ARGUMENTS may behave unexpectedly

docker exec -w /workspace arc-dev $ARGUMENTS

$ARGUMENTS is unquoted. If the argument string contains spaces (e.g. a file path like python/tests/my test.py) word-splitting will break it across multiple tokens. It also means shell metacharacters in user-provided arguments (semicolons, pipes, backticks) would be interpreted by the shell before Docker sees them.

Since the intended usage is to pass an arbitrary command string (e.g. pytest python/tests/unit/some_test.py), the cleaner form is:

Suggested change
docker exec -w /workspace arc-dev $ARGUMENTS
docker exec -w /workspace arc-dev bash -c "$ARGUMENTS"

This makes it explicit that the full argument string is a shell command fragment, avoids unexpected word-splitting, and matches the interactive usage shown in the Usage section.


Check if the container is running:
```bash
docker ps --filter name=arc-dev --format '{{.Names}}' | grep -q arc-dev
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Container check is fragile when a stopped container exists

The "is running" check is:

docker ps --filter name=arc-dev --format '{{.Names}}' | grep -q arc-dev

docker ps only shows running containers. If arc-dev exists but is stopped, this check returns false, and the next step tries docker run, which will fail with Error: Conflict. The container name "/arc-dev" is already in use. The instructions do handle this case, but the flow described is:

  1. Check running → false
  2. Try docker runfails with a conflict error
  3. Only then is docker start arc-dev mentioned

The agent will likely hit the error before reaching step 3. A more robust ordering would be to check docker ps -a (all containers, including stopped) first, then branch on whether the container exists at all vs. just stopped.

If not running, start it with the repo mounted:
```bash
docker run -d --name arc-dev -v "$(pwd)":/workspace -w /workspace arcticdb-env sleep infinity
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$(pwd) in a skill file may not produce the expected working directory

docker run -d --name arc-dev -v "$(pwd)":/workspace -w /workspace arcticdb-env sleep infinity

When an AI agent executes this snippet inside a bash -c call (which is the typical skill invocation model), $(pwd) will expand to whatever the agent's current working directory is at that moment — which may or may not be the repository root. If the agent's shell CWD is somewhere else, the wrong directory gets mounted.

Consider documenting that this command must be run from the repository root, or replacing $(pwd) with an explicit path derived from git rev-parse --show-toplevel:

REPO_ROOT=$(git rev-parse --show-toplevel)
docker run -d --name arc-dev -v "$REPO_ROOT":/workspace -w /workspace arcticdb-env sleep infinity


If it doesn't exist, build it from the repo root:
```bash
docker build -f docker/Dockerfile -t arcticdb-env .
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dockerfile path has diverged from the actual repo structure

The skill says:

Dockerfile: docker/Dockerfile

But the actual file in the repo is docker/Dockerfile (confirmed), so this is correct. However, the build command in Step 1 is:

docker build -f docker/Dockerfile -t arcticdb-env .

Building from the repo root (.) with docker/Dockerfile as context will include the entire repository tree in the Docker build context, which is very large (C++ sources, build artefacts, etc.) and will be slow. Consider adding a note that a .dockerignore file should exist or that users may see a large context upload, or point the build context to the docker/ subdirectory if the Dockerfile permits it.

---

Run commands or tests inside the arcticdb Docker container.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No mention of what the Docker image actually provides / prerequisites

The Usage section jumps straight to example invocations without explaining what environment the container provides (Python version, whether ArcticDB is pre-installed or needs to be built, etc.). A developer encountering this skill for the first time won't know whether they need to build ArcticDB into the image first or whether import arcticdb will work out of the box.

Adding a brief note like "The image is a build environment — ArcticDB must be installed into it (e.g. by running pip install -ve . inside the container) before running tests" would significantly improve usability.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 4, 2026

ArcticDB Code Review Summary

PR #2947Add docker-test skill for running commands in arcticdb Docker container

This PR adds a single new file: .claude/skills/docker-test/SKILL.md. It introduces a Claude Code slash-command skill (/docker-test) that instructs the AI agent to manage an arcticdb-env Docker container and execute arbitrary commands inside it. The change is purely documentation/tooling — no C++ or Python source code is modified.


API & Compatibility

  • No breaking changes to public Python API — N/A
  • On-disk format unchanged — N/A
  • Protobuf schema backwards-compatible — N/A
  • Key types and ValueType enum unchanged — N/A

Memory & Safety

  • All memory/safety checks — N/A (no C++ or Python code)

Correctness

  • All correctness checks — N/A (skill/documentation file only)

Code Quality

  • Shell robustness: Fixed — Step 3 now always routes through bash -c "$ARGUMENTS", eliminating word-splitting issues. (was ❌)
  • Container lifecycle: Fixed — uses docker inspect --format '{{.State.Status}}' correctly. (was ❌)
  • Working directory assumption: Fixed — uses git rev-parse --show-toplevel. (was ❌)
  • Large Docker build context: Fixed — now uses docker build -t arcticdb-env docker/ (scoped context, not .). (was ❌)
  • Missing prerequisites note: Fixed — header now states "The image is a build environment only — ArcticDB is not pre-installed." (was ❌)
  • xxd portability: Fixed — now uses openssl rand -hex 4 instead of xxd. (was ❌)

Testing

  • No automated tests applicable (skill/documentation file only)
  • Skill has not been validated against actual container/image names in CI

Build & Dependencies

  • No CMakeLists.txt, dependency, or cross-platform changes — N/A

Security

  • No hardcoded credentials or secrets
  • ✅ Shell injection risk eliminated — all execution paths now use bash -c "$ARGUMENTS"

PR Title & Description

  • Title is clear, concise, uses imperative verb, and is grammatically correct
  • PR is labelled (patch)
  • ❌ Description body is completely empty — "What does this implement or fix?" section is blank
  • ❌ PR author checklist is entirely unchecked

Documentation

  • No public API docstrings affected
  • No breaking changes

Summary

All previously flagged code issues have been resolved in this commit:

  • ✅ Shell safety: bash -c "$ARGUMENTS" is now used for all execution paths
  • ✅ Docker build context: scoped to docker/ instead of the entire repo tree
  • ✅ Prerequisites: build-environment-only note added at the top
  • xxd portability: replaced with openssl rand -hex 4

One remaining item before merge: fill in the PR description ("What does this implement or fix?") and tick the author checklist.

@G-D-Petrov G-D-Petrov added the patch Small change, should increase patch version label Mar 4, 2026

If no `--name` was provided, generate a random name on **first invocation only**:
```bash
_ARC_CONTAINER="arc-dev-$(head -c4 /dev/urandom | xxd -p)"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xxd is not universally available

_ARC_CONTAINER="arc-dev-$(head -c4 /dev/urandom | xxd -p)"

xxd is part of vim/vim-common and is absent on Alpine/BusyBox-based images and some minimal CI environments. Safer alternatives:

Suggested change
_ARC_CONTAINER="arc-dev-$(head -c4 /dev/urandom | xxd -p)"
_ARC_CONTAINER="arc-dev-$(openssl rand -hex 4)"

or, if openssl is also not guaranteed:

_ARC_CONTAINER="arc-dev-$(od -A n -t x4 -N 4 /dev/urandom | tr -d ' \n')"

openssl rand -hex 4 is available on virtually all developer machines and standard Docker images.


Run the user's command inside the container. For simple commands pass them directly:
```bash
docker exec -w /workspace "$_ARC_CONTAINER" $ARGUMENTS
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unquoted $ARGUMENTS still breaks paths with spaces in the simple-command path

docker exec -w /workspace "$_ARC_CONTAINER" $ARGUMENTS

The previous commit correctly wrapped shell-operator cases in bash -c "$ARGUMENTS", but this direct path still subjects $ARGUMENTS to host-shell word splitting. A test path like python/tests/my test dir/test_foo.py will be split into separate tokens and the exec will fail.

The simplest fix is to always use bash -c — the overhead is negligible and it eliminates the ambiguity of the "simple vs. complex" heuristic:

Suggested change
docker exec -w /workspace "$_ARC_CONTAINER" $ARGUMENTS
docker exec -w /workspace "$_ARC_CONTAINER" bash -c "$ARGUMENTS"

If the bash -c form is kept for the operator case only, the instruction should at minimum note that paths with spaces are not supported in the direct form.

## Instructions

Image name: `arcticdb-env`
Dockerfile: `docker/Dockerfile`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually subtly different from the docker images we use on the CI. For C++ test I think we use Dockerfile_clang and for python wheel we use the bespoke build_manylinux_image.sh.

It might be useful to add some description of this and information of how the ghcr images are called so when you tell claude "Reproduce this failure on the CI within the docker image" it can figure out which image it needs to pull

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Small change, should increase patch version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants