diff --git a/.github/workflows/qodo-gate.yml b/.github/workflows/qodo-gate.yml index 9ba5a2a7b0..8af61d4583 100644 --- a/.github/workflows/qodo-gate.yml +++ b/.github/workflows/qodo-gate.yml @@ -83,9 +83,20 @@ jobs: exit 0 fi - # Any review by the bot counts — the first pass reviews the whole - # diff, and its later in-place updates edit the same review object, - # so one review object existing == the PR has been Qodo-reviewed. + # Qodo signals a finished review in one of two ways, and a clean PR + # only ever produces the second: + # + # 1. a review OBJECT — created when it has inline comments to hang + # on the diff. Its later in-place updates edit the same object, + # so one existing == the PR has been reviewed. + # 2. an issue COMMENT headed "Code Review by Qodo" — which is ALL + # a zero-finding PR gets. Verified on #2269: "Bugs (0), Rule + # violations (0), Requirement gaps (0)", and no review object + # at all. + # + # Counting only (1) therefore blocks every clean PR forever — the + # gate can never go green precisely when there is nothing to fix. + # # Paginated: a busy PR accumulates well over 100 review objects # (every inline reply wraps itself in one), and the bot's first # review is the OLDEST — exactly what a last-100 window loses @@ -96,13 +107,27 @@ jobs: --paginate --jq ".[] | select(.user.login | startswith(\"$BOT\")) | .id" | wc -l) - if [ "$reviewed" -eq 0 ]; then + + # Match the review header specifically, NOT merely "a comment by the + # bot". Qodo also posts "Qodo is busy working" (a placeholder before + # it has read anything), "PR Summary by Qodo" (/describe output, not + # a review), and "failed to apply 'local' repo settings" (an error). + # Treating any of those as a review would let the gate pass on a PR + # the agent never actually reviewed. + commented=$(gh api "repos/$REPO_OWNER/$REPO_NAME/issues/$PR/comments" \ + --paginate --jq ".[] + | select(.user.login | startswith(\"$BOT\")) + | select(.body | test(\"Code Review by Qodo\"; \"i\")) + | .id" | wc -l) + + if [ "$reviewed" -eq 0 ] && [ "$commented" -eq 0 ]; then echo "FAIL: no Qodo review on this PR yet — it reviews new PRs" echo "automatically within a couple of minutes; comment /review to" echo "summon one, then re-run this check once it answers. (Outage?" echo "Apply the skip-qodo-gate label.)" exit 1 fi + echo "review found: $reviewed review object(s), $commented review comment(s)" # Unresolved Qodo threads, paginated (a long-lived PR can exceed one # 100-thread page; a truncated read must never produce a false pass). diff --git a/.pr_agent.toml b/.pr_agent.toml index 12aa2c13fa..e87a332d4f 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -5,6 +5,21 @@ # pr_compliance_checklist.yaml - binary hard gates # # Reference: https://docs.qodo.ai/install-and-configure/configuration-overview/configuration-file +# +# CAUTION - do not name the dynamic linker's library-injection environment +# variable in this file, in any case, and not inside a longer word either. +# PR-Agent parses this file with a hardened Dynaconf loader that rejects the +# directives capable of executing code or reading arbitrary files, and that +# variable's name ends in one of them. Matching is on the bare token, so the +# full variable name trips it even though it is only prose in a guideline. +# +# The failure mode is silent in the worst way: the agent posts "failed to apply +# 'local' repo settings / Forbidden directive", then reviews the PR with its +# stock prompt - so every guideline below is ignored while the review still +# looks normal. That is exactly what happened on #2268, which added this file. +# +# Write such rules in best_practices.md instead. It is plain markdown, is under +# no such restriction, and already states this one in full in section 4.1. [github_app] pr_commands = [ @@ -59,9 +74,11 @@ Prioritise, in order: majestic streamer belong to majestic's maintainers. Redirecting a contributor is a normal, useful review outcome; say which repo and why. -5. Monkey-patching in place of a fix. LD_PRELOAD shims are not acceptable anywhere in the - OpenIPC tree, in firmware or in builder. Neither is a kernel module that rewrites a - vendor blob's memory at runtime, nor a generated facade library checked in as a binary. +5. Monkey-patching in place of a fix. Dynamic-linker injection shims - a library forced + ahead of the real one through the linker's LD_* environment knob, spelled out in + best_practices.md section 4.1 - are not acceptable anywhere in the OpenIPC tree, in + firmware or in builder. Neither is a kernel module that rewrites a vendor blob's memory + at runtime, nor a generated facade library checked in as a binary. Each of these is bound to one exact build of one blob and fails silently at the next vendor drop. @@ -89,6 +106,7 @@ Do not raise a finding for pre-existing code that the diff merely moves or reind Note especially: a device-specific value added to general/overlay/ or to a shared load_ script fails the blast-radius gate even when it is correct for the board the contributor tested, because it changes the default for cameras already in the field. And -LD_PRELOAD has no sanctioned use in this tree - a majestic bug that appears to need one is -a majestic issue, not a firmware change. +dynamic-linker injection shims (best_practices.md section 4.1) have no sanctioned use in +this tree - a majestic bug that appears to need one is a majestic issue, not a firmware +change. """