fix: retrospective review fixes for merged PRs #1/#2/#3/#4/#5/#6/#7/#8/#14/#16/#17#84
Merged
Conversation
…LUGIN_LOG Retrospective /review of #16 (conformance suite, w62ukm4ju) flagged two cheap items among its unfixed findings: - AssertShellSafe only checked `()&;<>|`$` — missing whitespace, quotes, and backslash, all of which corrupt a plain `KEY=value` .env.local line the same way the existing v0.2.5 guard chars do. Extended the char class and added a regression test per character class. - BOUGH_PLUGIN_LOG was a duplicated string literal in pluginhost.go and create.go. Promoted to pluginhost.EnvPluginLog and reused. The other #16 findings (up-or-reuse loop gap, extractDialableAddrs empty-port drop, missing postgres NativeProbe, universal SkipDatadirPermission skip, ES chmod hard-fail edge case) are design-level or cross-plugin-blast-radius — filed as issues #70-#74 instead of forced here.
…ckend Retrospective /review of PR #8 (nix/docker auto-detect) found 6 confirmed issues, all fixed here: - hasDocker() shelled out to `docker info`, but every docker-backend plugin actually connects via pkg/dockerutil's Docker SDK client. The CLI resolves its endpoint from the active `docker context`, which can silently diverge from the SDK's client.FromEnv resolution (e.g. Colima switches context without exporting DOCKER_HOST) — causing both false positives (probe passes, plugin Up fails minutes later) and false negatives (SDK-reachable daemon misreported as absent because the `docker` CLI binary isn't installed). Switched hasDocker to probe via the same dockerutil.NewClient + Ping path plugins use. - buildEngineExtras() let the once-per-invocation auto-detected backend silently overwrite a `backend` key an operator set directly through the free-form `extras:` map, even though extras are documented as copied verbatim. Auto-detect now only fills a genuine gap. - Detect()'s two probes ran sequentially off one shared context deadline, so a caller with a tight external timeout (bough status caps at 3s) could have the docker probe starved by a slow nix probe, and worst-case latency was the sum of both timeouts rather than the max. Both probes now run concurrently. - A backend-detection failure (before any plugin subprocess exists) was wrapped with a "re-run with BOUGH_PLUGIN_LOG=debug" hint that can never produce extra output for that failure mode. Skipped for errors.Is(err, backend.ErrNoBackend). - The backend auto-detect call had no spinner, unlike every other multi-second step in `bough create`. detect_test.go's TestHasDocker_pathPresence assumed the docker CLI's presence on PATH gates the probe; replaced with a DOCKER_HOST-based test matching the new SDK-only contract. go build/test (incl. conformance) + golangci-lint (0 issues) + gofumpt green.
…e env_local failed Retrospective /review of PR #14 (post_create .env.local env injection) found 8 confirmed findings; fixed here: - fileEnv was parsed once per repo before the post_create loop, so a line that derived and appended a new value (the documented derive-then-consume idiom, e.g. composing a DSN from a bough- allocated port) was invisible to every later line in that repo's post_create list — reintroducing the "expands to empty" failure class this PR was written to eliminate, just moved from bash `source` into bough's own line loop. Now re-read before every line. - skipRepo was built only from materializeRepositories' worktree-add failures; a repo whose env_local render/write failed still had post_create run against it with an absent-or-stale .env.local, with no link back to the earlier failure. renderEnvLocals' failures now feed into skipRepo too. Its return type changed from []string (pre-formatted "repo (detail)" strings, unusable as skip keys) to a small envLocalFailure{Repo, Detail} struct. - parseEnvLocal collapsed every os.ReadFile error into a silent nil, indistinguishable from the legitimate "no .env.local yet" case. Non-ErrNotExist failures are now logged with the repo name. Two findings were investigated and NOT fixed as suggested: - Multi-line value truncation and quote-stripping both trace back to .env.local's wire format having no quoting at all — a parseEnvLocal- only fix would guess at a convention the writer (envwriter.Write) never established, and blind quote-stripping risks corrupting a value that genuinely needs literal quote characters. Filed as issue #75 (needs a coordinated read+write format decision). - Quote-stripping specifically would also contradict this PR's own tested contract (TestParseEnvLocal_ValuesWithShellMetachars asserts quoted values survive unchanged) — a deliberate design choice, not an oversight, per parseEnvLocal's doc comment. post_create_env_test.go (all 6 of PR #14's original regression tests, including the load-bearing shell-metachar guard) was wholesale deleted by a later v0.9 reset commit despite that commit's own message listing this logic as kept. Recreated with the original 6 tests plus 3 new ones: a read-error-vs-missing-file distinction test, and two runPostCreateHooks-level regression tests (later line sees an earlier line's append; skip map actually suppresses post_create). go build/test (incl. conformance) + golangci-lint (0 issues) + gofumpt green.
…scope, migrateLegacy overwrite Retrospective /review of PR #17 (v0.4.0 DB plugin orchestrator -> engine plugin orchestrator rename, largest diff in the sweep) found 9 confirmed findings; 8 fixed here, 1 filed as issue #76 (real feature gap, not a bundleable bug fix). - verify's non-engine port check looked up the bare registry key (e.g. "api"), but create.go now writes "<kind>.main" and Load() upgrades every legacy bare key to that same dotted form — the bare key never exists post-rename, so `bough verify` reported permanent false-positive DRIFT on any config with a non-empty `ports:` section (the shipped example.yaml's api/gateway shape included). - verify's engine check demanded a registry entry for EVERY role in eng.PortRanges, but allocateEngines only ever allocates/writes "main" (v0.4.0: single-port engines only). Any engine declaring more than one port_ranges role hit the same permanent false-positive DRIFT. Narrowed to "main" only, matching what create.go actually writes; filed the underlying "multi-port can't really be provisioned" gap as issue #76. - verify/status/list/backfill built their registry Store straight from cfg.Registry.Path, while create/remove prefer the canonical .bough-ports.json via this PR's own resolveRegistryPath() helper — the two command families could read different registry files during the v0.4.x transition (docs/MIGRATION-v0.3-to-v0.4.md explicitly says leaving the YAML field stale is fine "at your convenience"). All four now go through resolveRegistryPath too. - conformance/mock_plugin's Up() rollback-on-sentinel-write-failure loop closed every listener in req.Ports, not just the ones bound during that call — a role reused from a prior successful Up (up-or-reuse) got torn down as a side effect of an unrelated later failure. This is the in-tree reference plugin future rabbitmq/kafka/ nats plugins would copy the same regression from. - config.migrateLegacy unconditionally overwrote c.Engines (already populated from an engines: section) with the databases: conversion whenever legacy databases: was non-empty, silently discarding any engines: entries declared in the same file — breaking the incremental v0.3->v0.4 migration story schema_version: 1 exists for. Now appends instead of replacing. Cleanup: - Makefile's `proto` target still regenerated a legacy db.proto path deleted from the repo (plugins/db/), so `make proto` always failed; removed the dead line + its dir var. - config.Load's doc comment referenced a nonexistent LoadFromPath function; corrected to point at the real discovery logic (internal/cli/helpers.go::resolveConfigPath). - plugins/engine/api/handshake.go's LegacyHandshake / LegacyDBProviderPluginKey were dead exported symbols pointing at a deleted plugins/db/api path (pluginhost.DiscoverFromBinary never tries them) — removed. Tests added: verify_test.go (new file — verify.go had none), 3 cases covering the dotted-key / main-only / canonical-registry-path fixes; a config_test.go case for the engines:+databases: merge; a mock_plugin main_test.go case for the scoped rollback. go build/test (incl. conformance) + golangci-lint (0 issues) + gofumpt green.
…h race, port-conflict detection Retrospective /review of PR #7 (v0.2.1: hardening - resume idempotency + orphan cleanup + port collision) found 11 confirmed findings; 5 fixed here, 1 filed as issue #77 (real design gap, not a bundleable fix), the rest are stale-doc/test-coverage cleanup folded into the same commit or already tracked. - AddOrAttach's resume-idempotency check compared the caller's literal dst against git's symlink-resolved `worktree list --porcelain` path with a raw string ==, so on any host where dst traverses a symlink (stock macOS /tmp -> /private/tmp, /var -> /private/var - exactly what Go's own os.Getwd()/t.TempDir() return unresolved) the documented "second call is a no-op" contract silently broke: every `claude --worktree --resume` re-fired `git worktree add` against an already-existing dir and failed with exit 128. Resolve dst via filepath.EvalSymlinks before comparing (falls back to the literal path when dst doesn't exist yet, e.g. genuinely new worktrees). - dockerutil.UpOrReuse's ContainerRemove call (for a container LookupByName found stopped) propagated ANY error verbatim, turning the benign race where a concurrent actor removes the container between LookupByName and this call (another `bough create` retry for the same worktree - the exact retry-heavy pattern Up's callers are built around - a parallel `bough remove`, or a manual `docker rm`) into a hard Up failure, even though "nothing there" is exactly what the id == "" branch one line earlier already treats as success. Now tolerates errdefs.IsNotFound. - The ContainerStart-failure cleanup + "port is already allocated" rewrap block was duplicated verbatim (engine name aside) across all 4 engine plugins' docker.go, and only matched one of the two real daemon error shapes for a taken host port - a plain host-level listener (not another Docker container) produces "...bind: address already in use", which the bare substring check missed, silently falling through to a generic non-actionable error for exactly the case the hint exists for. Consolidated into dockerutil.StartOrCleanup + isPortConflictError, called identically from all 4 plugins - this incidentally fixes both the missed error shape and the duplication finding at once. - mysql/docker.go's file-header doc comment still described the pre-PR7 "always remove and recreate" behavior; corrected to describe UpOrReuse. Deferred: the redis-password / mysql-image / es-heap "config drift on resume" family (PR7's UpOrReuse skip-check runs before any of the three's desired values are computed, so a resumed container never picks them up) is a real design gap spanning all 4 plugins, not a bundleable fix - filed as issue #77 (redis password flagged as security-relevant). The conformance-suite false-green finding (IdempotentCount's loop always Downs, so UpOrReuse's reuse branch is never actually exercised by any test) restates already-filed issue #70 from the #16 retrospective sweep - no new issue. Tests added: gitwt_test.go's resume-through-a-real-symlink case; dockerutil container_test.go (new, no Docker needed) for isPortConflictError's two error shapes; a container_integration_test.go case pinning the errdefs.IsNotFound contract UpOrReuse's race tolerance depends on. go build/vet(-tags integration)/test (incl. conformance) + golangci-lint (0 issues) + gofumpt green.
…tray-process match Up() launched the detached `nix run -- up` process with the per-RPC gRPC context; grpc-go cancels that context the instant Up() returns, and Go's exec.CommandContext watchdog then SIGKILLs the still-starting nix process regardless of Setsid, well before flake evaluation finishes. Reproduced directly with a minimal grpc unary handler + exec.CommandContext: the child was reaped ~6ms after the RPC returned. Switched to context.WithoutCancel(ctx) plus a reaper goroutine so Down()'s existing lsof-based teardown remains the only thing that stops the process. Also anchored killStrayProcessCompose's cwd prefix match to a real path separator boundary — a bare HasPrefix let tearing down one worktree SIGTERM a sibling whose name is a literal prefix (e.g. auba-api-139 vs auba-api-1394). Surfaced by retrospective review of merged PRs #2/#3/#4/#6 (ikeikeikeike/bough).
…chor stray match Same context-cancellation fix as mysql/redis/elasticsearch: Up() used the per-RPC gRPC context for the detached `nix run -- up` process, so grpc-go's cancellation on return let Go's exec watchdog SIGKILL it before flake evaluation finished. Switched to context.WithoutCancel(ctx) + a reaper goroutine. Also: Up() was pre-creating req.Datadir itself before invoking nix, which defeats services-flake's postgres init-detection (`[[ ! -d "$PGDATA" ]]`) — initdb never ran against the already-existing empty directory, so the nix backend could never actually bring up postgres on a fresh worktree. Now only the parent directory is created, matching the postgres-specific requirement (mysql's own init-detection tolerates a pre-created empty dir via a marker-file check, postgres's does not). Anchored killStrayProcessCompose's cwd prefix match to a path-separator boundary, same as the other three plugins. Surfaced by retrospective review of merged PRs #2/#4 (ikeikeikeike/bough).
…tray-process match Same fix as mysql/postgres/elasticsearch: Up() used the per-RPC gRPC context for the detached `nix run -- up` process, so grpc-go's cancellation on return let Go's exec watchdog SIGKILL it before flake evaluation finished. Switched to context.WithoutCancel(ctx) + a reaper goroutine. Anchored killStrayProcessCompose's cwd prefix match to a path-separator boundary, same as the other three plugins. Surfaced by retrospective review of merged PRs #3/#4 (ikeikeikeike/bough).
…ept both heap keys Same context-cancellation fix as mysql/postgres/redis: Up() used the per-RPC gRPC context for the detached `nix run -- up` process, so grpc-go's cancellation on return let Go's exec watchdog SIGKILL it before flake evaluation finished. Switched to context.WithoutCancel(ctx) + a reaper goroutine. Anchored killStrayProcessCompose's cwd prefix match to a path-separator boundary, same as the other three plugins. Also: the docker backend's pickHeap() only read extras["es.heap"], while the nix backend's Up() has always read extras["heap"] for the identical JVM heap-size setting — a value tuned on one backend silently stopped mattering after switching to the other. pickHeap now accepts both keys. Surfaced by retrospective review of merged PRs #4/#6 (ikeikeikeike/bough).
…ngine kinds in env_local
runCreate started engine plugins (which deploy their flake into
<worktreeRoot>/<engine-provider-repo-name>/...) before running `git
worktree add` for that same repo path. `git worktree add` fails outright
against a non-empty destination, so worktree materialization for any
role: engine-provider repository always failed — the operator ended up
inside a worktree where the DB-provider repo was never actually checked
out, with mysqld/etc. running against no schema. Swapped the phase order:
materialize repos first, then start engines.
Also: renderEnvLocals always built its env_local template context via
engineContextFor("mysql", engines), so envwriter.Context.Mysql was the
only field ever populated — any worktree whose only engine was postgres,
redis, or elasticsearch got a zero-value Mysql context (Port 0) and no
way to reference its own engine's connection info at all. Extended
envwriter.Context with Postgres/Redis/Elasticsearch DBCtx fields
(mirroring the existing Mysql field) and populate all four from the
configured engines.
Surfaced by retrospective review of merged PR #1 (ikeikeikeike/bough).
DeleteBranch treated every git exec.ExitError as the idempotent "no such branch" case. `git branch -D` also exits 1 when the branch is checked out in another worktree, so a genuine failure (branch survives, no worktree touched) was silently reported as success with zero diagnostic trail. Now inspects the command's combined output and only swallows the actual "not found" message; anything else bubbles up. Surfaced by retrospective review of merged PR #1 (ikeikeikeike/bough).
…ut_sec The canonical example.yaml's env_local templates referenced .Ports.Api/.Ports.Gateway (capitalized), but Ports is a map populated only with the lowercase keys from the same file's own ports: section, and Render parses with missingkey=error. Copying this documented example verbatim into a real config made `bough create` abort with "map has no entry for key \"Api\"" — fixed to reference the actual lowercase keys. Also: ReadyTimeoutSec's validator only enforced a lower bound (min=1). The value crosses the plugin RPC wire as an int32; an operator-set value at or above 2^31 would silently wrap negative there and every plugin's own timeoutSec<=0 guard would substitute its internal default with no warning anywhere in the chain. Added max=86400 (24h, well under the int32-safe range and far past any sane ready-check budget). Surfaced by retrospective review of merged PRs #1/#5 (ikeikeikeike/bough).
… engine plugin defaultRemoveGracefulSecs=10 backed both `bough remove`'s --graceful-timeout default and the flagless WorktreeRemove hook dispatch. Every engine's Down() only falls back to its own tuned budget (30s for mysql's InnoDB recovery window, 60s for elasticsearch's translog flush) when GracefulTimeoutSec<=0 — since the host always sent 10 (>0), every engine's own budget was dead code on every single `bough remove`, silently SIGTERMing databases with a fifth to a sixth of their intended grace period. Changed the default to 0 so "operator didn't ask for anything specific" actually means that; an explicit --graceful-timeout is still honored as before. Surfaced by retrospective review of merged PR #6 (ikeikeikeike/bough).
…ping wave1+2 review fixes # Conflicts: # internal/cli/create.go
This was referenced Jul 4, 2026
6 tasks
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 (see repo history — most foundational PRs predate the review workflow). Wave 1 + wave 2 of a staged sweep; wave 3/4 (remaining PRs) to follow in a separate PR.Highlights: gRPC context cancellation was killing every nix-backend engine's subprocess seconds after start (mysql/postgres/redis/elasticsearch);
bough createalways failed to materialize anengine-provider-role repo's worktree;usingDockerBackendcould misdetect backend and letCleanup()delete a live engine's datadir;bough remove's hardcoded graceful-timeout default silently overrode every engine's own tuned shutdown budget.Fixes #41.
Design-level gaps this sweep surfaced but did not fix in-place (need maintainer sign-off): #78, #79, #80, #81, #82, #83.
Each commit is scoped to one plugin/area with its own rationale in the message. Full findings/fix/issue breakdown: see commit log.