fleet/fleetd
3
0
Fork 0
Code Issues 78 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

scripts/config-edit.sh — an auditable seam for editing the live fleetd.yaml, with the reload verdict read back #635

New Issue
Open
opened 2026-10-01 17:13:02 +02:00 by ltms · 10 comments
ltms commented 2026-10-01 17:13:02 +02:00
Owner
Copy Link
Copy Source

Why

fleetd/fleetd.yaml is gitignored and holds the live fleet's settings. Today the lead cannot edit it: the command classifier refuses a direct Edit on it, and that refusal is correct, because a bad edit reaches a daemon that is already serving.

The operator chose this option over simply allow-listing Edit on the file (decision taken 2026-10-01). The reason is the same reason scripts/redeploy-fleetd.sh exists: one auditable command the operator allow-lists once, which also carries the safety a raw file write does not have — a backup, a parse check before the file is installed, and the daemon's own reload verdict read back afterwards.

So the point of this script is not convenience. It is that an edit to a live config is not finished when the bytes are written. It is finished when the daemon has said what it did with them.

What the daemon actually says — measured 2026-10-01 on 158a2a8

ConfigRef re-reads the file, validates it, and then logs exactly one verdict. These are the five strings ConfigRef.Outcome.summary() can produce (ConfigRef.java:371-391):

config reload refused — <error message>
config reload refused — these keys cannot change under a running daemon: <keys>. Restart fleetd to apply them.
config reloaded
config reloaded; these changes need a restart to take effect: <keys>
config reloaded; partially live — <key: detail | key: detail>

A parse or validation failure logs a different line first, before any summary (ConfigRef.java:425):

config reload from <path> refused, keeping the running config: <message>

Note the em dash — in the summary strings. It is a real multi-byte character in the source; match on the stable prefix config reload refused instead of trying to match the dash.

In fleetd/fleetd.out a verdict looks like this (a real line):

17:33:54.616 INFO  [config-watcher] d.l.fleet.config.ConfigRef - config reloaded

Three facts that drive the design:

  1. A cold-key change throws away the whole reload. ConfigRef.java:429-434 returns before current.set(fresh). So the running config keeps every old value, not just the cold one. The cold keys are bind, herdrSocket, memberHerdrSocket, broker, auth.
  2. A deferred-key change IS applied — current.set(fresh) runs — but the running objects that already read it keep the old value until a restart. So "needs a restart" is a success with a follow-up, not a failure.
  3. The watcher polls every 10 seconds. The live daemon logs config watch: fleetd.yaml re-read when it changes (every 10s). Read the interval from that line rather than hardcoding 10; fall back to 10 if it is absent.

Scope — two files, both under scripts/

Follow the conventions of the existing pair scripts/redeploy-fleetd.sh and scripts/test-redeploy-fleetd.sh. Read both before you start. Match their option parsing, their die/logging helpers and their overall shape — this script sits beside them and will be allow-listed the same way.

1. scripts/config-edit.sh

scripts/config-edit.sh --check
scripts/config-edit.sh --set <yq-path>=<value> [--set ...]
scripts/config-edit.sh --from <candidate.yaml>
scripts/config-edit.sh --dry-run --set <yq-path>=<value>
scripts/config-edit.sh --restore

Overrides, needed so the test can drive it without a daemon:
--config <path> (default fleetd/fleetd.yaml), --log <path> (default fleetd/fleetd.out), --wait-seconds <n> (default: 4× the watch interval, so 40).

yq v4 is installed (v4.52.2 measured) — use it for --set. Use yq to parse-check too.

The sequence for an edit:

  1. Refuse early if the config file is missing.
  2. Take the log marker before anything else — the current line count of the log file. Every later log read starts after that marker. redeploy-fleetd.sh already does this; copy the approach. Without it an old refusal from hours ago reads as this edit's verdict.
  3. Probe whether the daemon is listening on its port. Probe the socket, do not use pgrep — pgrep/ps -f print argv and argv holds NAME=value, so they are a credential channel. Get the port from the config's bind value; fall back to 8765.
  4. Back up the config to a timestamped file. Print the path. Keep backups; do not prune.
  5. Build the candidate in a temp file — never edit the live file in place. For --set, copy then yq -i; for --from, use the given file.
  6. Parse-check the candidate before it goes anywhere near the live path. A candidate that does not parse is rejected here, with nothing installed.
  7. Install it atomically: write the temp file, then mv it over the live path.
  8. Wait for a verdict line after the marker, polling until --wait-seconds elapses.
  9. Report, and exit per the table below.

Exit codes — four outcomes, and they must stay four

0  applied; verdict read; clean
3  applied; verdict read; needs a restart (deferred or split keys named)
4  REFUSED by the daemon; backup restored; the restore was itself confirmed
5  CANNOT TELL — no verdict line within the wait window

This is the part most likely to be got wrong, so it is the acceptance criterion below. Exit 5 is not a failure and it is not a success. It means the edit is on disk and nobody knows what the daemon did with it — the daemon is down, or the watcher is stalled. A script that reports "cannot tell" as either "refused" or "applied" is worse than one that does not check at all, because the caller then acts on a verdict that was never read.

In state 5 the script must not restore. It leaves the edit in place and prints the backup path and the literal --restore command to undo it. The reasoning: an applied-but-unobserved edit is visible and recoverable; a silent revert of a good edit is invisible, and the caller would go on believing their change is live.

In state 4 the script restores the backup and then waits for a second verdict to confirm the restore reloaded cleanly. If that confirmation does not arrive, say so plainly — do not report a restore you did not observe.

Never print a secret

fleetd.yaml keeps credentials out by indirection (broker.uriEnv, gitTokenEnv), but the script must not rely on that staying true.

  • Never print the whole file, and never print the whole candidate.
  • --dry-run and the change report print a diff, and that diff must be redacted: pipe it through sed -E 's#://[^@]*@#://<redacted>@#g' — the g is required — and mask the value on any line whose key matches TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY.
  • Never print the value of an environment variable. To report presence, use [ -n "$V" ] && echo "set (${#V} chars)". Never write ${V:-x} — that form expands the secret.

--check is read-only. It reports: config path and whether it parses, the daemon's listening state, the watch interval it found, the last verdict line in the log, the newest backup, and yq's version. It changes nothing and exits 0 even when the daemon is down (it is a report, not a gate).

2. scripts/test-config-edit.sh

A self-contained test, in the style of scripts/test-redeploy-fleetd.sh. It drives config-edit.sh against a fixture config and a fixture log inside a throwaway temp directory it creates and removes. It must not touch fleetd/fleetd.yaml, must not touch fleetd/fleetd.out, and must not start, stop or contact any daemon.

There is no daemon in the test, so the test plays the daemon: it appends the verdict line it wants to the fixture log while the script is waiting. Run config-edit.sh in the background, append the line, wait for the exit code.

Acceptance criteria

Each of these is a property that must hold under a change, and each must be proved by a run whose output you paste into your reply. "The function exists" and "the flag is parsed" are not acceptance; a construct cannot satisfy these.

  1. Refusal restores the file byte-for-byte. Start from a fixture config. Run --set to change a value. Have the fixture log receive config reload refused — these keys cannot change under a running daemon: broker. Restart fleetd to apply them. Assert: exit code is 4, and cmp reports the config file identical to the backup taken at the start. Not "similar" — cmp silent, exit 0.
  2. A clean reload keeps the edit. Same start. Feed config reloaded. Assert exit 0 and that yq reads the new value back from the live fixture path.
  3. A deferred reload is distinguished from a clean one. Feed config reloaded; these changes need a restart to take effect: profiles. Assert exit 3, not 0, and that the output names profiles and tells the reader a restart is needed.
  4. Silence is its own answer. Feed the log nothing. Assert exit 5, that the edited value is still on disk (the script did not restore), and that the output contains the --restore command. Then run that printed command and assert the file matches the original backup.
  5. A broken candidate never reaches the live path. Run --from with a file that is not valid YAML. Assert a non-zero exit, and that the live fixture config is unchanged and that no new verdict was expected — the script must fail before installing. Prove the file is untouched with cmp.
  6. The marker works. Put an old config reload refused — something ancient line in the fixture log before running a --set that then gets a config reloaded. Assert exit 0. A script that scans the whole log instead of the part after its marker fails this and reports 4.
  7. The redaction holds. Put uri: amqp://user:hunter2@host/vhost in the fixture config, change something else, and assert the script's full output contains neither hunter2 nor user:. This is the one test whose failure is a security defect rather than a bug.
  8. scripts/test-config-edit.sh passes end to end, and bash -n (or zsh -n, matching whichever shebang you use) is clean on both scripts. If shellcheck is installed, it is clean too; if it is not installed, say so rather than claiming it passed.

Hard constraints

  • You cannot see fleetd/fleetd.yaml. It is gitignored, and your worktree does not have it. Do not look for it, do not try to create one, and do not report on its contents. Build and test entirely against fixtures. The lead runs the live probe.
  • Never restart, stop or redeploy the daemon. You are talking to the fleet through it. Do not run scripts/redeploy-fleetd.sh. Reading it is expected; running it is not.
  • Never print the value of any environment variable, in the scripts or in your own commands.
  • Stage files explicitly. Never git add -A. Do not commit .mcp.json, opencode.json, .autoenv or anything under wiki/.
  • Your worktree has stub copies of .mcp.json, opencode.json and .autoenv — not the repo's real files. An edit to one cannot be committed and will not tell you so. Read the list with git config --worktree --get-all fleet.neutralizedConfig.
  • If a command is denied, stop and report it. Do not find another route to the same effect. A denied mvn clean install is not an invitation to rm -rf target.
  • No Java changes are expected. If you believe one is needed, ask before writing it.

Also report

Beyond the unit: this script's shape — back up, check, install, read the verdict back, restore on refusal — is not specific to fleetd.yaml. If you spot another place in scripts/ that writes a live file without reading back what consumed it, name it in one line. Do not fix it.

## Why `fleetd/fleetd.yaml` is gitignored and holds the live fleet's settings. Today the lead cannot edit it: the command classifier refuses a direct `Edit` on it, and that refusal is correct, because a bad edit reaches a daemon that is already serving. The operator chose this option over simply allow-listing `Edit` on the file (decision taken 2026-10-01). The reason is the same reason `scripts/redeploy-fleetd.sh` exists: **one auditable command the operator allow-lists once**, which also carries the safety a raw file write does not have — a backup, a parse check before the file is installed, and the daemon's own reload verdict read back afterwards. So the point of this script is not convenience. It is that **an edit to a live config is not finished when the bytes are written.** It is finished when the daemon has said what it did with them. ## What the daemon actually says — measured 2026-10-01 on `158a2a8` `ConfigRef` re-reads the file, validates it, and then logs exactly one verdict. These are the five strings `ConfigRef.Outcome.summary()` can produce (`ConfigRef.java:371-391`): ``` config reload refused — <error message> config reload refused — these keys cannot change under a running daemon: <keys>. Restart fleetd to apply them. config reloaded config reloaded; these changes need a restart to take effect: <keys> config reloaded; partially live — <key: detail | key: detail> ``` A parse or validation failure logs a *different* line first, before any summary (`ConfigRef.java:425`): ``` config reload from <path> refused, keeping the running config: <message> ``` Note the em dash `—` in the summary strings. It is a real multi-byte character in the source; match on the stable prefix `config reload refused` instead of trying to match the dash. In `fleetd/fleetd.out` a verdict looks like this (a real line): ``` 17:33:54.616 INFO [config-watcher] d.l.fleet.config.ConfigRef - config reloaded ``` **Three facts that drive the design:** 1. **A cold-key change throws away the whole reload.** `ConfigRef.java:429-434` returns *before* `current.set(fresh)`. So the running config keeps every old value, not just the cold one. The cold keys are `bind`, `herdrSocket`, `memberHerdrSocket`, `broker`, `auth`. 2. **A deferred-key change IS applied** — `current.set(fresh)` runs — but the running objects that already read it keep the old value until a restart. So "needs a restart" is a success with a follow-up, not a failure. 3. **The watcher polls every 10 seconds.** The live daemon logs `config watch: fleetd.yaml re-read when it changes (every 10s)`. Read the interval from that line rather than hardcoding 10; fall back to 10 if it is absent. ## Scope — two files, both under `scripts/` Follow the conventions of the existing pair `scripts/redeploy-fleetd.sh` and `scripts/test-redeploy-fleetd.sh`. Read both before you start. Match their option parsing, their `die`/logging helpers and their overall shape — this script sits beside them and will be allow-listed the same way. ### 1. `scripts/config-edit.sh` ``` scripts/config-edit.sh --check scripts/config-edit.sh --set <yq-path>=<value> [--set ...] scripts/config-edit.sh --from <candidate.yaml> scripts/config-edit.sh --dry-run --set <yq-path>=<value> scripts/config-edit.sh --restore ``` Overrides, needed so the test can drive it without a daemon: `--config <path>` (default `fleetd/fleetd.yaml`), `--log <path>` (default `fleetd/fleetd.out`), `--wait-seconds <n>` (default: 4× the watch interval, so 40). `yq` v4 is installed (v4.52.2 measured) — use it for `--set`. Use `yq` to parse-check too. **The sequence for an edit:** 1. Refuse early if the config file is missing. 2. **Take the log marker before anything else** — the current line count of the log file. Every later log read starts after that marker. `redeploy-fleetd.sh` already does this; copy the approach. Without it an old refusal from hours ago reads as this edit's verdict. 3. Probe whether the daemon is listening on its port. **Probe the socket, do not use `pgrep`** — `pgrep`/`ps -f` print argv and argv holds `NAME=value`, so they are a credential channel. Get the port from the config's `bind` value; fall back to 8765. 4. Back up the config to a timestamped file. Print the path. Keep backups; do not prune. 5. Build the candidate in a temp file — never edit the live file in place. For `--set`, copy then `yq -i`; for `--from`, use the given file. 6. **Parse-check the candidate** before it goes anywhere near the live path. A candidate that does not parse is rejected here, with nothing installed. 7. Install it atomically: write the temp file, then `mv` it over the live path. 8. Wait for a verdict line after the marker, polling until `--wait-seconds` elapses. 9. Report, and exit per the table below. ### Exit codes — four outcomes, and they must stay four ``` 0 applied; verdict read; clean 3 applied; verdict read; needs a restart (deferred or split keys named) 4 REFUSED by the daemon; backup restored; the restore was itself confirmed 5 CANNOT TELL — no verdict line within the wait window ``` **This is the part most likely to be got wrong, so it is the acceptance criterion below.** Exit 5 is not a failure and it is not a success. It means the edit is on disk and nobody knows what the daemon did with it — the daemon is down, or the watcher is stalled. A script that reports "cannot tell" as either "refused" or "applied" is worse than one that does not check at all, because the caller then acts on a verdict that was never read. In state 5 the script **must not restore.** It leaves the edit in place and prints the backup path and the literal `--restore` command to undo it. The reasoning: an applied-but-unobserved edit is visible and recoverable; a silent revert of a *good* edit is invisible, and the caller would go on believing their change is live. In state 4 the script restores the backup and then **waits for a second verdict to confirm the restore reloaded cleanly.** If that confirmation does not arrive, say so plainly — do not report a restore you did not observe. ### Never print a secret `fleetd.yaml` keeps credentials out by indirection (`broker.uriEnv`, `gitTokenEnv`), but the script must not rely on that staying true. - Never print the whole file, and never print the whole candidate. - `--dry-run` and the change report print a diff, and that diff must be redacted: pipe it through `sed -E 's#://[^@]*@#://<redacted>@#g'` — **the `g` is required** — and mask the value on any line whose key matches `TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY`. - Never print the value of an environment variable. To report presence, use `[ -n "$V" ] && echo "set (${#V} chars)"`. Never write `${V:-x}` — that form expands the secret. `--check` is read-only. It reports: config path and whether it parses, the daemon's listening state, the watch interval it found, the last verdict line in the log, the newest backup, and `yq`'s version. It changes nothing and exits 0 even when the daemon is down (it is a report, not a gate). ### 2. `scripts/test-config-edit.sh` A self-contained test, in the style of `scripts/test-redeploy-fleetd.sh`. It drives `config-edit.sh` against a **fixture** config and a **fixture** log inside a throwaway temp directory it creates and removes. It must not touch `fleetd/fleetd.yaml`, must not touch `fleetd/fleetd.out`, and must not start, stop or contact any daemon. There is no daemon in the test, so the test plays the daemon: it appends the verdict line it wants to the fixture log while the script is waiting. Run `config-edit.sh` in the background, append the line, wait for the exit code. ## Acceptance criteria Each of these is a property that must hold **under a change**, and each must be proved by a run whose output you paste into your reply. "The function exists" and "the flag is parsed" are not acceptance; a construct cannot satisfy these. 1. **Refusal restores the file byte-for-byte.** Start from a fixture config. Run `--set` to change a value. Have the fixture log receive `config reload refused — these keys cannot change under a running daemon: broker. Restart fleetd to apply them.` Assert: exit code is **4**, and `cmp` reports the config file identical to the backup taken at the start. Not "similar" — `cmp` silent, exit 0. 2. **A clean reload keeps the edit.** Same start. Feed `config reloaded`. Assert exit **0** and that `yq` reads the *new* value back from the live fixture path. 3. **A deferred reload is distinguished from a clean one.** Feed `config reloaded; these changes need a restart to take effect: profiles`. Assert exit **3**, not 0, and that the output names `profiles` and tells the reader a restart is needed. 4. **Silence is its own answer.** Feed the log *nothing*. Assert exit **5**, that the edited value is **still on disk** (the script did not restore), and that the output contains the `--restore` command. Then run that printed command and assert the file matches the original backup. 5. **A broken candidate never reaches the live path.** Run `--from` with a file that is not valid YAML. Assert a non-zero exit, and that the live fixture config is unchanged **and** that no new verdict was expected — the script must fail before installing. Prove the file is untouched with `cmp`. 6. **The marker works.** Put an *old* `config reload refused — something ancient` line in the fixture log **before** running a `--set` that then gets a `config reloaded`. Assert exit **0**. A script that scans the whole log instead of the part after its marker fails this and reports 4. 7. **The redaction holds.** Put `uri: amqp://user:hunter2@host/vhost` in the fixture config, change something else, and assert the script's full output contains neither `hunter2` nor `user:`. This is the one test whose failure is a security defect rather than a bug. 8. `scripts/test-config-edit.sh` passes end to end, and `bash -n` (or `zsh -n`, matching whichever shebang you use) is clean on both scripts. If `shellcheck` is installed, it is clean too; if it is not installed, say so rather than claiming it passed. ## Hard constraints - **You cannot see `fleetd/fleetd.yaml`.** It is gitignored, and your worktree does not have it. Do not look for it, do not try to create one, and do not report on its contents. Build and test entirely against fixtures. The lead runs the live probe. - **Never restart, stop or redeploy the daemon.** You are talking to the fleet through it. Do not run `scripts/redeploy-fleetd.sh`. Reading it is expected; running it is not. - **Never print the value of any environment variable**, in the scripts or in your own commands. - Stage files explicitly. **Never `git add -A`.** Do not commit `.mcp.json`, `opencode.json`, `.autoenv` or anything under `wiki/`. - Your worktree has stub copies of `.mcp.json`, `opencode.json` and `.autoenv` — not the repo's real files. An edit to one cannot be committed and will not tell you so. Read the list with `git config --worktree --get-all fleet.neutralizedConfig`. - **If a command is denied, stop and report it.** Do not find another route to the same effect. A denied `mvn clean install` is not an invitation to `rm -rf target`. - No Java changes are expected. If you believe one is needed, ask before writing it. ## Also report Beyond the unit: **this script's shape — back up, check, install, read the verdict back, restore on refusal — is not specific to `fleetd.yaml`.** If you spot another place in `scripts/` that writes a live file without reading back what consumed it, name it in one line. Do **not** fix it.
ltms commented 2026-10-01 17:37:00 +02:00
Author
Owner
Copy Link
Copy Source

Lead verification of PR #636 — and one defect to fix

PR #636, 854 insertions, scripts/ only, measured with git diff --stat origin/main...pr636 (three dots — against a moving main a two-dot diff falsely reports deletions).

The suite has teeth — six mutations, all killed

The implementer ran its own suite and nothing else, so the suite's own soundness was unproven. I mutated config-edit.sh six times, each time a line the implementer did not write for that test, and ran the whole suite:

Mutation Suite Failed on
return 5 → return 0 (cannot-tell reported as clean) exit 1 criterion 4 — expected 5, got 0
drop restore_and_confirm "$backup" exit 1 criterion 1 — must restore the config byte for byte
return 3 → return 0 (deferred reported as clean) exit 1 criterion 3 — expected 3, got 0
scan the whole log instead of after the marker exit 1 criterion 6 — expected 0, got 4
redact() made a pass-through exit 1 criterion 7 — must NOT contain [hunter2], but it does
parse_check() always returns 0 exit 1 criterion 5 — broken candidate must never reach the live fixture

Every kill is behavioural — a real exit code or a real cmp on file contents. None is a source.contains match, which is what this family of test is usually faked with. config-edit.sh was byte-identical to pristine after each cycle.

One note on my own instrument. My first run of the marker mutation reported SURVIVOR. It was a false survivor: my sed replacement contained a |, which was also my sed delimiter, so the command failed and the file was never mutated — a pristine file passing its own suite reads exactly like a surviving mutant. I redid it with awk plus a control that asserts the file actually changed before the suite runs, and it killed. A mutation harness needs its own did-it-change control, or a broken mutation reads as a clean survival.

The implementer's own caveat: measured, and it is fine

--set writes every value as a YAML string via strenv() (deliberately, to keep a raw value out of the yq expression). The implementer flagged that it could not check whether the daemon's loader tolerates weight: "7" on a numeric field. I checked it, against the real ObjectMapper(new YAMLFactory()) from the built jar:

QUOTED -> OK  autoCompactWindow=300000 (Integer)  weight=7
BARE   -> OK  autoCompactWindow=300000 (Integer)  weight=7
quoted bool    -> OK  bool=false
quoted garbage -> REFUSED: InvalidFormatException: Cannot deserialize value of type
                  `java.lang.Integer` from String "abc": not a valid `java.lang.Integer` value

Jackson coerces a quoted scalar. So strenv() is safe for numbers and booleans, and a genuinely bad value is refused by the daemon — which the script then classifies as refused, restores, and reports as exit 4. The chain holds. No change needed for this.

The defect — a forgotten value reports success

--set accepts an empty value, and the result is a silently nulled field reported as a clean success.

Measured end to end against a fixture:

$ config-edit.sh --config cfg.yaml --set .profiles.sonnet.maxLoad=
== result: applied cleanly
exit=0

$ grep maxLoad cfg.yaml
    maxLoad: ""

and the daemon's own loader on that value:

quoted empty -> OK  int=null

So: --set .profiles.sonnet.maxLoad= — a plausible typo, the value simply forgotten — erases the field, the daemon accepts it, the reload is clean, and the script reports "applied cleanly", exit 0. The caller asked to change one number and deleted it instead, with a success report.

apply_set_pairs only checks that the argument contains = (*=*), which an empty value satisfies. The guard needs to be on the value, not the shape.

This matters more than it looks, because a null config value widens rather than empties. A nulled maxLoad falls to its default, so capacity silently moves instead of failing. That is the exact class of silent degradation this script exists to catch, and right now the script would hand it back as a success.

Fix wanted

In apply_set_pairs, refuse an empty value and say what to pass instead. Two cases must stay distinguishable, because they need opposite handling:

  • --set .a.b= — refuse. Exit non-zero, install nothing, and name the likely cause ("value is empty — did you mean --set .a.b=null to clear it, or quote an intentional empty string?").
  • Clearing a key on purpose — give it an explicit spelling and write a real YAML null, not "". "" and null are not the same thing to the loader and must not collapse onto one form.

Add one acceptance criterion in the same style as the existing eight: a forgotten value exits non-zero and cmp proves the live fixture untouched; and the explicit clear produces a bare null, not "".

Not blocking, noted only

apply_set_pairs splices path into the yq expression raw. strenv() protects the value but not the path, so a crafted path could evaluate as an expression. The caller here is the lead, and a malformed path makes yq fail with nothing installed, so the blast radius is small. Worth a path shape check (^[A-Za-z0-9_.\[\]"-]+$) if this ever takes input from anywhere but a human.

Also measured, no action

  • bash -n clean on both scripts; I ran the suite 1× myself (implementer ran it 3×, identical output).
  • shellcheck is genuinely not installed — the implementer was right to say so rather than claim a pass.
  • bash 3.2 is safe. SETS=() is declared before set -euo pipefail matters, so ${#SETS[@]} does not trip set -u on macOS's /bin/bash 3.2. I ran --check and --restore under 3.2: exit 0 and a clean named failure. env bash resolves to Homebrew bash 5.3.9 here, so the normal path is 5.x, but 3.2 works too.
## Lead verification of PR #636 — and one defect to fix PR #636, `854` insertions, `scripts/` only, measured with `git diff --stat origin/main...pr636` (three dots — against a moving `main` a two-dot diff falsely reports deletions). ### The suite has teeth — six mutations, all killed The implementer ran its own suite and nothing else, so the suite's own soundness was unproven. I mutated `config-edit.sh` six times, each time a line the implementer did **not** write for that test, and ran the whole suite: | Mutation | Suite | Failed on | |---|---|---| | `return 5` → `return 0` (cannot-tell reported as clean) | exit 1 | criterion 4 — `expected 5, got 0` | | drop `restore_and_confirm "$backup"` | exit 1 | criterion 1 — `must restore the config byte for byte` | | `return 3` → `return 0` (deferred reported as clean) | exit 1 | criterion 3 — `expected 3, got 0` | | scan the whole log instead of after the marker | exit 1 | criterion 6 — `expected 0, got 4` | | `redact()` made a pass-through | exit 1 | criterion 7 — `must NOT contain [hunter2], but it does` | | `parse_check()` always returns 0 | exit 1 | criterion 5 — `broken candidate must never reach the live fixture` | Every kill is behavioural — a real exit code or a real `cmp` on file contents. None is a `source.contains` match, which is what this family of test is usually faked with. `config-edit.sh` was byte-identical to pristine after each cycle. **One note on my own instrument.** My first run of the marker mutation reported SURVIVOR. It was a false survivor: my `sed` replacement contained a `|`, which was also my `sed` delimiter, so the command failed and **the file was never mutated** — a pristine file passing its own suite reads exactly like a surviving mutant. I redid it with `awk` plus a control that asserts the file actually changed before the suite runs, and it killed. A mutation harness needs its own did-it-change control, or a broken mutation reads as a clean survival. ### The implementer's own caveat: measured, and it is fine `--set` writes every value as a YAML string via `strenv()` (deliberately, to keep a raw value out of the `yq` expression). The implementer flagged that it could not check whether the daemon's loader tolerates `weight: "7"` on a numeric field. I checked it, against the real `ObjectMapper(new YAMLFactory())` from the built jar: ``` QUOTED -> OK autoCompactWindow=300000 (Integer) weight=7 BARE -> OK autoCompactWindow=300000 (Integer) weight=7 quoted bool -> OK bool=false quoted garbage -> REFUSED: InvalidFormatException: Cannot deserialize value of type `java.lang.Integer` from String "abc": not a valid `java.lang.Integer` value ``` Jackson coerces a quoted scalar. So `strenv()` is safe for numbers and booleans, and a genuinely bad value is refused by the daemon — which the script then classifies as `refused`, restores, and reports as exit 4. The chain holds. **No change needed for this.** ### The defect — a forgotten value reports success `--set` accepts an **empty** value, and the result is a silently nulled field reported as a clean success. Measured end to end against a fixture: ``` $ config-edit.sh --config cfg.yaml --set .profiles.sonnet.maxLoad= == result: applied cleanly exit=0 $ grep maxLoad cfg.yaml maxLoad: "" ``` and the daemon's own loader on that value: ``` quoted empty -> OK int=null ``` So: `--set .profiles.sonnet.maxLoad=` — a plausible typo, the value simply forgotten — erases the field, the daemon accepts it, the reload is **clean**, and the script reports **"applied cleanly", exit 0**. The caller asked to change one number and deleted it instead, with a success report. `apply_set_pairs` only checks that the argument *contains* `=` (`*=*`), which an empty value satisfies. The guard needs to be on the value, not the shape. This matters more than it looks, because **a null config value widens rather than empties.** A nulled `maxLoad` falls to its default, so capacity silently moves instead of failing. That is the exact class of silent degradation this script exists to catch, and right now the script would hand it back as a success. ### Fix wanted In `apply_set_pairs`, refuse an empty value and say what to pass instead. Two cases must stay distinguishable, because they need opposite handling: - `--set .a.b=` — **refuse.** Exit non-zero, install nothing, and name the likely cause ("value is empty — did you mean `--set .a.b=null` to clear it, or quote an intentional empty string?"). - Clearing a key on purpose — give it an explicit spelling and write a real YAML `null`, not `""`. `""` and `null` are not the same thing to the loader and must not collapse onto one form. Add one acceptance criterion in the same style as the existing eight: a forgotten value exits non-zero **and** `cmp` proves the live fixture untouched; and the explicit clear produces a bare `null`, not `""`. ### Not blocking, noted only `apply_set_pairs` splices `path` into the `yq` expression raw. `strenv()` protects the value but not the path, so a crafted path could evaluate as an expression. The caller here is the lead, and a malformed path makes `yq` fail with nothing installed, so the blast radius is small. Worth a path shape check (`^[A-Za-z0-9_.\[\]"-]+$`) if this ever takes input from anywhere but a human. ### Also measured, no action - `bash -n` clean on both scripts; I ran the suite 1× myself (implementer ran it 3×, identical output). - `shellcheck` is genuinely not installed — the implementer was right to say so rather than claim a pass. - **bash 3.2 is safe.** `SETS=()` is declared before `set -euo pipefail` matters, so `${#SETS[@]}` does not trip `set -u` on macOS's `/bin/bash` 3.2. I ran `--check` and `--restore` under 3.2: exit 0 and a clean named failure. `env bash` resolves to Homebrew bash 5.3.9 here, so the normal path is 5.x, but 3.2 works too.
ltms commented 2026-10-01 17:41:49 +02:00
Author
Owner
Copy Link
Copy Source

Second defect, verified by the lead — backups are not gitignored

Found by the backup-lifecycle reviewer, then checked here. Fix this in the same PR (#636) as the empty-value defect.

What is true

backup_config writes the backup beside the live file as fleetd/fleetd.yaml.bak.<ts>.<pid>, and that name is not gitignored. Measured:

$ git check-ignore -v fleetd/fleetd.yaml.bak.20261001T000000Z.123
(exit 1 — NOT ignored)

fleetd/.gitignore ignores fleetd.yaml and bridged.yaml by exact name, plus logs/, target/, *.iml, .idea/, .DS_Store. Nothing matches fleetd.yaml.bak.*. The script's own header says backups are never pruned, so they accumulate in a tracked directory indefinitely.

The whole reason fleetd.yaml is gitignored is that it must never be committed. A backup of it inherits that requirement and does not inherit the rule.

What is NOT true — the reviewer's severity is overstated

The finding was filed as high, on the grounds that the backups hold "plaintext credentials". They do not. I measured the live fleetd/fleetd.yaml:

  • ://user:pass@host style userinfo in a URL: 0 occurrences.
  • Every credential-shaped key with a value is an *Env indirection key holding a variable name, never a value: tokenEnv ×2, gitTokenEnv ×7. No token:, password:, secret: or passwd: key carries a literal.

The hunter2 password in the reviewer's paste came from the reviewer's own fixture, which they had created for the redaction test. They reported a property of their fixture as a property of the live file. That is the easiest mistake to make in this kind of review and it is worth naming, because it changed the severity by a whole grade.

So: medium, not high. A real defect that must be fixed, but not a live secret exposure today.

It still matters without secrets. An un-ignored backup is one git add -A or git add . away from committing this host's live configuration — ports, filesystem paths, configDir locations, model names, credential ids, host allow-lists. And the hazard is forward-looking: the day anyone puts a literal into that file, every historical backup beside it becomes a committable copy, and nothing would warn them.

Fix wanted

Write backups into a gitignored directory, and add the glob as well. Both, not either:

  1. A dedicated directory, e.g. fleetd/.config-backups/, created on demand, with an entry in fleetd/.gitignore. Prefer this over fleetd/logs/ — that one is the CB-505 audit trail and should not get a second purpose.
  2. Also add a fleetd.yaml.bak.* line to fleetd/.gitignore, so a stray backup written the old way, or by an older copy of this script, is still ignored.

A directory beats a glob on its own, because a glob only protects the filename format that exists today. The day someone changes the backup naming, the glob stops matching and nothing fails — whereas a location keeps working.

--restore and restore_command_line must keep working against the new location, and --check's "newest backup" line must still find it.

Acceptance — one more criterion, number 11

  1. A backup is never committable. After a --set run against a fixture, assert that git check-ignore reports the backup path as ignored — or, equivalently, that the backup lands under a path that git status --porcelain does not list as untracked. Then prove --restore still finds and uses it: run the printed restore command and cmp the result against the pre-edit file.

Write this criterion so it would fail today. A criterion that passes on the current code is not pinning the fix. Show it failing before your change and passing after.

Correctly dismissed — no action

The same reviewer noticed that restore_and_confirm always returns 0 and the refused branch always returns 4, even when the confirming verdict never arrives, and judged it documented behaviour rather than a bug. They were right. I tested it — a refusal with deliberate silence afterwards:

   ok    restored from cfg.yaml.bak.20261001T154105Z.8742
   WARN  the restore is on disk, but no confirming verdict line appeared within 3s
   WARN  cannot confirm the restore reloaded cleanly — check fleetd.out by hand
EXIT=4

and cmp confirms the file really is byte-identical to the pristine original. So exit 4 means "refused, and the restore is on disk" — which is true and verified — while the part the script could not observe is reported loudly instead of being folded into the code. That is the right shape. A separate exit code would be a nicety, not a correction, and the warning already names the one action the caller needs to take.

They also checked whether ls -t backup ordering could pick the wrong file on a same-second tie and found no reproducible failure, and said so rather than reporting a hypothesis. That is the right call too.

## Second defect, verified by the lead — backups are not gitignored Found by the backup-lifecycle reviewer, then checked here. **Fix this in the same PR (#636) as the empty-value defect.** ### What is true `backup_config` writes the backup beside the live file as `fleetd/fleetd.yaml.bak.<ts>.<pid>`, and **that name is not gitignored.** Measured: ``` $ git check-ignore -v fleetd/fleetd.yaml.bak.20261001T000000Z.123 (exit 1 — NOT ignored) ``` `fleetd/.gitignore` ignores `fleetd.yaml` and `bridged.yaml` by exact name, plus `logs/`, `target/`, `*.iml`, `.idea/`, `.DS_Store`. Nothing matches `fleetd.yaml.bak.*`. The script's own header says backups are never pruned, so they accumulate in a tracked directory indefinitely. The whole reason `fleetd.yaml` is gitignored is that it must never be committed. A backup of it inherits that requirement and does not inherit the rule. ### What is NOT true — the reviewer's severity is overstated The finding was filed as **high**, on the grounds that the backups hold "plaintext credentials". They do not. I measured the live `fleetd/fleetd.yaml`: - `://user:pass@host` style userinfo in a URL: **0 occurrences**. - Every credential-shaped key with a value is an `*Env` **indirection** key holding a variable *name*, never a value: `tokenEnv` ×2, `gitTokenEnv` ×7. No `token:`, `password:`, `secret:` or `passwd:` key carries a literal. The `hunter2` password in the reviewer's paste came from the reviewer's own fixture, which they had created for the redaction test. **They reported a property of their fixture as a property of the live file.** That is the easiest mistake to make in this kind of review and it is worth naming, because it changed the severity by a whole grade. So: **medium, not high.** A real defect that must be fixed, but not a live secret exposure today. It still matters without secrets. An un-ignored backup is one `git add -A` or `git add .` away from committing this host's live configuration — ports, filesystem paths, `configDir` locations, model names, credential **ids**, host allow-lists. And the hazard is forward-looking: the day anyone puts a literal into that file, every historical backup beside it becomes a committable copy, and nothing would warn them. ### Fix wanted Write backups into a **gitignored directory**, and add the glob as well. Both, not either: 1. A dedicated directory, e.g. `fleetd/.config-backups/`, created on demand, with an entry in `fleetd/.gitignore`. Prefer this over `fleetd/logs/` — that one is the CB-505 audit trail and should not get a second purpose. 2. Also add a `fleetd.yaml.bak.*` line to `fleetd/.gitignore`, so a stray backup written the old way, or by an older copy of this script, is still ignored. A directory beats a glob on its own, because a glob only protects the filename format that exists today. The day someone changes the backup naming, the glob stops matching and nothing fails — whereas a location keeps working. `--restore` and `restore_command_line` must keep working against the new location, and `--check`'s "newest backup" line must still find it. ### Acceptance — one more criterion, number 11 11. **A backup is never committable.** After a `--set` run against a fixture, assert that `git check-ignore` reports the backup path as ignored — or, equivalently, that the backup lands under a path that `git status --porcelain` does not list as untracked. Then prove `--restore` still finds and uses it: run the printed restore command and `cmp` the result against the pre-edit file. Write this criterion so it would **fail today**. A criterion that passes on the current code is not pinning the fix. Show it failing before your change and passing after. ### Correctly dismissed — no action The same reviewer noticed that `restore_and_confirm` always returns 0 and the `refused` branch always returns 4, even when the confirming verdict never arrives, and judged it documented behaviour rather than a bug. **They were right.** I tested it — a refusal with deliberate silence afterwards: ``` ok restored from cfg.yaml.bak.20261001T154105Z.8742 WARN the restore is on disk, but no confirming verdict line appeared within 3s WARN cannot confirm the restore reloaded cleanly — check fleetd.out by hand EXIT=4 ``` and `cmp` confirms the file really is byte-identical to the pristine original. So exit 4 means "refused, and the restore is on disk" — which is true and verified — while the part the script could not observe is reported loudly instead of being folded into the code. That is the right shape. A separate exit code would be a nicety, not a correction, and the warning already names the one action the caller needs to take. They also checked whether `ls -t` backup ordering could pick the wrong file on a same-second tie and found no reproducible failure, and said so rather than reporting a hypothesis. That is the right call too.
ltms commented 2026-10-01 17:45:32 +02:00
Author
Owner
Copy Link
Copy Source

Third defect, verified — every edit silently narrows the config's file mode, one way

Found by the install-path reviewer, reproduced here. Fix in the same PR (#636).

install_candidate does mv -f "$cand" "$live", and $cand comes from mktemp, which creates at mode 0600. The rename carries the temp file's mode onto the live path. Nothing ever puts the old mode back, and cp onto an existing file (as backup_config and restore_and_confirm do) keeps the destination's mode, so a restore does not undo it either.

Measured on a fixture that starts at 644:

before:            644
after --set:       600
after --restore:   600     <- the narrowing survives a restore

The live file is reachable. stat on the real one:

-rw-r--r--  644  fleetd/fleetd.yaml

So the first --set against the live config permanently takes it from 644 to 600, and no later command here brings it back.

Severity medium, and the direction matters: 644 → 600 is a narrowing, so this is not a security hole. fleetd runs as the same user and keeps reading the file fine. It is a correctness defect: a tool asked to change one key must not silently change file metadata as a side effect, and must certainly not do it irreversibly and cumulatively.

Fix wanted

Capture the live file's mode before the backup, and apply it to the candidate before the mv (preferred — then the file never exists at the wrong mode), or chmod it back immediately after. Do the same on the --restore path. If the config does not exist yet, fall back to the system default rather than inventing 644.

Acceptance — criterion 12

  1. The mode survives an edit and a restore. Start a fixture at 644. Run --set, assert stat still reports 644. Run --restore, assert 644 again. Then repeat the whole thing from 600 and assert it stays 600 — the fix must preserve the mode, not hardcode 644. Show this failing on today's code first.

The reviewer's secondary note: structurally true, but I could NOT reproduce it. No criterion.

The same reviewer noted there is no trap anywhere in the script, so a signal delivered while the candidate exists would leave a .config-edit.XXXXXX file beside the live config. They said honestly that they had not timed a kill well enough to reproduce it. I tried to, and I also failed.

Confirmed structurally: grep -c '^[[:space:]]*trap ' returns 0. Every rm -f "$cand" sits on an explicit path. And the temp name is not gitignored either, same as the backup.

Not confirmed behaviourally: 21 timed SIGINTs across three rounds, zero leaks. Two of those rounds were my own broken instruments, which is the part worth recording:

  1. My first 6 kills used delays of 0.08–0.48 milliseconds — the script had not started. I read 6 clean results as evidence.
  2. My next 10 used 0.05–1.0 s and still read clean. A control then showed the "killed" run had exit=0 and the same 12 output lines as an uninterrupted run — the script finishes in ~0.14 s, so every kill landed after it was done. Nothing was ever interrupted.
  3. My attempt to widen the window with 25 --set pairs passed them as a single argument, so the script died at unknown option in 0.015 s, before mktemp. Those runs could not have leaked whatever the code does.

A planted .config-edit.PLANTED was detected by the same check, so the detector itself works. The zeros are explained by the window being a few hundred milliseconds wide, not by the absence of a gap.

So: add the trap, but it is not gated by an acceptance criterion. One line, trap 'rm -f "$cand"' EXIT INT TERM scoped where $cand is live, is cheap and obviously right. I am not asking for a test, because I could not make a leak happen on demand, and a criterion I cannot see fail today is not a test — it would be a line that passes for reasons nobody has established. If you can reproduce a leak, say so and add the criterion; if you cannot either, say that too and we ship the trap as hygiene with the gap named.

Running total for PR #636 — four changes, three with criteria

# Defect Criterion Reproduced
1 --set .a.b= erases the key and reports "applied cleanly" 9, 10 yes
2 backup file is not gitignored 11 yes
3 every edit narrows the mode 644 → 600, irreversibly 12 yes
4 no trap, so a signal can leak the temp file none, deliberately no

Criteria 9–12 must each be shown failing before the fix and passing after. Four criteria, four demonstrated failures, pasted into the reply.

## Third defect, verified — every edit silently narrows the config's file mode, one way Found by the install-path reviewer, reproduced here. **Fix in the same PR (#636).** `install_candidate` does `mv -f "$cand" "$live"`, and `$cand` comes from `mktemp`, which creates at mode `0600`. The rename carries the temp file's mode onto the live path. Nothing ever puts the old mode back, and `cp` onto an existing file (as `backup_config` and `restore_and_confirm` do) keeps the *destination's* mode, so a restore does not undo it either. Measured on a fixture that starts at 644: ``` before: 644 after --set: 600 after --restore: 600 <- the narrowing survives a restore ``` **The live file is reachable.** `stat` on the real one: ``` -rw-r--r-- 644 fleetd/fleetd.yaml ``` So the first `--set` against the live config permanently takes it from 644 to 600, and no later command here brings it back. Severity **medium**, and the direction matters: 644 → 600 is a *narrowing*, so this is not a security hole. `fleetd` runs as the same user and keeps reading the file fine. It is a correctness defect: a tool asked to change one key must not silently change file metadata as a side effect, and must certainly not do it irreversibly and cumulatively. ### Fix wanted Capture the live file's mode **before** the backup, and apply it to the candidate before the `mv` (preferred — then the file never exists at the wrong mode), or `chmod` it back immediately after. Do the same on the `--restore` path. If the config does not exist yet, fall back to the system default rather than inventing 644. ### Acceptance — criterion 12 12. **The mode survives an edit and a restore.** Start a fixture at `644`. Run `--set`, assert `stat` still reports `644`. Run `--restore`, assert `644` again. Then repeat the whole thing from `600` and assert it stays `600` — the fix must *preserve* the mode, not hardcode 644. Show this failing on today's code first. --- ## The reviewer's secondary note: structurally true, but I could NOT reproduce it. No criterion. The same reviewer noted there is **no `trap` anywhere in the script**, so a signal delivered while the candidate exists would leave a `.config-edit.XXXXXX` file beside the live config. They said honestly that they had not timed a kill well enough to reproduce it. I tried to, and I also failed. Confirmed structurally: `grep -c '^[[:space:]]*trap '` returns **0**. Every `rm -f "$cand"` sits on an explicit path. And the temp name is not gitignored either, same as the backup. Not confirmed behaviourally: **21 timed `SIGINT`s across three rounds, zero leaks.** Two of those rounds were my own broken instruments, which is the part worth recording: 1. My first 6 kills used delays of 0.08–0.48 **milliseconds** — the script had not started. I read 6 clean results as evidence. 2. My next 10 used 0.05–1.0 s and still read clean. A control then showed the "killed" run had `exit=0` and **the same 12 output lines as an uninterrupted run** — the script finishes in ~0.14 s, so every kill landed *after* it was done. Nothing was ever interrupted. 3. My attempt to widen the window with 25 `--set` pairs passed them as a single argument, so the script died at `unknown option` in 0.015 s, before `mktemp`. Those runs could not have leaked whatever the code does. A planted `.config-edit.PLANTED` *was* detected by the same check, so the detector itself works. The zeros are explained by the window being a few hundred milliseconds wide, not by the absence of a gap. **So: add the trap, but it is not gated by an acceptance criterion.** One line, `trap 'rm -f "$cand"' EXIT INT TERM` scoped where `$cand` is live, is cheap and obviously right. I am not asking for a test, because I could not make a leak happen on demand, and **a criterion I cannot see fail today is not a test** — it would be a line that passes for reasons nobody has established. If you can reproduce a leak, say so and add the criterion; if you cannot either, say that too and we ship the trap as hygiene with the gap named. --- ## Running total for PR #636 — four changes, three with criteria | # | Defect | Criterion | Reproduced | |---|---|---|---| | 1 | `--set .a.b=` erases the key and reports "applied cleanly" | 9, 10 | yes | | 2 | backup file is not gitignored | 11 | yes | | 3 | every edit narrows the mode 644 → 600, irreversibly | 12 | yes | | 4 | no `trap`, so a signal can leak the temp file | none, deliberately | **no** | Criteria 9–12 must each be shown failing before the fix and passing after. Four criteria, four demonstrated failures, pasted into the reply.
ltms commented 2026-10-01 17:48:15 +02:00
Author
Owner
Copy Link
Copy Source

Fourth defect — this one is in the TEST. Criterion 7 cannot fail.

Two reviewers were blocked by the command classifier on this dimension, both times stopping and reporting rather than routing around it, which is correct. The second refusal was It executes a script from a temporary path whose effects are not shown. So I did this dimension myself. It was never denied to me, and I am saying so rather than quietly swapping the actor.

A surviving mutant

I deleted the redacted diff print on the edit path — the one that runs against the live config:

     say "change (redacted)"
-    diff -u "$backup" "$cand" | redact || true
+    : # MUTATED: the redacted diff is no longer printed at all

So the redaction is never exercised on that path at all. Result:

suite exit: 0
PASS: config-edit acceptance criteria

Criterion 7 — the one whose failure is a security defect rather than a bug — passed with the thing it guards removed.

(Controls: pristine anchor count 1; awk used so no sed delimiter could silently fail; cmp confirmed the file actually changed before the run, and byte-identical again after.)

Why it goes blind

assert_equals 0 "$RUN_RC" "redaction-case reload exit code"
assert_not_contains "hunter2" "$RUN_OUTPUT" "full output must never contain the password"
assert_not_contains "user:"   "$RUN_OUTPUT" "full output must never contain the userinfo"

Both content assertions are negative only. An absence assertion passes just as happily when the subject has left the output entirely as when it was correctly redacted. The exit-code assertion proves the run happened and succeeded; nothing proves the diff was ever printed. So the criterion cannot tell "redacted properly" from "printed nothing", and silently prefers to pass.

The dry-run test does still cover the other diff print, so the redaction is pinned on the dry-run path. The edit path — the one that matters — is not.

Fix — criterion 13, a positive control

Add a loud positive assertion to criterion 7. I verified it passes on today's unmutated code, against a fixture holding uri: amqp://user:hunter2@host/vhost:

contains '<redacted>':  1      <- proof the redaction actually ran on real content
contains 'hunter2':     0      <- the existing negative assertion
contains 'weight':      2      <- proof a diff was printed at all

and the diff the caller sees:

== change (redacted)
--- cfg.yaml.bak.20261001T154747Z.18919
+++ ./.config-edit.jAfZju
@@ -2,4 +2,4 @@
   uri: <redacted>
 profiles:
   sonnet:
  1. The redaction is proved to have run, not merely not to have failed. In criterion 7, additionally assert that $RUN_OUTPUT contains <redacted>, and that it contains the changed key name (so a diff was demonstrably printed). Then show it working as a test: remove the diff -u "$backup" "$cand" | redact line from the edit path, confirm criterion 7 now fails, and restore it.

That last step is the whole point. Today that mutation is a survivor. After the fix it must be a kill, and the reply must show both states.

Apply the same reasoning to the other assert_not_contains uses in the suite, if any share this shape: pair each with something that fails when the content goes missing. An absence assertion without a presence control is a test that gets quieter as the code gets more broken.

PR #636 — five changes now

# Defect Where Criterion Reproduced
1 --set .a.b= erases the key, reports "applied cleanly" script 9, 10 yes
2 backup file is not gitignored script 11 yes
3 mode narrows 644 → 600, irreversibly script 12 yes
4 no trap, temp file can leak script none, on purpose no
5 criterion 7 passes with the redaction removed test 13 yes

Note what the shape of this list says. Defects 1–3 are in the script and were found by reviewing it. Defect 5 is in the test, and the only reason it surfaced is that one reviewer was pointed at the test itself rather than at the code. A suite that killed six of my mutations still contained an assertion that could not fail.

## Fourth defect — this one is in the TEST. Criterion 7 cannot fail. Two reviewers were blocked by the command classifier on this dimension, both times stopping and reporting rather than routing around it, which is correct. The second refusal was `It executes a script from a temporary path whose effects are not shown.` So I did this dimension myself. It was never denied to me, and I am saying so rather than quietly swapping the actor. ### A surviving mutant I deleted the redacted diff print on the **edit** path — the one that runs against the live config: ```diff say "change (redacted)" - diff -u "$backup" "$cand" | redact || true + : # MUTATED: the redacted diff is no longer printed at all ``` So the redaction is never exercised on that path at all. Result: ``` suite exit: 0 PASS: config-edit acceptance criteria ``` **Criterion 7 — the one whose failure is a security defect rather than a bug — passed with the thing it guards removed.** (Controls: pristine anchor count 1; `awk` used so no `sed` delimiter could silently fail; `cmp` confirmed the file actually changed before the run, and byte-identical again after.) ### Why it goes blind ```bash assert_equals 0 "$RUN_RC" "redaction-case reload exit code" assert_not_contains "hunter2" "$RUN_OUTPUT" "full output must never contain the password" assert_not_contains "user:" "$RUN_OUTPUT" "full output must never contain the userinfo" ``` Both content assertions are **negative only**. An absence assertion passes just as happily when the subject has left the output entirely as when it was correctly redacted. The exit-code assertion proves the run happened and succeeded; nothing proves the diff was ever printed. So the criterion cannot tell "redacted properly" from "printed nothing", and silently prefers to pass. The dry-run test does still cover the *other* diff print, so the redaction is pinned on the dry-run path. The edit path — the one that matters — is not. ### Fix — criterion 13, a positive control Add a **loud positive assertion** to criterion 7. I verified it passes on today's unmutated code, against a fixture holding `uri: amqp://user:hunter2@host/vhost`: ``` contains '<redacted>': 1 <- proof the redaction actually ran on real content contains 'hunter2': 0 <- the existing negative assertion contains 'weight': 2 <- proof a diff was printed at all ``` and the diff the caller sees: ``` == change (redacted) --- cfg.yaml.bak.20261001T154747Z.18919 +++ ./.config-edit.jAfZju @@ -2,4 +2,4 @@ uri: <redacted> profiles: sonnet: ``` 13. **The redaction is proved to have run, not merely not to have failed.** In criterion 7, additionally assert that `$RUN_OUTPUT` **contains** `<redacted>`, and that it contains the changed key name (so a diff was demonstrably printed). Then show it working as a test: remove the `diff -u "$backup" "$cand" | redact` line from the edit path, confirm criterion 7 now **fails**, and restore it. That last step is the whole point. Today that mutation is a survivor. After the fix it must be a kill, and the reply must show both states. Apply the same reasoning to the other `assert_not_contains` uses in the suite, if any share this shape: pair each with something that fails when the content goes missing. An absence assertion without a presence control is a test that gets quieter as the code gets more broken. --- ## PR #636 — five changes now | # | Defect | Where | Criterion | Reproduced | |---|---|---|---|---| | 1 | `--set .a.b=` erases the key, reports "applied cleanly" | script | 9, 10 | yes | | 2 | backup file is not gitignored | script | 11 | yes | | 3 | mode narrows 644 → 600, irreversibly | script | 12 | yes | | 4 | no `trap`, temp file can leak | script | none, on purpose | **no** | | 5 | **criterion 7 passes with the redaction removed** | **test** | 13 | yes | Note what the shape of this list says. Defects 1–3 are in the script and were found by reviewing it. Defect 5 is in the **test**, and the only reason it surfaced is that one reviewer was pointed at the test itself rather than at the code. A suite that killed six of my mutations still contained an assertion that could not fail.
ltms commented 2026-10-01 18:16:39 +02:00
Author
Owner
Copy Link
Copy Source

Lead review of 4eb7200 — one new defect, acceptance criterion 14

I checked 4eb7200 myself in a clean copy of the branch. The four open items from the earlier brief are closed and I proved each one. Details at the end. First the one new defect.

Defect 6 — the --restore failure message names a directory the code no longer searches

config-edit.sh:617:

[ -n "$backup" ] || die "no backup found matching ${CONFIG}.bak.* — nothing to restore"

newest_backup was moved to the new layout and now searches the backup directory (config-edit.sh:324):

ls -t "${dir}/${base}".bak.* 2>/dev/null | head -1 || true

So the code looks in <dir>/.config-backups/, but the message prints <dir>/fleetd.yaml.bak.*. The message was not moved with the code.

Why this is worth a fix and not just a nit. The old naming still exists in the wild — that is exactly why fleetd/.gitignore keeps the fleetd.yaml.bak.* glob as a backstop. In that case the message points straight at a file that is really there while saying it found nothing. Measured in a fixture:

=== the message, with an old-style backup sitting right there ===
   FAIL  no backup found matching /tmp/.../w/fleetd.yaml.bak.* — nothing to restore

=== the path the message NAMES exists ===
/tmp/.../w/fleetd.yaml.bak.20260101T000000Z.111
  ^ the glob the message prints DOES match a real file

Exit code is 1, measured with no pipe (a pipe to head hid it as 0).

This is a reporting-only defect: refusing to restore from outside the managed directory is correct behaviour. Only the report lies. Keep the behaviour, fix the message.

What to change

  1. Make the message name the directory that was actually searched, so an operator looking for their backup is sent to the right place.
  2. Because an old-style backup beside the config is a real possibility, the message should also say that a backup written the old way is not used, and give the one-line cp to recover it by hand. Do not add a search fallback — reading backups from outside the managed directory is a behaviour change nobody asked for.

Acceptance criterion 14 — and it is red today

Run --restore against a fixture that has no backup in .config-backups/. Assert the output names the directory that was searched.

I confirmed this assertion fails on 4eb7200 before you write the fix:

=== proposed assertion would be RED today ===
names the searched dir: FAIL (red today)

Add the control the other way too: with a backup present in .config-backups/, --restore must still succeed. That stops the fix turning into a message that is always printed.

The four earlier items — closed, and how I proved it

The whole suite is green in a clean copy of the branch: criteria 1–7, 9–12 plus the 3 extras, exit=0. Criterion 13 is folded into 7.

The mutation that mattered most is now a KILL. Removing diff -u "$backup" "$cand" | redact || true from the edit path (config-edit.sh:579):

MUTANT  exit=1   (KILL)
PRISTINE exit=0

It fails by name at criterion 7: the redaction must be PROVEN to have run on real content, not merely absent: missing [<redacted>]. Before this commit the same mutation was a survivor. Criterion 7 is the one whose failure is a security defect, so this was the gate.

Both controls ran on every mutation: cmp proved the file really changed (otherwise the harness prints INSTRUMENT BROKEN, not SURVIVOR), and bash -n proved the mutant still parses, so a syntax error cannot masquerade as a kill.

The two new criteria also kill their own defects:

Mutation Result
drop the chmod in apply_mode KILL
hardcode chmod 644 instead of preserving KILL
write backups beside the config again KILL

The gitignore rules work, with a negative control:

fleetd/.gitignore:14:.config-backups/   fleetd/.config-backups/fleetd.yaml.bak.X   rc=0
fleetd/.gitignore:15:fleetd.yaml.bak.*  fleetd/fleetd.yaml.bak.2026...            rc=0
scripts/config-edit.sh                                                            rc=1  (correctly NOT ignored)

The scope expansion was the right call

Implementing criteria 11, 12 and 13 from comments 17655, 17657 and 17659 was correct. A newer ticket comment beats the brief — that is the rule, and following it is what it is for. No concern here.

Defect 4 (the temp-file trap) — still no criterion, and that stays

I could not reproduce a leak either, and I now know why my first two attempts proved nothing: kill -INT <pid> does not interrupt the script. Bash holds the trap until the foreground sleep returns, so the run finished normally and the probe read as a clean negative. My control and my interrupted run printed byte-identical output — the probe never acted.

Signalling the process group, the way a terminal Ctrl-C does, works. With that method I can report something new: the trap behaves correctly.

control:    exit=0    reaches PHASE-2, PHASE-3
interrupt:  exit=130  stops at PHASE-1, trap fires, never reaches the install

So the trap cleans up and the script stops. It does not resume into the install with a candidate it just deleted, which was my worry when I read the diff. The trap fires twice under an interrupt (once for INT, once for EXIT); that is harmless, because rm -f on an already-removed file is a no-op.

No criterion is added for this. The window where CAND is set runs from mktemp to the install and is a fraction of a second, so a test would have to slow the script down artificially to hit it. That tests a modified script, not this one. A criterion nobody has seen fail is not a test.

The global CAND instead of a function-local was the right choice, for the reason given: an EXIT trap that re-fires after a local $cand is out of scope trips set -u.

## Lead review of `4eb7200` — one new defect, acceptance criterion 14 I checked `4eb7200` myself in a clean copy of the branch. **The four open items from the earlier brief are closed and I proved each one.** Details at the end. First the one new defect. ### Defect 6 — the `--restore` failure message names a directory the code no longer searches `config-edit.sh:617`: ```bash [ -n "$backup" ] || die "no backup found matching ${CONFIG}.bak.* — nothing to restore" ``` `newest_backup` was moved to the new layout and now searches the backup directory (`config-edit.sh:324`): ```bash ls -t "${dir}/${base}".bak.* 2>/dev/null | head -1 || true ``` So the code looks in `<dir>/.config-backups/`, but the message prints `<dir>/fleetd.yaml.bak.*`. The message was not moved with the code. **Why this is worth a fix and not just a nit.** The old naming still exists in the wild — that is exactly why `fleetd/.gitignore` keeps the `fleetd.yaml.bak.*` glob as a backstop. In that case the message points straight at a file that is really there while saying it found nothing. Measured in a fixture: ``` === the message, with an old-style backup sitting right there === FAIL no backup found matching /tmp/.../w/fleetd.yaml.bak.* — nothing to restore === the path the message NAMES exists === /tmp/.../w/fleetd.yaml.bak.20260101T000000Z.111 ^ the glob the message prints DOES match a real file ``` Exit code is `1`, measured with no pipe (a pipe to `head` hid it as `0`). This is a reporting-only defect: refusing to restore from outside the managed directory is correct behaviour. Only the report lies. Keep the behaviour, fix the message. ### What to change 1. Make the message name the directory that was actually searched, so an operator looking for their backup is sent to the right place. 2. Because an old-style backup beside the config is a real possibility, the message should also say that a backup written the old way is not used, and give the one-line `cp` to recover it by hand. Do not add a search fallback — reading backups from outside the managed directory is a behaviour change nobody asked for. ### Acceptance criterion 14 — and it is red today Run `--restore` against a fixture that has **no** backup in `.config-backups/`. Assert the output names the directory that was searched. I confirmed this assertion fails on `4eb7200` before you write the fix: ``` === proposed assertion would be RED today === names the searched dir: FAIL (red today) ``` Add the control the other way too: with a backup present in `.config-backups/`, `--restore` must still succeed. That stops the fix turning into a message that is always printed. --- ## The four earlier items — closed, and how I proved it The whole suite is green in a clean copy of the branch: criteria 1–7, 9–12 plus the 3 extras, `exit=0`. Criterion 13 is folded into 7. **The mutation that mattered most is now a KILL.** Removing `diff -u "$backup" "$cand" | redact || true` from the edit path (`config-edit.sh:579`): ``` MUTANT exit=1 (KILL) PRISTINE exit=0 ``` It fails by name at criterion 7: `the redaction must be PROVEN to have run on real content, not merely absent: missing [<redacted>]`. Before this commit the same mutation was a survivor. Criterion 7 is the one whose failure is a security defect, so this was the gate. Both controls ran on every mutation: `cmp` proved the file really changed (otherwise the harness prints `INSTRUMENT BROKEN`, not `SURVIVOR`), and `bash -n` proved the mutant still parses, so a syntax error cannot masquerade as a kill. The two new criteria also kill their own defects: | Mutation | Result | |---|---| | drop the `chmod` in `apply_mode` | KILL | | hardcode `chmod 644` instead of preserving | KILL | | write backups beside the config again | KILL | The gitignore rules work, with a negative control: ``` fleetd/.gitignore:14:.config-backups/ fleetd/.config-backups/fleetd.yaml.bak.X rc=0 fleetd/.gitignore:15:fleetd.yaml.bak.* fleetd/fleetd.yaml.bak.2026... rc=0 scripts/config-edit.sh rc=1 (correctly NOT ignored) ``` ### The scope expansion was the right call Implementing criteria 11, 12 and 13 from comments 17655, 17657 and 17659 was correct. A newer ticket comment beats the brief — that is the rule, and following it is what it is for. No concern here. ### Defect 4 (the temp-file trap) — still no criterion, and that stays I could not reproduce a leak either, and I now know why my first two attempts proved nothing: `kill -INT <pid>` does not interrupt the script. Bash holds the trap until the foreground `sleep` returns, so the run finished normally and the probe read as a clean negative. My control and my interrupted run printed byte-identical output — the probe never acted. Signalling the **process group**, the way a terminal Ctrl-C does, works. With that method I can report something new: the trap behaves correctly. ``` control: exit=0 reaches PHASE-2, PHASE-3 interrupt: exit=130 stops at PHASE-1, trap fires, never reaches the install ``` So the trap cleans up and the script stops. It does not resume into the install with a candidate it just deleted, which was my worry when I read the diff. The trap fires twice under an interrupt (once for `INT`, once for `EXIT`); that is harmless, because `rm -f` on an already-removed file is a no-op. **No criterion is added for this.** The window where `CAND` is set runs from `mktemp` to the install and is a fraction of a second, so a test would have to slow the script down artificially to hit it. That tests a modified script, not this one. A criterion nobody has seen fail is not a test. The global `CAND` instead of a function-local was the right choice, for the reason given: an `EXIT` trap that re-fires after a local `$cand` is out of scope trips `set -u`.
ltms commented 2026-10-01 18:18:48 +02:00
Author
Owner
Copy Link
Copy Source

Addendum to comment 17664 — also refresh PR #636's description

Small, and part of the same round as criterion 14. PR #636's body still describes the first commit, so the merge record would carry two things that are no longer true.

1. The --set quoting caveat is settled — remove it, do not re-open it. The body says:

I did not test whether fleetd's config loader tolerates a quoted-string number on a genuinely numeric field like weight — that would need a real daemon or a FleetConfig-level test, both out of this script-only ticket's scope.

That was a fair thing to flag, and it has since been checked. Against the real ObjectMapper(new YAMLFactory()) from the built jar:

"300000" -> Integer
"false"  -> Boolean
"abc"    -> InvalidFormatException

So writing values as quoted strings via strenv() is correct and stays. A bad value is refused by the daemon, classified as refused, the backup is restored, and the script exits 4. The chain holds end to end. Replace the caveat with that result, so the next reader does not re-litigate a closed question.

2. The counts are stale. The body says "all 8 acceptance criteria plus 3 extra checks". It is now criteria 1–7 and 9–12, with 13 folded into criterion 7, plus the 3 extras — and criterion 14 once you add it. Say what the suite actually runs.

Leave the rest of the body as it is. In particular keep the note that shellcheck was not available and was not run: that is the honest form and it is still true on this host.

No code change in this addendum — description only.

### Addendum to comment 17664 — also refresh PR #636's description Small, and part of the same round as criterion 14. PR #636's body still describes the first commit, so the merge record would carry two things that are no longer true. **1. The `--set` quoting caveat is settled — remove it, do not re-open it.** The body says: > I did not test whether fleetd's config loader tolerates a quoted-string number on a genuinely numeric field like `weight` — that would need a real daemon or a FleetConfig-level test, both out of this script-only ticket's scope. That was a fair thing to flag, and it has since been checked. Against the real `ObjectMapper(new YAMLFactory())` from the built jar: ``` "300000" -> Integer "false" -> Boolean "abc" -> InvalidFormatException ``` So writing values as quoted strings via `strenv()` is **correct and stays**. A bad value is refused by the daemon, classified as `refused`, the backup is restored, and the script exits 4. The chain holds end to end. Replace the caveat with that result, so the next reader does not re-litigate a closed question. **2. The counts are stale.** The body says "all 8 acceptance criteria plus 3 extra checks". It is now criteria 1–7 and 9–12, with 13 folded into criterion 7, plus the 3 extras — and criterion 14 once you add it. Say what the suite actually runs. Leave the rest of the body as it is. In particular keep the note that `shellcheck` was not available and was not run: that is the honest form and it is still true on this host. No code change in this addendum — description only.
ltms commented 2026-10-01 18:32:41 +02:00
Author
Owner
Copy Link
Copy Source

Defect 7 — redact() leaks a multi-line value, and the output looks redacted

Found while reviewing 096f08c before merging. Criterion 14 is correct and I proved it (details at the end). This is a new defect, and it is in criterion 7's property — the one the ticket calls a security defect rather than a bug. It needs fixing before this merges.

What happens

redact() masks a line only when that line itself starts with a key. A YAML block scalar puts the value on the following lines, so the key line is masked and the value flows straight through.

Reproduced with the real script, a normal --dry-run, no mutation:

broker:
  token: <redacted>
    FAKELEAK-BLOCK-SCALAR
-  nearby: x
+  nearby: y
   passphrase: FAKELEAK-PASSPHRASE

FAKELEAK-BLOCK-SCALAR: >>> LEAKED <<<
FAKELEAK-PASSPHRASE:   >>> LEAKED <<<

Look at the shape of that output. token: <redacted> sits directly above the value it was supposed to hide. A reader skimming it sees the redaction marker and concludes the line was handled. An incomplete redactor that prints a reassuring marker is worse than one that prints nothing, because it stops the reader looking.

Two separate causes in that one paste:

  1. Structural, and the important one. A masked key's continuation lines are not masked. Any |, |-, > or >- value leaks in full.
  2. Name-list completeness. passphrase matches none of TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY, so it is never even considered. PASSWORD|PASSWD does not cover it.

How reachable this is — measured, not assumed

Not reachable with today's live config: every credential there is *Env indirection holding a variable name, and there are no inline secrets. But the ticket is explicit that the redactor must not depend on that staying true, and --from <candidate.yaml> accepts any file an operator hands it.

The path in is narrow and ordinary: diff -u prints the changed hunk plus three lines of context, so the secret leaks when it falls inside that window. My first attempt did not leak, because the secret sat further than three lines from the edit. Editing a key next to it leaked immediately. This matters for writing the test — see below.

Acceptance criterion 15

Put a block scalar under a secret-looking key in the fixture, change a key adjacent to it, and assert the full output contains neither the block-scalar value nor the key line's value.

The fixture design is the whole test here. If the edited key is more than three lines from the secret, the secret never enters the diff and the assertion passes while proving nothing — it would be green today, before any fix. So:

  1. Assert the edited line and the secret are in the same diff hunk. Simplest way: assert the output does contain the block scalar's key line, as a positive control, before asserting the value is absent. Without that control this criterion is a barrier by hope.
  2. Confirm the assertion is red on 096f08c before you write the fix, and say so in your reply. I have shown it is.

Then add a passphrase case. Keep it separate from the block-scalar case so one failing does not hide the other.

The fix

  1. Continuation lines. When a key line is masked, mask every following line indented deeper than that key, until the indentation returns to the key's level or less. Remember the diff prefix (+, -, space) does not count toward indentation — strip it before measuring, or a + line will be misread.
  2. Add passphrase to the pattern.

On the second one, be honest in the comment: a name list can never be complete, so it is a backstop. The structural fix is the one that holds, because it does not need to know the key's name to protect the value. Do not rewrite the redactor into a YAML parser — the two changes above are bounded and enough.

Also report, do not fix

One line each: does any other place in the script print content derived from the config or the candidate without going through redact? I am asking about the paths, not the key list.

Criterion 14 — verified, and correct

Your fix is message-only and newest_backup/backup_dir_for are untouched, as instructed. The suite is green at 096f08c in a clean copy (14 criteria + 3 extras, exit=0).

Mutation results, each with both controls (cmp proved the file changed, bash -n proved the mutant still parses):

Mutation Result
put the stale ${CONFIG}.bak.* path back in the message KILL, fails by name at criterion 14
make restore_mode always take the not-found branch KILL

The first one fails with exactly the right message: the not-found message must name the directory actually searched, not the old beside-the-config glob: missing [.config-backups/fleetd.yaml.bak.*].

Worth recording, because it cost me two bad readings first: my own harness reported INVALID BASH and then SURVIVOR before these results. Both were my instrument, not your code. I had replaced one line of a multi-line die, leaving the continuation dangling; and I had mutated the newest_backup call at line 521, which is in --check, not the one at line 616 in restore_mode. A mutation aimed at the wrong function reads as a clean survivor, and it is the reassuring answer. Your direction-2 proof in isolation and my M-2 in the full suite agree once aimed correctly — in the full suite criterion 4 catches it first.

The stale-message survey is the right answer and the -h|--help range check was a good addition to it.

## Defect 7 — `redact()` leaks a multi-line value, and the output looks redacted Found while reviewing `096f08c` before merging. Criterion 14 is correct and I proved it (details at the end). This is a new defect, and it is in criterion 7's property — the one the ticket calls a security defect rather than a bug. **It needs fixing before this merges.** ### What happens `redact()` masks a line only when that line itself starts with a key. A YAML block scalar puts the value on the *following* lines, so the key line is masked and the value flows straight through. Reproduced with the real script, a normal `--dry-run`, no mutation: ``` broker: token: <redacted> FAKELEAK-BLOCK-SCALAR - nearby: x + nearby: y passphrase: FAKELEAK-PASSPHRASE ``` ``` FAKELEAK-BLOCK-SCALAR: >>> LEAKED <<< FAKELEAK-PASSPHRASE: >>> LEAKED <<< ``` Look at the shape of that output. `token: <redacted>` sits directly above the value it was supposed to hide. A reader skimming it sees the redaction marker and concludes the line was handled. **An incomplete redactor that prints a reassuring marker is worse than one that prints nothing**, because it stops the reader looking. Two separate causes in that one paste: 1. **Structural, and the important one.** A masked key's continuation lines are not masked. Any `|`, `|-`, `>` or `>-` value leaks in full. 2. **Name-list completeness.** `passphrase` matches none of `TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY`, so it is never even considered. `PASSWORD|PASSWD` does not cover it. ### How reachable this is — measured, not assumed Not reachable with today's live config: every credential there is `*Env` indirection holding a variable *name*, and there are no inline secrets. But the ticket is explicit that the redactor must not depend on that staying true, and `--from <candidate.yaml>` accepts any file an operator hands it. The path in is narrow and ordinary: `diff -u` prints the changed hunk plus three lines of context, so the secret leaks when it falls inside that window. My first attempt did **not** leak, because the secret sat further than three lines from the edit. Editing a key next to it leaked immediately. This matters for writing the test — see below. ### Acceptance criterion 15 Put a block scalar under a secret-looking key in the fixture, change a key **adjacent to it**, and assert the full output contains neither the block-scalar value nor the key line's value. **The fixture design is the whole test here.** If the edited key is more than three lines from the secret, the secret never enters the diff and the assertion passes while proving nothing — it would be green today, before any fix. So: 1. Assert the edited line and the secret are in the same diff hunk. Simplest way: assert the output *does* contain the block scalar's key line, as a positive control, before asserting the value is absent. Without that control this criterion is a barrier by hope. 2. Confirm the assertion is **red on `096f08c`** before you write the fix, and say so in your reply. I have shown it is. Then add a `passphrase` case. Keep it separate from the block-scalar case so one failing does not hide the other. ### The fix 1. **Continuation lines.** When a key line is masked, mask every following line indented deeper than that key, until the indentation returns to the key's level or less. Remember the diff prefix (`+`, `-`, space) does not count toward indentation — strip it before measuring, or a `+` line will be misread. 2. **Add `passphrase`** to the pattern. On the second one, be honest in the comment: a name list can never be complete, so it is a backstop. The structural fix is the one that holds, because it does not need to know the key's name to protect the value. Do not rewrite the redactor into a YAML parser — the two changes above are bounded and enough. ### Also report, do not fix One line each: does any other place in the script print content derived from the config or the candidate without going through `redact`? I am asking about the *paths*, not the key list. --- ## Criterion 14 — verified, and correct Your fix is message-only and `newest_backup`/`backup_dir_for` are untouched, as instructed. The suite is green at `096f08c` in a clean copy (14 criteria + 3 extras, `exit=0`). Mutation results, each with both controls (`cmp` proved the file changed, `bash -n` proved the mutant still parses): | Mutation | Result | |---|---| | put the stale `${CONFIG}.bak.*` path back in the message | **KILL**, fails by name at criterion 14 | | make `restore_mode` always take the not-found branch | **KILL** | The first one fails with exactly the right message: `the not-found message must name the directory actually searched, not the old beside-the-config glob: missing [.config-backups/fleetd.yaml.bak.*]`. Worth recording, because it cost me two bad readings first: my own harness reported `INVALID BASH` and then `SURVIVOR` before these results. Both were my instrument, not your code. I had replaced one line of a multi-line `die`, leaving the continuation dangling; and I had mutated the `newest_backup` call at line 521, which is in `--check`, not the one at line 616 in `restore_mode`. A mutation aimed at the wrong function reads as a clean survivor, and it is the reassuring answer. Your direction-2 proof in isolation and my M-2 in the full suite agree once aimed correctly — in the full suite criterion 4 catches it first. The stale-message survey is the right answer and the `-h|--help` range check was a good addition to it.
ltms commented 2026-10-01 18:33:29 +02:00
Author
Owner
Copy Link
Copy Source

Addendum to comment 17670 — one sentence in PR #636's body describes redact and will go stale

Your PR description patch is right: the strenv() caveat now reads as settled with the ObjectMapper results, and the criterion list is accurate. Thank you.

One line in it needs to move with the defect 7 fix. The body currently says:

redact, which blanks any scheme://user:pass@host userinfo and masks the whole value on any line whose key looks like TOKEN/SECRET/PASSWORD/PASSWD/CREDENTIAL/URI/_KEY.

After the fix that is no longer the whole rule, and more importantly it is the sentence a future reader will trust when deciding whether the diff output is safe to paste somewhere. Update it to say what the redactor actually does: the userinfo rewrite, the key-name backstop, and that a masked key's deeper-indented continuation lines are masked too.

Say in that same sentence that the key-name list is a backstop and cannot be complete. A reader who knows the list is partial will look; one who thinks it is exhaustive will not. That is the whole lesson of defect 7 — the old output printed <redacted> directly above a leaked value, and the marker is what stopped it being noticed.

No code change in this addendum beyond what 17670 already asks for — description only.

### Addendum to comment 17670 — one sentence in PR #636's body describes `redact` and will go stale Your PR description patch is right: the `strenv()` caveat now reads as settled with the `ObjectMapper` results, and the criterion list is accurate. Thank you. One line in it needs to move with the defect 7 fix. The body currently says: > `redact`, which blanks any `scheme://user:pass@host` userinfo and masks the whole value on any line whose key looks like TOKEN/SECRET/PASSWORD/PASSWD/CREDENTIAL/URI/_KEY. After the fix that is no longer the whole rule, and more importantly it is the sentence a future reader will trust when deciding whether the diff output is safe to paste somewhere. Update it to say what the redactor actually does: the userinfo rewrite, the key-name backstop, **and** that a masked key's deeper-indented continuation lines are masked too. Say in that same sentence that the key-name list is a backstop and cannot be complete. A reader who knows the list is partial will look; one who thinks it is exhaustive will not. That is the whole lesson of defect 7 — the old output printed `<redacted>` directly above a leaked value, and the marker is what stopped it being noticed. No code change in this addendum beyond what 17670 already asks for — description only.
ltms commented 2026-10-01 18:36:14 +02:00
Author
Owner
Copy Link
Copy Source

Defect 8 — a failing --set echoes its own value, unredacted. Fix this in the same round as criterion 15.

I ran the survey I asked you for, because its output becomes your finding and I should not hand you a conclusion I have not measured. I found two unredacted paths. One is defect 8 below and belongs in this round. The other I am taking off your plate — see the end.

Only two places print config-derived content, and both already go through redact: config-edit.sh:579 (the edit's change report) and :608 (--dry-run). That part is correct. The leaks are elsewhere.

The defect

apply_set_pairs echoes $kv — the whole path=value the operator typed — in its failure messages at lines 391 and 395. Measured:

$ config-edit.sh --dry-run --set '.broker.["bad=FAKELEAK-SETVALUE' ...
   FAIL  yq could not apply --set '.broker.["bad=FAKELEAK-SETVALUE' — nothing was installed. ...

FAKELEAK-SETVALUE: >>> ECHOED UNREDACTED <<<

Line 378 (--set expects <yq-path>=<value>) is safe — it only fires when there is no =, so there is no value. Line 384 (the empty-value refusal) is safe by definition. Lines 391 and 395 are the two that matter, and both are failure paths.

Why a failure path is the worst place for this

The operator typed the value, so this leaks nothing they do not already know. That is not the risk. The risk is where the text goes next: this fleet pastes command output into tickets, PRs and fleet_reply bodies constantly, and a failure is exactly when someone copies the output to ask for help. A secret that was safe in a terminal becomes a secret in a ticket.

The fix

In those two messages, print the path and not the value. The path is what the operator needs to fix their command; the value they already have. Something like --set '.broker.password=<value>'. Do not route $kv through redact — it is not key: value shaped, so redact would pass it straight through and give you a false sense of coverage. That is the same mistake as defect 7.

Acceptance criterion 16

Run a --set whose yq expression fails, with a recognisable value. Assert the full output contains the path and does not contain the value.

Confirm it is red before the fix — I have shown it is. Add the positive control: assert the path is present, so the criterion cannot pass by the message disappearing entirely.

Not yours — I am filing the second path as its own ticket

$VERDICT_LINE is printed raw at lines 441, 442, 465, 469, 473 and 515. That is the daemon's own verdict text, and a refusal reads config reload refused — <error message>. If the loader's message quotes the offending value, a refused edit prints it.

I am not asking you to fix that, and you should not. Two reasons. I cannot verify it without a live daemon, which is mine to run and explicitly not yours. And masking the daemon's own refusal text could hide the one thing that explains the refusal, so it is a design decision about how refusals are reported rather than a bug with an obvious fix. I will file it separately and decide it with the measurement in hand.

Stay inside criterion 15 and criterion 16. If you spot anything else, name it in one line and do not fix it.

## Defect 8 — a failing `--set` echoes its own value, unredacted. Fix this in the same round as criterion 15. I ran the survey I asked you for, because its output becomes your finding and I should not hand you a conclusion I have not measured. I found two unredacted paths. **One is defect 8 below and belongs in this round. The other I am taking off your plate** — see the end. Only two places print config-derived content, and both already go through `redact`: `config-edit.sh:579` (the edit's change report) and `:608` (`--dry-run`). That part is correct. The leaks are elsewhere. ### The defect `apply_set_pairs` echoes `$kv` — the whole `path=value` the operator typed — in its failure messages at lines 391 and 395. Measured: ``` $ config-edit.sh --dry-run --set '.broker.["bad=FAKELEAK-SETVALUE' ... FAIL yq could not apply --set '.broker.["bad=FAKELEAK-SETVALUE' — nothing was installed. ... FAKELEAK-SETVALUE: >>> ECHOED UNREDACTED <<< ``` Line 378 (`--set expects <yq-path>=<value>`) is safe — it only fires when there is no `=`, so there is no value. Line 384 (the empty-value refusal) is safe by definition. **Lines 391 and 395 are the two that matter**, and both are failure paths. ### Why a failure path is the worst place for this The operator typed the value, so this leaks nothing they do not already know. That is not the risk. The risk is where the text goes next: this fleet pastes command output into tickets, PRs and `fleet_reply` bodies constantly, and a failure is exactly when someone copies the output to ask for help. A secret that was safe in a terminal becomes a secret in a ticket. ### The fix In those two messages, print the **path** and not the value. The path is what the operator needs to fix their command; the value they already have. Something like `--set '.broker.password=<value>'`. Do not route `$kv` through `redact` — it is not `key: value` shaped, so `redact` would pass it straight through and give you a false sense of coverage. That is the same mistake as defect 7. ### Acceptance criterion 16 Run a `--set` whose `yq` expression fails, with a recognisable value. Assert the full output contains the **path** and does **not** contain the value. Confirm it is red before the fix — I have shown it is. Add the positive control: assert the path *is* present, so the criterion cannot pass by the message disappearing entirely. --- ## Not yours — I am filing the second path as its own ticket `$VERDICT_LINE` is printed raw at lines 441, 442, 465, 469, 473 and 515. That is the daemon's own verdict text, and a refusal reads `config reload refused — <error message>`. If the loader's message quotes the offending value, a refused edit prints it. **I am not asking you to fix that, and you should not.** Two reasons. I cannot verify it without a live daemon, which is mine to run and explicitly not yours. And masking the daemon's own refusal text could hide the one thing that explains the refusal, so it is a design decision about how refusals are reported rather than a bug with an obvious fix. I will file it separately and decide it with the measurement in hand. Stay inside criterion 15 and criterion 16. If you spot anything else, name it in one line and do not fix it.
ltms commented 2026-10-01 18:53:20 +02:00
Author
Owner
Copy Link
Copy Source

Lead, after verifying d7f94ca: one wording fix, then this merges. No new code.

d7f94ca is good. I re-ran your suite in a pristine copy of the branch (git archive + git init,
no worktree, no daemon) and it passed all 16 criteria plus the 3 extras, exit 0. I read the diff
myself. Criteria 15a, 15b and 16 are accepted, and your criterion 15a comment — placing the edit
next to the block scalar on purpose, and running the positive control first — is exactly right.

Do not touch redact()'s behaviour. What follows is two sentences of prose.

While verifying, I measured a case your fix does not cover, and filed it as #639: the
continuation masking only works while the masked key line is itself inside the printed hunk.
diff -u prints three lines of context, so a block scalar's body routinely appears without its
key — and then nothing is masked. I reproduced it through the real script with --dry-run, with a
positive control first proving the body was inside the window. A blank line inside a block scalar
loses the anchor the same way.

This does not block the merge and it is not a regression — the pre-fix code leaked those cases
too, so your commit is a strict improvement. It is also latent, not live: today's fleetd.yaml has
5 block scalars and all 5 sit under non-secret keys.

The one thing to change

Two sentences currently claim more than the code does.

  1. scripts/config-edit.sh, in your comment above redact():

    This needs no knowledge of the key's name and so protects a block scalar under any masked key,
    present or future.

    Make it say that the masking holds while the masked key line is itself in the printed hunk,
    and that diff -u's three-line context window can deliver a block scalar's body without its key
    — in which case nothing is masked. Point at #639.

  2. PR #636's body, the same claim:

    the indentation-based continuation masking is what holds regardless of the key's name.

    Same correction, same pointer to #639.

Keep both short. The reason this is worth a commit rather than a follow-up: an incomplete redactor
that claims to be complete is the defect #635 is about, and a future session will read that
comment and trust it.

Acceptance

  • Both sentences name the anchor condition and reference #639.
  • Zero change to redact(), to any other function, or to the test suite.
  • bash -n scripts/config-edit.sh clean; the suite still exits 0.
  • Commit and push to worker/config-edit-seam-ca8dc1-1.

Then fleet_reply with the commit sha and the two new sentences quoted, so I can check them
without re-reading the file. Your last turn ended with no fleet_reply — the ticket, the push
and the PR body all landed, so nothing was lost, but I had to reconstruct your result from the
commit instead of reading it. Please end this one with the reply.

**Lead, after verifying `d7f94ca`: one wording fix, then this merges. No new code.** `d7f94ca` is good. I re-ran your suite in a pristine copy of the branch (`git archive` + `git init`, no worktree, no daemon) and it passed all 16 criteria plus the 3 extras, exit 0. I read the diff myself. Criteria 15a, 15b and 16 are accepted, and your criterion 15a comment — placing the edit next to the block scalar on purpose, and running the positive control first — is exactly right. **Do not touch `redact()`'s behaviour.** What follows is two sentences of prose. While verifying, I measured a case your fix does not cover, and filed it as **#639**: the continuation masking only works while the masked key line is itself inside the printed hunk. `diff -u` prints three lines of context, so a block scalar's body routinely appears without its key — and then nothing is masked. I reproduced it through the real script with `--dry-run`, with a positive control first proving the body was inside the window. A blank line inside a block scalar loses the anchor the same way. **This does not block the merge and it is not a regression** — the pre-fix code leaked those cases too, so your commit is a strict improvement. It is also latent, not live: today's `fleetd.yaml` has 5 block scalars and all 5 sit under non-secret keys. ### The one thing to change Two sentences currently claim more than the code does. 1. `scripts/config-edit.sh`, in your comment above `redact()`: > This needs no knowledge of the key's name and so protects a block scalar under any masked key, > present or future. Make it say that the masking holds **while the masked key line is itself in the printed hunk**, and that `diff -u`'s three-line context window can deliver a block scalar's body without its key — in which case nothing is masked. Point at #639. 2. PR #636's body, the same claim: > the indentation-based continuation masking is what holds regardless of the key's name. Same correction, same pointer to #639. Keep both short. The reason this is worth a commit rather than a follow-up: an incomplete redactor that *claims* to be complete is the defect #635 is about, and a future session will read that comment and trust it. ### Acceptance - Both sentences name the anchor condition and reference #639. - Zero change to `redact()`, to any other function, or to the test suite. - `bash -n scripts/config-edit.sh` clean; the suite still exits 0. - Commit and push to `worker/config-edit-seam-ca8dc1-1`. Then `fleet_reply` with the commit sha and the two new sentences quoted, so I can check them without re-reading the file. **Your last turn ended with no `fleet_reply`** — the ticket, the push and the PR body all landed, so nothing was lost, but I had to reconstruct your result from the commit instead of reading it. Please end this one with the reply.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#635
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?