feat(#2095): add verified/unchecked variable fields to review findings schema - #446
feat(#2095): add verified/unchecked variable fields to review findings schema#446ben-alkov wants to merge 4 commits into
Conversation
…s schema Add optional `verified_variables` and `unchecked_variables` array fields to the finding object in review-result.schema.json. These fields force the review agent's security sub-agent to structurally enumerate which variables it verified as sanitized and which it did not, preventing blanket safety claims from partial verification. Changes: - review-result.schema.json: add both arrays (string items, minLength 1) to the finding definition under additionalProperties: false - security.md (sub-agent): update verification methodology step 3 to require populating the new structured fields instead of prose - code-review/SKILL.md: update exhaustive security-control verification guidance to reference the new fields - agents/review.md: add new fields to the finding object documentation table - validate-output-schema-test.sh: add three test cases (partial coverage valid, full coverage valid, empty-string variable rejected) and update the allowed-properties assertion Pre-commit could not run (shellcheck install failed due to network restrictions in the sandbox). The post-script runs pre-commit authoritatively. Closes #2095 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
Make verified_variables and unchecked_variables unconditionally required in the finding schema so agents must always consciously populate them (using [] for non-security findings). Update jq templates in review.md to include the fields, preventing invalid JSON that would burn retry iterations. Add test for missing required variable arrays. Addresses review feedback on #2363 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
Update all consumers of the review finding schema to include the now- required verified_variables and unchecked_variables arrays: - SKILL.md: three finding templates (standard, sub-agent-failure, challenger-failure) - meta-prompt.md: sub-agent output format template - challenger.md: adjudicated_findings template - review-result-label-actions-test.sh: request-changes test finding - postreview.go: ReviewFinding Go struct Addresses review feedback on #2363 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
This file exists in fullsend under internal/scaffold/fullsend-repo/schemas/ but was never ported to agents. The source PR (fullsend#2363) modified it; copying the final state from the PR tip.
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
Site previewPreview: https://43bfc712-site.fullsend-ai.workers.dev Commit: |
🤖 Finished Review · ✅ Success · Started 8:07 PM UTC · Completed 8:20 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsCritical
Previous runReviewFindingsCritical
High
Medium
Previous runReviewFindingsMedium
Previous run (2)ReviewFindingsLow
Labels: PR modifies review agent schema and security sub-agent verification methodology Previous runReviewFindingsCritical
High
Medium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsLow
Info
|
|
"${REVIEW_SCHEMA}" \
[info] test-adequacy Tests cover empty-string rejection for verified_variables but not for unchecked_variables. Both arrays use identical JSON Schema constraints, so schema validation is identical. |
|
[info] documentation-consistency New table rows use longer descriptions than existing rows because the fields require more context about conditional requirements. |
I think this needs a couple changes before we can merge. See inline. |
[medium] These fields aren't in the finding's I think the simplest fix is making both fields unconditionally required, with |
|
[moderate] The jq templates below (lines 289–298) are what the agent copy-pastes to build its output. They don't include Worth updating the |
/fs-fix address those findings |
🤖 Finished Fix · ✅ Success · Started 3:32 PM UTC · Completed 3:40 PM UTC |
🔧 Fix agent — iteration 1 (human-triggered)Made verified_variables and unchecked_variables unconditionally required in the finding schema (with [] valid for non-security findings). Updated jq templates in review.md to include the new fields. Updated all test fixtures and documentation prose to match. Fixed (4):
Tests: passed Updated by fullsend fix agent |
🤖 Finished Review · ❌ Failure · Started 3:44 PM UTC · Completed 3:58 PM UTC |
/fs-review |
🤖 Finished Review · ✅ Success · Started 7:08 PM UTC · Completed 7:22 PM UTC |
See the review comment for full details. |
"$defs": {
[low] misleading-label The PR body describes the fields as optional but the schema adds them to the required array. The implementation correctly matches issue fullsend-ai/fullsend#2095; the PR description is imprecise. |
+run_test_custom_filename "review-finding-verified-variables-empty-string-rejected" \
[info] test-adequacy Tests cover empty-string rejection for verified_variables but not unchecked_variables. Both arrays share identical JSON Schema constraints, so validation is identical. |
🤖 Finished Review · ✅ Success · Started 12:48 PM UTC · Completed 1:09 PM UTC |
"$defs": {
[critical] breaking-schema-change The schema makes verified_variables and unchecked_variables required on every finding (additionalProperties: false), but multiple consumer templates and the Go struct have not been updated. (1) Three finding templates in SKILL.md (lines 380-389, 405-412, 537-544) omit the fields. (2) The meta-prompt.md template (lines 14-24) injected into every sub-agent system prompt omits them. (3) The challenger.md template (lines 48-59) omits them. (4) The review-result-label-actions-test.sh test at line 72 constructs a finding without the fields and expects validation to pass. (5) The Go ReviewFinding struct in internal/cli/postreview.go:166-174 lacks both fields. After this PR merges, all agents following any of these templates will produce findings that fail schema validation, triggering harness retry loops per ADR 0022. Suggested fix: Either (a) make the two new fields optional (remove from required array) and rely on documentation to ensure security sub-agents populate them, or (b) update ALL consumers atomically in this PR: SKILL.md templates, meta-prompt.md, challenger.md, the label-actions test, and the Go struct.
/fs-fix Address this finding by updating ALL consumers atomically in this PR: SKILL.md templates, meta-prompt.md, challenger.md, the label-actions test, and the Go struct. 2 comments in thread |
See the review comment for full details. |
|
/fs-fix Address fullsend-ai/fullsend#2363 (comment) by updating ALL consumers atomically in this PR: SKILL.md templates, meta-prompt.md, challenger.md, the label-actions test, and the Go struct. |
🤖 Finished Fix · ✅ Success · Started 8:12 PM UTC · Completed 8:20 PM UTC |
🔧 Fix agent — iteration 2 (human-triggered)Updated all 5 consumer locations to include the now-required verified_variables and unchecked_variables arrays: 3 SKILL.md templates, meta-prompt.md, challenger.md, label-actions test, and Go struct. All schema validation tests pass. Go code compiles and vets cleanly. Fixed (5):
Tests: passed Updated by fullsend fix agent |
This is an automated comment. We are moving agent content from |
Will move to agents repo |
PR Summary by QodoRequire verified/unchecked variable coverage in review finding schema
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
|
🤖 Finished Review · ✅ Success · Started 9:28 PM UTC · Completed 9:47 PM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. Fixtures miss required arrays
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| "finding": { | ||
| "type": "object", | ||
| "required": ["severity", "category", "file", "description"], | ||
| "required": ["severity", "category", "file", "description", "verified_variables", "unchecked_variables"], |
There was a problem hiding this comment.
2. verified_variables marked required 📜 Skill insight ≡ Correctness
The PR description claims the new fields are optional, but the JSON schema makes verified_variables and unchecked_variables required. This mismatch can cause reviewers to misunderstand the actual contract and produce invalid outputs.
| @@ -0,0 +1,168 @@ | |||
| #!/usr/bin/env bash | |||
| # Tests for label_actions support in review-result.schema.json | |||
There was a problem hiding this comment.
3. Unrelated label_actions test added 📜 Skill insight ✧ Quality
The PR adds a new test script specifically for label_actions schema support, which is not part of the stated work to add verified_variables/unchecked_variables. This expands the PR scope beyond the authorized issue intent.
Agent Prompt
## Issue description
A new script `schemas/review-result-label-actions-test.sh` was added to test `label_actions`, which is not directly related to adding `verified_variables`/`unchecked_variables` fields.
## Issue Context
The PR’s stated intent is to add structured variable enumeration fields to the review finding schema; additions unrelated to that goal should be removed or explicitly justified/authorized.
## Fix Focus Areas
- schemas/review-result-label-actions-test.sh[1-80]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "finding": { | ||
| "type": "object", | ||
| "required": ["severity", "category", "file", "description"], | ||
| "required": ["severity", "category", "file", "description", "verified_variables", "unchecked_variables"], |
There was a problem hiding this comment.
4. Fixtures miss required arrays 🐞 Bug ☼ Reliability
review-result.schema.json now requires every finding to include verified_variables and unchecked_variables, but multiple protected-path fixtures in scripts/validate-output-schema-test.sh still omit them, so those tests will now fail schema validation. This will break the Script tests CI job because it runs make script-test, which executes validate-output-schema-test.sh.
Agent Prompt
## Issue description
`review-result.schema.json` now requires `verified_variables` and `unchecked_variables` on every finding, but several protected-path fixtures in `scripts/validate-output-schema-test.sh` still use the old finding shape. This causes deterministic failures in `make script-test` / CI.
## Issue Context
The schema requires both fields for all findings; for non-security/protected-path fixtures these should typically be empty arrays (`[]`).
## Fix Focus Areas
- scripts/validate-output-schema-test.sh[417-441]
- schemas/review-result.schema.json[73-98]
- Makefile[40-56]
- .github/workflows/script-test.yml[13-43]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| --arg head_sha "<sha>" \ | ||
| --arg body "<markdown review comment>" \ | ||
| --argjson findings '<findings array>' \ | ||
| --argjson findings '[{"severity":"high","category":"<category>","file":"<path>","description":"<desc>","verified_variables":["<var1>"],"unchecked_variables":["<var2>"]}]' \ |
There was a problem hiding this comment.
5. Misleading approve findings example 🐞 Bug ⚙ Maintainability
In agents/review.md, the section “approve with actionable low/info findings” shows an example finding with severity "high" and no actionable flag, contradicting the surrounding guidance. This inconsistency can cause agents/authors to produce approve outputs with the wrong severity/actionable conventions.
Agent Prompt
## Issue description
The “approve with actionable low/info findings” example uses `"severity":"high"` and omits `actionable:true`, contradicting the text and intended conventions.
## Issue Context
For an approve-with-followups example, the sample finding should demonstrate a low/info severity and include `actionable:true` (and still include the required `verified_variables`/`unchecked_variables` arrays).
## Fix Focus Areas
- agents/review.md[292-319]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| #!/usr/bin/env bash | ||
| # 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 | ||
| } |
There was a problem hiding this comment.
6. Unwired label_actions test 🐞 Bug ⚙ Maintainability
schemas/review-result-label-actions-test.sh is added as a test but isn’t invoked by make script-test, so it won’t run in the Script tests CI job. This reduces regression coverage and risks the test silently rotting.
Agent Prompt
## Issue description
A new test script exists under `schemas/` but is not executed by `make script-test`, which is what CI runs.
## Issue Context
Either add this test script to the `script-test` Makefile target (preferred if you want ongoing coverage), or remove/relocate it if it’s meant to be manual-only.
## Fix Focus Areas
- schemas/review-result-label-actions-test.sh[1-168]
- Makefile[40-56]
- .github/workflows/script-test.yml[13-43]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsCritical
High
Medium
Low
Labels: PR modifies review agent schema, sub-agents, and skills |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
schemas/review-result.schema.json:75: [critical] breaking-schema-change
Adding verified_variables and unchecked_variables to the required array is a breaking change with zero backward compatibility. The schema has additionalProperties: false, so no finding is valid under both the old and new schema versions.
Suggested fix: Consider making the fields optional to enable gradual adoption, or coordinate the rollout so all consumers are updated simultaneously.
scripts/validate-output-schema-test.sh(file-level): Line 402 · [high] test-integrity
Three protected-path test cases (review-comment-with-protected-path-valid, review-request-changes-with-protected-path-valid, review-approve-no-protected-path-valid) were not updated with the now-required verified_variables and unchecked_variables arrays. These tests will flip from PASS to FAIL, breaking the test suite.
Suggested fix: Add verified_variables:[] and unchecked_variables:[] to the finding objects in all four protected-path test cases.
agents/review.md:299: [medium] documentation-correctness
The jq example findings are swapped between two code blocks. The 'approve with actionable low/info findings' block contains a high-severity finding without actionable:true, while the 'request-changes or reject' block contains a low-severity finding with actionable:true. These contradict their section headings.
schemas/review-result.schema.json:72: [low] schema-array-constraint-inconsistency
The new required arrays lack minItems constraints, unlike the existing findings and label_actions.actions arrays. Empty arrays are intentional for non-security findings, but the pattern departure is undocumented.
agents/review.md:269: [low] documentation-table-formatting
The new table rows for verified_variables and unchecked_variables appear after the optional actionable field, breaking the existing convention of listing required fields before optional ones.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional findings from an independent review pass (deduplicated against comments already on this PR):
Critical
-
[dco-signoff] commit
d5aad121(schemas/review-result-label-actions-test.sh) — The DCO check-run for this PR isaction_required: GitHub's own output states "Commit sha: d5aad12 ... The sign-off is missing." Of the 4 commits on this branch,43f47bee,58db71c, andd506196each carry aSigned-off-bytrailer, but the 4th,d5aad121("chore: port review-result-label-actions-test.sh from fullsend scaffold"), does not. This blocks merge under DCO policy.
Suggested fix: Rebase with sign-off (e.g.git rebase --signoff main, or amend the tip commit with-s) and force-push, then re-check PR checks to confirm DCO turns green.(A related critical finding on
skills/pr-review/sub-agents/challenger.md:62is posted as an inline comment on this review.)
High
- [cross-repo-consumer-gap]
internal/cli/postreview.go(fullsend-ai/fullsend, cross-repo — not part of this diff) — Checked directly against the livefullsend-ai/fullsendmain branch (this file doesn't exist inagents): theReviewFindingstruct (~line 168) has onlySeverity, Category, File, Line, Description, Remediation, Actionable— noVerifiedVariables/UncheckedVariables— andformatFindingComment(~line 450), which renders findings into GitHub review comments, only emits Severity/Category/Description/Remediation. Since Go'sencoding/jsonsilently ignores unknown fields, once this schema merges, agents will be forced to populate both arrays and the data will validate correctly but be silently dropped before reaching a human reviewer through this harness. Commitd506196's message claims to have updated "postreview.go: ReviewFinding Go struct," but no such file exists in this repo's diff, and the real file infullsend-ai/fullsendis unchanged. This defeats issue #2095's stated goal ("unchecked variables surface as explicit gaps rather than silent omissions") for the artifact a human actually reads.
Suggested fix: AddVerifiedVariables []string/UncheckedVariables []stringjson-tagged fields toReviewFindinginfullsend-ai/fullsendand extendformatFindingCommentto render them, landing that change in the same rollout window as this schema PR (or open and cross-link an explicit tracking issue infullsend-ai/fullsend) rather than leaving the cross-repo impact asserted away in the PR description.
Medium
- [doc-template-gap]
skills/pr-review/SKILL.md:1158(outside this PR's diff) — This PR only touches three literal JSON example blocks in SKILL.md (around lines 755-763, 778-786, 932-941); it doesn't touch the "Produce the review result" human-facing rendering template (from ~line 1137,#### Criticalheading at 1158:- **[<category>]** \:` — / Remediation:), which still has no mention ofverified_variables/unchecked_variables.scripts/post-review.shonly usesfindings[]for severity-threshold filtering and outcome-downgrade logic — it posts the agent's free-textbodyfield verbatim and never independently renders per-finding structured fields into it. So even once a sub-agent correctly populates both arrays, nothing in this repo's own prompt instructions tells the reviewing agent to restate that data in the human-visible comment; it reaches a human only if the model happens to duplicate it into free-textdescription, which was already possible before this PR. **Suggested fix:** Add a line to the SKILL.md finding-rendering template (e.g. show an "Unchecked: x, y" line whenunchecked_variables` is non-empty), or explicitly document in the schema/field descriptions that these two fields are for internal schema-enforcement/self-check only and not meant for human display, so the design intent is unambiguous.
| "description": "<description, possibly amended>", | ||
| "remediation": "<remediation, required for critical/high>", | ||
| "actionable": true|false, | ||
| "verified_variables": [], |
There was a problem hiding this comment.
[CRITICAL] Challenger template hardcodes empty verified_variables/unchecked_variables arrays, discarding real data
This line (and the matching unchecked_variables line) sets the adjudicated_findings template to a literal "verified_variables": [] / "unchecked_variables": [], unlike the neighboring fields in the same template, which use explicit pass-through placeholders ("description": "<description, possibly amended>", "remediation": "<remediation, required for critical/high>"). Per SKILL.md step 6d.3, whenever the challenger succeeds, the orchestrator replaces the merged finding set with the challenger's adjudicated_findings — the normal/common path. Nothing in challenger.md's own Constraints section, SKILL.md, or meta-prompt.md instructs the challenger to carry these two fields forward unmodified. An LLM following this template literally will blank out real verified/unchecked-variable data (e.g. a security finding's actual sanitization list) on every finding that survives adjudication — exactly the enumeration issue #2095 exists to preserve. Separately, SKILL.md's step 6b ("Merge identical-category findings") spells out explicit merge rules for severity, description, remediation, and actionable when combining two same-category findings, but says nothing about combining/preserving verified_variables/unchecked_variables at that earlier merge stage.
Suggested fix: Change the template here to an explicit carry-over placeholder, e.g. "verified_variables": ["<carried over unmodified from the original finding>"], and add a line to challenger.md's procedure stating these two fields must be copied unmodified since the challenger doesn't re-derive them. In SKILL.md section 6b, add an explicit rule (e.g. union of both findings' arrays) for combining/preserving verified_variables/unchecked_variables alongside the existing severity/description/remediation/actionable rules.
2 participants
Add optional
verified_variablesandunchecked_variablesarray fields to the finding object in review-result.schema.json. These fields force the review agent's security sub-agent to structurally enumerate which variables it verified as sanitized and which it did not, preventing blanket safety claims from partial verification.Changes:
to the finding definition under additionalProperties: false
require populating the new structured fields instead of prose
guidance to reference the new fields
table
coverage valid, full coverage valid, empty-string variable rejected)
and update the allowed-properties assertion
Pre-commit could not run (shellcheck install failed due to network restrictions in the sandbox). The post-script runs pre-commit authoritatively.
Closes fullsend-ai/fullsend#2095
Post-script verification
agent/2095-structured-variable-enumeration)32f73a4f93301493d2c31be3970aa4c51a26acc7..HEAD)Original PR metadata