Skip to content

Commit 320c4f7

Browse files
committed
fix(empty-flag-audit): measure the 23 combinations it could not
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>
1 parent 4c67680 commit 320c4f7

6 files changed

Lines changed: 175 additions & 116 deletions

File tree

‎docs/handover/2026-08-13-empty-value-decision.md‎

Lines changed: 51 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,11 @@ It measured 374 of the 653 combinations. Of those:
5959

6060
| What happens to an empty value | On a laptop | Inside GitHub Actions |
6161
|---|---|---|
62-
| the CLI refuses it | 190 | 188 |
62+
| the CLI refuses it | 203 | 201 |
6363
| the CLI accepts it and the server refuses it | 5 | 7 |
64-
| nothing refuses it | 156 | 156 |
65-
| the command did not work with any value, so nothing can be said | 23 | 23 |
64+
| nothing refuses it | 166 | 166 |
6665

67-
Of the 156 that nothing refuses, 152 do exactly what omitting the flag does, so
66+
Of the 166 that nothing refuses, 162 do exactly what omitting the flag does, so
6867
an empty value there is merely useless. The rest of this document is about the
6968
ones where it is not.
7069

@@ -98,8 +97,8 @@ which is all it takes.
9897
| `kosli attach-policy P --environment "$VAR"` | the policy is attached to no environment. Anything deployed there is judged without it | changes compliance |
9998
| `kosli detach-policy P --environment "$VAR"` | the policy is detached from no environment, so it stays in force | changes compliance |
10099
| `kosli create environment E --type logical --included-environments "$VAR"` | the record cannot be read back, and `list environments` returns HTTP 500 for every environment in the org until it is removed | **outright bug**, written up in `2026-08-13-included-environments-500.md` |
101-
| `kosli list environments --tag "$VAR"` | answers "No environments were found", identical to a real no-match, exit 0 | wrong answer |
102100
| `kosli create flow F` with no `--description` at all | wipes the description the flow already had. Same for `begin trail` and `create policy` | **outright bug**, no empty value needed, written up in `2026-08-13-description-wiped-on-upsert.md` |
101+
| `kosli list environments --tag "$VAR"` | answers "No environments were found", identical to a real no-match, exit 0 | wrong answer |
103102

104103
Ten findings, from one slice of one CLI, all of them silent. What the rest of the
105104
space holds we do not know - and that is the argument. We cannot keep finding
@@ -173,19 +172,35 @@ sharper than usual:
173172
anything, so "breaking" here means pipelines failing with no change on their
174173
side.
175174

176-
We are on v2.36.5, and #1059 already collects breaking changes for v3, which is
177-
where this belongs - unless we add `--clear-description` at the same time, which
178-
would keep the one capability this removes and make that part non-breaking.
175+
We are on v2.36.5, and #1059 collects breaking changes for v3. This does not have
176+
to join that batch, and I do not think it should:
177+
178+
- **A customer whose pipeline breaks should be able to read one release note and
179+
know why.** A major version carrying ten unrelated breaks cannot tell them
180+
that.
181+
- **The v3 batch has been accumulating for a long time.** Tying this to it means
182+
the compliance holes above stay open until everything else in it is ready.
183+
- **The right moment for this one is knowable on its own.** Step 2 reports how
184+
often empty values actually occur, so we can see when the impact has fallen
185+
far enough to flip the switch. That signal says nothing about whatever else is
186+
queued for v3.
187+
188+
So: this becomes its own major release, and the changes currently queued for v3
189+
become the one after. Major versions are cheap; a release note nobody can act on
190+
is not.
191+
192+
If we add `--clear-description` at the same time, the one capability this removes
193+
comes back, and that part stops being breaking at all.
179194

180195
### Proposed: four steps
181196

182-
156 combinations changing at once is a lot to ask of customers in one upgrade,
197+
166 combinations changing at once is a lot to ask of customers in one upgrade,
183198
so the rule arrives in stages.
184199

185200
#### Step 1: somewhere to put a warning, in app.kosli.com
186201

187202
Nobody reads warnings in a CI workflow run. A step that only prints one is not a
188-
migration, it is a delay, and we would arrive at v3 knowing no more than we do
203+
migration, it is a delay, and we would reach step 3 knowing no more than we do
189204
now. So before the CLI warns about anything, there has to be somewhere for the
190205
warning to go.
191206

@@ -194,7 +209,7 @@ A warning goes to two places, and no more than two:
194209
1. **The workflow run**, printed as now.
195210
2. **app.kosli.com, at the org level.** A command that has `--org` and
196211
`--api-token` can send the warning whatever else it was doing, so this covers
197-
153 of the 156. The exception is `kosli fingerprint`, which is entirely local
212+
163 of the 166. The exception is `kosli fingerprint`, which is entirely local
198213
and needs no credentials.
199214

200215
This is work in app.kosli.com: somewhere to receive the warnings, and one place
@@ -207,12 +222,15 @@ Fix the two outright bugs - the description wiping and the
207222
the flag, and report it. Nothing starts failing, and anyone whose pipeline has an
208223
unset variable can see it and fix it before it costs them anything.
209224

210-
#### Step 3: the warning becomes the error, in v3
225+
#### Step 3: the warning becomes the error, in a major release of its own
226+
227+
One guard, one migration, one release note, and nothing else breaking in the same
228+
version. Ship `KOSLI_ALLOW_EMPTY_FLAG_VALUES=true` alongside it as an escape
229+
hatch, so anyone caught out has a one-line unblock while they fix the pipeline,
230+
and remove it in the next major release.
211231

212-
One guard, one migration, one release note. Ship
213-
`KOSLI_ALLOW_EMPTY_FLAG_VALUES=true` alongside it as an escape hatch, so anyone
214-
caught out has a one-line unblock while they fix the pipeline, and remove it in
215-
v4.
232+
When to ship it is a question step 2 answers: when the reported warnings have
233+
fallen far enough that the remaining breakage is small and known.
216234

217235
#### Step 4: delete what the guard replaced
218236

@@ -221,12 +239,12 @@ one rule covers every flag. This is the step that is easiest to skip and the
221239
reason the CLI is inconsistent today, so it belongs in the plan rather than in
222240
someone's memory.
223241

224-
### Why steps 1 and 2 come before v3
242+
### Why steps 1 and 2 come first
225243

226244
Reporting warnings is not only a kindness to customers. It answers the question a
227-
v3 release note cannot: how much would v3 actually break? Today that is an
228-
argument. With this, by the time v3 is due, it is a number, per org, and we can
229-
tell the customers who are affected before it lands rather than after.
245+
release note cannot: how much would step 3 actually break? Today that is an
246+
argument. With this it becomes a number, per org, and we can tell the customers
247+
who are affected before it lands rather than after.
230248

231249
It also reaches where this audit could not. The 279 combinations on commands
232250
needing AWS, Azure, a git provider and the rest are unmeasured here for want of
@@ -279,15 +297,15 @@ the 152 names are:
279297

280298
| What the flag is for, with a few examples | Names | Does an empty value mean anything? | CLI always refuses | Only the server refuses | Refuses on some commands | Never refuses | Not measured |
281299
|---|---|---|---|---|---|---|---|
282-
| identity and selection - `--flow`, `--trail`, `--fingerprint`, `--name` | 46 | no. There is no artifact called "" | 12 | 2 | 7 | 12 | 13 |
283-
| location and input - `--template-file`, `--results-dir`, `--paths` | 26 | no. There is no file called "" | 6 | 1 | 0 | 7 | 12 |
284-
| filters - `--exclude`, `--namespaces`, `--services`, `--attestations` | 22 | no. Filtering on "" filters on nothing | 2 | 0 | 0 | 5 | 15 |
300+
| identity and selection - `--flow`, `--trail`, `--fingerprint`, `--name` | 46 | no. There is no artifact called "" | 12 | 1 | 9 | 11 | 13 |
301+
| location and input - `--template-file`, `--results-dir`, `--paths` | 26 | no. There is no file called "" | 7 | 1 | 0 | 6 | 12 |
302+
| filters - `--exclude`, `--namespaces`, `--services`, `--attestations` | 22 | no. Filtering on "" filters on nothing | 1 | 0 | 0 | 6 | 15 |
285303
| credentials - `--github-token`, `--aws-secret-key`, `--registry-password` | 17 | no. There is no token "" | 0 | 0 | 0 | 2 | 15 |
286-
| output and paging - `--output`, `--sort`, `--page`, `--reverse` | 14 | no | 6 | 0 | 1 | 5 | 2 |
287-
| behaviour switches - `--dry-run`, `--assert`, `--compliant` | 11 | no | 8 | 0 | 1 | 0 | 2 |
304+
| output and paging - `--output`, `--sort`, `--page`, `--reverse` | 14 | no | 7 | 1 | 0 | 4 | 2 |
305+
| behaviour switches - `--dry-run`, `--assert`, `--compliant` | 11 | no | 9 | 0 | 0 | 0 | 2 |
288306
| free-text metadata - `--description`, `--comment`, `--reason`, `--tag` | 9 | **sometimes** | 4 | 0 | 1 | 4 | 0 |
289307
| the global flags - `--org`, `--api-token`, `--host`, `--debug` | 7 | no | 6 | 0 | 0 | 1 | 0 |
290-
| **total** | **152** | | **44** | **3** | **10** | **36** | **59** |
308+
| **total** | **152** | | **46** | **3** | **10** | **34** | **59** |
291309

292310
The credentials row is the one to look at twice, and it is mostly unmeasured: 9
293311
of its 11 names appear only on commands needing a service this audit cannot
@@ -325,7 +343,7 @@ listed once.
325343
| `--api-token` | global | 1 of 1 | always |
326344
| `--archived` | filter | 1 of 1 | always |
327345
| `--artifact-type` | identity | 9 of 16 | some commands |
328-
| `--assert` | switch | 4 of 9 | some commands |
346+
| `--assert` | switch | 4 of 9 | always |
329347
| `--assume-yes` | switch | 2 of 2 | always |
330348
| `--attachments` | identity | 4 of 11 | always |
331349
| `--attestation-data` | identity | 1 of 1 | always |
@@ -391,7 +409,7 @@ listed once.
391409
| `--include` | filter | 0 of 2 | not measured |
392410
| `--include-regex` | filter | 0 of 2 | not measured |
393411
| `--included-environments` | filter | 1 of 1 | never |
394-
| `--input-file` | location | 1 of 1 | never |
412+
| `--input-file` | location | 1 of 1 | always |
395413
| `--interval` | output | 2 of 2 | never |
396414
| `--jira-api-token` | credentials | 0 of 1 | not measured |
397415
| `--jira-base-url` | location | 0 of 1 | not measured |
@@ -414,14 +432,14 @@ listed once.
414432
| `--org` | global | 1 of 1 | always |
415433
| `--origin-url` | location | 6 of 13 | never |
416434
| `--original-attestation-type` | identity | 1 of 1 | always |
417-
| `--output` | output | 32 of 33 | some commands |
435+
| `--output` | output | 32 of 33 | always, some only by the server |
418436
| `--page` | output | 8 of 8 | always |
419437
| `--page-limit` | output | 8 of 8 | always |
420438
| `--params` | location | 3 of 3 | never |
421439
| `--path` | location | 1 of 1 | always |
422440
| `--paths-file` | location | 1 of 1 | always |
423441
| `--physical` | identity | 1 of 1 | always |
424-
| `--policy` | identity | 4 of 4 | never |
442+
| `--policy` | identity | 4 of 4 | some commands |
425443
| `--privilege` | identity | 2 of 2 | always, some only by the server |
426444
| `--project` | identity | 0 of 3 | not measured |
427445
| `--provider` | identity | 2 of 3 | never |
@@ -449,7 +467,7 @@ listed once.
449467
| `--services-regex` | filter | 0 of 1 | not measured |
450468
| `--set` | metadata | 1 of 2 | always |
451469
| `--short` | output | 1 of 1 | always |
452-
| `--show-input` | output | 3 of 3 | never |
470+
| `--show-input` | output | 3 of 3 | always |
453471
| `--show-unchanged` | output | 1 of 1 | always |
454472
| `--sonar-api-token` | credentials | 0 of 1 | not measured |
455473
| `--sonar-ce-task-url` | location | 0 of 1 | not measured |
@@ -459,14 +477,14 @@ listed once.
459477
| `--sonar-working-dir` | location | 0 of 1 | not measured |
460478
| `--sort` | output | 1 of 1 | never |
461479
| `--sort-direction` | output | 3 of 3 | never |
462-
| `--space-id` | filter | 1 of 1 | always |
480+
| `--space-id` | filter | 1 of 1 | never |
463481
| `--start` | identity | 1 of 1 | never |
464482
| `--start-ts` | identity | 1 of 1 | always |
465483
| `--tag` | metadata | 3 of 3 | never |
466484
| `--template` | identity | 1 of 1 | always |
467485
| `--template-file` | location | 2 of 2 | never |
468486
| `--trail` | identity | 8 of 15 | some commands |
469-
| `--type` | identity | 4 of 4 | always, some only by the server |
487+
| `--type` | identity | 4 of 4 | some commands |
470488
| `--unset` | metadata | 1 of 2 | never |
471489
| `--upload-results` | switch | 1 of 2 | always |
472490
| `--use-empty-template` | switch | 1 of 1 | always |

