Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,7 @@ junit-test-results/.
.claude/settings.local.json
client_reference/
__pycache__/
# The empty-flag audit's evidence sidecar: whole request and response bodies for
# every replay, so it is large and changes on every run. results-api.tsv is the
# artefact worth committing and diffing.
hack/empty-flag-audit/results-api-evidence.jsonl
58 changes: 58 additions & 0 deletions hack/empty-flag-audit/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,58 @@ sending it again with one field emptied. It is a separate script rather than a
third mode because it measures something else: `--ci` produces rows that line up
with the plain run's, and these do not.

**`results-api.tsv` is an answer about the server it ran against, not a permanent
fact.** The checked-in file was taken on 2026-08-19 against a server built from
kosli-dev/server master at b9e213d9, which is the first to refuse an empty
`filename` and an empty `template` attestation name. `make test_setup` otherwise
pulls a published image, so a row can change without anything in this directory
changing. Re-run before quoting it, and say which server the run was against.

Running it against a locally built server takes two extra steps, because
`docker-compose.yml` pins `platform: linux/amd64`: build the server image for
amd64 (`docker image build --platform linux/amd64 --target test ...`, since an
arm64 image makes compose try to pull an amd64 one that does not exist), then pass
it in with `KOSLI_SERVER_IMAGE=<tag> ./bin/reset-or-start-server.sh` rather than
`make test_setup`. It runs emulated, so expect the run to take minutes rather than
seconds.

`replay.py` asks two questions of each field, and the second one is only worth
asking when the first says the server accepts the value:

| Column | Question |
|---|---|
| `verdict` | does the server refuse an emptied field? |
| `stored` | when it accepts one, does the empty value reach the record? |

The `stored` answer is `reaches the record` or `does not reach the record`, or a
sentence saying why the row is neither. It is measured by replaying the emptied
payload **without** freshening the name, so the request names the resource the
`set` run created, and by reading that resource back through the spec's own
`verify` steps before and after. A fresh name every time leaves no record holding
a value for the empty one to reach, which is why the freshening that keeps the
`verdict` question honest would destroy this one.

The wording is deliberately about the record rather than about a stored value
being replaced, because the two are not the same event. `create flow` writes its
description in place; the `attest` and `report` commands append a document and a
read answers with the most recent one, destroying nothing. Both show up here as
the empty value reaching the record. What the column is for is deciding whether
the endpoint should accept an empty value for that field at all, and that
question does not depend on which of the two mechanisms is underneath.

Each run also writes `results-api-evidence.jsonl` beside the results, which is
gitignored. Its first line names what the run was against: UTC time, the server
container's image id and start time, the CLI binary's sha256, the audit's git
commit and which of its files were uncommitted. Every line after that is one
replayed request - the label naming its row and role, the method, the url, the
payload as sent, the status, and the whole response body.

The results file keeps only the first 120 characters of an answer, so that a
column holding a whole listing cannot stop the file being diffed. The sidecar is
where an answer that surprises you is read in full, and where "was it even the
same server?" is answered without a rebuild. Both questions cost a day the first
time they were asked without it.

Narrower runs, for working on one entry:

