You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Add an installable unskip-closed-tests agentic-workflow package with a deterministic Roslyn producer, source/blob/span/declaration identities, canonical GitHub issue/PR eligibility, and a read-only agent planner.
Gate every edit through a custom safe-output job that revalidates source and remote state, invokes a repository-owned verification hook, parses exact TRX FQN/outcome evidence, and opens at most one draft PR.
Add behavioral regressions, deterministic local package staging, hostile-consumer restore/build/compile validation, collection catalog entries, and consumer contract documentation.
go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 -shellcheck= -pyflakes= .github/workflows/agentic-workflow-validation.yml — passed.
Concurrent microsoft/testfx consumer validation — locked staged-tool restore/build passed with 0 warnings/errors; strict compile passed; live issue resolution classified closed/not-planned and open/null references as ineligible; the configured hook executed the exact intended FQN and produced TRX evidence with total=1, passed=1, skipped=0.
Checklist
I searched existing issues and pull requests to avoid duplicates.
I kept this pull request focused and avoided unrelated refactors.
I added or updated tests, evals, or documentation when changing skill or agent behavior.
I updated CODEOWNERS when adding or moving owned content.
I updated all marketplace manifests when plugin metadata changed.
I updated eng/known-domains.txt for any new external domains referenced by skill content.
👋 @Evangelink — this PR has 9 unresolved review thread(s). When you're ready, please address the feedback and push an update; the triage bot will pick up the next state automatically. (Add the no-stale label to silence further pings.)
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Package staging can follow a symlinked grader directory outside the consumer root, and the documented exit-code contract is inconsistent with implementation.
This exit-code contract does not match the implementation: a hook nonzero exit, timeout, or malformed/missing TRX becomes a failed VerificationOutcome; when no candidate survives, apply returns code 10, not 30. Reserve 30 for failures that throw InfrastructureException (such as being unable to start the hook), or change the implementation if consumers should distinguish protocol failures.
Correct CLI help for verification failure exit code
The CLI help also assigns verification protocol failures to exit 30, but malformed/missing TRX, hook timeouts, and hook nonzero exits are converted into candidate reverts and yield exit 10 when nothing remains. Update this text to match the implemented contract (or change the return path consistently).
These read-back checks run under set -e, so any draft/head/base/body mismatch aborts the step but leaves the just-created PR and branch published. That defeats the fail-closed verification these checks are meant to enforce. Handle all invariant failures like the baseRefOid mismatch above: close the PR and delete its branch before exiting.
Reject symlinked parent directories in package includes
This only rejects a symlink at the final include path. A symlinked parent such as workflows -> real-workflows makes workflows/example.md.is_symlink() false, and resolve_package_include accepts it when the target remains inside the package, so staging still copies through mutable symlink indirection. Walk and reject every path component between the package root and include before resolving it.
Check unresolved consumer destinations before symlink resolution
relative_destination came from staged_destination, which resolve_package_include has already resolved. If the intended consumer file is a symlink to another path inside the consumer, this check examines the target rather than the symlink and silently stages to that target. Preserve/check the unresolved install destination (including parent components) before resolution so consumer symlinks are actually refused.
Align verification failure exit behavior with documented contract
The documented exit contract does not match the implementation: a nonzero verification hook, missing/malformed TRX, or non-passing outcome becomes a candidate rejection, and apply returns 10 when no candidate remains; it does not return 30. The CLI help repeats the same mismatch. Clarify that candidate-level hook/TRX rejection is a clean no-op, or change the implementation and workflow handling if protocol failures should fail the run.
Count retained tests rather than Ignore-attribute candidates
retained.Count counts Ignore-attribute candidates, not tests. A supported class-level candidate can contain multiple Owner.TestFqns, so one retained class currently produces the singular “Unskip test” title and multiple classes undercount the tests. Derive the count from the retained FQNs (or describe these as sites/candidates) and cover a class-level retained candidate.
These methods come from a syntax tree parsed with no preprocessor symbols. For a class-level ignore, a test declared inside #if CUSTOM is therefore absent here; if the trusted hook builds with CUSTOM, removing the class ignore enables that test while the request/TRX verifies only the methods visible to this parse. Defer class candidates containing conditional directives, or enumerate with the same project parse symbols used by verification.
MSTest data-driven tests can emit several UnitTestResult elements with the same method-level testId—one per data row. This check rejects the second row even when every row passed, so the configured DataTestMethodAttribute support can never authorize a multi-row test. Allow multiple results for a mapped ID while requiring every result to pass and every definition to have at least one result; use executionId/row identity if duplicate-result detection is still required.
Rerun validation commands against the current test suite
The PR validation says this command passed 10/10 tests, but the current file now contains 17 test methods (including this newly added staging suite). The behavioral suite likewise contains 36 methods while the description reports 22/22. Those counts show the cited runs predate the current head; rerun the listed validation commands and update the PR validation results so the added regressions are actually covered.
Source aliases retain a leading global::, but this normalizer never removes it. As a result, valid aliases such as using MSTest = global::Microsoft.VisualStudio.TestTools.UnitTesting; make [MSTest.Ignore] resolve to a nonmatching global::...IgnoreAttribute string and the candidate is silently missed. Normalize the global alias prefix just as the direct-attribute path already does.
An existing directory or other non-regular entry at a destination reaches read_bytes() here, which raises an uncaught IsADirectoryError/OSError instead of the documented conflict RuntimeError (and a FIFO can block). Reject non-files before reading so staging fails deterministically and main() can report the normal concise error.
Correct exit semantics for TRX evidence and authorize
The table conflicts with the behavior documented above and covered by the regression suite: malformed, missing, skipped, or mismatched TRX evidence reverts candidates and yields exit 10 when none remain, not exit 30. It also describes only apply, although authorize uses the same 0/10 result semantics.
Update CLI exit descriptions for authorize and materialize
These exit descriptions omit the new authorize and materialize commands, and classify verification-protocol rejection as exit 30 even though malformed/missing/non-passing TRX evidence is deliberately converted to a clean no-op (exit 10). Align the CLI help with the implemented and tested semantics.
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
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.
github-actions
Bot
added
pr-state/ready-for-eval
Summary
unskip-closed-testsagentic-workflow package with a deterministic Roslyn producer, source/blob/span/declaration identities, canonical GitHub issue/PR eligibility, and a read-only agent planner.Related issue
N/A
Validation
python agentic-workflows/unskip-closed-tests/tests/run_tests.py— passed, 22/22 behavioral regressions.python -m unittest eng/agentic-workflows/test_validate_agentic_workflows.py— passed, 10/10 tests.python eng/agentic-workflows/validate_agentic_workflows.py— passed; all active and packaged workflows strict-compiled with gh-aw v0.89.15.python eng/agentic-workflows/test_unskip_closed_tests_package.py— passed; clean consumer staging, locked restore, Release build with 0 warnings/errors, UTF-8 BOM audit, and strict installed-workflow compile.go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 -shellcheck= -pyflakes= .github/workflows/agentic-workflow-validation.yml— passed.microsoft/testfxconsumer validation — locked staged-tool restore/build passed with 0 warnings/errors; strict compile passed; live issue resolution classified closed/not-planned and open/null references as ineligible; the configured hook executed the exact intended FQN and produced TRX evidence with total=1, passed=1, skipped=0.Checklist
eng/known-domains.txtfor any new external domains referenced by skill content.