Skip to content

fix(cli): parse --at with the canonical time-spec grammar, not a commit prefix; render JSON-LD SELECT as JSON under --format table - #1871

Open
aaj3f wants to merge 11 commits into
mainfrom
fix/cli-time-spec-grammar
Open

aaj3f wants to merge 11 commits into
mainfrom
fix/cli-time-spec-grammar

Conversation

@aaj3f

@aaj3f aaj3f commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

fluree query --at t:5 is parsed as a commit-id prefix and fails with Commit prefix must be at least 6 characters, got 3. This makes the canonical spelling work — along with the four others that were equally broken — on every surface that takes a time spec.

Fixes #1805
Fixes #1872

Correction — this body claimed verification that had not been delivered

Between 2026-09-16 and 2026-09-18 this body described the commit-prefix consolidation below as done, and cited a test, every_surface_shares_one_commit_prefix_floor, twice: once as the assertion that every message quotes the constant, and once in the mutation table as having stayed green when the constant was bumped to 7. On the head that was actually pushed (badaf744f), that test did not exist and the consolidation was not there — three independent definitions of the floor remained. @bplatz caught it by grepping the branch rather than taking this text at its word.

The cause was not a lost rebase. The commit existed locally, gated and mutation-tested, and was never pushed: this body was edited to describe it, then a long test suite blocked the push and it never happened. That explanation is true and it is irrelevant to a reviewer — from outside, the tree is the only evidence, and the tree said the test did not exist. A test that does not exist cannot have gone green.

One thing worth naming, because it is worse than it first looks. The mutation was not weak-but-green; it ran against local state that genuinely had the consolidation, so it passed truthfully. The result was true and unverifiable — and a reader cannot tell that apart from a weak check, because a weak check at least leaves its evidence in the tree to be re-examined. Nothing I published gave anyone a way to distinguish them.

There was no shipped-behaviour defect. All three definitions were 6, so every surface agreed, and CI was legitimately green on what was delivered. The damage was to the record, not to the code.

Now true at 84ea72573, and checkable against the tree rather than against anyone's local history: COMMIT_PREFIX_MIN_LEN is defined once, in fluree-db-core/src/ledger_id.rs, re-exported from ledger_view; MIN_COMMIT_PREFIX_LEN has zero occurrences; core's check and its message both quote the constant; and every_surface_shares_one_commit_prefix_floor exists in fluree-db-api/src/dataset.rs. The citation is kept above rather than deleted, so a reader who found the discrepancy can see how it was closed.

The issue is right, and under-scoped three ways

All five canonical spellings fail, not just t:N. t:, t:latest, iso:, recorded: and commit: all reached the commit-prefix resolver as literal strings named after their own tag. --at commit:abcdef is the clearest tell: it looked up a commit whose hex digest starts with commit:abcdef. Only the two untagged heuristics — a bare integer and a bare ISO-8601 timestamp — ever worked.

Three CLI surfaces are affected, not one: query --at, export --at and history --from/--to. That is two independent copies of the heuristic in the CLI (commands/query.rs, commands/history.rs), a third partial hand-roll in commands/show.rs, and a byte-identical fourth in the server feeding POST /export's at field. A fix confined to query.rs leaves history broken and looks green — the mutation testing below reproduces exactly that.

--at means two contradictory things depending on the subcommand. This is the part the issue does not mention, and the part most likely to regress later:

branch create --at t:2   =>  Created branch 'b' from 'main' at t=2
branch create --at 2     =>  error: Commit prefix must be at least 6 characters, got 1
query --at 2             =>  works
query --at t:2           =>  error: Commit prefix must be at least 6 characters, got 3

Same flag name, exactly inverted grammars. Fixing only the time surfaces relocates that inversion instead of closing it.

Approach: call the parser that already exists

A canonical parser already existed — two, in fact — and parse_time_spec matched neither. fluree_db_core::ledger_id::split_time_travel_suffix is the address grammar (mydb:main@t:5) that the server and every JSON-LD/SPARQL graph source already accept, and fluree_db_api::CommitRef::parse is what branch create --at and branch revert already call. The grammar proposed in the issue is precisely the address grammar minus the @. So this delegates to it rather than extending a second heuristic.

parse_time_travel_spec is extracted from the body of split_time_travel_suffix and is now the only place t: / iso: / recorded: / commit: are recognised anywhere in the tree. TimeSpec::parse exposes it (plus t:latest, which needs the ledger's current t and so cannot live in core); TimeSpec::parse_at layers on the untagged spellings --at has always taken. All four hand-rolled copies are deleted, not patched, along with dataset.rs's @t:latest special case and four separate hand-written copies of the LedgerIdTimeSpec → TimeSpec mapping — dataset.rs plus two more in routes/query.rs that the issue does not mention — now one From impl.

Two parsers, not one — and why that is the point

CommitRef and TimeSpec stay separate, because they denote different things: iso:/recorded:/latest are meaningless for branch revert <commit>, and CommitRef::Exact(CID) is not a point in time. Collapsing them would either hand branch revert three spellings it cannot honour or strip the time axes off --at.

What changes is that they now accept the same spellings wherever they overlapt:N, a bare integer, commit:<prefix> and a bare hex prefix mean the same thing on all eight surfaces. That is what closes the inversion rather than moving it.

Surface t:N bare int latest iso:/recorded: commit:<p>
query --at ✗ → ✗ → ✗ → ✗ →
export --at ✗ → ✗ → ✗ → ✗ →
history --from/--to ✗ → ✗ → ✗ →
branch create --at ✗ → n/a n/a ✗ →
branch revert <commit> ✗ → n/a n/a ✗ →
branch revert --from/--to ✗ → n/a n/a ✗ →
show <commit> ✗ → n/a n/a ✗ →
POST /export at= ✗ → ✗ → ✗ → ✗ →

Eight, not seven: the server's copy has the identical bug over HTTP and the same one-line delegation fixes it.

One behaviour change worth a reviewer's attention

123456 is simultaneously a valid t and a valid six-character hex digest prefix. A bare integer resolves to a t, with commit:<prefix> as the escape hatch. On query --at that is unchanged — it is what --at has always done. On branch create --at, branch revert and show it is new: those three previously read a bare 123456 as a prefix, and now read it as t=123456. Extending the existing resolution is the only reading that lets the two grammars agree, and the tags exist precisely to disambiguate, but it is a real if narrow compatibility edge and I would rather it were argued than discovered.

The failure shape is worth naming rather than leaving to be found: if a t of that value exists, fluree show 123456 does not error — it returns a different commit than the user named. That is the silent-wrong-answer class, which is normally the one to design away. I still think this is the right call, because the alternative — rejecting an ambiguous all-digit string of six or more characters on the commit surfaces — costs exactly the cross-surface consistency the PR is buying, and the input has to be both all-decimal-digits (roughly 6% of 6-char hex prefixes) and a live t to bite. But it is a judgement call and I would rather it were argued here than rediscovered from a support thread.

I did not apply breaking-change: the affected input class is narrow and putting a CLI ergonomics fix in the Breaking Changes section would bury the changes that belong there. Considered and declined rather than overlooked. Happy to be overruled.

recorded: is wired rather than documented as write-only. The AtRecorded variant, its resolver, and time_spec_to_suffix's @recorded: renderer all already shipped, and the address grammar already accepted it — the CLI was simply the one surface that could not name it. It costs one match arm.

parse_time_spec is also now fallible, so --at t:abc is a usage error naming the flag and the accepted spellings rather than a commit-prefix miss reported three layers away.

Also in this PR

The --format table secondary. A JSON-LD SELECT result under --format table renders as ++/|| empty columns while --format json and SPARQL render correctly. commands/query.rs was the only format_result call site passing the requested format straight through instead of coercing JSON-LD to JSON via json_path_display_format, so array-of-array rows reached a renderer that discovers columns via as_object(). One line — plus a guard so the renderer reports what it cannot column-ise instead of rendering it away, and the next caller that skips the coercion gets a diagnostic rather than an empty table under a footer truthfully reporting N rows.

A published contract changed. docs/cli/server-integration.md's "Required semantics" specified the broken heuristic to third-party server implementors as normative — "parse as integer first (t), then as ISO-8601 if it contains both - and :, else as a commit CID prefix." Anyone who built a compatible POST /export from that page reproduced this bug on purpose. It now states the real grammar. That section was also self-inconsistent: it cited "the merge-preview / show contracts" as the same rules, when those parse commit refs, which took t:N and rejected the bare integer it required.

Help text on all six CLI surfaces now lists the accepted spellings, which is the discoverability half of what the issue asks for.

Folded in from review

history was building its time range off the raw alias. format_time_spec pasted :main onto whatever resolve_ledger returned, which is the -l value verbatim or the active ledger straight from config — either of which may already carry a branch. -l mydb:dev became mydb:dev:main, which split_ledger_id reads as a branch named dev:main, and the nameservice rejects it. fluree history failed outright on every branch but main, by both routes that can reach one, while fluree query -l mydb:dev worked fine on the same ledger.

I had originally scoped this out as pre-existing and filed it as #1872. That was the wrong call: the correctly-normalized value was already being computed twelve lines below the call site and used for nothing but the auth path segment, so this is a hoist and a parameter rename. Writing the mutation test also turned up something #1872 does not say — the explicit -l mydb:main spelling was broken too, not just non-main branches, because it is equally branch-qualified. All four cases (mydb, mydb:main, mydb:dev, and fluree use mydb:dev) are verified working.

Two inline copies of json_path_display_format survived the secondary fix, at the tracked-remote and local-view paths, while only the third was converted. All three sites now call the helper. The CONSTRUCT/DESCRIBE shadow at the second site is a separate rule and stays layered on top.

This change is preventive, and worth being clear about that. The duplicates were not wrong — they were the helper's body verbatim, so they computed the identical answer. What they were is the two copies the next edit to the coercion rule would not reach. If we ever decide to keep the table for JSON-LD node-object results — which the renderer handles fine, as jsonld_table_still_renders_node_objects shows — changing the helper would have fixed one of three sites and silently left two. That is the same shape as the primary bug in this PR, where four hand-rolled copies of a heuristic meant patching three left the fourth looking green, and the mutation table below shows exactly what that failure mode looks like.

A pattern worth naming, because this PR hit it twice: a grep-scoped enumeration is a floor, not a count. The time-spec heuristic was reported as living in one place and turned out to be four, plus four more copies of the LedgerIdTimeSpec → TimeSpec mapping. Here, the coercion was identified at four sites and there are in fact seven format_result call sites. So rather than convert the two flagged and stop, I walked all seven. The three not previously enumerated are safe, but for reasons that had to be checked rather than assumed: query.rs:1034 passes a hardcoded Table but sits inside a QueryFormat::Sparql match arm, and the two Cypher sites can only ever produce Json/TypedJson/CypherJson, never Table.

That walk establishes something stronger than the fix asked for: with every coercion site routing through the helper, and format_as_table's JSON-LD arm reachable only via Table + QueryFormat::JsonLd, no caller can now reach format_jsonld_table at all. The reported ++/|| is structurally unreachable rather than fixed at one site, and the renderer guard is defense-in-depth. The honest corollary: its three tests therefore do not cover the reported symptom — they cover the renderer in isolation.

A too-short commit prefix is now rejected in both spellings. commit:abc was rejected at the CLI boundary while a bare abc was accepted and died at the resolver with the same complaint several layers down. Both were errors either way, so this only moves the bare one to the boundary — but "tagged and bare mean the same thing" should hold for the failures too.

That fix initially added a third name for the rule, which is the pattern this PR argues against. Worth stating plainly rather than quietly correcting. The six-character minimum existed as a bare literal in the core address grammar — with the 6 written into its message string too, so the text could drift from the check — as a pub const COMMIT_PREFIX_MIN_LEN on the resolver, and then as a new private mirror in dataset.rs. Closing a duplication nit by duplicating something else is not a close.

COMMIT_PREFIX_MIN_LEN now lives in fluree-db-core, because the address grammar has to apply the floor without a ledger in hand and cannot depend on fluree-db-api. It is re-exported from ledger_view, so fluree_db_api::COMMIT_PREFIX_MIN_LEN still resolves and the static assertion in commands/log.rs is untouched. Every check and every message quotes it, and every_surface_shares_one_commit_prefix_floor asserts the messages do — proved by bumping the constant to 7 and watching that test stay green while only the two tests with deliberately 6-character fixtures moved, and by putting a stale literal back in one message and watching it fail.

Decision: CommitRef::parse does not get the floor

Recorded here because it is a decision, not an oversight, and the opposite call is defensible.

Consistency argues for adding it: commit:abc is one of the spellings this PR makes common to both grammars, and TimeSpec::parse_at rejects it at the boundary while CommitRef::parse defers. Two things decided it the other way.

First, the floor is already applied to every CommitRef::Prefix. LedgerView::resolve_commit routes it through normalize_commit_ref, which enforces the same constant and emits the same message. A check in parse would be a second application on a path that already has one, not a missing one.

Second, and decisively, it would measure the wrong string. normalize_commit_ref strips fluree:commit: and sha256: and decodes canonical CIDs before measuring; CommitRef::parse does none of that. sha256:abc is ten characters at parse and three at resolve, so a parse-time floor would wave through a string the resolver correctly rejects — a check that looks like it ran and did not. The length rule belongs where the stripping happens. The reasoning is on the function so the next person to notice the asymmetry finds the answer rather than re-deriving it.

The user-visible consequence, stated so it is not a surprise: query --at commit:abc reports at the CLI, branch create --at commit:abc reports from the resolver. Same message, same rejection, different layer.

Testing

parse_time_spec had zero tests before this, so all of it is net-new: 11 unit tests on the shared grammar, 4 on CommitRef's overlap, 4 at the CLI boundary, 3 on the renderer guard, and 11 integration tests in a new fluree-db-cli/tests/time_travel_spec.rs covering every spelling on every surface. tests/integration.rs is deliberately untouched.

The highest-value one is a round-trip property: for every TimeSpec, parse_time_spec(time_spec_to_suffix(s)) == s. That pins the CLI's input grammar to its own output grammar and would have caught this at write time — the old parser turned time_spec_to_suffix(AtT(2)) = "@t:2" back into AtCommit("t:2"), which is the @commit:t:2 the issue captured on the wire.

Each fix was proved non-vacuous by reverting it:

Mutation Result
shared parser → the old heuristic 8 of 11 integration tests fail, with the issue's verbatim error Commit prefix must be at least 6 characters, got 3
only history.rs given its copy back exactly the 2 history tests fail, all 9 others pass — the "looks green" failure mode, caught
CommitRef::parse loses its bare-integer arm exactly the 3 commit-naming tests fail; the --at tests are untouched
renderer guard removed both guard tests fail, reproducing the reported ++/`
history reverted to the raw alias + :main the branch test fails with #1872's verbatim error, Invalid ledger ID format 'ttbranch:dev:main'
short-prefix check removed parse_at_rejects_a_short_prefix_in_both_spellings fails
COMMIT_PREFIX_MIN_LEN bumped to 7 every_surface_shares_one_commit_prefix_floor stays green — the constant reaches all three checks and all three messages. (Re-run at 84ea72573; see the correction above — the first time this was reported, the test was not on the pushed branch.)
a stale literal 6 put back in one message that test fails, quoting the drifted text
CommitRef::parse's bare-integer arm removed parse_resolves_the_digits_ambiguity_toward_t fails, now including the negative case

The json_path_display_format convergence is the one change with no behavioural test, and cannot have one: the copies were the helper's body verbatim, so reverting it changes nothing observable. It is a duplication removal, not a fix.

The secondary was additionally verified by hand against a running fluree server + fluree remote add, since that call site is the untracked-remote branch and fluree-db-cli/tests/ stands up no server.

Gates

Run locally, in an isolated worktree:

  • cargo test -p fluree-db-core — pass (783)
  • cargo test -p fluree-db-cli — pass (384, all targets, including the docs_coverage command-tree gate)
  • cargo test -p fluree-db-api — pass (3208, 30 binaries)
  • cargo test -p fluree-db-server — pass (538)
  • cargo clippy -p {core,api,cli,server} --all-targets --no-deps — clean. One pre-existing warning survives in fluree-db-core/src/commit.rs, a file this PR does not touch.
  • cargo fmt --all --check — clean, re-verified after the last edit

Not run: testsuite-sparql (not touched by this change; it is excluded from the workspace and has its own fmt/clippy pair) and a --all-features workspace build. CI is the authority on both.

One coverage gap, stated rather than implied: the one-line coercion at commands/query.rs:846 has no automated test, because that call site is the untracked---remote branch and the CLI test harness stands up no server. The helper it calls is unit-tested, the renderer guard is, and the path was verified live — but the line itself is pinned only by construction.

Perf: no impact. Every touched path parses a handful of bytes once per invocation or once per request; none is inside a query execution loop.

Follow-ups, not fixed here

  • branch create --at <10-char CID prefix> still fails with "is an abbreviated CID, not a commit id this can resolve" while the help advertises prefix resolution. A resolver issue, not a parsing one.

A ledger address's `@` suffix (`mydb:main@t:5`) and a CLI `--at` argument are
the same grammar modulo the `@`, but only the address had a parser. `--at` had
a heuristic that recognised a bare integer and a bare ISO-8601 timestamp and
swept every other string into a commit-prefix lookup, so every canonical
spelling — `t:N`, `t:latest`, `iso:`, `recorded:`, `commit:` — was passed to
the resolver as a literal prefix named after its own tag.

Extract the tag matching out of `split_time_travel_suffix` into
`parse_time_travel_spec`, which takes the spec with or without a leading `@`
and a sigil used only to quote the tag back in errors, so each surface reports
the spelling that was typed. Expose it through `TimeSpec::parse` (the strict
address grammar, plus `t:latest`) and `TimeSpec::parse_at` (that, plus the
bare spellings `--at` has always taken).

`parse_ledger_id_time_travel` now delegates, dropping both its `@t:latest`
special case and its hand-written LedgerIdTimeSpec -> TimeSpec mapping; a
`From` impl replaces that mapping, which was written out four times.

`CommitRef::parse` gains the two spellings it shares with the time grammar —
a bare integer and `commit:<prefix>`. The two parsers stay separate because
they denote different things (a commit has no `iso:`), but they were exactly
inverted: `branch create --at t:2` worked and rejected `2`, while
`query --at 2` worked and rejected `t:2`. A bare integer resolves to a `t`
on both, with `commit:` as the escape hatch for an all-digit hex prefix.
Four independent copies of the same broken heuristic existed: `query.rs`
(serving both `query --at` and `export --at`), `history.rs` (its own, with a
`latest` case the others lacked), `show.rs` (a partial `t:`-only hand-roll),
and one in the server feeding `POST /export`'s `at` field. Delete all four and
call the shared parser.

`parse_time_spec` becomes fallible, so a malformed tagged spec is a usage
error naming the flag and the accepted spellings instead of a commit-prefix
miss reported three layers away. `history` renders its range by parsing with
the same function and rendering with `time_spec_to_suffix`, which makes its
input grammar its own output grammar by construction.

`show` and `branch revert` move to `CommitRef::parse`, so the commit-naming
surfaces accept the spellings they share with `--at`.

Help text on all six surfaces now lists what is accepted, including that a
bare integer is read as a transaction number and `commit:<prefix>` forces a
prefix that is all digits.

Fixes #1805
A JSON-LD SELECT result is an array of positional arrays. The JSON-LD table
renderer discovers columns by inspecting each row as a JSON object, so such a
result produced no columns, no header and no cells: a bare `++`/`||` table
under a footer that truthfully reported the row count. It reproduced only via
`--remote`, because that branch of `run()` was the one `format_result` call
site passing the requested format straight through instead of coercing
JSON-LD to JSON via `json_path_display_format`.

Coerce at that call site, and make the renderer reject input it cannot
column-ise rather than rendering it away. The renderer is reachable only by
passing `Table` for a JSON-LD result, which the coercion exists to prevent, so
the next caller that skips it gets a message naming both the fix and the
guard it missed.
`docs/cli/server-integration.md` specified the broken heuristic to
third-party server implementors as a required semantic — "parse as integer
first, then as ISO-8601 if it contains both `-` and `:`, else as a commit CID
prefix" — so anyone who built a Fluree-compatible `POST /export` from that
page reproduced this bug deliberately. It now states the tagged grammar,
keeps the untagged forms as an explicit compatibility list, and says that a
tagged string is never reinterpreted as an untagged one.

The page was also self-inconsistent: it cited "the merge-preview / show
contracts" as the same rules, but those parse commit refs, which took `t:N`
and rejected the bare integer this text requires. That is now stated as the
overlap it actually is.

Also refreshes the `--at` / `--from` / `--to` / `<COMMIT>` reference rows and
examples on query, export, history, branch, show, the REST endpoint table and
the Rust API page.
`parse_ledger_id_time_travel` delegated through the bare entry point, so
`mydb@t:` reported "Missing value after 't:'" — the address surface quoting
the CLI's spelling of its own tag. Split the two entry points so each names
its own, and pin all four tags on both at the unit level.
TIME_TRAVEL_TAGS documents itself as that order and was not in it. The tags
are mutually non-prefixing so nothing depended on it, but a const whose doc
comment is wrong is worse than one without.
@aaj3f aaj3f added bug Something isn't working as expected area:cli fluree CLI UX, rdf toolkit, publish/export/insert flows area:server HTTP surface, routes, error mapping, swagger, timeouts/admission, config graph labels Sep 16, 2026
`format_time_spec` pasted `:main` onto whatever alias it was handed, and
`resolve_ledger` hands back the `-l` value verbatim or the active ledger
straight out of config — either of which may already carry a branch. So
`-l mydb:dev` became `mydb:dev:main`, which `split_ledger_id` reads as a
branch named `dev:main` and the nameservice rejects. `fluree history` failed
outright on every branch but `main`, by both routes that can reach one, while
`fluree query -l mydb:dev` worked fine on the same ledger.

The correct value was already being computed twelve lines below the call:
`to_ledger_id(&alias)` at the old `:52`, used only for the auth-driving path
segment. Hoist it above `build_history_query` and thread it through instead
of the raw alias. `normalize_ledger_id` appends the default branch when the
alias names none, so the `main` case is byte-identical.

Fixes #1872
The `--format table` fix converted one `format_result` call site and left two
byte-identical inline copies of `json_path_display_format` behind, at the
tracked-remote and local-view paths. That is the same shape as the bug this PR
is fixing: four hand-rolled copies of a time-spec heuristic, of which patching
three would have left the fourth looking green.

No behaviour change — the copies were the helper's body verbatim. What it buys
is that the coercion rule now has one definition, so changing it (for instance
to keep the table for JSON-LD node-object results, which the renderer handles
fine) reaches every site rather than two of three. The CONSTRUCT/DESCRIBE
shadow at the second site is a separate rule and stays layered on top.
`commit:abc` was rejected at the CLI boundary while a bare `abc` was accepted
as an `AtCommit` and died at the resolver several layers down with the same
complaint. Both are errors either way, so this only moves the bare one to the
boundary — but the claim this PR makes is that the tagged and bare spellings
mean the same thing, and that should hold for the failures too.
@aaj3f aaj3f changed the title fix(cli): parse --at with the canonical time-spec grammar, not a commit prefix fix(cli): parse --at with the canonical time-spec grammar, not a commit prefix; render JSON-LD SELECT as JSON under --format table Sep 16, 2026
Closing the tagged-vs-bare asymmetry added a third name for a rule that
already had one. The six-character minimum was a bare literal in the core
address grammar — with the `6` written into its message string too, so the
text would drift silently — a `pub const COMMIT_PREFIX_MIN_LEN` on the
resolver, and a new private `MIN_COMMIT_PREFIX_LEN` mirror in `dataset.rs`.
A nit about duplication closed by adding a copy is the pattern this PR argues
against.

Move `COMMIT_PREFIX_MIN_LEN` into `fluree-db-core`, since the address grammar
has to apply the floor without a ledger in hand and cannot depend on
`fluree-db-api`. Re-export it from `ledger_view` so `fluree_db_api::` still
resolves and `log.rs`'s static assertion is untouched. Every check and every
message now quotes it, and the test asserts the messages do — a literal that
drifts away from the constant is the failure this guards.

`CommitRef::parse` deliberately does not get the check; the reasoning is on
the function. Short version: `resolve_commit` already routes every
`CommitRef::Prefix` through `normalize_commit_ref`, which enforces the same
constant with the same message, and a parse-time copy would measure the wrong
string — `normalize_commit_ref` strips `fluree:commit:` / `sha256:` before
measuring, so `sha256:abc` is ten characters at parse and three at resolve.
The length rule belongs where the stripping happens.
@aaj3f
aaj3f marked this pull request as draft September 17, 2026 03:07
@aaj3f
aaj3f marked this pull request as ready for review September 17, 2026 15:08
@aaj3f
aaj3f requested review from bplatz and zonotope September 17, 2026 15:08

@bplatz bplatz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The core of this is right, and the structural claim holds up under checking.

Verified sound:

  • Four hand-rolled heuristics really did collapse to one. I grepped for survivors: the contains('-') && contains(':') heuristic now exists at exactly one site (dataset.rs:587), and the four LedgerIdTimeSpec -> TimeSpec copies at one From impl (dataset.rs:625). The enumeration in the body is accurate.
  • The core extraction is faithfulparse_time_travel_spec is split_time_travel_suffix's body verbatim, with sigil threaded into error text only, so no grammar changed in the move.
  • CommitRef::parse's new bare-integer arm is properly guarded. bare_integer_arm_does_not_shadow_a_canonical_cid proves the arm ordering cannot swallow a CID by deriving it from a real ContentId, rather than asserting it. That is the test I would have asked for.
  • The digits-ambiguity call is argued honestly and I agree with it. Naming the fluree show 123456 silent-wrong-answer shape explicitly, rather than leaving it to be found, is the right way to land a judgement call like that.
  • The round-trip property (parse_time_spec(time_spec_to_suffix(s)) == s) is the highest-value test here, as the body says.

Please review the three inline comments and address what you judge relevant before merging. One is substantive:

  1. (fluree-db-core/src/ledger_id.rs) The commit-prefix floor consolidation the body describes is not in the code — three independent definitions of 6 remain, this one hardcoded in both the check and the message, and every_surface_shares_one_commit_prefix_floor does not exist in the tree. No user-visible bug today (all three agree), but the drift it guards against is precisely the cross-surface divergence this PR closes. Likely a commit lost in a rebase; either finish it or amend the body.
  2. (fluree-db-api/src/dataset.rs) Cross-reference for the above, plus a note that this site's doc comment is the honest one.
  3. (fluree-db-api/src/ledger_view.rs) Minor: a bare negative integer is now a t; untested.

Nothing here is a correctness problem in the shipped behaviour, which is why this is an approval rather than a hold — (1) is a claim-versus-code gap, and your call on which way to close it.

Comment thread fluree-db-core/src/ledger_id.rs Outdated
}
if val.len() < 6 {
return Err(LedgerIdParseError::new(
"Commit prefix must be at least 6 characters",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit-prefix floor consolidation described in the PR body isn't in the delivered code.

This check and its message both hardcode 6, so neither quotes a shared constant. On this head (badaf74) three independent definitions of the floor exist:

Location Form
here, ledger_id.rs:145,147 bare literal 6 in the check and "at least 6 characters" hardcoded in the message string
fluree-db-api/src/ledger_view.rs:244 pub const COMMIT_PREFIX_MIN_LEN: usize = 6
fluree-db-api/src/dataset.rs:613 const MIN_COMMIT_PREFIX_LEN: usize = 6

That last one is the "new private mirror in dataset.rs" the body identifies as the problem and says was resolved. The body states:

COMMIT_PREFIX_MIN_LEN now lives in fluree-db-core, because the address grammar has to apply the floor without a ledger in hand and cannot depend on fluree-db-api. It is re-exported from ledger_view […] Every check and every message quotes it

COMMIT_PREFIX_MIN_LEN is defined in fluree-db-api/src/ledger_view.rs:244, not in core and not re-exported from it.

And every_surface_shares_one_commit_prefix_floor does not exist anywhere in the tree. I grepped the whole branch. It is cited twice in the body — once as the assertion that every message quotes the constant, once in the mutation table as having stayed green when the constant was bumped to 7. Since this site's literal would not move under that bump, green was ambiguous rather than confirming.

The two tests that do cover this (dataset.rs:1724-1727) assert .contains("at least 6 characters") as a string literal, so they pin the text rather than the constant.

No user-visible bug today — all three are 6. The risk is drift, and it lands exactly on what this PR is buying: bump COMMIT_PREFIX_MIN_LEN to 7 and query --at abcdef still accepts at the boundary while branch create --at abcdef rejects, reintroducing the cross-surface divergence #1805 is about.

My guess is a commit was lost in a rebase, since the body argues the right thing in its own words — "Closing a duplication nit by duplicating something else is not a close." Worth either finishing the consolidation or amending the body so it doesn't claim a gate that didn't run.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, and the explanation is worse than the rebase one — thank you for grepping rather than taking the body at its word.

There was no lost commit. bc94ae599 "Give the commit-prefix floor one definition" exists, was written, gated and mutation-tested, and was never pushed. I edited the PR body to describe it before pushing, then blocked on a long test suite and never came back to push. So the body has been describing work that existed only on my machine for two days. git log origin/fix/cli-time-spec-grammar..HEAD is exactly that one commit.

So every specific you list is accurate at badaf74: three definitions, this site hardcoding 6 in both the check and the message, and every_surface_shares_one_commit_prefix_floor nowhere in the tree. A test that does not exist cannot have gone green, and citing it twice as evidence was the worst part of this.

One correction to your reasoning, and it cuts against me rather than for me. You say this literal would not move under a bump to 7, so the green was ambiguous. On what I delivered, exactly right. But the mutation I actually ran was not ambiguous — it ran against local state that did have the consolidation, and it genuinely passed. Which means the result was true and unverifiable. I think that is a worse failure mode than a weak check, because from outside there is no way to tell the two apart, and I gave you no way to. The check being real is not a defence when nobody else could have run it.

Closing it by finishing the consolidation rather than amending the body, since the consolidation is written and you have confirmed the body argues the right thing. The body gets corrected too — stating what it claimed and what was actually verified, not quietly dropping the citation. I will post the new SHA here when it is up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed: 84ea72573 (fast-forward on badaf744f, two commits — bc94ae599 the consolidation, 84ea72573 the negative-integer test from your third note).

Checkable at that SHA rather than against my local history, which is the whole lesson applied to its own fix:

  • COMMIT_PREFIX_MIN_LEN defined once, fluree-db-core/src/ledger_id.rs:107, re-exported from ledger_view so fluree_db_api::COMMIT_PREFIX_MIN_LEN still resolves and the static assertion in commands/log.rs is untouched
  • MIN_COMMIT_PREFIX_LEN — zero occurrences
  • core’s check at :158 and its message both quote the constant
  • every_surface_shares_one_commit_prefix_floor exists, in fluree-db-api/src/dataset.rs
  • the two tests you flagged at dataset.rs:1724-1727 now derive their expected string from the constant instead of the literal

One thing I owe you that my first reply left out, and its absence was misleading in the other direction: there was no shipped-behaviour defect. All three definitions were 6, so every surface agreed at badaf74, and CI was legitimately green on what was delivered. The drift you identified was the real risk and it was latent, not live. I would rather you did not go looking for a bug that was not there on the strength of how bad the record-keeping was.

The body is corrected in place. It now leads with what it claimed, that the claim described an unpushed commit, and what is actually true at 84ea72573 — and it keeps the every_surface_shares_one_commit_prefix_floor citation rather than deleting it, so anyone who found the discrepancy can see how it closed rather than finding it quietly gone.

Also adopting the obvious convention out of this, since the failure was procedural rather than technical: never edit a PR body to describe a commit that is not yet pushed. The body and the branch move in one direction — push, then describe. What made the gap is that a body edit is instant and a suite run is not, and I was blocked on suites, which was the right thing to be doing. So the rule has to hold even when waiting is correct.

Comment thread fluree-db-api/src/dataset.rs Outdated
/// so both the tagged and the bare spelling are rejected at the same boundary.
/// Must stay in step with the `commit:` arm of
/// [`fluree_db_core::ledger_id::parse_time_travel_spec`].
const MIN_COMMIT_PREFIX_LEN: usize = 6;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-reference to the note on fluree-db-core/src/ledger_id.rs:147 — this is the third of the three definitions of the 6-character floor.

Worth saying that this doc comment is honest about what it is: "mirrored here… Must stay in step with the commit: arm of parse_time_travel_spec." That is an accurate description of a deliberate duplication with a manual-sync requirement.

The problem is only that nothing enforces the sync, and the PR body describes it as having been eliminated rather than documented. If the consolidation is finished, this constant and core's literal both collapse into it. If it stays as-is, the doc comment here is the right shape and the body is what needs amending — but then a test asserting the two messages agree would be worth the few lines, since that is the invariant the comment is asking a future reader to maintain by hand.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceded — same root cause as the ledger_id.rs note, and this is the mirror the body claimed had been removed.

Your read of the doc comment is the part I want to keep. It is honest: it says the constant is mirrored and must stay in step by hand, which is an accurate description of what I actually did. The body then described that same thing as eliminated. So the code was telling the truth and the prose was not, which is the wrong way round for the two to disagree.

Finishing the consolidation rather than keeping this and amending the body. The constant moves to fluree-db-core — the address grammar has to apply the floor with no ledger in hand, so it cannot reach into fluree-db-api — and is re-exported from ledger_view, which keeps fluree_db_api::COMMIT_PREFIX_MIN_LEN resolving and leaves the static assertion in commands/log.rs untouched. This mirror and core’s literal both collapse into it, and the message strings quote it too, since a message with 6 written into it drifts independently of the check.

On your alternative: you are right that if it stayed as-is, a test asserting the two messages agree would be the thing worth writing, precisely because the doc comment is asking a future reader to maintain that by hand. That is close to the test I had written and not delivered, and I think it is the better version of it — it pins the invariant the comment names rather than pinning my constant. The one I am shipping asserts the messages quote the constant, which gets both.

One more thing worth flagging since you were looking here anyway: the two tests at dataset.rs:1724-1727 you noted assert .contains("at least 6 characters") as a literal. Those are mine and you are right that they pin the text rather than the constant. They now derive the expected string from COMMIT_PREFIX_MIN_LEN, so a bump moves the assertions with the code.

Ok(CommitRef::Prefix(prefix.to_string()))
} else if s.is_empty() {
Err(ApiError::query("empty commit reference"))
} else if let Ok(t) = s.parse::<i64>() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, no action needed unless you want the coverage: this arm also makes a bare negative integer a t. CommitRef::parse("-5") now returns CommitRef::T(-5) where it previously fell through to CommitRef::Prefix("-5").

Harmless in practice — a negative t fails downstream either way, and -5 was never a valid hex prefix — so this is strictly a better error path. But it is a behaviour change in the same class as the digits ambiguity you documented, and unlike that one it has no test. parse_resolves_the_digits_ambiguity_toward_t would be the natural home for a one-line case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking the coverage — you marked it no-action and I think it earns the line anyway, for the reason you gave: it is the same class as the digits ambiguity, and that one I argued in the body precisely so it would not be discovered later. Leaving its sibling untested is inconsistent with that.

Confirmed the mechanism rather than assuming it: "-5".parse::<i64>() is Ok(-5), so the arm returns T(-5) where it previously fell through to Prefix("-5"). Agreed it is strictly a better error path — a negative t fails cleanly downstream, and -5 could never have been a hex prefix, so nothing that used to resolve stops resolving.

Worth noting the reachability, since it is narrower than it looks: on the CLI clap consumes -5 as a flag before this ever sees it, so the arm is reachable through the library and routes/ledger.rs rather than through branch revert -5. That is an argument for the test rather than against it — a path no CLI invocation can exercise is exactly the one that will not get caught by the integration suite.

Going into parse_resolves_the_digits_ambiguity_toward_t as you suggest, since the two belong together: that test already pins which way an ambiguous all-digit string resolves, and the negative case is the same question with a sign on it.

`CommitRef::parse("-5")` returns `T(-5)` where it used to fall through to
`Prefix("-5")`. Strictly a better error path — a negative `t` fails cleanly
downstream and `-5` was never a valid hex prefix — but it is a behaviour
change in the same class as the all-digit ambiguity, which is argued in the
PR body precisely so it would not be discovered later.

It needs the test more than that one does, not less: on the CLI clap consumes
`-5` as a flag before the parser sees it, so the arm is reachable only through
the library and the server's commit routes, which the integration suite never
walks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:cli fluree CLI UX, rdf toolkit, publish/export/insert flows area:server HTTP surface, routes, error mapping, swagger, timeouts/admission, config graph bug Something isn't working as expected

Projects

None yet

2 participants