‎hack/empty-flag-audit/audit.py‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,38 @@ def reset_server():
303303
)
304304

305305

306+
# Commands whose own results are fine but which leave the server unable to
307+
# answer something else. `create environment --included-environments ""` writes a
308+
# record that makes `list environments` return 500 for the whole org, so it is
309+
# measured after everything that needs the server intact. Remove an entry here
310+
# once the bug behind it is fixed.
311+
MEASURE_LAST = ["create environment"]
312+
313+
314+
def in_order(spec):
315+
"""Return the commands to measure, poisoners last."""
316+
names = sorted(spec)
317+
deferred = [c for c in names if c in MEASURE_LAST]
318+
return [c for c in names if c not in deferred] + deferred
319+
320+
321+
def merged(path, rows):
322+
"""Fold new rows into whatever the file already holds.
323+
324+
A run limited with --only or --flag measures a handful of combinations.
325+
Writing just those would throw away every other result in the file, so the
326+
rows it did measure replace their old selves and the rest are left alone.
327+
"""
328+
header, fresh = rows[0], rows[1:]
329+
kept = {}
330+
if path.exists():
331+
for line in path.read_text().splitlines()[1:]:
332+
kept[tuple(line.split("\t")[:2])] = line
333+
for line in fresh:
334+
kept[tuple(line.split("\t")[:2])] = line
335+
return [header] + sorted(kept.values())
336+
337+
306338
def wanted(spec, only, only_flag):
307339
"""Return the command-and-flag pairs this run will actually measure."""
308340
pairs = []
@@ -320,7 +352,8 @@ def audit(binary, spec, only, only_flag, as_ci, home):
320352
rows = ["command\tflag\tempty_exit\tomitted_exit\tset_exit\trefused_by"
321353
"\tvs_omitted\tvs_set\tmessage"]
322354
total, done = len(wanted(spec, only, only_flag)), 0
323-
for command, entry in sorted(spec.items()):
355+
for command in in_order(spec):
356+
entry = spec[command]
324357
if only and only not in command:
325358
continue
326359
if entry.get("skip"):
@@ -398,7 +431,7 @@ def main():
398431
spec = json.loads(SPEC.read_text())
399432
rows = audit(args.binary, spec, args.only, args.flag, args.ci, home)
400433
out = RESULTS_CI if args.ci else RESULTS
401-
out.write_text("\n".join(rows) + "\n")
434+
out.write_text("\n".join(merged(out, rows)) + "\n")
402435
print(f"\nwrote {out}")
403436

404437

‎hack/empty-flag-audit/bootstrap.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
2525
POLICY_FILE = "cmd/kosli/testdata/policy-files/test-policy.yml"
2626
ARTIFACT_PATH = "cmd/kosli/testdata/person-schema.json"
2727
ATTESTATION_SCHEMA = "cmd/kosli/testdata/person-schema.json"
28-
REGO_POLICY = "cmd/kosli/testdata/policies/deny-no-violations.rego"
28+
# A policy that passes. A denying policy would make every `evaluate` run exit
29+
# non-zero whatever its flags held, leaving nothing to compare an empty value to.
30+
REGO_POLICY = "cmd/kosli/testdata/policies/allow-all.rego"
2931

3032
# The global flags are declared once on the root command and behave the same
3133
# wherever they appear, so they are audited on this command alone rather than
@@ -130,6 +132,10 @@
130132
# Without --input-file the command reads stdin, which the audit does not
131133
# feed, and it fails on end-of-file before reaching its own flags.
132134
"evaluate input": ["input-file", "policy"],
135+
# By the time this runs, the audit has created hundreds of environments, and
136+
# asking for all of them times out. A page limit keeps the request small
137+
# enough to answer, which is all these flags need.
138+
"list environments": ["page-limit"],
133139
}
134140

135141
# The environment a command reports to must be of the type it reports. An

0 commit comments

Comments
 (0)