Add 'run_command_and_assert' assertion - #401
Merged
Merged
Conversation
Youssef1313
force-pushed
the
dev/ygerges/command-assertion
branch
from
March 18, 2026 11:16
3728c9d to
41a8e28
Compare
JanKrivanek
reviewed
Mar 18, 2026
JanKrivanek
left a comment
JanKrivanek
left a comment
Member
There was a problem hiding this comment.
I like the proposal!
Let's add tests. And possibly guard over runaway command (timeout). Then it should be good to take
JanKrivanek
reviewed
Mar 20, 2026
JanKrivanek
reviewed
Mar 20, 2026
Member
|
Addressed all three review comments in e5bca1a:
New tests added for regex matching and timeout configuration. All 412 tests pass. |
JanKrivanek
approved these changes
Mar 20, 2026
JanKrivanek
marked this pull request as ready for review
March 20, 2026 17:15
JanKrivanek
enabled auto-merge (squash)
March 20, 2026 17:15
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new run_command_and_assert assertion to skill-validator eval scenarios, enabling evaluation runs to execute a command in the scenario work directory and assert on exit code/stdout/stderr.
Changes:
- Introduces
AssertionType.RunCommandAndAssertplusCommandAssertionArgsto represent command-based assertions. - Extends
EvalSchemaYAML parsing/validation to recognize and populaterun_command_and_assert. - Implements runtime evaluation in
AssertionEvaluatorand wires scenario timeouts throughEvaluateCommand. - Adds unit tests covering the evaluator behavior for the new assertion type.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/skill-validator/tests/AssertionsTests.cs | Adds unit tests validating run_command_and_assert evaluation behavior (exit code, stdout/stderr contains/regex, timeout). |
| eng/skill-validator/src/Services/EvalSchema.cs | Parses/validates run_command_and_assert and maps YAML fields into CommandAssertionArgs. |
| eng/skill-validator/src/Services/AssertionEvaluator.cs | Executes the configured command and performs assertions against exit code/stdout/stderr, with timeouts. |
| eng/skill-validator/src/Models/Models.cs | Adds the new assertion type and models for command execution arguments. |
| eng/skill-validator/src/Commands/EvaluateCommand.cs | Passes scenario timeout into assertion evaluation so command assertions can inherit scenario timeout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JanKrivanek
force-pushed
the
dev/ygerges/command-assertion
branch
from
March 20, 2026 17:24
3acbfc5 to
e5bca1a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
eng/skill-validator/src/Services/AssertionEvaluator.cs:263
Process.Start(processStartInfo)can throw (e.g., file not found) or return null. Right now this will crash the assertion evaluation via the null-forgivingprocess!. Consider wrapping Start + wiring in try/catch and returning a failedAssertionResultwith the exception message, and ensure theProcessis disposed (e.g.,using).
var process = Process.Start(processStartInfo);
var outBuilder = new StringBuilder();
var errBuilder = new StringBuilder();
process!.OutputDataReceived += (_, e) =>
{
eng/skill-validator/src/Services/AssertionEvaluator.cs:278
- On timeout,
process.Kill()only targets the parent process. For common patterns likecmd /c ...or/bin/sh -c ..., this can leave child processes (e.g.,sleep,ping) running and leaking across test runs/CI. Consider killing the entire process tree (Kill(true)), handling race/exception cases, and waiting for exit after the kill.
if (!process.WaitForExit(TimeSpan.FromSeconds(timeoutSeconds)))
{
process.Kill();
return new AssertionResult(a, false, $"Command timed out after {timeoutSeconds}s");
eng/skill-validator/src/Services/AssertionEvaluator.cs:256
- Child processes inherit the current environment by default. Since this assertion can run arbitrary commands, it should scrub sensitive env vars (tokens, Actions runtime vars, etc.) the same way setup commands do via
AgentRunner.ScrubSensitiveEnvironment(ProcessStartInfo). Without this, a command can accidentally print/exfiltrate secrets from the evaluator environment.
var processStartInfo = new ProcessStartInfo(command, cmd.CommandArguments ?? string.Empty)
{
WorkingDirectory = workDir,
RedirectStandardOutput = true,
RedirectStandardError = true,
UseShellExecute = false,
};
eng/skill-validator/src/Services/AssertionEvaluator.cs:276
timeoutSecondscan be 0 or negative (from either the assertion orscenarioTimeoutSeconds).TimeSpan.FromSeconds(timeoutSeconds)+WaitForExit(TimeSpan)will throw for negative values. Add validation/clamping (e.g., require > 0, or treat non-positive as immediate timeout) to avoid crashing assertion evaluation on bad config.
var timeoutSeconds = cmd.Timeout ?? scenarioTimeoutSeconds;
var processStartInfo = new ProcessStartInfo(command, cmd.CommandArguments ?? string.Empty)
{
WorkingDirectory = workDir,
RedirectStandardOutput = true,
RedirectStandardError = true,
UseShellExecute = false,
};
var process = Process.Start(processStartInfo);
var outBuilder = new StringBuilder();
var errBuilder = new StringBuilder();
process!.OutputDataReceived += (_, e) =>
{
if (e.Data is not null)
outBuilder.AppendLine(e.Data);
};
process.ErrorDataReceived += (_, e) =>
{
if (e.Data is not null)
errBuilder.AppendLine(e.Data);
};
process.BeginOutputReadLine();
process.BeginErrorReadLine();
if (!process.WaitForExit(TimeSpan.FromSeconds(timeoutSeconds)))
{
eng/skill-validator/src/Services/AssertionEvaluator.cs:298
- These
Containschecks are currently case-sensitive (StringComparison.Ordinal), while the existingoutput_containsassertion is case-insensitive (OrdinalIgnoreCase). Consider aligning semantics (or documenting the difference) so assertion behavior is consistent across output-based assertions.
if (cmd.ExpectedStdOutContains is not null)
{
if (!actualStdOut.Contains(cmd.ExpectedStdOutContains, StringComparison.Ordinal))
{
return new AssertionResult(a, false, $"Command stdout did not contain expected value. Stdout: {actualStdOut}");
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WaitForExit(TimeSpan) does not guarantee that async output event handlers have completed. Add parameterless WaitForExit() call after the timed wait to ensure stdout/stderr are fully captured before checking their contents. Also handle the timeout case explicitly by killing the process.
…configurable timeout - Extract command-specific fields from Assertion into a dedicated CommandAssertionArgs record type for cleaner separation of concerns - Add ExpectedStdOutMatches/ExpectedStdErrorMatches for regex-based stdout/stderr assertions (matches type asserts) - Make command timeout configurable via assertion-level 'command_timeout' field, falling back to the scenario timeout when not specified - Show actual timeout value in timeout error messages - Update EvalSchema parsing, EvaluateCommand call sites, and all tests - Add new tests for regex matching and timeout configuration
- Convert EvalRunCommandAndAssert to async using WaitForExitAsync with CancellationTokenSource for timeout instead of blocking thread - Wrap Process.Start in try/catch, handle null return with clear failure message including command and args - Dispose Process with 'using', use Kill(entireProcessTree: true) on timeout to avoid leaking child processes - Truncate stdout/stderr in failure messages to 4KB to avoid log bloat - Validate timeout > 0 before use, return clear error for bad config - Fix validation error message to show correct YAML field names (expected_std_output_contains/expected_std_error_contains) - Add EvalSchema parsing tests for run_command_and_assert (valid parse, missing command_to_run, missing expected checks) - Use 'timeout /t' instead of 'ping' for Windows sleep in tests
JanKrivanek
force-pushed
the
dev/ygerges/command-assertion
branch
from
March 20, 2026 18:07
bca8a18 to
9f34b43
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
eng/skill-validator/src/Services/AssertionEvaluator.cs:303
outBuilder/errBuilderareStringBuilders mutated fromOutputDataReceived/ErrorDataReceivedcallbacks, which can run on background threads. AccessingStringBuilderconcurrently (and then callingToString()afterWaitForExitAsync) is not thread-safe and can lead to corrupted output or intermittent exceptions, plus you don't currently wait for async output reads to fully drain after process exit. Prefer readingStandardOutput/StandardErrorviaReadToEndAsynctasks (or protect the builders with a lock + wait for stream completion) so capture is deterministic and thread-safe.
var outBuilder = new StringBuilder();
var errBuilder = new StringBuilder();
process.OutputDataReceived += (_, e) =>
{
if (e.Data is not null)
outBuilder.AppendLine(e.Data);
};
process.ErrorDataReceived += (_, e) =>
{
if (e.Data is not null)
errBuilder.AppendLine(e.Data);
};
process.BeginOutputReadLine();
process.BeginErrorReadLine();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Treat null/whitespace expected_std_* values as 'not specified' when
building CommandAssertionArgs, preventing empty strings from becoming
accidental always-pass assertions (Contains('') and IsMatch('') always
succeed).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ains - Call AgentRunner.ScrubSensitiveEnvironment before Process.Start to prevent leaking CI/credential env vars to child processes - Replace BeginOutputReadLine/BeginErrorReadLine with ReadToEndAsync to reliably drain stdout/stderr before evaluating assertions - Change StringComparison.Ordinal to OrdinalIgnoreCase for contains checks, consistent with output_contains and regex assertions
Member
|
/evaluate |
Contributor
Skill Validation Results
[1] (Plugin) Quality unchanged but weighted score is -8.4% due to: tokens (23753 → 49906), time (12.1s → 22.4s), tool calls (2 → 3)
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I find the current assertions very limiting, at least for my scenarios. I want to be able to run
dotnet buildand assert that it's building successfully, or rundotnet testand assert some output. This might even allow to run an arbitrary assertion script that can be very custom if a complex scenario needs that.TODO: