Add system(command; args) operator (disabled by default)#2640
Add system(command; args) operator (disabled by default)#2640
Conversation
Agent-Logs-Url: https://github.com/mikefarah/yq/sessions/8a11e9a0-10d2-4f2a-ae29-4e9d0bfc266f Co-authored-by: mikefarah <1151925+mikefarah@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new system(...) operator to yqlib that can execute external commands (disabled by default) and wires it through parsing, CLI configuration, tests, and operator documentation.
Changes:
- Introduces
SecurityPreferences.EnableSystemOps(defaultfalse) and a--enable-system-operatorCLI flag to opt in. - Implements
systemoperator parsing/registration and execution logic (stdin = current node YAML, stdout -> string). - Adds operator scenarios + generated documentation pages/headers.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/yqlib/security_prefs.go | Adds security preference toggle for system operator. |
| pkg/yqlib/operator_system.go | Implements system operator execution and result mapping. |
| pkg/yqlib/operation.go | Registers SYSTEM operation type. |
| pkg/yqlib/lexer_participle.go | Adds system to the lexer/operator table. |
| cmd/root.go | Adds --enable-system-operator persistent flag. |
| pkg/yqlib/operator_system_test.go | Adds scenarios for disabled/enabled behavior and failures. |
| pkg/yqlib/doc/operators/system-operators.md | Adds generated operator docs with examples/output. |
| pkg/yqlib/doc/operators/headers/system-operators.md | Adds doc header snippet included by generator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/yqlib/operator_system.go
Outdated
| var command string | ||
| var argsExpression *ExpressionNode | ||
|
|
||
| // check if it's a block operator (command; args) or just (command) | ||
| if expressionNode.RHS.Operation.OperationType == blockOpType { |
There was a problem hiding this comment.
command/argsExpression are resolved once using context.ReadOnlyClone() before iterating over context.MatchingNodes, and only the first match is used. This means system(<expr>) and system(<expr>; <expr>) cannot vary per matched node (e.g. system(.cmd; .args)), and may pick an unrelated first result when multiple nodes are matched. Consider evaluating the command/args expressions inside the per-candidate loop using context.SingleReadonlyChildContext(candidate) (and failing if they don't yield a single scalar/sequence) so parameter expressions behave consistently with other operators like load.
There was a problem hiding this comment.
Done in da611f7. Command and args expressions are now evaluated inside the per-candidate loop using context.SingleReadonlyChildContext(candidate), matching the pattern used by loadOperator. This allows expressions like system(.cmd; .arg) to resolve .cmd and .arg against each matched node individually. Also added a test case for per-node evaluation, used exec.LookPath for portability, and fixed CRLF-aware newline trimming.
pkg/yqlib/operator_system_test.go
Outdated
| subdescription: "Use `--enable-system-operator` to enable the system operator.", | ||
| document: "country: Australia", | ||
| expression: `.country = system("/usr/bin/echo"; "test")`, | ||
| expected: []string{ | ||
| "D0, P[], (!!map)::country: null\n", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| var systemOperatorEnabledScenarios = []expressionScenario{ | ||
| { | ||
| description: "Run a command with an argument", | ||
| subdescription: "Use `--enable-system-operator` to enable the system operator.", | ||
| document: "country: Australia", | ||
| expression: `.country = system("/usr/bin/echo"; "test")`, | ||
| expected: []string{ |
There was a problem hiding this comment.
These tests depend on hard-coded absolute paths like /usr/bin/echo and /bin/echo, which can fail on some Linux distros/containers and all non-POSIX platforms. Consider resolving the executable at runtime (e.g. via exec.LookPath) or skipping these scenarios when the required commands aren't available, to keep the test suite portable and reliable.
| type SecurityPreferences struct { | ||
| DisableEnvOps bool | ||
| DisableFileOps bool | ||
| DisableEnvOps bool | ||
| DisableFileOps bool | ||
| EnableSystemOps bool | ||
| } |
There was a problem hiding this comment.
SecurityPreferences currently uses a Disable* naming pattern (DisableEnvOps, DisableFileOps), but this introduces EnableSystemOps. For consistency and to reduce confusion when adding future prefs, consider renaming so all fields use the same polarity/pattern (e.g. DisableSystemOps with an inverted default, or rename the existing fields).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ldContext Agent-Logs-Url: https://github.com/mikefarah/yq/sessions/dca841eb-3f63-4f23-adeb-556431560420 Co-authored-by: mikefarah <1151925+mikefarah@users.noreply.github.com>
…m op docs Agent-Logs-Url: https://github.com/mikefarah/yq/sessions/3f8a5375-25fd-4428-a8e6-b630194c36b2 Co-authored-by: mikefarah <1151925+mikefarah@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/yqlib/operator_system.go
Outdated
| if commandNodes.MatchingNodes.Front() == nil { | ||
| return Context{}, fmt.Errorf("system operator: command expression returned no results") | ||
| } | ||
| command = commandNodes.MatchingNodes.Front().Value.(*CandidateNode).Value | ||
|
|
||
| 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)) | ||
| } | ||
| } else { | ||
| commandNodes, err := d.GetMatchingNodes(nodeContext, expressionNode.RHS) | ||
| if err != nil { | ||
| return Context{}, err | ||
| } | ||
| if commandNodes.MatchingNodes.Front() == nil { | ||
| return Context{}, fmt.Errorf("system operator: command expression returned no results") | ||
| } | ||
| command = commandNodes.MatchingNodes.Front().Value.(*CandidateNode).Value | ||
| } |
There was a problem hiding this comment.
command is taken from the first matched node’s .Value without validating type/tag. If the command expression yields !!null or a non-scalar node, exec.Command will be invoked with an empty/invalid command (leading to confusing errors like “exec: no command”). Consider validating that the command node is a non-null scalar (and possibly erroring if multiple results are returned) and returning a clearer system operator: command must be a string scalar error.
There was a problem hiding this comment.
@copilot apply changes based on this feedback If there are multiple results, just use the first (and log a debug, not a warning, of all the matches).
There was a problem hiding this comment.
Done in 53abbba. Extracted a resolveCommandNode helper that validates the command is a non-null scalar (returning "system operator: command must be a string scalar" if not) and logs a Debugf when multiple results are returned before using the first.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/mikefarah/yq/sessions/928aabc5-ad71-41d8-94ab-403942e3f92d Co-authored-by: mikefarah <1151925+mikefarah@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result := string(output) | ||
| if strings.HasSuffix(result, "\r\n") { | ||
| result = result[:len(result)-2] | ||
| } else if strings.HasSuffix(result, "\n") { | ||
| result = result[:len(result)-1] | ||
| } |
There was a problem hiding this comment.
The PR description says stdout is "trimmed", but the implementation only strips a single trailing \n/\r\n. If the intent is to trim all trailing whitespace/newlines, use strings.TrimRight/TrimSpace; otherwise update the description/docs to match the current behavior ("strip one trailing newline").
| func TestSystemOperatorEnabledScenarios(t *testing.T) { | ||
| echoPath := findExec(t, "echo") | ||
| falsePath := findExec(t, "false") | ||
|
|
||
| originalEnableSystemOps := ConfiguredSecurityPreferences.EnableSystemOps | ||
| defer func() { | ||
| ConfiguredSecurityPreferences.EnableSystemOps = originalEnableSystemOps | ||
| }() | ||
|
|
||
| ConfiguredSecurityPreferences.EnableSystemOps = true | ||
|
|
||
| scenarios := []expressionScenario{ | ||
| { | ||
| description: "Run a command with an argument", | ||
| subdescription: "Use `--enable-system-operator` to enable the system operator.", | ||
| yqFlags: "--enable-system-operator", | ||
| document: "country: Australia", | ||
| expression: `.country = system("` + echoPath + `"; "test")`, | ||
| expected: []string{ |
There was a problem hiding this comment.
The doc-generating enabled scenarios embed echoPath from exec.LookPath, which can vary across environments (e.g. /usr/bin/echo vs /bin/echo) and cause non-deterministic generated docs. Consider hardcoding a stable command path/name for scenarios that are included in docs (or marking these scenarios skipDoc: true and adding a separate deterministic doc scenario).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
cmd/root.go
Outdated
| rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.DisableEnvOps, "security-disable-env-ops", "", false, "Disable env related operations.") | ||
| rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.DisableFileOps, "security-disable-file-ops", "", false, "Disable file related operations (e.g. load)") | ||
| 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, "enable-system-operator", "", false, "DEPRECATED: use --security-enable-system-operator instead. Enable system operator to allow execution of external commands.") |
There was a problem hiding this comment.
@copilot no need to add enable-system-operator in this PR only to deprecate it. Also MarkPersistentFlagDeprecated doesn't exist anyway
There was a problem hiding this comment.
Done in b3b4478. Removed the deprecated --enable-system-operator alias and the MarkPersistentFlagDeprecated call entirely. The flag is now only --security-enable-system-operator, consistent with --security-disable-env-ops and --security-disable-file-ops. All references in the operator warning message, test yqFlags, and docs are updated accordingly.
…le-system-operator consistently Agent-Logs-Url: https://github.com/mikefarah/yq/sessions/286b95e9-b6d7-4ab8-b401-2d7a03853922 Co-authored-by: mikefarah <1151925+mikefarah@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@jandubois - I don't suppose you could run your deep review tool against this PR as well? That report was really neat..what are you using? Is this something you made? |
I did run it: https://jandubois.github.io/yq/20260406-103045-pr-2640.html There is also a Markdown version at https://jandubois.github.io/yq/20260406-103045-pr-2640.md if you want to copy some of it into a comment here, to feed back to Copilot. Be aware that the Markdown version includes the individual agent responses, so is quite verbose; you don't want to copy all of it. The HTML version is post-processed and has added code blocks and some other minor things. Since this was another multi-pass review I now ran out of quota for Gemini for today. Unfortunately I only have a limited subscription for it and it looks like running 2 multi-pass reviews use up all of it. 😢 I can still run the tool, but have to run it with just Claude and Codex until 11pm...
Yes, it is my own tool running on "Zilicon Island", my off-leash sandbox (like Jurassic Park, except there are roaming AI agents instead of dinosaurs). It has a lead agent preparing prompts for review agents using the top reasoning models, and then combining their findings. In a It takes a bit of time (you can see this latest review ran for 90 minutes wall time). It also generates metrics and feedback that I use to continuously improve it. You can see all reviews I ran against the You can see that I ran the tool (single-pass only) against my oss-fuzz branches. It found issues in each of them, that I fixed before creating the PRs. Those PRs were also all made with AI, but of course with some steering. PS: Just realized that some of the code blocks have off-by-one errors in the line numbers and highlighted lines. So added a script to the tool to verify the code blocks, so the agent can check and fix it by themselves for future reviews. This is perpetual work-in-progress... |
|
Regarding I6. system operator subsumes env/file security restrictions I think it would be best not just to add the section to the docs, but to actually make |
Adds a
systemoperator that executes an external command and returns its stdout as a string value. Disabled by default for security — requires explicit opt-in via--security-enable-system-operator.Behaviour
nullfor each matched node!!strChanges
security_prefs.go— addsEnableSystemOps bool(defaultfalse)operator_system.go— operator implementation; supportssystem("cmd"),system("cmd"; "arg"), andsystem("cmd"; ["arg1", "arg2"]); captures stderr in error messages; validates command is a non-null, non-empty scalar; evaluates command/args per matched node usingSingleReadonlyChildContextoperation.go— registerssystemOpTypelexer_participle.go— registerssimpleOp("system", systemOpType)cmd/root.go— adds--security-enable-system-operatorpersistent flag (consistent with--security-disable-env-ops/--security-disable-file-ops)operator_system_test.go— covers disabled/enabled states, array args, per-node expression evaluation, null command error, and command failure; usesexec.LookPathfor portabilitydoc/operators/headers/system-operators.md— documentation header📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.