feat(parse): add usage__varargs_idx env var#573
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #573 +/- ##
==========================================
+ Coverage 79.04% 79.15% +0.11%
==========================================
Files 48 48
Lines 7235 7437 +202
Branches 7235 7437 +202
==========================================
+ Hits 5719 5887 +168
- Misses 1141 1153 +12
- Partials 375 397 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR adds a Key implementation decisions:
Both prior review threads (bare Confidence Score: 5/5Safe to merge — both prior P1 concerns are resolved, no new logic or security issues found. Both review-thread concerns (bare usize subtraction → saturating_sub; emitting on parse errors → None guard) are properly addressed. The index arithmetic in exec.rs and shell.rs is correct relative to what the subprocess sees as $@. Six unit tests cover the meaningful edge cases. The only remaining finding is a P2 documentation wording suggestion. No files require special attention; docs/cli/scripts.md has one minor wording/quoting suggestion. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User Shell
participant CLI as usage CLI (exec/shell)
participant Lib as usage-lib parse()
participant Script as Subprocess Script
User->>CLI: usage bash script.sh --verbose file.txt extra1 extra2
CLI->>Lib: parse(spec, ["bin", "--verbose", "file.txt", "extra1", "extra2"])
Lib-->>CLI: ParseOutput { args: {file→"file.txt", extra→["extra1","extra2"]}, errors: [] }
CLI->>CLI: trailing_varargs_count() → Some(2)
CLI->>CLI: usage__varargs_idx = self.args.len() - 2 = 2
CLI->>Script: spawn(env: {usage_file="file.txt", usage_extra="extra1 extra2", usage__varargs_idx="2"})
Script->>Script: shift "$usage__varargs_idx" # shift 2
Script->>Script: $@ = ["extra1", "extra2"]
Reviews (7): Last reviewed commit: "feat(parse): add usage__varargs_idx env ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request introduces a new environment variable, usage__varargs_idx, which identifies the starting index of trailing variadic arguments within the original CLI input. This is implemented by tracking the total input argument count in the ParseOutput struct and calculating the offset during environment variable generation. While the logic is supported by new unit tests, feedback indicates a high-risk issue where the calculation could cause a subtraction overflow panic if variadic arguments are populated by default values rather than CLI input. Additionally, it is recommended to add documentation comments to the new public struct field for better maintainability.
3185992 to
c976318
Compare
…args Add `trailing_varargs_count()` to `ParseOutput`, returning the number of values captured by trailing contiguous vararg groups (or `None` when the spec has no trailing varargs). The CLI shell/exec runners use this to set `usage__varargs_idx` on the subprocess, equal to `args.len() - trailing_varargs_count`. Scripts can `shift $usage__varargs_idx` to remove parsed args and keep only the trailing varargs in `$@`. https://claude.ai/code/session_01CwiKAozb4jeH6soFJrNxPk
c976318 to
a72e971
Compare
|
I'd like to hear your thoughts here, @jdx |
Add special env var for usage scripts containing number of parsed args before trailing varargs.
Intended use -- pass on varargs without needing to use
eval. Despite being safe, it raises eyebrows and this change is IMO simple and unobtrusive. It can exist alongside, being just another option.Example of a script that has args and flags followed by varargs:
It's not the most elegant solution in the world but it might be less inelegant than
eval.Code is by Claude with significant handholding by me.