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 |
PR Summary by QodoHarden risk scoring with deterministic Tier 1 and security floors
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| PRIOR_ROWS=$(GH_TOKEN="${REVIEW_TOKEN}" gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments" \ | ||
| --jq '[.[] | select(.body | contains("<!-- fullsend:risk-assessment -->"))] | last | .body // empty' 2>/dev/null \ | ||
| | grep -E "${ROW_RE}" || true) |
There was a problem hiding this comment.
2. Older comments pollute risk history 📜 Skill insight ≡ Correctness
The new gh api --paginate --jq pipeline selects last separately on every response page and feeds every selected comment body into the row collector. Once a pull request has comments on multiple pages, rows from older page-level comments join those from the actual latest sticky comment and consume the 20-row history.
Agent Prompt
## Issue description
Pagination currently produces one selected sticky comment per API page, while the history collector expects one globally latest prior comment.
## Fix Focus Areas
- scripts/post-review.src.sh[484-493]
- scripts/post-review.sh[894-903]
- scripts/post-review-test.sh[1852-1859]
## Recommended Fix
Slurp all paginated comment responses before selecting the globally latest matching sticky comment, then extract rows only from that body. Regenerate the bundled script and add a multi-page fixture containing older sticky comments.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ROW_RE='^\| `[0-9a-f]{6,7}` \| [0-9]{4}-[0-9]{2}-[0-9]{2} \| [1-5]/5 [a-z]+ \| [0-9.-]+ \| [a-z0-9-]* \|$' | ||
| PRIOR_ROWS=$(GH_TOKEN="${REVIEW_TOKEN}" gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments" \ | ||
| --jq '[.[] | select(.body | contains("<!-- fullsend:risk-assessment -->"))] | last | .body // empty' 2>/dev/null \ | ||
| | grep -E "${ROW_RE}" || true) |
There was a problem hiding this comment.
5. Users can forge the risk history 🐞 Bug ⛨ Security
PRIOR_ROWS selects the latest comment containing the public risk marker and copies every line matching ROW_RE into the replacement sticky comment without verifying the author, comment identifier, or other trusted provenance. When a PR participant posts a newer marker-bearing comment with conforming fabricated rows, the next review carries its scores and head hashes into the displayed history as apparent bot-generated provenance.
Agent Prompt
## Issue description
Risk history is imported from any marker-bearing PR comment, allowing another commenter to inject fabricated scores, head hashes, and provenance into the bot's next sticky comment.
## Fix Focus Areas
- scripts/post-review.src.sh[475-493]
- scripts/post-review.src.sh[505-510]
- scripts/post-review-test.sh[1852-1859]
## Recommended Fix
Resolve the authenticated Fullsend review actor and restrict the comment query to marker-bearing comments authored by that identity before selecting a comment and extracting rows. Alternatively, fetch the exact sticky comment identifier maintained by `fullsend post-comment`. Preserve strict row-shape validation as defense in depth, add a test fixture proving that a newer user-authored marker comment with conforming rows is ignored, and regenerate `scripts/post-review.sh`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| PRIOR_ROWS=$(GH_TOKEN="${REVIEW_TOKEN}" gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments" \ | ||
| --jq '[.[] | select(.body | contains("<!-- fullsend:risk-assessment -->"))] | last | .body // empty' 2>/dev/null \ | ||
| | grep -E "${ROW_RE}" || true) | ||
| NEW_ROW="" | ||
| if [[ -n "${RISK_HEAD}" && "${RISK_SCORE}" =~ ^[1-5]$ ]]; then | ||
| NEW_ROW="| \`${RISK_HEAD:0:7}\` | $(date -u +%Y-%m-%d) | ${RISK_SCORE}/5 ${RISK_LEVEL} | ${TIER1_SCORE:--} | ${RISK_DEGRADED} |" | ||
| fi | ||
| ROWS=$(printf '%s\n%s\n' "${PRIOR_ROWS}" "${NEW_ROW}" | sed '/^$/d' | tail -n 20) |
There was a problem hiding this comment.
6. Temporary api failures erase risk history 🐞 Bug ☼ Reliability
The prior-comment fetch suppresses all gh and parsing errors and converts them to an empty PRIOR_ROWS value. On a transient GitHub failure, the script still replaces the sticky comment using only the current row, so all previously retained history disappears.
Agent Prompt
Issue description
A failed GitHub history fetch is treated identically to a PR with no history, so the next marker-based comment update overwrites the existing history table.
Fix Focus Areas
- scripts/post-review.src.sh[479-510]
Recommended Fix
Capture and check the API and query exit status separately from the no-matching-comment case. If fetching prior comments fails, skip the risk-comment replacement and emit a warning, or otherwise retain the existing sticky body rather than publishing a replacement with empty history; add a regression test for a failing comments API call.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| case "${prot}" in UNKNOWN) ;; 0) _add 1 ;; 1) _add 3 ;; *) _add 5 ;; esac | ||
| case "${sec}" in UNKNOWN) ;; 0) _add 1 ;; 1) _add 3 ;; 2|3) _add 4 ;; *) _add 5 ;; esac | ||
| case "${ci}" in true) _add 4 ;; false) _add 1 ;; esac | ||
| case "${depfiles}" in UNKNOWN) ;; none) _add 1 ;; *,*) _add 5 ;; *) _add 3 ;; esac |
There was a problem hiding this comment.
8. One dependency file looks like several 🐞 Bug ≡ Correctness
score_tier1 treats the presence of a comma in DEPENDENCY_FILES_CHANGED as proof that two or more dependency files changed. A single matching manifest whose filename contains a comma is emitted as one comma-joined value by find_dependency_files, then receives the table's five-point multi-file score instead of the three-point one-file score.
Agent Prompt
Issue description
The dependency score infers the number of changed manifests from a comma-delimited display string, so a comma in one filename is misclassified as multiple manifests.
Fix Focus Areas
- skills/pr-risk-assessment/scripts/risk-tier1.sh[92-107]
- skills/pr-risk-assessment/scripts/risk-tier1.sh[179-203]
Recommended Fix
Pass the dependency-file count to `score_tier1` separately from the human-readable dependency list, and score that count directly. Retain the joined list only for emitted diagnostics and add a regression test for a single matching filename containing a comma.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ROWS=$(printf '%s\n%s\n' "${PRIOR_ROWS}" "${NEW_ROW}" | sed '/^$/d' | tail -n 20) | ||
| if [[ -n "${ROWS}" ]]; then | ||
| RISK_HISTORY=$'\n\n<details>\n<summary>History</summary>\n\n| head | date | score | tier 1 | note |\n|---|---|---|---|---|\n'"${ROWS}"$'\n\n</details>' |
There was a problem hiding this comment.
7. Risk history duplicates old rows 🔗 Cross-repo conflict ≡ Correctness
post-review.src.sh copies matching rows from the existing comment into a new 20-row table, but it posts that table without disabling Fullsend's separate sticky-comment history. On each re-review Fullsend archives the previous table as another Previous run block, and the next grep reimports rows from both active and archived tables, creating duplicates that can displace unique head revisions.
Agent Prompt
## Issue description
The risk script now maintains its own bounded history, while `fullsend post-comment` independently preserves every prior comment by default. Archived rows are fetched again on later runs, causing duplicates in the active history table.
## Fix Focus Areas
- scripts/post-review.src.sh[475-510]
- scripts/post-review.sh[885-916]
## Recommended Fix
Pass `--keep-history=false` when posting the risk comment so only the script's bounded per-head history is retained. Regenerate the bundled `scripts/post-review.sh` from the source script and extend the test mock to verify that history preservation is disabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…, 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>
12a79a8 to
e6026cd
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings (review-only, no verdict change)
Verified at head e6026cd. Three findings are posted inline below; this one has no line in the diff to anchor to, because the file the fix belongs in is not part of this PR.
CRITICAL — review-result schema rejects the new risk fields; the fix must land in schemas/review-result.schema.json, which this PR never touches
schemas/review-result.schema.json:54 declares risk_assessment with additionalProperties: false, permitting only score, level, rationale, tier1_signals, tier2_signals, tier3_signals. That file is not in this PR's diff at all (gh pr diff 1245 lists 10 files; this is not one of them).
Meanwhile the PR makes the new fields mandatory:
skills/pr-review/sub-agents/risk-assessment.md:71-73(added): "rationale,tier1_scoreandrisk_floorare required"skills/pr-risk-assessment/SKILL.md:191-193(added): lists them under Required fieldsskills/pr-review/SKILL.md:633(added): tells the orchestrator fallback to emitdegraded: "tier1-only"as well
harness/review.yaml wires scripts/validate-output-schema.sh as a validation_loop with max_iterations: 1. So a compliant result fails validation and the whole review — verdict, findings, labels, risk comment — is dropped. A result that instead obeys agents/review.md's "only include fields listed below — the schema is strict" strips the fields, and the floor / provenance / degraded features are silently inert.
FEATURES.md:123 ("Add the field to schemas/<agent>-result.schema.json") was skipped, and agents/review.md's field table was not updated.
Suggestion: add to properties.risk_assessment in schemas/review-result.schema.json:
tier1_score— number,minimum: 1,maximum: 5risk_floor— integer,minimum: 1,maximum: 5degraded— string,enum: ["tier1-only"]
All three optional, so legacy and UNKNOWN results stay valid. Mirror them in agents/review.md's documented field table, and extend scripts/validate-output-schema-test.sh (which already loads REVIEW_SCHEMA at line 242) with the exact RISK_FLOORED_RESULT / RISK_DEGRADED_RESULT / RISK_BAD_PROVENANCE fixtures from post-review-test.sh, so the two suites cannot drift again.
Note: qodo-code-review flagged this same class from the prompt side at skills/pr-review/SKILL.md:634 (comment 3979232169). This entry anchors it on the file where the fix actually lands, where no comment exists.
| TIER1_SCORE=$(jq -r '.risk_assessment.tier1_score // empty' "${RESULT_FILE}") | ||
| [[ "${TIER1_SCORE}" =~ ^[1-5](\.[0-9]{1,2})?$ ]] || TIER1_SCORE="" | ||
| RISK_DEGRADED=$(jq -r '.risk_assessment.degraded // empty' "${RESULT_FILE}") | ||
| [[ "${RISK_DEGRADED}" =~ ^[a-z0-9-]{1,32}$ ]] || RISK_DEGRADED="" |
There was a problem hiding this comment.
MEDIUM — degraded is parsed and validated but never consulted when the risk label is applied
Verified at head e6026cd. RISK_DEGRADED is read and validated here at lines 440-441 (added by this PR), then referenced only twice more: line 473 appends it to the comment header via RISK_META, and line 489 puts it in the history row.
The labelling block at 451-465 keys purely on RISK_LEVEL — forge_create_label "risk/${RISK_LEVEL}" / forge_add_label_edit "risk/${RISK_LEVEL}" — with no RISK_DEGRADED check.
This PR simultaneously adds the contract "Anything that routes or gates on the score must treat degraded as 'no score'" in two added places: docs/review.md:76-77 and skills/pr-review/SKILL.md:636-637. The label is the only surface automation keys on, and a tier-1-only fallback score produces a risk/high (or any other) label byte-identical to a fully-computed one — so no consumer can honour the contract the PR just documented.
To be precise about scope: there is a visible marker — the sticky comment header carries · degraded: tier1-only. The gap is specifically the label.
Suggestion: either skip forge_create_label / forge_add_label_edit when RISK_DEGRADED is non-empty (matching the "no score" contract literally), or apply an additional risk/degraded marker label alongside the level so label consumers can tell the two apart. Add a post-review-test.sh case asserting the chosen behaviour for RISK_DEGRADED_RESULT.
|
|
||
| **`TIER1_SCORE` and `RISK_FLOOR`:** the script's last two lines are the | ||
| Tier 1 composite computed from this table and the floor — `2` when any | ||
| security-sensitive path is touched, else `1`. Use both as given; the |
There was a problem hiding this comment.
MEDIUM — this file now carries both "assign a sub-score and average" and "do not re-derive it" for Tier 1 — the exact intro/procedure split that caused the bug this PR fixes
Verified at head e6026cd. The PR adds this paragraph at lines 76-81 — "Use both as given; the script computes them so the score is the same on every run (re-deriving deterministic signals in the LLM is what made the same PR flip between 1 and 2 across re-reviews)" — and rewrites procedure step 2 at 214-217 to "Take the Tier 1 composite from the script: TIER1_SCORE is the Tier 1 composite. Do not re-derive it."
But the section intro at lines 51-54 is unchanged and still reads as an imperative:
The
risk-tier1.shscript outputs these KEY=VALUE signals. Evaluate each dimension and assign a 1-5 sub-score. Then average the dimension sub-scores for the Tier 1 composite.
An LLM reading the section header first gets exactly the instruction this PR is trying to eliminate.
Separately confirmed: skills/pr-review/SKILL.md:613-615 still describes the sub-agent's return as "a JSON object with score, level, rationale, and optional signal arrays" — it was not updated for tier1_score / risk_floor, even though the fallback text 12 lines below it was.
Suggestion: reword the lines 51-54 intro so the table reads as the rubric the script implements — e.g. "risk-tier1.sh scores these dimensions and emits the composite as TIER1_SCORE; the table below documents the rubric it applies, and is used directly only when TIER1_SCORE is UNKNOWN" — rather than deleting the table, since procedure step 2's UNKNOWN branch still needs it. Also update skills/pr-review/SKILL.md:613-615 to list tier1_score and risk_floor in the parsed payload.
| "gh label create risk/low" | ||
| run_risk_comment_test "risk-bad-provenance-not-rendered" \ | ||
| "${RISK_BAD_PROVENANCE}" \ | ||
| "**Risk Assessment: low (1/5)** |
There was a problem hiding this comment.
MEDIUM — the two new "garbage dropped" / "legacy unchanged" assertions are substring matches that pass on the very output they are meant to reject
Verified at head e6026cd; all lines below are inside the added hunk @@ -1776,6 +1783,97 @@.
1. The trailing-newline patterns collapse to plain substring matches. The assertions at 1866-1869 and 1872-1875 pass a pattern whose last character is a newline to grep -qF inside run_risk_comment_test (helper at 1789-1826). grep strips the trailing newline, so the check degrades to a plain substring match. Reproduced:
printf '**Risk Assessment: low (1/5)** · tier 1: 9;rm -rf\n' | grep -qF '**Risk Assessment: low (1/5)**
' # matches
The RISK_BAD_PROVENANCE fixture at 1862 carries tier1_score: "9;rm -rf", so the test named for proving garbage provenance is dropped would still pass if that garbage were rendered. It asserts nothing about dropping.
2. The "legacy results unchanged" case at 1872 is not actually unchanged. It uses RISK_LOW_RESULT, whose head_sha is hex and matches the RISK_HEAD regex ^[0-9a-f]{6,40}$ at post-review.src.sh:482 — so that path now also emits a <summary>History</summary> block. The substring assertion cannot see that change.
3. Five tests ship literal placeholder names: "risk-f..." at 1842 and 1848, "risk-b..." at 1863 and 1866, "risk-l..." at 1872. Both helpers derive run_dir="${TMPDIR}/run-${test_name}" and stdout-${test_name}.log, so each duplicated pair shares a working directory and log file, and the PASS/FAIL lines are ambiguous.
Suggestion: replace the newline-terminated grep -qF patterns with an anchored check — grep -qxF '**Risk Assessment: low (1/5)**' or grep -qE '^\*\*Risk Assessment: low \(1/5\)\*\*$' — so appended meta fails the test, and add explicit negative assertions that tier 1: and degraded: are absent for RISK_BAD_PROVENANCE. Give all five tests unique descriptive names. Add an assertion for whether a legacy result should now get a History block, and fix either the code or the PR description to match.
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>
|
@waynesun09 thanks — all four addressed. Pushed
Names were already unique in the current file (the |
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>
…risk branch Signed-off-by: guy oron <goron@redhat.com>
guyoron1
changed the title
…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>
318034b to
801fd8a
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings (review-only, no verdict change)
Verified at head 801fd8a8. One finding, posted inline below.
| PR_FILES=$(forge_get_pr_files) || PR_FILES="" | ||
| fi | ||
| if [ -z "${PR_FILES}" ]; then | ||
| echo "::warning::Could not fetch PR files — security floor uses the sub-agent's risk_floor only" |
There was a problem hiding this comment.
MEDIUM — Security floor fails open when the PR file list cannot be fetched on comment/request-changes actions
Verified at head 801fd8a8, in the hunk added by that commit (lines 462-487). The floor block fetches PR_FILES itself on non-approve actions (PR_FILES=$(forge_get_pr_files) || PR_FILES="" at 473) and, when the result is empty, only warns — ::warning::Could not fetch PR files — security floor uses the sub-agent's risk_floor only — and skips the SECURITY_PATTERNS loop, so RISK_FLOOR stays at whatever the sub-agent emitted (possibly empty).
The comment directly above (462-464) promises the opposite: "a sub-agent that omits or lowers risk_floor cannot disable it: a security-sensitive path never labels low". That guarantee now holds only when the fetch succeeds.
The same file already treats this exact failure as fail-closed on the approve path (235-258: retry once after sleep 10, then exit 1 with "refusing to approve"), and that block's comment cites issue #2093 — empty file lists are a known transient correlated with merge-commit updates — so the path is reachable, not hypothetical. On a comment or request-changes review of a PR touching e.g. internal/auth/, a transient empty fetch plus a sub-agent that omitted risk_floor yields a risk/low label. risk-tier1.sh:238-245 applies the repo's stated rule for the identical condition ("no file list ... never a PR with zero risky files" — fail closed, agents#1227).
No test covers this branch: MOCK_PR_FILES_FAIL is used only by the approve-path tests (post-review-test.sh:1753-1791); the floor tests at 2093-2135 exercise echoed-low, omitted, and the non-approve fetch-success case, never a fetch failure.
Lineage: the earlier qodo comment at :456 asked for the model floor to be replaced by a trusted measurement and to fail closed when that measurement is missing; 801fd8a8 honoured the first half, and this new branch at 475-476 is the unaddressed second half.
Suggestion: Mirror the approve path: retry forge_get_pr_files once after the transient-race sleep, and if it still comes back empty treat the change as security-sensitive for floor purposes (RISK_FLOOR=2, or at minimum refuse to apply risk/low and log why) instead of trusting the sub-agent's value. Add a post-review-test.sh case with MOCK_PR_FILES_FAIL=1 and a comment-action fixture that omits risk_floor, asserting risk/low is not created.
…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>
|
@waynesun09 thanks — addressed in
Upstream |
150fff7 to
d1147ed
Compare
Signed-off-by: guy oron <goron@redhat.com>
…isk comment The table re-fetched the prior sticky comment on every review and carried its rows forward. It is the least-needed part of this PR; the tier 1 value and the degraded marker stay in the comment header. Signed-off-by: guy oron <goron@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings (review-only, no verdict change)
Checked at head 624acaf9. One finding, posted inline below.
| # Provenance fields from risk-tier1.sh via the sub-agent — optional, | ||
| # validated, never interpolated raw. | ||
| RISK_FLOOR=$(jq -r '.risk_assessment.risk_floor // empty' "${RESULT_FILE}") | ||
| [[ "${RISK_FLOOR}" =~ ^[1-5]$ ]] || RISK_FLOOR="" |
There was a problem hiding this comment.
MEDIUM: the sub-agent's risk_floor is accepted up to 5, but risk-tier1.sh only emits 1 or 2, so a bad value can inflate the label
Checked at head 624acaf9. Line 461 reads RISK_FLOOR from the sub-agent JSON, and line 462 checks it only against ^[1-5]$. The schema also allows 1-5. The block at 468-503 only ever raises the floor to 2, when the recomputed security-pattern match hits or the file fetch fails. It never lowers or clamps a higher value the sub-agent reported.
risk-tier1.sh:risk_floor() (lines 208-212) can only return 1 or 2 (or UNKNOWN, which the sub-agent is told to omit). So a result such as {score: 1, tier1_score: 1.5, risk_floor: 5} on a clean-path PR reaches lines 504-510. RISK_SCORE is raised to 5, RISK_LEVEL becomes critical, and the log says floored to 5 (security-sensitive path). That message is false, because no security path was touched. The comment at 468-470 covers only the downward case (a sub-agent "that omits or lowers risk_floor cannot disable it").
Impact is limited. The sub-agent already controls score and level directly, so this does not cross a new trust boundary. The problem is that the "deterministic floor" can hold a value the script never produces, and it is reported under a misleading reason. The existing threads at :462 and :476 cover only the fail-open (downward) direction.
Suggestion: treat the floor as locally owned. Either clamp the sub-agent value to {1,2} (for example [[ "${RISK_FLOOR}" =~ ^[12]$ ]] || RISK_FLOOR=""), or ignore it and derive RISK_FLOOR only from the recomputed SECURITY_PATTERNS match or the fetch-failure path, keeping the reported value for logging only. Consider narrowing the risk_floor enum in schemas/review-result.schema.json to [1, 2] to match. Add a post-review-test.sh case with risk_floor: 5 on a non-security file list, asserting that risk/critical is not applied.
This branch has not been deployed
2 participants
ADR 0089 leaves tier 1 to
risk-tier1.shbut has the sub-agent re-emit it, which is how the same PR scored 1, 2, 1 across three re-reviews (#1037; #1038 now anchors re-reviews to the prior score in the prompt). This makes the score reproducible and never silently missing, without changing the model:risk-tier1.shends withTIER1_SCOREandRISK_FLOOR— the SKILL.md table computed once, deterministically. The sub-agent copies them intotier1_score/risk_floorinstead of re-deriving.RISK_FLOORis 2 whenever a security-sensitive path is touched.post-review.shrecomputes it from the changed paths, sorisk/lowcannot land on a security PR whatever the LLM returned. Over the 246 production PRs measured on Add PR-level risk assessment score to the review pipeline fullsend#4698, no score-1 PR touched such a path — the floor changes nothing today and closes the gap for later.max(round(TIER1_SCORE), RISK_FLOOR)withdegraded: "tier1-only"instead of dropping the score. Four production reviews since 08-25 lost their score toclaude-sonnet-4-5@20250929 is not available(Sonnet 4.5 model unavailable on Vertex causes review sub-agent failures and wasted retries fullsend#6922, now closed); the fallback covers the next outage like it, and scores are still missing on about 15% of PRs (Risk-assessment review sub-agent silently omits its comment on ~15% of PRs (e.g. #7385) fullsend#7387). Consumers must treatdegradedas no score.post-review.shwhen the file list cannot be fetched.Tests:
risk-tier1-test.shcovers tier-1 scoring (including the SKILL.md anchoring example), the size composite, source detection, the risk floor, and the empty-file-list case.post-review-test.shcovers the floor raising and never lowering, the floor recomputed from the changed paths, fail-closed and retry when the file list cannot be fetched, the degraded header and marker label, garbage provenance ignored, and legacy results unchanged.make check-bundle, shellcheck, andmake lintare green, with no skillsaw ceiling changes.Refs #1037, #1227, fullsend-ai/fullsend#4698.