Skip to content

feat: improve local container Docker test support#144

Merged
steipete merged 1 commit into
mainfrom
feat/local-container-docker-actions
May 22, 2026
Merged

feat: improve local container Docker test support#144
steipete merged 1 commit into
mainfrom
feat/local-container-docker-actions

Conversation

@steipete
Copy link
Copy Markdown
Contributor

Summary

  • add local-container Docker socket pass-through with active local Unix socket detection and host-visible work roots
  • expand local Actions hydration for repo-local composite actions, cache restore/save no-ops, simple input/step-output conditions, safe hashFiles, and Node 24.x setup
  • document Docker provider socket behavior and add regression coverage

Verification

  • go test ./internal/cli -run 'TestLocalActions'
  • go test ./internal/providers/localcontainer
  • go test ./...
  • /Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode local --parallel-tests 'go test ./...'
  • bin/crabbox warmup --provider docker --local-container-docker-socket --slug crabbox-socket-pr-smoke --keep --timing-json
  • bin/crabbox run --provider docker --id crabbox-socket-pr-smoke -- docker version --format '{{.Server.Version}} {{.Server.Os}}/{{.Server.Arch}}'
  • bin/crabbox stop --provider docker crabbox-socket-pr-smoke

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 22, 2026

Codex review: needs real behavior proof before merge.

Latest ClawSweeper review: 2026-05-22 09:26 UTC / May 22, 2026, 5:26 AM ET.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The PR adds opt-in local-container Docker socket pass-through, host-visible work roots, expanded local Actions hydration, docs, changelog, and regression tests.

Reproducibility: yes. for the PR defect: source inspection shows multi-pattern hashFiles is collapsed into one literal pattern, and the added tests only cover a single pattern. I did not run tests because this review was constrained to read-only inspection.

PR rating
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Summary: The patch is substantial and directionally useful, but missing real behavior proof plus a source-confirmed local Actions bug make it not quality-ready yet.

Rank-up moves:

  • Fix multi-pattern hashFiles parsing and add a regression test covering at least two patterns.
  • Add redacted terminal output, logs, or a recording that shows the Docker socket smoke command working after the change.
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Real behavior proof
Needs real behavior proof before merge: The PR body lists verification commands, including a Docker socket smoke, but does not include copied output, logs, screenshots, recordings, or linked artifacts showing the after-fix behavior; contributors should redact private paths, IPs, tokens, phone numbers, and non-public endpoints before posting proof, and updating the PR body should trigger a fresh ClawSweeper review or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • Merging as-is would silently produce empty hashFiles values for standard multi-pattern expressions, which can make local Actions cache keys or conditions diverge from GitHub Actions.
  • The Docker socket feature intentionally gives opted-in local-container leases access to the host Docker daemon, so maintainers should be comfortable with that documented boundary before merge.

Maintainer options:

  1. Fix multi-pattern hashFiles before merge (recommended)
    Parse hashFiles arguments as a comma-separated Actions argument list, hash matches from every valid pattern, and add a regression test for at least two patterns.
  2. Accept limited local hydration divergence
    Maintainers could intentionally accept the unsupported multi-pattern behavior, but the docs and error behavior should make that limitation explicit instead of returning an empty hash silently.

Next step before merge
Needs human PR follow-up because the contributor must provide real behavior proof and the hashFiles defect should be fixed before merge.

Security
Cleared: The diff is security-sensitive because it adds host Docker socket access, but the access is opt-in, documented, rejects remote Docker hosts, and does not add new dependencies or secret handling.

Review findings

  • [P2] Parse multi-pattern hashFiles arguments correctly — internal/cli/actions.go:1274-1279
Review details

Best possible solution:

Keep the feature direction, but fix hashFiles argument parsing with focused regression coverage and add real Docker socket smoke proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes for the PR defect: source inspection shows multi-pattern hashFiles is collapsed into one literal pattern, and the added tests only cover a single pattern. I did not run tests because this review was constrained to read-only inspection.

Is this the best way to solve the issue?

No, not as-is. The overall feature path is maintainable, but hashFiles needs proper multi-pattern parsing and the PR needs observed real behavior proof before it is merge-ready.

