fix: retrospective review fixes for merged PRs #9/#10/#11/#12/#18/#19/#20#85
Merged
Conversation
…t/doc accuracy Extracted the "is a container with this name actually running" check (client lookup + ContainerInspect + State.Running) into a new exported IsBackendRunning, shared by all four engine plugins' usingDockerBackend instead of an identical function pasted into each docker.go — the State.Running hardening from the previous sweep round had to be hand-copied into all four files; the next fix to this logic won't. RemoveIfExists never received the container-vanish-race tolerance its sibling UpOrReuse got in an earlier round, despite being documented as serving the identical idempotency contract — a future caller wiring this exported, unused-in-production helper into a new call site would silently reintroduce the exact race UpOrReuse was already patched for. Added the same errdefs.IsNotFound tolerance. Also: TestIsPortFree_BindString never called IsPortFree, so it could not catch the dual-stack regression its own comment claimed to guard against — the existing TestIsPortFree_OccupiedPort already covers that exact scenario correctly, so the tautological test is removed and its intent folded into that comment instead. labels.go's doc comment claimed `bough remove` discovers containers by listing on these labels; no code path does (every plugin discovers by exact name) — corrected to describe the labels as currently write-only. The test package's own testLabel constant was declared but never referenced (createSleepContainer hardcoded an equivalent literal instead). Surfaced by retrospective review of merged PR #9 (ikeikeikeike/bough).
dockerUp's extras-to-env passthrough loop excluded only "docker."- prefixed/"version"/"backend" keys, not "database" or "allow_empty_password" — both already have a hardcoded MYSQL_* entry earlier in the same Env slice. An extras key colliding with either appended a second, conflicting entry for the same env name instead of being dropped; the official mysql image's entrypoint behavior on a duplicate env name is undefined, and bough's own bookkeeping (initDB) would disagree with whatever the container actually initialized. Extracted the env-building logic into a standalone buildDockerEnv (mirroring elasticsearch's existing pattern) so the collision has a regression test without needing a running Docker daemon. Also centralized usingDockerBackend on dockerutil.IsBackendRunning and removed a dead doc comment for a dockerCleanup function deleted by a prior commit. Surfaced by retrospective review of merged PRs #9/#10 (ikeikeikeike/bough).
…nning Same duplication as mysql: each plugin's usingDockerBackend re- implemented the identical client-connect + lookup + running-state check byte-for-byte. Now a thin wrapper around the shared dockerutil.IsBackendRunning. Surfaced by retrospective review of merged PR #9 (ikeikeikeike/bough).
…ered kinds computeEngineBackends only checked eng.Backend, never the equally- authoritative eng.Extras["backend"] override create.go's buildEngineExtras treats the same way — an engine pinned via extras.backend showed as auto-detected/unresolved in `bough status` even though it was correctly running under the declared backend. Also: the Detect() call used its own disconnected 3s context.WithTimeout(context.Background(), ...) instead of the command's cmd.Context() — the only bough subcommand that ignored Ctrl+C/SIGTERM during a slow probe, and a shorter deadline than create's effective ~5s window (Detect's own detectTimeout), causing status to report "unresolved" for an engine create had just proven healthy on a merely-slow (not down) nix store. Now passes cmd.Context() straight through and lets Detect's own internal cap apply, matching create.go's detectBackendIfNeeded exactly. Also: the detection call fired unconditionally before the registry was even inspected, paying probe latency on every invocation regardless of whether any registry row needed it. Now scoped to engine kinds the registry actually has a row for. Also: a failed Detect() surfaced only as the bare "unresolved" string with no diagnostic; now includes the error. The doc comment on statusEntry.Backend now states plainly that the auto-detect case is a single host-level guess made at status time, not a per-worktree readback of what that instance's `bough create` actually chose (no per-worktree backend choice is persisted anywhere) — see issue #78 for the underlying architecture gap. Surfaced by retrospective review of merged PR #12 (ikeikeikeike/bough).
…elp text overclaim newConfigValidateCmd's no-args path discarded loadConfigAndRoot's real error (missing file, malformed YAML, failed schema validation) and always reported a generic, actively-wrong "path argument missing" message instead — the path DID resolve; only loading it failed. This same defect was independently reported against merged PR #1 in an earlier sweep round and deferred to the cleanup-backlog issue; it resurfaced independently in PR #18's review, so fixing it directly now rather than deferring again. Also: the root --help text claimed bough "spawns the configured engine (MySQL / PostgreSQL / Redis / Elasticsearch / rabbitmq / kafka / NATS / ...) via a Hashicorp go-plugin gRPC plugin" — no rabbitmq/kafka/NATS plugin binary exists anywhere in the repo and the port allocator is hard-coded to single-port "main" allocation, so following the documented example fails immediately. README.md's own Status section already states this accurately ("reference plugins are not yet bundled"); the CLI help text now matches it. Also: the "v0.3 .worktree-isolation.yaml accepted on fallback" caption was duplicated verbatim between the root --config flag and `bough config validate`'s Short text; extracted to a shared constant. Surfaced by retrospective review of merged PR #18 (ikeikeikeike/bough).
… README contradiction PR #19 rewrote the e2e mysql fixture from v0.3 (databases:/port_range:/ initial_databases:/role: db-provider) to the v0.4 canonical shape, leaving the still-supported legacy schema with zero end-to-end coverage — neither the general create->registry->plugin->remove pipeline for a migrateLegacy()-produced Engine, nor (more concretely) engineProviderRepo()'s "db-provider" role-alias branch, which no other test anywhere in the repo exercises. A regression in either would stay green in CI and only surface for a real user still on .worktree-isolation.yaml, e.g. as a silently-wrong worktree path or a leftover datadir on `bough remove`. Restored as a second end-to-end test sharing the existing fixture helper (refactored to take the YAML as a parameter) rather than a mere unit test on the in-memory Config struct, so a regression in how the real CLI/plugin pipeline consumes a migrateLegacy() output is actually caught. Verified locally: both the v0.4 and the new v0.3 test pass end-to-end against a real nix-launched mysqld. Also fixed README's Status section, which named Postgres as one of "the four bundled engine plugins" that "are battle-tested... in a real... monorepo" one sentence before stating "The Postgres plugin is integration-test-only" — directly contradicting itself. Scoped the battle-tested claim to the three engines that actually earned it. Surfaced by retrospective review of merged PR #19 (ikeikeikeike/bough).
README's recommended install one-liner built the release asset name
from raw `uname -s`/`uname -m` (e.g. bough_linux_x86_64.tar.gz), but
GoReleaser publishes assets keyed by GOARCH (bough_linux_amd64.tar.gz)
— a 404 on every platform except Apple Silicon macOS, where `uname -m`
(arm64) happens to already match GOARCH. The PR that introduced the
"resolves the latest tagged release automatically" wording also
dropped the prior "replace the URL" hedge that gave a stuck user
something to try. Added an explicit uname->GOARCH mapping so the
command actually works on all four published platform/arch combos.
Also, since deleting plugins/db/api/CONTRACT.md and naming
plugins/engine/api/CONTRACT.md as its complete successor:
- Added the two invariants that were missing from the successor
contract despite each having caused a real, previously-fixed bug in
the plugins this contract governs: backend self-detection must
check Running state, not mere existence (the exists-vs-running bug
that risked deleting a live engine's datadir), and a detached
subprocess launched from Up() must use a context decoupled from the
RPC's own (the context-cancellation bug that silently killed nix
subprocesses immediately after Up() returned).
- Fixed plugins/engine/api/interface.go's package comment, which still
claimed plugins/db/api and its legacy DBProvider fallback handshake
exist for "the v0.4.x transition" — both were fully removed in
v0.5.0, which has already shipped.
- Fixed docs/MIGRATION-v0.3-to-v0.4.md's TL;DR, which promised "v0.4.0
keeps working without edits" and framed v0.5.0 removal as a future
event ("before v0.5.0 ships") — v0.5.0 already shipped and already
removed that compat path; a reader on this guide today would
conclude no action is needed when the opposite is true.
- Fixed README's nix-backend routing summary, which dropped the
"with flakes enabled" qualifier both the numbered auto-detect list
and the actual hasNixWithFlakes() check require.
Surfaced by retrospective review of merged PR #20 (ikeikeikeike/bough).
This was referenced Jul 16, 2026
ikeikeikeike
commented
Jul 16, 2026
| **v0.4.0 keeps working without edits**. The host loader detects the | ||
| old file and reads it with a deprecation warning. Rename to the new | ||
| shape at your convenience before v0.5.0 ships. | ||
| **v0.5.0 already shipped and removed the v0.3.x fallback described |
Member
Author
There was a problem hiding this comment.
Retrospective re-review of PR #85 itself (this PR was the retro-review for #9/#10/#11/#12/#18/#19/#20, but had never been reviewed itself).
This TL;DR rewrite is factually wrong, and contradicted by this very PR's own commit 18e0d6c ("fix(tests): restore E2E coverage for the v0.3 legacy schema path"), which restores a test explicitly because the v0.3 legacy schema is "the still-supported legacy schema".
Only the plugin gRPC handshake (DBProvider/BOUGH_DB_PLUGIN, internal/pluginhost) was actually removed in v0.5.0 — confirmed by the v0.5.0 CHANGELOG entry, which lists only that as breaking. The YAML-level fallback (.worktree-isolation.yaml file name, databases:/port_range:/initial_databases: section/field names, schema_version: 1) was never removed: internal/config.migrateLegacy() still runs at current HEAD (v0.15.0), internal/cli/helpers.go's resolveConfigPath still falls back to the old file name, and CHANGELOG.md shows LegacyConfig being extended release over release (v0.6.x, v0.6.1) rather than retired.
Filed as #120 (fixes this file's TL;DR + Timeline, plus the matching over-broad "removed in v0.5.0" claim in README's roadmap table for the same v0.4.0 row).
ikeikeikeike
added a commit
that referenced
this pull request
Jul 16, 2026
fix: retrospective review fixes for merged PR #85
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.
Retrospective
/code-reviewsweep of merged PRs that shipped without pre-merge review. Wave 3 of a staged sweep (follows #84); wave 4 (#27/#30/#38) to follow in a separate PR.Highlights: the README install one-liner was broken on 3 of 4 published platform/arch combos (uname-vs-GOARCH mismatch); PR #19's own docs refresh had silently dropped the repo's only end-to-end coverage of the still-supported v0.3 legacy schema path, now restored;
bough status's backend column ignoredextras.backendand used a disconnected timeout/context; mysql's docker backend could silently duplicate MYSQL_DATABASE/MYSQL_ALLOW_EMPTY_PASSWORD env entries when an operator's extras collided with the hardcoded defaults;usingDockerBackendwas duplicated verbatim across all 4 engine plugins, now centralized in pkg/dockerutil.Design-level gaps this sweep surfaced but did not fix in-place (added as comments to existing issues rather than new ones): #78 (bough status has the same per-worktree backend-detection gap as usingDockerBackend), #82 (large dockerUp functions, cli.Close() duplication, config-filename string duplication).
Each commit is scoped to one plugin/area with its own rationale in the message. Full findings/fix breakdown: see commit log.