-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/docker-guard-package #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThe changes enforce Docker execution for tests by integrating the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this 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-guardpackage, 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-testanddeno-testservices only setGIT_STUNTS_DOCKER=1, whilenode-testalso includesNODE_ENV=test. If any test code checksNODE_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=1Also applies to: 21-22
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mddocker-compose.ymlpackage.jsonsrc/domain/services/CommandSanitizer.jstest.jstest/deno_entry.jstest/support/ensure-docker.jsvitest.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-guardv0.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 ifGIT_STUNTS_DOCKER=1is 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.jsat 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
setupFilesto ensure Docker checks run before tests. The referenced setup file properly imports and invokesensureDocker()from@git-stunts/docker-guard, establishing the Docker environment guard as part of the test initialization pipeline.
repository.
tarball.
dependency error by rebuilding the test images inside Docker.
Summary by CodeRabbit
Release Notes
New Features
GIT_STUNTS_DOCKER=1environment variable when running tests.Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.