feat: approximate Compose's restart: policy with a foreground wip up --watch poll loop - #73
Merged
Merged
Conversation
… --watch` poll loop wslc has no restart-policy support and no public event-stream CLI (confirmed against the now-public microsoft/WSL source: no RestartPolicy concept anywhere in the engine, and the one in-progress events PR — microsoft/WSL#40971 — is SDK/COM-level only, not something wip's shell-out-to-CLI architecture could consume even once merged). Polling `wslc list --all --format json` is the only viable mechanism, so `compose.yml`'s `restart:` has been silently dropped by mode: compose-native until now — a crashed sidecar (e.g. a MySQL dependency) just stays down until someone manually reruns `wip up`. `wip up --watch` closes that gap without adding a background daemon (which the README explicitly rules out): it's a foreground, opt-in poll loop the user keeps a terminal open for, the same shape as the already-shipped `wip sync --watch`. `restart:` is now parsed (compose-native) and accepted (mode: container) instead of ignored, defaulting to "no" — including handling YAML's boolean coercion of an unquoted `restart: no` into `false`. The loop treats always/unless-stopped/on-failure identically (restart on exited/dead, no exit-code gating) and is deliberately status-based rather than transition-based, so it can race with a concurrent manual `wip stop`/`down` in another terminal — documented as a known limitation rather than solved. The exited/dead status read from `wslc list --all --format json` assumes a `State` field matching `docker ps --format json`'s own shape; unconfirmed against a real wslc install (no fixture exists anywhere in this repo for that output today), so it's isolated to one method and logged under --debug to make a wrong guess immediately visible rather than silently inert. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 minutes This review is too large to run within your organization's remaining usage spending cap. Raise or remove your spending cap in the billing tab, then retry. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis change adds restart-policy support for dependencies and Compose services. ChangesDependency restart watching
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WipCLI
participant ContainerRuntime
User->>WipCLI: run wip up --watch
WipCLI->>ContainerRuntime: start containers detached
loop configured interval
WipCLI->>ContainerRuntime: query dependency status
ContainerRuntime-->>WipCLI: return JSON status
WipCLI->>ContainerRuntime: restart eligible exited or dead containers
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in wip up --watch foreground poll loop to approximate Docker Compose restart: behavior when running under mode: compose-native, and ensures restart: is parsed/retained (instead of ignored) across config sources.
Changes:
- Add
wip up --watch+--intervalto pollwslc list --all --format jsonand restart exited/dead dependencies whoserestart:policy allows it. - Parse and default
restart:to"no"in bothwip.ymldependencies and compose-nativecompose.ymlparsing, including normalization of YAML’s unquotedrestart: no(booleanfalse). - Update docs and templates to expose the new
restart:key and describe the--watchbehavior/limitations.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/wip/initializer_spec.rb | Updates initializer template expectations to include restart. |
| spec/wip/config_spec.rb | Asserts default restart: "no" and YAML-boolean normalization in config dependencies. |
| spec/wip/compose_file_spec.rb | Covers compose-native parsing/defaulting of restart: and normalization of unquoted no. |
| spec/wip/cli_spec.rb | Adds coverage for up --watch behavior (detach implication, restart loop behavior, validation, debug logging, compose-mode rejection). |
| README.md | Documents restart: semantics and the wip up --watch poll loop behavior/limitations. |
| lib/wip/initializer.rb | Adds restart: "no" to the generated container-mode template. |
| lib/wip/config.rb | Adds default restart: "no" and normalizes YAML false to "no" for wip.yml dependencies. |
| lib/wip/compose_file.rb | Parses restart: from compose files, defaults it to "no", and normalizes YAML false to "no". |
| lib/wip/cli.rb | Implements up --watch polling + restart logic and enforces watch incompatibility with mode: compose. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@lib/wip/cli.rb`:
- Around line 85-88: Update the restart-policy matching logic that consumes
AUTO_RESTART_POLICIES so only exact “always” and “unless-stopped” values, plus
valid “on-failure” values with an optional numeric retry suffix, trigger
restarts; reject prefixed invalid values such as “always-invalid”,
“unless-stopped-extra”, and “on-failurex”. Add coverage for these invalid
prefixed policies.
- Around line 97-105: Validate the watch interval at the start of `up`, before
`ensure_compose_images`, `ensure_network`, dependency startup, sync, or
container creation; reuse the existing validation around the interval handling
near `watch_restarts`/`ConfigError`. Add a spec asserting that an invalid
`--watch --interval` causes `ConfigError` and no startup command is executed.
- Around line 480-494: Update container_status to convert WSLC’s integer State
enum into the string states consumed by watch-mode comparisons, mapping Exited
(3) to exited and Deleted (4) to the appropriate terminal state while leaving
other states handled consistently. Add a fixture covering the WSLC integer State
payload and verify exited containers trigger the existing restart behavior.
In `@lib/wip/config.rb`:
- Around line 212-214: Update the restart normalization logic in the surrounding
configuration processing to convert explicit nil and empty-string restart values
to "no", matching ComposeFile#normalize_restart and preserving the existing
false-to-"no" conversion. Add examples covering both restart: and restart: ""
inputs.
In `@README.md`:
- Around line 211-214: Update the README guidance describing the JSON response
to distinguish the capitalized `State` field name from its lowercase values,
including `exited` and `dead`. Preserve the existing diagnostic instructions for
checking the logged `list` entry when watch mode does not restart an exited
service.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63f82a67-15cc-4fce-9fe4-5e7eda5e0661
📒 Files selected for processing (9)
README.mdlib/wip/cli.rblib/wip/compose_file.rblib/wip/config.rblib/wip/initializer.rbspec/wip/cli_spec.rbspec/wip/compose_file_spec.rbspec/wip/config_spec.rbspec/wip/initializer_spec.rb
…s review fixes CodeRabbit flagged that container_status assumed a lowercase string State field (docker ps-style) with no way to confirm it against a real wslc install. Verified directly against microsoft/WSL's own docs and source (ContainerModel.h's ContainerInformation#State has no custom JSON enum serializer, so nlohmann::json emits WslcContainerState's raw ordinal): State is an integer — 0 invalid, 1 created, 2 running, 3 exited, 4 deleted — with no separate "dead" state. `%w[exited dead].include?(status)` against that integer could never match, so the whole restart mechanism would have silently never fired against a real wslc install. Fixed to compare against the confirmed WSLC_CONTAINER_STATE_EXITED (3) directly. Also addresses the rest of CodeRabbit's review: - auto_restart? used start_with?, so "always-invalid" or "on-failurex" would incorrectly trigger a restart; now exact-matches always/ unless-stopped and regex-matches on-failure[:N] only. - --interval is now validated before any startup side effect (image build, network/dependency/container creation), not after — threaded through to watch_restarts instead of re-read from inside the loop. - config.rb's validate_dependency! now normalizes an explicit nil/"" restart: to "no" too, not just the YAML `false` case, matching ComposeFile#normalize_restart's equivalent handling. - README's restart-detection bullet rewritten with the verified enum facts instead of the old, unconfirmed docker ps-shaped guess. lib/wip/config.rb added to Metrics/ClassLength's exclude list (joining cli.rb and compose_file.rb, already excluded there) rather than cramming the new normalization logic to dodge a 3-line overage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot caught that the README's sample transposed "every 5s" and "for exited restart: containers" relative to what cli.rb's watch_restarts actually prints. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/wip/cli.rb:466
watch_restartslogs that it is watching all dependencies, but the loop only performs status checks/restarts for dependencies whoserestart:policy matchesauto_restart?. This makes the startup message misleading and also needlessly iterates over entries that will never be polled.
names = load_config.dependencies.keys
warn "wip: watching #{names.join(', ')} for exited restart: containers every #{interval}s " \
'(running detached; Ctrl-C to stop)'
loop do
names.each { |name| restart_if_exited(name) }
lib/wip/cli.rb:502
container_statusassumeswslc list --format jsonreturns an array of hashes. If it ever returns a non-array JSON value (e.g., a hash),JSON.parse(output).firstwill yield a non-hash andfetch('State')can raise (e.g., TypeError for Array#fetch with a string key), which would crash the--watchloop. This method is intended to fail closed (return nil), so it should validate the parsed JSON shape before indexing.
entry = JSON.parse(output).first
warn "wip: [debug] '#{name}': #{entry.inspect}" if debug?
entry&.fetch('State', nil)
rescue JSON::ParserError
nil
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
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.
Summary
restart:incompose.ymlwas silently dropped bymode: compose-native—wslc run/exechas no restart-policy flag. A crashed sidecar (e.g. a MySQL dependency) just stayed down until someone manually reranwip up.wslcitself (now open-sourced inmicrosoft/WSL) could support this some other way: it can't, today. NoRestartPolicyconcept anywhere in its engine, and no publicwslc eventsCLI command (only an unmerged, SDK/COM-level PR — Wslc events microsoft/WSL#40971 — not consumable from wip's shell-out-to-CLI architecture even once merged). The only viable mechanism is pollingwslc list --all --format json, which wip already calls.wip up --watchadds that polling as a foreground, opt-in loop — the same shape as the already-shippedwip sync --watch— not a background daemon (which the README explicitly rules out as a non-goal; see the updated Roadmap section for how this stays inside that boundary).restart:is now parsed (compose-native) and accepted (mode: container) instead of ignored, defaulting to"no". Handles a real gotcha: YAML coerces an unquotedrestart: noto the booleanfalse, not the string"no"— both parsing paths correct this back.always/unless-stopped/on-failureare all treated identically (restart on exited/dead, no exit-code gating foron-failure), and the loop is status-based rather than transition-based, so it can race with a concurrent manualwip stop/downin another terminal — documented as a known limitation, not solved.Known unverified assumption (flagged prominently in code + README)
The exited/dead detection assumes
wslc list --all --format jsonreports a lowercaseStatefield matchingdocker ps --format json's own shape. No fixture or sample output exists anywhere in this repo to confirm the exact field name against a realwslcinstall — isolated to one method (container_statusincli.rb) and logged under--debugso a wrong guess is immediately visible rather than silently inert. Needs a follow-up check against a real WSLC install (see README's new "Restarting exited dependencies" section for exactly what to check).Test plan
bundle exec rspec(278 examples, 0 failures)bundle exec rubocop(46 files, no offenses)Config#dependencymergesrestart: "no"default correctly for bothmode: containerandmode: compose-native; unquotedrestart: nonormalizes correctly in both;wip init's scaffolded template parses with the new key.wslc list --all --format json's actual field name/values for container status.wip up --watch --debugagainst a real stack with arestart: alwayssidecar, kill it externally, confirm it gets restarted.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
wip up --watchto automatically restart exited dependency services based on configured restart policies.--interval.Bug Fixes
Documentation