test(keys): cover keys/pull, keys/status, keys/discovery + recipe CMD-SHELL contract check (#404) - #528
Conversation
… recipe CMD-SHELL contract check - tests/test_keys_pull.bats: covers keys_pull dispatch (--from vps/containers/ env-file/unknown), keys_pull_from_vps (dry-run masking, chmod 600, missing env file, VPS_HOST check, --force/confirm, --keys filter, connection failure, JSON output), keys_pull_from_containers (dry-run, chmod 600, error paths), keys_pull_from_env_file (copy, chmod 600, dry-run, missing source, fallback to stack-specific env file, --force), and keys_pull_help. - tests/test_keys_status.bats: covers keys_status --json output shape (ssh_keys/api_keys/vps_status/env_vars fields and values from fixtures), keys_status text output, keys_recent --limit slicing and edge cases, discover_local_keys (env file detection, template_secrets count), and generate_recommendations (SSH rotation, GitHub review, large-secret flag). - tests/test_recipes.bats: adds contract check (d) — any healthcheck.test that contains a command substitution ($(...) / $$(...)) inside an exec-form CMD array must use CMD-SHELL instead, so the shell actually evaluates it. - templates/recipes/ghost/docker-compose.yml: fixes the mysql healthcheck from exec-form CMD (never evaluates $$(cat ...)) to CMD-SHELL, the form the check now enforces. The $$(cat ...) idiom reads the root password from a file — it was silently broken before this fix. No production lib/ changes; no CI workflow edits needed (bats tests/ is already auto-discovered). lib/migrate/phase-*.sh coverage (~2 100 lines) is tracked as a separate follow-up; flagged in PR description. Closes #404
- test_keys_pull.bats: make ssh stub command-aware so 'test -f' and other probe calls don't emit KEY=value lines; dry-run output no longer contains unmasked values (test 1462) - test_keys_status.bats: save $output into local json_out before subsequent 'run jq' calls overwrite it; each run jq now receives the original keys_status JSON, not the previous jq result (test 1525) - test_keys_status.bats: delete key-audit.log before testing the no-log-exists path; ensure_keys_dir in setup() creates it via touch so the test must remove it first (test 1534) - lib/keys/discovery.sh: guard empty recommendations array in generate_recommendations; printf '%s\n' on an empty array emits a blank line which jq -s . converts to [""] instead of [] (test 1546)
There was a problem hiding this comment.
🔎 Agent review (kiro · sonnet→opus) — CONCERNS
REVIEW: CONCERNS
RESOLVES: partial
The PR adds sound, genuine coverage for keys/pull, keys/status, keys/discovery, the recipe check-d lint, and a correct ghost healthcheck fix; the generate_recommendations empty-array guard is a real fix (also avoids a set -u unbound-array error). The only substantive issue is that both new test files root CLI_ROOT at the real repo tree instead of a temp fixture; the migrate-phase coverage from the issue is deferred to a follow-up.
1 concern — 1 inline on the diff
Point CLI_ROOT at TEST_TMP instead of the real repo tree so stacks/ and .prod.env fixtures live in the temp fixture and cannot leak gitignored artifacts on interrupted runs; source lib/ from REPO_ROOT and simplify teardown to rm -rf TEST_TMP.
There was a problem hiding this comment.
🔎 Agent re-review (muse · sonnet, delta) — LGTM
REVIEW: LGTM
RESOLVES: full
Revise cleanly fixes the prior fixture-isolation concern and 4 failing assertions with no regressions; ghost CMD-SHELL and empty-array fixes are correct and minimal.
What
Closes two genuinely-uncovered gaps from the strut#404 audit:
lib/keys/pull.sh— zero coverage →tests/test_keys_pull.bats(343 lines)keys_pulldispatch:--from vps,containers,env-file, unknown source, default (vps)keys_pull_from_vps: dry-run value masking,chmod 600enforcement, missing.prod.env, missingVPS_HOST,--force/confirminterplay,--keysfilter, connection failure, JSON output validitykeys_pull_from_containers: dry-run,chmod 600, error pathskeys_pull_from_env_file: copy +chmod 600, dry-run, missing source, fallback to stack-specific env file,--forcekeys_pull_help: usage textlib/keys/status.sh+lib/keys/discovery.sh— zero coverage →tests/test_keys_status.bats(377 lines)keys_status --json: shape (ssh_keys/api_keys/vps_status/env_vars), counts from fixture JSON, vps_status transitions (unknown/unreachable/connected)keys_statustext: exits 0, non-empty, mentions SSH Keyskeys_recent: missing-log → non-zero,--limitslicing,--limit=Nformdiscover_local_keys: returns valid JSON, detects.prod.env,template_secretscountgenerate_recommendations: array output, SSH rotation recommendation, GitHub review recommendation, large-secret-count flag, empty-input → empty arrayRecipe-contract check (d) → added to
tests/test_recipes.batsAny
healthcheck.testline containing$()or$$()inside an exec-form["CMD", ...]array must use["CMD-SHELL", ...]instead. Exec-form CMD never runs a shell, so command substitution silently never evaluates at runtime.Ghost recipe healthcheck bug fix →
templates/recipes/ghost/docker-compose.ymlThe mysql healthcheck used
["CMD", "mysqladmin", "ping", ..., "-p$$(cat /etc/mysql/conf.d/passwd ...)"]. The$$(cat ...)was never evaluated (exec-form, no shell), so the healthcheck always passed-pwith a literal dollar-cat string. Fixed to["CMD-SHELL", "mysqladmin ping ... -p$$(cat ...)"]so the substitution actually runs. This is the exact bug the contract lint was designed to catch.Why
Closes #404
Plane: OSS-783
Note on scope:
lib/migrate/phase-*.sh(~2,100 lines across 7 phase files) is intentionally excluded. It's substantially larger than the combined coverage here and warrants its own dedicated PR rather than bloating this one. The item is still open in the tracker for a follow-up.How
.batsfiles auto-discovered bybats tests/in CI (test.yml) — no workflow edits neededvalidate_vps_connection,ssh,build_ssh_opts,resolve_deploy_dir) — no live host requiredcommand -v jqandskipcleanly when jq is absent (CI has jq; parity with existingtest_recipes.bats)lib/changes → shellcheck risk is zero for library coderecipes.ymlCI only runsdocker compose config, which parsesCMD-SHELLfineTesting
bash -non new.batsfiles — same result as all existing.batsfiles (BATS@testsyntax isn't parsed bybash -n; expected)templates/recipes/*/docker-compose.yml— zero false positives, ghost correctly skipped after theCMD-SHELLfixCMD-SHELLline is now hit by thegrep -qE '"CMD-SHELL"'guard and skipped, not flaggedCI: pending
🤖 Generated by the harbor agent loop. Reviewed by a human before merge.
Closes #404