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

probe-member-credentials.sh has no test harness at all, and its new arity message is off by one on an empty parse #519

New Issue
Closed
opened 2026-09-12 06:10:37 +02:00 by ltms · 1 comment
ltms commented 2026-09-12 06:10:37 +02:00
Owner
Copy Link
Copy Source

Both found while adjudicating #516 (merged, fixes #500). The second is one line; the first is why nobody would have found the second.

1. Nothing automatically tests this script

ls scripts/ | grep -i test
test-redeploy-fleetd.sh

grep -rln 'probe-member-credentials' --include='*.sh' --include='*.java' .
fleetd/src/main/java/dev/ltms/fleet/member/MemberCredentialPolicyView.java
fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java
scripts/probe-member-credentials.sh

The two Java hits are the daemon-side endpoint the probe reads. Neither is a test. test-redeploy-fleetd.sh is the only test script in scripts/, and it does not touch this one.

So #516 added three refusal guards to a security-adjacent probe — an interpreter gate, a parser-exit-status check, and an arity check — and all three are pinned by nothing that runs on its own. They are held up by the worker's manual cell matrix and by mine.

This also means mutation testing gives zero information here. Every mutant survives, not because a dimension is unpinned, but because there is no harness to run. That is worth stating plainly, because a report of "mutations survived" on this file would read like a finding and would mean nothing.

What that cost, concretely

Every cell in #516's acceptance had to be run by hand, twice — once by the implementer and once by me, because a worker's "clean" is never promoted to a fact. That is the whole cost of the missing harness, paid once per change, forever.

The fix

Add scripts/test-probe-member-credentials.sh, modelled on scripts/test-redeploy-fleetd.sh — same shape: source the script with its existing SOURCED guard if it has one (add one if it does not), call the pure pieces directly, and use fixture files rather than the network.

The cells that must be covered are exactly the matrix #516 was accepted on:

cell expected
bash older than 4 refuses, exit 3, message names the interpreter and the running version
parser exits non-zero refuses, exit 4, message names the parser and its status
parser exits 0, returns fewer than 5 fields refuses, exit 5, message names the parser and the count
parser exits 0, returns nothing at all refuses, exit 5, count must read 0 — see item 2
well-formed policy, several known names passes, exit 0, and the name table matches the fixture

The last row is the control, and it is not optional. Three refusal cells with no passing cell prove only that the script refuses things.

Never let a test contact the real POLICY_URL. Use fixtures. The probe reads a live daemon endpoint and a test must not depend on one being up, or on what its policy happens to hold today.

2. The arity message says "1 field(s)" when the parser returned nothing

I ran a cell neither the implementer nor the ticket had:

a fake jq that exits 0 and prints nothing
  -> exit 5
  -> "refusing to run: the policy parser (jq) returned 1 field(s); at least 5 are required ..."

The parser returned zero fields. The message says one.

The cause, measured with a control:

bash -c 'R=""; mapfile -t F <<< "$R"; echo "count=${#F[@]}"'      -> count=1   (one empty element)
bash -c 'R="a
b"; mapfile -t F <<< "$R"; echo "count=${#F[@]}"'                  -> count=2   (control)

<<< appends a newline, so a herestring of the empty string is one empty line, and mapfile -t yields a one-element array. #516 changed the read from < <(producer) to <<< "$captured" for a good reason — capturing the producer's exit status — and this is the one behavioural difference that came with it.

The refusal is correct, the exit code is correct, and the message's main claim ("the parse ran but its shape is wrong") stays true. Only the number is wrong, and it is wrong by exactly one, only in the empty case.

It is worth fixing anyway, because of what this ticket family is about: #500 was a message that stated the wrong cause, #511 was a message that stated a recovery that does not work, and this is a message that states the wrong count. The number is the only piece of evidence that message carries. An operator who sees "returned 1 field(s)" will go looking for the one field that came back, and there is not one.

The fix

Treat a _FIELDS_RAW that is empty as zero fields before the arity check runs — for example, refuse on [ -z "$_FIELDS_RAW" ] first with its own message saying the parser produced no output at all, or normalise the count. Either is fine; say in a comment why the empty case needs its own handling, so the next reader does not "simplify" it back.

Acceptance

  • bash scripts/test-probe-member-credentials.sh exits 0, and covers every row of the table above.
  • bash -n scripts/probe-member-credentials.sh passes under both /bin/bash (3.2.57 on this Mac) and env bash (5.x). The two interpreters behave differently and that difference is what #500 was about.
  • The empty-parse cell reports a count of 0.
  • With a harness in place, mutation proofs become meaningful — so add them: for each new test, apply a mutation it should catch, prove it applied with two greps using different search strings and a grep -n re-read of the line, run the suite, quote the FAIL line and the exit code, restore, confirm byte-identical with shasum -a 256, then a green control run.
  • Never contact the real POLICY_URL from a test, and never print the value of any environment variable.

Related

  • #500 / #516 — the change this was found in.
  • #512, #517 — the other two outstanding script defects, both also in a file with thin or source-text-only coverage.
Both found while adjudicating #516 (merged, fixes #500). The second is one line; the first is why nobody would have found the second. ## 1. Nothing automatically tests this script ``` ls scripts/ | grep -i test test-redeploy-fleetd.sh grep -rln 'probe-member-credentials' --include='*.sh' --include='*.java' . fleetd/src/main/java/dev/ltms/fleet/member/MemberCredentialPolicyView.java fleetd/src/main/java/dev/ltms/fleet/rest/FleetApp.java scripts/probe-member-credentials.sh ``` The two Java hits are the daemon-side endpoint the probe reads. Neither is a test. `test-redeploy-fleetd.sh` is the only test script in `scripts/`, and it does not touch this one. So #516 added three refusal guards to a security-adjacent probe — an interpreter gate, a parser-exit-status check, and an arity check — and **all three are pinned by nothing that runs on its own.** They are held up by the worker's manual cell matrix and by mine. This also means mutation testing gives zero information here. Every mutant survives, not because a dimension is unpinned, but because there is no harness to run. That is worth stating plainly, because a report of "mutations survived" on this file would read like a finding and would mean nothing. ### What that cost, concretely Every cell in #516's acceptance had to be run by hand, twice — once by the implementer and once by me, because a worker's "clean" is never promoted to a fact. That is the whole cost of the missing harness, paid once per change, forever. ### The fix Add `scripts/test-probe-member-credentials.sh`, modelled on `scripts/test-redeploy-fleetd.sh` — same shape: source the script with its existing `SOURCED` guard if it has one (add one if it does not), call the pure pieces directly, and use fixture files rather than the network. The cells that must be covered are exactly the matrix #516 was accepted on: | cell | expected | |---|---| | bash older than 4 | refuses, exit 3, message names the interpreter and the running version | | parser exits non-zero | refuses, exit 4, message names the parser and its status | | parser exits 0, returns fewer than 5 fields | refuses, exit 5, message names the parser and the count | | parser exits 0, returns nothing at all | refuses, exit 5, **count must read 0** — see item 2 | | well-formed policy, several known names | passes, exit 0, and the name table matches the fixture | The last row is the control, and it is not optional. Three refusal cells with no passing cell prove only that the script refuses things. **Never let a test contact the real `POLICY_URL`.** Use fixtures. The probe reads a live daemon endpoint and a test must not depend on one being up, or on what its policy happens to hold today. ## 2. The arity message says "1 field(s)" when the parser returned nothing I ran a cell neither the implementer nor the ticket had: ``` a fake jq that exits 0 and prints nothing -> exit 5 -> "refusing to run: the policy parser (jq) returned 1 field(s); at least 5 are required ..." ``` The parser returned **zero** fields. The message says one. The cause, measured with a control: ``` bash -c 'R=""; mapfile -t F <<< "$R"; echo "count=${#F[@]}"' -> count=1 (one empty element) bash -c 'R="a b"; mapfile -t F <<< "$R"; echo "count=${#F[@]}"' -> count=2 (control) ``` `<<<` appends a newline, so a herestring of the empty string is one empty line, and `mapfile -t` yields a one-element array. #516 changed the read from `< <(producer)` to `<<< "$captured"` for a good reason — capturing the producer's exit status — and this is the one behavioural difference that came with it. The refusal is correct, the exit code is correct, and the message's main claim ("the parse ran but its shape is wrong") stays true. Only the number is wrong, and it is wrong by exactly one, only in the empty case. It is worth fixing anyway, because of what this ticket family is about: #500 was a message that stated the wrong cause, #511 was a message that stated a recovery that does not work, and this is a message that states the wrong count. The number is the only piece of evidence that message carries. An operator who sees "returned 1 field(s)" will go looking for the one field that came back, and there is not one. ### The fix Treat a `_FIELDS_RAW` that is empty as zero fields before the arity check runs — for example, refuse on `[ -z "$_FIELDS_RAW" ]` first with its own message saying the parser produced no output at all, or normalise the count. Either is fine; say in a comment why the empty case needs its own handling, so the next reader does not "simplify" it back. ## Acceptance - `bash scripts/test-probe-member-credentials.sh` exits 0, and covers every row of the table above. - `bash -n scripts/probe-member-credentials.sh` passes under **both** `/bin/bash` (3.2.57 on this Mac) and `env bash` (5.x). The two interpreters behave differently and that difference is what #500 was about. - The empty-parse cell reports a count of 0. - With a harness in place, mutation proofs become meaningful — so add them: for each new test, apply a mutation it should catch, prove it applied with two greps using *different* search strings **and** a `grep -n` re-read of the line, run the suite, quote the FAIL line and the exit code, restore, confirm byte-identical with `shasum -a 256`, then a green control run. - Never contact the real `POLICY_URL` from a test, and never print the value of any environment variable. ## Related - #500 / #516 — the change this was found in. - #512, #517 — the other two outstanding script defects, both also in a file with thin or source-text-only coverage.
ltms commented 2026-09-12 06:59:47 +02:00
Author
Owner
Copy Link
Copy Source

Closed by #523 (merged). Every item below was checked by me against the merged revision, not read off the PR body.

Item 1 — the harness

scripts/test-probe-member-credentials.sh added. 5 test functions defined, 5 invoked (measured, both counts). Every row of the ticket's table is covered:

cell test what it asserts
bash older than 4 test_bash_older_than_four_refuses exit 3, This shell is bash, and the running $BASH_VERSION
parser exits non-zero test_parser_non_zero_refuses exit 4, jq exited non-zero (status 17)
exits 0, fewer than 5 fields test_short_parser_output_refuses exit 5, policy parser (jq) returned 4 field(s)
exits 0, returns nothing test_empty_parser_output_reports_zero_fields exit 5, policy parser (jq) returned 0 field(s)
well-formed policy (the control) test_well_formed_policy_prints_name_table exit 0, and the three fixture names in the table

The control row is there, so this is not three refusals with nothing proving the script ever passes.

Item 2 — the off-by-one on an empty parse

Fixed. An empty _FIELDS_RAW is now handled before mapfile runs, with a comment saying why, so the next reader does not simplify it back:

# A herestring adds a newline, so mapfile would turn an empty parser result into one empty field.
# Keep that case separate so the refusal reports what the parser actually returned: zero fields.

The empty cell now reports 0 field(s), measured.

Acceptance, ticked

  • suite exits 0 — yes, PASS: probe member credentials guards.

  • bash -n under both interpreters — rc=0 under /bin/bash 3.2.57 and bash 5.3.9, on both scripts.

  • empty-parse cell reports 0 — yes.

  • mutation proofs — I ran two, not one per test, and I am saying so rather than implying full coverage. Each was proven applied two ways (mutant present and original gone), restored byte-identical by hash, with a green control after each:

    • removing the empty-parse special case → FAIL: empty parser output count: missing policy parser (jq) returned 0 field(s), exit 1
    • arity threshold 5 → 0 → FAIL: short parser output status: expected 5, got 0, exit 1

    I re-ran both against the revision I actually merged. The PR's own proofs were run against 0e243e03, before its final edit, so they were proofs about a different revision.

  • never contacts the real POLICY_URL — checked by reading. run_parser sets POLICY_JSON from a fixture file and POLICY_URL to fixture://member-credentials; the well-formed test replaces curl with an exported shell function that cats the fixture. It also self-detects if that isolation ever failed: it asserts on ALPHA_TOKEN/BETA_TOKEN/GAMMA_TOKEN, which only the fixture contains, so a real daemon answering would fail the test rather than silently pass it.

  • never prints an environment variable's value — yes.

Fixed at the gate, by me

  1. Stranded comments. The extraction left about 25 lines of explanation at the old parse site, including "Check the count here" pointing at a function call instead of the check, and a pipefail note saying "handled below" about code that had moved above it. That is the wrong-stated-fact class this ticket family is entirely about — in the file whose history is about it. Each block moved above the code it explains; a three-line pointer left at the call site.
  2. Two needles started mid-parenthetical (jq) returned N field(s)), so a real failure printed missing jq) returned 0 field(s) and read as if the script's own message had an unbalanced paren. It does not. Widened to policy parser (jq) returned N field(s), which also pins that the refusal names the parser it used.