Label changes:

  • add P2: This is a normal priority feature PR with a contained but merge-blocking local Actions correctness issue.
  • add merge-risk: 🚨 automation: The PR changes local Actions hydration, and the current hashFiles implementation can silently generate wrong cache-key values for common workflows.
  • add rating: 🧂 unranked krab: Current PR rating is 🧂 unranked krab because proof is 🧂 unranked krab, patch quality is 🦐 gold shrimp, and The patch is substantial and directionally useful, but missing real behavior proof plus a source-confirmed local Actions bug make it not quality-ready yet.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists verification commands, including a Docker socket smoke, but does not include copied output, logs, screenshots, recordings, or linked artifacts showing the after-fix behavior; contributors should redact private paths, IPs, tokens, phone numbers, and non-public endpoints before posting proof, and updating the PR body should trigger a fresh ClawSweeper review or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a normal priority feature PR with a contained but merge-blocking local Actions correctness issue.
  • merge-risk: 🚨 automation: The PR changes local Actions hydration, and the current hashFiles implementation can silently generate wrong cache-key values for common workflows.
  • rating: 🧂 unranked krab: Current PR rating is 🧂 unranked krab because proof is 🧂 unranked krab, patch quality is 🦐 gold shrimp, and The patch is substantial and directionally useful, but missing real behavior proof plus a source-confirmed local Actions bug make it not quality-ready yet.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists verification commands, including a Docker socket smoke, but does not include copied output, logs, screenshots, recordings, or linked artifacts showing the after-fix behavior; contributors should redact private paths, IPs, tokens, phone numbers, and non-public endpoints before posting proof, and updating the PR body should trigger a fresh ClawSweeper review or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Full review comments:

  • [P2] Parse multi-pattern hashFiles arguments correctly — internal/cli/actions.go:1274-1279
    hashFiles accepts comma-separated patterns, but this implementation trims the whole argument string into one pattern and passes it to single-pattern matching. Expressions like hashFiles('**/package-lock.json', '**/Gemfile.lock') therefore retain comma/quote characters, match nothing, and silently produce an empty cache key during local hydration.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.88

What I checked:

  • Current main lacks requested surface: Current main has no DockerSocket, dockerSocket, local-container-docker-socket, hashFiles, local cache restore, or repo-local composite action support in the inspected implementation/docs paths. (4bea91906edc)
  • PR head adds broad feature surface: The PR head changes 12 files with local-container Docker socket config/docs/tests plus local Actions hydration changes. (e5dfadf713eb)
  • hashFiles parser bug: localActionsHashFiles trims the whole argument string and passes it as one pattern, so comma-separated hashFiles('a', 'b') expressions retain comma/quote characters and do not match separate patterns. (internal/cli/actions.go:1274, e5dfadf713eb)
  • Test coverage gap for multi-pattern hashFiles: The added hashFiles test covers a single recursive glob but not GitHub Actions' comma-separated multi-pattern form. (internal/cli/actions_test.go:469, e5dfadf713eb)
  • Docker socket security posture: The PR mounts the selected local Unix Docker socket only when LocalContainer.DockerSocket is true and rejects non-local Docker hosts; docs also describe this as opt-in host daemon access. (internal/providers/localcontainer/backend.go:274, e5dfadf713eb)
  • Feature history provenance: The checked-out main history for the central local Actions/local-container files is dominated by commit 77353c1, which current blame/log attributes to Peter Steinberger. (internal/cli/actions.go:642, 77353c1642be)

Likely related people:

  • steipete: Current-main git log, git blame, and git shortlog for the local Actions and local-container provider paths point to Peter Steinberger's prior merged commit, and the PR context maps that contributor to steipete. (role: recent area contributor and PR author with prior main history; confidence: high; commits: 77353c1642be, e5dfadf713eb; files: internal/cli/actions.go, internal/providers/localcontainer/backend.go, internal/providers/localcontainer/flags.go)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 4bea91906edc.

@steipete steipete force-pushed the feat/local-container-docker-actions branch from 5b7bace to e5dfadf Compare May 22, 2026 09:21
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5b7bace8ff

ℹ️ 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 thread internal/cli/actions.go
Comment on lines +1274 to +1278
arg := strings.TrimSpace(strings.TrimSuffix(strings.TrimPrefix(expr, "hashFiles("), ")"))
arg = strings.Trim(arg, `'"`)
if !validLocalActionsHashPattern(arg) {
return "", true
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Parse multi-pattern hashFiles arguments correctly

hashFiles accepts comma-separated patterns (for example hashFiles('**/package-lock.json', '**/Gemfile.lock')), but this implementation trims the full argument into a single pattern string and passes it to one-pattern matching. In that case the comma/quote characters remain part of the pattern, no files match, and the function returns an empty hash even when matching files exist. That silently changes interpolated values used in keys/conditions during local hydration and can cause incorrect cache-key behavior compared with GitHub Actions.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels May 22, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 22, 2026

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@steipete steipete merged commit d303d6b into main May 22, 2026
11 checks passed
@steipete steipete deleted the feat/local-container-docker-actions branch May 22, 2026 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant