feat: /secret-generate — the CSPRNG half of the secret flow (v1.43.0) - #61
Conversation
The toolkit had three secret commands and all three assumed the value already existed somewhere: a human types it (/secret-input), one command consumes it (/secret-use), shred removes it (/secret-clear). Nothing MADE one. That gap is not cosmetic, because the obvious substitute defeats the other three. `openssl rand -base64 32` prints the value on stdout, and when an agent runs it that stdout lands in the agent's context and in the session transcript on disk — a place the value cannot be recalled from. The whole reason /secret-input reaches for an OS-native dialog is to keep a typed secret out of exactly those places. So the invariant is structural rather than a matter of care: THE GENERATED VALUE NEVER GOES TO STDOUT. It is written straight into the same staged file /secret-input uses, is never assigned to a variable at the script's top scope so no later line can print it by accident, and is shown only in a GUI window a human is looking at. stdout carries length, alphabet size and entropy. Both controls run, in both directions: - real script: value absent from stdout, 40 chars staged at 0600 - sabotaged copy: the check FINDS the value in stdout - fingerprint: stable for one value, different for another - shellcheck: clean Three decisions worth reading in the diff rather than inferring: Rejection sampling, not `byte % n`. 256 is not a multiple of most alphabet sizes, so the modulo makes the first `256 mod n` symbols more likely — for the 89-symbol default, 78 of them at ~1.35x. /dev/urandom throughout, never $RANDOM, which is a 15-bit generator seeded from pid and time. The symbol class excludes ' " ` \ deliberately. The value gets pasted into shell one-liners, JSON and YAML, and a backtick inside a double-quoted shell string is command substitution. --fingerprint answers the question a secret store cannot: a store will not read a value back, so after loading the same value into two of them nothing confirms they match. A truncated SHA-256 settles it with neither end printing anything sensitive. Labelled in the code as a comparison handle — NOT encryption, NOT protection — and the case where it is unsafe is named rather than left implicit: for a human-chosen password the hash IS dictionary-attackable, which is what a salt exists for. Also carries the control /secret-input already had: it refuses to stage an empty value. An empty staged secret is indistinguishable from a good one to every consumer downstream. The `have()` probe uses the plain form already in this repo's Makefile, because shellcheck flags the reverse order as SC2069 and `command -v` writes nothing to stderr on a miss — so nothing diagnostic is lost at that one call site.
![dojo-code-reviewer[bot]](https://avatars.githubusercontent.com/in/3092887?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Changes Requested
Changes requested — 1 blocker, 2 P2, 1 P3. Confidence: 1.00/5.00.
Walkthrough
main branch directly. According to GitFlow conventions, feature branches should target develop, which is then merged into main during a release cycle. Since this is an informative warning, the final review verdict remains strictly based on the technical code analysis.
Walkthrough of Changes
This pull request introduces /secret-generate, a CSPRNG-driven command-line generator for minting random staging secrets. It utilizes /dev/urandom and rejection sampling to avoid modulo bias, outputs metadata (length, alphabet, entropy) to stdout, and displays the generated secret in a secure GUI window using zenity. It also includes --status and --fingerprint (truncated SHA-256 handle) commands. The package versions are bumped to 1.43.0 across configurations.
Areas Reviewed
scripts/secret-generate.sh— Checked security parameters, CSPRNG rejection sampling loop,zenitywindow integration, permissions, and argument parsing.commands/secret-generate.md— Reviewed technical specifications, rules, and Spanish localization requirements.scripts/secret-store.sh— Used as design reference to check staging directory consistency and permission handling.
Safety Rationale
The PR is not safe to merge yet because the plaintext secret is passed directly to zenity --entry via command-line arguments (--entry-text). This exposes the sensitive secret value to the system process table (visible via ps or /proc), directly violating the project's security constraints and the script's own Absolute Rules.
Changes requested — 1 blocker, 2 P2s, 1 P3.
🔴 P1 — Blockers
scripts/secret-generate.sh:216— 🔴 P1 (blocker) — Passing the plaintext secret as a command-line argument (--entry-text="$(cat "$SECRET_PATH")") exposes it in the system's process table (e.g., viaps auxor/proc/<pid>/cmdline), where any local user or monitoring daemon can read it. This directly violates the codebase's own Absolute Rules (Reglas absolutas) documented incommands/secret-generate.md: "Nunca lo pases como argumento de otro comando. Unargves legible por cualquier proceso vía/procy queda en el trace del shell."
To fix this securely without exposing the secret to the process table, we can pipe the secret and metadata into standard input of zenity --text-info. This avoids command line exposure entirely. We also save and check the exit code of zenity so that if the window is closed/cancelled by the user (via X or Esc) or if a GUI error occurs, the script safely aborts instead of spinning in an infinite loop.
[pass 1]
🟡 P2 — Major
scripts/secret-generate.sh:171— 🟡 P2 (major) — Runningodandtrin a subshell for every single byte of the secret is a massive performance bottleneck. Spawning 2 processes per byte (e.g., 64 processes for a 32-character secret, or 256 processes for 128 characters) can make the script take several seconds to execute, especially on systems with slow process creation overhead (such as macOS, WSL, or Git Bash on Windows).
We can optimize this by reading random bytes in batches of size want * 2 in a single od invocation, then processing those bytes from an in-memory array in Bash. Rejection sampling is preserved perfectly, but the number of external processes spawned is reduced to exactly one.
[pass 1]
scripts/secret-generate.sh:199— 🟡 P2 (major) — In environments withset -euo pipefailactive, runningchmod 600 "$SECRET_PATH"directly can cause the script to immediately crash and terminate if the target filesystem does not support POSIX permissions (such as shared host mounts in Docker, or NTFS/FAT partitions on Git Bash/WSL/MSYS2).
To ensure parity with how secret-store.sh handles file permissions, we should ignore errors from this command by appending 2>/dev/null || true.
[pass 1]
🔵 P3 — Minor
scripts/secret-generate.sh:152— 🔵 P3 (minor) —bcis not part of the standard POSIX core utilities and is frequently missing from minimal Linux distributions, slim container images, and bare environments. Ifbcis missing, the script will crash underset -e.
We can calculate the entropy bits using standard awk (specifically the built-in log function), which is standard POSIX and guaranteed to be present on all Linux/macOS/Git Bash systems.
[pass 1]
Total findings: 1 security, 2 compliance, 1 business context (4 total)
| case "$sel" in *Uppercase*) USE_UPPER=1 ;; *) USE_UPPER=0 ;; esac | ||
| case "$sel" in *Lowercase*) USE_LOWER=1 ;; *) USE_LOWER=0 ;; esac | ||
| case "$sel" in *Digits*) USE_DIGIT=1 ;; *) USE_DIGIT=0 ;; esac | ||
| case "$sel" in *Symbols*) USE_SYMBOL=1 ;; *) USE_SYMBOL=0 ;; esac |
There was a problem hiding this comment.
🔵 P3 (minor) — bc is not part of the standard POSIX core utilities and is frequently missing from minimal Linux distributions, slim container images, and bare environments. If bc is missing, the script will crash under set -e.
We can calculate the entropy bits using standard awk (specifically the built-in log function), which is standard POSIX and guaranteed to be present on all Linux/macOS/Git Bash systems.
[pass 1]
| LC_ALL=C printf '%.0f' "$(echo "$1 * l($2) / l(2)" | bc -l)" | ||
| } | ||
|
|
||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
🟡 P2 (major) — Running od and tr in a subshell for every single byte of the secret is a massive performance bottleneck. Spawning 2 processes per byte (e.g., 64 processes for a 32-character secret, or 256 processes for 128 characters) can make the script take several seconds to execute, especially on systems with slow process creation overhead (such as macOS, WSL, or Git Bash on Windows).
We can optimize this by reading random bytes in batches of size want * 2 in a single od invocation, then processing those bytes from an in-memory array in Bash. Rejection sampling is preserved perfectly, but the number of external processes spawned is reduced to exactly one.
[pass 1]
| umask 077 | ||
| mkdir -p "$(dirname "$SECRET_PATH")" | ||
|
|
||
| while :; do |
There was a problem hiding this comment.
🟡 P2 (major) — In environments with set -euo pipefail active, running chmod 600 "$SECRET_PATH" directly can cause the script to immediately crash and terminate if the target filesystem does not support POSIX permissions (such as shared host mounts in Docker, or NTFS/FAT partitions on Git Bash/WSL/MSYS2).
To ensure parity with how secret-store.sh handles file permissions, we should ignore errors from this command by appending 2>/dev/null || true.
[pass 1]
| rm -f "$SECRET_PATH" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🔴 P1 (blocker) — Passing the plaintext secret as a command-line argument (--entry-text="$(cat "$SECRET_PATH")") exposes it in the system's process table (e.g., via ps aux or /proc/<pid>/cmdline), where any local user or monitoring daemon can read it. This directly violates the codebase's own Absolute Rules (Reglas absolutas) documented in commands/secret-generate.md: "Nunca lo pases como argumento de otro comando. Un argv es legible por cualquier proceso vía /proc y queda en el trace del shell."
To fix this securely without exposing the secret to the process table, we can pipe the secret and metadata into standard input of zenity --text-info. This avoids command line exposure entirely. We also save and check the exit code of zenity so that if the window is closed/cancelled by the user (via X or Esc) or if a GUI error occurs, the script safely aborts instead of spinning in an infinite loop.
[pass 1]
Opening the PR for /secret-generate returned `mergeable=MERGEABLE, 0 failures`
with ZERO checks having examined it. The only entry in the list was
`[code]smith=SKIPPED`.
Not a fluke and not a misconfiguration: `test-hooks.yml` filters on
`paths: hooks/** , scripts/build-rules.mjs`, and a new file under `scripts/`
matches neither. The repo had no lane that reads a shell script at all — while
shipping `secret-store.sh` at 248 lines handling credentials.
"0 failures" and "nothing ran" render identically in the checks list. That is the
whole reason for this file.
A RATCHET ON CHANGED FILES, not a whole-tree gate, and the reason is measured
rather than assumed. `shellcheck -S warning` over the 20 tracked .sh files:
17 clean
3 carrying 9 findings between them:
hooks/cross-cutting/lib/jq-input.sh 4
hooks/cross-cutting/lib/load-config.sh 3
hooks/test-hooks.sh 2
A whole-tree gate would open RED on its own repository on day one, and a gate
that reds the repo it guards is one that gets deleted or routed around inside a
week. Diff-scoped, it is green from the first run and every NEW script is held to
the standard.
The known cost of diff-scoping is stated in the file rather than left to be
discovered: an untouched file never gets re-checked. So a second job runs the
whole tree, prints every finding into the step summary, and FAILS if the count
grows past the 3 recorded — the backlog can shrink but cannot quietly expand.
Both jobs simulated locally, with the negative control:
debt, BASELINE=3 -> passes (failing=3)
debt, BASELINE=2 -> FAILS (the growth check can actually fail)
changed -> collects scripts/secret-generate.sh, shellcheck clean
Two details that would otherwise be silent defects:
`--diff-filter=d` drops deletions. A removed file cannot be linted, and
treating its absence as a failure is how a cleanup PR goes red.
The zero-changed-files path emits an explicit `::notice::` saying it checked
NOTHING. A job that found nothing and a job that looked at nothing must not
read the same — which is the defect this workflow exists for, one level up.
Both SHAs reach the shell through `env:` and are read as `$VAR`, never
interpolated into a `run:` body.
TWO THINGS IN ONE PR, declared rather than left implicit. The feature is
/secret-generate; this is CI governance. They are folded together because the
governance gap was found BY the feature — the PR came back green having been
examined by nothing — and shipping the script under that condition is the thing
the script itself argues against. They share no code and revert independently.
There was a problem hiding this comment.
✅ Approved
Approved — 0 blockers, 3 P3. Confidence: 4.40/5.00.
Walkthrough
main branch directly. GitFlow expects feature branches to target develop before merging into main. This warning is non-blocking and purely informative for release coordination.
Walkthrough
This PR adds the /secret-generate command, completing the CSPRNG-based secret flow for the toolkit. It introduces a highly disciplined Bash script scripts/secret-generate.sh that securely generates random secrets via /dev/urandom and rejection sampling, keeps the secret entirely out of stdout, and integrates a selectable zenity GUI or headless fallback. It also introduces a robust linting ratchet with a Shellcheck CI workflow (.github/workflows/shellcheck.yml) to prevent shell scripting debt from growing.
Files Reviewed
scripts/secret-generate.sh(Entropy calculations, rejection sampling, CLI option parsing, and GUI window creation).github/workflows/shellcheck.yml(CI changed-file diff filter, whole-tree debt check, and rating baseline)commands/secret-generate.md(Command documentation, Absolute rules, Spanish localization guidance)README.md,CHANGELOG.md,package.json,.claude-plugin/*metadata (Command registry integration and version bumping)
Safety Rationale
The code is exceptionally well-engineered with robust security controls. It strictly adheres to the toolkit's design constraints: the generated secret is written directly to the memory-backed staging file (SECRET_PATH) and never assigned to top-level variables, printed to stdout, or passed as sub-process arguments. It also enforces umask 077 and permission 600 for file storage. Rejection sampling ensures perfectly uniform distribution, and /dev/urandom provides strong CSPRNG entropy.
Approved — 0 blockers, 3 P3.
🔵 P3 — Minor
scripts/secret-generate.sh:46— 🔵 P3 (minor) — Headless positive class selection flags (like--digits) have no effect because all four classes default to1by default. Specifying positive flags should restrict selection to only those classes instead of keeping all of them active. To fix this, consider initializing the variables as empty strings, tracking if any positive flags were set during CLI parsing, and then resolving defaults dynamically (e.g., defaulting to0if other positive flags are specified, or1if none are).
[pass 1]
scripts/secret-generate.sh:90— 🔵 P3 (minor) — Passing--lengthas the final option with no value causes an unbound variable error and an immediate exit underset -uwhen accessing$2. Checking the remaining argument count before shifting ensures a graceful error message instead of a crash.
[pass 1]
scripts/secret-generate.sh:119— 🔵 P3 (minor) — Thebcutility is missing in some minimal environments (such as stripped-down Docker containers), which will causeentropy_bitsto fail and crash the script due toset -eo pipefail. Checking forbcavailability and falling back to a safe placeholder prevents runtime failures.
[pass 1]
Total findings: 3 business context (3 total)
| echo "/run/user/$(id -u)" | ||
| elif [ -n "${TMPDIR:-}" ]; then | ||
| echo "${TMPDIR%/}" | ||
| elif [ -n "${TEMP:-}" ]; then |
There was a problem hiding this comment.
🔵 P3 (minor) — Headless positive class selection flags (like --digits) have no effect because all four classes default to 1 by default. Specifying positive flags should restrict selection to only those classes instead of keeping all of them active. To fix this, consider initializing the variables as empty strings, tracking if any positive flags were set during CLI parsing, and then resolving defaults dynamically (e.g., defaulting to 0 if other positive flags are specified, or 1 if none are).
[pass 1]
| --lower) USE_LOWER=1; GUI=0; shift ;; | ||
| --digits) USE_DIGIT=1; GUI=0; shift ;; | ||
| --symbols) USE_SYMBOL=1; GUI=0; shift ;; | ||
| --no-symbols) USE_SYMBOL=0; GUI=0; shift ;; |
There was a problem hiding this comment.
🔵 P3 (minor) — Passing --length as the final option with no value causes an unbound variable error and an immediate exit under set -u when accessing $2. Checking the remaining argument count before shifting ensures a graceful error message instead of a crash.
[pass 1]
| # human-chosen password the hash IS dictionary-attackable and publishing | ||
| # it leaks; that case is exactly what a salt exists for. This is safe only | ||
| # because the value came from a CSPRNG with the entropy printed below. | ||
| [ -s "$SECRET_PATH" ] || { echo "no secret staged." >&2; exit 1; } |
There was a problem hiding this comment.
🔵 P3 (minor) — The bc utility is missing in some minimal environments (such as stripped-down Docker containers), which will cause entropy_bits to fail and crash the script due to set -eo pipefail. Checking for bc availability and falling back to a safe placeholder prevents runtime failures.
[pass 1]
1 participant
The gap
The toolkit had three secret commands and all three assumed the value already
existed somewhere:
/secret-input/secret-use/secret-clearNothing made one. And the obvious substitute defeats the other three:
openssl rand -base64 32prints the value on stdout, so when an agent runs itthat stdout is in the agent's context and in the session transcript on disk — a
place the value cannot be recalled from. The whole reason
/secret-inputreachesfor a GUI dialog is to keep a typed secret out of exactly those places.
The invariant, and why it is structural
Not "be careful not to print it" — the script is arranged so it cannot. The value
is written straight into the same staged file
/secret-inputuses(
generate … >"$SECRET_PATH"), is never assigned to a variable at the script'stop scope, and is shown only in a GUI window a human is looking at. Every later
line re-reads the file for metadata, so there is exactly one place it lives.
stdout carries this and nothing else:
Controls, run in both directions
A verification that cannot fail proves nothing, so the leak check was run against
a deliberately sabotaged copy first:
echoappended)600, on tmpfs--fingerprinton the same value twice--fingerprintafter regenerating--fingerprintwith nothing stagedshellcheck scripts/secret-generate.shThree decisions worth reading rather than inferring
Rejection sampling, not
byte % n. 256 is not a multiple of most alphabetsizes, so the modulo makes the first
256 mod nsymbols more likely — for the89-symbol default, 78 of them at about 1.35x. Small, real, avoidable for the
price of a loop.
/dev/urandomthroughout, never$RANDOM, which is a 15-bitgenerator seeded from pid and time.
The symbol class excludes the quote, backtick and backslash characters. A
generated secret gets pasted into shell one-liners, JSON and YAML, and a backtick
inside a double-quoted shell string is command substitution. A generator that can
emit a value its consumer cannot safely carry fails at 3am, in a way that looks
like the consumer's bug.
--fingerprintanswers the question a secret store cannot. A store will notread a value back — that is the point of it — so after loading the same value
into two of them nothing confirms they match. A truncated SHA-256 settles it with
neither end printing anything sensitive; same move as an SSH key fingerprint.
It is labelled in the code as what it is: a comparison handle, not encryption
and not protection. SHA-256 is one-way, so it cannot be turned back into the
secret — and equally it cannot make an exposed secret safe. And the case where it
is unsafe is named rather than left implicit: for a human-chosen password the
hash IS dictionary-attackable, which is precisely what a salt exists for. It is
safe here only because the input came from a CSPRNG with the entropy printed
above.
Also carried over
The control
/secret-inputalready applies to typed input: it refuses to stagean empty value. An empty staged secret is indistinguishable from a good one to
every consumer downstream, and that is how an empty value reaches a store and
then reads as configured.
Known limits, stated rather than discovered
zenity(Linux). Without it the script says so and names theheadless form — it does not silently fall back to a different alphabet or
length.
secret-store.ps1exists for native Windows;secret-generateis bash only, so Windows without Git Bash/WSL/MSYS2 has no/secret-generate. The command doc says this plainly and points at/secret-inputinstead, rather than suggesting a workaround.have()uses the plaincommand -vprobe form already in this repo'sMakefile. The reverse redirect order is flagged by shellcheck as SC2069, andcommand -vwrites nothing to stderr on a miss, so nothing diagnostic is lostat that one call site.
Files
scripts/secret-generate.sh755commands/secret-generate.mdREADME.mdCHANGELOG.md1.43.0package.json·.claude-plugin/plugin.json·.claude-plugin/marketplace.json1.42.0→1.43.0Created by Claude Code on behalf of @lapc506