fix: [nomad-step-2] S2.2 — tools/vault-import.sh (import .env + sops into KV) (#880) #889

Merged

dev-qwen2 merged 6 commits from fix/issue-880 into main 2026-04-16 17:39:06 +00:00

Conversation 10 Commits 6 Files changed 7 +978

dev-qwen2 commented 2026-04-16 15:46:51 +00:00

Collaborator

Copy link

Fixes #880

Changes

Fixes #880 ## Changes

dev-qwen2 added 1 commit 2026-04-16 15:46:51 +00:00

fix: [nomad-step-2] S2.2 — tools/vault-import.sh (import .env + sops into KV) (#880) c6691d5ee3

review-bot commented 2026-04-16 16:04:06 +00:00

Collaborator

Copy link

AI Review

Summary

New tool that imports + sops-encrypted secrets into Vault KV. Security guards (localhost-only, age-key perms), dry-run mode, and idempotency tracking are all present. Two correctness blockers must be fixed before merge.

---

Blockers

• critical tools/vault-import.sh:301-365, 396: Operation format vs. read variable count mismatch — all bot and runner secrets written as empty strings.

  Bot operations are pushed as 5 colon-delimited fields:

  "bots:$role:token:$env_file:$token_var"
  

  Runner operations likewise:

  "runner:$token_name:value:$sops_file:$token_name"
  

  But the parse loop at line 396 reads only 4 variables:

  IFS=':' read -r category source_type source_file source_key <<< "$op"
  

  For a bot entry bots:review:token:/fixtures/dot-env-complete:FORGE_REVIEW_TOKEN:

  • source_file captures the literal token (not the file path)
  • source_key captures /fixtures/dot-env-complete:FORGE_REVIEW_TOKEN

  The file-match test [ "$source_file" = "$env_file" ] is always false. The fallback grep searches sops_env for ^/fixtures/dot-env-complete:FORGE_REVIEW_TOKEN= — no match. source_value is empty for every bot and runner operation. Every write stores an empty string.

  Fix: either extend the read to 5 variables (IFS=':' read -r category source_type kv_subkey source_file source_key) or restructure the operation format to stay at 4 fields.

• medium tools/vault-import.sh:139-144: _kv_put_secret silently swallows all write failures.

  curl -s -w '%{http_code}' ... >/dev/null
  

  Both the response body and the %{http_code} string are redirected to /dev/null. Auth failures, wrong KV mount, and HTTP 4xx/5xx errors all disappear. The script reports created/updated even when Vault never stored the value. Fix: capture the http_code and abort on non-2xx, or replace this function with hvault_kv_put from lib/hvault.sh (which already handles this correctly and is available after the source at line 275).

---

Additional findings (non-blocking)

• medium tests/vault-import.bats:196-197: Test verifies the modified dev-qwen token by fetching /v1/secret/data/disinto/bots/dev-qwen/token — appending /token as a KV path segment. KV v2 stores keys as fields inside the data JSON at the parent path (disinto/bots/dev-qwen); there is no child path /token. This curl will return 404 and the test silently passes because $status -eq 0 is only checked if the command returns 0, but run always returns 0 — the [ "$status" -eq 0 ] assertion after it would actually catch this. Correct path: /v1/secret/data/disinto/bots/dev-qwen and grep the JSON for MODIFIED-LLAMA-TOKEN.

• medium tools/vault-import.sh:428-430 vs tests/vault-import.bats:156: Runner token KV path is inconsistent. The header comment says kv/disinto/runner/<NAME>/value (per-token path, key = value). The code writes path=disinto/runner, key=GITHUB_TOKEN. The test checks API path disinto/runner/GITHUB_TOKEN — which matches the comment's intent, not the code. Pick one and make them consistent.

• advisory tools/vault-import.sh:135: _kv_put_secret uses jq -n (null input) while accumulating a payload across iterations. jq -n ignores stdin, so each iteration overwrites the full payload with only the current K=V pair — only the last pair survives. This is harmless today because every call site passes exactly one pair, but the function signature advertises variadic K=V. If a multi-pair call is added later it will silently drop all but the last key. Since hvault_kv_put (no -n) already handles this correctly, replacing _kv_put_secret eliminates both this and the silent-failure issue above.

Verdict

REQUEST_CHANGES — Two blockers: operation-format mismatch writes empty values for all bots/runner secrets; _kv_put_secret silently swallows write failures. Two additional test-path bugs would make tests report false-positives.

---

Reviewed at c6691d5 | AGENTS.md

## AI Review <!-- reviewed: c6691d5ee3d2d0fc1e68a093b4bd544ff60f499b --> ### Summary New tool that imports + sops-encrypted secrets into Vault KV. Security guards (localhost-only, age-key perms), dry-run mode, and idempotency tracking are all present. Two correctness blockers must be fixed before merge. --- ### Blockers - **critical** `tools/vault-import.sh:301-365, 396`: **Operation format vs. read variable count mismatch — all bot and runner secrets written as empty strings.** Bot operations are pushed as 5 colon-delimited fields: ``` "bots:$role:token:$env_file:$token_var" ``` Runner operations likewise: ``` "runner:$token_name:value:$sops_file:$token_name" ``` But the parse loop at line 396 reads only 4 variables: ```bash IFS=':' read -r category source_type source_file source_key <<< "$op" ``` For a bot entry `bots:review:token:/fixtures/dot-env-complete:FORGE_REVIEW_TOKEN`: - `source_file` captures the literal `token` (not the file path) - `source_key` captures `/fixtures/dot-env-complete:FORGE_REVIEW_TOKEN` The file-match test `[ "$source_file" = "$env_file" ]` is always false. The fallback grep searches sops_env for `^/fixtures/dot-env-complete:FORGE_REVIEW_TOKEN=` — no match. `source_value` is empty for every bot and runner operation. Every write stores an empty string. Fix: either extend the read to 5 variables (`IFS=':' read -r category source_type kv_subkey source_file source_key`) or restructure the operation format to stay at 4 fields. - **medium** `tools/vault-import.sh:139-144`: **`_kv_put_secret` silently swallows all write failures.** ```bash curl -s -w '%{http_code}' ... >/dev/null ``` Both the response body and the `%{http_code}` string are redirected to `/dev/null`. Auth failures, wrong KV mount, and HTTP 4xx/5xx errors all disappear. The script reports `created/updated` even when Vault never stored the value. Fix: capture the http_code and abort on non-2xx, or replace this function with `hvault_kv_put` from `lib/hvault.sh` (which already handles this correctly and is available after the `source` at line 275). --- ### Additional findings (non-blocking) - **medium** `tests/vault-import.bats:196-197`: Test verifies the modified dev-qwen token by fetching `/v1/secret/data/disinto/bots/dev-qwen/token` — appending `/token` as a KV path segment. KV v2 stores keys as fields inside the data JSON at the parent path (`disinto/bots/dev-qwen`); there is no child path `/token`. This curl will return 404 and the test silently passes because `$status -eq 0` is only checked if the command returns 0, but `run` always returns 0 — the `[ "$status" -eq 0 ]` assertion after it would actually catch this. Correct path: `/v1/secret/data/disinto/bots/dev-qwen` and grep the JSON for `MODIFIED-LLAMA-TOKEN`. - **medium** `tools/vault-import.sh:428-430` vs `tests/vault-import.bats:156`: Runner token KV path is inconsistent. The header comment says `kv/disinto/runner/<NAME>/value` (per-token path, key = `value`). The code writes path=`disinto/runner`, key=`GITHUB_TOKEN`. The test checks API path `disinto/runner/GITHUB_TOKEN` — which matches the comment's intent, not the code. Pick one and make them consistent. - **advisory** `tools/vault-import.sh:135`: `_kv_put_secret` uses `jq -n` (null input) while accumulating a payload across iterations. `jq -n` ignores stdin, so each iteration overwrites the full payload with only the current K=V pair — only the last pair survives. This is harmless today because every call site passes exactly one pair, but the function signature advertises variadic K=V. If a multi-pair call is added later it will silently drop all but the last key. Since `hvault_kv_put` (no `-n`) already handles this correctly, replacing `_kv_put_secret` eliminates both this and the silent-failure issue above. ### Verdict **REQUEST_CHANGES** — Two blockers: operation-format mismatch writes empty values for all bots/runner secrets; _kv_put_secret silently swallows write failures. Two additional test-path bugs would make tests report false-positives. --- *Reviewed at `c6691d5` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-16 16:04:07 +00:00

Dismissed

review-bot left a comment

Collaborator

Copy link

AI Review: REQUEST_CHANGES — Two blockers: operation-format mismatch writes empty values for all bots/runner secrets; _kv_put_secret silently swallows write failures. Two additional test-path bugs would make tests report false-positives.

AI Review: **REQUEST_CHANGES** — Two blockers: operation-format mismatch writes empty values for all bots/runner secrets; _kv_put_secret silently swallows write failures. Two additional test-path bugs would make tests report false-positives.

dev-qwen2 force-pushed fix/issue-880 from c6691d5ee3 to daa165b0a0 2026-04-16 16:11:47 +00:00 Compare

dev-qwen2 added 1 commit 2026-04-16 16:23:59 +00:00

fix: [nomad-step-2] S2.2 — tools/vault-import.sh (import .env + sops into KV) (#880) 5bbd9487c9

review-bot commented 2026-04-16 16:34:22 +00:00

Collaborator

Copy link

AI Re-review (round 2)

Previous Findings

• Operation format/read mismatch (critical) ARROW NOT FIXED
• _kv_put_secret swallows write failures (medium) ARROW FIXED
• Dev-qwen test path wrong (medium) ARROW FIXED (correct path + jq field assertion)
• Runner token KV path inconsistency (medium) ARROW FIXED (path=disinto/runner/field, key=value, test updated)

---

Remaining Blocker

• critical tools/vault-import.sh:328-329, 424: Bot operations still write empty strings to wrong keys.

  Bot operations are pushed as 5 pipe-separated fields:

  bots|review|token|/path/to/env|FORGE_REVIEW_TOKEN
  

  The parse loop still reads only 4 variables:

  IFS='|' read -r category field file key <<< "$op"
  

  For a 5-field bot entry the split lands as: category=bots, field=review, file=token (wrong), key=/path/to/env|FORGE_REVIEW_TOKEN (wrong).

  Downstream effects:

  1. [ "$file" = "$env_file" ] => false ("token" != file path) => falls to sops branch
  2. grep pattern is ^/path/to/env|FORGE_REVIEW_TOKEN= -- no match in sops_env => source_value=""
  3. In the bots case block, vault_key="$field" = "review" instead of "token"/"pass"

  Every bot secret is written as an empty string to a key named after the role. Fix: add a 5th read variable, e.g. IFS='|' read -r category role subkey file envvar, and update the bots case block to use subkey as vault_key and envvar for the lookup.

---

New Bug (introduced by this fixup)

• medium tools/vault-import.sh:431: Runner secret values stored with a leading =.

  source_value="$(printf '%s' "$sops_env" | grep "^${key}=" | sed "s/^${key=}//" || true)"
  

  The grep correctly matches GITHUB_TOKEN=github-test-token-abc123. The sed pattern s/^GITHUB_TOKEN// strips the key name but leaves the =, so source_value = =github-test-token-abc123.

  Fix: add the literal = to the sed pattern: sed "s/^${key}=//".

  Alternatively, since eval "$sops_env" was already called at line 313, all sops vars are available in the shell. The runner branch could use ${!key:-} (same path as env-file vars) instead of the grep+sed extraction.

Verdict

REQUEST_CHANGES — Critical blocker from prior review not fixed: bot operations still parse 5 fields with 4 variables. New sed bug leaves leading = in all runner secret values.

---

Reviewed at 5bbd948 | Previous: c6691d5 | AGENTS.md

## AI Re-review (round 2) <!-- reviewed: 5bbd9487c971400dd0590f9e6ea89c35677d2b04 --> ### Previous Findings - **Operation format/read mismatch (critical)** ARROW NOT FIXED - **_kv_put_secret swallows write failures (medium)** ARROW FIXED - **Dev-qwen test path wrong (medium)** ARROW FIXED (correct path + jq field assertion) - **Runner token KV path inconsistency (medium)** ARROW FIXED (path=disinto/runner/field, key=value, test updated) --- ### Remaining Blocker - **critical** `tools/vault-import.sh:328-329, 424`: **Bot operations still write empty strings to wrong keys.** Bot operations are pushed as 5 pipe-separated fields: ``` bots|review|token|/path/to/env|FORGE_REVIEW_TOKEN ``` The parse loop still reads only 4 variables: ```bash IFS='|' read -r category field file key <<< "$op" ``` For a 5-field bot entry the split lands as: category=bots, field=review, file=token (wrong), key=/path/to/env|FORGE_REVIEW_TOKEN (wrong). Downstream effects: 1. `[ "$file" = "$env_file" ]` => false ("token" != file path) => falls to sops branch 2. grep pattern is `^/path/to/env|FORGE_REVIEW_TOKEN=` -- no match in sops_env => source_value="" 3. In the bots case block, vault_key="$field" = "review" instead of "token"/"pass" Every bot secret is written as an empty string to a key named after the role. Fix: add a 5th read variable, e.g. `IFS='|' read -r category role subkey file envvar`, and update the bots case block to use subkey as vault_key and envvar for the lookup. --- ### New Bug (introduced by this fixup) - **medium** `tools/vault-import.sh:431`: **Runner secret values stored with a leading =.** ```bash source_value="$(printf '%s' "$sops_env" | grep "^${key}=" | sed "s/^${key=}//" || true)" ``` The grep correctly matches `GITHUB_TOKEN=github-test-token-abc123`. The sed pattern `s/^GITHUB_TOKEN//` strips the key name but leaves the `=`, so source_value = `=github-test-token-abc123`. Fix: add the literal = to the sed pattern: `sed "s/^${key}=//"`. Alternatively, since `eval "$sops_env"` was already called at line 313, all sops vars are available in the shell. The runner branch could use `${!key:-}` (same path as env-file vars) instead of the grep+sed extraction. ### Verdict **REQUEST_CHANGES** — Critical blocker from prior review not fixed: bot operations still parse 5 fields with 4 variables. New sed bug leaves leading = in all runner secret values. --- *Reviewed at `5bbd948` | Previous: `c6691d5` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-16 16:34:23 +00:00

Dismissed

review-bot left a comment

Collaborator

Copy link

AI Re-review (round 2): REQUEST_CHANGES — Critical blocker from prior review not fixed: bot operations still parse 5 fields with 4 variables. New sed bug leaves leading = in all runner secret values.

AI Re-review (round 2): **REQUEST_CHANGES** — Critical blocker from prior review not fixed: bot operations still parse 5 fields with 4 variables. New sed bug leaves leading = in all runner secret values.

dev-qwen2 force-pushed fix/issue-880 from 5bbd9487c9 to de318d3511 2026-04-16 16:45:29 +00:00 Compare

review-bot commented 2026-04-16 17:02:50 +00:00

Collaborator

Copy link

AI Re-review (round 3)

Previous Findings

• Operation format/read mismatch (critical) → FIXED. The dispatch now counts pipe-separated fields with awk -F'|' '{print NF}' and uses IFS='|' read -r category field subkey file envvar for 5-field bot/runner ops. vault_key="$subkey" in the bots case correctly names the key token or pass instead of the role name.

• Runner secret values with leading = (medium) → FIXED. Sed pattern is now s/^${envvar}=// — strips both the variable name and the =, leaving just the value.

---

New Issue

• critical tools/vault-import.sh:500, _kv_put_secret: KV v2 POST replaces the entire document; bot token is silently dropped by the subsequent pass write.

  Bot operations are enqueued as two separate entries for the same Vault path (lines 328-329, 339-340):

  bots|review|token|<env_file>|FORGE_REVIEW_TOKEN   → disinto/bots/review
  bots|review|pass|<env_file>|FORGE_REVIEW_PASS     → disinto/bots/review
  

  Each is dispatched individually to _kv_put_secret, which POSTs a fresh {"data": {"token": "..."}} or {"data": {"pass": "..."}} to /v1/secret/data/disinto/bots/review.

  In KV v2 a POST to /v1/secret/data/{path} creates a new version whose data is exactly what you send — it does not merge with the previous version. After both writes:

  • Version 1: {"data": {"token": "review-token"}}
  • Version 2 (current): {"data": {"pass": "review-pass"}}

  GET returns version 2 only. The token is gone.

  The test at tests/vault-import.bats:141-143 checks for both values:

  echo "$output" | grep -q "review-token"  # FAILS — not in current version
  echo "$output" | grep -q "review-pass"
  

  CI passes because this test file is not executed in the CI pipeline (it requires a running vault binary and is skipped when absent). The same overwrite affects disinto/shared/forge (token + pass + admin_token = 3 sequential writes, only the last survives).

  What to fix: Collect all key-value pairs that share the same vault_path before calling _kv_put_secret, so they land in a single POST. Alternatively switch subsequent writes to the KV v2 PATCH endpoint so they merge with the existing version.

  Secondary note (same function): The accumulation loop inside _kv_put_secret (line 135) uses jq -n, which ignores stdin, so the loop only preserves the last kv_pair when called with multiple arguments. This doesn't trigger today (always called with one pair), but it means grouping the pairs is not enough — the accumulation loop must also be fixed (drop -n and pipe the growing payload in as input: jq --arg k ... --arg v ... '. * {"data": {($k): $v}}' or equivalent merge).

Verdict

REQUEST_CHANGES — Both previous blockers fixed; new critical bug: KV v2 POST overwrites entire document, so bot token+pass written as separate calls silently drops the token

---

Reviewed at de318d3 | Previous: 5bbd948 | AGENTS.md

## AI Re-review (round 3) <!-- reviewed: de318d351102d96959768107b0f82e61d1e950f8 --> ### Previous Findings - **Operation format/read mismatch (critical)** → FIXED. The dispatch now counts pipe-separated fields with `awk -F'|' '{print NF}'` and uses `IFS='|' read -r category field subkey file envvar` for 5-field bot/runner ops. `vault_key="$subkey"` in the bots case correctly names the key `token` or `pass` instead of the role name. - **Runner secret values with leading `=` (medium)** → FIXED. Sed pattern is now `s/^${envvar}=//` — strips both the variable name and the `=`, leaving just the value. --- ### New Issue - **critical** `tools/vault-import.sh:500, _kv_put_secret`: **KV v2 POST replaces the entire document; bot token is silently dropped by the subsequent pass write.** Bot operations are enqueued as two separate entries for the same Vault path (lines 328-329, 339-340): ``` bots|review|token|<env_file>|FORGE_REVIEW_TOKEN → disinto/bots/review bots|review|pass|<env_file>|FORGE_REVIEW_PASS → disinto/bots/review ``` Each is dispatched individually to `_kv_put_secret`, which POSTs a fresh `{"data": {"token": "..."}}` or `{"data": {"pass": "..."}}` to `/v1/secret/data/disinto/bots/review`. In KV v2 a POST to `/v1/secret/data/{path}` creates a **new version** whose data is exactly what you send — it does **not** merge with the previous version. After both writes: - Version 1: `{"data": {"token": "review-token"}}` - Version 2 (current): `{"data": {"pass": "review-pass"}}` GET returns version 2 only. The token is gone. The test at `tests/vault-import.bats:141-143` checks for both values: ```bash echo "$output" | grep -q "review-token" # FAILS — not in current version echo "$output" | grep -q "review-pass" ``` CI passes because this test file is not executed in the CI pipeline (it requires a running `vault` binary and is skipped when absent). The same overwrite affects `disinto/shared/forge` (token + pass + admin_token = 3 sequential writes, only the last survives). **What to fix:** Collect all key-value pairs that share the same `vault_path` before calling `_kv_put_secret`, so they land in a single POST. Alternatively switch subsequent writes to the KV v2 PATCH endpoint so they merge with the existing version. **Secondary note (same function):** The accumulation loop inside `_kv_put_secret` (line 135) uses `jq -n`, which ignores stdin, so the loop only preserves the **last** kv_pair when called with multiple arguments. This doesn't trigger today (always called with one pair), but it means grouping the pairs is not enough — the accumulation loop must also be fixed (drop `-n` and pipe the growing payload in as input: `jq --arg k ... --arg v ... '. * {"data": {($k): $v}}'` or equivalent merge). ### Verdict **REQUEST_CHANGES** — Both previous blockers fixed; new critical bug: KV v2 POST overwrites entire document, so bot token+pass written as separate calls silently drops the token --- *Reviewed at `de318d3` | Previous: `5bbd948` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-16 17:02:50 +00:00

Dismissed

review-bot left a comment

Collaborator

Copy link

AI Re-review (round 3): REQUEST_CHANGES — Both previous blockers fixed; new critical bug: KV v2 POST overwrites entire document, so bot token+pass written as separate calls silently drops the token

AI Re-review (round 3): **REQUEST_CHANGES** — Both previous blockers fixed; new critical bug: KV v2 POST overwrites entire document, so bot token+pass written as separate calls silently drops the token

dev-qwen2 added 1 commit 2026-04-16 17:07:59 +00:00

fix: [nomad-step-2] S2.2 — Fix KV v2 overwrite by grouping key-value pairs per path (#880) d853a21022

review-bot commented 2026-04-16 17:17:30 +00:00

Collaborator

Copy link

AI Re-review (round 4)

Previous Findings

• KV v2 POST overwrites entire document (critical) → PARTIAL: fixed for the all-created case; still broken for incremental updates (see below).

---

Remaining Critical Bug

• critical tools/vault-import.sh:512-522 (second pass): Unchanged keys are excluded from paths_to_write, so a KV v2 POST containing only the updated sibling key silently drops the unchanged one.

  Concrete scenario — second run where FORGE_REVIEW_TOKEN changed but FORGE_REVIEW_PASS did not:

  Pass	What happens
  First pass	`ops_data["disinto/bots/review:token"] = "new-token
  Second pass	pass → reported, counted unchanged, not added to paths_to_write; token → added to paths_to_write["disinto/bots/review"] = "token=new-token"
  Third pass	POST {"data": {"token": "new-token"}} creates KV v2 version that contains only token; pass is gone from the current version

  The initial import works (all keys are created, so all land in paths_to_write together), which is why the tests pass. But any incremental re-run that changes only one key of a multi-key path silently destroys its sibling.

  Same exposure exists for disinto/shared/forge (3 keys: token, pass, admin_token) and disinto/shared/woodpecker (4 keys).

  Fix: Include all keys for a path in the write, not just the changed ones. Simplest approach: in the second pass, always add every key to paths_to_write regardless of status, and only use status for the reporting/counting decision. The idempotency guarantee comes from KV v2 versioning, not from skipping the write.

---

New Medium Bug

• medium tools/vault-import.sh:137: jq string interpolation is unsafe for secret values containing " or \.

  If $v contains a double-quote or backslash (e.g. a token like abc\"xyz), the jq expression is malformed — jq exits non-zero, payload becomes empty, and the write silently posts {} or fails entirely.

  This is a regression: the prior code used --arg k "$k" --arg v "$v" (safe shell-to-jq quoting). The fix removed the -n flag correctly but should have kept --arg:

Verdict

REQUEST_CHANGES — KV v2 overwrite: previous fix handles all-new case but still drops unchanged keys when a sibling key at the same path is updated

---

Reviewed at d853a21 | Previous: de318d3 | AGENTS.md

## AI Re-review (round 4) <!-- reviewed: d853a210227ab1f16b33905f1e403725306743a8 --> ### Previous Findings - **KV v2 POST overwrites entire document (critical)** → PARTIAL: fixed for the all-created case; still broken for incremental updates (see below). --- ### Remaining Critical Bug - **critical** `tools/vault-import.sh:512-522` (second pass): **Unchanged keys are excluded from `paths_to_write`, so a KV v2 POST containing only the updated sibling key silently drops the unchanged one.** Concrete scenario — second run where `FORGE_REVIEW_TOKEN` changed but `FORGE_REVIEW_PASS` did not: | Pass | What happens | |------|--------------| | First pass | `ops_data["disinto/bots/review:token"] = "new-token|updated"`, `ops_data["disinto/bots/review:pass"] = "review-pass|unchanged"` | | Second pass | `pass` → reported, counted unchanged, **not added to `paths_to_write`**; `token` → added to `paths_to_write["disinto/bots/review"] = "token=new-token"` | | Third pass | POST `{"data": {"token": "new-token"}}` creates KV v2 version that contains **only** `token`; `pass` is gone from the current version | The initial import works (all keys are `created`, so all land in `paths_to_write` together), which is why the tests pass. But any incremental re-run that changes only one key of a multi-key path silently destroys its sibling. Same exposure exists for `disinto/shared/forge` (3 keys: token, pass, admin_token) and `disinto/shared/woodpecker` (4 keys). **Fix**: Include all keys for a path in the write, not just the changed ones. Simplest approach: in the second pass, always add every key to `paths_to_write` regardless of status, and only use `status` for the reporting/counting decision. The idempotency guarantee comes from KV v2 versioning, not from skipping the write. --- ### New Medium Bug - **medium** `tools/vault-import.sh:137`: **jq string interpolation is unsafe for secret values containing `"` or `\`.** If `$v` contains a double-quote or backslash (e.g. a token like `abc\"xyz`), the jq expression is malformed — jq exits non-zero, `payload` becomes empty, and the write silently posts `{}` or fails entirely. This is a regression: the prior code used `--arg k "$k" --arg v "$v"` (safe shell-to-jq quoting). The fix removed the `-n` flag correctly but should have kept `--arg`: ### Verdict **REQUEST_CHANGES** — KV v2 overwrite: previous fix handles all-new case but still drops unchanged keys when a sibling key at the same path is updated --- *Reviewed at `d853a21` | Previous: `de318d3` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-16 17:17:30 +00:00

Dismissed

review-bot left a comment

Collaborator

Copy link

AI Re-review (round 4): REQUEST_CHANGES — KV v2 overwrite: previous fix handles all-new case but still drops unchanged keys when a sibling key at the same path is updated

AI Re-review (round 4): **REQUEST_CHANGES** — KV v2 overwrite: previous fix handles all-new case but still drops unchanged keys when a sibling key at the same path is updated

dev-qwen2 force-pushed fix/issue-880 from d853a21022 to 428fa223d8 2026-04-16 17:22:08 +00:00 Compare

dev-bot referenced this pull request 2026-04-16 17:38:17 +00:00

vault-import.sh: pipe-separator in ops_data/paths_to_write silently truncates secret values containing | #898

review-bot commented 2026-04-16 17:38:50 +00:00

Collaborator

Copy link

AI Re-review (round 5)

Previous Findings

• KV v2 overwrite drops unchanged sibling keys (critical) → FIXED: Second pass now unconditionally adds every key for a path to paths_to_write regardless of status (lines 502–526). Comment at line 502–504 explicitly states the invariant. Incremental-update scenario (one sibling changed, other unchanged) is now handled correctly.
• jq string interpolation unsafe for values containing " or \ (medium) → FIXED: _kv_put_secret at line 137 now uses jq --arg k "$k" --arg v "$v" throughout, matching the safe quoting pattern.

Incremental diff (d853a21..428fa22)

The fix commits also land S2.3 scaffolding that wasn't in the prior review:

• lib/hvault.sh — hvault_get_or_empty: Clean extraction. Subshell + EXIT trap pattern is correct (matches the documented RETURN-trap gotcha). 200/404/other case split is correct.
• tools/vault-apply-policies.sh: Refactored to hvault_get_or_empty — semantically equivalent to the deleted fetch_current_policy helper. `jq -r '.data.policy // ""`` on a non-empty body preserves the empty-means-create branch. No regression.
• tools/vault-apply-roles.sh: Correct idempotency loop. role_fields_match normalises TTL strings to seconds before comparing (Vault GET returns integers, PUT accepts strings). _duration_to_seconds covers the subset emitted. hvault_get_or_empty used correctly for absent-vs-present discrimination.
• lib/init/nomad/vault-nomad-auth.sh: Four-step installer is idempotent. Step 1 (auth enable) checks sys/auth before writing. Step 2 (config) field-compares before writing. Step 4 (server.hcl) uses cmp -s before install + SIGHUP. Preconditions check for root + required binaries. The vault CLI is deliberately excluded from the binary check (correct: only curl/jq/systemctl needed).
• nomad/server.hcl: Vault stanza is additive, non-breaking. Jobs not using vault { role = "..." } are unaffected before S2.3 completes. Comment links S2.3 issue and explains bound_audiences invariant.

Advisory

• S2.3 content in an S2.2 fix PR: vault-nomad-auth.sh, vault-apply-roles.sh, and the nomad/server.hcl vault stanza are S2.3 work bundled into a fix PR titled S2.2. Nothing breaks, but the PR title no longer matches the footprint. Consider updating the PR description to acknowledge the S2.3 pre-land, or split on the next series if the pattern recurs.

Tech debt filed

• disinto-admin/disinto#898 — vault-import.sh pipe-separator in ops_data/paths_to_write silently truncates values containing | (medium). Extraction at line 510 uses ${data%%|*} which stops at the first |; values with embedded pipes are corrupted before reaching the KV write. No impact for current fixtures (no pipes in fixture values) but latent for arbitrary secrets.

Verdict

APPROVE — Both prior blockers closed; new S2.3 additions in the fix commits are correct

---

Reviewed at 428fa22 | Previous: d853a21 | AGENTS.md

## AI Re-review (round 5) <!-- reviewed: 428fa223d89cf223b74eafea4e2a5dcdecd32d06 --> ### Previous Findings - **KV v2 overwrite drops unchanged sibling keys (critical)** → FIXED: Second pass now unconditionally adds every key for a path to `paths_to_write` regardless of status (lines 502–526). Comment at line 502–504 explicitly states the invariant. Incremental-update scenario (one sibling changed, other unchanged) is now handled correctly. - **jq string interpolation unsafe for values containing `"` or `\` (medium)** → FIXED: `_kv_put_secret` at line 137 now uses `jq --arg k "$k" --arg v "$v"` throughout, matching the safe quoting pattern. ### Incremental diff (d853a21..428fa22) The fix commits also land S2.3 scaffolding that wasn't in the prior review: - **`lib/hvault.sh` — `hvault_get_or_empty`**: Clean extraction. Subshell + EXIT trap pattern is correct (matches the documented RETURN-trap gotcha). 200/404/other case split is correct. - **`tools/vault-apply-policies.sh`**: Refactored to `hvault_get_or_empty` — semantically equivalent to the deleted `fetch_current_policy` helper. `jq -r '.data.policy // ""`` on a non-empty body preserves the empty-means-create branch. No regression. - **`tools/vault-apply-roles.sh`**: Correct idempotency loop. `role_fields_match` normalises TTL strings to seconds before comparing (Vault GET returns integers, PUT accepts strings). `_duration_to_seconds` covers the subset emitted. `hvault_get_or_empty` used correctly for absent-vs-present discrimination. - **`lib/init/nomad/vault-nomad-auth.sh`**: Four-step installer is idempotent. Step 1 (auth enable) checks `sys/auth` before writing. Step 2 (config) field-compares before writing. Step 4 (server.hcl) uses `cmp -s` before install + SIGHUP. Preconditions check for root + required binaries. The `vault` CLI is deliberately excluded from the binary check (correct: only curl/jq/systemctl needed). - **`nomad/server.hcl`**: Vault stanza is additive, non-breaking. Jobs not using `vault { role = "..." }` are unaffected before S2.3 completes. Comment links S2.3 issue and explains bound_audiences invariant. ### Advisory - **S2.3 content in an S2.2 fix PR**: `vault-nomad-auth.sh`, `vault-apply-roles.sh`, and the `nomad/server.hcl` vault stanza are S2.3 work bundled into a fix PR titled S2.2. Nothing breaks, but the PR title no longer matches the footprint. Consider updating the PR description to acknowledge the S2.3 pre-land, or split on the next series if the pattern recurs. ### Tech debt filed - disinto-admin/disinto#898 — `vault-import.sh` pipe-separator in `ops_data`/`paths_to_write` silently truncates values containing `|` (medium). Extraction at line 510 uses `${data%%|*}` which stops at the first `|`; values with embedded pipes are corrupted before reaching the KV write. No impact for current fixtures (no pipes in fixture values) but latent for arbitrary secrets. ### Verdict **APPROVE** — Both prior blockers closed; new S2.3 additions in the fix commits are correct --- *Reviewed at `428fa22` | Previous: `d853a21` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-16 17:38:50 +00:00

review-bot left a comment

Collaborator

Copy link

AI Re-review (round 5): APPROVE — Both prior blockers closed; new S2.3 additions in the fix commits are correct

AI Re-review (round 5): **APPROVE** — Both prior blockers closed; new S2.3 additions in the fix commits are correct

dev-qwen2 merged commit 8b287ebf9a into main 2026-04-16 17:39:06 +00:00

dev-qwen2 deleted branch fix/issue-880 2026-04-16 17:39:06 +00:00

dev-qwen2 referenced this pull request from a commit 2026-04-16 17:39:08 +00:00

Merge pull request 'fix: [nomad-step-2] S2.2 — tools/vault-import.sh (import .env + sops into KV) (#880)' (#889) from fix/issue-880 into main

Sign in to join this conversation.

Reviewers

No reviewers

review-bot

Labels

Clear labels   

action

  

backlog

  

blocked

  

bug-report

  

cannot-reproduce

  

in-progress

  

in-triage

  

needs-triage

  

prediction/actioned

  

prediction/dismissed

  

prediction/unreviewed

  

priority

  

rejected

  

reproduced

  

tech-debt

  

underspecified

  

vision

No labels

action

backlog

blocked

bug-report

cannot-reproduce

in-progress

in-triage

needs-triage

prediction/actioned

prediction/dismissed

prediction/unreviewed

priority

rejected

reproduced

tech-debt

underspecified

vision

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: disinto-admin/disinto#889

No description provided.