:PROPERTIES:
:GPTEL_MODEL: claude-opus-4-8
:GPTEL_BACKEND: Claude
:GPTEL_SYSTEM: Act as a Code Review Specialist. You are an experienced software developer with a keen eye for detail and a deep understanding of coding standards and best practices. \n\nYour task is to review the code provided by the user, focusing on areas such as:\n- Code quality and readability\n- Adherence to coding standards\n- Potential bugs and security vulnerabilities\n- Performance optimization\n\nYou will:\n- Provide constructive feedback on the code\n- Suggest improvements and refactoring where necessary\n- Highlight any security concerns\n- Ensure the code follows best practices\n\nRules:\n- Be objective and professional in your feedback\n- Prioritize clarity and maintainability in your suggestions\n- Consider the specific context and requirements provided with the code
:GPTEL_TOOLS:
:GPTEL_MAX_TOKENS: 10000
:GPTEL_BOUNDS: ((response (1310 15709) (15767 21231) (21271 23808)))
:END:

*** I have a script that helps me insert records into a recutils database that works well. I would like to adapt this to work when the recfile database is encrypted using age. Create a wrapper script that can take in recfiles encrypted with age then reencrypts them when finished. I would like this to work for both Ubuntu and MacOS.

* Code Review & Age-Encryption Wrapper

Before diving into the wrapper, let me share a few observations on your existing =zrec= script, then provide the encryption wrapper.

** Quick Review Notes on =zrec=

A few things worth flagging for the wrapper to work cleanly:

1. *Bug in mandatory/free-form prompts*: In the "Mandatory fields" and "Additional fields" loops, you call =prompt_field "$f" "$mand" ...= but =$mand= is not set in that scope (it's a leftover from the allowed-fields loop). For mandatory fields it should be ="yes"=; for free-form fields it should be ="no"=. This is a latent bug regardless of encryption.

2. *=recins= modifies the file in place*, which is exactly what our wrapper needs to intercept. Good news: the wrapper can stay decoupled by operating on a temp plaintext file and passing /that/ path to =zrec=.

3. The script reads from =recfile= via =recinf=/=awk= and writes via =recins= — so the wrapper just needs to hand =zrec= a decrypted temp file path.

---

** The Wrapper: =zrec-age=

This wrapper:
- Decrypts an age file to a secure temp location
- Runs =zrec= against the plaintext
- Re-encrypts on success
- Cleans up plaintext securely (best-effort) on all exit paths
- Supports both *passphrase* and *identity/recipient key* workflows
- Works on Ubuntu and macOS

#+begin_src bash
#!/usr/bin/env bash
#
# zrec-age - Encrypted-recfile wrapper around zrec.
#
# Decrypts an age-encrypted recfile to a temporary plaintext file, runs zrec
# against it, then re-encrypts the result back to the original location.
#
# Usage:
#   zrec-age [options] <encrypted-recfile> [record-type]
#
# Options:
#   -i <identity-file>     age identity (private key) file for DECRYPTION.
#                          May be repeated. (Implies key-based mode.)
#   -r <recipient>         age recipient (public key) for ENCRYPTION.
#                          May be repeated. (Implies key-based mode.)
#   -R <recipients-file>   File containing one recipient per line.
#   -p                     Use passphrase (symmetric) mode for both
#                          decrypt and encrypt. (Default if no -i/-r/-R given.)
#   -z <path-to-zrec>      Path to the zrec script (default: looks on PATH,
#                          then alongside this script).
#   -h                     Show this help.
#
# Environment:
#   ZREC_BIN               Overrides the zrec location (same as -z).
#
# Notes:
#   * In passphrase mode you will be prompted up to three times by age
#     (decrypt, then encrypt). This is unavoidable with symmetric age.
#   * Plaintext only ever lives in a 0700 temp directory and is removed
#     (best-effort shredded) on exit.
#
set -o pipefail

# ---------------------------------------------------------------------------
# Globals
# ---------------------------------------------------------------------------
PROG="$(basename "$0")"
WORKDIR=""
PLAINTEXT=""
PASSPHRASE_MODE="auto"     # auto | yes | no
declare -a IDENTITIES=()
declare -a RECIPIENTS=()
declare -a RECIPIENT_FILES=()
ZREC_BIN="${ZREC_BIN:-}"

# ---------------------------------------------------------------------------
# Logging helpers (all to stderr; stdout stays clean)
# ---------------------------------------------------------------------------
log()  { printf '%s\n' "$*" >&2; }
err()  { printf 'Error: %s\n' "$*" >&2; }
die()  { err "$*"; exit 1; }

usage() {
    sed -n '3,40p' "$0" | sed 's/^# \{0,1\}//' >&2
    exit "${1:-1}"
}

# ---------------------------------------------------------------------------
# Dependency checks
# ---------------------------------------------------------------------------
check_deps() {
    command -v age >/dev/null 2>&1 || die "'age' not found. Install it (brew install age / apt install age)."

    if [ -z "$ZREC_BIN" ]; then
        if command -v zrec >/dev/null 2>&1; then
            ZREC_BIN="$(command -v zrec)"
        else
            # Fall back to a zrec sitting next to this wrapper.
            local self_dir
            self_dir="$(cd "$(dirname "$0")" && pwd)"
            if [ -x "$self_dir/zrec" ]; then
                ZREC_BIN="$self_dir/zrec"
            fi
        fi
    fi
    [ -n "$ZREC_BIN" ] && [ -x "$ZREC_BIN" ] \
        || die "Could not find an executable 'zrec' (use -z or set ZREC_BIN)."
}

# ---------------------------------------------------------------------------
# Cross-platform secure temp dir creation.
# ---------------------------------------------------------------------------
make_workdir() {
    # mktemp -d differs slightly across platforms but -d <template> is portable.
    WORKDIR="$(mktemp -d "${TMPDIR:-/tmp}/zrec-age.XXXXXX")" \
        || die "Failed to create temporary directory."
    chmod 700 "$WORKDIR"
}

# ---------------------------------------------------------------------------
# Best-effort secure delete (cross-platform).
# ---------------------------------------------------------------------------
secure_rm() {
    local f="$1"
    [ -f "$f" ] || return 0
    if command -v shred >/dev/null 2>&1; then
        shred -u "$f" 2>/dev/null && return 0
    fi
    # macOS has no shred; overwrite once then unlink as a fallback.
    if command -v dd >/dev/null 2>&1; then
        local size
        size="$(wc -c < "$f" 2>/dev/null | tr -d ' ')"
        if [ -n "$size" ] && [ "$size" -gt 0 ]; then
            dd if=/dev/urandom of="$f" bs="$size" count=1 conv=notrunc 2>/dev/null || true
        fi
    fi
    rm -f "$f"
}

# ---------------------------------------------------------------------------
# Cleanup trap: shred plaintext, remove workdir. Runs on ANY exit.
# ---------------------------------------------------------------------------
cleanup() {
    [ -n "$PLAINTEXT" ] && secure_rm "$PLAINTEXT"
    [ -n "$WORKDIR" ] && [ -d "$WORKDIR" ] && rm -rf "$WORKDIR"
}
trap cleanup EXIT INT TERM HUP

# ---------------------------------------------------------------------------
# Build the age recipient argument array for encryption.
# Echoes nothing; populates a global array via name. We just assemble here.
# ---------------------------------------------------------------------------
build_recipient_args() {
    AGE_ENC_ARGS=()
    local r f
    for r in "${RECIPIENTS[@]}"; do
        AGE_ENC_ARGS+=(-r "$r")
    done
    for f in "${RECIPIENT_FILES[@]}"; do
        [ -f "$f" ] || die "Recipients file not found: $f"
        AGE_ENC_ARGS+=(-R "$f")
    done
}

# ---------------------------------------------------------------------------
# Decrypt encrypted file -> $PLAINTEXT
# ---------------------------------------------------------------------------
decrypt_file() {
    local encfile="$1"

    if [ "$PASSPHRASE_MODE" = "yes" ]; then
        log "Decrypting (passphrase mode)..."
        age --decrypt -o "$PLAINTEXT" "$encfile" \
            || die "Decryption failed (wrong passphrase or corrupt file?)."
    else
        local -a id_args=()
        local id
        for id in "${IDENTITIES[@]}"; do
            [ -f "$id" ] || die "Identity file not found: $id"
            id_args+=(-i "$id")
        done
        [ "${#id_args[@]}" -gt 0 ] || die "Key-based decryption requires at least one -i <identity>."
        log "Decrypting (key mode)..."
        age --decrypt "${id_args[@]}" -o "$PLAINTEXT" "$encfile" \
            || die "Decryption failed (wrong identity or corrupt file?)."
    fi
}

# ---------------------------------------------------------------------------
# Encrypt $PLAINTEXT -> encrypted file (atomic replace via temp + mv).
# ---------------------------------------------------------------------------
encrypt_file() {
    local encfile="$1"
    local tmp_enc="$WORKDIR/out.age"

    if [ "$PASSPHRASE_MODE" = "yes" ]; then
        log "Re-encrypting (passphrase mode)..."
        age --passphrase -o "$tmp_enc" "$PLAINTEXT" \
            || die "Re-encryption failed; original file left UNCHANGED."
    else
        build_recipient_args
        [ "${#AGE_ENC_ARGS[@]}" -gt 0 ] \
            || die "Key-based encryption requires at least one -r/-R recipient."
        log "Re-encrypting (key mode)..."
        age --encrypt "${AGE_ENC_ARGS[@]}" -o "$tmp_enc" "$PLAINTEXT" \
            || die "Re-encryption failed; original file left UNCHANGED."
    fi

    # Preserve original permissions where possible.
    if [ -f "$encfile" ]; then
        # Copy mode from the original encrypted file onto the new ciphertext.
        if command -v stat >/dev/null 2>&1; then
            local mode
            # GNU stat vs BSD stat.
            mode="$(stat -c '%a' "$encfile" 2>/dev/null || stat -f '%Lp' "$encfile" 2>/dev/null)"
            [ -n "$mode" ] && chmod "$mode" "$tmp_enc" 2>/dev/null || true
        fi
    fi

    # Atomic replace.
    mv -f "$tmp_enc" "$encfile" \
        || die "Failed to move new ciphertext into place; original may be intact, check $tmp_enc."
}

# ---------------------------------------------------------------------------
# Argument parsing
# ---------------------------------------------------------------------------
parse_args() {
    while getopts ":i:r:R:pz:h" opt; do
        case "$opt" in
            i) IDENTITIES+=("$OPTARG");      PASSPHRASE_MODE="no" ;;
            r) RECIPIENTS+=("$OPTARG");      PASSPHRASE_MODE="no" ;;
            R) RECIPIENT_FILES+=("$OPTARG"); PASSPHRASE_MODE="no" ;;
            p) PASSPHRASE_MODE="yes" ;;
            z) ZREC_BIN="$OPTARG" ;;
            h) usage 0 ;;
            \?) die "Unknown option: -$OPTARG" ;;
            :)  die "Option -$OPTARG requires an argument." ;;
        esac
    done
    shift $((OPTIND - 1))

    [ "$#" -ge 1 ] || usage 1

    ENCFILE="$1"
    shift
    # Remaining args (record-type) get forwarded to zrec verbatim.
    ZREC_EXTRA_ARGS=("$@")

    # Resolve auto mode -> passphrase if no keys supplied.
    [ "$PASSPHRASE_MODE" = "auto" ] && PASSPHRASE_MODE="yes"

    # Sanity: in key mode, ensure we can both decrypt and re-encrypt.
    if [ "$PASSPHRASE_MODE" = "no" ]; then
        [ "${#IDENTITIES[@]}" -gt 0 ] \
            || die "Key mode: provide -i <identity> for decryption."
        if [ "${#RECIPIENTS[@]}" -eq 0 ] && [ "${#RECIPIENT_FILES[@]}" -eq 0 ]; then
            die "Key mode: provide -r/-R recipient(s) for re-encryption."
        fi
    fi
}

# ---------------------------------------------------------------------------
# Main
# ---------------------------------------------------------------------------
main() {
    parse_args "$@"
    check_deps

    [ -f "$ENCFILE" ] || die "Encrypted recfile '$ENCFILE' does not exist."
    [ -r "$ENCFILE" ] || die "Encrypted recfile '$ENCFILE' is not readable."

    make_workdir
    PLAINTEXT="$WORKDIR/recfile.rec"

    decrypt_file "$ENCFILE"

    # Snapshot to detect whether zrec actually changed anything.
    local before_sum after_sum
    before_sum="$(cksum < "$PLAINTEXT")"

    log ""
    log "Launching zrec on decrypted copy..."
    log "----------------------------------------"

    # Run zrec interactively against the plaintext temp file.
    if "$ZREC_BIN" "$PLAINTEXT" "${ZREC_EXTRA_ARGS[@]}"; then
        after_sum="$(cksum < "$PLAINTEXT")"
        if [ "$before_sum" = "$after_sum" ]; then
            log "No changes detected; skipping re-encryption."
        else
            encrypt_file "$ENCFILE"
            log "Encrypted recfile updated: $ENCFILE"
        fi
    else
        local rc=$?
        err "zrec exited with status $rc; original encrypted file left UNCHANGED."
        exit "$rc"
    fi
}

main "$@"
#+end_src

---

** How to Use It

*Passphrase mode* (simplest — default when no keys given):
#+begin_src bash
zrec-age contacts.rec.age
zrec-age contacts.rec.age Person     # pre-select a record type
#+end_src

*Key mode* (recommended for automation / shared files):
#+begin_src bash
zrec-age -i ~/.age/key.txt -r age1ql3z7hjy54... contacts.rec.age
# or with a recipients file:
zrec-age -i ~/.age/key.txt -R ~/.age/recipients.txt contacts.rec.age
#+end_src

---

** Design Decisions & Safety Highlights

| Concern | How it's handled |
|---|---|
| *Plaintext exposure* | Lives only in a =chmod 700= temp dir; shredded (or overwritten on macOS) on every exit path via =trap=. |
| *Crash/Ctrl-C safety* | =trap cleanup EXIT INT TERM HUP= guarantees cleanup. |
| *Don't corrupt originals* | Re-encryption goes to a temp =.age=, then =mv -f= (atomic same-filesystem replace). On any failure, the original ciphertext is untouched. |
| *No needless re-encrypt* | =cksum= comparison skips re-encryption if =zrec= made no changes (also avoids an extra passphrase prompt). |
| *macOS vs Ubuntu* | =shred= fallback to =dd=; =stat -c= vs =stat -f=; =${TMPDIR:-/tmp}=; portable =mktemp -d= template. |
| *Permission preservation* | Copies the original ciphertext's mode onto the new one. |

---

** Recommendations & Caveats

1. *Fix the =$mand= bug in =zrec= first.* As noted, the mandatory and free-form loops pass an unset =$mand=. This isn't an encryption issue, but it will produce incorrect prompts. Change those calls to ="yes"= and ="no"= respectively.

2. *Passphrase prompts twice.* Symmetric age inherently prompts on decrypt and again on encrypt. If that bothers you, prefer key mode. I deliberately did *not* cache the passphrase in a variable — keeping a passphrase in a shell variable/=age --passphrase= via env is a meaningful security regression.

3. *=mv= across filesystems.* If =$TMPDIR= is on a different filesystem than the recfile, =mv= becomes a copy+delete and loses atomicity. If you need strict atomicity, set =TMPDIR= to the same filesystem as the recfile, or write the temp ciphertext next to the target (e.g. =${ENCFILE}.tmp.$$=) instead of in =$WORKDIR=.

4. *Shredding on copy-on-write/SSD filesystems is unreliable* (APFS, btrfs, ext4 with journaling). The =secure_rm= here is best-effort; treat it as defense-in-depth, not a guarantee.

5. *Backup before first real use.* Test on a copy. A bug in either script could, in the worst case, leave you with a re-encrypted file you can't easily reconstruct.

6. *Consider verifying decrypt output is non-empty* before launching =zrec= — a zero-byte decrypt usually signals a silent failure on some =age= versions. You could add =[ -s "$PLAINTEXT" ] || die "Decryption produced empty output."= after =decrypt_file=.

Would you like me to also add a non-interactive =--backup= option (keeping a timestamped copy of the previous ciphertext), or a dry-run mode?

*** What is the $mand bug in zrec and how do I fix it?

- The =$mand= Bug in =zrec=

* What's Wrong

In your =main()= function, the variable =$mand= is a /local/ declared inside the /allowed-fields/ loop:

#+begin_src bash
# ---- 1. Prompt for ALLOWED fields (if defined) ------------------------
if [ -n "$allowed_fields" ]; then
    ...
    for f in $allowed_fields; do
        ...
        local mand="no"                       # <-- declared HERE
        in_list "$f" $mandatory_fields && mand="yes"
        ...
        val=$(prompt_field "$f" "$mand" "$ftype" "$label")
        ...
    done
fi
#+end_src

That =local mand= only exists meaningfully within that loop. But you then reference =$mand= again in /two other loops/ where it was never set for those fields:

/Loop 2 — Mandatory fields:/
#+begin_src bash
for f in $mandatory_fields; do
    if ! in_list "$f" $prompted; then
        ...
        val=$(prompt_field "$f" "$mand" "$ftype" "$label")   # <-- $mand is STALE
        ...
    fi
done
#+end_src

/Loop 3 — Additional / free-form fields:/
#+begin_src bash
while true; do
    ...
    val=$(prompt_field "$newfield" "$mand" "$ftype" "$label") # <-- $mand is STALE
    ...
done
#+end_src

* Why It Matters

=$mand= is being used as the =is_mandatory= argument to =prompt_field=. Because it's a single =local= variable scoped to =main()=, its value /leaks/ from the allowed-fields loop into the other two loops, carrying whatever value it happened to hold on the last iteration.

The practical consequences:

| Scenario | Expected | Actual (buggy) behavior |
|---|---|---|
| /Mandatory fields loop/ | Each field should be ="yes"= (it's mandatory, by definition of this loop) | Uses leftover =$mand= — could be ="no"=, so a genuinely required field won't loop-until-non-empty. You can submit it empty. |
| /Free-form fields loop/ | Each field should be ="no"= (user-defined, optional) | Uses leftover =$mand= — could be ="yes"=, forcing the user to enter a non-empty value for a field they may want to skip. |
| /=$mand= never set at all/ | — | If =allowed_fields= is empty, the allowed-loop never runs, so =$mand= is /unset/empty/ in the other loops. =prompt_field= treats empty as "not mandatory". |

The subtlest case is the /mandatory fields loop being too lenient/: a field that's declared =%mandatory:= but /not/ in the =%allowed:= list could be left empty, and =recins= would then reject the record (or insert an invalid one). That's the opposite of what your interactive prompt is supposed to guarantee.

* The Fix

Set the correct, explicit value in each loop instead of relying on the leaked variable.

/Loop 2 — Mandatory fields/ (these are always mandatory, so hardcode ="yes"=):
#+begin_src bash
if [ -n "$mandatory_fields" ]; then
    local f
    local printed_header="no"
    for f in $mandatory_fields; do
        if ! in_list "$f" $prompted; then
            if [ "$printed_header" = "no" ]; then
                echo "== Mandatory fields ==" >&2
                printed_header="yes"
            fi
            local info ftype label
            info=$(get_field_type_info "$block" "$f")
            ftype="${info%%"$TAB"*}"
            label="${info#*"$TAB"}"

            local val
            val=$(prompt_field "$f" "yes" "$ftype" "$label")   # <-- was "$mand"
            args+=(-f "$f" -v "$val")
            prompted="$prompted $f"
        fi
    done
fi
#+end_src

/Loop 3 — Additional / free-form fields/ (always optional, so hardcode ="no"=):
#+begin_src bash
while true; do
    printf "Field name: " >&2
    local newfield
    IFS= read -r newfield
    case "$newfield" in
    "" | done | DONE | q | quit) break ;;
    esac

    local info ftype label
    info=$(get_field_type_info "$block" "$newfield")
    ftype="${info%%"$TAB"*}"
    label="${info#*"$TAB"}"

    local val
    val=$(prompt_field "$newfield" "no" "$ftype" "$label")     # <-- was "$mand"

    args+=(-f "$newfield" -v "$val")
done
#+end_src

The /allowed-fields loop (Loop 1) is correct already/ — leave its =local mand="no"= / =in_list ... && mand="yes"= logic as-is, since it genuinely needs to check whether each allowed field is also mandatory.

* Recommended Hardening

To prevent this class of leak in the future, declare =mand= explicitly per-loop (or avoid the shared name entirely). Two options:

1. /Scope it inside each loop/ — declare =local mand="yes"= (or ="no"=) at the top of Loops 2 and 3 even though you hardcode it. This documents intent and prevents accidental reliance on an outer value.

2. /Enable strict mode/ during development to catch unset variables:
#+begin_src bash
set -u   # treat unset variables as an error
#+end_src
With =set -u=, the empty-=$mand= case (when =allowed_fields= is empty) would have surfaced immediately as an error rather than silently passing an empty string. Be aware this requires auditing the rest of the script for other unguarded unset-variable references first.

* Quick Verification

After applying the fix, test these cases:

1. A recfile where a =%mandatory:= field is /not/ in =%allowed:= — confirm the prompt now refuses an empty value (loops until you type something).
2. A free-form recfile (no =%allowed:=) — confirm the "Additional fields" prompts let you submit fields with empty values without being forced to re-enter.
3. A recfile with no =%allowed:= and a =%mandatory:= field — confirm mandatory enforcement still works.

This bug is independent of the age wrapper, so fix it directly in =zrec= and the wrapper will benefit automatically.

*** PROG in zrec-age appears unused.

Good catch — you're right.

- Unused Variable: =PROG=

In =zrec-age=, this global is declared but never referenced:

#+begin_src bash
PROG="$(basename "$0")"
#+end_src

The reason it's dead: I route usage output through =usage()=, which extracts the header comment block directly from the script file rather than composing an error string from =$PROG=. And the =die()=/=err()=/=log()= helpers print plain messages without a program-name prefix. So =PROG= never gets used.

- Two Ways to Resolve It

* Option A — Remove it (simplest)

Just delete the line:

#+begin_src bash
# ---------------------------------------------------------------------------
# Globals
# ---------------------------------------------------------------------------
WORKDIR=""
PLAINTEXT=""
PASSPHRASE_MODE="auto"     # auto | yes | no
declare -a IDENTITIES=()
declare -a RECIPIENTS=()
declare -a RECIPIENT_FILES=()
ZREC_BIN="${ZREC_BIN:-}"
#+end_src

* Option B — Actually use it (better UX)

Prefixing diagnostics with the program name is a common, useful convention — it makes messages traceable when =zrec-age= is invoked from a pipeline, cron job, or another script. If you'd like that, keep =PROG= and wire it into the logging helpers:

#+begin_src bash
err()  { printf '%s: Error: %s\n' "$PROG" "$*" >&2; }
die()  { err "$*"; exit 1; }
#+end_src

You could optionally prefix =log()= too, though I'd leave =log()= unprefixed since it carries interactive/progress messages meant for the human at the terminal:

#+begin_src bash
log()  { printf '%s\n' "$*" >&2; }   # leave as-is
#+end_src

With Option B, output changes from:

#+begin_example
Error: 'age' not found. Install it ...
#+end_example

to:

#+begin_example
zrec-age: Error: 'age' not found. Install it ...
#+end_example

- Recommendation

I'd go with /Option B/ — the prefix costs almost nothing and improves diagnosability, which matters for a security-sensitive wrapper that may run non-interactively. But if you prefer to keep messages terse and =zrec-age= is only ever run interactively, /Option A/ is perfectly fine.

- Prevention Tip

This is exactly the kind of thing a linter catches automatically. Running =shellcheck= over both scripts would have flagged it as:

#+begin_example
SC2034: PROG appears unused. Verify use (or export if used externally).
#+end_example

I'd recommend adding =shellcheck= to your workflow — it will also catch a number of other subtle issues (unquoted expansions, unset-variable risks like the =$mand= bug we discussed, etc.) across both =zrec= and =zrec-age=.