Skip to content

fix(query): validate --where and --sort field names against the corpus - #129

Merged
pablontiv merged 5 commits into
masterfrom
pablontiv/w5-issue63-field-validation
Aug 6, 2026
Merged

fix(query): validate --where and --sort field names against the corpus#129
pablontiv merged 5 commits into
masterfrom
pablontiv/w5-issue63-field-validation

Conversation

@pablontiv

Copy link
Copy Markdown
Owner

Slice B of 3 for #63. Stacked on #127 (slice A), which is itself stacked on #124.

Root cause

Two sub-defects, one omission: a field name the user typed was never checked against the field names that exist.

§4 — the documented warning was unreachable. cmd/rootline/filter.go gates query.CheckFieldNames on a non-empty knownFields slice, and all five production call sites passed nilgraph.go, query.go, stats.go, tree.go, validate.go. The function was exercised only by its own unit test and one e2e test, both calling it directly. So a misspelled --where field — the highest-frequency user error on this CLI — was indistinguishable from "no records match": zero results, empty stderr, exit 0. In CI that reads as a green check. CLAUDE.md documented the feature as shipping.

§7 — --sort validated half its input. internal/query/sort.go treats "field absent on both records" as continue, so with a nonexistent key every comparison falls through and sort.SliceStable preserves scan order. A typo produced plausible-looking, unsorted output at exit 0 — while a bad direction was already caught cleanly with rc=1.

Approach

knownWhereFields builds one field set that both checks use:

  • the query builtins query.BuildEnv always injects (path, body, type, sections),
  • every key any record in the scanned corpus carries (frontmatter and derived),
  • every schema: / derive: / aggregate: name the effective .stem chain declares.

The union is deliberately generous. A false "unknown field" on a name that does work is worse than the silence it replaces: it teaches callers to ignore the warning. For the same reason it returns nil for a corpus with neither records nor schema — there is nothing to check against there, and every name would look wrong.

It is computed before --where narrows the corpus, so --where 'estado == "NeverMatches"' --sort estado:asc does not report estado as unknown.

filterRecords now takes the warning writer as a parameter rather than reaching for os.Stderr, so warnings land on the command's error stream and are testable like every other output in this package.

Decisions

  • --where stays a WARNING; the exit code is untouched. internal/query/field_check.go already returns FieldWarning with warning-shaped prose, and a field absent from every record is a legal filter that yields zero matches. Promoting that to rc=1 would break existing pipelines.
  • --sort is an ERROR. The issue is explicit that a bad field deserves what a bad direction already gets. Unlike --where, an unsortable key cannot produce a defensible result: the output is silently in scan order and indistinguishable from a correct sort.

Evidence

Before

$ rootline query docs/ --count --where "estadoo == 'Pending'"
{"version":1,"kind":"rootline/count","meta":{"count":0},"count":0}
# rc=0, stderr empty
$ rootline stats docs/ --where "estadoo == 'Pending'"
{"version":1,"kind":"rootline/stats","by_lifecycle_state":{},"by_record_type":{},"total":0}
# rc=0, stderr empty
$ rootline tree docs/ --where "estadoo == 'Pending'"
{"version":2,"kind":"rootline/tree","root":{"name":"docs","path":"docs","total":0}}
# rc=0, stderr empty
$ rootline validate --all docs/ --where "estadoo == 'Pending'"
{"version":2,"kind":"rootline/validate-batch","results":[],...}
# rc=0, stderr empty
$ rootline graph docs/ --check --where "estadoo == 'Pending'"
No cycles or broken links found.
# rc=0, stderr empty

$ rootline query docs/ --select path,estado --sort "nosuch:asc" -o csv
path,estado
r1.md,Pending
r2.md,Done
r3.md,Pending
...
# rc=0 — scan order, silently unsorted

After

$ rootline query docs/ --count --where "estadoo == 'Pending'"
warning: unknown field "estadoo" in where expression (did you mean "estado"?)
{"version":1,"kind":"rootline/count","meta":{"count":0},"count":0}
# rc=0

$ rootline stats docs/ --where "estadoo == 'Pending'"
warning: unknown field "estadoo" in where expression (did you mean "estado"?)
{"version":1,"kind":"rootline/stats","by_lifecycle_state":{},"by_record_type":{},"total":0}
# rc=0

$ rootline tree docs/ --where "estadoo == 'Pending'"
warning: unknown field "estadoo" in where expression (did you mean "estado"?)
{"version":2,"kind":"rootline/tree","root":{"name":"docs","path":"docs","total":0}}
# rc=0

$ rootline validate --all docs/ --where "estadoo == 'Pending'"
warning: unknown field "estadoo" in where expression (did you mean "estado"?)
{"version":2,"kind":"rootline/validate-batch","results":[],...}
# rc=0

$ rootline graph docs/ --check --where "estadoo == 'Pending'"
warning: unknown field "estadoo" in where expression (did you mean "estado"?)
No cycles or broken links found.
# rc=0

$ rootline query docs/ --select path,estado --sort "nosuch:asc" -o csv
Error: unknown sort field "nosuch": no record carries it and no .stem in scope declares it
# rc=1

$ rootline query docs/ --select path,estado --sort "estadoo:asc" -o csv
Error: unknown sort field "estadoo": no record carries it and no .stem in scope declares it (did you mean "estado"?)
# rc=1

No false positives, verified by test: estado (schema), path and body (builtins), a frontmatter key no .stem declares, a valid sort key surviving a filter that matches nothing, and an empty corpus all stay silent.

Gates

Gate Result
just check pass, 0 issues
just test (-race) pass
just coverage-check pass — cmd/rootline 86.8%, internal/query 89.1%, total 89.6%

Not in this slice

§5 --field repeatability and §6 --field on non-JSON output → slice C.

Refs #63

…alth

`validate --all` folded `.stem` health diagnostics and directory structural
verdicts into the same `results` slice and `summary.total` as documents, so
`total` was not a record count: a three-document corpus with two health findings
reported 5 while `query --count`, `tree --field root.total` and `stats --field
total` all reported 3 on the same path. Those pseudo-records also survived a
`--where` filter they carry no frontmatter to match, and an emptied or renamed
path reported `total: 1, valid: 1` — the `stem-files-exist` entry — which a CI
gate read as green.

Two of the twelve health checks were unreachable through the command. Stem
health ran first, then `index.Scan` failed and every Phase-1 diagnostic was
discarded: `yaml-valid` and `stem-files-exist` are exactly the checks that fire
when the scan cannot succeed, so a consumer asking for JSON got a raw Go error
on stderr and no JSON at all.

The envelope also changed shape by invocation: a bare `rootline/validate` object
for one file, `rootline/validate-batch` for several, and zero bytes for
`--staged` with an empty index — breaking the pre-commit hook idiom
`rootline validate --staged | jq -e '.summary.invalid == 0'`.

Now every invocation emits `rootline/validate-batch` version 2 with six
always-present keys: `results` (documents), `structural` (directories),
`stem_health` (`.stem` files, with `error`/`warn`/`info` severity), `notices`
(run-level, keyed by a stable `code`), `drift_warnings`, and `summary`. The
populations are disjoint and counted on their own axes; splitting them changed
where a verdict is reported, never whether it counts, so an error on any axis
still exits 1.

Also fixed, in the same surface:

- `nested-root-marker` is delivered at `info` as authored. The severity mapper
  handled only `pass` and `fail`, promoting it to a warning that failed
  `--strict` with no way to suppress a supported configuration.
- `monotonic-violations` names the category it detected. Type widening, required
  loosening, severity loosening and structural loosening all rendered as
  `(type change: ...)`, and structural paths were truncated to the field
  `structural`, making `min_children` and `max_children` indistinguishable.

BREAKING CHANGE: `validate` emits `rootline/validate-batch` version 2 for every
invocation. Read a single-file verdict as `.results[0]`; `--field valid` becomes
`--field "results[].valid"`. `.stem` findings move from `results[]` to
`stem_health[]`, directory verdicts to `structural[]`, and `summary.total` is now
a record count. See the upgrade table in docs/validate.md.

Closes #68
--output advertised json|jsonl|csv|table and validated none of it. The only
root PersistentPreRunE was boundaryPreflight, and all 23 consumers were bare
equality tests, so -o sdlkfj, -o JSON and -o "" each exited 0 with whatever
the command's default branch happened to be.

Three failures shared that root cause. jsonl and csv fell through to JSON on
validate, stats, describe and explain. On tree and graph the test was
inverted (== "json"), so those same values fell through to an ASCII tree or a
Graphviz document — contradicting graph's own help, which binds the diagram to
-o table. And graph --check returned before any format dispatch, accepting an
--output it could never honour.

cmd/rootline/output.go now holds the advertised enum and a per-command-path
table of what each command implements; rootPreflight rejects an unknown value,
then an unsupported one, before the command body runs. A central table rather
than 23 switches: a switch per command is a default arm per command to forget,
and TestCommandOutputFormats_CoversEveryCommand fails CI when a new command
has no entry. Unsupported pairs reject rather than gain a writer — describe
has no defensible CSV shape, and a wrong CSV is worse than a refusal.

tree and graph additionally get their dispatch corrected so the diagram is
reachable only from -o table. graph --check rejects an explicitly-set --output
(cmd.Flags().Changed), which leaves the documented default invocation, and
every CI pipeline calling it, untouched.

BREAKING CHANGE: input the CLI previously accepted and discarded now exits 1.
Callers passing an unknown --output, or jsonl/csv to a command that never
implemented them, must correct the flag. Emitting a rootline/graph-check
envelope is deliberately out of scope: docs/graph.md publishes the text-only
contract and a new kind needs its own shape agreed.

Refs #63
The unknown-field warning CLAUDE.md documents as shipping was unreachable.
filter.go gates query.CheckFieldNames on a non-empty knownFields slice and all
five production call sites passed nil, so a misspelled --where field was
indistinguishable from "no records match": zero results, empty stderr, exit 0,
on query, stats, tree, graph and validate --all. In CI that reads as a green
check. The same omission cost --sort its field validation: SortRecords treats
"absent on both records" as "equal", so every comparison fell through and
sort.SliceStable preserved scan order — while a bad sort *direction* already
exited 1, so the command validated half its input.

knownWhereFields builds one field set for both: the query builtins, every key
the scanned corpus carries, and every schema:/derive:/aggregate: name the
effective .stem chain declares. The union is deliberately generous — a false
"unknown field" on a name that does work would teach callers to ignore the
warning — and it is nil for a corpus with neither records nor schema, where
there is nothing to check against.

It is computed BEFORE --where narrows the corpus, so a filter matching nothing
cannot invalidate a valid --sort key.

--where stays a WARNING with the exit code untouched: a field absent from every
record is a legal filter yielding zero matches, and pipelines depend on that.
--sort is an ERROR, matching what a bad direction already got: an unsortable
key produces output that is silently in scan order and indistinguishable from
a correct sort.

filterRecords now takes the warning writer as a parameter instead of reaching
for os.Stderr, so warnings land on the command's error stream and are testable.

Refs #63
@pablontiv

Copy link
Copy Markdown
Owner Author

Why this PR shows zero checks

This PR targets pablontiv/w5-issue63-output-contract (PR #127), not master. .github/workflows/ci.yml filters both push and pull_request on branches: [main, master], and GitHub matches that filter against the base branch — so no workflow fires at all. mergeStateStatus is CLEAN with an empty statusCheckRollup, which is visually indistinguishable from a fully passing PR. Tracked in #130.

Local evidence in place of CI

Every CI job was reproduced locally against this PR's head (03cf8cd, which contains PR #127 and PR #124) in a clean detached worktree:

CI job Local equivalent Result
ci (build, vet, lint) just check pass
ci (tests) just test (go test ./... -race) pass
ci (coverage ≥ 85) just coverage-check pass — total 89.5%, no package below floor
docs-validate go build -o rootline ./cmd/rootline/ && ./rootline validate --all docs/roadmap/ exit 0
installer-tests sh tests/installers/install-sh-test.sh exit 0
gitleaks gitleaks detect 1022 commits scanned, no leaks

Commit hygiene: exactly one commit, subject identical to the PR title, no AI attribution or Co-Authored-By trailer.

This is orchestrator-side verification, not a substitute for the real checks. Once #130 lands and this branch picks up the fixed workflow, GitHub-side checks should appear.

@pablontiv
pablontiv changed the base branch from pablontiv/w5-issue63-output-contract to master August 6, 2026 12:47
Reconcile this branch with master after its former parent (#127, the
--output format contract) was squash-merged, alongside the validate
envelope (#124) and the repair-surface contract (#133).

All conflicts were additive: CHANGELOG.md, CLAUDE.md, docs/graph.md and
.claude/skills/rootline/ref-query.md keep both sides, and
cmd/rootline/staged_test.go keeps master's new
TestGetStagedFilesIgnoresAmbientGitScope next to the existing empty-index
assertions. The resulting diff against master is exactly this PR's own
change: field-name validation for --where (warning) and --sort (error).
@pablontiv
pablontiv merged commit 9d516a2 into master Aug 6, 2026
11 checks passed
@pablontiv
pablontiv deleted the pablontiv/w5-issue63-field-validation branch August 6, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant