Merge pull request 'v0.7.3.1 refactor: external-extractor + read-guard dedup, legacy-pull nested-blob warning (EGB-701)' (#12) from brian/egb-701-secrets-stage-1-structural-cleanups-dedup-external-entry into main

CHANGELOG.md

@@ -5,6 +5,34 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to a four-digit MAJOR.MINOR.PATCH.MICRO version scheme.
 
+## [0.7.3.1] - 2026-06-18
+
+### Changed
+
+- **EGB-677 stage-1 structural cleanups (EGB-701)** — tech-debt dedup with one
+  new safety warning; no behavior change for the manifest-driven (v2) happy path.
+  - **`secrets which` now reuses the one external-entry extractor** the push/pull
+    path uses (`_json_external_entries`) instead of its own duplicated `jq` 
+    projection. So `which` applies the same `properties`→`gradle-properties`
+    normalization and skips (with a warning) the same malformed external entries
+    the sync path drops — `which` shows exactly what will sync, not a stale raw
+    projection that could drift from the real behavior.
+  - **The two external-manifest read guards are factored into shared helpers** —
+    `_json_readable` (plain regular file, silent) and `_legacy_readable` (warns
+    and skips a symlinked `.secrets-files`) — so `_external_entries_for_push` and
+    `_external_entries_for_pull` can't drift apart.
+
+### Fixed
+
+- **Legacy (manifest-less) `pull` no longer silently under-restores (EGB-701)** —
+  the manifest-less pull path globs only top-level `*.age`/`.*.age`, so a nested
+  dotenv blob (`<project>/<relpath>.age`) written by a manifest-driven push on
+  another machine was invisible: restored nothing, counted nothing, said nothing.
+  It now **warns** and names each nested blob it can't reach (external blobs are
+  excluded — `pull_external_files` handles those), pointing at committing a
+  `.secrets.json` as the fix. The manifest-driven pull already restored nesting
+  correctly; this only closes the legacy path's blind spot.
+
 ## [0.7.3.0] - 2026-06-08
 
 ### Added

CLAUDE.md

@@ -76,8 +76,8 @@ secrets              # CLI script (~2000 lines bash)
 hooks/pre-commit     # Pre-commit hook template
 test/
   secrets.bats       # bats-core test suite (140 tests)
-  manifest.bats      # EGB-677 .secrets.json manifest tests (78 tests)
+  manifest.bats      # EGB-677 .secrets.json manifest tests (83 tests)
-  migrate.bats       # EGB-703 store-format-v2 migration tests (26 tests)
+  migrate.bats       # EGB-703 store-format-v2 migration tests (35 tests)
   test_helper.bash   # Shared setup/teardown
 README.md            # User-facing documentation
 CLAUDE.md            # This file
@@ -107,7 +107,7 @@ The active store directory is picked by `resolve_store()` using these rules, hig
 Key design decisions (all driven by /autoplan review):
 
 - **Wire-in is at command scope** (`cmd_push`/`cmd_pull`), via `push_external_files` / `pull_external_files`, **not** inside `push_dir_to_project` / `pull_project_to_dir` (those loop per-workspace and `pull_project_to_dir` uses stdout as a data channel).