Caveats — reported by the implementer, not re-checked by me

The harness does not cover the non-member path, the missing-parser path, the curl-fetch failure path, the known-count mismatch, or the hash-tool fallback. The ticket did not ask for those, so this is scope to note, not a regression.

One property worth keeping

The suite runs under set -e, so the first failing test aborts before the closing printf 'PASS: …'. That PASS line is reachable only from the fully successful path — the same property #512 needs for its drain-complete line. Worth copying, not breaking.

One consequence to know when reading a red run: because set -e aborts on the first failure, a mutation that breaks several assertions reports only one FAIL line.

Closed by #523 (merged). Every item below was checked by me against the merged revision, not read off the PR body. ## Item 1 — the harness `scripts/test-probe-member-credentials.sh` added. 5 test functions defined, 5 invoked (measured, both counts). Every row of the ticket's table is covered: | cell | test | what it asserts | |---|---|---| | bash older than 4 | `test_bash_older_than_four_refuses` | exit 3, `This shell is bash`, and the running `$BASH_VERSION` | | parser exits non-zero | `test_parser_non_zero_refuses` | exit 4, `jq exited non-zero (status 17)` | | exits 0, fewer than 5 fields | `test_short_parser_output_refuses` | exit 5, `policy parser (jq) returned 4 field(s)` | | exits 0, returns nothing | `test_empty_parser_output_reports_zero_fields` | exit 5, `policy parser (jq) returned 0 field(s)` | | well-formed policy (the control) | `test_well_formed_policy_prints_name_table` | exit 0, and the three fixture names in the table | The control row is there, so this is not three refusals with nothing proving the script ever passes. ## Item 2 — the off-by-one on an empty parse Fixed. An empty `_FIELDS_RAW` is now handled before `mapfile` runs, with a comment saying why, so the next reader does not simplify it back: ```bash # A herestring adds a newline, so mapfile would turn an empty parser result into one empty field. # Keep that case separate so the refusal reports what the parser actually returned: zero fields. ``` The empty cell now reports `0 field(s)`, measured. ## Acceptance, ticked - **suite exits 0** — yes, `PASS: probe member credentials guards`. - **`bash -n` under both interpreters** — rc=0 under `/bin/bash` 3.2.57 and `bash` 5.3.9, on both scripts. - **empty-parse cell reports 0** — yes. - **mutation proofs** — I ran **two**, not one per test, and I am saying so rather than implying full coverage. Each was proven applied two ways (mutant present *and* original gone), restored byte-identical by hash, with a green control after each: - removing the empty-parse special case → `FAIL: empty parser output count: missing policy parser (jq) returned 0 field(s)`, exit 1 - arity threshold `5` → `0` → `FAIL: short parser output status: expected 5, got 0`, exit 1 I re-ran both against the revision I actually merged. The PR's own proofs were run against `0e243e03`, before its final edit, so they were proofs about a different revision. - **never contacts the real `POLICY_URL`** — checked by reading. `run_parser` sets `POLICY_JSON` from a fixture file and `POLICY_URL` to `fixture://member-credentials`; the well-formed test replaces `curl` with an exported shell function that cats the fixture. It also self-detects if that isolation ever failed: it asserts on `ALPHA_TOKEN`/`BETA_TOKEN`/`GAMMA_TOKEN`, which only the fixture contains, so a real daemon answering would fail the test rather than silently pass it. - **never prints an environment variable's value** — yes. ## Fixed at the gate, by me 1. **Stranded comments.** The extraction left about 25 lines of explanation at the old parse site, including "Check the count here" pointing at a function call instead of the check, and a pipefail note saying "handled below" about code that had moved above it. That is the wrong-stated-fact class this ticket family is entirely about — in the file whose history is about it. Each block moved above the code it explains; a three-line pointer left at the call site. 2. **Two needles started mid-parenthetical** (`jq) returned N field(s)`), so a real failure printed `missing jq) returned 0 field(s)` and read as if the script's own message had an unbalanced paren. It does not. Widened to `policy parser (jq) returned N field(s)`, which also pins that the refusal names the parser it used. ## Caveats — reported by the implementer, not re-checked by me The harness does not cover the non-member path, the missing-parser path, the curl-fetch failure path, the known-count mismatch, or the hash-tool fallback. The ticket did not ask for those, so this is scope to note, not a regression. ## One property worth keeping The suite runs under `set -e`, so the first failing test aborts before the closing `printf 'PASS: …'`. That PASS line is reachable only from the fully successful path — the same property #512 needs for its drain-complete line. Worth copying, not breaking. One consequence to know when reading a red run: because `set -e` aborts on the first failure, a mutation that breaks several assertions reports only one FAIL line.
ltms closed this issue 2026-09-12 06:59:54 +02:00
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#519
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?