fix: preserve raw text for t-typed CLI args#242
Merged
Conversation
CLI args declared as `t` used to be silently coerced to Number when the
raw input parsed as a finite float. `f arg:t` with input `2` arrived
at runtime as `Number(2.0)`, so `num arg` returned nil (it expects
text), which then collapsed the surrounding `?r{~i:..;^e:..}` match
into a silent nil return because there is no nil arm. The static types
were fine, so the verifier never saw it.
Add `parse_cli_args_typed` which consults the declared param types
during parsing: `t` params keep the raw CLI string as Text (with the
existing quoted-string strip preserved for back-compat); `L _` params
still wrap a scalar as a one-element list; everything else parses by
the same rules as before. The six production callsites that used to
do parse-then-coerce-by-type switch to the new typed parser.
The legacy `coerce_cli_args` is kept under #[cfg(test)] so its
list-wrap test cases keep pinning that behaviour starting from
pre-parsed Values, but it has no remaining production callers.
Adds unit tests covering text-preservation across numeric, bool, nil,
and list-shaped inputs, plus regression coverage for non-text params
(number passthrough, list wrap, mixed signatures, no-func-name
fallback).
Exercises all four engines (default JIT, --run-tree, --run-vm, --run-cranelift) for the t-param coercion bug. Pre-fix all four returned nil for `f arg:t` with input `2`; post-fix all four return the parsed number. Covers the num round-trip (digit and non-digit input), identity on bool-shaped and nil-shaped inputs, and a sanity check that n-typed params still parse as numbers.
Adds a worked example showing the now-correct behaviour: digit-shaped, bool-shaped, and nil-shaped CLI inputs all arrive at t-typed params as literal Text, so `num arg` unwraps cleanly and identity returns the verbatim string. Picked up by tests/examples_engines.rs so it acts as a third regression layer across every engine.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CLI args declared as
tused to be silently coerced toNumberwhen the raw input parsed as a finite float.f arg:twith input2arrived at runtime asNumber(2.0), sonum argreturned nil, which then collapsed the surrounding?r{~i:..;^e:..}match into a silent nil return because there is no nil arm. Static types were fine, so the verifier never caught it.This was the longstanding 🔴 friction documented in
ilo_assessment_feedback.mdsince v0.10, and the same friction surfaced again in the v0.11.1 interactive-CLI rerun. Highest-leverage manifesto win here: zero retries for any agent that declaresarg:tand passes digit-shaped input. Pre-fix the agent had to invent a workaround (a separatei:nparam threaded through the dispatcher purely to keep the type honest). Post-fix the declared type does what it says.Repro
Before:
After:
Same result on
--run-tree,--run-vm,--run-cranelift.What's in the diff
fix: preserve raw text for t-typed CLI args— addsparse_cli_args_typedwhich consults declared param types during CLI parsing.tparams keep the raw string verbatim asText(the existing quoted-string strip is preserved for back-compat);L _params still wrap a scalar; everything else parses by the same rules as before. Switches the six production callsites that used to do parse-then-coerce-by-type to the new typed parser. The legacycoerce_cli_argsis kept under#[cfg(test)]so its list-wrap tests keep pinning that behaviour from pre-parsed values, but has no remaining production callers. Unit tests cover text-preservation across numeric, bool, nil, and list-shaped inputs, plus number passthrough, list wrap, mixed signatures, and the no-func-name fallback.test: cross-engine regression for t-typed CLI args—tests/regression_cli_text_arg.rsexercises all four engines for the num round-trip (both digit and non-digit input), identity on bool-shaped and nil-shaped inputs, and a sanity check thatn-typed params still parse as numbers.examples: t-typed CLI args preserve raw input—examples/cli-text-arg.ilodocuments the now-correct behaviour and is picked up bytests/examples_engines.rsfor a third regression layer across every engine.Test plan
cargo test --release --features cranelift— full suite green (2866 lib tests + integration crates)cargo clippy --release --features cranelift --all-targets -- -W clippy::all— cleancargo fmt --check— clean--run-tree,--run-vm,--run-cranelift) — all return2forf arg:twith input2Follow-ups
None. The fix is surgical: type-aware parsing for
tonly, every other declared type's behaviour is byte-for-byte unchanged.