-- **Storage:** blobs live in `$SECRETS_DIR/<project>/external/<slug>.gradle-properties.age`. The `external/` subdir keeps them out of the legacy non-recursive `*.age` / `.*.age` globs the dotenv `pull` path uses, so a dotenv pull can never decrypt an external blob into cwd. `cmd_rekey` and `cmd_list` instead walk the **entire** project tree (`find -type f`), so they cover both `external/<slug>.age` and nested manifest dotenv blobs (`<project>/<relpath>.age`) — rekey MUST recurse, or any nested/external blob is orphaned under the old key after rotation = data loss (EGB-677 regression test: "rekey re-encrypts a nested manifest dotenv blob"). `<slug>` = manifest path token with non-`[A-Za-z0-9._-]` chars → `_`, plus a `cksum` suffix of the original path so paths that clean to the same string (`a/b` vs `a_b`) don't collide. Machine-independent (derived from the committed manifest token, not the expanded path). `cmd_list --json` (EGB-699) emits the same recursive walk as a machine-readable object (`{store, projects[].entries[]}`, each entry `dotenv`→`path` or `external`→`subtype`+`path`) for tooling/CI (feeds EGB-671); jq assembles it so paths escape correctly and stdout stays pure JSON (the human store hint is suppressed; jq is a hard dep only in `--json` mode).
+- **Storage:** blobs live in `$SECRETS_DIR/<project>/external/<slug>.gradle-properties.age`. The `external/` subdir keeps them out of the legacy non-recursive `*.age` / `.*.age` globs the dotenv `pull` path uses, so a dotenv pull can never decrypt an external blob into cwd. `cmd_rekey` and `cmd_list` instead walk the **entire** project tree (`find -type f`), so they cover both `external/<slug>.age` and nested manifest dotenv blobs (`<project>/<relpath>.age`) — rekey MUST recurse, or any nested/external blob is orphaned under the old key after rotation = data loss (EGB-677 regression test: "rekey re-encrypts a nested manifest dotenv blob"). **EGB-701 cleanups:** (1) the *legacy* (manifest-less) `pull` keeps its non-recursive globs but now **warns** when nested `<project>/<relpath>.age` blobs exist that those globs can't see (it excludes `external/`, which `pull_external_files` handles) — so a manifest-less pull never silently under-restores; the fix the warning points at is committing a `.secrets.json`. (2) `cmd_which`, push, and pull share one external extractor (`_json_external_entries`), so `which` applies the same `properties`→`gradle-properties` normalization and skip-with-warning rules the sync path does (it shows exactly what will sync, not a stale raw projection). (3) the two external-manifest read guards are factored into `_json_readable` (plain regular file, silent) / `_legacy_readable` (warn-and-skip on a symlinked legacy manifest). `<slug>` = manifest path token with non-`[A-Za-z0-9._-]` chars → `_`, plus a `cksum` suffix of the original path so paths that clean to the same string (`a/b` vs `a_b`) don't collide. Machine-independent (derived from the committed manifest token, not the expanded path). `cmd_list --json` (EGB-699) emits the same recursive walk as a machine-readable object (`{store, projects[].entries[]}`, each entry `dotenv`→`path` or `external`→`subtype`+`path`) for tooling/CI (feeds EGB-671); jq assembles it so paths escape correctly and stdout stays pure JSON (the human store hint is suppressed; jq is a hard dep only in `--json` mode).
 - **Merge is pure bash, no `sed`/regex** (`merge_gradle_keys`): exact-string key comparison (avoids `beaconClerkPk` vs `beaconClerkPkTest` substring bug), value treated as opaque literal (survives `& \ /` in values). Updates a managed key in place at its first occurrence, collapses duplicates, appends new keys, preserves unrelated lines/comments/order. Continuation lines (trailing odd backslashes, tracked by `_trailing_bs_odd`) are never matched as keys. Atomic write: temp in the same dir → `chmod` to match (or `600` on create) → `mv`. Backs up to `<target>.secrets-bak` before each merge.
 - **Properties separator parsing** (`_props_get`): key ends at the first `=`, `:`, or whitespace (after lstrip); handles `key=value`, `key = value`, `key:value`, `key value`; last definition wins.
 - **Security:** the write target comes from a committed file, so `_validate_external_target_path` locks it down — basename must be `gradle.properties`, must resolve inside `$HOME` (deepest-existing-ancestor resolved, symlink target/parent refused, `..` rejected). This blocks a malicious manifest from appending decrypted keys to `~/.gitconfig`/`~/.bashrc`. `_parse_secrets_files_manifest` rejects shell metacharacters/control chars in path and keys (path allows `[A-Za-z0-9/._~-]` only; keys allow `[A-Za-z0-9._-]` + space), mirrors the `.secrets-store` posture (no shell expansion, symlinked manifest skipped).

VERSION

@@ -1 +1 @@
-0.7.3.0
+0.7.3.1

secrets

@@ -1038,6 +1038,31 @@ _json_external_entries() {
   done < <(jq -r '.external // [] | .[] | [.type, .path, ((.keys // []) | join(" "))] | @tsv' "$manifest")
 }
 
+# EGB-701 item 2: the read guards for the two external-manifest sources,
+# factored out of _external_entries_for_push/_pull so they can't drift.
+#
+# _json_readable — true when a .secrets.json is a safe regular file to read.
+# A symlinked manifest is treated as absent and silently ignored: it's the
+# project's own committed file, so a symlink there is just skipped (the fatal
+# symlink refusal lives in _check_manifest_file, used by the linting paths).
+_json_readable() {
+  [ -f "$1" ] && [ ! -L "$1" ]
+}
+
+# _legacy_readable — true when a legacy .secrets-files is a safe regular file
+# to read, warning (and returning false) when it exists but is a symlink: a
+# symlinked legacy manifest's target is attacker-influenceable, so never follow
+# it. A missing or non-regular file returns false silently.
+_legacy_readable() {
+  local legacy="$1"
+  [ -e "$legacy" ] || return 1
+  if [ -L "$legacy" ]; then
+    echo "WARNING: $legacy is a symlink; ignoring." >&2
+    return 1
+  fi
+  [ -f "$legacy" ]
+}
+
 # External tuples for PUSH: .secrets.json entries first, then legacy
 # .secrets-files entries whose (type, path) the manifest doesn't cover —
 # the absorb set, which cmd_push folds into the manifest after a
