feat: empty flag audit - #1103
Merged
Merged
Conversation
We have been fixing empty-value bugs one at a time - #1091, #1092, #6070 - without knowing how many there are or where. Reading the code cannot answer that: whether a flag refuses an empty value depends on a required-flag check, a PreRunE allowlist, a mutual-exclusion rule, the server, or a third party, and some differences never reach the output at all. `kosli attest generic` prints the same line and exits 0 whether the attestation went to the artifact or to the trail. So this measures it instead. The audit runs each command three times - flag omitted, flag set, flag empty - and compares exit codes, output, and the state left on the server, on a laptop and again with the variables GitHub Actions sets. It covers 374 of 653 command-and-flag combinations; the rest need AWS, Azure, a git provider and the like. Ten findings, each re-run outside the audit and read back from the server. Seven change a compliance answer: a flow that requires none of the attestations intended, an attestation recorded against the trail instead of the artifact, a verdict about the wrong flow, a policy governing nothing. All exit 0 and print what success prints. Two are bugs in their own right and get their own write-ups: an empty --included-environments leaves a record that 500s an organization's entire environment listing, and re-running create flow, begin trail or create policy without --description wipes the description, needing no empty value at all. The decision document proposes one rule - an empty value is always an error - and a four-step rollout that starts by reporting warnings to app.kosli.com, so the impact is measured before anything breaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The audit reported 23 command-and-flag combinations where nothing could be said, and the decision document carried a row admitting the gap. None of them were the CLI's fault. Twenty were the evaluate commands, run against deny-no-violations.rego, which is `allow = false`. No run of those commands could exit 0, so an empty value had nothing to be compared against. They now use allow-all.rego. Three were `list environments` asking for every environment in an org the audit had filled with hundreds of them, and timing out. A page limit keeps the request small enough to answer, and removes the slowest block of the run with it. `create environment` now runs last. One of its combinations is the --included-environments bug that 500s an organization's entire environment listing, so measuring it early left every later `list environments` unmeasurable. That is the bug's blast radius reaching the audit itself. Two harness traps went with them. A run limited by --only wrote a results file containing only what it ran, silently discarding every other result; it now merges. And each pass writes its own file, so --ci no longer overwrites the laptop run. All 374 measured combinations now yield a result. Refused rises 203 from 190, let-through 166 from 156, and the document's figures follow. The release argument changes too. This no longer joins the v3 batch: a customer whose pipeline breaks should be able to read one release note and know why, and step 2's warnings say when the moment is right independently of whatever else is queued for v3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Not every request arrives through the CLI. A customer calling the API directly keeps every one of these behaviours, and the warnings proposed in steps 1 and 2 will never see them, so the impact they measure covers CLI traffic only. The document claimed more reach than the proposal has. Reading the payloads with --debug splits the findings by where the behaviour actually lives. Some are the CLI's own: an empty --fingerprint sends no fingerprint to the trail-scoped endpoint, and the CLI is what picked that endpoint - someone calling the API chooses artifact or trail deliberately and is not misled. Others are the server accepting what it should not: an empty --included-environments sends a logical environment with no included_environments field at all, which returns 201 and then cannot be read back, and any client can send that payload. So the server half is the part that closes the hole for everyone, which makes the server work in step 2 more than a precondition. Three inaccuracies went with it. The claim that 162 combinations behaving like an omitted flag made an empty value "merely useless" contradicted the findings table, where most entries are exactly those: omitting --template-file means "use the default template", which is how a flow ends up requiring nothing. The breakage estimate said 166 when 166 is only what could be measured. And step 2 did not mention that the description fix needs the server to accept a payload without one before the CLI stops sending it, or every kosli create flow fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The section claiming a CLI rule cannot reach API callers had the mechanism wrong. It implied its examples were empty values arriving at the server. They are not: in 162 of the 166 cases an empty value and an absent flag produce an identical payload, so nothing empty ever reaches the server and there is nothing there for it to reject. That is why the server's half is a different rule rather than a mirror of the CLI's. The three examples are now a table, one per kind, with whose problem each payload is in its own column - the CLI's, the server's, or neither. Two of them were being read as the four combinations where an empty value differs from omitting the flag; they are not, they are drawn from the 162 where it does not, which is the whole point of citing them. Dropped "a record the server cannot read back is not one it should accept". The included-environments 500 is a plain defect that will be fixed regardless, and stating it as a rule here prescribed one of two possible fixes - refusing the write, or tolerating the read - which is not this document's call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two claims in this report were wrong, both found by pushing on it rather than by re-reading it. It said re-running these commands "wipes the description", "silently destroying data". Trails and attestations are event-sourced: re-running begin trail appends an event, and the value reported earlier is still on the trail. Nothing is destroyed there. Flows keep no events, and policy events record only that a metadata_update happened with an empty data field, so on those two the earlier text really is gone. The report now says which is which. It also claimed more damage than the model allows. A trail snapshots the flow state it was created with, so editing a flow afterwards does not reach back into trails already created - verified by changing a flow's template and re-reading an existing trail, which still requires what it always did. No compliance verdict moves. What is lost is what someone wrote to explain the thing. And it was too narrow. Scanning every write command's payload with --debug for fields sent when no flag was passed found the same shape on --user-data and on create policy --comment: ten combinations across eight commands, including all five attest commands, each confirmed by running it twice and reading the object back. Two candidates the scan flagged - create environment --description and tag --set - do not overwrite, so a payload alone does not settle it; the server decides. Renamed from 2026-08-13-description-wiped-on-upsert.md, which named one field of three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`kosli create flow` says "Create or update a Kosli flow. You can specify flow parameters in flags", and its examples always pass --description. That is the apply model: what you pass is what the flow becomes, so an absent --description meaning "no description" is the model working. Calling it a bug was wrong. What is wrong is that `begin trail` describes itself the same way and behaves differently. Its update on the server is guarded by `if "description" in payload`, so an absent description leaves the stored one alone - and the CLI never lets that guard fire, because it puts the field in the payload whether or not the flag was passed. Two commands of the same shape, two models, and within `begin trail` the two layers disagree with each other. Neither model is wrong; having both is. So the server-side rule this document proposed - "an absent or empty field is not an instruction to erase what is stored" - was also wrong, because under the apply model erasing is correct. It becomes: each command must say which model it uses, and the CLI and the server must agree. The fix has two shapes per command rather than one, and either way both layers change together. The example used for that rule was wrong too. It cited `create environment` treating an absent description as an instruction to erase, when that command sends `"description": ""` and the description survives. `begin trail` is the case where it actually happens. Also drops the data-loss framing. Trails and attestations keep the earlier value in their events, and a trail snapshots the flow state it was created with, so editing a flow later does not reach into trails already created. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Saying each command must declare whether an absent field means "leave it alone"
or "set it to nothing" is ninety-one decisions nobody will remember, which is how
the CLI came to be inconsistent in the first place. The verbs already promise it,
and the CLI already mostly follows them:
create, begin apply - what you pass is what the object becomes
update patch - the fields you name change, the rest are left alone
Both halves are observed, not assumed. `create flow` replaces the description on
both sides. `kosli update control vc --name "second name"` leaves the description
untouched and takes the record's version from 1 to 2, so update is patch and
versioned.
By that rule `begin trail` is the only outlier, and it is the server side that is
out of step: its guard leaves an absent description alone, which is patch
behaviour in a begin command. The CLI is already correct.
It also explains a cost the decision document could previously only assert.
`update control --description` and `update service-account --description` clear on
purpose because update is patch - omitting the flag cannot clear anything there,
so an empty value is the only way to say it. That is why exactly those two want
`--clear-description` and no others do.
The bug report stops reading as ten defects. Its table is what the CLI does
today: the create rows are apply working correctly, and the begin trail and
attest rows are the ones the choice decides. The trade-off is stated rather than
assumed away - re-running a workflow re-runs begin trail, and under apply a
re-run that omits --user-data leaves the trail with none, recoverable from the
trail's events but no longer the current view.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
279 of the 653 combinations sit on commands needing AWS, Azure, a git provider, Jira, SonarQube, Snyk or a credentials store, and the audit skipped them because their baseline cannot succeed here. But the question this audit exists to answer is whether the CLI refuses an empty value, and the CLI's own checks run long before anything is sent to those services. Skipping them threw away an answer we could have had. They are now run like everything else, and the existing three-run comparison reads it: if the empty run fails differently from the omitted and set runs, a check in the CLI caught it; if all three fail identically, the empty value passed every check the CLI has and died at the service instead. Only what the service would then have done stays unknown, which no work on this machine could show. Of the 279: 173 refused by the CLI, 88 reaching the service untouched, 18 let through with exit 0. Appendix 2 changes character as a result - "not measured" was its largest column at 59, and is now empty; flag names the CLI always refuses go from 46 to 79. It also corrects a claim the audit had never checked. The document said `--github-token ""` gets as far as calling GitHub. It does not: that flag is refused on both its commands, as are --gitlab-token, the --bitbucket-* and --azure-* flags, --jira-api-token, --sonar-api-token and --kubeconfig. The credentials that do slip through are --aws-key-id and --aws-secret-key on all three snapshot commands, --jira-pat, and --registry-password and --registry-username on 6 of their 16 - the same registry credential checked on six commands and not on the other ten. The claim came from the type-derived analysis that preceded the audit and had survived unexamined since. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pliance The audit says which combinations accept an empty value; it does not say what that costs. 136 of them are on commands that record or judge compliance, and only about a dozen had ever been looked at one at a time. Three rounds of looking have now produced two findings, then three, then one. Four are added here. `evaluate input --params ""` drops back to the policy's own defaults, and a score of 5 that is ALLOWED at threshold 3 is denied at the default of 10 - a verdict flipped by an unset variable. `evaluate trails --attestations ""` evaluates every attestation instead of the ones named. The three repo flags record no repository provenance at all. And `--redact-commit-info ""` sends the commit author and message to Kosli in the clear, which is precisely what the flag exists to prevent. That last one is the first finding here that is not about compliance data, and it was not on anyone's list. It surfaced because a test of mine printed "present" for a field whose value was "**REDACTED**", so the test could not tell redacted from exposed. Chasing that showed redaction works and an empty value silently disables it. Rows are now grouped by what they cost - the eleven compliance ones together, then the two new categories, then the bugs - and sorted from the tags rather than by hand, so adding a row keeps the grouping. Three flags were confirmed protected on the way, and are not listed: --artifact-type, attest junit --results-dir and attest custom --attestation-data all refuse an empty value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The audit discovered commands by walking --help, and reported 653 combinations
as though that were all of them. It was not. Cobra prints no listing entry for
a Hidden or a Deprecated command, and pflag prints none for a hidden flag, so
a whole class of the CLI was invisible to the one tool whose claim is that it
measured everything. `attest override` was in the audit only because someone
had named it by hand, and that hand-patch was the warning.
A test now walks the command tree itself, where nothing is filtered, and writes
what it finds to coverage.json. It found 48 combinations the audit had never
run: `report artifact` and `snapshot server`, both Deprecated; `completion`,
which declares no flags and so never matched the "[flags]" the walk looked for;
and, the larger half, flags that deprecation hides - --registry-provider on 16
commands, --visibility, --yes, --e, --cluster, --function-name and the rest.
Those are real flags a customer can still pass an empty value to.
audit.py now reads coverage.json and stops rather than measuring less than it
claims. It stops before resetting the server, so a run that cannot be complete
does not throw away the last one that was, and it stops rather than warning,
because a warning at the top of several hundred combinations is gone by the
time anyone reads the results file, which looks complete either way.
The directory moves out of hack/ in the same change. It is not a one-off aid
any more: it stays in the repo to catch regressions, and a guard only works if
it is run. The decision document and its two companions move with it, into
empty-flag-audit/docs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard added in 7e7c98b left the audit refusing to run, naming 33 command-and-flag combinations spec.json had no entry for. This covers them, and removes the reason they were missing in the first place. Discovery no longer walks --help. It reads coverage.json, which is written from the command tree, so discover, hidden_flags, HIDDEN_COMMANDS, global_flags, declared_flags, own_flags and FLAG_TYPES all go with it: nothing parses a help page any more, so the blind spot cannot come back. coverage.json gains each flag's type, which is the one thing the help page was still being read for - a boolean takes no value of its own, so the audit has to know which flags are boolean before it can invent one. Running the three commands it had never run found five faults in the bootstrap itself, none of which could show while it only ever saw commands that behave: - Cobra prints `Command "artifact" is deprecated` above the output of a deprecated command, which still runs. It was being read as the command's first meaningful line, so `report artifact` and `snapshot server` looked like failures whatever their flags held. - `completion` takes one of four shell names and nothing else, so an invented positional could never get it as far as running. - `snapshot server` writes `required flag "paths" not set` from its own PreRunE, without cobra's "(s)", so the fix-the-invocation loop did not recognise its own cue. - `report artifact` calls --git-commit what the attest commands call --commit, and it is resolved against the repository the audit runs in, so an invented name fails. - `snapshot server` reports a server environment, and the fixture was created as K8S, which it refuses. The 23 commands that were missing only flags are extended in place rather than bootstrapped again, so the invocations corrected by hand in earlier commits survive. results.tsv is not regenerated here. The audit runs again now, and the figures the decision document quotes change when it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A run took twenty minutes, and one command-and-flag pair sat behind a macOS
keychain dialog waiting for a person, which meant it could not be left to run
at all. Almost none of that time was the CLI doing work. It was the CLI
waiting, and every wait turned out to be a defect in the audit or in what it
measured:
- The AWS clients wait five seconds for an instance-metadata endpoint this
machine will never answer on. Saying there are no instance credentials makes
them fail at once, in the same place, with the same kind of error. Seven
minutes.
- gitlab.com answers normally for twenty-six requests and then starts
refusing, and its client retries that with a backoff. A run sends it eighty
of them carrying a token that was never valid. Pointing the client at the
local server stops both the waiting and the asking. Two minutes, and the
comparisons stop depending on a third party's load: one row's verdict was
changing with it.
- `config` is the only command that stores the API token, encrypting it with a
key from the operating system's credentials store. The audit passes a token
to everything, so `config` reached the keychain, and with a throwaway HOME
there is none, so macOS asked. It needs no token to do its work. Four and a
half minutes, and the audit can be left alone again.
- `search` looks for the fingerprint every attest command reports, so late in
a run it matches an artifact that has collected hundreds of attestations.
Fixtures now include a digest of their own, and `search` primes its own
artifact. Sixteen seconds, and `--only search` stops giving a different
answer from a full run.
Four point one minutes of measured CLI time now, median three tenths of a
second a pair.
Two checks keep it that way, both reported after the results are written so a
run that trips one still keeps what it measured. A pair may not take longer
than five seconds: 694 of 700 finish inside 1.5, the next is 7.3, so the limit
sits in a gap with nothing in it. And no run may retry a request until it
gives up: the client retries 5xx, 429 and a request that never arrived, and
nothing else, so giving up means the server broke or was never reached, while
a 400 is the server working. Both allow named exceptions carrying their reason.
The retry check earned itself immediately, twice.
`attest override --commit` has been failing with a 500 since before this
branch, and the audit recorded the row as accepted, correctly: the empty run
is what it measures and the 500 is in its control. Only the seven seconds of
backoff showed it. Written up.
And `list environments` was returning 500 mid-run, which turned out to be the
bug already written up as being about an empty --included-environments. It is
not. Any logical environment created without included environments cannot be
read back, and the listing renders every environment in the org in one
comprehension, so one such record 500s all of them. Creating one that way is
ordinary use. The write-up is corrected and renamed, and its first suggested
fix - the CLI refusing an empty value - is struck, because it would not have
helped. The audit was poisoning its own organization this way in `join
environment`'s setup; that setup now seeds its logical environment.
The figures in the decision document are now stale: the audit covers 700
combinations across 165 flag names where the document still says 653 and 152.
That is the next commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The audit covers 700 combinations across 165 flag names on 94 commands, where
the document still said 653, 152 and 91. Every figure it argues from moved
with them, so all of them are re-derived here from results.tsv and
results-ci.tsv rather than rescaled: 412 commands work here rather than 374,
218 empty values are refused by the CLI rather than 203, and 189 are refused
by nothing rather than 166.
results-ci.tsv is regenerated too. It had not been run since before any of
today's changes, and half the figures are its column.
Three of the changes are not renumbering.
Appendix 1 now shows four combinations losing their CLI check inside CI rather
than two. `report artifact` was one of the commands --help never showed the
audit, and it has the same --build-url and --commit-url defaults as `attest
artifact`, so it doubles the evidence for a blind spot that was only ever
argued from one command.
The list of services the audit cannot reach loses "a credentials store".
`config` never needed one: it reached the keychain because the audit handed it
an --api-token, and it is measured like anything else now.
And the count of accepted combinations on commands that record or judge
compliance is 160, not a rescaled 136. The rule behind the original was not
written down, so this one is derived afresh and the commands it counts are the
attest, assert, evaluate, allow, snapshot and report commands, plus `begin
trail`, the create commands for flows, policies and controls, and
attach-policy and detach-policy.
Appendix 3 is regenerated by report.py --write-appendices, and now lists the
flags that were invisible to a help page: --registry-provider on sixteen
commands, --visibility, --git-commit, --paths, --e and --yes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ield
The audit drives the CLI, so it can only answer half the question. When an
empty value produces the same request as omitting the flag - which is 172 of
the 189 the CLI does not refuse - the server never receives anything empty to
reject, and no amount of running the CLI harder will make it.
replay.py asks the other half by capturing a request and sending it again with
one field emptied. It reads the same spec.json, reuses the audit's own
invocations so it captures what the audit measures, and writes results-api.tsv.
A separate script rather than a third mode of audit.py: --ci asks the same
question in another environment and its rows line up with the plain run's,
while these rows are about a captured request and do not.
The mapping from a flag to the payload field it controls is measured, not
written down: the payload of a run with the flag set, diffed against one with
it omitted. The field is emptied here rather than by the CLI, which is why this
keeps working after the CLI starts refusing empty values.
requests.go now logs the method beside the URL. Without it the probe would have
needed a hand-written table of which endpoint takes which verb, which is the
thing this approach exists to avoid.
Of 412 rows, 19 are an answer about the server: 11 fields it accepts empty and
8 it refuses. The eight refusals are mostly types rather than emptiness - a
boolean, an enum, two objects, a URL. Among the eleven, `create flow
--template` is refused by the CLI by the type added in #1092 and accepted by
the API, which stores a flow requiring an attestation whose name is empty. That
is the decision document's "what a CLI rule cannot reach" argument with a
measured instance behind it.
Two things the control replay caught, and both are the point of having one. A
captured request that was never valid gives a 400 beside an emptied 400, which
reads as a refusal and is not: 35 rows say so rather than pretending. And
chasing why those controls failed led to the CLI audit's own controls, where
114 of 412 runs with a supposedly real value fail. The audit invents the values
it gives flags. Written up, with the tempting remedy measured and rejected:
the server publishes a schema, and it would fix 6 of the 114.
Most rows are not an answer, and mostly for good reasons the file states rather
than hides. The read commands put their flags in the query string and send no
body, which is 85 rows and the largest thing still missing. Some flags never
leave the machine. `kosli fingerprint` sends nothing at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…worked
Every combination is three runs: the flag omitted, the flag set to a real
value, the flag empty. For 114 of the 412 that run here, the run with a real
value failed, because the value was invented: `--redact-commit-info
probe-redact-commit-info`, `--expires-at probe-expires-at`, `--annotate
probe-annotate`. The audit still reported those rows, and what they said rested
on a comparison with a run that never worked. It is how the `attest override`
500 stayed hidden - the row read "accepted" while its control had been a 500
for weeks.
Now 30. The values come from what the CLI or the server says when refusing an
invented one, so each is transcribed rather than guessed: an enum the server
lists, `key=value` where the CLI says so, an epoch where it wants a number,
`author` where it names the commit fields it will redact.
Three of them needed more than a value.
`--external-fingerprint` labels an external url, and the url is a different
flag, so testing them one at a time meant the label never had one. The url now
sits in those commands' baseline, which the tested flag is dropped from, so its
own measurement is untouched.
`--fingerprint` names an artifact the server must already hold, and nothing
created one. The attest commands now report the artifact first. This is the
flagship finding's own row: `attest generic --fingerprint ""` attesting to the
trail instead of the artifact was true, and now the run it is compared against
binds an attestation to a real artifact rather than failing.
`--artifact-type` says what kind of thing to fingerprint and is refused without
something to fingerprint, while `--attestation-id` is refused *because* the
command is given an attestation name. Both now name their own positional
arguments, via `args_for_flag`, used for all three of a flag's runs so they
stay comparable with each other.
That last one exposed the fingerprint the audit used being invented too. The
artifact the setup reported and the artifact the CLI computed from the file
were different things. FINGERPRINT is now the real SHA256 of the file the audit
attests, replacing 61 occurrences in the spec, and the two flags agree.
The figures move twice, both times away from claiming protection the CLI does
not give. On the commands needing a service, "refused by the CLI" falls from
182 to 164, because the audit's own invalid values were what stopped 18 of
them, and a run that fails differently from its controls is read as a check
firing. On the commands that work here it goes the other way, 218 to 223: six
combinations that looked unrefused are refused once the control they are
compared against works. results.tsv and results-ci.tsv are single clean runs
again, and the document's figures come from them.
What is left needs more than data. Twenty are the registry credentials, which
are only valid with an image and force a lookup over HTTPS the local registry
cannot answer. The rest want a connected git provider, or are flags that cannot
be given a working value at all: `--http-proxy` on the command whose proxy it
is, and `--commit` on the override the server 500s.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rameter
A read carries its flags in the query string, not a body, so replay.py had
nothing to capture from `list`, `get`, `log`, `diff` and `search` - 254 rows
reading "nothing to read", most of them theirs. It now takes the same diff over
the query string, empties the parameter there, and sends the url back. Answers
go from 19 to 77, and "nothing to read" from 254 to 117.
Nothing new is logged for it. Every request is logged again with its full url
when the answer comes back, which is where a read's query string survives. That
line does not name the method, and a request with no body could be a GET or a
DELETE, so only the commands whose verb settles it are read this way. Replaying
a read is also the only replay that is safe to repeat, since nothing is
created and the control cannot disturb the emptied request.
What the reads say is worth the whole exercise: every emptied filter is
accepted. `tag=`, `search=`, `name=`, `fingerprint=`, `flow_tag=`, `repo_id=`,
`space_id=`, `interval=`, `trail=` - all answered 200 with a list, as though no
filter had been asked for. What the server does refuse is the parameters it
type-checks: page, per_page, sort_direction, reverse, snappish1 and snappish2.
So the two halves agree, and unhelpfully. The decision document already records
that 21 of the 24 filter flags are never refused by the CLI, and that an empty
value means nothing for a filter - there is no artifact called "". The server
does not refuse them either, so `kosli list environments --tag "$VAR"`
answering "No environments were found" has an exact twin for anyone calling the
API, and the rule proposed for the CLI would not close it.
The two controls that failed are both `--repo`, and between them they show one
parameter treated two ways: an unknown repo_name 404s on `list artifacts` while
an empty one returns every artifact, and on `log environment` an empty one is
looked up and refused with "Repo '' not found". Neither is claimed as a verdict
while the control fails - the audit has no repository that exists, for want of
a connected git provider - but the inconsistency is recorded.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`list snapshots` prints how long ago each snapshot was taken, so two runs seconds apart say "about a second" and "2 seconds" - and the table's column widths move with it. The comparison read that as a difference and recorded `--interval ""` as doing something omitting the flag does not, which put it in the decision document as a case needing an explanation. It never was one. stabilise now blanks an elapsed time whether or not it ends in "ago", and collapses column padding so a blanked value cannot shift the header along with it. Re-running both passes moves exactly one row in each: 166 combinations now match omitting the flag and 17 do not. The document carries those figures, and answers what a reader asked of the earlier draft: what the eighteen were, what "read off the code" meant, and which half of the 700 each table is about.
`update control --description ""` sets a control's description to null, which is one of only two places in the CLI where an empty value does what someone meant. The audit filed it with the no-ops, because the fixture created a control without a description: neither the empty run nor the omitted run had anything to clear, so the two could not be told apart. The setup now seeds one, and the row reads as clearing in both passes. That restores the split to 165 combinations that match omitting and 18 that do not, and puts both `--description` clearing cases together - the only capability the proposal takes away, which is what the cost section claims. The 165 are also broken down by what the flag is for, to answer a question the earlier draft invited: whether any of them do something worth keeping. None do. In all 165 the empty value already does what omitting the flag does, so what refusing removes is the spelling, not the behaviour.
…swer The one combination the audit could say nothing about was one it could not run. `--fingerprint` needs the attestation name it was told to drop, an attestation that is on an artifact, and no `--trail` beside it - the two are mutually exclusive. All three runs died on "one of ATTESTATION-NAME argument or --attestation-id flag is required", which is a fact about the invocation and not about the empty value. The spec could drop a command's positionals for one flag but not its other flags, so `flags_for_flag` joins `args_for_flag`. With a setup step that attests to the artifact, the control run succeeds and the empty value is refused: 224 refused on a laptop, 220 in CI, and the table no longer needs a row for a question left open. report.py anchors the generated appendix on its heading. It cut the document at the first table row beginning with a flag name, which the new breakdown of the 165 also has, and truncated everything below it.
`--fingerprint ""` defeats the same check as `--trail ""` and produces the same request with no URL, verified standalone: omitting either says "at least one of --trail, --fingerprint is required when using ATTESTATION-NAME", and emptying either shows "unsupported protocol scheme". The row named only the flag the audit could measure before its invocation for --fingerprint worked. Still nineteen findings. One defect reachable two ways, not a new one.
replay.py imports invocation_for, normalise and prepare from audit.py and reads the same spec, so this week's fixes to the CLI audit could have moved its rows too. Re-running says one did: `get attestation --fingerprint` now has an invocation that works, so it captures a request instead of failing, and moves from "nothing to read" to "no field". Everything else holds. 417 rows and 77 answers about the server, 25 accepted and 52 refused, all unchanged. The two counts either side of the moved row are updated in the table and the sentences quoting them.
`kosli tag` already spells this: --set adds, --unset removes, and the field is named as the flag's argument. `update control X --unset description` follows it, and unlike a --clear-description flag per field it takes whatever becomes clearable next. The alternative of a magic --description null was rejected: it puts the meaning back in the value, where an unset variable reaches it, which is the failure this document exists to stop. Whether anyone is affected is now answerable rather than argued. Every create and update of a control writes a version document carrying that version's description, so a description going from non-empty to empty between adjacent versions is a clear, with who and when. Service accounts keep no such history, and the document says so rather than implying the question is settled. The rest is scope. Problems 1 and 2 belong to the API write-up and the absent-flag handover, and the material duplicating them moves there - including the kubectl-apply framing and the point that refusing empty flag values cannot reach a field the CLI sends with no flag passed. The staged four-step release becomes Appendix 0, since it is the fallback rather than the plan.
The decision named three kinds of problem and pointed at a document for each, but they sat in two directories, so the three read as unrelated. They are one piece of work: the audit found all three, and each write-up is only useful beside the others. The three bug write-ups move to empty-flag-audit/docs/ alongside the decision, the API sweep, the size estimate and the note on invented input values. Both sets of citations follow, including audit.py's SLOW_EXPECTED, which explains `attest override --commit` being slow by naming the override-500 write-up.
…te-ups The logical-environment and attest-override 500s are now tracked as kosli-dev/server 6503 and 6504. The issues are where the fixes will be discussed and closed, so the table cites them directly and the two write-ups they duplicate are gone. audit.py's SLOW_EXPECTED note cited one of those files as its reason for accepting the wait; it now cites 6504.
Re-running replay.py moved the row counts (329 write, 88 read, 20 body refusals over 14 fields, 29 control failures), so the prose that quoted the old numbers no longer described the data beside it. The decision document has also been restructured since this was written, so the two references into it now name the proposal and Appendix 0 rather than sections that have gone.
An unset shell variable expands to `--flag ""`, which is indistinguishable from a deliberate empty value once the command is running. The check that catches this already existed but only fired on flags RequireFlags had marked required, which is the flags whose default is empty; a default filled in from a CI environment variable took the flag out of scope, so the CLI checked less inside CI than on a laptop (cli#1088). Slice flags escaped it entirely, required or not, because their value renders as "[]" rather than "". Asking pflag whether the value is a SliceValue distinguishes an emptied slice from a string flag whose value happens to be the two characters "[]". One rule now says it for every flag, so the message no longer claims the flag was required.
The two `update` commands that used `--description ""` to clear a description were the whole cost of refusing empty values, and both documents planned a replacement for it: `--unset description`, in the shape `kosli tag --unset` uses. That replacement is not wanted. Losing the ability to empty a description is accepted instead, so the sections that promised a new flag, an exemption, or a staged release conditional on one now say the capability goes and nothing takes its place. The measurements are untouched, including how to find out whether anyone relied on it, which matters more without a replacement than with one.
An empty element in a multi-value flag is only visible while it is being set. pflag's stringSlice splits each value with a CSV reader, which yields nothing for an empty string, so `--exclude "" --exclude vendor` arrives at the end of parsing as a list holding only vendor: nothing downstream can tell it from a list that never mentioned the first one. A fingerprint taken that way excludes less than it was told to, and every attestation made with it then refers to an artifact that was never built. Wrapping each flag's value moves the refusal to the only moment that information exists, and wrapping by a walk over the command tree means a new command cannot be added without it. Config-file and environment values go through Set too, so one path covers every source. This replaces two narrower mechanisms and their duplication: the nonEmptyStringSlice type, which only two flags used, and the post-parse check, which could only see whether a flag had ended up carrying nothing. snapshot k8s no longer needs its own --config-file emptiness check either. Reporting through the root's flag-error function keeps one wording for every flag, including the int and bool values pflag used to reject in its own words. The audit re-run beside this records the result: 700 of 700 combinations refused by the CLI, in one message.
The documents described a rule spread over three partial mechanisms and argued about which layer should own it. One wrapper owns it, so the survey of layers and the table weighing them describe a choice that is no longer open, and the file citations point at code that is gone. Appendix 1 changes its claim rather than its wording. It explained why the CLI checked less inside CI: a default filled in from a CI variable stopped RequireFlags marking the flag required, which took it out of the check's scope. The wrapper refuses what a command is handed, so defaults do not enter into it, and the audit run with CI variables set records all 700 combinations refused by the CLI. That closes cli#1088. The file is renamed because its subject moved: sizing unstarted work is now one table inside a description of how the rule works.
…names Building the substitution pattern by joining the fixture names with "|" makes the names part of the regex, so a name carrying a regex character changes what the pattern matches, and the alternation grows with the number of names. Snyk Code reads it as a ReDoS sink (python/reDOS) in audit.py, and in replay.py through its import. Matching any placeholder with a static pattern and looking the name up afterwards removes both the sink and the latent bug. An unknown placeholder still survives untouched, and a value that is itself empty braces still has nothing between them to match. The results beside this are the same 700 rows refused by the CLI; the lines that differ do so only in the measured seconds.
runMultiHost splits --host itself, so the wrapper around a flag's value never sees the empty element in "hostA,,hostB": one non-empty string reaches the split. What catches it is the per-host call, which appends each element as its own --host and so hands the empty one to Set. The error is worth pinning as it stands, label and all: it names the host it came from, and for this element that name is empty.
Three of the wrapper's methods had no test. Append and Replace are the element-level ones nothing in the CLI calls, which is why they were untested and why testing them matters: the guarantee they exist for is the one no current caller exercises. Removing Append's refusal fails TestWrappedFlagAppendRejectsAnEmptyElement, so the test has teeth rather than just coverage. The comma list from argv was untested too. An empty element arriving from the environment was covered, on the route bindFlags takes, but the shape a user types was not.
The refusal changes what a working pipeline does, so the release it ships in needs to say that plainly: accepting an empty flag value was a bug, and a run that starts failing was already producing a result nobody asked for. Saying it at release time means writing it under time pressure, from memory of which examples were measured. It is written here while the measurements are at hand. The release flow drafts dist/release_notes.md for a human to edit, and that file is build output, so this is where the wording can live until then.
Contributor
|
Claude finished @JonJagger's task in 3m 49s —— View job Claude PR Review — Empty flag audit
Verdict: high-quality, well-tested change. The core idea — refuse empty flag values at a single choke point by wrapping every flag's What's done well
Things worth confirming (not blockers)
Nothing here blocks merge from a code-correctness standpoint. Main ask is confirming point (1) with the downstream repos before/with release. |
JonJagger
enabled auto-merge (squash)
August 18, 2026 11:29
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The audit is developer tooling, run by hand and never shipped, so it belongs beside the other tooling in hack/ rather than at the repository root next to cmd/ and internal/. coverage.json goes the other way, to cmd/kosli/testdata/. The Go test writes that file from the live command tree and the audit only reads it, so the module now owns the fixture it produces and the tool reaches in for it. That is the direction the data already flowed, and it means the test no longer refers to the tooling directory at all: remove the tooling and the audit stops, not the test suite. The name gains its subject, empty-flag-audit-coverage.json, because inside testdata/ "coverage" alone would not say which audit, or distinguish it from Go's own coverage output. bootstrap.py's paths-file default and the two entries it has already written into spec.json follow the tooling to its new path. The generator is corrected as well as the artifact, so regenerating produces the same value.
The refusal covers every source, but only the command line got the sentence written for it. A value from a KOSLI_ variable or ~/.kosli.yml is applied by bindFlags rather than parsed by cobra, so it never reaches the flag-error function, and what surfaced was the whole chain: our "failed to set", pflag's "invalid argument", and errEmptyFlagValue itself, which was never meant to be read by anyone. The cause has to be inspected before %v flattens it, which is where the errors.Is test goes. configValueSource now says which kind of source it names. A bare KOSLI_ATTACHMENTS left the reader to know that is a variable and not a file, and the same ambiguity was already in the message for a value that fails to apply for any other reason.
The comment above the leading-dash check still described the empty-variable case, which the value wrapper now refuses before this runs, so it explained a mechanism that is no longer beneath it. It now describes what that block does guard: the next flag swallowed as this flag's value. The audit's token reads like a leaked credential to anyone scanning, and to a reviewer. It authenticates nothing beyond localhost:8001, the same token 76 Go test files use, and the audit must run without a real credential so a run cannot reach a real organisation. Saying so beats leaving it to be inferred.
nonEmptySliceValue embedded nonEmptyValue, which has a wrapped field, and declared a wrapped field of its own for the same value seen as a pflag.SliceValue. Both worked, because the methods that need elements used the outer one and the methods that need a value used the embedded one, but a reader has to work that out before trusting either. The wholly-empty environment variable is the one gap in "an empty value is always an error", and it now has a test saying so: viper treats such a variable as unset, so it never reaches the flag and the refusal cannot speak. Enabling AllowEmptyEnv would change that, and this test fails first.
AlexKantor87
approved these changes
Aug 18, 2026
3 tasks
FayeSGW
added a commit
that referenced
this pull request
Aug 19, 2026
… rule (#1106) * test(attestation-type): reconcile --summary flags with the empty-value rule The summaries PR (#1099) and the empty-flag audit PR (#1103) were both green on their own branches and red together on main. #1103 made every flag refuse an empty value while pflag is still parsing, which lands before the summary flags' own validation, so three cases pinned behaviour that can no longer happen: - `--summary-json ''` was accepted as a no-op; it is now refused. - `--summary ''` reported its NAME=EXPRESSION wording; the empty-value rule speaks first. - `--summary X --summary-json ''` reported the mutual exclusion; parsing refuses before PreRunE runs. Each still fails for the reason it was written to pin, in the one wording every flag now uses. '[]' stays accepted: a summary with no entries is not a missing value. Also registers the two new flags with the audit, which pins its coverage to the command tree and so fails on a flag it does not know about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(attestation-type): reject a whitespace-only --summary-json parseSummaryJSON trimmed before testing for a blank value, so `--summary-json " "` was read as the flag being left out and the summary was silently dropped. That is the trap the empty-value rule exists to close, reached with a space instead of nothing: `--summary-json "$VAR"` with VAR set to a space created the type with no summary and said nothing. Only "" is the flag being left out, because the rule refuses an empty value on the command line. Whitespace was written on purpose, so it now falls through to json.Unmarshal and is reported as invalid JSON. The same gap is open on every other flag, since the rule tests `value == ""` rather than trimming. Closing it there changes behaviour on all 165 flags and wants its own audit run, so it is left alone here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR