feat(#439): scale review detail to change complexity - #441
feat(#439): scale review detail to change complexity#441fullsend-ai-coder[bot] wants to merge 4 commits into
Conversation
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 8:06 PM UTC · Completed 8:18 PM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR adds a new capability (complexity-based review calibration) to the review agent skills. |
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-fix Address all review findings.
<details>
<summary>Info findings</summary>
- info 1
- info 2
</details> |
|
🤖 Finished Fix · ✅ Success · Started 8:52 PM UTC · Completed 8:58 PM UTC |
- Remove all language making challenger optional for simple changes: step 3c dispatch, step 6d skip directive, dispatch examples table - Challenger now always runs after dimension sub-agents, regardless of change complexity classification - Place info-level findings in collapsed <details> section in the review comment template and formatting rules - Verified step 3c-1 (security-critical file triage) is not contradicted: simple changes (≤2 files) never reach per-file mode (≥50 files), so the two steps are mutually exclusive by definition Addresses review feedback on #441
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 3 human-instruction items: (1) made challenger agent mandatory for all changes including simple ones by removing skip directives in steps 3c, 6d, and the dispatch table; (2) verified step 3c-1 is not contradicted since simple (2 or fewer files) and per-file mode (50+ files) are mutually exclusive; (3) added collapsed details section for info-level findings in the review comment template and formatting rules. Fixed (5):
Tests: passed Decision points
Updated by fullsend fix agent |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 9:09 PM UTC · Completed 9:28 PM UTC |
![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:
skills/code-review/SKILL.md(file-level): Line 323 · [medium] logic-error
Internal contradiction between the new complexity calibration and the Constraints section. The calibration (line 71) instructs simple changes to 'evaluate only Correctness and Style/conventions — skip the other four dimensions.' However, the Constraints section (line 323) states: 'If you cannot complete all six dimensions (tool failure, missing context, ambiguous findings), state that clearly rather than producing an incomplete result.' An agent following both instructions on a simple change faces a contradiction.
Suggested fix: Update the constraint to say 'all applicable dimensions' instead of 'all six dimensions', with a parenthetical noting that simple changes intentionally evaluate only Correctness and Style/conventions.
skills/pr-review/SKILL.md:1186: [low] edge-case
Step 7a brief format example omits the hidden SHA comment that the instruction text (line 1182) explicitly requires ('the hidden SHA comment followed by a 2-3 sentence summary'). Agents pattern-matching on the example may omit the SHA, breaking re-review anchoring.
Suggested fix: Add the hidden SHA comment to the example block.
skills/pr-review/SKILL.md:445: [low] edge-case
The dispatch examples table lists 'challenger' in the agents-dispatched column for simple rows (445-447) and re-review rows (454-455) but omits it from other standard rows (448-453). Since the challenger always runs (lines 357, 364), its selective inclusion creates inconsistency that could mislead an agent into inferring the challenger is conditional.
Suggested fix: Either add 'challenger' to all rows, or remove it from all rows and add a footnote noting the challenger always runs.
skills/code-review/SKILL.md:68: [low] design-smell
The simple/standard complexity classification criteria are duplicated across code-review/SKILL.md (lines 68-78) and pr-review/SKILL.md (lines 327-339). Future drift could cause inconsistent classification behavior between standalone code-review and orchestrated pr-review.
Suggested fix: Consider adding synchronization comments cross-referencing the two locations.
|
/fs-fix Address the following review findings Medium
Low
|
|
🤖 Finished Fix · ✅ Success · Started 9:33 PM UTC · Completed 9:38 PM UTC |
🔧 Fix agent — iteration 2 (human-triggered)Addressed all 3 review findings: fixed the logic error where a constraint hardcoded 'all six dimensions' contradicting the new complexity calibration, added the hidden SHA comment to the brief format example, and added missing trivial scope annotation to correctness in the dispatch table's simple rows. Fixed (3):
Tests: passed Updated by fullsend fix agent |
- Fix logic error in code-review constraint: "all six dimensions" hardcoded count contradicts the new complexity calibration which intentionally evaluates only two dimensions for simple changes. Changed to "all applicable dimensions" with a parenthetical clarifying what "applicable" means per classification. - Add hidden SHA comment placeholder to pr-review brief format example so the example matches the prose instruction requiring the SHA comment. - Add "(trivial scope)" annotation to correctness agent in simple change dispatch table rows, consistent with the step 3e scope constraint table which mandates trivial scope for ALL sub-agents on simple changes. Addresses review feedback on #441
|
/fs-fix Address the following review finding The dispatch examples table lists 'challenger' in the agents-dispatched column for simple rows (445-447) and re-review rows (454-455) but omits it from other standard rows (448-453). Since the challenger always runs (lines 357, 364), its selective inclusion creates inconsistency that could mislead an agent into inferring the challenger is conditional. Suggested fix: Either add 'challenger' to all rows, or remove it from all rows and add a footnote noting the challenger always runs. |
|
🤖 Finished Fix · ✅ Success · Started 9:42 PM UTC · Completed 9:47 PM UTC |
🔧 Fix agent — iteration 3 (human-triggered)Addressed 1 review finding: removed 'challenger' from all dispatch examples table rows and added a footnote noting it always runs. This resolves the inconsistency that could mislead agents into inferring the challenger is conditional. Fixed (1):
Tests: passed Decision points
Updated by fullsend fix agent |
Remove 'challenger' from all rows in the dispatch examples table and add a footnote clarifying it always runs (step 6d). The selective inclusion in some rows but not others implied it was conditional, contradicting the text at lines 357 and 364. Addresses review feedback on #441
Add complexity-based calibration to the review agent so simple changes (deletion-only, docs/CI config edits affecting ≤2 files) receive a brief 2-3 sentence review instead of a full multi-dimension analysis table. Changes to pr-review skill (orchestrator): - Add step 3b-1: classify change complexity as simple or standard with explicit criteria and edge case guidance - Modify step 3c: simple changes dispatch only correctness and style-conventions sub-agents, skip the challenger pass - Add simple row to step 3e scope constraints table (trivial) - Add challenger skip note in step 6d for simple changes - Restructure step 7 into 7a (brief format) and 7b (standard format) so simple approvals produce concise prose output Changes to code-review skill (standalone): - Add complexity calibration subsection before dimension evaluation with matching simple/standard classification No changes to sub-agent definitions, output schemas, or post-scripts. Complex PRs retain the full multi-dimension analysis. The brief format still includes the hidden SHA comment for re-review anchoring. Closes #439
- Remove all language making challenger optional for simple changes: step 3c dispatch, step 6d skip directive, dispatch examples table - Challenger now always runs after dimension sub-agents, regardless of change complexity classification - Place info-level findings in collapsed <details> section in the review comment template and formatting rules - Verified step 3c-1 (security-critical file triage) is not contradicted: simple changes (≤2 files) never reach per-file mode (≥50 files), so the two steps are mutually exclusive by definition Addresses review feedback on #441
- Fix logic error in code-review constraint: "all six dimensions" hardcoded count contradicts the new complexity calibration which intentionally evaluates only two dimensions for simple changes. Changed to "all applicable dimensions" with a parenthetical clarifying what "applicable" means per classification. - Add hidden SHA comment placeholder to pr-review brief format example so the example matches the prose instruction requiring the SHA comment. - Add "(trivial scope)" annotation to correctness agent in simple change dispatch table rows, consistent with the step 3e scope constraint table which mandates trivial scope for ALL sub-agents on simple changes. Addresses review feedback on #441
Remove 'challenger' from all rows in the dispatch examples table and add a footnote clarifying it always runs (step 6d). The selective inclusion in some rows but not others implied it was conditional, contradicting the text at lines 357 and 364. Addresses review feedback on #441
8fead6b to
5c7a9c8
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review
Follow-up pass after the 3 prior review/fix rounds on this PR — focused on cross-cutting issues a full fresh read surfaces that the narrowly-scoped /fs-fix rounds didn't touch. Posting medium-and-above findings only. The 3 previously-fixed items (six-dimensions wording in the Constraints section, missing SHA in the step 7a example, challenger listing in the dispatch table) are confirmed fixed at this head commit and not re-flagged.
Findings
High
See inline comments on skills/pr-review/SKILL.md:343-351, :1196-1199, :1188-1194, and skills/code-review/SKILL.md:68-76.
Medium
See inline comments on skills/pr-review/SKILL.md:360, :333, :363, :334-335, and the file-level comment on skills/code-review/SKILL.md.
- [premature-decision]
PR-wide— Issue #439 defines explicit validation criteria ("On the next 10 deletion-only or simple config PRs... review output substantially shorter... no regression in catching actual issues... no user complaints about excessive review verbosity"). Nothing in either changed file adds a marker or log that would let a future retro actually check these against real usage — the classification/dispatch outcome isn't recorded anywhere retrievable.
Remediation: Add a lightweight marker (e.g., include the complexity classification in the sticky comment's hidden metadata) so a future retro can sample subsequent simple-classified reviews against issue #439's own criteria.
Not requesting changes — these are worth resolving given this PR governs the review agent's own future behavior, but the merge call is a human one.
| **Edge cases:** | ||
|
|
||
| - A deletion that removes a security check or auth guard is NOT | ||
| simple — removing security controls requires full analysis | ||
| - A CI config change that modifies permissions blocks, secret | ||
| references, or token scoping is NOT simple — these are | ||
| security-sensitive | ||
| - Mostly deletions with even 1 line of new logic → standard | ||
| - 3+ files even if all are mechanical renames → standard |
There was a problem hiding this comment.
[fail-open] HIGH — Neither this edge-case list nor code-review/SKILL.md's calibration section excludes test-file deletions from "simple." A deletion-only PR removing test files/cases qualifies as simple today, which routes it to the trivial scope constraint at line 624 — "Do NOT read... git history" — forbidding exactly the check code-review/SKILL.md:44-50 prescribes for this scenario (git log --oneline -10 -- <test-file-path> to catch weakened coverage). This codebase's own correctness dimension explicitly tracks test-weakened/test-removed/coverage-reduced as findings, so this is a first-class risk falling through the simple path's trivial scope.
Remediation: Add an edge case here mirroring the existing security-guard carve-out, e.g. "A change that deletes or modifies test files/test cases is NOT simple — test coverage changes require the full correctness check, including git history on the affected test file."
| If a simple change produces medium+ findings (unexpected for a | ||
| genuinely simple change — consider whether the complexity | ||
| classification was wrong), fall through to the standard format | ||
| below. |
There was a problem hiding this comment.
[logic-error] HIGH — This escape hatch only changes which markdown template step 7 uses; it can't actually re-investigate. By this point, dispatch already happened at step 3c/4 (this document is explicitly sequential — "Follow these steps in order. Do not skip steps.", line 84 — with no reclassify/re-dispatch/loop-back instruction anywhere in the file). So "fall through to the standard format" still only has correctness/style-conventions findings to show, in a template that implies full 6-dimension coverage. code-review/SKILL.md:74-76's parallel language ("reclassify as standard and evaluate all dimensions") works there because it's single-pass — the equivalent doesn't exist here.
Remediation: State that a misclassification discovered here triggers a return to step 3c/4 to dispatch the previously-skipped conditional sub-agents before synthesis — not just a template change at step 7.
| **Simple** — ALL of the following hold: the diff is purely subtractive | ||
| (deletion-only) OR touches only CI/CD configuration or documentation | ||
| files; affects 2 or fewer files; does not modify production logic; does | ||
| not touch security-sensitive paths. For simple changes, evaluate only | ||
| **Correctness** and **Style/conventions** — skip the other four | ||
| dimensions. Produce a brief 2-3 sentence assessment rather than a | ||
| per-dimension breakdown. If a simple change reveals unexpected | ||
| concerns (e.g., a deletion removes a security guard), reclassify as | ||
| standard and evaluate all dimensions. |
There was a problem hiding this comment.
[design-smell] HIGH — This section and pr-review/SKILL.md:327-351 are meant to define the same "simple" classification but have drifted in substance, not just wording. pr-review has a "no added lines" qualifier, concrete CI/doc file-type lists, and 4 edge cases (security-guard deletion, CI permission/secret changes, "1 line of new logic → standard," "3+ files even if mechanical renames → standard"). This section reproduces only 1 of those 4 edge cases in a single dense paragraph. A standalone code-review invocation (its documented use case) can classify a change as simple that pr-review's fuller criteria would correctly reject as standard. (Flagged as a low "design-smell"/sync-comment suggestion in a prior review round — deeper look shows it's a real behavioral gap between the two entry points, not cosmetic duplication.)
Remediation: Port pr-review's full edge-case list and qualifiers into this section verbatim, or have one file explicitly reference the other as the source of truth for the criteria.
| ```markdown | ||
| <!-- **Head SHA:** abc1234 --> | ||
| Deletion of the Tekton task definition and corresponding README | ||
| reference. No logic or security implications — the removal is | ||
| clean with no remaining references to the deleted task. Looks | ||
| good. | ||
| ``` |
There was a problem hiding this comment.
[logic-error] HIGH — This example asserts two conclusions the simple path's own scope constraint can't back up. "No remaining references to the deleted task" requires searching other files, but the trivial scope constraint at line 624 is a stated hard limit: "Do NOT read... surrounding files... directory listings." And "No logic or security implications" is asserted despite security never being dispatched for simple changes. Worth noting: issue #439's own "what happened" section cites "'No security implications — removes a documentation-enforcement CI check only,' which are self-evident for a deletion-only change" as the exact noise this PR exists to eliminate — this example now reproduces almost that same phrase.
Remediation: Rewrite to claims verifiable from the diff alone under trivial scope, e.g. "the deletion is confined to the task file and its README entry; the diff touches no security-sensitive paths" instead of asserting an investigation-backed conclusion.
|
|
||
| **Dispatch sub-agents based on the classification — typically 3-6.** | ||
| **Simple change dispatch:** When the change is classified as simple | ||
| (step 3b-1), dispatch ONLY `correctness` and `style-conventions`. |
There was a problem hiding this comment.
[premature-decision] MEDIUM — Issue #439's "Proposed change" asks only for shorter output ("produce a brief summary... rather than a multi-dimension analysis table"). This dispatch rule additionally skips 4 sub-agents entirely and (via the line-624 scope constraint) caps the remaining 2 at 5 tool calls with git-history/context reads forbidden — a materially larger change to investigation depth than what the issue asked for, not explicitly confirmed anywhere in the PR or issue thread as an intended scope expansion.
Remediation: Confirm with the issue author/reviewer whether reduced investigation (not just reduced write-up) was intended; if so, say so explicitly in the PR description rather than leaving it implicit here.
| OR the change touches only CI/CD configuration files (workflows, | ||
| pipeline definitions, `.pre-commit-config.yaml`) or documentation | ||
| files (`.md`, `.adoc`, `.rst`, `README`, `CHANGELOG`) | ||
| - The change affects 2 or fewer files |
There was a problem hiding this comment.
[premature-decision] MEDIUM — Issue #439 reserves full review for changes that (among other things) "span multiple subsystems." This criterion never tests that directly — two files belonging to unrelated subsystems (e.g., one workflow file for one service plus one doc file for a different one) would still pass every "simple" criterion. The file-count proxy was never discussed or confirmed against the issue's actual "multiple subsystems" language.
Remediation: Either add an explicit "does not span multiple subsystems/services" criterion, or note in the PR that file count is treated as a sufficient proxy and get that confirmed.
| (step 3b-1), dispatch ONLY `correctness` and `style-conventions`. | ||
| Do not dispatch conditional sub-agents (`security`, | ||
| `intent-coherence`, `docs-currency`, `cross-repo-contracts`) | ||
| regardless of step 3b's domain classification. The challenger pass |
There was a problem hiding this comment.
[logic-error] MEDIUM — This "regardless of step 3b's domain classification" override doesn't address the separately-stated "Re-review dispatch (prior-finding-aware)" rule later in this same step (unchanged by this PR): when PRIOR_REVIEW_PROVENANCE is app-verified and prior findings exist, dimensions with prior findings dispatch at normal scope "to verify the fixes." Neither rule states precedence for a re-review whose current diff also now satisfies the simple criteria (e.g., a fix that shrank the diff to a 1-file deletion). This is the same shape of ambiguity round 1 flagged for challenger (resolved by making challenger unconditional) but was never generalized to the other four conditional dimensions.
Remediation: Add an explicit precedence rule, e.g. "a dimension with prior findings always dispatches at normal scope to verify the fix, even when the current diff classifies as simple."
| - The change does not modify production logic (application code, | ||
| library code, controllers, handlers, models) |
There was a problem hiding this comment.
[edge-case] MEDIUM — This criterion excludes changes that "modify" production logic, but criterion 1 above independently allows any "purely subtractive (deletion-only, no added lines)" diff. If deleting a file wholesale isn't read as "modifying" it, a full removal of a non-auth controller/handler/model could pass both criteria — yet plausibly still needs intent-coherence (was the deletion authorized/intentional?) and cross-repo-contracts (does anything still call it?) review. Only the security-guard variant of this pattern is called out as an edge case below.
Remediation: Reword to "does not add, modify, or remove production logic," or add an edge case: "deletion of a production application/library file (controller, handler, model, or public function) is standard, not simple."
There was a problem hiding this comment.
[logic-error] MEDIUM — The frontmatter description (lines 5-6) still says this skill "evaluates across six review dimensions" with no qualifier — a separate, still-unconditional claim from the already-fixed "all six dimensions" wording in the Constraints section below (that fix was correctly scoped to the Standard branch; this frontmatter line predates it and wasn't touched).
Remediation: Update to something like "evaluates across the applicable review dimensions (two for simple changes, six for standard)."
2 participants
Summary
correctnessandstyle-conventionssub-agents with trivial scope, skip the challenger pass, and produce condensed prose outputContext
On PR fullsend-ai/fullsend#3700 (deletion of 31 lines of Tekton YAML + 4-line README edit), the review agent produced a 6-dimension analysis table with self-evident observations like "YAML structure remains valid after task removal." This was characterized as noise that creates an illusion of thoroughness and feeds misleading data into retro autonomy assessments.
Changes
skills/pr-review/SKILL.md(orchestrator)correctnessandstyle-conventions, no conditional sub-agents, no challengerskills/code-review/SKILL.md(standalone)Test plan
🤖 Generated with Claude Code
Closes #439
Post-script verification
agent/439-scale-review-detail)e3293716ee87a1b1a6d07a403ccdeba4d724c9b9..HEAD)