fix: date implements %F and -I instead of emitting them literally - #63
Conversation
63c49e1 to
57eaffd
Compare
ivarvong
left a comment
There was a problem hiding this comment.
Blocking: date +%P raises out of JustBash.exec/2
lib/just_bash/commands/date.ex:223:
defp directive(?P, dt), do: dt |> directive(?p) |> String.downcase()The pipe makes dt the first argument, so this calls directive(dt, ?p). That lands in the catch-all directive(other, _dt), which tries to build <<?%, other>> with a DateTime struct:
$ date +%P
** (ArgumentError) construction of binary failed: segment 2 of type 'integer':
expected an integer but got: ~U[2026-08-03 20:21:24Z]
The error escapes JustBash.exec/2 and crashes the caller — the same failure shape #57 just eliminated for cp, and arguably worse than the %F bug this PR fixes, since any format containing %P kills the host instead of returning a wrong answer. The suite misses it because the 12-hour test exercises %I %p but never %P on its own.
Fix is one line:
defp directive(?P, dt), do: directive(?p, dt) |> String.downcase()— with the failing test first, per CLAUDE.md: date -d '2024-06-15 13:30:00' '+%P' → "pm\n" (and midnight → "am\n" would cover both branches).
Everything else verified good. I ran all five gates in a fresh clone (all green, matching CI) and compared byte-for-byte against GNU date (TZ=UTC0 gdate): %F %T %R %D, %y %C, space-padded %e, %I %p %Z, %j %u %w %h %N, the escaping cases (%%F, trailing %, unknown %J, raw non-UTF-8 bytes), and every -I form including the , in -Ins and multiple output formats specified in both argument orders — all identical to GNU except %P. Nice catch on repeated -I being last-wins rather than an error; I confirmed GNU agrees (gdate -I -Iseconds → exit 0, seconds form), and only erroring against an explicit +format is the right discrimination.
Two non-blocking notes:
date -I seconds(spec as a separate operand) ignoressecondsand prints the ISO date; GNU errorsinvalid date 'seconds'. Pre-existing catch-all-operand behavior, fine to leave.- The PR body's environment notes don't match the supported toolchain: 3,989 tests (this branch runs 3,771), 14 pre-existing compile warnings, and a repo-wide credo crash on 1.20 — none reproduce on 1.19/OTP 28, where main and this branch are both fully clean. Worth reconciling so the body doesn't understate how clean the repo is.
`date +%F` printed the literal string "%F" and exited 0. An agent driving a sandbox asked it what today's date was, got "%F" back, and carried on — a silently wrong answer, which is the worst thing a sandbox command can return. `date -I` had the matching bug one level up: the flag parsed into the catch-all clause and was discarded, so it printed the full default format as though it had never been passed. Rewrite `format_datetime/2` as a single left-to-right scan. The chain of `String.replace/3` it replaces could not express escaping: it rewrote `%Y` everywhere before it ever considered `%%`, so `%%Y` — a literal percent followed by Y — came out as `%2024`. No ordering fixes that, because a later pass cannot tell which percents an earlier pass already consumed. Scanning consumes each directive exactly once. Adds the directives a caller actually reaches for (%F %T %R %D %y %C %e %I %p %P %Z %h) and passes unknown ones through verbatim as GNU date does, so "unsupported" stays distinguishable from a real value. Adds `-I[FMT]` / `--iso-8601[=FMT]`, which errors on a competing `+format` rather than silently dropping one — measured against real date.
Address review feedback: - Scan the format string byte-wise. Matching with ::utf8 crashed with a FunctionClauseError on raw non-UTF-8 bytes (date +$'\xff%Y'), which the shell can produce via printf %b, echo -e, or $'...'. Every known directive is ASCII, and passthrough reconstructs other bytes verbatim. - Implement %N (nanoseconds) and use it for -Ins instead of a hardcoded ,000000000, so fractional seconds render for real. - Reject 'date,ns' as an -I argument, matching GNU date. - Drop the dead || "UTC" fallback; DateTime.zone_abbr is always set. - Trim reviewer-directed narrative comments down to the constraints. - Add tests: the non-UTF-8 byte regression, %N, -Ins, --iso-8601=seconds, '+%Y' -I conflict order, and the -Ibogus / date,ns error paths.
`directive(?P, dt)` piped `dt` into `directive/2`, so it called `directive(dt, ?p)`. That fell to the catch-all clause, which tried to build `<<?%, other>>` from a DateTime and raised an ArgumentError out of `JustBash.exec/2` — any format containing `%P` killed the host rather than returning a wrong answer. Two properties over the directive alphabet pin the class of bug rather than the instance: every single-byte directive must format without raising, and a sequence of directives must equal the concatenation of each alone. Both fail on byte 80 before the fix. Verified against GNU date: `%P` is `pm`/`am`.
e9081e7 to
8a28d18
Compare
|
Fixed in 8a28d18 — thanks, that was a real crash and the diagnosis was exactly right. Per CLAUDE.md the failing test came first, and I wrote it as two properties over the directive alphabet rather than a
Both shrink to byte 80 against the previous head. The one-line fix is yours; I also added the two On your second note: you were right, and the numbers in the body were wrong. On 1.19/OTP 28 the branch is
|
Fixes #62.
date +%Fprinted the literal string%Fand exited 0. That is the shape of bug worth prioritising: a consumer cannot tell it apart from a real answer. I found it because an LLM agent driving a JustBash sandbox asked what today's date was, got%Fback, and used it.What changed
format_datetime/2is now a single left-to-right scan instead of a chain ofString.replace/3.The chain could not express escaping. It rewrote
%Yeverywhere before it ever considered%%, so%%Y— a literal percent followed by Y — came out as%2024. Reordering does not fix this: any fixed order has the same flaw for some input, because a later pass cannot tell which percent signs an earlier pass already consumed. Scanning consumes each directive exactly once, so%%is just another two-character directive and the ambiguity disappears.Added the directives that were missing:
%F %T %R %D(composed from their single-field parts so the two can't drift), plus%y %C %e %I %p %P %Z %h.Unknown directives now pass through verbatim (
%J→%J), matching GNU date. This is the point of the whole change: "unsupported" stays distinguishable from a value, rather than looking like output.Implemented
-I[FMT]/--iso-8601[=FMT], which previously fell through the catch-all arg clause and was silently discarded. Supportsdate,hours,minutes,seconds,ns, and errors with GNU'smultiple output formats specifiedwhen it competes with an explicit+formatrather than quietly picking one.Testing
Tests in
test/commands/utilities_test.exscover the compound directives, the single-field ones, percent escaping (%%F,%%Y, trailing bare%, unknown directive), and every-Iform. Expected values were measured against realdaterather than derived from the spec — which caught one of my own assumptions: I had assumed an explicit+formatwould win over-I, and realdateerrors instead.%e's space padding was checked the same way.Two properties in
test/property_test.exscover the directive alphabet rather than the directives I happened to think of:exec/2Both fail on byte 80 (
%P) against the first version of this branch — see below.All five gates pass on Elixir 1.19 / OTP 28:
mix test(4,010 tests, 58 properties, 0 failures),mix format --check-formatted,mix compile --warnings-as-errors,mix dialyzer(13 skips, all pre-existing), andmix credo --strict(clean apart from the intentionaltest/support/banned_fixture_apply.exfixture finding).An earlier revision of this body reported 3,989 tests, 14 pre-existing compile warnings, and a repo-wide credo crash. None of that reproduces on the supported toolchain — it came from a different local setup and understated how clean the repo is. Corrected above.
Fixed in review
date +%PraisedArgumentErrorout ofJustBash.exec/2and crashed the caller:directive(?P, dt)pipeddtin as the first argument, so it calleddirective(dt, ?p)and fell to the catch-all clause, which tried to build<<?%, other>>from a DateTime. Thanks @ivarvong for catching it. Fixed, with the two properties above written first — they reproduce it without naming%P, so the whole class stays covered.Not addressed
datestill reports UTC regardless ofTZ. Out of scope here, but worth knowing if you're weighing how far to take this: a sandbox consumer that needs a user's local day still can't get it fromdate. Happy to follow up separately if you want it.date -I seconds(spec as a separate operand) prints the ISO date and ignoresseconds, where GNU errorsinvalid date 'seconds'. Pre-existing catch-all-operand behavior, left alone.