Surfaced by the wave-2 retrospective /code-review sweep of PR #5 (which introduced ready_timeout_sec). The int32-truncation risk this PR introduced was fixed directly in this sweep (internal/config/config.go's ReadyTimeoutSec now has validate:"omitempty,min=1,max=86400"). Three related gaps remain that need a design decision rather than a same-file patch:
1. The "0 means use the plugin's 600s default" contract lives only in comments, independently in 4 binaries
internal/config/config.go's doc comment and the README both describe "0 → 600s", but that fallback is re-implemented as a bare if timeoutSec <= 0 { timeoutSec = 600 } independently inside each of plugins/engine/{mysql,postgres,redis,elasticsearch}/*.go's ReadyCheck() — it is not part of the versioned proto contract. Since this project explicitly supports installing the host and each bough-plugin-<kind> binary independently (README's go install .../bough-plugin-mysql@latest etc.), a host built after this default was raised to 600s paired with an older plugin binary still on 120s silently disagrees about what "0" means, with no version-mismatch warning anywhere.
2. The 600s fallback is a duplicated bare literal, not a shared constant
All four plugins independently hardcode 600. A later change that touches only one plugin's fallback (or a copy-paste into a 5th plugin) can silently make identical omitted-ready_timeout_sec configs behave differently per engine kind, with no compiler error or failing test.
3. No test exercises the actual runtime fallback
config_test.go only tests that config parsing rejects an out-of-range value; no test in any of plugins/engine/*/*_test.go calls ReadyCheck(ctx, port, 0) and asserts it resolves to 600s. A future refactor that consolidates the 4 duplicated blocks and mistypes one as 60 would pass the full suite while silently reintroducing slow-cold-start timeouts.
(A fourth, lower-confidence finding: internal/cli/create.go's (ready=false, err=nil) branch formats its timeout error from the raw, possibly-zero eng.ReadyTimeoutSec rather than the plugin's actual effective timeout, so a future non-bundled plugin returning (false, nil) on genuine timeout would render a nonsensical "not ready within 0ds". Currently dormant since all 4 bundled plugins return a non-nil error on timeout instead.)
Proper fix: move the default to a single named constant shared via the plugins/engine/api package (or carry it in the proto contract itself so host and plugin never disagree), and add a runtime regression test per plugin.
Files
internal/config/config.go (doc comment)plugins/engine/{mysql,postgres,redis,elasticsearch}/*.go (ReadyCheck)internal/cli/create.go (timeout error formatting)
Surfaced by the wave-2 retrospective
/code-reviewsweep of PR #5 (which introducedready_timeout_sec). The int32-truncation risk this PR introduced was fixed directly in this sweep (internal/config/config.go'sReadyTimeoutSecnow hasvalidate:"omitempty,min=1,max=86400"). Three related gaps remain that need a design decision rather than a same-file patch:1. The "0 means use the plugin's 600s default" contract lives only in comments, independently in 4 binaries
internal/config/config.go's doc comment and the README both describe "0 → 600s", but that fallback is re-implemented as a bareif timeoutSec <= 0 { timeoutSec = 600 }independently inside each ofplugins/engine/{mysql,postgres,redis,elasticsearch}/*.go'sReadyCheck()— it is not part of the versioned proto contract. Since this project explicitly supports installing the host and eachbough-plugin-<kind>binary independently (README'sgo install .../bough-plugin-mysql@latestetc.), a host built after this default was raised to 600s paired with an older plugin binary still on 120s silently disagrees about what "0" means, with no version-mismatch warning anywhere.2. The 600s fallback is a duplicated bare literal, not a shared constant
All four plugins independently hardcode
600. A later change that touches only one plugin's fallback (or a copy-paste into a 5th plugin) can silently make identical omitted-ready_timeout_secconfigs behave differently per engine kind, with no compiler error or failing test.3. No test exercises the actual runtime fallback
config_test.goonly tests that config parsing rejects an out-of-range value; no test in any ofplugins/engine/*/*_test.gocallsReadyCheck(ctx, port, 0)and asserts it resolves to 600s. A future refactor that consolidates the 4 duplicated blocks and mistypes one as60would pass the full suite while silently reintroducing slow-cold-start timeouts.(A fourth, lower-confidence finding:
internal/cli/create.go's(ready=false, err=nil)branch formats its timeout error from the raw, possibly-zeroeng.ReadyTimeoutSecrather than the plugin's actual effective timeout, so a future non-bundled plugin returning(false, nil)on genuine timeout would render a nonsensical "not ready within 0ds". Currently dormant since all 4 bundled plugins return a non-nil error on timeout instead.)Proper fix: move the default to a single named constant shared via the
plugins/engine/apipackage (or carry it in the proto contract itself so host and plugin never disagree), and add a runtime regression test per plugin.Files
internal/config/config.go(doc comment)plugins/engine/{mysql,postgres,redis,elasticsearch}/*.go(ReadyCheck)internal/cli/create.go(timeout error formatting)