Update empty-flag-audit docs - #1132
Conversation
|
Claude encountered an error after 7m 53s —— View job Review: empty-flag-audit —
|
| Check | Result |
|---|---|
go build ./... |
pass |
go vet ./... |
pass |
go mod tidy → git diff go.mod go.sum |
clean |
python3 -m py_compile replay.py |
not run — needed approval |
No Go code changed, so this is a clean bill of health rather than evidence about the PR.
What this PR does
Adds a second question to replay.py: for a field the server accepts empty, does the empty value reach the record? Measured by replaying the emptied payload unfreshened so it names the resource the set run created, and diffing the spec's own verify read-back before and after. Plus an ungitignored-by-design evidence sidecar, provenance for each run, two suspect guards on the replayed request, and a substantial docs update.
Findings
1. before is read after two mutating replays — a stored verdict can be produced by the freshened name reverting. (inline) The one I'd fix before quoting the column. FRESHEN = ["name"] only separates resources when the payload's name is the record's identity; for path-identified endpoints it doesn't, so probe's control and emptied replays already mutated the measured record before stored_answer takes before, and the diff then holds two changes.
Traced through update control --description, which the results file records as reaches the record: spec.json gives it flags: {"name": "{name}"}, updateControl.go:70 identifies the control by the path arg, so the emptied replay set description="" and name=replay-<hex>. before already reads description=""; the unfreshened replay only flips the name back — and replay-<hex> matches no VOLATILE pattern, so it survives normalise as a literal diff. Of the three reaches the record rows, only create flow --description is structurally sound. The mirror failure (payload with no name at all → false does not reach the record) is the same root cause.
2. does not reach the record can't distinguish "server ignored it" from "the verify step never shows this field". (inline) The function reports every other uncertainty rather than guessing; this one is missing, and it's load-bearing — the remove_tags "no constraint needed" decision and the README's closing claim both rest on such rows.
3. Evidence written only at the end. (inline) A killed run, or one hitting the SystemExit in replay(), leaves no sidecar — the run that most needs it. Append per item instead.
4. replay.py ignores MEASURE_LAST. (inline) Newly relevant because stored_answer now depends on read-backs working for ~380 later rows, and create environment --included-environments deliberately sends the request MEASURE_LAST exists for. in_order(spec) is already exported.
5. provenance reads cwd-dependent git state. (inline) Two runs launched from different directories aren't comparable, which is what the line is for. Pass cwd=REPO.
6. only_this_parameter_changed is blind to duplicate query parameters (inline) — because it parses with the same lossy dict(parse_qsl(...)) that with_parameter used, so the one difference the code itself introduces slips past the guard. Latent: nothing in the current results has a repeated parameter.
7. server_log returns str on failure, list on success, and doesn't check docker logs' exit code. (inline)
8. Hardcoded fingerprint in a new verify step (inline) — third copy in that entry, and the only one that fails silently.
Nits: three f-strings with no placeholders (f"\tnot asked, nothing was replayed", replay.py:397, :411, :451); stored_answer takes 10 positional parameters, all passed positionally at :432 — entry/command/home could travel as one object.
What's good
The docs are the strongest part, and unusually honest for this kind of artefact. list flows --name is written up as unstable, cause unknown, not attributable to the fixes rather than quietly dropped — and the reproduction attempt (curl, isolated probe, full run against the pre-fix image) is recorded. The "a model that validates a write may also validate a read" section is a genuinely reusable lesson, with the per-field table explaining why remove_tags got nothing, and "a constraint of this kind that has not been checked that way has not been checked" is the right standard. 2026-08-13-upsert-overwrites-unmentioned-fields.md replacing "the server decides" with three server file:line citations closes a real gap. The README's "results-api.tsv is an answer about the server it ran against, not a permanent fact" is the sentence that keeps this file from being misquoted in six months.
On the code: both suspect guards are the right instinct — refusing to record a verdict against a request that couldn't have produced it — and the keep_blank_values fixes are correct and each carries the reason. The 201-and-nothing-changed branch, and the 409 that create service-account --description honestly reports as "not an answer", show the design would rather say nothing than say something wrong. Findings 1 and 2 are asking for that same standard in the two places it isn't yet applied.
Verdict: docs and .gitignore are ready. I'd want finding 1 addressed before the stored column is cited as evidence, since two of its three positive rows may not be measuring what they claim.
· branch update-empty-flag-docs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| key, captured = owned | ||
| before = read_back(binary, entry, command, key, captured, home) | ||
| if before is None: | ||
| return "not asked, the command has no verify step" | ||
| status, _ = replay(method, url, emptied(carried, field), | ||
| label=f"{command} {field} emptied, unfreshened") | ||
| if not 200 <= status < 300: | ||
| return f"not an answer, the replay was itself refused with {status}" | ||
| after = read_back(binary, entry, command, key, captured, home) |
There was a problem hiding this comment.
before is read after two mutating replays, and for path-identified endpoints those replays hit the very record being measured.
FRESHEN = ["name"] only makes a replay a separate resource when the payload's name is the record's identity. For the many endpoints identified by the URL path, it is not — so the control and emptied replays at probe lines 418–421 already mutated the set run's record before before is taken here, and the before→after diff then contains two changes: the emptied field and the freshened name reverting.
Confirmed by reading, for update control --description (which the results file records as reaches the record):
spec.jsongivesupdate controlflags: {"name": "{name}"}, so the captured payload is{name, description}.updateControl.go:70buildsPUT /api/v2/controls/{org}/{identifier}— identity is the path arg, not the payloadname.- So the emptied replay set
description=""andname=replay-<hex>on the real control.beforetherefore readsdescription=""already. stored_answerthen replays the unfreshened payload:description=""(no change) andname={name}(changes back).before != after, so the row readsreaches the record— driven entirely by the name reverting.normalisecan't rescue this:replay-<hex>matches noVOLATILEpattern and no fixture name, so it survives as a literal diff.
The mirror-image failure is a false negative: where the payload carries no name at all, freshened() is a no-op, the emptied replay has already written the empty value, and before == after yields does not reach the record.
Two changes would close it, and the module already holds the standard for both ("reported rather than guessed at"):
- Take
beforeinprobebefore the first replay for the pair and pass it in, so it is the state thesetrun actually left. - Make the comparison blind to freshening — e.g. add
(re.compile(r"replay-[0-9a-f]{8}"), "{replay}")to whatstored_answernormalises — so a reverting name cannot be read as the emptied field arriving.
Of the three reaches the record rows, only create flow --description (PUT with name in the payload, genuinely a separate flow) is unaffected. attest override --description is path-identified too; its diff is an appended document, which the README says counts, but the append is of a document already appended, so it isn't measuring what the row claims either.
| out = SPEC.parent / RESULTS | ||
| out.write_text("\n".join(rows) + "\n") | ||
| evidence = SPEC.parent / EVIDENCE_FILE | ||
| evidence.write_text( | ||
| "".join(json.dumps(item) + "\n" for item in EVIDENCE)) | ||
| print(f"\nwrote {out}") | ||
| print(f"wrote {evidence}") |
There was a problem hiding this comment.
The evidence file is written only at the end, so the run that most needs it loses it. EVIDENCE accumulates every whole response body (plus up to 200 log lines per 5xx) in memory for all 417 rows, and is flushed once here. A run killed part-way, or one that dies on the SystemExit in replay(), leaves no sidecar at all — precisely the run whose surprising verdict you want to read the body of.
Appending as you go removes both the loss and the memory growth:
with (SPEC.parent / EVIDENCE_FILE).open("a") as sink:
sink.write(json.dumps(item) + "\n")Related: audit.py folds a --only run into the existing file with merged(); replay.py overwrites both files wholesale. That keeps results-api.tsv and the sidecar consistent with each other, so it isn't wrong — but a narrow --flag run silently discards 400+ rows of the last full run, which is worth a line in the README next to the --only examples.
| window = (since - datetime.timedelta(seconds=1)).isoformat() | ||
| try: | ||
| done = subprocess.run( | ||
| ["docker", "logs", SERVER_CONTAINER, "--since", window], | ||
| capture_output=True, text=True, timeout=30) | ||
| except (OSError, subprocess.SubprocessError) as exc: | ||
| return f"unavailable: {exc}" | ||
| return (done.stdout + done.stderr).strip().splitlines()[-200:] |
There was a problem hiding this comment.
Two small things in server_log, both about the evidence file being machine-readable:
- The return type changes with the outcome —
stron failure,list[str]on success. Anything readingserver_logout of the JSONL has to handle both.return [f"unavailable: {exc}"]keeps it one shape. - A non-zero
docker logsexit is not checked. If the container is gone, docker writes its complaint to stderr and returns non-zero, and(stdout + stderr)hands that back as though it were log lines.if done.returncode: return [f"unavailable: {(done.stderr or done.stdout).strip()}"]says which it is.
| def said(argv): | ||
| """Return a command's whole output, or why there is none.""" | ||
| try: | ||
| done = subprocess.run(argv, capture_output=True, text=True, timeout=30) | ||
| except (OSError, subprocess.SubprocessError) as exc: | ||
| return f"unavailable: {exc}" | ||
| return (done.stdout or done.stderr).strip() | ||
|
|
||
| binary_path = pathlib.Path(binary) | ||
| changed = said(["git", "status", "--porcelain", "."]) | ||
| return { | ||
| "label": "provenance", | ||
| "at": datetime.datetime.now(datetime.timezone.utc).isoformat(), | ||
| "host": HOST, | ||
| "server_image": said( | ||
| ["docker", "inspect", SERVER_CONTAINER, "--format", "{{.Image}}"]), | ||
| "server_started": said( | ||
| ["docker", "inspect", SERVER_CONTAINER, "--format", "{{.State.StartedAt}}"]), | ||
| "cli_binary": str(binary_path), | ||
| "cli_sha256": hashlib.sha256(binary_path.read_bytes()).hexdigest() | ||
| if binary_path.is_file() else "unavailable: not a file", | ||
| "audit_commit": said(["git", "rev-parse", "HEAD"]), | ||
| "audit_uncommitted": changed.splitlines(), |
There was a problem hiding this comment.
provenance depends on the caller's working directory, which is the one thing provenance shouldn't do. git status --porcelain . and git rev-parse HEAD both run in whatever cwd the script was launched from, so audit_uncommitted means "the repo" when run from the root and something narrower when run from hack/empty-flag-audit/. Two runs recorded from different directories aren't comparable, which defeats the purpose of the line.
audit.py already exposes REPO; importing it and passing cwd= fixes both, and scoping the status to the audit directory would make the field match its name:
def said(argv, cwd=REPO):
...
done = subprocess.run(argv, cwd=cwd, capture_output=True, text=True, timeout=30)
...
changed = said(["git", "status", "--porcelain", "hack/empty-flag-audit"])Also worth one line: said returns done.stdout or done.stderr without checking returncode, so docker inspect against a missing container records Error: No such object: ... in the server_image field. Legible to a human, but a reader of the JSONL can't tell a value from a failure.
| # keep_blank_values, or a parameter that is already blank looks like one that | ||
| # is absent, and dropping it counts as a second change. | ||
| before = dict(urllib.parse.parse_qsl(control.query, keep_blank_values=True)) | ||
| after = dict(urllib.parse.parse_qsl(emptied_parts.query, keep_blank_values=True)) | ||
| changed = {key for key in set(before) | set(after) | ||
| if before.get(key) != after.get(key)} | ||
| if changed != {parameter}: | ||
| return (f"the emptied request changed {sorted(changed)} rather than only" | ||
| f" {parameter}") |
There was a problem hiding this comment.
Latent gap: the guard is blind to the one difference the code itself introduces. with_parameter collapses the query through dict(parse_qsl(...)), so a url carrying a parameter twice — ?tag=a&tag=b, which the repeatable StringSlice flags on list controls/list environments/list repos can produce — loses one value in the replayed request. This guard then parses both sides the same lossy way, so the dropped duplicate never shows up as a second change and the row reads as a clean verdict.
Nothing in the committed results-api.tsv has a repeated parameter today, so it's latent rather than active. Keeping the multi-valued form on both sides closes it:
before = urllib.parse.parse_qs(control.query, keep_blank_values=True)
after = urllib.parse.parse_qs(emptied_parts.query, keep_blank_values=True)and building the emptied url from parse_qsl list + urlencode(..., doseq=True) rather than a dict would stop the loss at source.
| [ | ||
| "get", | ||
| "artifact", | ||
| "{flow}@1bef738d0bb1e690500f99a5b57d958caf3a5eb3e00d9012e1f4369fc6812e01", |
There was a problem hiding this comment.
This is the third copy of 1bef738d0bb... in this entry (flag_values.fingerprint:4609, baseline_output:4587, and here), and fixtures() already mints a {digest} placeholder per command-and-flag pair precisely so a fingerprint isn't written out by hand. It happens to be right — it's the sha256 of cmd/kosli/testdata/person-schema.json — but if that fixture file ever changes, the two other copies fail loudly (baseline mismatch) while this one quietly reads back an artifact that isn't there, and the stored column silently degrades to "no verify output changed".
A comment naming what the constant is would be enough if {digest} can't be used here.
| if normalise(before, command, runs) != normalise(after, command, runs): | ||
| return "reaches the record" | ||
| if status == 201: | ||
| return ("not an answer, the reply was 201 and nothing changed, so the" | ||
| " replay may have named a separate record") | ||
| return "does not reach the record" |
There was a problem hiding this comment.
does not reach the record can't tell "the server ignored it" from "the verify step never shows this field". The function is careful in every other direction — a refused replay, a 201 that might have named a separate record, an absent verify step are all reported rather than guessed — but there's no check that the read-back displays the field at all. A get X --output json that omits the field yields before == after unconditionally, and the row reads as a finding about the server.
Cheap and in keeping with the rest: confirm the field's control value is visible in before first.
was = carried.get(field)
if isinstance(was, str) and was and was not in before:
return "not an answer, the verify step does not show this field"Worth having because does not reach the record is now load-bearing in the docs — 2026-08-13-empty-value-decision.md uses it to justify putting no constraint on remove_tags ("Measured: an empty entry removes no tag"), and the README's closing claim that every remaining acceptance is "a description field ... or a value that does not reach the record" rests on three such rows.
| rows = ["command\tflag\tmethod\turl\tfield\tcontrol\temptied\tverdict\tanswer"] | ||
| rows = ["command\tflag\tmethod\turl\tfield\tcontrol\temptied\tverdict" | ||
| "\tstored\tanswer"] | ||
| for command, entry in sorted(spec.items()): |
There was a problem hiding this comment.
audit.py keeps MEASURE_LAST = ["create environment"] and in_order() so that a command which leaves the server unable to answer something else is measured after everything that needs the server intact. replay.py iterates plain sorted(spec.items()), so create environment runs near the front.
That asymmetry didn't matter much while replay only compared statuses. It matters now: stored_answer runs the spec's verify steps for every later row, and for create environment --included-environments it deliberately PUTs included_environments: [""] at the real environment — the exact request MEASURE_LAST's comment blames for making list environments 500 for the whole org. If that ever regresses, ~380 later stored answers degrade to "nothing changed" with nothing in the file saying why.
for command in in_order(spec): (already exported from audit.py) makes the two scripts agree. The docs here note server#6503 is closed, so this may be moot today — but MEASURE_LAST still lists the entry, and one of the two scripts is now wrong about it either way.
1 participant
Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR