Skip to content

Conversation

@flyingrobots
Copy link
Member

@flyingrobots flyingrobots commented Jan 8, 2026

  • Summary
    • Switches the plumbing repo to depend on the freshly published @git-stunts/docker-guard package instead of the nested
      repository.
    • Removes the embedded guard .git and tightens up package-lock.json so Docker-based tests pull the published guard
      tarball.
    • Verifies that npm test (which orchestrates the guarded Docker compose runs) now passes without the earlier E402
      dependency error by rebuilding the test images inside Docker.
  • Testing
    • npm test

Summary by CodeRabbit

Release Notes

  • New Features

    • Added Docker environment safety mechanism to prevent tests from running on the host system.
    • Requires GIT_STUNTS_DOCKER=1 environment variable when running tests.
  • Documentation

    • Added safety guidelines in README detailing Docker execution requirements and setup instructions.
  • Chores

    • Added docker-guard dependency for environment validation.
    • Extended allowed Git commands in sanitizer.
    • Updated Docker Compose services with required environment variable.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 8, 2026

📝 Walkthrough

Walkthrough

The changes enforce Docker execution for tests by integrating the @git-stunts/docker-guard package across configuration files, test entry points, and docker-compose services. A GIT_STUNTS_DOCKER environment variable requirement is introduced, CommandSanitizer expands to allow 'log' commands, and documentation is added explaining safety requirements.

Changes

Cohort / File(s) Summary
Configuration & Documentation
CHANGELOG.md, README.md, package.json
Added changelog entries documenting Docker Guard feature (Unreleased 2026-01-08 and version 2.8.0); added safety section to README detailing Docker execution requirements and ensureDocker usage; added @git-stunts/docker-guard ^0.1.0 as devDependency.
Docker Environment Setup
docker-compose.yml, vitest.config.js, test/support/ensure-docker.js
Added GIT_STUNTS_DOCKER=1 environment variable to node-test, bun-test, and deno-test services; created new Vitest config with setupFiles pointing to ensure-docker.js; created new ensure-docker.js support file importing and invoking ensureDocker().
Test Entry Points
test.js, test/deno_entry.js
Added top-level imports for ensureDocker from @git-stunts/docker-guard and Deno shim to enforce Docker availability before tests; added import of ensure-docker.js support file in Deno entry.
Domain Logic
src/domain/services/CommandSanitizer.js
Extended static _ALLOWED_COMMANDS set to include 'log' command in sanitization whitelist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Docker guards now stand so tall,
Tests run safe within the wall,
No host execution shall we allow,
Container comfort, here and now! 🐳

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The PR title 'Feature/docker-guard-package' is partially related to the changeset. It references the docker-guard package addition, but is vague and overly broad—it doesn't clearly communicate the main change of switching to a published package dependency versus the embedded implementation. Consider a more descriptive title like 'Switch to published @git-stunts/docker-guard package' or 'Use published docker-guard package instead of embedded repo' to clearly convey the primary objective of the changeset.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @package.json:
- Around line 38-39: The new devDependency @git-stunts/docker-guard@0.1.0 is
very recently published and unvetted—either remove it from package.json or
perform vetting before merging: confirm the package is necessary for the repo,
verify the publisher identity and repository/README/commit history for
@git-stunts/docker-guard, run a manual security review (inspect package tarball,
source code, and verify no hidden postinstall scripts), consider pinning to a
specific checksum (or use an internal script/known tool as a replacement), and
document the decision in the PR summary or package.json comments so the team
accepts the supply-chain risk.
🧹 Nitpick comments (2)
CHANGELOG.md (1)

8-24: CHANGELOG accurately documents the changes.

The entries clearly describe the transition from an embedded DockerGuard to the published @git-stunts/docker-guard package, along with the related changes to command whitelisting and test infrastructure.

Minor note: Line 13 contains "Minecraft" before src/infrastructure/DockerGuard.js - this appears to be either a typo or an internal reference that might confuse external readers.

📝 Consider clarifying the "Minecraft" reference

If "Minecraft" is not intentional, consider removing it:

-- **Plumbing Guard Wiring**: `@git-stunts/plumbing` now imports the shared guard via `test/support/ensure-docker.js`, `vitest.config.js`, and `test/deno_entry.js`, removing the in-repo Minecraft `src/infrastructure/DockerGuard.js`.
+- **Plumbing Guard Wiring**: `@git-stunts/plumbing` now imports the shared guard via `test/support/ensure-docker.js`, `vitest.config.js`, and `test/deno_entry.js`, removing the in-repo `src/infrastructure/DockerGuard.js`.
docker-compose.yml (1)

14-15: Consider adding NODE_ENV=test for consistency.

The bun-test and deno-test services only set GIT_STUNTS_DOCKER=1, while node-test also includes NODE_ENV=test. If any test code checks NODE_ENV, this inconsistency could lead to different behavior across runtimes.

Suggested consistency improvement
   bun-test:
     build:
       context: .
       dockerfile: Dockerfile.bun
     environment:
+      - NODE_ENV=test
       - GIT_STUNTS_DOCKER=1

   deno-test:
     build:
       context: .
       dockerfile: Dockerfile.deno
     environment:
+      - NODE_ENV=test
       - GIT_STUNTS_DOCKER=1

Also applies to: 21-22

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7719ce1 and 7b0face.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (9)
  • CHANGELOG.md
  • README.md
  • docker-compose.yml
  • package.json
  • src/domain/services/CommandSanitizer.js
  • test.js
  • test/deno_entry.js
  • test/support/ensure-docker.js
  • vitest.config.js
🔇 Additional comments (6)
README.md (1)

23-42: Excellent safety documentation!

The new section clearly communicates the Docker execution requirement and provides practical guidance for developers. The reference to @git-stunts/docker-guard v0.1.0+ aligns with the package.json dependency constraint.

test/support/ensure-docker.js (1)

1-3: Correct implementation for Docker environment enforcement.

The module-load-time invocation of ensureDocker() is appropriate for Vitest's setupFiles mechanism, ensuring the Docker environment check occurs before tests run. Per the README, ensureDocker() will terminate the process if GIT_STUNTS_DOCKER=1 is not set, which is the intended safety check for the test suite.

test/deno_entry.js (1)

1-1: LGTM! Docker guard integration properly placed.

The import of ensure-docker.js at the top ensures Docker environment checks run before any test execution, which aligns with the Docker guard integration objectives.

test.js (2)

9-9: LGTM! Deno compatibility shim loaded early.

The Deno shim import is appropriately placed after the Docker guard check and before the main test logic, ensuring cross-runtime compatibility.


5-7: Integration of @git-stunts/docker-guard is correct.

The ensureDocker() call properly enforces Docker availability before tests run. Package version 0.1.0 exists in npm registry, the API usage matches the published documentation, and no security advisories are present.

vitest.config.js (1)

1-7: LGTM! Standard Vitest configuration with Docker environment guard.

The configuration correctly uses setupFiles to ensure Docker checks run before tests. The referenced setup file properly imports and invokes ensureDocker() from @git-stunts/docker-guard, establishing the Docker environment guard as part of the test initialization pipeline.

@flyingrobots flyingrobots merged commit 3251539 into main Jan 8, 2026
3 checks passed
@flyingrobots flyingrobots deleted the feature/docker-guard-package branch January 8, 2026 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants