# Deep Review: 20260406-103045-pr-2640 | | | |---|---| | **Date** | 2026-04-06 10:30 | | **Repo** | [mikefarah/yq](https://github.com/mikefarah/yq) | | **Round** | 1 | | **Mode** | Thorough (4 passes) | | **Author** | @copilot-swe-agent | | **PR** | [#2640](https://github.com/mikefarah/yq/pull/2640) — Add system(command; args) operator (disabled by default) | | **Branch** | `copilot/add-system-command-operator` | | **Commits** | `6f94991c` Apply suggestions from code review
`b3b44788` Remove deprecated --enable-system-operator alias
`e10e8127` Apply suggestions from code review
`53abbbae` Validate command node type and handle multiple results
`5ea069a5` Update system-operators.md
`2c8605f6` Update headers/system-operators.md
`884c2d8b` Add yqFlags to expressionScenario; fix system op docs
`da611f7a` Evaluate system command/args per matched node
`6315cfe9` Update operator_system.go
`9a72c0f7` Add system(command; args) operator
`d181cf87` Initial plan | | **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro | | **Verdict** | **Merge with fixes** — address I1 (silent null on disabled) and I4 (misleading docs) before merge; remaining items can follow | | **Wall-clock time** | `96 min 32 s` | --- ## Executive Summary This PR adds a `system` operator that executes external commands from yq expressions, gated behind `--security-enable-system-operator`. The implementation follows established yq patterns: `simpleOp` registration, `blockOpType` for optional arguments, `SingleReadonlyChildContext` for per-node evaluation, and `exec.Command` (no shell) to avoid injection. The main concerns are the silent-null behaviour when disabled (inconsistent with how env/file operators fail hard) and misleading documentation about semicolon-separated arguments. --- ## Critical Issues None. --- ## Important Issues I1. **Disabled mode silently injects nulls instead of erroring** — `pkg/yqlib/operator_system.go:63-71` (important, regression) [Claude Opus 4.6, Gemini 3.1 Pro] ```go if !ConfiguredSecurityPreferences.EnableSystemOps { log.Warning("system operator is disabled, use --security-enable-system-operator flag to enable") results := list.New() for el := context.MatchingNodes.Front(); el != nil; el = el.Next() { candidate := el.Value.(*CandidateNode) results.PushBack(candidate.CreateReplacement(ScalarNode, "!!null", "null")) } return context.ChildContext(results), nil } ``` When the system operator is disabled (the default), it returns `null` per matched node with only a warning on stderr. The existing env and file operators return **errors** when their security gates are active (`operator_env.go:20-22`, `operator_load.go:66-68`). In a pipeline like `yq '.field = system("cmd")' file.yml | kubectl apply -f -`, a user who forgets the flag gets null values silently embedded in output with exit code 0. The warning goes to stderr, which pipelines routinely discard. Fix: Return an error, matching the codebase convention: ```diff if !ConfiguredSecurityPreferences.EnableSystemOps { - log.Warning("system operator is disabled, use --security-enable-system-operator flag to enable") - results := list.New() - for el := context.MatchingNodes.Front(); el != nil; el = el.Next() { - candidate := el.Value.(*CandidateNode) - results.PushBack(candidate.CreateReplacement(ScalarNode, "!!null", "null")) - } - return context.ChildContext(results), nil + return Context{}, fmt.Errorf("system operations are disabled, use --security-enable-system-operator to enable") } ``` If the null-return behaviour is intentional, document the divergence from the env/file pattern in a code comment. I2. **Multi-result arg expressions silently truncated to first match** — `pkg/yqlib/operator_system.go:101-103` (important, regression) [Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro] ```go if argsNodes.MatchingNodes.Front() != nil { args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode)) } ``` When the args expression produces multiple results (e.g. `system("echo"; .args[])` on `{"args":["a","b"]}`), only the first result is used; the rest are silently dropped. The command side (`resolveCommandNode` at line 50-51) logs a debug message for multiple results, but the args side does not. A user writing `system("cmd"; .items[])` gets only the first item with no indication that the rest were dropped. Fix: Add a debug log matching the command-resolution pattern, or flatten all matching arg nodes into argv: ```diff +if argsNodes.MatchingNodes.Len() > 1 { + log.Debugf("system operator: args expression returned %d results, using first", argsNodes.MatchingNodes.Len()) +} if argsNodes.MatchingNodes.Front() != nil { args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode)) } ``` I3. **Command validation accepts non-string scalars** — `pkg/yqlib/operator_system.go:53-55` (important, regression) [Codex GPT 5.4] ```go cmdNode := commandNodes.MatchingNodes.Front().Value.(*CandidateNode) if cmdNode.Kind != ScalarNode || cmdNode.Tag == "!!null" { return "", fmt.Errorf("system operator: command must be a string scalar") } ``` The check rejects !!null and non-scalars but allows !!int, !!bool, and other scalar tags through. `system(123)` tries to exec "123" rather than failing validation, contradicting both the error text and the documented contract. In practice the OS returns a "not found" error, but the validation should catch this before reaching exec. Fix: Check the tag explicitly: ```diff -if cmdNode.Kind != ScalarNode || cmdNode.Tag == "!!null" { +if cmdNode.Kind != ScalarNode || cmdNode.guessTagFromCustomType() != "!!str" { return "", fmt.Errorf("system operator: command must be a string scalar") } ``` I4. **Misleading documentation about semicolons for multiple args** — `pkg/yqlib/doc/operators/headers/system-operators.md:15` (important, regression) [Gemini 3.1 Pro] ```markdown - An argument or array of arguments separated by `;` (optional) ``` This suggests multiple arguments can be semicolon-separated: `system("cmd"; "arg1"; "arg2")`. In yq, `;` is the block operator, which evaluates expressions sequentially and yields the right-hand result. Writing `system("cmd"; "arg1"; "arg2")` silently drops `"arg1"` and passes only `"arg2"`. Multiple arguments require an array: `system("cmd"; ["arg1", "arg2"])`. Fix: ```diff -- An argument or array of arguments separated by `;` (optional) +- An argument (or an array of arguments), separated from the command by `;` (optional) ``` I5. **Argument resolution silently ignores invalid node types** — `pkg/yqlib/operator_system.go:23-26` (important, regression) [Gemini 3.1 Pro] ```go if child.Kind != ScalarNode || child.Tag == "!!null" { log.Warningf("system operator: argument must be a non-null scalar; got kind=%v tag=%v - ignoring", child.Kind, child.Tag) continue } ``` When a sequence argument contains a non-scalar or null element, `resolveSystemArgs` logs a warning and skips it. Silently dropping command arguments alters execution: `system("rm"; ["-rf", {bad: node}])` resolves to `rm -rf`, losing the invalid node without halting. Fix: Return an error instead of continuing: ```diff if child.Kind != ScalarNode || child.Tag == "!!null" { - log.Warningf("system operator: argument must be a non-null scalar; got kind=%v tag=%v - ignoring", child.Kind, child.Tag) - continue + return nil, fmt.Errorf("system operator: argument must be a non-null scalar; got kind=%v tag=%v", child.Kind, child.Tag) } ``` This requires changing `resolveSystemArgs` to return `([]string, error)` and updating its callers. I6. **system operator subsumes env/file security restrictions — undocumented** — `pkg/yqlib/doc/operators/headers/system-operators.md`, `cmd/root.go:213-215` (important, gap) [Claude Opus 4.6] (pass 2) When `--security-enable-system-operator` is active, `--security-disable-env-ops` and `--security-disable-file-ops` become ineffective: `system("printenv"; "SECRET")` reads environment variables and `system("cat"; "/path/to/file")` reads arbitrary files. The documentation does not mention this. Fix: Add a note to `headers/system-operators.md`: ```diff **Security warning**: The system operator is disabled by default. You must explicitly pass `--security-enable-system-operator` to use it. + +**Note:** When enabled, the system operator can replicate the functionality of `env` and `load` +operators via external commands. Enabling it effectively overrides `--security-disable-env-ops` +and `--security-disable-file-ops`. ``` I7. **Multi-node execution path untested** — `pkg/yqlib/operator_system_test.go` (important, gap) [Claude Opus 4.6] (pass 2) All 7 test scenarios exercise the per-node loop (`operator_system.go:79-145`) with exactly one matched node. Result accumulation across iterations, per-node context isolation, and failure-on-Nth-node behaviour are untested. Fix: Add a multi-document or multi-element test: ```go { description: "System operator processes multiple documents", skipDoc: true, document: "a: first", document2: "a: second", expression: `.a = system("` + echoPath + `"; "replaced")`, expected: []string{ "D0, P[], (!!map)::a: replaced\n", "D1, P[], (!!map)::a: replaced\n", }, }, ``` I8. **`os/exec` import has no build-tag exclusion for tinygo** — `pkg/yqlib/operator_system.go:7` (important, gap) [Claude Opus 4.6] (pass 3) The system operator is the only file in `pkg/yqlib` that imports `os/exec`. Every other heavy dependency (JSON, XML, TOML, Lua, HCL, etc.) has a corresponding `no_*.go` stub and build tag for tinygo builds. The system operator lacks this mechanism. Fix: Add a `yq_nosystem` build tag. Create `no_system.go` with a stub that returns an error, gate `operator_system.go` with `//go:build !yq_nosystem`, and update `scripts/build-tinygo-yq.sh` to include `yq_nosystem`. --- ## Suggestions S1. **No test verifying stdin piping** — `pkg/yqlib/operator_system_test.go` (suggestion, gap) [Claude Opus 4.6] The documentation states "The current matched node's value is serialised and piped to the command via stdin" but no test verifies this. A test using `cat` (which echoes stdin) would confirm the YAML encoding is correctly piped. S2. **No test for `--null-input` usage** — `pkg/yqlib/operator_system_test.go` (suggestion, gap) [Claude Opus 4.6] The documentation header shows `--null-input` as a primary usage pattern, but all test scenarios provide a `document`. A null-input scenario would verify the operator handles a null scalar node on stdin. S3. **Misleading "parse time" comment** — `pkg/yqlib/operator_system.go:74` (suggestion, regression) [Claude Opus 4.6] ```go // determine at parse time whether we have (command; args) or just (command) ``` This runs during expression **evaluation**, not at parse time. Fix: remove "at parse time". S4. **CLI security flag not exercised by tests** — `pkg/yqlib/operator_system_test.go:29-52` (suggestion, gap) [Codex GPT 5.4] Tests toggle `ConfiguredSecurityPreferences.EnableSystemOps` directly instead of testing the Cobra flag wiring at `cmd/root.go:215`. The test suite would still pass if the flag were renamed or unregistered. S5. **Test doc generation uses host-specific absolute paths** — `pkg/yqlib/operator_system_test.go:44-60` (suggestion, regression) [Gemini 3.1 Pro] `findExec` embeds machine-specific paths (e.g. `/usr/bin/echo` vs `/bin/echo`) into generated docs. Running `make generate-docs` on a different host produces a diff. S6. **Command stderr discarded on success** — `pkg/yqlib/operator_system.go:132-138` (suggestion, gap) [Gemini 3.1 Pro] The stderr buffer is checked only when `cmd.Output()` returns an error. Commands that succeed with warnings on stderr have those warnings silently discarded. S7. **Embedded newline in error value** — `pkg/yqlib/operator_system.go:132` (suggestion, regression) [Claude Opus 4.6] (pass 2) ```go return Context{}, fmt.Errorf("system command '%v' failed: %w\nstderr: %v", command, err, stderrStr) ``` Go errors are conventionally single-line. No other `fmt.Errorf` in `pkg/yqlib/` embeds a newline. Fix: use a semicolon or parenthetical. S8. **No subprocess timeout** — `pkg/yqlib/operator_system.go:123-128` (suggestion, enhancement) [Claude Opus 4.6] A hanging command blocks yq indefinitely. Using `exec.CommandContext` with a timeout would prevent this. Low priority — matches standard CLI tool conventions. S9. **Unnecessary double memory allocation for stdin** — `pkg/yqlib/operator_system.go:115-121` (suggestion, enhancement) [Gemini 3.1 Pro] (pass 2) `encodeToYamlString` allocates a string; `stdin.WriteString(encoded)` copies it into a `bytes.Buffer`. Use `strings.NewReader(encoded)` instead to avoid the extra allocation. S10. **Doc generation depends on test execution order** — `pkg/yqlib/operator_system_test.go:40,113` (suggestion, gap) [Claude Opus 4.6] (pass 2) `TestSystemOperatorDisabledScenarios` must run before `TestSystemOperatorEnabledScenarios` because the first creates the doc file (via `os.Create`) and the second only appends. This implicit ordering dependency is fragile but matches existing patterns in `operator_load_test.go`. S11. **Multiple semicolons produce a confusing error** — `pkg/yqlib/operator_system.go:86-88` (suggestion, regression) [Claude Opus 4.6] (pass 3) `system("cmd"; "arg1"; "arg2")` creates nested BLOCK nodes. The operator evaluates the inner BLOCK, gets an empty context, and reports "command expression returned no results" — a confusing error for what is really a syntax mistake. A check for nested blocks could produce a targeted error message. S12. **`flagsPrefix` logic duplicated in `documentInput`** — `pkg/yqlib/operators_test.go:360-374` (suggestion, regression) [Claude Opus 4.6] (pass 3) The same 3-line pattern for building `flagsPrefix` appears in both branches of `documentInput`. Hoist above the if/else. S13. **Flag suffix inconsistent with existing security flags** — `cmd/root.go:215` (suggestion, regression) [Claude Opus 4.6] (pass 4) Existing flags use plural abbreviated suffixes: `--security-disable-env-ops`, `--security-disable-file-ops`. The new flag uses singular spelled-out `--security-enable-system-operator`. The Go field `EnableSystemOps` already follows the `*Ops` convention. Consider renaming to `--security-enable-system-ops` for consistency. S14. **YAML encoding error returned without operator context** — `pkg/yqlib/operator_system.go:116-118` (suggestion, regression) [Claude Opus 4.6] (pass 4) Every other error path in `systemOperator` prefixes with `"system operator:"` or `"system command '...' failed:"`. An encoding error here propagates without context. Fix: wrap with `fmt.Errorf("system operator: failed to encode input: %w", err)`. --- ## Design Observations ### Concerns - **(in-scope)** The inverted security model (enable vs. disable) is sound, but the inverted failure mode (null vs. error) breaks the principle of least surprise. Users who learn that `--security-disable-env-ops` causes errors will expect that the absence of `--security-enable-system-operator` also causes errors. [Claude Opus 4.6] - **(in-scope)** The security model has an implicit hierarchy: `system` >= `env` + `load`. The flags are presented as independent toggles, but enabling system ops effectively grants everything the other two provide. This should be explicit to users. [Claude Opus 4.6] (pass 2) - **(future)** The operator returns `!!str` for all output. A future enhancement could support `system_yaml`/`system_json` variants that parse command output, similar to `load` vs `load_str`. [Claude Opus 4.6] (pass 2) ### Strengths - **(in-scope)** Using `exec.Command` (no shell) avoids shell injection entirely. Arguments pass directly to the exec syscall. [Claude Opus 4.6, Gemini 3.1 Pro] - **(in-scope)** Per-node evaluation via `SingleReadonlyChildContext` correctly scopes expression evaluation, matching the pattern used by `loadStringOperator` and other operators. [Claude Opus 4.6, Codex GPT 5.4] - **(in-scope)** The inverted default (`EnableSystemOps: false`) correctly makes the more dangerous capability opt-in rather than opt-out. [Claude Opus 4.6, Gemini 3.1 Pro] - **(in-scope)** The `#nosec G204` annotation at line 122 with explanation properly acknowledges the gosec warning and documents the mitigation. [Claude Opus 4.6] - **(in-scope)** The `yqFlags` field added to `expressionScenario` is a clean, reusable solution for doc generation — any operator needing CLI flags in examples can use it. [Claude Opus 4.6, Codex GPT 5.4] - **(in-scope)** The `blockOpType` check for optional arguments follows the pattern used by `match`, `datetime`, and `with` operators. [Claude Opus 4.6] (pass 3) - **(in-scope)** `.system` (dot-prefixed) is correctly tokenized as a `PathElement`, not as the SYSTEM operator, so users with YAML keys named "system" are unaffected. [Claude Opus 4.6] (pass 3) --- ## Testing Assessment Test coverage is adequate for the core paths: disabled mode, single-arg, no-arg, array args, per-node expression evaluation, command failure, and null command. The highest-risk untested scenarios: 1. **Multi-node execution** (I7) — the core for-loop is never tested with >1 node 2. **Stdin piping** (S1) — the core feature of passing node YAML to the command's stdin 3. **`--null-input` mode** (S2) — documented primary usage pattern 4. **CLI flag wiring** (S4) — Cobra flag binding is untested 5. **Non-string command scalars** (I3) — `system(123)` reaches `exec.Command` unchecked --- ## Documentation Assessment The header documentation uses correct UK English ("serialised") and clearly warns about the security gate. The generated examples correctly show the flag for enabled scenarios and omit it for disabled. Two gaps: - The semicolon description at `headers/system-operators.md:15` is misleading (I4) - The security subsumption relationship with env/file ops is undocumented (I6) --- ## Commit Structure The history shows iterative Copilot agent refinement: an initial plan commit (`d181cf87`, empty), the main implementation, then multiple fixup commits responding to review feedback. Several commits are patches on earlier ones: - `6315cfe9`, `5ea069a5`, `2c8605f6` — filename-only messages with no indication of what changed - `e10e8127`, `6f94991c` — "Apply suggestions from code review" without specifics - `d181cf87` — empty plan commit This is expected for a Copilot-authored PR iterated via review comments. A squash merge would produce a clean single commit. --- ## Acknowledged Limitations - `#nosec G204` at `operator_system.go:122` — intentional use of variable command paths for `exec.Command`, gated by explicit opt-in via `--security-enable-system-operator`. --- ## Unresolved Feedback All four substantive review comments from @mikefarah have been addressed by subsequent commits: 1. Per-node evaluation with `SingleReadonlyChildContext` — commit `da611f7a` 2. Missing `--security-enable-system-operator` in doc examples — commit `884c2d8b` 3. Validate command node type, debug log for multiple results — commit `53abbbae` 4. Remove deprecated `--enable-system-operator` alias — commit `b3b44788` However, the arg side of review comment 3 remains partially unaddressed: the request was to "use the first (and log a debug, not a warning, of all the matches)." The command side logs extra results at debug level (`operator_system.go:50-52`), but the arg side still truncates silently (`operator_system.go:101-103`). See I2. URL: https://github.com/mikefarah/yq/pull/2640#discussion_r3004049491 --- ## Agent Performance Retro ### [Claude] **Pass 1** (604s, 43 tool calls): Produced 1 important and 5 suggestions. Read 19 files and ran 15 grep searches. Used `git blame` (2 calls) to verify findings. The strongest unique contribution was I1 (disabled mode returns null), which Claude traced through `operator_env.go` and `operator_load.go` to demonstrate the inconsistency. Also identified 4 testing gaps (S1-S4) that no other agent found. **Pass 2** (648s, 38 tool calls): Found the security subsumption issue (I6) — a cross-cutting concern requiring understanding of how `exec.Command` subsumes env/file operations. Also identified the multi-node test gap (I7) and the embedded-newline convention issue (S7). Used `git blame` 4 times to verify findings. **Pass 3** (804s, 53 tool calls): Identified the tinygo build-tag gap (I8) by checking all `no_*.go` stubs and `build-tinygo-yq.sh`. This required 30 grep searches and 20 file reads — the most thorough exploration of any pass. Also found the multiple-semicolons UX issue (S11), the duplicated flagsPrefix code (S12), and the eval+system composition gap (S20). Used `git blame` 10 times. **Pass 4** (757s, 54 tool calls): Found the flag naming inconsistency (S13) by comparing all `--security-*` flags, and two error-handling suggestions (S14, S22). 30 grep searches, 0 important findings — diminishing returns as expected for the final pass. **Coverage misses**: None. Claude reviewed all 9 files in every pass. ### [Codex] **Pass 1** (249s, 45 tool calls): Produced 2 important and 1 suggestion. Codex was the only agent to identify the non-string scalar validation gap (I3), tracing through YAML tag handling. It also independently found the arg truncation issue (I2) and the CLI flag test gap (S4). Used `git blame` 6 times — the most of any agent in pass 1. Ran `go test` (6 calls) to verify the test suite passes. **Pass 2** (200s, 46 tool calls): Found no new issues beyond the 13 already known. Thorough investigation (23 `sed -n` for targeted line reads, 11 `rg` searches, 3 `git blame`) but the review was exhausted. Dropped after this pass. **Coverage misses**: None. Codex reviewed all 9 files. ### [Gemini] **Pass 1** (394s, 26 tool calls): Most productive initial pass with 5 important and 2 suggestions. Unique contributions: the misleading docs finding (I4) — an insight requiring understanding of yq's block operator semantics — and the invalid arg type issue (I5). Also independently found I1 (disabled returns null) and S6 (stderr discarded on success). Notably ran `./yq` 10 times for live verification, the only agent to do so. **Pass 2** (350s, 27 tool calls): Found 3 new issues (double allocation S9, stdout discarded on failure, missing command abort handling). Ran `go test` 13 times for verification. However, Gemini's I3 (missing command aborts evaluation) is a design choice, not a bug — erroring on a missing command is the safer default. **Pass 3** (301s, 0 useful output): Hit Gemini Pro quota exhaustion (`TerminalQuotaError: QUOTA_EXHAUSTED`). Dropped. **Coverage misses**: In pass 1, did not run `git blame` (expected — known Gemini quota limitation). This means Gemini cannot distinguish regressions from pre-existing issues. In this PR all changed files are new, so the distinction is moot. **Tool call highlights:** - **`git blame` usage**: Claude used blame in every pass (2, 4, 10, 2 = 18 total). Codex used it in passes 1-2 (6, 3 = 9 total). Gemini never used blame — a known quota trade-off. - **Live verification**: Gemini was the only agent to run `./yq` directly (10 times in pass 1), testing operator behaviour against the actual binary rather than relying solely on code reading. - **Test execution**: Codex ran `go test` 6 times in pass 1; Gemini ran it 13 times in pass 2. Claude never ran tests, relying on code analysis. ### Summary | | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro | |---|---|---|---| | Duration (pass 1) | 604s | 249s | 394s | | Findings (pass 1) | 1I 5S | 2I 1S | 5I 2S | | Tool calls (pass 1) | 43 (Read 19, Grep 15) | 45 (exec 43) | 26 (shell 15, grep 9) | | Duration (pass 2) | 648s | 200s | 350s | | Findings (pass 2) | 2I 3S | 0 | 3I 2S | | Tool calls (pass 2) | 38 (Read 15, Grep 10) | 46 (exec 44) | 27 (shell 19, grep 6) | | Duration (pass 3) | 804s | — | 301s (empty) | | Findings (pass 3) | 1I 4S | — | 0 (quota) | | Tool calls (pass 3) | 53 (Grep 30, Read 20) | — | — | | Duration (pass 4) | 757s | — | — | | Findings (pass 4) | 0I 3S | — | — | | Tool calls (pass 4) | 54 (Grep 30, Read 22) | — | — | | **Totals** | **4I 15S** | **2I 1S** | **5I 2S** + empty | | False positives | 0 | 0 | 1 (I3 pass 2: design choice) | | Downgraded | 0 | 0 | 3 (I1→S9, I2→informational, I3→informational) | | Unique insights | I1, I6, I7, I8, S3, S7-S14 | I3, I2 | I4, I5, S5, S6 | | Coverage misses | 0 | 0 | 0 | **Reconciliation:** - Gemini P1 I5 (stderr on success) → S6 (suggestion; standard CLI behaviour) - Gemini P2 I1 (double allocation) → S9 (suggestion; marginal performance impact) - Gemini P2 I2 (stdout on failure) → informational (merged into existing S7 context) - Gemini P2 I3 (missing command aborts) → informational (erroring is the correct defensive behaviour) - Claude P1 S1 (arg truncation) → promoted to I2 (important; Codex's severity assessment was more accurate) All three agents contributed distinct value. Gemini excelled in pass 1 with the highest finding count and unique documentation insight (I4). Codex was the fastest and most precise, finding the non-string validation gap (I3) that required deep YAML tag knowledge. Claude was the most persistent across all 4 passes and found the cross-cutting issues (I6 security subsumption, I8 tinygo build tag) that required broad codebase exploration. --- ## Review Process Notes No suggestions for this review — the prompt guided agents well, the findings were high quality, and no prompt gaps were identified. --- ## Appendix: Original Reviews ### [Claude] — Pass 1 Now I have a thorough understanding of the code. Let me write the review. --- ## Executive Summary This PR adds a `system` operator for running external commands from yq expressions, disabled by default behind `--security-enable-system-operator`. The implementation correctly follows yq's existing patterns for argument parsing (semicolon-separated blocks), per-node evaluation, and security preference gating. The primary concern is that the disabled mode silently injects null values instead of returning an error, diverging from the env/file operator precedent in a way that risks silent data corruption in pipelines. --- ## Findings ### Critical Issues None. ### Important Issues I1. **Disabled mode silently injects nulls instead of erroring** — `operator_system.go:63-71` (important, regression) ```go func systemOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) { if !ConfiguredSecurityPreferences.EnableSystemOps { log.Warning("system operator is disabled, use --security-enable-system-operator flag to enable") results := list.New() for el := context.MatchingNodes.Front(); el != nil; el = el.Next() { candidate := el.Value.(*CandidateNode) results.PushBack(candidate.CreateReplacement(ScalarNode, "!!null", "null")) } return context.ChildContext(results), nil } ``` When the system operator is disabled (the default), it returns `null` for each matched node with only a warning log. The existing env and file operators return **errors** when their security gates are active: ```go // operator_env.go:20-22 if ConfiguredSecurityPreferences.DisableEnvOps { return Context{}, fmt.Errorf("env operations have been disabled") } // operator_load.go:66-68 if ConfiguredSecurityPreferences.DisableFileOps { return Context{}, fmt.Errorf("file operations have been disabled") } ``` The practical risk: in a pipeline like `yq '.field = system("cmd")' file.yml | kubectl apply -f -`, a user who forgets `--security-enable-system-operator` gets `null` values silently embedded in their output with a zero exit code. The warning goes to stderr, which pipelines routinely ignore. The env/file operators prevent this class of mistake by failing hard. Fix: return an error for consistency, or at minimum return a non-zero exit code. The error approach is simpler and matches the codebase convention: ```diff if !ConfiguredSecurityPreferences.EnableSystemOps { - log.Warning("system operator is disabled, use --security-enable-system-operator flag to enable") - results := list.New() - for el := context.MatchingNodes.Front(); el != nil; el = el.Next() { - candidate := el.Value.(*CandidateNode) - results.PushBack(candidate.CreateReplacement(ScalarNode, "!!null", "null")) - } - return context.ChildContext(results), nil + return Context{}, fmt.Errorf("system operations are disabled, use --security-enable-system-operator to enable") } ``` If the null-return behaviour is intentional (the test and docs indicate it is), this should be documented as a deliberate divergence and the rationale recorded in a code comment. --- ### Suggestions S1. **Args expression silently drops extra results** — `operator_system.go:101-103` (suggestion, regression) ```go if argsNodes.MatchingNodes.Front() != nil { args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode)) } ``` When the args expression produces multiple results (e.g. `system("echo"; "a", "b")` via union), only the first result is used and the rest are silently discarded. This is inconsistent with `resolveCommandNode` (line 50-51), which logs a debug message when multiple command results are returned. A user writing `system("cmd"; "a", "b")` gets only `"a"` with no indication that `"b"` was dropped. Fix: add a debug log matching the command-resolution pattern: ```diff +if argsNodes.MatchingNodes.Len() > 1 { + log.Debugf("system operator: args expression returned %d results, using first", argsNodes.MatchingNodes.Len()) +} if argsNodes.MatchingNodes.Front() != nil { args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode)) } ``` S2. **Misleading "parse time" comment** — `operator_system.go:74` (suggestion, regression) ```go // determine at parse time whether we have (command; args) or just (command) hasArgs := expressionNode.RHS.Operation.OperationType == blockOpType ``` This runs inside the operator handler during expression **evaluation**, not at parse time. The parser has already finished by this point. Fix: ```diff -// determine at parse time whether we have (command; args) or just (command) +// determine whether we have (command; args) or just (command) ``` S3. **No test verifying stdin piping** — `operator_system_test.go` (suggestion, gap) The documentation header states "The current matched node's value is serialised and piped to the command via stdin" but no test verifies this. A test using `cat` (which echoes stdin to stdout) would confirm the YAML encoding is correctly piped: ```go { description: "Stdin is piped to command", skipDoc: true, document: "greeting: hello", expression: `system("` + catPath + `")`, expected: []string{ "D0, P[], (!!str)::greeting: hello\n", }, }, ``` S4. **No test for `--null-input` usage** — `operator_system_test.go` (suggestion, gap) The documentation header shows `--null-input` as a primary usage pattern, but all test scenarios provide a `document`. A `--null-input` scenario (empty `document` field) would verify the operator works correctly when the matched node is a null scalar. S5. **No subprocess timeout** — `operator_system.go:123-128` (suggestion, enhancement) ```go cmd := exec.Command(command, args...) cmd.Stdin = &stdin var stderr bytes.Buffer cmd.Stderr = &stderr output, err := cmd.Output() ``` A command that hangs (e.g. `cat` with no stdin EOF, or a network command) will block yq indefinitely. Using `exec.CommandContext` with a configurable or reasonable default timeout would prevent this. This is a future enhancement — the current behaviour matches standard CLI tool conventions. --- ## Design Observations **Concerns:** - **(in-scope)** The inverted security model (enable vs. disable) is sound, but the inverted failure mode (null vs. error) breaks the principle of least surprise. Users who learn that `--security-disable-env-ops` causes errors will reasonably expect that the absence of `--security-enable-system-operator` also causes errors. See I1. **Strengths:** - **(in-scope)** Clean separation of security gating from operator logic. The opt-in model is appropriate for command execution. - **(in-scope)** Per-node evaluation via `SingleReadonlyChildContext` correctly scopes expression evaluation, matching the pattern used by `loadStringOperator` and other operators. - **(in-scope)** The `#nosec G204` annotation (line 122) with explanation is good practice — it acknowledges the gosec warning and documents the mitigation. - **(in-scope)** The `resolveCommandNode` helper cleanly validates the command expression result with specific error messages for each failure mode (no results, wrong type, empty string). - **(in-scope)** The `yqFlags` field added to `expressionScenario` is a clean, general solution for doc generation — reusable by any operator that needs CLI flags in examples. --- ## Testing Assessment **Covered scenarios:** disabled mode (null return), single string arg, no args, array args, per-node expression evaluation, command failure (non-zero exit), null command expression. **Untested scenarios ranked by risk:** 1. **Stdin piping** — the core feature of passing the node's YAML to the command's stdin is not directly verified by any test. 2. **`--null-input` mode** — documented as a primary usage pattern but not tested. 3. **Multiple matched nodes with different values** — only one test has a single matched node in enabled mode. No test verifies that each node in a multi-node context gets its own command invocation with the correct stdin. 4. **Empty string command** — the `resolveCommandNode` check at line 57-58 is not exercised by any test. 5. **Command that produces stderr but succeeds** — the stderr capture (line 130-133) is only tested via the failure path. --- ## Documentation Assessment The header documentation (`headers/system-operators.md`) is clear and well-structured, with correct UK English ("serialised"). The generated documentation correctly includes both disabled and enabled examples. Minor note: the header says "An argument or array of arguments separated by `;`" which could be read as "arguments separated by semicolons" rather than "the command and args separated by a single semicolon." Consider: "Optional: a single argument or an array of arguments, separated from the command by `;`." --- ## Commit Structure The commit history shows typical Copilot agent iteration: an initial plan commit (`d181cf87`, empty), the main implementation, followed by multiple fixup commits in response to review feedback. Several commits are patches on earlier ones: - `6f94991c Apply suggestions from code review` and `e10e8127 Apply suggestions from code review` fix issues in earlier commits - `6315cfe9 Update pkg/yqlib/operator_system.go` is a small fixup from the maintainer - `d181cf87 Initial plan` is an empty commit This is expected for a Copilot-authored PR iterated via review comments. Consider squash-merging to produce a clean single commit. --- ## Acknowledged Limitations - `#nosec G204` (line 122): the gosec subprocess-with-variable-args warning is explicitly suppressed with documented rationale. --- ## Unresolved Feedback All four substantive review comments from the repo owner have been addressed by subsequent commits: 1. Per-node evaluation with `SingleReadonlyChildContext` → commit `da611f7a` 2. Missing `--security-enable-system-operator` in doc examples → commit `884c2d8b` 3. Validate command node type, debug log for multiple results → commit `53abbbae` 4. Remove deprecated `--enable-system-operator` alias → commit `b3b44788` --- ## Coverage Summary | File | Status | |------|--------| | `cmd/root.go` | Reviewed, no issues | | `pkg/yqlib/doc/operators/headers/system-operators.md` | Reviewed, minor wording note in Documentation Assessment | | `pkg/yqlib/doc/operators/system-operators.md` | Reviewed, no issues (generated) | | `pkg/yqlib/lexer_participle.go` | Reviewed, no issues | | `pkg/yqlib/operation.go` | Reviewed, no issues | | `pkg/yqlib/operator_system.go` | **I1, S1, S2, S5** | | `pkg/yqlib/operator_system_test.go` | **S3, S4** | | `pkg/yqlib/operators_test.go` | Reviewed, no issues | | `pkg/yqlib/security_prefs.go` | Reviewed, no issues | ### [Claude] — Pass 2 Now I have enough context for a thorough pass-2 review. Let me write the report. --- ## Executive Summary This PR adds a `system(command; args)` operator for executing external commands, gated behind `--security-enable-system-operator`. The implementation is functional and the core security gate works. Pass 2 identifies a documentation gap in the security model (system ops silently subsume the env/file security flags), an untested multi-node execution path, and several smaller issues. ## Findings ### Critical Issues No new critical issues found. ### Important Issues I1. **`system` operator subsumes env/file security restrictions — undocumented** — `headers/system-operators.md:1-23`, `cmd/root.go:213-215` (important, gap) ```go // cmd/root.go:213-215 rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.DisableEnvOps, "security-disable-env-ops", "", false, ...) rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.DisableFileOps, "security-disable-file-ops", "", false, ...) rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.EnableSystemOps, "security-enable-system-operator", "", false, ...) ``` When `--security-enable-system-operator` is active, both `--security-disable-env-ops` and `--security-disable-file-ops` become ineffective: `system("printenv"; "SECRET")` reads environment variables and `system("cat"; "/path/to/file")` reads arbitrary files. An operator using `exec.Command` is strictly more powerful than both `env()` and `load()` combined. The documentation says only "The system operator is disabled by default" without mentioning this subsumption. An admin who configures `--security-disable-env-ops --security-enable-system-operator` may believe env access is blocked. Fix: Add a note to `headers/system-operators.md`: ```diff **Security warning**: The system operator is disabled by default. You must explicitly pass `--security-enable-system-operator` to use it. + +**Note:** When enabled, the system operator can replicate the functionality of `env` and `load` operators via external commands. +Enabling it effectively overrides `--security-disable-env-ops` and `--security-disable-file-ops`. ``` --- I2. **Multi-node execution path completely untested** — `operator_system_test.go` (important, regression) ```go // operator_system.go:79-145 for el := context.MatchingNodes.Front(); el != nil; el = el.Next() { candidate := el.Value.(*CandidateNode) nodeContext := context.SingleReadonlyChildContext(candidate) // ... evaluate command, exec, accumulate results ... results.PushBack(newNode) } ``` All 7 test scenarios (1 disabled + 6 enabled) exercise this loop with exactly one matched node. The result accumulation across multiple iterations, per-node context isolation, and failure-on-Nth-node behavior are untested. A bug in how `nodeContext` is constructed per iteration or how results are pushed to the list would go undetected. Fix: Add a multi-node test. For example, a multi-document scenario or a sequence-based input: ```go { description: "System operator processes multiple documents", skipDoc: true, document: "a: first", document2: "a: second", expression: `.a = system("` + echoPath + `"; "replaced")`, expected: []string{ "D0, P[], (!!map)::a: replaced\n", "D1, P[], (!!map)::a: replaced\n", }, }, ``` ### Suggestions S1. **Embedded newline in error value** — `operator_system.go:132` (suggestion, regression) ```go return Context{}, fmt.Errorf("system command '%v' failed: %w\nstderr: %v", command, err, stderrStr) ``` This error message contains a literal `\n`. In Go, error values are conventionally single-line strings. A newline inside an error can break structured log parsers, JSON error output, and any consumer that assumes errors are one-line. No other `fmt.Errorf` call in `pkg/yqlib/` embeds a newline (verified via grep). Fix: Use a semicolon or parenthetical to keep it single-line: ```diff -return Context{}, fmt.Errorf("system command '%v' failed: %w\nstderr: %v", command, err, stderrStr) +return Context{}, fmt.Errorf("system command '%v' failed (stderr: %v): %w", command, stderrStr, err) ``` --- S2. **Doc generation depends on test execution order** — `operator_system_test.go:40,113` (suggestion, gap) ```go // line 40 — creates fresh file with header + disabled scenarios documentOperatorScenarios(t, "system-operators", systemOperatorDisabledScenarios) // line 113 — appends enabled scenarios to existing file appendOperatorDocumentScenario(t, "system-operators", scenarios) ``` `TestSystemOperatorDisabledScenarios` must run before `TestSystemOperatorEnabledScenarios` because the first creates the doc file with its header (via `os.Create`) and the second only appends (via `os.O_APPEND`). Running the enabled test in isolation (`go test -run TestSystemOperatorEnabled`) creates a headerless doc file. While Go runs tests in source order within a file, this implicit ordering dependency is fragile. This replicates the same pattern from `operator_load_test.go` and `operator_traverse_path_test.go`. Fix: No code change needed for correctness under `go test`, but a comment documenting the ordering dependency would prevent future breakage: ```go // NOTE: TestSystemOperatorDisabledScenarios must run first to create the // doc file with its header. This test appends to it. ``` --- S3. **Partial side effects on multi-node failure are inherent but undocumented** — `operator_system.go:79-145` (suggestion, gap) If the system operator processes multiple matched nodes and the command for the Nth node fails, commands for nodes 1 through N-1 have already executed. Their side effects (file writes, network calls, etc.) persist despite the error. This is inherent to executing external commands sequentially and cannot be fixed without a transactional model. However, the documentation makes no mention of this behavior. Fix: Add a note to the documentation header: ```markdown When processing multiple matched nodes, the command runs once per node. If a command fails partway through, earlier commands' effects are not rolled back. ``` ## Design Observations ### Concerns - **(in-scope)** The security model has an implicit hierarchy: `system` ≥ `env` + `load`. The flags are presented as independent toggles, but enabling system ops effectively grants everything the other two provide. This should be made explicit to users. - **(in-scope)** The operator executes commands sequentially per matched node. For large node sets, this could be slow with no parallelism option, but sequential execution is the correct default for determinism and simplicity. - **(future)** The operator returns `!!str` for all output. A future enhancement could support `system_yaml`/`system_json` variants that parse the command output, similar to how `load` vs `load_str` work. ### Strengths - **(in-scope)** Using `exec.Command` (no shell) avoids shell injection entirely. This is the right choice. - **(in-scope)** The `SingleReadonlyChildContext` per-node evaluation (commit da611f7a, responding to review comment #1) correctly scopes expression evaluation so that `system(.cmd; .arg)` resolves per matched node. - **(in-scope)** The inverted default (`EnableSystemOps: false` vs `DisableEnvOps: false`) correctly makes the more dangerous capability opt-in rather than opt-out. - **(in-scope)** The `#nosec G204` annotation at line 122 properly acknowledges the intentional use of variable command paths for gosec. ## Testing Assessment Test coverage is reasonable for the happy path but has gaps in the following areas, ranked by risk: 1. **Multi-node execution** (I2 above) — the core for loop is never tested with >1 node 2. Stdin piping (known S4) — the YAML-to-stdin pipeline is not directly verified 3. `--null-input` usage (known S5) — documented pattern without test coverage 4. CLI flag wiring (known S7) — Cobra flag binding is untested 5. **Partial failure** (S3 above) — no test where Nth command fails after N-1 succeed ## Documentation Assessment - The header doc at `headers/system-operators.md:15` still says "An argument or array of arguments separated by `;`" (known I4), which conflates the block separator `;` with argument list separators. - The security documentation does not mention the subsumption relationship with env/file ops (I1 above). - Generated docs embed host-specific paths (known S1), which will produce different output on different machines. ## Commit Structure The history contains 5 commits that either patch mistakes from earlier commits or have uninformative messages: | Commit | Message | Issue | |--------|---------|-------| | `6315cfe9` | "Update pkg/yqlib/operator_system.go" | Filename-only message; no indication of *what* changed | | `5ea069a5` | "Update pkg/yqlib/doc/operators/system-operators.md" | Same | | `2c8605f6` | "Update pkg/yqlib/doc/operators/headers/system-operators.md" | Same | | `e10e8127` | "Apply suggestions from code review" | Opaque; `--stat` shows it touched `cmd/root.go` + `operator_system.go` | | `6f94991c` | "Apply suggestions from code review" | Opaque; `--stat` shows it touched `operator_system.go` | These are Copilot-generated fix-up commits responding to review feedback. Before merge, consider squashing them into the substantive commits they amend so the history tells a coherent story. ## Acknowledged Limitations - `#nosec G204` at line 122 acknowledges the intentional use of user-controlled command paths, with the comment explaining the opt-in security model. ## Unresolved Feedback All 4 PR review comments from @mikefarah appear to have been addressed by subsequent commits. No unresolved substantive feedback. ## Coverage Summary | File | Findings | |------|----------| | `cmd/root.go` | I1 | | `pkg/yqlib/doc/operators/headers/system-operators.md` | I1 | | `pkg/yqlib/doc/operators/system-operators.md` | Reviewed, no new issues | | `pkg/yqlib/lexer_participle.go` | Reviewed, no new issues | | `pkg/yqlib/operation.go` | Reviewed, no new issues | | `pkg/yqlib/operator_system.go` | S1, S3 | | `pkg/yqlib/operator_system_test.go` | I2, S2 | | `pkg/yqlib/operators_test.go` | Reviewed, no new issues | | `pkg/yqlib/security_prefs.go` | Reviewed, no new issues | ### [Claude] — Pass 3 # Deep Code Review — Pass 3 **PR #2640: Add system(command; args) operator (disabled by default)** **Verdict:** Approve with reservations (address I8 before merge; others can follow) --- ## Executive Summary The system operator is well-implemented and composes correctly with yq's existing operators (pipe, select, with, reduce, eval). The lexer registration has no conflicts, operator precedence is appropriate, and the security gate works at runtime as required. This pass identifies one important issue (the `os/exec` import breaks the tinygo build pattern) and several suggestions around error clarity and test infrastructure. --- ## Findings ### Critical Issues None. ### Important Issues I8. **`os/exec` import has no build-tag exclusion mechanism, may break tinygo build** — `operator_system.go:7`, `scripts/build-tinygo-yq.sh` (important, gap) ```go import ( "os/exec" ) ``` The system operator is the **only** file in the entire `pkg/yqlib` package that imports `os/exec`. Every other conditionally-heavy dependency (JSON, XML, TOML, Lua, HCL, etc.) has a corresponding `no_*.go` stub and build tag so it can be excluded from tinygo builds (`scripts/build-tinygo-yq.sh`). The system operator has no such mechanism. On native tinygo targets, `os/exec` has partial support and may work. On constrained targets (e.g. WASM), it does not. Either way, this is the first new external-process dependency in the library and should follow the established exclusion pattern. Fix: Add a `yq_nosystem` build tag. Create `no_system.go` with a stub handler that returns an error: ```go //go:build yq_nosystem package yqlib func init() { systemOpType.Handler = func(_ *dataTreeNavigator, _ Context, _ *ExpressionNode) (Context, error) { return Context{}, fmt.Errorf("system operator not available in this build") } } ``` And gate `operator_system.go` with `//go:build !yq_nosystem`. Update the tinygo build script to include `yq_nosystem`. ### Suggestions S17. **Multiple semicolons produce a confusing "no results" error** — `operator_system.go:86-88` (suggestion, regression) ```go if hasArgs { block := expressionNode.RHS commandNodes, err := d.GetMatchingNodes(nodeContext, block.LHS) ``` When a user writes `system("cmd"; "arg1"; "arg2")`, the parser creates nested BLOCK nodes: `BLOCK(BLOCK("cmd", "arg1"), "arg2")`. The system operator evaluates `block.LHS` (the inner BLOCK) via `GetMatchingNodes`, which invokes `emptyOperator` (the BLOCK handler) and returns an empty context. This triggers the error at line 48: `"system operator: command expression returned no results"`. The error doesn't hint at the real problem. Since the documented syntax for multiple args is `system("cmd"; ["arg1", "arg2"])`, this is a usability issue rather than a correctness issue. Fix: Add a check before evaluating the block: ```diff if hasArgs { block := expressionNode.RHS + if block.LHS != nil && block.LHS.Operation != nil && + block.LHS.Operation.OperationType == blockOpType { + return Context{}, fmt.Errorf( + "system operator: too many semicolons; use system(\"cmd\"; [\"arg1\", \"arg2\"]) for multiple arguments") + } commandNodes, err := d.GetMatchingNodes(nodeContext, block.LHS) ``` S18. **`flagsPrefix` logic duplicated in `documentInput`** — `operators_test.go:360-363,371-374` (suggestion, regression) ```go // Lines 360-363 (document branch) flagsPrefix := "" if s.yqFlags != "" { flagsPrefix = s.yqFlags + " " } // ... // Lines 371-374 (null-input branch) flagsPrefix := "" if s.yqFlags != "" { flagsPrefix = s.yqFlags + " " } ``` The same 3-line pattern is repeated in both branches of `documentInput`. Fix: Hoist to before the if/else: ```diff + flagsPrefix := "" + if s.yqFlags != "" { + flagsPrefix = s.yqFlags + " " + } + if s.document != "" { // ... - flagsPrefix := "" - if s.yqFlags != "" { - flagsPrefix = s.yqFlags + " " - } ``` S19. **`yqFlags` is cosmetic — not used in test execution or doc output generation** — `operators_test.go:34`, `operators_test.go:380-436` (suggestion, gap) ```go yqFlags string // extra yq flags to include in generated doc command snippets ``` The `yqFlags` field only affects the bash snippet shown in generated docs (`documentInput`). It is NOT used by `testScenario` (which evaluates the expression directly) nor by `documentOutput` (which generates the output shown in docs). Both rely on global state (`ConfiguredSecurityPreferences`) set by the calling test function. This means the doc snippet says `yq --security-enable-system-operator ...` but the test and doc output are produced by a completely separate mechanism (global variable toggle). If a future `yqFlags` value controlled output-affecting behaviour (not just security gating), the docs would show incorrect output. Fix: Document the limitation in the field comment: ```diff - yqFlags string // extra yq flags to include in generated doc command snippets + yqFlags string // cosmetic: included in generated doc command snippets only; does not affect test execution or doc output evaluation ``` S20. **No test for `eval` dynamically invoking `system`** — `operator_system_test.go` (suggestion, gap) The `eval` operator can construct system calls at runtime: `eval("system(\"echo\"; \"hello\")")`. The security gate correctly blocks this at runtime (the check is in the operator handler, not the parser). But there is no test confirming this interaction works in both enabled and disabled modes. Fix: Add a test case: ```go { description: "eval can invoke system when enabled", skipDoc: true, document: "a: hello", expression: `.a = eval("system(\"` + echoPath + `\"; \"test\")")`, expected: []string{ "D0, P[], (!!map)::a: test\n", }, }, ``` --- ## Design Observations ### Concerns - **(in-scope)** The security model is runtime-only: the `system` keyword is always parsed and the expression tree always includes SYSTEM nodes. Only the handler checks the flag. This is correct (necessary for `eval` composition) but means static analysis of yq expressions cannot determine whether system calls will execute. - **(future)** The `os/exec` import is unconditional and unique in the library. As yq expands to more targets (WASM, embedded), a compile-time exclusion mechanism (see I8) would prevent surprises. ### Strengths - **(in-scope)** The operator correctly uses `SingleReadonlyChildContext` for evaluating command/args per matched node, preventing argument expressions from mutating the document. - **(in-scope)** The `blockOpType` check for optional arguments follows the exact pattern used by `match`, `datetime`, and `with` operators. - **(in-scope)** `.system` (dot-prefixed) is correctly tokenized as a `PathElement`, not as the SYSTEM operator, because the `PathElement` lexer rule at `lexer_participle.go:215` matches before the keyword rule. Users with YAML keys named "system" are unaffected. - **(in-scope)** The `CheckForPostTraverse` field is intentionally omitted from `systemOpType`. Since `system()` always uses parentheses, the closing `)` token (which has `CheckForPostTraverse: true`) handles post-traversal insertion for patterns like `system("cmd").field`. --- ## Testing Assessment Test coverage is adequate for the core paths. Untested scenarios, ranked by risk: 1. **`eval` + `system` composition** (see S20) — confirms the security gate works through dynamic expression construction. 2. **Multi-document YAML input** — verifies per-node loop and stdin encoding when documents have different structures. 3. **Nested `system` calls** — e.g. `system(system("echo"; "echo"); "hello")`, confirming the inner call's result is used as the outer command. 4. **Expression yielding zero nodes** — e.g. `.nonexistent[] | system("echo")`, confirming no command is executed and an empty result is returned. --- ## Documentation Assessment No additional gaps beyond those flagged in prior passes (I4, S1). --- ## Commit Structure The commit history shows iterative refinement from Copilot review feedback (commits `da611f7a`, `53abbbae`, `884c2d8b`, `e10e8127`, `6f94991c`, `b3b44788`). Each commit addresses specific feedback. The pattern of "implement, review, fix" produces a clean final state but a noisy history. A squash before merge would be appropriate. --- ## Acknowledged Limitations - `#nosec G204` at line 122 — intentional: the operator requires explicit opt-in via `--security-enable-system-operator`. --- ## Unresolved Feedback None. All four PR review comments from @mikefarah have been addressed by subsequent commits. --- ## Coverage Summary | File | Status | |------|--------| | `cmd/root.go` | I8 | | `pkg/yqlib/doc/operators/headers/system-operators.md` | Reviewed, no new issues | | `pkg/yqlib/doc/operators/system-operators.md` | Reviewed, no new issues | | `pkg/yqlib/lexer_participle.go` | Reviewed, no new issues (no conflicts, correct placement) | | `pkg/yqlib/operation.go` | I8 | | `pkg/yqlib/operator_system.go` | I8, S17 | | `pkg/yqlib/operator_system_test.go` | S19, S20 | | `pkg/yqlib/operators_test.go` | S18, S19 | | `pkg/yqlib/security_prefs.go` | Reviewed, no new issues | ### [Claude] — Pass 4 Now I have enough context. Let me write the review. --- # Deep Code Review — PR #2640 (Pass 4, Final) | Property | Value | |----------|-------| | **PR** | #2640 — Add system(command; args) operator | | **Verdict** | Approve with suggestions | | **Risk** | Low (new, isolated operator behind explicit opt-in) | ## Executive Summary This PR adds a well-scoped `system` operator gated behind `--security-enable-system-operator`. The implementation follows established yq operator patterns (simpleOp registration, blockOpType for optional args, SingleReadonlyChildContext per node). The three prior review passes covered the major correctness, security, and testing issues. This final pass focuses on cross-cutting concerns and boundary conditions the earlier passes missed. ## Findings ### Critical Issues None. ### Important Issues None. ### Suggestions S21. **Flag suffix inconsistent with existing security flags** — `cmd/root.go:215` (suggestion, regression) ```go rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.EnableSystemOps, "security-enable-system-operator", "", false, ...) ``` The existing flags use a plural abbreviated suffix: - `--security-disable-env-ops` - `--security-disable-file-ops` The new flag uses singular spelled-out `--security-enable-system-operator`. The Go field name `EnableSystemOps` already follows the `*Ops` convention, so only the CLI flag is inconsistent. Fix: rename to `--security-enable-system-ops` for consistency. ```diff - rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.EnableSystemOps, "security-enable-system-operator", "", false, "Enable system operator to allow execution of external commands.") + rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.EnableSystemOps, "security-enable-system-ops", "", false, "Enable system operator to allow execution of external commands.") ``` Note: all references in operator_system.go log messages, test descriptions, and both doc files would need updating to match. S22. **YAML encoding error returned without operator context** — `operator_system.go:116-118` (suggestion, regression) ```go encoded, err := encodeToYamlString(candidate) if err != nil { return Context{}, err } ``` Every other error path in `systemOperator` prefixes with `"system operator:"` or `"system command '...' failed:"`. An encoding error here (e.g. from a node with unserialisable state) would propagate without that prefix, making it hard to trace in complex expressions. Fix: wrap the error. ```diff encoded, err := encodeToYamlString(candidate) if err != nil { - return Context{}, err + return Context{}, fmt.Errorf("system operator: failed to encode input: %w", err) } ``` S23. **Repeated warning on per-element assignment** — `operator_system.go:65` (suggestion, regression) ```go log.Warning("system operator is disabled, use --security-enable-system-operator flag to enable") ``` When used with `|=` on multiple elements (e.g. `.[] |= system("cmd")`), the assign operator calls `systemOperator` once per LHS candidate. Each call logs the identical warning. For large arrays this produces significant log noise. Compare with `DisableEnvOps` / `DisableFileOps` which return an error on first invocation, halting immediately. Fix: this is low-priority, but if addressed, either (a) use a `sync.Once` or package-level flag to emit the warning once per process, or (b) align with env/load and return an error instead of null (which would also address known finding I1). ## Design Observations ### Concerns - **(in-scope)** The polarity inversion (enable vs disable) between system and the env/file flags is intentional but may confuse users writing security-hardened scripts. A user who sets `--security-disable-env-ops --security-disable-file-ops` might assume system execution is also disabled — and it is (by default), but the asymmetry in flag naming makes it easy to overlook during auditing. All three flags should be visible in `--help` output with consistent naming (see S21). ### Strengths - **(in-scope)** The operator correctly pipes the per-node YAML serialisation to stdin and strips exactly one trailing newline from stdout, matching the semantics users would expect from jq's `@sh` / `input` patterns. - **(in-scope)** Using `exec.Command(command, args...)` avoids shell injection entirely — arguments are passed directly to the exec syscall, not through a shell. - **(in-scope)** The `hasArgs` check at line 75 correctly inspects the AST structure once outside the loop, consistent with how `match`, `sub`, and `with` operators handle optional block arguments. ## Testing Assessment Test coverage for the new operator is reasonable. The known gaps from prior passes (S4: no stdin piping test, S5: no --null-input test, I7: multi-node path untested, S7: CLI flag not exercised) remain the most impactful untested scenarios. No additional untested scenarios identified beyond those already flagged. ## Documentation Assessment The generated documentation at `doc/operators/system-operators.md` correctly shows the flag for enabled scenarios and omits it for the disabled scenario. The header documentation at `doc/operators/headers/system-operators.md` uses UK English ("serialised") consistent with repo conventions. The known issue I4 (misleading semicolon description for multiple args) remains the main doc gap. ## Commit Structure The commit history shows iterative refinement responding to reviewer feedback (commits `53abbbae`, `e10e8127`, `b3b44788`, `6f94991c`). While earlier commits introduced issues that later commits fixed (e.g. the deprecated alias added and then removed), this is typical of a review-driven workflow and the final state is clean. No additional structural concerns beyond S10 (doc generation depends on test execution order, already known). ## Acknowledged Limitations - `#nosec G204` at line 122 acknowledges the intentional command execution, gated by the explicit opt-in flag. ## Unresolved Feedback All four substantive PR review comments from @mikefarah have been addressed by subsequent commits: 1. SingleReadonlyChildContext evaluation — addressed in `da611f7a` 2. Missing flag in doc examples — addressed in `884c2d8b` 3. Command node type validation — addressed in `53abbbae` 4. Deprecated alias removal — addressed in `b3b44788` No unresolved feedback. ## Coverage Summary | File | Status | |------|--------| | `cmd/root.go` | **S21** | | `pkg/yqlib/doc/operators/headers/system-operators.md` | Reviewed, no new issues | | `pkg/yqlib/doc/operators/system-operators.md` | Reviewed, no new issues | | `pkg/yqlib/lexer_participle.go` | Reviewed, no new issues | | `pkg/yqlib/operation.go` | Reviewed, no new issues | | `pkg/yqlib/operator_system.go` | **S22, S23** | | `pkg/yqlib/operator_system_test.go` | Reviewed, no new issues | | `pkg/yqlib/operators_test.go` | Reviewed, no new issues | | `pkg/yqlib/security_prefs.go` | Reviewed, no new issues | ### [Codex] — Pass 1 | Verdict | Needs changes | | --- | --- | ### Executive Summary This PR adds a new opt-in `system()` operator, wires it into the parser and CLI, and documents the security gate. The overall shape is sound, but the implementation still has two correctness gaps in `pkg/yqlib/operator_system.go`: command validation does not actually enforce “string scalar”, and arg expressions silently discard all matches after the first. ### Findings **Critical Issues** None. **Important Issues** I1. **`system()` accepts non-string commands despite advertising string-only validation** — `pkg/yqlib/operator_system.go:53-55` `(important, regression)` ```go cmdNode := commandNodes.MatchingNodes.Front().Value.(*CandidateNode) if cmdNode.Kind != ScalarNode || cmdNode.Tag == "!!null" { return "", fmt.Errorf("system operator: command must be a string scalar") } ``` The check at `pkg/yqlib/operator_system.go:54` only rejects nulls and non-scalars; it still accepts `!!int`, `!!bool`, and any other scalar tag. In practice this means `yq -n --security-enable-system-operator 'system(123)'` tries to execute `123` instead of failing validation, which contradicts both the error text at `pkg/yqlib/operator_system.go:55` and the documented contract that the command must be a string. This turns simple expression mistakes into external process launches and makes the operator harder to reason about. Fix: ```diff - if cmdNode.Kind != ScalarNode || cmdNode.Tag == "!!null" { + if cmdNode.Kind != ScalarNode || cmdNode.guessTagFromCustomType() != "!!str" { return "", fmt.Errorf("system operator: command must be a string scalar") } ``` Add a regression test for `system(123)` that expects the validation error instead of an `exec` failure. I2. **Multi-result arg expressions are truncated to the first match with no signal** — `pkg/yqlib/operator_system.go:97-102` `(important, regression)` ```go argsNodes, err := d.GetMatchingNodes(nodeContext, block.RHS) if err != nil { return Context{}, err } if argsNodes.MatchingNodes.Front() != nil { args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode)) } ``` At `pkg/yqlib/operator_system.go:101-102`, the operator evaluates the arg expression but only consumes `Front()`. Everything after the first match is silently dropped. A natural expression such as `.out = system("/bin/echo"; .args[])` on `{"args":["a","b"]}` currently produces `out: a`, losing `"b"` without a warning or error. Because the PR explicitly moved command/args evaluation into `SingleReadonlyChildContext`, users will reasonably expect RHS expressions that yield multiple results to behave consistently rather than being truncated. Fix: either flatten all matching arg nodes into `argv`, or reject multi-result arg expressions explicitly. Flattening is the least surprising: ```diff - if argsNodes.MatchingNodes.Front() != nil { - args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode)) - } + if argsNodes.MatchingNodes.Len() > 1 { + log.Debugf("system operator: args expression returned %d results, appending all", argsNodes.MatchingNodes.Len()) + } + for argEl := argsNodes.MatchingNodes.Front(); argEl != nil; argEl = argEl.Next() { + args = append(args, resolveSystemArgs(argEl.Value.(*CandidateNode))...) + } ``` Add a regression test covering `.args[]` so this stays stable. **Suggestions** S1. **The CLI security flag is not exercised by the tests that introduced it** — `pkg/yqlib/operator_system_test.go:29-35`, `pkg/yqlib/operator_system_test.go:47-52`, `cmd/root.go:215` `(suggestion, enhancement)` ```go ConfiguredSecurityPreferences.EnableSystemOps = false ... ConfiguredSecurityPreferences.EnableSystemOps = true ``` The new tests toggle `ConfiguredSecurityPreferences.EnableSystemOps` directly instead of asserting the user-facing wiring added at `cmd/root.go:215`. That means the suite would still pass if the Cobra flag were renamed, not registered on the right command, or otherwise broken. Since the operator is disabled by default, the CLI gate is part of the feature, not just incidental plumbing. Fix: add a `cmd`-level or `acceptance_tests/` case that runs `yq` with and without `--security-enable-system-operator` and asserts the observed behaviour. ### Design Observations **Concerns** - `(in-scope)` The operator’s cardinality rules are underspecified. `system(cmd; expr)` currently treats array-valued args and multi-result args differently, which is why `pkg/yqlib/operator_system.go:97-102` became lossy. **Strengths** - `(in-scope)` Evaluating command and args through `SingleReadonlyChildContext` is the right shape for per-node execution and matches the owner feedback on the PR. - `(in-scope)` The opt-in security gate is implemented at execution time, so merely parsing expressions does not enable process execution. ### Testing Assessment `go test ./...` passes, and the new unit tests cover the basic enabled/disabled paths, array args, per-node evaluation, null commands, and command failure. Highest-risk gaps still untested: 1. CLI flag plumbing for `--security-enable-system-operator` in the actual Cobra command path. 2. Rejection of non-string command nodes such as `system(123)`. 3. Behaviour when the arg expression yields multiple matches, e.g. `.args[]`. ### Documentation Assessment The new operator docs correctly call out that the feature is disabled by default and show the required flag in enabled examples. Once I2 is fixed, the docs should also say whether `system(cmd; expr)` flattens all arg matches or rejects multi-result expressions; the current behaviour is undocumented and surprising. ### Commit Structure The branch history is serviceable but not especially clean. `e10e8127`, `6f94991c`, and `b3b44788` are explicit fixup commits for issues introduced earlier in `9a72c0f7`, which makes the review trail clear but leaves the series less self-contained than ideal. ### Unresolved Feedback - PR comment 3 appears only partially addressed: the request was to “use the first (and log a debug, not a warning, of all the matches).” The current code only logs the count of extra command matches at `pkg/yqlib/operator_system.go:50-52`, and the arg side still truncates extra matches silently at `pkg/yqlib/operator_system.go:101-102`. URL: https://github.com/mikefarah/yq/pull/2640#discussion_r3004049491 ### Coverage Summary - `cmd/root.go` — Reviewed, no issues - `pkg/yqlib/doc/operators/headers/system-operators.md` — Reviewed, no issues - `pkg/yqlib/doc/operators/system-operators.md` — Reviewed, no issues - `pkg/yqlib/lexer_participle.go` — Reviewed, no issues - `pkg/yqlib/operation.go` — Reviewed, no issues - `pkg/yqlib/operator_system.go` — I1, I2 - `pkg/yqlib/operator_system_test.go` — S1 - `pkg/yqlib/operators_test.go` — Reviewed, no issues - `pkg/yqlib/security_prefs.go` — Reviewed, no issues ### [Codex] — Pass 2 ### Executive Summary This PR adds a gated `system(...)` operator, a new persistent CLI flag, and the associated operator documentation/test scaffolding. After reviewing every changed file in context, tracing the parser and execution path, and verifying candidate issues with `git blame`, I did not find any new regressions beyond the 13 issues already recorded in the prior pass. ### Findings **Critical Issues** None. **Important Issues** None. **Suggestions** None. ### Design Observations **Concerns** None beyond the prior pass findings. **Strengths** - `(in-scope)` The operator evaluates command/argument expressions with `SingleReadonlyChildContext`, which matches existing per-node evaluation patterns used by operators like `load`, `select`, and `sort`. - `(in-scope)` The `yqFlags` plumbing in [`pkg/yqlib/operators_test.go`](pkg/yqlib/operators_test.go) centralises flag rendering for generated docs instead of baking one-off string fixes into the `system` docs. ### Testing Assessment Reviewed and spot-checked with: ```bash go test ./pkg/yqlib -run 'TestSystemOperatorEnabledScenarios|TestSystemOperatorDisabledScenarios' go test ./pkg/yqlib -run 'Test.*Operator.*Scenarios' ``` Both passed. No additional test gaps were identified beyond the previously-known pass-1 items. ### Documentation Assessment No new documentation defects identified beyond the previously-known pass-1 items. ### Commit Structure Later commits are follow-up fixes for reviewer feedback, but they stay within the PR’s scope. No new commit-structure concern to flag on this pass. ### Acknowledged Limitations None. ### Unresolved Feedback No newly-identified unresolved feedback beyond the issues already captured in the prior pass. ### Coverage Summary - `cmd/root.go` — Reviewed, no new issues - `pkg/yqlib/doc/operators/headers/system-operators.md` — Reviewed, no new issues - `pkg/yqlib/doc/operators/system-operators.md` — Reviewed, no new issues - `pkg/yqlib/lexer_participle.go` — Reviewed, no new issues - `pkg/yqlib/operation.go` — Reviewed, no new issues - `pkg/yqlib/operator_system.go` — Reviewed, no new issues - `pkg/yqlib/operator_system_test.go` — Reviewed, no new issues - `pkg/yqlib/operators_test.go` — Reviewed, no new issues - `pkg/yqlib/security_prefs.go` — Reviewed, no new issues ### [Gemini] — Pass 1 ### Executive Summary This PR introduces the `system(command; args)` operator, allowing expressions to execute external commands and capture their output. The implementation handles execution via `os/exec` securely and correctly processes arguments as an array rather than subject to shell evaluation. However, the documentation incorrectly instructs users to separate arguments with semicolons, causing silent argument drops. Furthermore, the operator silently ignores invalid arguments instead of halting, discards `stderr` on successful execution, and differs from existing security preference patterns by returning `null` when disabled instead of a hard error. ### Critical Issues *(None)* ### Important Issues I1. **Misleading documentation encourages syntax that silently drops arguments** — `pkg/yqlib/doc/operators/headers/system-operators.md:15` (important, regression) ```markdown - An argument or array of arguments separated by `;` (optional) ``` The documentation incorrectly instructs users to separate multiple arguments with a semicolon. In `yq`, `;` is the block operator, which evaluates expressions sequentially and yields the result of the right-hand expression. If a user writes `system("cmd"; "arg1"; "arg2")`, the arguments block evaluates as `"arg1" ; "arg2"`, which yields `"arg2"`. The command executes without `"arg1"`, silently dropping it. Multiple arguments must be provided as a sequence node (e.g., `["arg1", "arg2"]`). Fix: Update the documentation to clarify that multiple arguments require an array node. ```diff -- An argument or array of arguments separated by `;` (optional) +- An argument (or an array of arguments), separated from the command by `;` (optional) ``` I2. **Disabled security preference returns null instead of an error** — `pkg/yqlib/operator_system.go:65-71` (important, gap) ```go if !ConfiguredSecurityPreferences.EnableSystemOps { log.Warning("system operator is disabled, use --security-enable-system-operator flag to enable") results := list.New() for el := context.MatchingNodes.Front(); el != nil; el = el.Next() { candidate := el.Value.(*CandidateNode) results.PushBack(candidate.CreateReplacement(ScalarNode, "!!null", "null")) } return context.ChildContext(results), nil } ``` Unlike `DisableFileOps` and `DisableEnvOps`, which cleanly return an error when a restricted operation is attempted, `systemOperator` logs a warning, evaluates the node to `null`, and allows the expression to continue processing. This inconsistency can silently corrupt data by injecting `null` into configurations instead of halting the deployment or script pipeline. Fix: Return an error to fail execution, matching existing security controls. Update `operator_system_test.go` to expect an error instead of `null` matches. ```go if !ConfiguredSecurityPreferences.EnableSystemOps { return Context{}, fmt.Errorf("system operations have been disabled, use --security-enable-system-operator flag to enable") } ``` I3. **Argument resolution silently ignores invalid node types** — `pkg/yqlib/operator_system.go:23-26` (important, regression) ```go if child.Kind != ScalarNode || child.Tag == "!!null" { log.Warningf("system operator: argument must be a non-null scalar; got kind=%v tag=%v - ignoring", child.Kind, child.Tag) continue } ``` If the user passes an array with an invalid node (e.g., an object or map) or passes a non-scalar directly, `resolveSystemArgs` ignores it and continues. Silently dropping command arguments alters the command's behavior and can lead to dangerous execution paths (e.g. executing `rm ["-rf", {bad: node}]` resolves to `rm -rf`). Fix: Change `resolveSystemArgs` to return an error and halt execution. Update callers to bubble up the error. ```go func resolveSystemArgs(argsNode *CandidateNode) ([]string, error) { // ... if child.Kind != ScalarNode || child.Tag == "!!null" { return nil, fmt.Errorf("system operator: argument must be a non-null scalar; got kind=%v tag=%v", child.Kind, child.Tag) } // ... ``` I4. **Test documentation generation relies on local absolute paths** — `pkg/yqlib/operator_system_test.go:44-60` (important, regression) ```go echoPath := findExec(t, "echo") // ... expression: `.country = system("` + echoPath + `"; "test")`, ``` The test uses `findExec` to dynamically resolve the absolute path of `echo` (e.g., `/usr/bin/echo` or `/bin/echo`), and embeds it into the documentation scenario. This causes the generated markdown files to differ depending on the host machine's environment, creating flaky git diffs and CI check failures during `make generate-docs`. This is likely why the multiple-arguments scenario skipped documentation generation. Fix: Use a literal `"echo"` for the expression and avoid `findExec` for doc-generated scenarios. Since `echo` is standard, `exec.Command("echo")` will locate it via `$PATH` automatically. I5. **Command stderr is silently discarded on success** — `pkg/yqlib/operator_system.go:132-138` (important, gap) ```go output, err := cmd.Output() if err != nil { stderrStr := strings.TrimSpace(stderr.String()) ``` If `cmd.Output()` succeeds (`err == nil`), the contents of the `stderr` buffer are completely discarded. Commands often write critical warnings or diagnostic messages to stderr while returning a `0` exit code. Discarding stderr suppresses these messages from operators. Fix: Forward or log non-empty `stderr` even when the command succeeds. ```go output, err := cmd.Output() stderrStr := strings.TrimSpace(stderr.String()) if err != nil { if stderrStr != "" { return Context{}, fmt.Errorf("system command '%v' failed: %w\nstderr: %v", command, err, stderrStr) } return Context{}, fmt.Errorf("system command '%v' failed: %w", command, err) } else if stderrStr != "" { log.Warningf("system command '%v' stderr: %v", command, stderrStr) } ``` ### Suggestions S1. **Trailing newlines stripped from command input** — `pkg/yqlib/operator_system.go:116` (suggestion, gap) `encodeToYamlString(candidate)` applies a global regex chomper to strip *all* trailing newlines from the string before writing it to `cmd.Stdin`. While mostly harmless, if the command requires a trailing newline (e.g. `wc -l` counting lines correctly), this behavior may surprise operators. Consider preserving a trailing newline on `stdin` or adding a code comment documenting the stripping. S2. **Union arguments silently drop extra matches** — `pkg/yqlib/operator_system.go:101-103` (suggestion, gap) ```go if argsNodes.MatchingNodes.Front() != nil { args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode)) } ``` If a user incorrectly uses the union operator for arguments (`system("cmd"; "arg1", "arg2")`), `argsNodes` will yield multiple nodes. Currently, it processes the `.Front()` node (`"arg1"`) and silently drops the rest. Similar to how `resolveCommandNode` emits a debug log when `Len() > 1`, consider logging a debug message here to assist users debugging dropped arguments. ### Design Observations **Concerns:** - `(in-scope)` The fallback to `null` on a disabled security preference creates invisible failure states. **Strengths:** - `(in-scope)` Properly evaluating the block per `nodeContext` makes it simple to execute different commands or arguments dynamically from individual elements. - `(in-scope)` Opt-in security flag effectively prevents injection out of the box. ### Testing Assessment Test coverage is effective at asserting both positive runs and `null` commands. Untested scenarios: 1. Providing an invalid object or map node to the arguments sequence (related to I3). 2. Union nodes in the arguments expression silently falling back to the first element (related to S2). ### Documentation Assessment The documentation explicitly and incorrectly instructs users to delimit arguments with `;` which acts as a sequential expression execution in `yq`, causing the parser to silently trim preceding arguments. ### Coverage Summary - `cmd/root.go` — Reviewed, no issues - `pkg/yqlib/doc/operators/headers/system-operators.md` — I1 - `pkg/yqlib/doc/operators/system-operators.md` — Reviewed, no issues (generated) - `pkg/yqlib/lexer_participle.go` — Trivial - `pkg/yqlib/operation.go` — Trivial - `pkg/yqlib/operator_system.go` — I2, I3, I5, S1, S2 - `pkg/yqlib/operator_system_test.go` — I4 - `pkg/yqlib/operators_test.go` — Reviewed, no issues - `pkg/yqlib/security_prefs.go` — Reviewed, no issues ### [Gemini] — Pass 2 sylv sylv ### Executive Summary This PR introduces a new `system` operator to execute external shell commands, piping the current matched node's YAML representation to the command's `stdin` and replacing the node with the command's `stdout`. The second-pass review identified memory allocation inefficiencies when buffering the input payload and important gaps in error reporting, specifically around the complete loss of command `stdout` context upon failure and ungraceful handling of empty evaluation nodes. ### Critical Issues None. ### Important Issues I1. **Unnecessary double memory allocation for stdin piping** — `pkg/yqlib/operator_system.go:115-121` `(important, enhancement)` ```go var stdin bytes.Buffer encoded, err := encodeToYamlString(candidate) if err != nil { return Context{}, err } stdin.WriteString(encoded) ``` `encodeToYamlString` allocates memory proportional to the entire YAML representation of the `candidate` node. Writing this string to a `bytes.Buffer` immediately allocates and copies the entire string a second time. If the operator processes large documents, this needlessly doubles the memory footprint of the operation before the child process is even started. Fix: Use `strings.NewReader` to create a lightweight `io.Reader` over the existing string allocation instead of duplicating it. ```diff - var stdin bytes.Buffer encoded, err := encodeToYamlString(candidate) if err != nil { return Context{}, err } - stdin.WriteString(encoded) // #nosec G204 - intentional: user must explicitly enable this operator cmd := exec.Command(command, args...) - cmd.Stdin = &stdin + cmd.Stdin = strings.NewReader(encoded) ``` I2. **stdout completely discarded on command failure** — `pkg/yqlib/operator_system.go:132-138` `(important, gap)` ```go output, err := cmd.Output() if err != nil { stderrStr := strings.TrimSpace(stderr.String()) if stderrStr != "" { return Context{}, fmt.Errorf("system command '%v' failed: %w\nstderr: %v", command, err, stderrStr) } return Context{}, fmt.Errorf("system command '%v' failed: %w", command, err) } ``` When a command exits with a non-zero status code, `cmd.Output()` returns a non-nil error, but the `output` variable still contains any data written to standard output before the failure occurred. This output is entirely discarded. Many command-line tools emit valuable context or failure reasons to `stdout`, and hiding it makes diagnosing failing `system()` calls exceedingly difficult for users. Fix: Capture and append the partial `stdout` to the error message when available. ```diff output, err := cmd.Output() if err != nil { stderrStr := strings.TrimSpace(stderr.String()) + stdoutStr := strings.TrimSpace(string(output)) + errMsg := fmt.Sprintf("system command '%v' failed: %w", command, err) + if stdoutStr != "" { + errMsg += fmt.Sprintf("\nstdout: %v", stdoutStr) + } if stderrStr != "" { - return Context{}, fmt.Errorf("system command '%v' failed: %w\nstderr: %v", command, err, stderrStr) + errMsg += fmt.Sprintf("\nstderr: %v", stderrStr) } - return Context{}, fmt.Errorf("system command '%v' failed: %w", command, err) + return Context{}, fmt.Errorf("%s", errMsg) } ``` I3. **Missing command node aborts evaluation completely** — `pkg/yqlib/operator_system.go:43-45` `(important, gap)` ```go func resolveCommandNode(commandNodes Context) (string, error) { if commandNodes.MatchingNodes.Front() == nil { return "", fmt.Errorf("system operator: command expression returned no results") } ``` If the user attempts to execute `.a = system(.non_existent_command)`, `resolveCommandNode` returns an error that halts the entire `yq` evaluation immediately. `yq` operators that map values typically handle missing contextual nodes gracefully by skipping the assignment (returning an empty context list) rather than failing the pipeline. Fix: Check for missing context prior to resolving the node, and return an empty context instead of an error so the pipeline skips missing nodes gracefully. ```diff commandNodes, err := d.GetMatchingNodes(nodeContext, block.LHS) if err != nil { return Context{}, err } + if commandNodes.MatchingNodes.Front() == nil { + return Context{}, nil + } command, err = resolveCommandNode(commandNodes) ``` ### Suggestions S1. **Include command arguments in error messages** — `pkg/yqlib/operator_system.go:134-137` `(suggestion, enhancement)` ```go return Context{}, fmt.Errorf("system command '%v' failed: %w\nstderr: %v", command, err, stderrStr) ``` When an expression maps an array of dynamic arguments into `system()`, an error message only displaying the executable `command` makes it impossible to distinguish which specific invocation failed. Fix: Inject the arguments into the error string to improve operator observability. ```go return Context{}, fmt.Errorf("system command '%v %v' failed: %w", command, args, err) ``` S2. **Non-zero exit codes halt evaluation entirely** — `pkg/yqlib/operator_system.go:133-139` `(suggestion, gap)` Because `yq` has no `try/catch` mechanism, a hard-failure on a non-zero exit code limits the utility of `system()` for commands that legitimately exit `1` to indicate a negative boolean result (e.g. `grep` finding no matches). Consider adding a `--security-allow-system-failures` preference or documenting this "fail-fast" behavior prominently in the system operator docs. ### Design Observations **Strengths** - `(in-scope)` Relying on Go's `exec.Command` ensures argument values are passed cleanly via arrays without being evaluated by a shell, drastically reducing command-injection risk on properly structured `system("cmd"; ["arg1"])` expressions. ### Testing Assessment The test coverage covers success, failure, and security gating, but lacks these critical paths: 1. **Partial stdout on failure**: An external command exits `1` but successfully writes diagnostic output to `stdout` first. 2. **Missing dynamic contexts**: The behavior of `.a = system(.b)` when `.b` does not exist in the document. 3. **Empty sequences**: Using `system("echo"; [])` to ensure empty array evaluations map to zero variadic arguments reliably. ### Documentation Assessment The documentation effectively covers the opt-in flag. However, it should explicitly disclose that any non-zero exit code from the target command will act as an uncatchable exception, halting `yq` script execution completely. ### Commit Structure The commits are atomic, clear, and accurately summarize the progression of code review changes. Clean. ### Acknowledged Limitations - None explicitly marked with `TODO` or internal code comments. ### Unresolved Feedback None. Subsequent commits address all referenced PR feedback. ### Coverage Summary - `cmd/root.go` — Reviewed, no issues - `pkg/yqlib/doc/operators/headers/system-operators.md` — Reviewed, no issues - `pkg/yqlib/doc/operators/system-operators.md` — Reviewed, no issues - `pkg/yqlib/lexer_participle.go` — Reviewed, no issues - `pkg/yqlib/operation.go` — Reviewed, no issues - `pkg/yqlib/operator_system.go` — I1, I2, I3, S1, S2 - `pkg/yqlib/operator_system_test.go` — Reviewed, no issues - `pkg/yqlib/operators_test.go` — Trivial - `pkg/yqlib/security_prefs.go` — Trivial