@@ -1046,24 +1071,20 @@ _external_entries_for_push() {
   local root="$1"
   local json="$root/$SECRETS_JSON_NAME" legacy="$root/$SECRETS_FILES_NAME"
   local seen="" t p k
-  if [ -f "$json" ] && [ ! -L "$json" ]; then
+  if _json_readable "$json"; then
     while IFS=$'\t' read -r t p k; do
       [ -n "$t" ] || continue
       printf '%s\t%s\t%s\n' "$t" "$p" "$k"
       seen="$seen$t|$p"$'\n'
     done < <(_json_external_entries "$json")
   fi
-  if [ -e "$legacy" ]; then
+  if _legacy_readable "$legacy"; then
-    if [ -L "$legacy" ]; then
-      echo "WARNING: $legacy is a symlink; ignoring." >&2
-    elif [ -f "$legacy" ]; then
     while IFS=$'\t' read -r t p k; do
       [ -n "$t" ] || continue
       case "$seen" in *"$t|$p"$'\n'*) continue ;; esac
       printf '%s\t%s\t%s\n' "$t" "$p" "$k"
     done < <(_parse_secrets_files_manifest "$legacy")
   fi
-  fi
 }
 
 # External tuples for PULL: the manifest wins entirely when present;
@@ -1071,19 +1092,18 @@ _external_entries_for_push() {
 _external_entries_for_pull() {
   local root="$1"
   local json="$root/$SECRETS_JSON_NAME" legacy="$root/$SECRETS_FILES_NAME"
-  if [ -f "$json" ] && [ ! -L "$json" ]; then
+  if _json_readable "$json"; then
-    if [ -f "$legacy" ] && [ ! -L "$legacy" ]; then
+    # A regular (non-symlink) legacy file alongside the manifest is superseded:
+    # warn but don't read it. _json_readable is the "plain regular file" test —
+    # exactly the supersede condition (and unlike _legacy_readable it stays
+    # silent on a symlink, matching the original no-warn-on-symlink behavior).
+    if _json_readable "$legacy"; then
       echo "WARNING: $legacy is superseded by $SECRETS_JSON_NAME and was ignored on pull. Run 'secrets push' to absorb it, then delete it." >&2
     fi
     _json_external_entries "$json"
     return 0
   fi
-  [ -e "$legacy" ] || return 0
+  _legacy_readable "$legacy" && _parse_secrets_files_manifest "$legacy"
-  if [ -L "$legacy" ]; then
-    echo "WARNING: $legacy is a symlink; ignoring." >&2
-    return 0
-  fi
-  [ -f "$legacy" ] && _parse_secrets_files_manifest "$legacy"
   return 0
 }
 
@@ -1838,6 +1858,26 @@ cmd_pull() {
 
   info "Decrypted $count file(s) into $target_dir"
 
+  # EGB-701 item 3: the globs above are non-recursive, so a nested dotenv blob
+  # (<project>/<relpath>.age) written by a manifest-driven push on another
+  # machine is invisible here — silently restored nothing, counted nothing.
+  # external/<slug>.age blobs are restored by pull_external_files, so exclude
+  # them. Warn (don't die) so a manifest-less pull never under-restores in
+  # silence; the fix is a committed .secrets.json, which the recursive
+  # manifest-driven branch above handles correctly.
+  local nested
+  nested=$(find "$SECRETS_DIR/$project" -mindepth 2 -type f -name '*.age' \
+    -not -path "$SECRETS_DIR/$project/external/*" 2>/dev/null)
+  if [ -n "$nested" ]; then
+    echo "WARNING: this project has nested encrypted files the manifest-less pull can't restore:" >&2
+    while IFS= read -r nf; do
+      [ -n "$nf" ] || continue
+      local rel="${nf#"$SECRETS_DIR/$project/"}"
+      echo "  ${rel%.age}" >&2
+    done <<< "$nested"
+    echo "  Add a $SECRETS_JSON_NAME manifest (run 'secrets push' on a machine that has these files) so they restore." >&2
+  fi
+
   # Merge any external files (.secrets-files) declared in this project.
   pull_external_files "$PWD" "$project"
 
@@ -2307,11 +2347,15 @@ cmd_which() {
         echo "  dotenv    $entry  [UNSAFE — will be refused]"
       fi
     done < <(jq -r '.dotenv // [] | .[]' "$json_manifest")
+    # EGB-701 item 1: reuse the one external extractor the sync path uses,
+    # so `which` applies the same normalization + skip-with-warning rules
+    # push/pull do — `which` shows exactly what will sync, never a stale
+    # raw projection that drifts from the helper.
     local etype epath ekeys
     while IFS=$'\t' read -r etype epath ekeys; do
       [ -n "$etype" ] || continue
       echo "  $etype  $epath  $ekeys"
-    done < <(jq -r '.external // [] | .[] | [.type, .path, ((.keys // []) | join(" "))] | @tsv' "$json_manifest")
+    done < <(_json_external_entries "$json_manifest")
   fi
 
   # Read back any external-file manifest in cwd (validates the format and

test/manifest.bats

@@ -642,6 +642,31 @@ m_nojq_path() {
   [[ "$output" == *"k1"* ]] || false
 }
 
+# EGB-701 item 1: `which` and the push/pull external extractor share one
+# helper (_json_external_entries), so `which` applies the same
+# properties→gradle-properties normalization the sync path uses — no drift.
+@test "which normalizes a properties external to the gradle-properties token (EGB-701)" {
+  create_project_dir whichnorm
+  printf '{"version":2,"dotenv":[".env"],"external":[{"type":"properties","path":"~/.gradle/gradle.properties","keys":["k1"]}]}\n' > .secrets.json
+  run "$SECRETS_BIN" which
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"gradle-properties"* ]] || false
+}
+
+# EGB-701 item 1: a malformed external (a properties entry with no keys) is
+# skipped by the sync path; routing `which` through the shared extractor means
+# `which` skips+warns it too, so it faithfully shows what actually syncs
+# rather than printing an entry push/pull silently drop.
+@test "which skips a malformed external entry the sync path would drop (EGB-701)" {
+  create_project_dir whichmalformed
+  printf '{"version":2,"dotenv":[".env"],"external":[{"type":"properties","path":"~/.gradle/gradle.properties"}]}\n' > .secrets.json
+  run "$SECRETS_BIN" which
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"has no keys"* ]] || false
+  # The skipped entry's path must NOT appear in the printed manifest summary.
+  [[ "$output" != *"  gradle-properties  ~/.gradle/gradle.properties"* ]] || false
+}
+
 # ─── F: ship Step 7 coverage backfill (audit gaps) ─────────────────────
 
 @test "which flags an unsafe dotenv entry with the UNSAFE marker" {
@@ -739,6 +764,46 @@ m_nojq_path() {
   [ "$(cat packages/web/.env.development)" = "N=nested" ]
 }
 
+@test "legacy (manifest-less) pull warns about nested blobs it can't restore (EGB-701)" {
+  # The legacy pull path globs only top-level *.age/.*.age. A nested dotenv
+  # blob (<project>/<relpath>.age) written by a manifest-driven push on another
+  # machine is invisible to those globs — restored nothing, counted nothing.
+  # The fix: warn so a manifest-less pull never silently under-restores.
+  init_with_remote
+  create_project_dir nestlegacy
+  mkdir -p packages/web
+  echo "N=nested" > packages/web/.env.development
+  "$SECRETS_BIN" add packages/web/.env.development >/dev/null
+  "$SECRETS_BIN" push >/dev/null 2>&1
+  [ -f "$SECRETS_DIR/nestlegacy/packages/web/.env.development.age" ]
+  # Simulate a machine with no manifest: drop .secrets.json + local files,
+  # forcing the legacy non-recursive glob branch.
+  rm -f .secrets.json
+  rm -rf packages
+  run "$SECRETS_BIN" pull nestlegacy
+  [ "$status" -eq 0 ]
+  # The warning names the nested blob and points at the manifest as the fix.
+  [[ "$output" == *"packages/web/.env.development"* ]] || false
+  [[ "$output" == *"$SECRETS_JSON_NAME"* || "$output" == *".secrets.json"* ]] || false
+  # The legacy path genuinely can't restore it (the warning is the contract).
+  [ ! -f packages/web/.env.development ]
+}
+
+@test "legacy pull does NOT warn about external/ blobs (handled separately, EGB-701)" {
+  # external/<slug>.age blobs are restored by pull_external_files, not the
+  # dotenv globs, so they must not trip the nested-blob warning.
+  init_with_remote
+  m_gradle_src $'beaconClerkPkTest=pk_test_abc\n'
+  create_project_dir extnolwarn
+  printf 'gradle-properties ~/.gradle/gradle.properties beaconClerkPkTest\n' > .secrets-files
+  "$SECRETS_BIN" push >/dev/null 2>&1
+  [ -d "$SECRETS_DIR/extnolwarn/external" ]
+  run "$SECRETS_BIN" pull extnolwarn
+  [ "$status" -eq 0 ]
+  [[ "$output" != *"can't restore"* ]] || false
+  [[ "$output" != *"nested encrypted"* ]] || false
+}
+
 @test "list shows a nested manifest blob" {
   init_with_remote
   create_project_dir nestlist