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 10 commits into
Conversation
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 |
|
🤖 Finished Review · ✅ Success · Started 8:11 PM UTC · Completed 8:53 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
|
Risk Assessment: elevated (3/5) DetailsElevated risk driven by 9 protected path changes, high fix/revert history (13.1 avg per file in 90d), significant multi-author contention (5.1 avg authors), and security/priority-high issue labels with no feature flag for rollback — partially offset by clear acceptance criteria coverage, no dependency or CI changes, and experienced non-bot author. Previous runRisk Assessment: elevated (3/5) DetailsElevated risk driven by 9 protected path changes, CI workflow modification, high fix/revert history (13.7 avg per file in 90d), significant multi-author contention (5.1 avg authors), and security/priority-high issue labels with no feature flag for rollback — partially offset by clear acceptance criteria coverage, no dependency changes, and experienced non-bot author. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
…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
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
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
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.
…riable fields - Add verified_variables/unchecked_variables to protected-path and short-SHA test fixtures in validate-output-schema-test.sh so they test their intended constraint rather than failing on missing fields - Fix "approve with actionable findings" jq example in review.md to use severity:"low" and include actionable:true, matching the section heading - Wire schemas/review-result-label-actions-test.sh into make script-test and fix short SHA values in its test data so tests pass - Add verified_variables/unchecked_variables to the finding format example in intent-coherence.md sub-agent Addresses review feedback on #446
- SKILL.md finding-rendering template: add "Unchecked:" line showing unchecked_variables when non-empty, so the human-facing review body surfaces variable-coverage gaps - SKILL.md section 6b merge rules: add union semantics for verified_variables and unchecked_variables when merging same-category findings (deduplicate, drop from unchecked if verified) - challenger.md: change template placeholders to explicit carry-over markers and add procedure step 5 requiring unmodified copy of both fields (challenger does not re-derive them) Addresses review feedback on #446 fixup! 9025f9c
Add verified_variables and unchecked_variables arrays to all finding JSON objects in post-review-test.sh to match the now-required schema fields. Reorder the finding field table in agents/review.md to list required fields before optional ones. Addresses review feedback on #446
- Add "Verified:" line to review output format template (intent-alignment) - Use placeholder notation in generic finding template (api-shape-consistency) Addresses review feedback on #446
- SKILL.md: add explanatory paragraph after finding JSON template clarifying that both arrays are required on every finding (security sub-agents populate with names; non-security use []) - meta-prompt.md: replace bare [] with descriptive placeholders matching SKILL.md template style - challenger.md: clarify that merged findings union the arrays per step 6b rules rather than copying unmodified Addresses review feedback on #446
…riable fields - Add verified_variables/unchecked_variables to protected-path and short-SHA test fixtures in validate-output-schema-test.sh so they test their intended constraint rather than failing on missing fields - Fix "approve with actionable findings" jq example in review.md to use severity:"low" and include actionable:true, matching the section heading - Wire schemas/review-result-label-actions-test.sh into make script-test and fix short SHA values in its test data so tests pass - Add verified_variables/unchecked_variables to the finding format example in intent-coherence.md sub-agent Addresses review feedback on #446 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
- SKILL.md finding-rendering template: add "Unchecked:" line showing unchecked_variables when non-empty, so the human-facing review body surfaces variable-coverage gaps - SKILL.md section 6b merge rules: add union semantics for verified_variables and unchecked_variables when merging same-category findings (deduplicate, drop from unchecked if verified) - challenger.md: change template placeholders to explicit carry-over markers and add procedure step 5 requiring unmodified copy of both fields (challenger does not re-derive them) Addresses review feedback on #446 fixup! 9025f9c Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
Add verified_variables and unchecked_variables arrays to all finding JSON objects in post-review-test.sh to match the now-required schema fields. Reorder the finding field table in agents/review.md to list required fields before optional ones. Addresses review feedback on #446 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
- Add "Verified:" line to review output format template (intent-alignment) - Use placeholder notation in generic finding template (api-shape-consistency) Addresses review feedback on #446 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
- SKILL.md: add explanatory paragraph after finding JSON template clarifying that both arrays are required on every finding (security sub-agents populate with names; non-security use []) - meta-prompt.md: replace bare [] with descriptive placeholders matching SKILL.md template style - challenger.md: clarify that merged findings union the arrays per step 6b rules rather than copying unmodified Addresses review feedback on #446 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
466db30 to
69f7024
Compare
|
🤖 Review · Commit: |
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 8:11 PM UTC · Completed 8:25 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.50 |
- Fix heading/content mismatch in agents/review.md jq example (heading said request-changes but jq used approve action) - Add verified_variables/unchecked_variables to 7 post-review-test.sh fixtures that were missing the now-required fields - Remove out-of-scope label_actions test file and Makefile entry (unrelated to issue #2095) - Revert unauthorized actionable field description change in schema - Add explanatory paragraph to meta-prompt.md for non-security sub-agent guidance on verified/unchecked variable fields Addresses #446
🔧 Fix agent — iteration 8 (human-triggered)Iteration 8: Fixed validation failure where agent commits contained Signed-off-by trailers. Used git filter-branch to strip all Signed-off-by trailers from the 10 PR commits (c4e059f..HEAD). All previous review fixes from prior iterations remain intact: heading/content mismatch fix, test fixture updates, scope-creep removal, actionable field revert, meta-prompt explanatory paragraph, placeholder notation consistency, and Verified: line addition. Fixed (8):
Disagreed (3):
Tests: passedNext steps:
|
69f7024 to
a33bbeb
Compare
|
🤖 Finished Review · ✅ Success · Started 8:27 PM UTC · Completed 8:50 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
skills/code-review/SKILL.md(file-level): Line 220 · [medium] consumer-completeness
The 'Compile findings' section (step 4) defines the canonical finding field list for standalone code-review invocations but does not include verified_variables or unchecked_variables. The security dimension section (around line 135) references these arrays and states 'Both arrays are required on every finding', but the output specification at step 4 only lists Severity, Category, Description, Location, Remediation, and Actionable. In standalone/pre-push mode where the meta-prompt is not injected, the agent receives conflicting instructions within the same skill file.
Suggested fix: Add verified_variables and unchecked_variables to the finding field list in step 4 ('Compile findings'), with a note that both are required on every finding (use [] for non-security findings).
| "finding": { | ||
| "type": "object", | ||
| "required": ["severity", "category", "file", "description"], | ||
| "required": ["severity", "category", "file", "description", "verified_variables", "unchecked_variables"], |
There was a problem hiding this comment.
[medium] breaking-schema
Adding verified_variables and unchecked_variables to the required array is a backward-incompatible schema change. The original companion PR (fullsend-ai/fullsend#2363) was CLOSED without merging. A replacement issue (fullsend-ai/fullsend#6233) is open and this PR declares itself blocked by that issue, but no companion implementation updating the ReviewFinding Go struct exists as a mergeable artifact yet.
Suggested fix: Do not merge this PR until fullsend-ai/fullsend#6233 has a merged PR that updates the ReviewFinding Go struct.
| | `line` | integer | no | Line number (minimum 1) | | ||
| | `description` | string | yes | Finding description (min 1 char) | | ||
| | `remediation` | string | no | Suggested fix | | ||
| | `actionable` | boolean | no | If `true`, marks the finding for future follow-up issue creation[1] | |
There was a problem hiding this comment.
[low] scope-creep
The actionable field description was changed from 'When true with a non-empty remediation, routes the verdict to request-changes...' to 'If true, marks the finding for future follow-up issue creation' with a footnote restricting to 'Only for low/info findings in an approve result.' This semantic narrowing goes beyond issue #2095's scope. The JSON schema description is unchanged, limiting behavioral impact to human-readable documentation.
| | `line` | integer | no | Line number (minimum 1) | | ||
| | `description` | string | yes | Finding description (min 1 char) | | ||
| | `remediation` | string | no | Suggested fix | | ||
| | `actionable` | boolean | no | If `true`, marks the finding for future follow-up issue creation[1] | |
There was a problem hiding this comment.
[low] pattern-inconsistency
The actionable field description in the review.md finding-object table now says 'marks the finding for future follow-up issue creation' while the schema description still says 'routes the verdict to request-changes.' This creates a semantic discrepancy between the human-readable table and the machine-enforced schema.
Suggested fix: Align the actionable description in agents/review.md with the schema description in review-result.schema.json, or update both to reflect the intended semantics.
2 participants
Add required
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
Blocked by fullsend-ai/fullsend#6233
Post-script verification
agent/2095-structured-variable-enumeration)32f73a4f93301493d2c31be3970aa4c51a26acc7..HEAD)Original PR metadata