diff --git a/.github/workflows/dev-lead-reusable.yml b/.github/workflows/dev-lead-reusable.yml index 306726857..da5683c7f 100644 --- a/.github/workflows/dev-lead-reusable.yml +++ b/.github/workflows/dev-lead-reusable.yml @@ -225,11 +225,23 @@ jobs: - name: Parse intent context fields run: | ctx="${INTENT_CONTEXT:-{\}}" + # #1919: the triggering comment's node id, for fix-bot-comment's disposition. + # Only a well-formed GraphQL node id is exported: base64/base64url characters, + # including legacy padding (=, +, /). Never a newline or quote. + _cnid=$(echo "$ctx" | jq -r '.comment_node_id // ""') + [[ "$_cnid" =~ ^[-A-Za-z0-9_+/=]+$ ]] || _cnid="" + # fix-bot-comment dispositions the notice BY this id. Without it the run can only + # no-op and leave the notice blocking, so fail loudly instead (#1928 review). + if [ "${INTENT_TYPE:-}" = "fix-bot-comment" ] && [ -z "$_cnid" ]; then + echo "::error::fix-bot-comment: the triggering comment's node id is missing or malformed; refusing to run a fix-bot-comment that cannot disposition its notice" + exit 1 + fi echo "INTENT_PR_NUMBER=$(echo "$ctx" | jq -r '.pr_number // ""')" >> "$GITHUB_ENV" echo "INTENT_HEAD_SHA=$(echo "$ctx" | jq -r '.head_sha // ""')" >> "$GITHUB_ENV" echo "INTENT_CHECKS=$(echo "$ctx" | jq -c '.checks // []')" >> "$GITHUB_ENV" echo "INTENT_ISSUE_NUMBER=$(echo "$ctx" | jq -r '.issue_number // ""')" >> "$GITHUB_ENV" echo "INTENT_ACTOR=$(echo "$ctx" | jq -r '.actor // ""')" >> "$GITHUB_ENV" + echo "INTENT_COMMENT_NODE_ID=${_cnid}" >> "$GITHUB_ENV" # #1767: a hold-label skip carries the blocking label + subject number so # the workflow can post the one-time "held" notice (empty for non-holds). echo "INTENT_HOLD_LABEL=$(echo "$ctx" | jq -r '.hold_label // ""')" >> "$GITHUB_ENV" @@ -398,7 +410,9 @@ jobs: HEAD_SHA: ${{ env.INTENT_HEAD_SHA }} REPO: ${{ github.repository }} REVIEW_ENGINE: ${{ env.DEV_LEAD_ENGINE }} + ACTOR: ${{ env.INTENT_ACTOR }} COMMENT_BODY: ${{ env.INTENT_BODY }} + COMMENT_NODE_ID: ${{ env.INTENT_COMMENT_NODE_ID }} CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} GOOGLE_API_KEY: ${{ secrets.GOOGLE_API_KEY }} GEMINI_API_KEY: ${{ secrets.GOOGLE_API_KEY }} diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 64914aa38..92431f1ee 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -6,6 +6,7 @@ on: - 'scripts/**' - '.github/workflows/**' - 'tests/**' + - 'prompts/**' - 'agents/**' - 'personas/**' - 'interaction-contracts/**' @@ -22,6 +23,7 @@ on: paths: - 'scripts/**' - 'tests/**' + - 'prompts/**' - '.github/workflows/**' workflow_dispatch: @@ -185,6 +187,7 @@ jobs: tests/pr_review_canary.bats \ tests/dev-lead/unit/test_maintainer_review_thread_gate.bats \ tests/dev-lead/unit/test_maintainer_comment_gate.bats \ + tests/dev-lead/unit/test_agent_marker_convention.bats \ tests/dev-lead/unit/test_maintainer_resolve_comment.bats \ tests/dev-lead/unit/test_comment_disposition_verify.bats \ tests/dev-lead/unit/test_conflict_integrity.bats \ diff --git a/AGENTS.md b/AGENTS.md index 37ecd242a..fd409cb82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -240,6 +240,12 @@ calling it a regression, and treat CI as authoritative.** updates one rolling PR (`chore/readme-refresh`, label `readme-refresh`) per repo for human review. It never direct-pushes. It must not be removed by template syncs. If the org template gains an equivalent, remove this exception and defer to the template instead. +- **Exception:** The `pull_request`/`push` `paths:` filters in `lint.yml` are extended with `prompts/**` + beyond the org template baseline (#1919). The dev-lead prompt files under `prompts/dev-lead/` are now + guarded by bats tests in the lint suite (e.g. `tests/dev-lead/unit/test_agent_marker_convention.bats`), + so a prompt-only change must trigger this workflow or the guard never runs and the CI gap it closes + silently reopens. Template syncs must not drop `prompts/**` from these path filters. If the org template + adds `prompts/**` to its baseline, remove this exception and defer to the template instead. - **Exception:** The `bats` test list in `lint.yml` is extended with repo-specific test files (e.g. `tests/test_push_protection.bats`) beyond the org template baseline. When adding new test files to this repo, add them to this list. Template syncs must not reset it to the base template list. This includes diff --git a/prompts/dev-lead/fix-bot-comment.md b/prompts/dev-lead/fix-bot-comment.md index 709533e17..e0e11e996 100644 --- a/prompts/dev-lead/fix-bot-comment.md +++ b/prompts/dev-lead/fix-bot-comment.md @@ -1,4 +1,4 @@ - + # Dev-Lead Agent: Fix Bot Comment Issues You are the dev-lead agent for the `${REPO}` repository. Your task is to address issues raised by an automated code analysis bot on a pull request. @@ -110,6 +110,36 @@ If `${ACTOR}` is `sonarqubecloud[bot]` and the comment reports security hotspots 3. Fix each identified hotspot — for `curl | bash` patterns, replace with a safer alternative such as a pinned binary download with SHA verification, `gh extension install /`, or a package manager install 4. If no hotspot is found in changed files, read any newly introduced shell scripts or workflow YAML steps for the patterns above +## Non-actionable bot notices — disposition the ORIGINAL comment, never leave it undispositioned (#1919) + +Some bot comments are pure **operational notices**, not code findings: a rate-limit / "review limit reached" notice, a trial-ended or usage-limit notice, or a clean status re-post (e.g. SonarCloud's `Quality Gate passed`). There is nothing to fix in the diff for these — but the bot's **original PR issue comment** is still subject to the **maintainer-comment gate**, which withholds pr-review's approval while any PR issue comment lacks a **verified disposition**. You run as the owner account `don-petry` — the *same* login a human maintainer uses — and the gate discriminates by **marker, not author**. Two cases follow, and the difference matters: + +1. **Registered clean-status re-post → post nothing, it is auto-cleared.** This applies **only** when the notice matches its source's *full* `info_status_pattern` in `scripts/lib/reviewer-sources.tsv`. That is what the gate's classifier checks (#1918). For SonarCloud this means the `**Quality Gate passed**` headline **and** the `[0 New issues]` **and** `[0 Security Hotspots]` lines. A "Quality Gate passed" comment that still lists new issues or security hotspots is **not** clean: it stays a blocker, and its issues or hotspots are findings to address (see the SonarCloud guidance above). Handle it as **findings only**: fix them, or reply with specifics. **Never** give it a case-2 `informational` disposition, because that would minimize real findings as resolved without addressing them. It is never case 1 either. When the notice does match the full pattern, the gate already treats it as addressed. Do not reply; record the acknowledgement in your **output summary** below (it lands in the run/step summary), not on the PR conversation. + +2. **Every other notice → you MUST disposition the ORIGINAL comment.** A trial-ended, usage-limit, or rate-limit notice is **not** a registered clean-status pattern, so the gate still counts the bot's original comment as an **undispositioned** blocker. **Suppressing your reply does NOT clear it, and neither does a separate `` comment** — an ack only marks the *new* comment as agent-authored; the *original* bot comment stays undispositioned and keeps blocking the very approval it was posted to unblock (exactly the #1919 loop). Only a **verified disposition on the original comment** clears it. Post **exactly one** reply that names what the notice is and why no code change is needed, ending with a single disposition marker tied to the original comment's node id: + + ``` + + ``` + + The harness verifies the disposition and minimizes the original comment RESOLVED (#1813) — that is what actually clears the gate. Do **not** call `minimizeComment` yourself. This is the same issue-comment disposition flow documented in `fix-reviews.md` Phase 1b (`scripts/lib/comment-disposition-verify.sh` is the normative parser); `informational` requires only a non-empty reply body, so no `sha=` is needed. + + The triggering notice's node id is **`${COMMENT_NODE_ID}`**, taken from the webhook event, so there is nothing to search for. **Never** paste the comment body into a shell command. It is untrusted bot text that may contain quotes, backticks or `$(…)`, which would break the command or execute. Before dispositioning, confirm that id is still the right target: authored by `${ACTOR}` and not already minimized `RESOLVED`. If `${COMMENT_NODE_ID}` is empty, or the check fails, **do not guess**. Post nothing, and record in your output summary that the notice could not be dispositioned automatically. + + ```bash + node_id='${COMMENT_NODE_ID}' + [ -n "$node_id" ] || { echo "no triggering comment node id — not dispositioning" >&2; exit 1; } + meta=$(gh api graphql -f query='query($id:ID!){ node(id:$id){ ... on IssueComment { author{login} isMinimized minimizedReason } } }' -f id="$node_id") + author=$(echo "$meta" | jq -r '.data.node.author.login // ""') + min=$(echo "$meta" | jq -r '(.data.node.isMinimized // false) and ((.data.node.minimizedReason // "") | ascii_upcase) == "RESOLVED"') + actor='${ACTOR}' + { [ "$author" = "$actor" ] || [ "$author" = "${actor%\[bot\]}" ]; } \ + || { echo "node $node_id is authored by '$author', not '$actor' — not dispositioning" >&2; exit 1; } + [ "$min" != "true" ] || { echo "node $node_id is already minimized RESOLVED — nothing to do"; exit 0; } + # Post the disposition reply on the PR (the body is yours, never the notice text): + # gh pr comment "${PR_NUMBER}" --repo "${REPO}" --body "…" + ``` + ## Constraints - Only fix issues that are clearly actionable from the bot's output diff --git a/prompts/dev-lead/review-changes.md b/prompts/dev-lead/review-changes.md index bc54627fe..c2102e7cb 100644 --- a/prompts/dev-lead/review-changes.md +++ b/prompts/dev-lead/review-changes.md @@ -56,7 +56,8 @@ Identify any entries with `state` = `"CHANGES_REQUESTED"`. Each is a **Tier 1 bl When you fix a failing check (a Tier 1 CI blocker), your posted fix comment must state, in one line, **what the failing check verifies** and **why this diff makes that true** — **not just that the check now passes**. A red check exists to protect something (an invariant, a contract, a behavior); the fix must make *that thing* true, not merely flip the check green by changing what the check measures. Stating the intent gives a reviewer (or `pr-review`) a concrete claim to spot-check instead of a bare before/after CI status, and guards against the "agent optimizes the check, not the intent" failure mode (`docs/agentic-interaction-model.md` §13). - Post this as a PR comment (`gh pr comment "${PR_NUMBER}" --repo "${REPO}"` — target the PR and repo explicitly so it works even in detached HEAD or isolated worktree checkouts) for a failing check that has no associated review thread; when the failing check *is* referenced by a review thread, put the same one-line statement in that thread reply. -- Concrete shape: "`` verifies ``; this diff makes that true by `` (not by silencing/relaxing the check)." Example: "`bats` verifies the test tooling is installed the documented way; this diff removes the vendored `node_modules/bats/` because `lint.yml` already installs bats via `apt-get` — the tool was already available, so the fix is deletion, not re-vendoring." +- **End the PR comment with the `` marker (#1919).** You run as the owner account `don-petry` — the *same* login a human maintainer uses — so the **maintainer-comment gate** (which withholds pr-review's approval while any PR issue comment lacks a verified disposition) discriminates your comments from a maintainer's by **marker, not author**. An unmarked note is counted as a fresh **undispositioned** finding and blocks the very approval this PR needs. The marker makes the gate recognise the comment as agent-authored and skip it. (A reply *inside* a review thread does not need this marker — the gate only scans top-level issue comments.) +- Concrete shape — the one-line statement, then the marker on its own final line: "`` verifies ``; this diff makes that true by `` (not by silencing/relaxing the check)." followed by ``. Example statement: "`bats` verifies the test tooling is installed the documented way; this diff removes the vendored `node_modules/bats/` because `lint.yml` already installs bats via `apt-get` — the tool was already available, so the fix is deletion, not re-vendoring." - If the only way you can make the check green is to change what the check asserts (edit its threshold, its fixture, its expected output, or the assertion itself), **stop** — that is the failure mode this guards against. Fix the root cause the check protects, or leave the check red and explain why in your comment. (One exception: a check that is itself stale or incorrect — asserting something no longer true of the intended behavior. There, correcting the check *is* the root-cause fix, but say so explicitly: state what the check should protect and why its current assertion is wrong, per `docs/agentic-interaction-model.md` §13.3.) ### Phase 1 — Address Threads diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index e85436ded..e7451adbc 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -2119,7 +2119,7 @@ case "$INTENT_TYPE" in ;; fix-bot-comment) export PR_NUMBER PR_URL="https://github.com/${REPO}/pull/${PR_NUMBER}" - export REPO ACTOR="${ACTOR:-}" COMMENT_BODY="${COMMENT_BODY:-}" HEAD_SHA + export REPO ACTOR="${ACTOR:-}" COMMENT_BODY="${COMMENT_BODY:-}" COMMENT_NODE_ID="${COMMENT_NODE_ID:-}" HEAD_SHA fetch_pr_context rc=0 build_and_run "fix-bot-comment" || rc=$? diff --git a/scripts/dev-lead-intent.sh b/scripts/dev-lead-intent.sh index b6f782240..1e57b0dce 100755 --- a/scripts/dev-lead-intent.sh +++ b/scripts/dev-lead-intent.sh @@ -431,6 +431,9 @@ case "$EVENT_NAME" in comment_body=$(jq -r '.comment.body // empty' "$EVENT_PATH" 2>/dev/null || true) pr_number=$(jq -r '.issue.number // empty' "$EVENT_PATH" 2>/dev/null || true) author_assoc=$(jq -r '.comment.author_association // empty' "$EVENT_PATH" 2>/dev/null || true) + # The triggering comment's GraphQL node id — fix-bot-comment targets its + # disposition by id instead of re-matching the raw body in a shell command. + comment_node_id=$(jq -r '.comment.node_id // empty' "$EVENT_PATH" 2>/dev/null || true) # Rebase sentinel check (highest priority, before bot-skip) if echo "$comment_body" | grep -qF "` agent marker so the maintainer-comment gate +# (scripts/lib/maintainer-comment-gate.sh) recognises it as agent-authored and +# does NOT count it as an undispositioned finding. +# +# dev-lead commits and comments as `don-petry` (a human login), and the gate +# discriminates by MARKER, not author. So an UNMARKED acknowledgement/note is +# indistinguishable from a maintainer finding and becomes a fresh gate blocker on +# the very PR it was posted to unblock. The marker-prefixed paths (fix-reviews +# Phase 1b disposition, hold-notice, rate-limit-ack) are already correct; this +# guards the paths that historically bypassed the convention. + +PROMPTS_DIR="$(cd "$BATS_TEST_DIRNAME"/../../.. && pwd)/prompts/dev-lead" + +# The gate's agent-marker regex only matches HTML markers of the form +# ``. A prompt that tells the agent to `gh pr comment` must +# therefore also tell it to stamp such a marker. +# +# These assertions tie the marker to the comment-emitting instruction by scanning +# the context window around EACH `gh pr comment` line independently — every +# occurrence must carry the marker within its own window. A whole-file grep (or one +# that concatenates every window before matching) would let a single marked example +# vouch for an unmarked sibling instruction: a later prompt change could add a new +# unmarked `gh pr comment` and the assertion would stay green because some other +# block still holds the marker — the exact regression #1919 guards against. + +# assert_each_pr_comment_marked FILE MARKER +# Fails unless there is at least one `gh pr comment` occurrence AND every one has +# MARKER within its surrounding context window (2 lines before, 6 after), checked +# per occurrence so no single marked block can vouch for an unmarked sibling. +assert_each_pr_comment_marked() { + # Each `gh pr comment` instruction must claim its OWN marker line inside its window + # (2 lines before, 6 after). A marker already claimed by an earlier instruction + # cannot vouch for a later one, so two adjacent instructions whose windows overlap + # still need two distinct markers (#1928 review). + local file="$1" marker="$2" + local found=0 lineno start end total m pick claimed=" " + total=$(wc -l < "$file") + local marker_lines + marker_lines=$(grep -nF "$marker" "$file" | cut -d: -f1) + while IFS=: read -r lineno _; do + [ -n "$lineno" ] || continue + found=1 + start=$(( lineno - 2 < 1 ? 1 : lineno - 2 )) + end=$(( lineno + 6 > total ? total : lineno + 6 )) + pick="" + for m in $marker_lines; do + if [ "$m" -ge "$start" ] && [ "$m" -le "$end" ] && [[ "$claimed" != *" $m "* ]]; then + pick="$m"; break + fi + done + [ -n "$pick" ] || return 1 + claimed="${claimed}${pick} " + done < <(grep -nF 'gh pr comment' "$file") + [ "$found" -eq 1 ] +} + +@test "helper: two adjacent gh pr comment instructions sharing ONE marker fail" { + local f="$BATS_TEST_TMPDIR/two-one.md" + printf '%s\n' 'gh pr comment 1 --body "a "' 'gh pr comment 1 --body "b"' > "$f" + run assert_each_pr_comment_marked "$f" '' + [ "$status" -ne 0 ] +} + +@test "helper: two adjacent gh pr comment instructions each with a marker pass" { + local f="$BATS_TEST_TMPDIR/two-two.md" + printf '%s\n' 'gh pr comment 1 --body "a "' 'gh pr comment 1 --body "b "' > "$f" + run assert_each_pr_comment_marked "$f" '' + [ "$status" -eq 0 ] +} + +@test "fix-bot-comment: case 1 (post nothing) requires the FULL info-status pattern, not the headline" { + # Bind all three requirements to the case-1 paragraph itself, not the whole file. + local case1 + case1=$(grep -F '1. **Registered clean-status re-post' "$PROMPTS_DIR/fix-bot-comment.md") + [ "$(printf '%s\n' "$case1" | grep -c .)" -eq 1 ] + [[ "$case1" == *'[0 New issues]'* ]] + [[ "$case1" == *'[0 Security Hotspots]'* ]] + [[ "$case1" == *'is **not** clean'* ]] + # A pass-with-findings must never be dispositioned informational (case 2) (#1928 review). + [[ "$case1" == *'**Never** give it a case-2 `informational` disposition'* ]] +} + +@test "review-changes.md failing-check PR comment instruction carries a dev-lead marker (#1919)" { + # The failing-check fix note is posted as a PR issue comment; every gh pr comment + # instruction that emits it must carry the check-fix marker in its own window. + run assert_each_pr_comment_marked "$PROMPTS_DIR/review-changes.md" '' + [ "$status" -eq 0 ] +} + +@test "fix-bot-comment.md dispositions non-actionable bot-notice comments (#1919)" { + # A non-actionable bot issue-comment notice (trial-ended, usage-limit, rate-limit) + # is NOT auto-cleared by the info-status classifier, so a plain ack or suppression + # leaves the ORIGINAL comment an undispositioned maintainer-gate blocker. The prompt + # must instead disposition the original via the verified comment-disposition marker, + # stamped on each gh pr comment instruction that emits the disposition reply. + run assert_each_pr_comment_marked "$PROMPTS_DIR/fix-bot-comment.md" ' marker, does NOT increment the undispositioned count → 0. +@test "AC3: dev-lead :ack marked acknowledgement is ignored → 0" { + local json='{"comments":[{"author":{"login":"don-petry"},"body":"Acknowledged — this is a CodeRabbit rate-limit notice, not an actionable finding. No action needed.\n","isMinimized":false,"minimizedReason":""}]}' + _run_check "$json" + [ "$status" -eq 0 ] +} + +# AC3: the failing-check fix note dev-lead posts as a PR comment (review-changes.md) +# carries the marker and is excluded → 0. +@test "AC3: dev-lead :check-fix marked comment is ignored → 0" { + local json='{"comments":[{"author":{"login":"don-petry"},"body":"`bats` verifies the test tooling is installed the documented way; this diff removes the vendored node_modules/bats.\n","isMinimized":false,"minimizedReason":""}]}' + _run_check "$json" + [ "$status" -eq 0 ] +} + +# AC3 (the defect): the exact UNMARKED prose acknowledgement from the #1919 +# evidence still blocks — it is the MISSING marker, not the author, that let it +# leak. This is what the marker convention on the emitting paths prevents. +@test "AC3: an UNMARKED 'Acknowledged — …' prose comment still blocks → 1" { + local json='{"comments":[{"author":{"login":"don-petry"},"body":"Acknowledged — this is a Qodo trial-ended/billing notice, not a code finding. No action needed.","isMinimized":false,"minimizedReason":""}]}' + _run_check "$json" + [ "$status" -eq 1 ] +} + @test "Runtime: malformed snapshot → 2 (fail closed)" { _run_check 'not json at all {' [ "$status" -eq 2 ]