feat(#2095): add verified/unchecked variable fields to review findings schema #446
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| #!/usr/bin/env bash | ||
|
There was a problem hiding this comment. [medium] missing-test New test file is not registered in the Makefile's script-test target. All other test scripts are listed there and run by the script-test.yml CI workflow. This file will not execute in CI. Suggested fix: Add $(call run-timed,bash schemas/review-result-label-actions-test.sh) to the script-test target in the Makefile. There was a problem hiding this comment. [low] naming-convention Test file placed in schemas/ directory, which contains only .schema.json files. Convention places test scripts in scripts/. Suggested fix: Move to scripts/review-result-label-actions-test.sh. There was a problem hiding this comment. [low] naming-convention Test script placed in schemas/ directory which on the base branch contains only .schema.json files. All existing test scripts reside in scripts/, hack/, eval/scripts/, or .github/scripts/. The test also uses a different validation pattern (Draft202012Validator) than the established harness validator. Suggested fix: Move to scripts/review-result-label-actions-test.sh and adapt to use the established validation pattern. |
||
| # Tests for label_actions support in review-result.schema.json | ||
| set -euo pipefail | ||
|
|
||
| SCHEMA="$(cd "$(dirname "$0")" && pwd)/review-result.schema.json" | ||
| FAILURES=0 | ||
|
|
||
| fail() { | ||
| echo "FAIL: $1" | ||
| FAILURES=$((FAILURES + 1)) | ||
| } | ||
|
|
||
| validate() { | ||
| local desc="$1" | ||
| local json="$2" | ||
| local expect_pass="$3" | ||
|
|
||
| if echo "${json}" | python3 -c " | ||
| import sys, json | ||
| from jsonschema import validate, ValidationError, Draft202012Validator | ||
| schema = json.load(open('${SCHEMA}')) | ||
| instance = json.load(sys.stdin) | ||
| Draft202012Validator(schema).validate(instance) | ||
| sys.exit(0) | ||
| " 2>/dev/null; then | ||
| if [ "${expect_pass}" = "true" ]; then | ||
| echo "PASS: ${desc}" | ||
| else | ||
| fail "${desc} (expected rejection but schema accepted it)" | ||
| fi | ||
| else | ||
| if [ "${expect_pass}" = "false" ]; then | ||
| echo "PASS: ${desc}" | ||
| else | ||
| fail "${desc} (expected acceptance but schema rejected it)" | ||
| fi | ||
| fi | ||
| } | ||
|
|
||
| # 1. approve without label_actions (baseline) | ||
| validate "approve-without-label-actions" '{ | ||
| "action": "approve", | ||
| "pr_number": 42, | ||
| "repo": "org/repo", | ||
| "head_sha": "abcdef0123456789abcdef0123456789abcdef01", | ||
| "body": "Looks good to me." | ||
| }' true | ||
|
|
||
| # 2. approve with valid label_actions | ||
| validate "approve-with-label-actions" '{ | ||
| "action": "approve", | ||
| "pr_number": 42, | ||
| "repo": "org/repo", | ||
| "head_sha": "abcdef0123456789abcdef0123456789abcdef01", | ||
| "body": "Looks good to me.", | ||
| "label_actions": { | ||
| "reason": "Approved PR, adding reviewed label", | ||
| "actions": [ | ||
| { "action": "add", "label": "reviewed" } | ||
| ] | ||
| } | ||
| }' true | ||
|
|
||
| # 3. request-changes with label_actions | ||
| validate "request-changes-with-label-actions" '{ | ||
| "action": "request-changes", | ||
| "pr_number": 42, | ||
| "repo": "org/repo", | ||
| "head_sha": "abcdef0123456789abcdef0123456789abcdef01", | ||
| "body": "Please fix the issues.", | ||
| "findings": [ | ||
| { | ||
| "severity": "high", | ||
| "category": "security", | ||
| "file": "main.go", | ||
| "description": "SQL injection vulnerability", | ||
| "verified_variables": [], | ||
| "unchecked_variables": [] | ||
| } | ||
| ], | ||
| "label_actions": { | ||
| "reason": "Security issue found, flagging for review", | ||
| "actions": [ | ||
| { "action": "add", "label": "security" }, | ||
| { "action": "remove", "label": "needs-review" } | ||
| ] | ||
| } | ||
| }' true | ||
|
|
||
| # 4. failure with label_actions | ||
| validate "failure-with-label-actions" '{ | ||
| "action": "failure", | ||
| "pr_number": 42, | ||
| "repo": "org/repo", | ||
| "reason": "tool-failure", | ||
| "label_actions": { | ||
| "reason": "Tool failure, marking for manual review", | ||
| "actions": [ | ||
| { "action": "add", "label": "needs-manual-review" } | ||
| ] | ||
| } | ||
| }' true | ||
|
|
||
| # 5. label_actions missing reason — should fail | ||
| validate "label-actions-missing-reason" '{ | ||
| "action": "approve", | ||
| "pr_number": 42, | ||
| "repo": "org/repo", | ||
| "head_sha": "abcdef0123456789abcdef0123456789abcdef01", | ||
| "body": "LGTM", | ||
| "label_actions": { | ||
| "actions": [ | ||
| { "action": "add", "label": "reviewed" } | ||
|
There was a problem hiding this comment. [low] pattern-inconsistency Uses python3/jsonschema for schema validation while the established pattern (validate-output-schema-test.sh) delegates to validate-output-schema.sh, which provides diagnostic features like allowed-properties error messages. |
||
| ] | ||
| } | ||
| }' false | ||
|
|
||
| # 6. label_actions with empty actions array — should fail | ||
| validate "label-actions-empty-actions" '{ | ||
| "action": "approve", | ||
| "pr_number": 42, | ||
| "repo": "org/repo", | ||
| "head_sha": "abcdef0123456789abcdef0123456789abcdef01", | ||
| "body": "LGTM", | ||
| "label_actions": { | ||
| "reason": "No labels to change", | ||
| "actions": [] | ||
| } | ||
| }' false | ||
|
|
||
| # 7. label_actions with invalid action verb — should fail | ||
| validate "label-actions-invalid-verb" '{ | ||
| "action": "approve", | ||
| "pr_number": 42, | ||
| "repo": "org/repo", | ||
| "head_sha": "abcdef0123456789abcdef0123456789abcdef01", | ||
| "body": "LGTM", | ||
| "label_actions": { | ||
| "reason": "Replace a label", | ||
| "actions": [ | ||
| { "action": "replace", "label": "old-label" } | ||
| ] | ||
| } | ||
| }' false | ||
|
|
||
| # 8. label_actions with extra property — should fail | ||
| validate "label-actions-extra-property" '{ | ||
| "action": "approve", | ||
| "pr_number": 42, | ||
| "repo": "org/repo", | ||
| "head_sha": "abcdef0123456789abcdef0123456789abcdef01", | ||
| "body": "LGTM", | ||
| "label_actions": { | ||
| "reason": "Adding label", | ||
| "actions": [ | ||
| { "action": "add", "label": "reviewed" } | ||
| ], | ||
| "priority": "high" | ||
| } | ||
| }' false | ||
|
|
||
| echo "" | ||
| if [ "${FAILURES}" -gt 0 ]; then | ||
| echo "${FAILURES} test(s) failed." | ||
| exit 1 | ||
| else | ||
| echo "All tests passed." | ||
| fi | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
[high] logic-error
The jq example heading says 'For request-changes (including actionable low/info findings) or reject:' but the jq command uses --arg action "approve". This heading/content mismatch could cause an agent following the first template to emit action: "approve" when it intends to emit request-changes.
Suggested fix: Change the heading at line 303 to 'For approve with actionable low/info findings:' to match the --arg action "approve" in the jq example below it.