```bash
Expand Down Expand Up @@ -127,6 +179,12 @@ commands, hidden commands, and flags hidden by deprecation.
wherever they appear, so they are audited once, on `archive flow`, rather than on
every command.

**It repeats.** Two `replay.py` runs against one server image, a day apart, gave
identical verdicts for 416 of 417 rows. That is what makes a row that does change
worth investigating rather than shrugging at - and one row did, `list flows
--name`, whose cause is still unknown. Comparing runs is only meaningful because
the rest of the file holds still.

## Known gaps

**The audit invents the values it gives flags.** A flag it knows nothing about
Expand Down
45 changes: 44 additions & 1 deletion hack/empty-flag-audit/docs/2026-08-13-empty-value-decision.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@ An audit easily found nineteen cases so far:

1. the server accepts an empty value it should refuse.
**Proposal** - _not_ covered in this document:
- add schema `minLength: 1` on `filename`, `template`, `remove_tags`
- refuse an empty `filename` and an empty `template` attestation name
- `remove_tags` turned out not to need it, see below
- decide empty query params
- leave the 6 description fields alone
- measured in
`hack/empty-flag-audit/docs/2026-08-15-auditing-empty-values-at-the-api.md`
- what was done, and the trap in the original wording, in "Refusing an empty
value on the server" below

2. the CLI passes an empty value to the server where it should send nothing.
**Proposal** - _not_ covered in this document:
Expand Down Expand Up @@ -200,6 +203,46 @@ It matters most on the flags carrying a verdict - `--compliant`,
bare form on those flags, requiring `--compliant=true`, which is a separate
decision.

## Refusing an empty value on the server (done 2026-08-19)

Problem 1's proposal above originally read "add schema `minLength: 1` on
`filename`, `template`, `remove_tags`". Two of the three landed; the wording of
the third was a trap worth recording, because it would have caused an outage.

**The trap: a model that validates a write may also validate a read.** A Pydantic
constraint is not a rule about incoming requests. It is a rule about every
construction of that model, and in this codebase several models are built both
from a request and from a stored document. Adding a constraint there does not
reject bad input; it rejects **data already in the database**, turning a bad
record into one that cannot be loaded at all. That is strictly worse: a wrong
requirement becomes a failed read, and one record can break a listing for
everyone in the org, which is the shape of
[server issue 6503](https://github.com/kosli-dev/server/issues/6503).

So the question to ask of any empty-value constraint is not "is this field
user-supplied?" but "is this class ever constructed from Mongo?".

| Field | Where the constraint went | Why |
|---|---|---|
| `filename` | `min_length=1` on `CreateArtifact` | Bound only as a FastAPI request body, constructed nowhere else. `ArtifactResponseBase` carries the same field and **is** built from stored artifacts, so it was left permissive on purpose. |
| template attestation name | a check on the write path in `common/flow.py`, not on the model | `Attestation` is built when a stored template is read back, and no shape difference distinguishes a write from reading a legacy record, so there was no model-level place to put it. |
| `remove_tags` | nothing | Measured: an empty entry removes no tag, so it does not reach the record. Refusing it is tidiness, not a fix. |

Both fixes carry a test that loads the bad value from the stored shape and
asserts it still works, and both were checked by restoring the constraint and
watching that test fail. A constraint of this kind that has not been checked that
way has not been checked.

Everything above is about newly submitted values. Records already holding an
empty value stay as they are, and reading them keeps working. Cleaning them up
would be a migration, which nobody has asked for.

Confirmed by measurement rather than by the tests alone: `replay.py` re-run
against a server carrying both fixes answers "the server refuses it" for all five
rows that reach `filename` and `template`, taking the acceptances from 25 to 22.
See "What the re-run changed" in
`2026-08-15-auditing-empty-values-at-the-api.md`.

## The one capability this removes

Two flags, and only two, use an empty value to mean something you cannot say any
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,46 @@ too, adopt patch and every one of them is wrong today.
description survives, so what the CLI sends does not settle it on its own - the
server decides. Only running each one shows which is which.

## The server half, with line numbers (2026-08-19)

Read in the server repo, which is what lines 25-27 above could not do. The three
`create` commands are three different shapes, and only one of them behaves the
way this document assumed.

**`create flow`.** `description: str` is required (`models/flows.py:17`), and the
handler passes it straight to `flow.update` with no presence guard
(`common/flow.py:155`). So a re-run empties the stored description. (`visibility`
on the same endpoint is `None`-tolerant, handled at `common/flow.py:147-149` and
`:164`. Recorded as a fact about the code, not as a decision about description.)

**`create environment`.** Required twice: `description: str`
(`models/environments.py:22`), and a second gate below it raising `BadRequest`
when the key is absent (`model/environments.py:179`). But the update path drops a
falsy description before applying it (`_upsert_data`,
`model/environments.py:266-275`) and writes only `if "description" in data`
(`_apply_update`, `:282`). That is why the description survives, as line 52 above
records. Clearing is done through the PATCH endpoint, whose docstring names the
contrast (`v2/environment.py:87-89`).

**`create policy`.** `description: str = Field("")` (`models/policies.py:25`), so
an absent description has already become `""` by the time the handler sees it,
and the update writes the field whenever it differs from the stored value
(`model/policies/commands.py:180-181`). `comment` is declared the same way
(`models/policies.py:27`). Adding `omitempty` to the CLI changes nothing here on
its own: the model default has to become `None` as well.

## Two questions the line numbers separate

They were one question while the server was unread, and answering only the first
looks like a fix.

1. **Will the server accept an absent field?** Today, no: `create flow` answers
422 and `create environment` answers 400. So a clean CLI payload is blocked on
the server whatever is decided about meaning.
2. **What does an absent field mean once accepted?** Under the apply rule in this
document, "use the default", so a re-run still empties a flow's description and
only the wire payload gets tidier. Under patch, "preserve". Undecided.

## How much is at stake

Less than "data loss" would suggest. A trail keeps its earlier value in its
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ reach.
- Both are measured instances of the gap the decision document argues about:
refusing empty flag values in the CLI does nothing for a customer calling the
API directly, so neither of these closes.
- **Both were closed on the server on 2026-08-19.** The figures in this document
are the measurement that prompted that work, and are left as they were taken;
`results-api.tsv` has since been re-run against the fixed server and now reads
22 acceptances and 56 refusals. What changed, and why the obvious way to change
it would have broken reads of existing records, is in
`2026-08-13-empty-value-decision.md` under "Refusing an empty value on the
server"; which rows moved is in "What the re-run changed" below.
- `replay.py` now also answers, for an accepted value, whether it reaches the
record - the `stored` column, described in the README.
- Every emptied filter is accepted: `tag=`, `search=`, `name=` are answered 200
rather than refused. What that means then differs by endpoint - an empty tag
matches nothing, an empty repo name matches everything - and the status does
Expand Down Expand Up @@ -117,8 +126,8 @@ customer can empty by calling the API however the CLI behaves:
`create flow --template` is the one to look at. `--template ""` is refused by the
CLI, by the wrapper every flag's value carries (`cmd/kosli/nonEmptyValue.go`,
applied by the walk in `cmd/kosli/root.go`).
Sending `template: [""]` straight to the API is accepted, and reading the flow
back shows it stored:
Sending `template: [""]` straight to the API was accepted, and reading the flow
back showed it stored:

```
version: 1
Expand All @@ -138,8 +147,8 @@ document's proposal, which says a CLI rule can only reach CLI traffic, with a
measured instance behind it instead of a worked example.

The four rows reaching `filename` say the same thing about a different field,
and it is the one a customer would feel. The server accepts an artifact whose
`filename` is empty, stores it, and serves it back that way:
and it is the one a customer would feel. The server accepted an artifact whose
`filename` is empty, stored it, and served it back that way:

```
$ kosli get artifact FLOW@aaaa...
Expand Down Expand Up @@ -319,3 +328,54 @@ It also gives the server-side work a test list. What the server has to enforce
is not "refuse an empty value" but "an absent field means what the verb says it
means", and a probe that replays every captured request with one field emptied,
and again with it removed, is how that gets checked rather than asserted.

That test list was used. The two rows with a named consequence - `template` and
`filename` - were closed on the server on 2026-08-19, each with an endpoint test
asserting the refusal and a second test asserting that a record already holding
the empty value still loads. The second test is the one this probe could not have
suggested: measuring what a request does says nothing about what a schema change
would do to data already stored, and that is where the obvious fix turned out to
be an outage. See "Refusing an empty value on the server" in
`2026-08-13-empty-value-decision.md`.

The 25 acceptances also collapse further than the row count suggests. Four rows
reach one `filename` field on one endpoint, so they are four CLI routes to a
single server-side defect rather than four defects. A row count is a count of
command-and-flag pairs, not of things to fix.

## What the re-run changed (2026-08-19)

Re-run against a server built from kosli-dev/server master at b9e213d9. Eight of
the 417 rows moved. Five are the fixes:

| Row | Field | Now |
|---|---|---|
| `attest artifact --artifact-type` | `filename` | refuses |
| `attest artifact --display-name` | `filename` | refuses |
| `report artifact --artifact-type` | `filename` | refuses |
| `report artifact --name` | `filename` | refuses |
| `create flow --template` | `template` | refuses |

Three moved for reasons other than this work, and the third of them turned out not
to be a move at all:

- `attest override --commit` went from an unusable control to a real refusal,
because [server#6504](https://github.com/kosli-dev/server/issues/6504) is closed
and the control request now succeeds.
- `create environment --included-environments` went from refusing to accepting,
because [server#6503](https://github.com/kosli-dev/server/issues/6503) is closed
and a logical environment with nothing included is now a record the server can
handle. Its `stored` answer is "does not reach the record".
- `list flows --name` went from refusing to accepting, and this is **not
attributable to the fixes**. Running the whole audit against the pre-fix image
again gives "accepts" for that row, with all 416 other rows identical to the
earlier pre-fix run, so the row differs between runs of the same server rather
than between servers. The 400 it recorded on 2026-08-18, `{"message":"Empty
string is not allowed for flow names."}`, could not be reproduced by curl, by an
isolated probe of that row, or by a full run, and its message can only come from
a create or a rename, neither of which a GET of the list endpoint reaches. Cause
unknown; treat the row as unstable rather than as a finding.

Every write acceptance that remains is either a description field, which is out of
scope by decision, or a value that does not reach the record. Nothing with a named
consequence is still open.
Loading
Loading