Conversation
Functional tests did not runFunctional tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the |
…, per-SHA history ADR 0089 leaves tier 1 to a bash script but has the sub-agent re-emit it, which is how the same PR scored 1, 2, 1 across three re-reviews (fullsend-ai#1037). risk-tier1.sh now ends with TIER1_SCORE and RISK_FLOOR — the SKILL.md table computed once, deterministically — and the sub-agent copies them into tier1_score / risk_floor instead of re-deriving. RISK_FLOOR is 2 whenever a security-sensitive path is touched. post-review.sh enforces it from the echoed field, so risk/low cannot be applied to a security PR whatever the LLM returned. Over the 246 production PRs measured on fullsend#4698, no score-1 PR touched such a path, so the floor changes nothing today and closes the gap for later. When the sub-agent fails, the orchestrator no longer drops the score: it runs the script itself and emits max(round(TIER1_SCORE), RISK_FLOOR) with degraded: "tier1-only". Four production reviews since 08-25 lost their score to "claude-sonnet-4-5@20250929 is not available" (the error behind fullsend#6922, visible from 08-25); consumers must treat degraded as no score. The sticky risk comment now carries the tier 1 value, the degraded marker, and a per-head-SHA history table carried forward from the prior comment (GitHub only, 20 rows, rows re-admitted only when they match the exact shape this script writes), so drift across re-reviews is visible on the PR instead of only in run artifacts. Tests: risk-tier1-test.sh covers score_tier1, _score_size, has_source_files, risk_floor and both e2e fixtures; post-review-test.sh covers floor raises/never lowers, degraded header, history row, garbage provenance dropped, and legacy results unchanged. Baseline ceilings for the two SKILL.md files bumped for the added prose. Refs fullsend-ai#1037, fullsend-ai/fullsend#4698 Signed-off-by: guy oron <goron@redhat.com>
c45af41 to
e385ea3
Compare
The scorer makes tier1_score and risk_floor required in the risk assessment, and the orchestrator fallback emits degraded. But review-result.schema.json set additionalProperties:false on risk_assessment, so a compliant result failed validation and the whole review was dropped by the max_iterations:1 validation loop. Add the three fields as optional (number 1-5, integer 1-5, and the tier1-only enum) so legacy and UNKNOWN results stay valid, and add schema tests using post-review-test.sh's own fixtures so the strict schema and the lenient post-script cannot drift apart again. Signed-off-by: guy oron <goron@redhat.com>
…n tests Three review follow-ups on the risk-hardening PR: - Labels: a degraded (tier-1-only fallback) score produced a risk/level label byte-identical to a computed one, so no consumer could honour the "treat degraded as no score" contract the PR documents. Apply a risk/degraded marker label alongside the level, and sweep it with the other stale risk labels so it clears when a later review is computed. - Docs: the Tier 1 section intro still told the LLM to assign sub-scores and average them, contradicting the new "take TIER1_SCORE, do not re-derive" procedure — the exact split that caused the flip this PR fixes. Reword the intro as the rubric the script implements, used directly only when TIER1_SCORE is UNKNOWN. Also list tier1_score and risk_floor in pr-review's parsed-payload description. - Tests: the garbage-dropped and legacy assertions used newline- terminated grep -qF, which strips the newline and degrades to a substring match that passed on the output it meant to reject. Add line/absent match modes, assert tier 1 and degraded meta are absent for bad provenance, and cover the degraded marker label and the legacy history row. Signed-off-by: guy oron <goron@redhat.com>
Signed-off-by: guy oron <goron@redhat.com>
The risk-hardening text pushed pr-review/SKILL.md and pr-risk-assessment/SKILL.md past their skillsaw context-budget ceilings. Condense it instead of raising the baseline: - pr-risk-assessment: one intro sentence for TIER1_SCORE/RISK_FLOOR, fold the UNKNOWN, TIER1_SCORE and degenerate-case paragraphs into one. TIER1_SCORE=UNKNOWN only when no signal could be scored, so it means Tier 1 is unavailable; there is no manual re-scoring path. - pr-review: shorter failure fallback, same behaviour; the "treat degraded as no score" rule stays in docs/review.md. .skillsaw-baseline.json is now identical to main. Signed-off-by: guy oron <goron@redhat.com>
…he tier-1 payload contract post-review.src.sh now derives the security floor from the PR's changed files with the same SECURITY_PATTERNS list as risk-tier1.sh and takes the max with the sub-agent's echoed risk_floor, so an omitted or lowered field can no longer disable the floor (qodo Q1). Non-approve actions fetch the file list for this check; a failed fetch logs a warning and keeps the echoed value. post-review-test.sh covers echoed-1, omitted, comment-action and non-security cases, and pins the two pattern lists equal. skills/pr-review/SKILL.md step 6 again names the risk_assessment fields and the rule that anything routing or gating on the score treats `degraded` as no score (waynesun09 W3, condensed away in f49fede). Two nearby sentences were shortened to stay under the context-budget ceiling; .skillsaw-baseline.json is unchanged. Signed-off-by: guy oron <goron@redhat.com>
e385ea3 to
7d68dcb
Compare
…t be fetched On comment and request-changes reviews the floor block fetches the PR's changed files itself. An empty result only logged a warning and kept the sub-agent's risk_floor, so a transient empty list (#2093) plus an omitted field labelled a security-sensitive change risk/low (waynesun09). The fetch now retries once after the same sleep as the approve path. If the list is still empty nothing was measured, so the change is treated as security-sensitive: RISK_FLOOR=2, the value risk-tier1.sh and the pattern match use, with a ::warning:: saying why. The review is still posted; only the label is held at the floor. The label follows the level, and the schema does not tie level to score, so RISK_FLOOR=2 alone still labelled low for a "low" level beside a score of 2 or an invalid score. The level is now raised to moderate whenever the floor is 2 or more, on the fetched-list path as well. post-review-test.sh covers the failed fetch on comment and request-changes (moderate created, low not, warning logged, review still posted), the first-empty-then-populated retry in both directions, and the two level cases. docs/review.md names the new floor condition. Signed-off-by: guy oron <goron@redhat.com>
…-in flag ADR 0089 / fullsend-ai#861 compute the composite risk score and leave acting on it for later. This is the smallest later: REVIEW_RISK_ROUTING_ENABLED (default "false"). When on, risk-assessment runs first instead of in the step-4 batch, and a composite score of exactly 1 keeps only correctness and security, dropping intent-coherence, docs-currency, style-conventions and cross-repo-contracts. No model changes anywhere: since fullsend#7116 models are owned by the consuming repo's .fullsend/config.yaml, and the orchestrator's own model is never touched. A missing or unparseable score, or one marked degraded ("tier1-only", the orchestrator's fallback when the sub-agent failed), fails open and leaves the full review in place. Routing keys on the composite rather than on tier 1 alone because, over the 246 production PRs that carry both a risk comment and a review, composite == 1 had 0/53 with a major or critical finding while a tier-1-only gate would have narrowed 6/104 that did. The pre-pass latency is the price of that. Bumps the pr-review context-budget ceiling for the added text. Refs fullsend-ai/fullsend#4698 Signed-off-by: guy oron <goron@redhat.com>
a62f39d to
557f101
Compare
|
🤖 Finished Retro · ✅ Success · Started 8:19 AM UTC · Completed 8:28 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.80 |
|
PR #1246 (stacked on the still-open #1245, both authored by external contributor guyoron1) was closed unmerged after 18 days with zero review of any kind — no fullsend review-agent dispatch, no human comment, no third-party bot. Cause: dispatch routing (fullsend-ai/fullsend's reusable-dispatch.yml) only auto-triggers the review stage when the PR author holds at least triage-level repo permission; guyoron1 holds the public-repo default of 'read'. This is a known, intentional trade-off (ADR 0054), and the general fix (relax the gate for trusted/fork PRs) is already tracked upstream in open issues fullsend-ai/fullsend#4374, #2967, #5154, and #6802 — I did not re-file that. What nobody has filed is the agents-repo-specific consequence: guyoron1 has opened 16 PRs to fullsend-ai/agents since July 2026 (only 2 merged, 8 still open and stale), and two other sub-triage contributors (Benkapner, amastbau) add 4 more open PRs that also never get auto-reviewed — a real backlog of seemingly good-faith, well-tested contributions with no review-agent coverage. I'm proposing the repo adopt the already-shipped OWNERS-file opt-in (config.yaml authorization provider) to grant a small number of proven contributors triage-equivalent access, as an immediate, reversible, repo-local mitigation that doesn't require waiting on the upstream platform issues. Separately, I independently re-reviewed PR #1246's actual diff (since no one else did): the risk-based dispatch-narrowing logic it documents is logically sound and fails open in the cases it claims to, but the PR ships zero automated tests for this safety-critical behavior — it's pure prompt/doc text riding on #1245's tested scripts — so I'm proposing a functional test case for the REVIEW_RISK_ROUTING_ENABLED narrowing and fail-open paths. Minor aside not filed as a proposal: skills/pr-review/SKILL.md's tracked skillsaw budget is already ~2.8x its stated ceiling and has been ratcheted upward across several recent PRs rather than trimmed — worth a maintainer's attention but I lack enough evidence of systemic harm to propose a specific fix. Proposals filed |
1 participant
Stacked on #1245 (its commit is the first on this branch; review only the top commit).
ADR 0089 / #861 compute the composite risk score and leave acting on it for later. This is the smallest later:
REVIEW_RISK_ROUTING_ENABLED(default"false"). When on,risk-assessmentruns first instead of in the step-4 batch, and a score of exactly 1 with nodegradedmarker narrows the dispatch tocorrectnessandsecurity(intent-coherence, docs-currency, style-conventions, cross-repo-contracts are skipped; security-triage and challenger untouched). No model is changed anywhere — models are owned by the consuming repo's.fullsend/config.yamlsince fullsend-ai/fullsend#7116 — and the orchestrator's own model is never touched. Any other score, a missing or unparseable score, ordegradedpresent → the full selection (fail open).Routing keys on the composite rather than tier 1 because, over the 246 production PRs that carry both a risk comment and a review (fullsend-ai/fullsend#4698), composite == 1 had 0/53 with a major or critical finding while a tier-1-only gate would have narrowed 6/104 that did. Draft until Marta has looked at the numbers; the paired cheaper-model comparison is still to run.
Refs fullsend-ai/fullsend#4698.