Skip to content

Commit 81e41a5

Browse files
chaliyclaude
andauthored
fix(jq): fix argument parsing, add test coverage, update docs (#221)
## Summary - Fix jq argument parsing: combined short flags (`-rn`, `-sc`, `-snr`), `--arg name value`, `--argjson name value`, `--indent N`, and `--` separator now work correctly - Add comprehensive positive and negative test coverage (10 new unit tests, 8 new spec tests, 2 unskipped spec tests) - Update specs (009-implementation-status, 005-builtins) and compatibility docs with full jq flag documentation ## Test plan - [x] `cargo fmt --check` passes - [x] `cargo clippy --all-targets --all-features -- -D warnings` passes - [x] `cargo test --all-features` passes (all 37 jq unit tests, 100/108 spec tests pass, 8 skipped) - [x] Negative tests cover: invalid JSON input, invalid filter syntax, invalid --argjson value, empty/whitespace input - [x] Positive tests cover: NDJSON, multiple --arg bindings, --argjson with objects, combined flags, exit status for null/false/truthy --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f4f9d31 commit 81e41a5

File tree

5 files changed

+390
-46
lines changed

5 files changed

+390
-46
lines changed

crates/bashkit/docs/compatibility.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ for sandbox security reasons. See the compliance spec for details.
7272
| `grep` | `-i`, `-v`, `-c`, `-n`, `-E`, `-q` | Pattern matching |
7373
| `sed` | `s///[g]`, `d`, `p`, `q`, `a`, `i`, `c`, `h/H/g/G/x`, `-E`, `-n`, `!` | Stream editing |
7474
| `awk` | `'{print}'`, `-F`, `-v`, loops, arrays, increment, ternary | Text processing |
75-
| `jq` | `.field`, `.[n]`, pipes, file args | JSON processing |
75+
| `jq` | `.field`, `.[n]`, pipes, file args, `-r`, `-c`, `-n`, `-s`, `-S`, `-e`, `-j`, `--tab`, `--arg`, `--argjson`, `-V`, combined flags | JSON processing |
7676
| `sleep` | `N`, `N.N` | Pause execution (max 60s) |
7777
| `head` | `-n N`, `-N` | First N lines (default 10) |
7878
| `tail` | `-n N`, `-N` | Last N lines (default 10) |

crates/bashkit/src/builtins/jq.rs

Lines changed: 284 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@ impl Builtin for Jq {
129129
}
130130
}
131131

