-
Notifications
You must be signed in to change notification settings - Fork 2k
Improve try-fix skill: add eval.yaml and fix prompt issues #34807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
2f0d3f6
8cd307d
0968c60
9ec2b08
8f7a01f
8b6a318
5513a91
fbbc308
7010339
61babe9
eb5859d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,142 @@ | ||||||||||||||||||||||||||||||||||
| scenarios: | ||||||||||||||||||||||||||||||||||
| - name: "Happy path: propose alternative fix with different approach" | ||||||||||||||||||||||||||||||||||
| prompt: | | ||||||||||||||||||||||||||||||||||
| Use the try-fix skill to attempt a fix for the following problem: | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Problem: CollectionView throws ObjectDisposedException when navigating back from a page on Android. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| The existing PR fix already adds a null check on the adapter in OnMeasure(). We need a DIFFERENT approach. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Test command: pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform android -TestFilter "Issue54321" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Target files: | ||||||||||||||||||||||||||||||||||
| - src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Android.cs | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Platform: android | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Hints: Focus on lifecycle/disposal timing rather than null checks. | ||||||||||||||||||||||||||||||||||
| assertions: | ||||||||||||||||||||||||||||||||||
| - type: output_contains | ||||||||||||||||||||||||||||||||||
| value: "Approach" | ||||||||||||||||||||||||||||||||||
| - type: output_contains | ||||||||||||||||||||||||||||||||||
| value: "Different from existing fix" | ||||||||||||||||||||||||||||||||||
| - type: output_not_contains | ||||||||||||||||||||||||||||||||||
| value: "null check on the adapter" | ||||||||||||||||||||||||||||||||||
| - type: output_contains | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+15
to
+19
|
||||||||||||||||||||||||||||||||||
| - type: output_not_contains | |
| value: "null check on the adapter" | |
| - type: output_contains | |
| - type: output_contains | |
| value: "lifecycle" | |
| - type: output_contains |
Copilot
AI
Apr 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scenario 3's output_not_contains: "Pass" is overly broad and will fail if the agent writes something like "I cannot mark this as Pass" while correctly reporting Blocked. Make the negative assertion more specific (e.g., target the structured result line such as "Result: Pass" / "result.txt: Pass") so the scenario doesn't reject correct explanations.
| value: "Pass" | |
| value: "Result: Pass" | |
| - type: output_not_contains | |
| value: "result.txt: Pass" |
Copilot
AI
Apr 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scenario 4 asserts output_not_contains: "OnPageSelected", but the prompt and rubric require the agent to acknowledge the prior failed attempt, which very likely involves mentioning OnPageSelected explicitly. This assertion will reject compliant outputs; consider instead asserting that the new approach is different (e.g., it references a different lifecycle hook) without forbidding the prior attempt name from being mentioned.
| - type: output_not_contains | |
| value: "OnPageSelected" | |
| - type: output_contains | |
| value: "Different from existing fix" | |
| - type: output_contains | |
| value: "approach" | |
| - type: output_contains | |
| value: "Different from existing fix" | |
| - type: output_contains | |
| value: "approach" | |
| - type: output_contains | |
| value: "before measurement" |
Copilot
AI
Apr 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scenario 5 bans any occurrence of git restore / git reset / git checkout HEAD, but the agent may correctly include those strings in a warning (e.g., "do not use git restore"). That makes the test fragile and may fail even when the workflow is compliant. Prefer positive assertions that the agent uses the baseline/restore script (and/or only forbid the exact command lines it tries to execute).
| - type: output_not_contains | |
| value: "git checkout HEAD" | |
| - type: output_not_contains | |
| value: "git restore" | |
| - type: output_not_contains | |
| value: "git reset" | |
| rubric: | |
| - "The agent uses EstablishBrokenBaseline.ps1 to establish the baseline and restore working directory, not raw git commands" | |
| - "The agent does not use 'git checkout HEAD', 'git restore', or 'git reset' to revert fix files" | |
| - type: output_contains | |
| value: "EstablishBrokenBaseline.ps1" | |
| - type: output_contains | |
| value: "RestoreAfterAttempt" | |
| rubric: | |
| - "The agent uses EstablishBrokenBaseline.ps1 to establish the baseline and restore working directory, not raw git commands" | |
| - "The assertions prefer positive evidence of script-based baseline/restore behavior instead of banning generic git command substrings that may appear in warnings" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Step 8 now says not to use manual
git checkout/git restore/git resetand points to Step 2, but later in the same doc the Error Handling table still recommendsgit checkout HEAD -- ./git clean -fdfor unrecoverable git state. This reintroduces the same contradiction the PR is trying to remove. Please align the guidance (e.g., keep EstablishBrokenBaseline as the primary/only restore mechanism, or clearly label any git commands as an absolute last resort with constraints).