Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/workflows/dev-lead-reusable.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 }}
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
- 'scripts/**'
- '.github/workflows/**'
- 'tests/**'
- 'prompts/**'
Comment thread
don-petry marked this conversation as resolved.
- 'agents/**'
- 'personas/**'
- 'interaction-contracts/**'
Expand All @@ -22,6 +23,7 @@
paths:
- 'scripts/**'
- 'tests/**'
- 'prompts/**'
- '.github/workflows/**'
workflow_dispatch:

Expand Down Expand Up @@ -185,6 +187,7 @@
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 \
Comment thread
don-petry marked this conversation as resolved.
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
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 \
Expand Down Expand Up @@ -269,7 +272,7 @@
timeout-minutes: 5
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
# NOTE: persist-credentials is intentionally left at the default (true) —

Check warning on line 275 in .github/workflows/lint.yml

View workflow job for this annotation

GitHub Actions / Lint

275:9 [comments-indentation] comment not indented like content
# vci_resolve_reusable needs `git fetch origin` to authenticate against this
# private repo when resolving same-repo channel tags. Setting
# persist-credentials: false would silently push every such caller into the
Expand Down
6 changes: 6 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 31 additions & 1 deletion prompts/dev-lead/fix-bot-comment.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<!-- VARIABLES: PR_NUMBER, PR_URL, REPO, ACTOR, COMMENT_BODY, HEAD_SHA, CI_STATUS_JSON, ALL_REVIEWS_JSON -->
<!-- VARIABLES: PR_NUMBER, PR_URL, REPO, ACTOR, COMMENT_BODY, COMMENT_NODE_ID, HEAD_SHA, CI_STATUS_JSON, ALL_REVIEWS_JSON -->
# 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.

Expand Down Expand Up @@ -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 <owner>/<repo>`, 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 `<!-- dev-lead:ack -->` 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:

```
<!-- dev-lead:comment-disposition id=<comment_node_id> disposition=informational -->
```

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.
Comment thread
don-petry marked this conversation as resolved.

```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")
Comment thread
don-petry marked this conversation as resolved.
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 "…<!-- dev-lead:comment-disposition id=$node_id disposition=informational -->"
```

## Constraints

- Only fix issues that are clearly actionable from the bot's output
Expand Down
3 changes: 2 additions & 1 deletion prompts/dev-lead/review-changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: "`<check>` verifies `<invariant/behavior>`; this diff makes that true by `<root-cause change>` (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 `<!-- dev-lead:check-fix -->` 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: "`<check>` verifies `<invariant/behavior>`; this diff makes that true by `<root-cause change>` (not by silencing/relaxing the check)." followed by `<!-- dev-lead:check-fix -->`. 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
Expand Down
2 changes: 1 addition & 1 deletion scripts/dev-lead-fix-reviews.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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=$?
Expand Down
6 changes: 5 additions & 1 deletion scripts/dev-lead-intent.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 "<!-- auto-rebase-conflict:"; then
Expand All @@ -449,7 +452,8 @@ case "$EVENT_NAME" in
--argjson pr_number "${pr_number:-0}" \
--arg actor "${commenter:-}" \
--arg body "${comment_body:-}" \
'{"pr_number":$pr_number,"actor":$actor,"body":$body}')
--arg comment_node_id "${comment_node_id:-}" \
'{"pr_number":$pr_number,"actor":$actor,"body":$body,"comment_node_id":$comment_node_id}')

if is_trusted_bot "$commenter"; then
# Authorship gate (#1311): a bot comment only drives dev-lead's auto-fix/
Expand Down
Loading
Loading