132-
// Parse arguments for flags
132+
// Parse arguments for flags using index-based loop to support
133+
// multi-arg flags like --arg name value and --argjson name value.
133134
let mut raw_output = false;
134135
let mut compact_output = false;
135136
let mut null_input = false;
@@ -140,32 +141,103 @@ impl Builtin for Jq {
140141
let mut join_output = false;
141142
let mut filter = ".";
142143
let mut file_args: Vec<&str> = Vec::new();
144+
// Store variable bindings as (name, serde_json::Value) to avoid
145+
// holding non-Send jaq Val across await points.
146+
let mut var_bindings: Vec<(String, serde_json::Value)> = Vec::new();
143147

144148
let mut found_filter = false;
145-
for arg in ctx.args {
149+
let mut i = 0;
150+
while i < ctx.args.len() {
151+
let arg = &ctx.args[i];
146152
if found_filter {
147153
// Everything after the filter is a file argument
148154
file_args.push(arg);
149-
} else if arg == "-r" || arg == "--raw-output" {
155+
i += 1;
156+
continue;
157+
}
158+
159+
if arg == "--" {
160+
// End of options: next arg is filter, rest are files
161+
i += 1;
162+
if i < ctx.args.len() {
163+
filter = &ctx.args[i];
164+
found_filter = true;
165+
}
166+
i += 1;
167+
continue;
168+
}
169+
170+
if arg == "--raw-output" {
150171
raw_output = true;
151-
} else if arg == "-c" || arg == "--compact-output" {
172+
} else if arg == "--compact-output" {
152173
compact_output = true;
153-
} else if arg == "-n" || arg == "--null-input" {
174+
} else if arg == "--null-input" {
154175
null_input = true;
155-
} else if arg == "-S" || arg == "--sort-keys" {
176+
} else if arg == "--sort-keys" {
156177
sort_keys = true;
157-
} else if arg == "-s" || arg == "--slurp" {
178+
} else if arg == "--slurp" {
158179
slurp = true;
159-
} else if arg == "-e" || arg == "--exit-status" {
180+
} else if arg == "--exit-status" {
160181
exit_status = true;
161182
} else if arg == "--tab" {
162183
tab_indent = true;
163-
} else if arg == "-j" || arg == "--join-output" {
184+
} else if arg == "--join-output" {
164185
join_output = true;
165-
} else if !arg.starts_with('-') {
186+
} else if arg == "--arg" {
187+
// --arg name value: bind $name to string value
188+
if i + 2 < ctx.args.len() {
189+
let name = format!("${}", &ctx.args[i + 1]);
190+
let value = serde_json::Value::String(ctx.args[i + 2].to_string());
191+
var_bindings.push((name, value));
192+
i += 3;
193+
continue;
194+
}
195+
i += 1;
196+
continue;
197+
} else if arg == "--argjson" {
198+
// --argjson name value: bind $name to parsed JSON value
199+
if i + 2 < ctx.args.len() {
200+
let name = format!("${}", &ctx.args[i + 1]);
201+
let json_val: serde_json::Value = serde_json::from_str(&ctx.args[i + 2])
202+
.map_err(|e| {
203+
Error::Execution(format!("jq: invalid JSON for --argjson: {}", e))
204+
})?;
205+
var_bindings.push((name, json_val));
206+
i += 3;
207+
continue;
208+
}
209+
i += 1;
210+
continue;
211+
} else if arg == "--indent" {
212+
// --indent N: skip the numeric argument (use default formatting)
213+
i += 2;
214+
continue;
215+
} else if arg == "--args" || arg == "--jsonargs" {
216+
// Remaining args are positional, not files; skip for now
217+
i += 1;
218+
continue;
219+
} else if arg.starts_with("--") {
220+
// Unknown long flag: skip
221+
} else if arg.starts_with('-') && arg.len() > 1 {
222+
// Short flag(s): may be combined like -rn, -sc, -snr
223+
for ch in arg[1..].chars() {
224+
match ch {
225+
'r' => raw_output = true,
226+
'c' => compact_output = true,
227+
'n' => null_input = true,
228+
'S' => sort_keys = true,
229+
's' => slurp = true,
230+
'e' => exit_status = true,
231+
'j' => join_output = true,
232+
_ => {} // ignore unknown short flags
233+
}
234+
}
235+
} else {
236+
// Non-flag argument: this is the filter
166237
filter = arg;
167238
found_filter = true;
168239
}
240+
i += 1;
169241
}
170242

171243
// Build input: read from file arguments if provided, otherwise stdin
@@ -221,19 +293,23 @@ impl Builtin for Jq {
221293
))
222294
})?;
223295

224-
// Compile the filter
225-
let filter = Compiler::default()
226-
.with_funs(jaq_std::funs().chain(jaq_json::funs()))
227-
.compile(modules)
228-
.map_err(|errs| {
229-
Error::Execution(format!(
230-
"jq: compile error: {}",
231-
errs.into_iter()
232-
.map(|e| format!("{:?}", e))
233-
.collect::<Vec<_>>()
234-
.join(", ")
235-
))
236-
})?;
296+
// Compile the filter, registering any --arg/--argjson variable names
297+
let var_names: Vec<&str> = var_bindings.iter().map(|(n, _)| n.as_str()).collect();
298+
let compiler = Compiler::default().with_funs(jaq_std::funs().chain(jaq_json::funs()));
299+
let compiler = if var_names.is_empty() {
300+
compiler
301+
} else {
302+
compiler.with_global_vars(var_names.iter().copied())
303+
};
304+
let filter = compiler.compile(modules).map_err(|errs| {
305+
Error::Execution(format!(
306+
"jq: compile error: {}",
307+
errs.into_iter()
308+
.map(|e| format!("{:?}", e))
309+
.collect::<Vec<_>>()
310+
.join(", ")
311+
))
312+
})?;
237313

238314
// Process input as JSON
239315
let mut output = String::new();
@@ -260,8 +336,12 @@ impl Builtin for Jq {
260336
// Create empty inputs iterator
261337
let inputs = RcIter::new(core::iter::empty());
262338

263-
// Run the filter
264-
let ctx = Ctx::new([], &inputs);
339+
// Run the filter, passing any --arg/--argjson variable values
340+
let var_vals: Vec<Val> = var_bindings
341+
.iter()
342+
.map(|(_, v)| Val::from(v.clone()))
343+
.collect();
344+
let ctx = Ctx::new(var_vals, &inputs);
265345
for result in filter.run((ctx, jaq_input)) {
266346
match result {
267347
Ok(val) => {
@@ -706,4 +786,183 @@ mod tests {
706786
assert_eq!(arr[0]["id"], 1);
707787
assert_eq!(arr[1]["id"], 2);
708788
}
789+
790+
// --- Argument parsing bug regression tests ---
791+
792+
#[tokio::test]
793+
async fn test_jq_combined_short_flags() {
794+
// -rn should be parsed as -r + -n
795+
let result = run_jq_with_args(&["-rn", "1+1"], "").await.unwrap();
796+
assert_eq!(result.trim(), "2");
797+
}
798+
799+
#[tokio::test]
800+
async fn test_jq_combined_flags_sc() {
801+
// -sc should be parsed as -s + -c
802+
let result = run_jq_with_args(&["-sc", "add"], "1\n2\n3\n")
803+
.await
804+
.unwrap();
805+
assert_eq!(result.trim(), "6");
806+
}
807+
808+
#[tokio::test]
809+
async fn test_jq_arg_flag() {
810+
// --arg name value: $name should resolve to "value" in the filter
811+
let result = run_jq_with_args(&["--arg", "name", "world", "-n", r#""hello \($name)""#], "")
812+
.await
813+
.unwrap();
814+
assert_eq!(result.trim(), r#""hello world""#);
815+
}
816+
817+
#[tokio::test]
818+
async fn test_jq_argjson_flag() {
819+
// --argjson count 5: $count should resolve to 5 (number, not string)
820+
let result = run_jq_with_args(&["--argjson", "count", "5", "-n", "$count + 1"], "")
821+
.await
822+
.unwrap();
823+
assert_eq!(result.trim(), "6");
824+
}
825+
826+
#[tokio::test]
827+
async fn test_jq_arg_does_not_eat_filter() {
828+
// --arg name value '.' should NOT treat '.' as a file
829+
let result = run_jq_with_args(&["--arg", "x", "hello", "."], r#"{"a":1}"#)
830+
.await
831+
.unwrap();
832+
assert!(result.contains("\"a\": 1"));
833+
}
834+
835+
#[tokio::test]
836+
async fn test_jq_double_dash_separator() {
837+
// -- ends option parsing; next arg is filter
838+
let result = run_jq_with_args(&["-n", "--", "1+1"], "").await.unwrap();
839+
assert_eq!(result.trim(), "2");
840+
}
841+
842+
#[tokio::test]
843+
async fn test_jq_indent_flag() {
844+
// --indent 4 should not eat the filter
845+
let result = run_jq_with_args(&["--indent", "4", "."], r#"{"a":1}"#)
846+
.await
847+
.unwrap();
848+
assert!(result.contains("\"a\""));
849+
}
850+
851+
// --- Negative tests ---
852+
853+
#[tokio::test]
854+
async fn test_jq_invalid_json_input() {
855+
let result = run_jq(".", "not valid json");
856+
assert!(result.await.is_err(), "invalid JSON input should error");
857+
}
858+
859+
#[tokio::test]
860+
async fn test_jq_invalid_filter_syntax() {
861+
let result = run_jq(".[", r#"{"a":1}"#);
862+
assert!(result.await.is_err(), "invalid filter should error");
863+
}
864+
865+
#[tokio::test]
866+
async fn test_jq_invalid_argjson_value() {
867+
let result = run_jq_with_args(&["--argjson", "x", "not json", "-n", "$x"], "");
868+
assert!(
869+
result.await.is_err(),
870+
"invalid JSON for --argjson should error"
871+
);
872+
}
873+
874+
#[tokio::test]
875+
async fn test_jq_empty_input_no_null() {
876+
// Empty stdin without -n should produce empty output, not error
877+
let result = run_jq(".", "").await.unwrap();
878+
assert_eq!(result, "");
879+
}
880+
881+
#[tokio::test]
882+
async fn test_jq_whitespace_only_input() {
883+
let result = run_jq(".", " \n\t ").await.unwrap();
884+
assert_eq!(result, "");
885+
}
886+
887+
#[tokio::test]
888+
async fn test_jq_ndjson_multiple_values() {
889+
// Multiple JSON values on separate lines (NDJSON)
890+
let result = run_jq(".a", "{\"a\":1}\n{\"a\":2}\n").await.unwrap();
891+
assert_eq!(result.trim(), "1\n2");
892+
}
893+
894+
#[tokio::test]
895+
async fn test_jq_exit_status_false() {
896+
// -e flag: false output -> exit code 1
897+
let jq = Jq;
898+
let fs = Arc::new(InMemoryFs::new());
899+
let mut vars = HashMap::new();
900+
let mut cwd = PathBuf::from("/");
901+
let args = vec!["-e".to_string(), ".".to_string()];
902+
let ctx = Context {
903+
args: &args,
904+
env: &HashMap::new(),
905+
variables: &mut vars,
906+
cwd: &mut cwd,
907+
fs,
908+
stdin: Some("false"),
909+
#[cfg(feature = "http_client")]
910+
http_client: None,
911+
#[cfg(feature = "git")]
912+
git_client: None,
913+
};
914+
let result = jq.execute(ctx).await.unwrap();
915+
assert_eq!(result.exit_code, 1, "-e with false should exit 1");
916+
}
917+
918+
#[tokio::test]
919+
async fn test_jq_exit_status_truthy() {
920+
// -e flag: truthy output -> exit code 0
921+
let jq = Jq;
922+
let fs = Arc::new(InMemoryFs::new());
923+
let mut vars = HashMap::new();
924+
let mut cwd = PathBuf::from("/");
925+
let args = vec!["-e".to_string(), ".".to_string()];
926+
let ctx = Context {
927+
args: &args,
928+
env: &HashMap::new(),
929+
variables: &mut vars,
930+
cwd: &mut cwd,
931+
fs,
932+
stdin: Some("42"),
933+
#[cfg(feature = "http_client")]
934+
http_client: None,
935+
#[cfg(feature = "git")]
936+
git_client: None,
937+
};
938+
let result = jq.execute(ctx).await.unwrap();
939+
assert_eq!(result.exit_code, 0, "-e with truthy value should exit 0");
940+
}
941+
942+
#[tokio::test]
943+
async fn test_jq_multiple_arg_bindings() {
944+
let result = run_jq_with_args(
945+
&[
946+
"--arg",
947+
"x",
948+
"hello",
949+
"--arg",
950+
"y",
951+
"world",
952+
"-n",
953+
r#""[\($x)] [\($y)]""#,
954+
],
955+
"",
956+
)
957+
.await
958+
.unwrap();
959+
assert_eq!(result.trim(), r#""[hello] [world]""#);
960+
}
961+
962+
#[tokio::test]
963+
async fn test_jq_combined_flags_snr() {
964+
// -snr: slurp + null-input + raw-output
965+
let result = run_jq_with_args(&["-snr", r#""hello""#], "").await.unwrap();
966+
assert_eq!(result.trim(), "hello");
967+
}
709968
}

0 commit comments

Comments
 (0)