Network mode host#6
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds agent KeyValue summary contributions and collection, implements the Vibe Kanban agent with supervisor, port discovery, and SummaryInfo; introduces host vs bridge networking with persistence; updates Docker builder/runner, purge/stop flows, tests, and documentation. ChangesAgent Summary Info and Vibe Kanban Implementation
Docker Networking and Image Lifecycle
Testing Infrastructure
Constants and Documentation
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/root.go (1)
677-700:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun agent health checks on the reconnect path too.
This early-return path skips the health-check loop on Lines 750-755. For an already-running container, the command can still print summary info/URLs without warning that an agent is unhealthy, so reconnect behavior is less reliable than fresh-start behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/root.go` around lines 677 - 700, The reconnect branch currently skips the agent health-check loop and returns early, so change it to run the same agent health checks before printing the session summary: reuse/extract the health-check logic that iterates over enabledAgents (the loop used later that detects unhealthy agents and emits warnings) into a helper (e.g., runAgentHealthChecks(ctx, c, containerName, enabledAgents) returning agentInfo and any health errors) and call it here before printSessionSummary(containerName, sshPort, enabledIDs, agentInfo); ensure warnings for unhealthy agents are emitted the same way as on fresh start and only then return.
🧹 Nitpick comments (2)
internal/docker/export_test.go (1)
1-1: ⚡ Quick winEmpty test file serves no purpose.
This file contains only a package declaration with no exports, functions, or test code. Consider either removing it if it was added accidentally, or clarifying its intended purpose if it's a placeholder for future test utilities.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/docker/export_test.go` at line 1, The file export_test.go currently only contains "package docker" and should either be deleted if accidental or turned into a real test placeholder: either remove export_test.go to avoid empty test files, or replace its contents with a minimal test scaffold (e.g., a TestExportPlaceholder function in package docker that calls t.Skip or add a clear TODO comment explaining intended tests) so it no longer is empty and causes confusion; locate export_test.go and update or remove it accordingly.internal/cmd/purge_test.go (1)
145-167: ⚡ Quick winAssert prune execution in the new ordering test.
The new test verifies image-removal order but doesn’t verify that
ImagesPruneactually ran; a regression could skip prune and still pass.✅ Suggested test hardening
err := cmd.RunPurgeWith(mock) require.NoError(t, err) require.Len(t, mock.removedImageIDs, 3) +require.True(t, mock.pruned, "purge should prune dangling images between instance and base removal")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/purge_test.go` around lines 145 - 167, The test currently checks removal order but not that ImagesPrune ran; update the test around the RunPurgeWith(mock) call to assert the mock's prune-call indicator (e.g., mock.pruneCalled, mock.ImagesPruneCalled, or mock.pruneCount) is set/greater than zero after cmd.RunPurgeWith returns; keep the new assertion near the top (immediately after require.NoError(t, err)) and reference the mock and ImagesPrune-related field/method names so a regression that skips ImagesPrune will fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.kiro/specs/bootstrap-ai-coding/design-vibekanban.md:
- Line 698: Two Dockerfile example code fences lack a language tag which
triggers markdownlint MD040; locate the two Dockerfile examples in the
design-vibekanban.md content (the fenced blocks used for Dockerfile snippets)
and add the language identifier by changing the opening fences to use
```dockerfile (or ```Dockerfile) so the code blocks declare the language and
satisfy MD040.
- Around line 137-179: Update the design doc to remove the stale core-coupled
requirements: delete or stop prescribing SessionSummary.VibeKanbanURL, the
discoverVibeKanbanPort() flow in root.go, and any direct core checks against
constants.VibeKanbanAgentName; likewise stop asserting bridge mode is not
host-accessible. Instead document that each agent implements
AgentInfo/SummaryInfo to expose its own summary (including any UI URL), and that
the renderer (FormatSessionSummary / root.go) should be core-agnostic and only
display what SummaryInfo provides; replace references to container port
discovery with the generic internal/docker/DiscoverListeningPort helper (or note
agent-side responsibility) and update the other listed sections (450-530,
592-597, 613-614) to reflect this agent-owned SummaryInfo pattern.
In `@internal/agents/vibekanban/integration_test.go`:
- Around line 67-72: The temp project directory created in setupSharedContainer
(variable projectDir / dirName) is never cleaned up; modify setupSharedContainer
to persist the full projectDir (e.g., store it on the test fixture struct or
return it) so teardownSharedContainer can access it, and in
teardownSharedContainer call os.RemoveAll(projectDir) (with error
handling/logging) to delete the temp dir; apply the same change for the other
creation sites referenced (lines ~194-209) so every MkdirTemp has a matching
RemoveAll in teardown.
In `@internal/agents/vibekanban/vibekanban.go`:
- Around line 164-170: The health check currently uses docker.ExecInContainer
with pgrep -f "vibe-kanban", which can match supervisor scripts like
"vibe-kanban-supervisor.sh" and produce false positives; update the pgrep
invocation in the same docker.ExecInContainer call to use a stricter match
(e.g., use pgrep -x "vibe-kanban" or pgrep -f with a word-boundary/anchor) so it
only matches the actual service process name, keep the existing exitCode
handling (exitCode == 0 means running) and error wrapping in the fmt.Errorf
return.
- Around line 87-99: The generated bash script uses "date +%%%%s" in the
fmt.Sprintf format string which becomes "date +%%s" in the output (causing
literal "%s" in bash); change the format token to "date +%%s" (i.e., use two
percent signs in the Go format string) where the restart timestamps are
appended/checked (the places that build the script around RESTART_TIMES, the
loop that prunes by WINDOW_SECONDS and the line that appends a new timestamp) so
the produced script contains "date +%s" and returns epoch seconds as intended.
In `@internal/cmd/purge.go`:
- Around line 83-87: The dangling-images prune call currently removes all host
dangling images because danglingFilter only sets "dangling=true" and ImagesPrune
is called directly; restrict pruning to BAC-owned artifacts by adding the BAC
label filter (e.g. "label", "bac.managed=true") to the filter set (or
merge/replace danglingFilter with the existing purgeFilter() usage) before
calling api.ImagesPrune so the prune only targets images matching both
dangling=true and bac.managed=true; update the code that builds danglingFilter
(symbol: danglingFilter) or call purgeFilter() and pass that filter into
ImagesPrune (symbol: ImagesPrune) to scope the operation.
In `@internal/cmd/root_test.go`:
- Around line 482-529: The test is falsely matching any line containing the key
and value; change the assertions to look for exact agent-info lines produced by
FormatSessionSummary instead of substring matches: for each agent.KeyValue in
agentInfo build the exact expectedLine (e.g. fmt.Sprintf("%s: %s", kv.Key,
kv.Value)) and assert that this expectedLine appears in lines at an index
greater than the detected "Enabled agents:" index (use equality or anchored
regex match against lines slice), replacing the current contains-based checks in
the loops that reference agentInfo and FormatSessionSummary.
In `@internal/cmd/root.go`:
- Around line 394-400: The current call to c.ImagesPrune with only
danglingFilter (dangling=true) is daemon-wide and can remove unrelated images;
change this so the purge is scoped to BAC-owned artifacts only: either add a
BAC-specific filter (e.g.,
danglingFilter.Add("label","<BAC-ownership-label>=true") or
danglingFilter.Add("reference","<bac-repo-or-name-pattern>") before calling
c.ImagesPrune, or replace the ImagesPrune call with a targeted flow that lists
images (c.ImageList / ImagesList) filtered for BAC-owned images and then
prunes/removes those specific image IDs; update the code around danglingFilter
and c.ImagesPrune to use that BAC-scoped filter or targeted deletion so --purge
only affects BAC resources.
In `@internal/docker/integration_test.go`:
- Around line 653-654: Replace the hardcoded SSH port assignments (e.g., the
sshPort := 22222 declarations) with dynamically allocated ports by calling the
existing helper findFreePort(); specifically, locate the sshPort variables in
integration_test.go (the sshPort := 22222 at the top and the similar
declarations around lines referenced) and change them to sshPort :=
findFreePort() (or the equivalent call pattern used elsewhere in this file),
ensuring any dependent uses continue to reference sshPort so tests bind to an
available port.
In `@internal/docker/runner_network_test.go`:
- Line 39: The handler currently checks the endpoint by slicing
r.URL.Path[len(r.URL.Path)-18:] which panics for short paths; replace that
fixed-length slice with strings.HasSuffix(r.URL.Path, "/containers/create") and
ensure strings is imported; keep the rest of the condition (r.Method ==
http.MethodPost) unchanged so the branch still triggers only for POST to the
create endpoint.
---
Outside diff comments:
In `@internal/cmd/root.go`:
- Around line 677-700: The reconnect branch currently skips the agent
health-check loop and returns early, so change it to run the same agent health
checks before printing the session summary: reuse/extract the health-check logic
that iterates over enabledAgents (the loop used later that detects unhealthy
agents and emits warnings) into a helper (e.g., runAgentHealthChecks(ctx, c,
containerName, enabledAgents) returning agentInfo and any health errors) and
call it here before printSessionSummary(containerName, sshPort, enabledIDs,
agentInfo); ensure warnings for unhealthy agents are emitted the same way as on
fresh start and only then return.
---
Nitpick comments:
In `@internal/cmd/purge_test.go`:
- Around line 145-167: The test currently checks removal order but not that
ImagesPrune ran; update the test around the RunPurgeWith(mock) call to assert
the mock's prune-call indicator (e.g., mock.pruneCalled, mock.ImagesPruneCalled,
or mock.pruneCount) is set/greater than zero after cmd.RunPurgeWith returns;
keep the new assertion near the top (immediately after require.NoError(t, err))
and reference the mock and ImagesPrune-related field/method names so a
regression that skips ImagesPrune will fail.
In `@internal/docker/export_test.go`:
- Line 1: The file export_test.go currently only contains "package docker" and
should either be deleted if accidental or turned into a real test placeholder:
either remove export_test.go to avoid empty test files, or replace its contents
with a minimal test scaffold (e.g., a TestExportPlaceholder function in package
docker that calls t.Skip or add a clear TODO comment explaining intended tests)
so it no longer is empty and causes confusion; locate export_test.go and update
or remove it accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 078b8525-f03f-4e85-a078-18d5b8198db9
📒 Files selected for processing (51)
.kiro/specs/bootstrap-ai-coding/design-agent-summary-info.md.kiro/specs/bootstrap-ai-coding/design-build-resources.md.kiro/specs/bootstrap-ai-coding/design-components.md.kiro/specs/bootstrap-ai-coding/design-data-models.md.kiro/specs/bootstrap-ai-coding/design-docker.md.kiro/specs/bootstrap-ai-coding/design-properties.md.kiro/specs/bootstrap-ai-coding/design-vibekanban.md.kiro/specs/bootstrap-ai-coding/design.md.kiro/specs/bootstrap-ai-coding/requirements-agent-summary-info.md.kiro/specs/bootstrap-ai-coding/requirements-agents.md.kiro/specs/bootstrap-ai-coding/requirements-cli-combinations.md.kiro/specs/bootstrap-ai-coding/requirements-core.md.kiro/specs/bootstrap-ai-coding/requirements-two-layer-image.md.kiro/specs/bootstrap-ai-coding/requirements.md.kiro/specs/bootstrap-ai-coding/tasks.md.kiro/steering/agent-module.md.kiro/steering/cli-flags.md.kiro/steering/constants.md.kiro/steering/product.md.kiro/steering/structure.mdinternal/agent/agent.gointernal/agent/registry_test.gointernal/agents/augment/augment.gointernal/agents/augment/augment_test.gointernal/agents/augment/integration_test.gointernal/agents/buildresources/buildresources.gointernal/agents/buildresources/buildresources_test.gointernal/agents/buildresources/integration_test.gointernal/agents/claude/claude.gointernal/agents/claude/claude_test.gointernal/agents/claude/integration_test.gointernal/agents/vibekanban/integration_test.gointernal/agents/vibekanban/vibekanban.gointernal/agents/vibekanban/vibekanban_test.gointernal/cmd/purge.gointernal/cmd/purge_test.gointernal/cmd/root.gointernal/cmd/root_test.gointernal/cmd/stop.gointernal/constants/constants.gointernal/datadir/datadir.gointernal/datadir/datadir_test.gointernal/docker/builder.gointernal/docker/builder_test.gointernal/docker/client.gointernal/docker/export_test.gointernal/docker/integration_test.gointernal/docker/runner.gointernal/docker/runner_network_test.gointernal/docker/testing_helper.gomain.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.kiro/specs/bootstrap-ai-coding/design-vibekanban.md:
- Around line 72-77: The design text and the supervisor script examples are
inconsistent: the spec says the supervisor should discover the port via
PID-based `ss` (`grep "pid=$VK_PID,"`) and write it to `/tmp/vibe-kanban.port`
for `SummaryInfo()` to read, but the supplied supervisor scripts only restart
the process and never write the port file; fix by either (A) updating both
supervisor script examples to implement the PID-based discovery loop (start
vibe-kanban, capture VK_PID, poll `ss -tlnp` filtered with `pid=$VK_PID,` until
a port is found, then write the numeric port to `/tmp/vibe-kanban.port` before
exiting the script) or (B) update the design text to remove the file-write
contract and change `SummaryInfo()`’s contract to match the actual restart-only
supervisor behavior—ensure all referenced places (the supervisor script examples
and the `SummaryInfo()` description) are made consistent.
In `@internal/docker/runner_network_test.go`:
- Line 98: The test currently does an unbounded receive from the fake server
channel (req := <-ch) which can hang if the fake server stops emitting; update
both occurrences (the receives at the two req := <-ch sites) to use a select
that waits on ch and a time.After timeout (e.g., time.After) and call t.Fatalf
or t.Fatal on timeout so the test fails deterministically instead of blocking
forever.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e57a2ac4-d1e5-476e-990a-30c3efcae75a
📒 Files selected for processing (9)
.kiro/specs/bootstrap-ai-coding/design-vibekanban.md.kiro/specs/bootstrap-ai-coding/tasks.mdinternal/agents/vibekanban/integration_test.gointernal/agents/vibekanban/vibekanban.gointernal/cmd/purge.gointernal/cmd/root.gointernal/cmd/root_test.gointernal/docker/integration_test.gointernal/docker/runner_network_test.go
💤 Files with no reviewable changes (1)
- .kiro/specs/bootstrap-ai-coding/tasks.md
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/agents/vibekanban/vibekanban.go
- internal/cmd/purge.go
- internal/cmd/root_test.go
- internal/docker/integration_test.go
- internal/agents/vibekanban/integration_test.go
- internal/cmd/root.go
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
1 participant
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.
@coderabbitai
Summary by CodeRabbit
New Features
Enhancements