feat(risk): compute tier 1 in the script, floor on security paths, degrade instead of drop, keep history - #1245
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}
| RISK_FLOOR=$(jq -r '.risk_assessment.risk_floor // empty' "${RESULT_FILE}") | ||
| [[ "${RISK_FLOOR}" =~ ^[1-5]$ ]] || RISK_FLOOR="" |
There was a problem hiding this comment.
1. Security changes can receive low risk 📜 Skill insight ≡ Correctness
post-review.sh treats the optional model-emitted risk_floor as authoritative after checking only that it is a digit from 1 to 5, without verifying it against changed paths or trusted Tier 1 output. If the field is omitted or set to 1 for a security-sensitive change, the floor branch is skipped and the unchanged score proceeds to risk labeling and the sticky comment.
Agent Prompt
## Issue description
The security floor is accepted from optional, model-emitted JSON rather than independently derived from trusted path analysis, so a missing, malformed, conflicting, or incorrectly low value can bypass the security-path guard.
## Fix Focus Areas
- scripts/post-review.src.sh[433-449]
- scripts/post-review.sh[843-859]
- scripts/post-review-test.sh[1829-1844]
- skills/pr-risk-assessment/scripts/risk-tier1.sh[207-212]
- skills/pr-risk-assessment/scripts/risk-tier1.sh[229-289]
- skills/pr-review/sub-agents/risk-assessment.md[26-31]
## Recommended Fix
Obtain the authoritative floor outside the sub-agent by consuming trusted Tier 1 output, rerunning `risk-tier1.sh` in the posting path, or independently inspecting trusted changed-path metadata. Compare any submitted model field with the trusted value and enforce the trusted floor; if the trusted measurement is missing, malformed, or conflicting, fail closed by withholding a low-risk label rather than allowing the model value to reduce protection. Regenerate `scripts/post-review.sh` from its source and add tests covering omitted and incorrectly low model-provided floors.
ⓘ 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) |
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
| case "${file##*/}" in | ||
| *.md|*.markdown|*.rst|*.adoc|*.txt|*.yaml|*.yml|*.json|*.toml|*.ini|*.cfg|*.conf|LICENSE*|CODEOWNERS|.gitignore|.gitattributes|.editorconfig) ;; | ||
| *) echo "true"; return ;; |
There was a problem hiding this comment.
4. Config-only changes get inflated risk 📜 Skill insight ≡ Correctness
has_source_files treats every basename outside a short extension list as source, so config files such as .env and .properties are not recognized as config-only. A config-only pull request with no tests then takes the source-path 0.00 score of 5 instead of the documented neutral score of 1, inflating the Tier 1 composite.
Agent Prompt
## Issue description
The new source-file classifier does not cover common configuration filenames and extensions, contradicting the documented neutral treatment of config-only changes.
## Fix Focus Areas
- skills/pr-risk-assessment/scripts/risk-tier1.sh[136-147]
- skills/pr-risk-assessment/scripts/risk-tier1.sh[191-196]
- scripts/risk-tier1-test.sh[332-338]
## Recommended Fix
Expand the non-source classification using the repository's established configuration-file patterns, including extensionless and dotfile configurations where appropriate. Add tests showing representative config-only changes receive the neutral test-ratio score while mixed source changes do not.
ⓘ 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
…grade instead of drop, keep 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 |
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). This makes the score gate-worthy 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.shenforces it from the echoed field, 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(the error behind Sonnet 4.5 model unavailable on Vertex causes review sub-agent failures and wasted retries fullsend#6922, visible from 08-25). Consumers must treatdegradedas no score.Tests:
risk-tier1-test.sh(+15:score_tier1,_score_size,has_source_files,risk_floor, both e2e fixtures, empty-list fail-closed),post-review-test.sh(+11: floor raises / never lowers, degraded header, history row, garbage provenance dropped, legacy results unchanged).make check-bundle, shellcheck,make lintgreen (two context-budget ceilings bumped for the added prose).Refs #1037, #1227, fullsend-ai/fullsend#4698.