diff --git a/hooks/hooks.json b/hooks/hooks.json index 3f274e390..2839d210d 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -362,10 +362,17 @@ "scripts/check-pr-fully-clean.py matches that emoji as a", "REVIEW_BODY_MARKERS entry, so a disclosed claim comment would be", "admitted into the fully-clean verdict scan as a review -- and a claim", - "carries no findings, so it would scan as a CLEAN one. That is the", + "carries no findings, so nothing blocking is found there. That is the", "false-clean failure shared/workflow/fully-clean.md already records for", "a human-authored self-review, arriving through the very mechanism", - "added to make authorship legible." + "added to make authorship legible.", + "", + "Admission is necessary and not sufficient: the comment must", + "also name the head SHA to count toward criterion 2. The emoji", + "removes the one filter standing between a claim comment and", + "that scan, rather than single-handedly manufacturing a clean", + "verdict. Corrected after a cross-vendor review; the fragment", + "and the hook's user-facing warning say the same thing now." ], "_note_cannot_see_every_body": [ "--body-file, --editor, `-F ` and an interpolated $BODY all put the", diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index f9daa9faa..fb4ee94a1 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -74,6 +74,15 @@ r"(?:[A-Za-z_][A-Za-z0-9_]*=\S*\s+)*" ) +# Tail of one command, for a lookahead that must cross a line continuation. +# NOTE: the close/reopen verbs above are gated over the WHOLE SEGMENT by +# `CLOSE_REOPEN_RE` + `COMMENT_FLAG_RE` below, not by a lookahead. A raw +# `[^\\n;&|]` tail is not quote-aware, so +# `gh issue close 5 --duplicate-of "see #3; also #4" --comment "..."` went +# undetected -- the `;` inside a quoted value ended the tail early. Every +# other matcher here routes through `split_segments` for that reason, and +# `gh pr review` was already gated the right way. + # The named CLI verbs, where the command word alone settles it. _POST_CMDS = ( r"gh\s+pr\s+comment", @@ -90,8 +99,19 @@ # where the whole command is already in hand. r"gh\s+pr\s+review\b", # `glab ... comment` is a real alias of `... note`; both spellings ship. - r"glab\s+mr\s+(?:note|comment)", - r"glab\s+issue\s+(?:note|comment)", + # NOT a bare `glab mr note` prefix: current glab exposes `list`, `resolve`, + # `delete` and `update` as subcommands, none of which posts anything. + r"glab\s+mr\s+(?:note|comment)(?!\s+(?:list|resolve|delete|update)\b)", + r"glab\s+issue\s+(?:note|comment)(?!\s+(?:list|resolve|delete|update)\b)", + # `--comment` on a state change posts a real comment. Missed for eleven + # review rounds because the command word is `close`/`reopen`, so nothing + # about it reads as commenting -- and `skills/rescue-closed/SKILL.md` + # carries a live undisclosed one. + r"gh\s+issue\s+(?:close|reopen)\b", + # NOT `gh pr merge`: its `-b/--body` is the MERGE-COMMIT body and it has no + # `--comment` at all (`gh pr merge --help`), so that alternative could never + # fire. Verified against gh 2.98.0. + r"gh\s+pr\s+(?:close|reopen)\b", ) POST_RE = re.compile(_ANCHOR + r"(?:" + "|".join(_POST_CMDS) + r")", re.MULTILINE) @@ -108,6 +128,15 @@ # So test the parts independently over the whole segment, which the quote-aware # splitter has already bounded to one command. Order-independent by # construction, which also fixes `gh api -f body=... `. +# Every flag that can supply an API field body, in ONE place. +# +# Four patterns listed these independently and drifted three separate times -- +# `-F body=`, `--raw-field`, and `--form` were each added to some sites and not +# others, and the last was reported missing from `UNREADABLE_RE` in two +# consecutive review rounds. A shared constant makes the drift impossible rather +# than reviewable. +_FIELD_FLAGS = r"(?:-f|-F|--field|--raw-field|--form)" + API_CMD_RE = re.compile(_ANCHOR + r"(?:gh|glab)\s+api\b", re.MULTILINE) # A body-supplying field is what separates a POST from the review-READ that # `CLAUDE.md` prescribes and every ARDI round runs. @@ -116,7 +145,13 @@ # quoted-whole-argument spelling for sibling flags too # (`request-pr-review`'s `-f "reviewers[]="`). Without it the registry line # this change annotates was completely invisible to the guard. -API_BODY_FIELD_RE = re.compile(r"(?:-f|-F|--field|--raw-field)\s+[\"']?body=") +API_BODY_FIELD_RE = re.compile( + _FIELD_FLAGS + r"[\s=]*[\"']?body=" + # `--form` is glab's flag, not gh's -- both appear because the detector + # matches `(?:gh|glab) api`. + # `--input ` supplies the whole payload, body included. The body is + # then unreadable rather than absent, which is what the caller reports. + r"|--input\b") # The comment-bearing endpoints, and the GraphQL comment mutations. # No `/replies` alternative. GitHub's reply route is # `POST /repos/{o}/{r}/pulls/{n}/comments/{id}/replies`, so it always contains @@ -128,11 +163,91 @@ r"|addDiscussionComment|addComment", re.IGNORECASE) +# The endpoint argument of a `gh api` / `glab api` call. +# +# Tokenized rather than pattern-matched. A regex expecting flags-then-endpoint +# broke the order-independent form `gh api -f body="..." `, which is valid +# and which an earlier round added a fixture for. Tokenizing respects quoting, +# so a body containing spaces or a `/comments` mention stays one token and +# cannot be mistaken for the path. +def _tokens(text): + """Whitespace-split *text*, keeping quoted runs together.""" + out, cur, quote = [], [], None + esc = False + for ch in text: + if esc: + cur.append(ch); esc = False; continue + if ch == "\\" and quote != "'": + esc = True; continue + if quote: + if ch == quote: + quote = None + else: + cur.append(ch) + continue + if ch in "\"'": + quote = ch; continue + if ch.isspace(): + if cur: + out.append("".join(cur)); cur = [] + continue + cur.append(ch) + if cur: + out.append("".join(cur)) + return out + + +def api_endpoint(segment): + """The endpoint argument of an api call, or None. + + A path-shaped token: it contains `/` and no `=`, so a `body=...` field -- + even one whose text mentions `/comments` -- is never mistaken for it. + `graphql` is the one endpoint with no slash. + """ + toks = _tokens(segment) + for i, t in enumerate(toks): + if t in ("api",) and i + 1 < len(toks): + for u in toks[i + 1:]: + if u == "graphql": + return u + if "/" in u and "=" not in u and not u.startswith("-"): + return u + return None + return None + + def is_api_post(segment): """True when this segment posts a comment through a raw forge API.""" if not API_CMD_RE.search("\n" + segment): return False - if not API_COMMENT_TARGET_RE.search(segment): + # Match the ENDPOINT, not the whole segment. Searching the segment let + # `gh api repos/o/r/issues -f body='Please use the /comments endpoint.'` -- + # an issue creation -- read as a comment post, because the body mentioned + # the path. GraphQL has no path, so it keeps the segment-wide test below. + endpoint = api_endpoint(segment) + is_graphql = bool(endpoint and endpoint.strip("\"'") == "graphql") + target_text = segment if is_graphql else (endpoint or "") + # A GraphQL call whose payload comes from `--input` hides its mutation name + # in the file, so neither the command nor the endpoint can say whether it + # posts a comment. Treat it as a post with an unreadable body: the note says + # "this check cannot read the body", which is true and asserts nothing, and + # the corpus writes no such command today so the cost is nil. + if is_graphql and re.search(r"--input\b", segment): + return True + if not API_COMMENT_TARGET_RE.search(target_text): + return False + # A GraphQL target is a mutation NAME, which a comment can merely mention. + # `--input` satisfies the body-field test below, so without this the name + # alone would classify `# addDiscussionComment payload` as a post. + if (re.search(r"addDiscussionComment|addComment", segment, re.IGNORECASE) + and not re.search(r"/comments|/notes|/discussions", segment) + and not re.search(r"\bmutation\b", segment)): + return False + # An explicit read is not a post, however many fields it carries. `gh api` + # infers POST from the presence of a field, so only an explicit GET (or a + # method that is not a create) can be ruled out here. + if re.search(r"(?:-X|--method)[\s=]*(?:GET|HEAD|PATCH|PUT|DELETE)\b", + segment, re.IGNORECASE): return False # A GraphQL comment mutation may carry its body inside the query text or in # an `--input` file rather than in a `body=` field, so the field test alone @@ -151,18 +266,47 @@ def is_api_post(segment): # there is nothing to disclose. Tested over the whole segment rather than in a # lookahead, so a continuation line cannot hide the flag. REVIEW_ONLY_RE = re.compile(_ANCHOR + r"gh\s+pr\s+review\b", re.MULTILINE) + +# `gh issue|pr close|reopen` post a comment only when `--comment`/`-c` is given. +# Tested over the whole segment, which `split_segments` has already bounded with +# quote awareness, so a `;` inside an earlier flag's value cannot truncate it. +CLOSE_REOPEN_RE = re.compile( + _ANCHOR + r"gh\s+(?:issue|pr)\s+(?:close|reopen)\b", re.MULTILINE) +# `gh` is a pflag CLI, whose documented shorthand syntax attaches a value with +# no separator (`-cvalue`) or with an equals (`-c=value`). Requiring whitespace +# missed both, on the exact posting surface this file was extended to cover. +# +# `(? forged` did too -- so the guard could be discharged by text that is +# not the marker. The agent name stays substitutable; the rest is fixed. +MARKER_RE = re.compile( + r"posted by .{0,40}\(ai agent\)[^\n]{0,12}not written by a human", + re.IGNORECASE) ROBOT = "\U0001f916" # Only a body that both carries the emoji AND reads as an attribution is @@ -214,27 +364,27 @@ def is_post_segment(segment): # different flags spelled alike, so both shapes are listed rather than one # being assumed to cover the other. UNREADABLE_RE = re.compile( - r"--body-file|--description-file|--editor\b|--web\b" + r"--body-file|--description-file|--editor\b|--web\b|--input\b" # `-F ` is gh pr comment's own body-file shorthand. Matched as a token # with NO `=` in it, rather than by a negative lookahead after an optional # quote: the optional quote gave the engine a backtracking path where it # skipped the quote, failed to find `key=` starting at `"`, and so satisfied # the negation -- which made `-F "in_reply_to=5"` look like a file. r"|(?"` so it could reuse the @@ -279,8 +445,7 @@ def is_post_segment(segment): # argument early, and `@dependabot rebase" and a long note for the humans ...` # took the exemption. Reconstructing syntax to reuse a matcher is what reopened # a hole the Bash path had a fixture against. -BOT_BODY_RE = re.compile( - r"^\s*@(?:" + _BOT_HANDLES + r")\b[ \w-]{0,40}\s*$", re.IGNORECASE) +BOT_BODY_RE = re.compile(r"^\s*" + _BOT_BODY + r"$", re.IGNORECASE) # MCP comment-posting tools. @@ -324,8 +489,9 @@ def is_post_segment(segment): "instead:\n\n " + MARKER_TEXT + "\n\n" "scripts/check-pr-fully-clean.py matches the robot emoji as a " "REVIEW_BODY_MARKERS entry, so a comment carrying it is admitted into the " - "fully-clean verdict scan as a review -- and a claim or status comment " - "carries no findings, so it scans as a CLEAN one. " + SEE + "fully-clean verdict scan as a review. Admission is not the whole story -- the " + "comment must also name the head SHA to count -- but the emoji removes the " + "one filter standing between a claim comment and that scan. " + SEE ) @@ -406,6 +572,139 @@ def bodies_for(segment, bodies): if int(i) < len(bodies)) +# The inline body value, when the segment carries one. Needed because searching +# the whole segment for the marker accepts it ANYWHERE -- including in a +# trailing shell comment, in a `--repo` value, or followed by more human prose +# after the marker. Each of those is a body that does not disclose, passed by a +# check that says it does. +# +# Written as explicit cases rather than one regex. A single pattern got all +# three quoting shapes wrong at once: it read `$'...'` as the bare token +# `$'Done,`, and it read `-f "body=X"` -- where the quote precedes `body=` -- +# as the bare token `X` truncated at the first space. +_FLAG_BEFORE_VALUE = re.compile( + r"(?:--(?:body|message|comment)[\s=]+|(?"` extracted + `--body` as the body and warned on a compliant comment, while the same + flags in the other order passed. + """ + return bool(value) and value.startswith("--") and len(value) > 2 + + +def _read_quoted(text, i): + """Value starting at *i*, honouring '...', "...", $'...', or a bare token.""" + if text.startswith("$'", i): + i += 2 + quote = "'" + elif i < len(text) and text[i] in "\"'": + quote = text[i] + i += 1 + else: + m = re.compile(r"\S+").match(text, i) + return m.group(0) if m else None + out = [] + while i < len(text): + ch = text[i] + if ch == "\\" and i + 1 < len(text): + out.append(text[i + 1]); i += 2; continue + if ch == quote: + return "".join(out) + out.append(ch); i += 1 + return "".join(out) + + +def inline_body(segment): + """The segment's inline body value, or None when it has no readable one. + + Candidates are ranked by POSITION, not by pattern order. Trying the field + patterns first meant a `--body`-supplied body that merely mentioned + `-f body=` -- an ordinary thing to say in a comment about this very feature + -- had that inner text taken as the body, so a compliant comment warned. + A false positive on a compliant comment is the worst outcome available to a + warn-only guard, since the whole corpus is about to start appending markers. + """ + candidates = [] + m = _FIELD_QUOTED.search(segment) + if m: + candidates.append((m.start(), "field_quoted", m)) + m = _FIELD_BARE.search(segment) + if m: + candidates.append((m.start(), "field_bare", m)) + m = _FLAG_BEFORE_VALUE.search(segment) + if m: + candidates.append((m.start(), "flag", m)) + if not candidates: + return None + candidates.sort(key=lambda c: c[0]) + for idx, (_, kind, m) in enumerate(candidates): + value = _extract(segment, kind, m) + if not _looks_like_flag(value): + return value + # This candidate consumed a boolean flag; try the next one along. + return None + + +def _extract(segment, kind, m): + if kind == "field_quoted": + # The quote opened BEFORE `body=`, so the value runs to its partner. + quote = m.group(1) + rest = segment[m.end():] + j = rest.find(quote) + return rest if j < 0 else rest[:j] + return _read_quoted(segment, m.end()) + + +def discloses(text): + """True when *text* ENDS with the disclosure marker. + + Anchored at the end rather than searched, because the marker's whole job is + to be the last thing a reader sees. A marker followed by further prose reads + as a quotation of the convention rather than as a disclosure, and a marker + in a trailing shell comment is not in the body at all. + """ + # A heredoc body arrives with its terminator line still attached, so strip a + # trailing all-caps delimiter before anchoring -- otherwise a blank line + # before `EOF` reads as the body continuing past the marker. + # Delimiters are case-free in shell (`<<'eof'` is as valid as `<<'EOF'`), + # and HEREDOC_RE already accepts both -- so stripping only an uppercase + # terminator warned on a compliant lowercase-delimited body. + text = re.sub(r"\n[ \t]*[A-Za-z_][A-Za-z0-9_]*[ \t]*$", "", text) + tail = text.rstrip().rstrip("\"'").rstrip() + m = None + for m in MARKER_RE.finditer(tail): + pass + if m is None: + return False + # A blank line after the marker, or a long trailing run, means the body + # continues past the disclosure -- which reads as quoting the convention + # rather than as disclosing. A SINGLE newline plus a short run is tolerated + # deliberately: a trailing signature or a wrapped marker line is common, and + # refusing it would warn on compliant comments, which is the costlier error + # for a warn-only guard. + # Only the marker's own closing punctuation may follow -- `._`, a quote, a + # heredoc terminator already stripped above. The previous 60-character + # allowance was there for a trailing signature, and it also admitted + # ` forged`, which is the whole point of anchoring. + after = tail[m.end():].strip() + return bool(re.fullmatch(r"[.\s_*'\"`)\]]*", after)) + + def judge_segment(segment, extra): """Return a warning for one command-position segment, or None. @@ -416,10 +715,24 @@ def judge_segment(segment, extra): provide, defeated by the argument meant to support it. """ text = segment + "\n" + extra - if BOT_COMMAND_RE.search(segment): + # Tested against the EXTRACTED BODY, not the segment. Searching the segment + # let `--body "Tell humans to run --body '@dependabot rebase' now."` take the + # exemption, because the quoted example matched anywhere in the command. + _body_for_bot = inline_body(segment) + if _body_for_bot is not None and BOT_BODY_RE.match(_body_for_bot.strip()): return None - if MARKER_RE.search(text): + if _body_for_bot is None and BOT_COMMAND_RE.search(segment): return None + body = inline_body(segment) + if body is not None and not extra: + # A readable body settles it on its own terms: the marker must END it. + if discloses(body): + return None + elif MARKER_RE.search(text): + # No readable inline body (a heredoc supplies it, say), so fall back to + # the segment-wide search this cannot improve on. + if discloses(text): + return None if EMOJI_DISCLOSURE_RE.search(text): return EMOJI # A heredoc body we actually READ settles it: the body is in hand and @@ -429,7 +742,14 @@ def judge_segment(segment, extra): # same misdiagnosis the `-F ` case produced. if extra: return MISSING - if UNREADABLE_RE.search(segment) or not HAS_INLINE_BODY_RE.search(segment): + # `inline_body` is the single authority on whether a body is readable, and + # `HAS_INLINE_BODY_RE` is consulted only for the shapes it cannot parse. + # Keeping two independent flag lists is what let `--form body=` be extracted + # correctly and then reported as unreadable anyway -- the same drift that + # made `-F body=` and `--raw-field body=` misreport two rounds earlier. + if UNREADABLE_RE.search(segment): + return UNREADABLE + if body is None and not HAS_INLINE_BODY_RE.search(segment): return UNREADABLE return MISSING @@ -464,7 +784,12 @@ def verdict_mcp(tool_name, tool_input): return None if BOT_BODY_RE.match(body): return None - if MARKER_RE.search(body): + # `discloses`, not `MARKER_RE.search`. The end-anchoring fix landed on the + # Bash path and not here -- on the easier case, where the raw body is + # already in hand -- so the MCP route accepted a marker followed by more + # human prose. That is the population this branch exists for: a remote + # session has no `gh`, so MCP is its only route. + if discloses(body): return None if EMOJI_DISCLOSURE_RE.search(body): return EMOJI diff --git a/hooks/test-require-agent-disclosure.py b/hooks/test-require-agent-disclosure.py index 8716065f6..0b4565a5c 100755 --- a/hooks/test-require-agent-disclosure.py +++ b/hooks/test-require-agent-disclosure.py @@ -47,9 +47,17 @@ def GQL(body): # --- must NOT warn ------------------------------------------------------- ("marker present", f'gh pr comment 12 --body "Working on this.\n\n{MARKER}"', False), + # The NAME substitutes; the rest of the line stays verbatim, per AGENTS.md. + # This fixture used to carry an invented tail ("-- not a human._"), which + # encoded a laxness the rule forbids -- and which the prefix-only MARKER_RE + # happened to accept. Tightening the matcher against marker forgery is what + # exposed it. ("marker with another agent name", - 'gh pr comment 12 --body "Done.\n\n_Posted by Codex (AI agent) -- not a human._"', - False), + 'gh pr comment 12 --body "Done.\n\n_Posted by Codex (AI agent) --- not ' + 'written by a human._"', False), + ("an invented marker tail does not disclose", + 'gh pr comment 12 --body "Done.\n\n_Posted by Codex (AI agent) -- not a ' + 'human._"', "missing"), ("dependabot rebase is exempt", 'gh pr comment 12 --repo o/r --body "@dependabot rebase"', False), ("dependabot squash is exempt", @@ -267,9 +275,19 @@ def GQL(body): 'gh pr review 12 --comment \\\n --body-file /tmp/r.md', None), # --- round-5: naming a mutation is not posting --------------------------- - ("a command that merely NAMES the mutation posts nothing", + # SUPERSEDED by the #2185 cross-vendor round. This asserted silence for + # `--input payload.json` on the strength of the mutation name appearing in + # a trailing SHELL COMMENT -- reading that comment as evidence the command + # posts nothing. It is not evidence: `--input` supplies the whole payload + # from a file, so the command may post a comment and the check cannot see + # it. "Cannot read the body" is the honest verdict, and it asserts nothing. + # The corpus writes no such command, so the cost is nil. + ("gh api graphql --input hides its mutation inside the file", 'gh api graphql --input payload.json # addDiscussionComment payload', - False), + None), + # The genuine mention-only case: nothing supplies a payload at all. + ("naming the mutation with no payload flag posts nothing", + 'echo "see addDiscussionComment in the docs"', False), # --- round-5: the `--body=` equals form is inline, not unreadable -------- ("--body= equals form is a visible body", @@ -304,8 +322,8 @@ def GQL(body): ("a GraphQL mutation whose body is not in a body= field", "gh api graphql --input p.json -f query='mutation { addDiscussionComment(x) }'", None), - ("a comment mentioning the mutation posts nothing", - 'gh api graphql --input p.json # addDiscussionComment payload', False), + ("a bare gh api graphql with no payload flag posts nothing", + 'gh api graphql -f query="query { viewer { login } }"', False), # --- round-6: properties that survived mutation with the suite green ----- ("command substitution is a command position", @@ -322,11 +340,274 @@ def GQL(body): ("the review-thread reply route is a comment target", 'gh api "repos/o/r/pulls/1/comments/9/replies" -f body="bare"', True), + # --- cross-vendor round: the marker must END THE BODY -------------------- + # + # Eleven same-vendor rounds accepted a marker found ANYWHERE in the command. + # A cross-vendor reviewer supplied all four of these on its first pass. + ("a marker in a trailing shell comment is not in the body", + 'gh pr comment 1 --body "bare" # ' + MARKER, "missing"), + ("a marker followed by more human prose does not disclose", + 'gh pr comment 1 --body "Done.\n\n' + MARKER + + '\n\nAlso, a human note."', "missing"), + ("a partial marker does not disclose", + 'gh pr comment 1 --body "Done.\n\n_Posted by Claude Code_"', "missing"), + + # --- cross-vendor round: posting surfaces that are not named "comment" ---- + ("gh issue reopen --comment posts a comment", + 'gh issue reopen 5 -R o/r --comment "Reviving: still matters."', "missing"), + ("gh issue close --comment posts a comment", + 'gh issue close 5 -R o/r --comment "Superseded."', "missing"), + ("gh pr close --comment posts a comment", + 'gh pr close 5 -R o/r --comment "Superseded."', "missing"), + ("gh issue reopen --comment WITH marker", + 'gh issue reopen 5 -R o/r --comment "Reviving.\n\n' + MARKER + '"', False), + + # --- cross-vendor round: --input and --form ------------------------------ + ("gh api --input supplies an unreadable body", + 'gh api repos/o/r/issues/1/comments --input payload.json', None), + ("glab api --form body= is a post", + 'glab api projects/:id/merge_requests/1/notes --form body="bare"', + "missing"), + + # --- cross-vendor round: the bot exemption is a command vocabulary ------- + ("prose after a real bot verb is not exempt", + 'gh pr comment 1 --body "@dependabot rebase please humans"', "missing"), + ("an explicit GET is not a post", + 'gh api repos/o/r/issues/1/comments -X GET -f per_page=100', False), + + # --- push-gate round: the extractor must not warn on a COMPLIANT comment -- + # + # A body that merely MENTIONS a field flag had that inner text taken as the + # body, so a compliant comment about this very feature warned. A false + # positive on a compliant comment is the worst outcome for a warn-only + # guard. + ("a compliant body that quotes -f body= is not a false positive", + 'gh pr comment 2130 --body "Addressed: inline_body now handles -f body= ' + 'and --form body=.\n\n' + MARKER + '"', False), + ("a compliant body that quotes -F body=@file", + 'gh pr comment 2130 --body "Rebutted: the canonical reply is -F ' + 'body=@file, so the quote comes first.\n\n' + MARKER + '"', False), + + # --- push-gate round: the exemption across every accepted spelling ------- + ("bot command via --body=", 'gh pr comment 5 --body="@dependabot rebase"', + False), + ("bot command via -b", 'gh pr comment 5 -b "@dependabot rebase"', False), + ("bot command via --comment", + 'gh issue close 5 -R o/r --comment "@dependabot close"', False), + + # --- push-gate round: a heredoc keeps its terminator line ---------------- + ("heredoc with a blank line before its terminator still discloses", + "gh pr comment 5 --body-file - <<'EOF'\nHi.\n\n" + MARKER + "\n\nEOF", + False), + ("heredoc where prose follows the marker does not disclose", + "gh pr comment 5 --body-file - <<'EOF'\nHi.\n\n" + MARKER + + "\n\nAnd more human prose.\nEOF", "missing"), + + # --- push-gate round: gh pr merge has no --comment ----------------------- + ("gh pr merge --body is a merge-commit body, not a comment", + 'gh pr merge 5 -R o/r --body "merge commit body"', False), + + # --- #2185 review: the close/reopen gate must be QUOTE-AWARE ------------- + # + # The first version used a lookahead over a raw `[^\n;&|]` tail, so a `;` + # inside an EARLIER flag's quoted value ended the tail before `--comment` + # and the command went undetected. `gh issue close` really does take a + # free-text `--duplicate-of`, so this is a shape the corpus can produce. + # Every other matcher in the guard routes through `split_segments` for this + # reason; these now do too. + ("a semicolon in an earlier flag does not hide --comment", + 'gh issue close 5 -R o/r --duplicate-of "see issue #3; also #4" ' + '--comment "Closing without disclosure."', "missing"), + ("an ampersand in an earlier flag does not hide --comment", + 'gh issue close 5 -R o/r --duplicate-of "A & B" --comment "bare"', + "missing"), + # `-R` rather than `--title`: gh has no `--title` on `pr close`, so the old + # fixture exercised the regex against a command the CLI would reject. + ("a pipe in an earlier flag does not hide --comment", + 'gh pr close 5 -R "o/r|fork" --comment "bare"', "missing"), + ("the same command WITH the marker stays silent", + 'gh issue close 5 -R o/r --duplicate-of "see #3; also #4" ' + '--comment "Closing.\n\n' + MARKER + '"', False), + # Without `(? is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), ("--editor is unreadable", 'gh pr comment 12 --editor', None), + ("glab api --form body= with marker discloses", + 'glab api projects/1/merge_requests/2/notes --form body="Done.\n\n' + MARKER + '"', + False), + ("glab api --form body= bare misses marker", + 'glab api projects/1/merge_requests/2/notes --form body="Done."', + "missing"), ] # --- the emoji branch -------------------------------------------------------- @@ -337,7 +618,11 @@ def GQL(body): INDIRECT_CASES = [ ("body-file", 'gh pr comment 12 --body-file /tmp/b.md'), ("api body file", 'gh pr comment 12 -F body=@/tmp/b.md'), + ("glab api body file", 'glab api projects/1/merge_requests/2/notes --form body=@/tmp/b.md'), + ("glab api quoted body file", 'glab api projects/1/merge_requests/2/notes --form body="@/tmp/b.md"'), ("variable body", 'gh pr comment 12 --body "$BODY"'), + ("glab api variable body", 'glab api projects/1/merge_requests/2/notes --form body=$BODY'), + ("glab api quoted variable body", 'glab api projects/1/merge_requests/2/notes --form body="$BODY"'), ] @@ -470,6 +755,16 @@ def run(): "Bare review body.", True), ("MCP discussion comment", "mcp__github__discussion_comment_write", "Bare discussion reply.", True), + # The end-anchoring fix landed on the Bash path only, and no MCP + # fixture covered it -- 121 green cases did not catch a marker followed + # by human prose on the route a remote session must use. + ("MCP marker followed by human prose does not disclose", + "mcp__github__add_issue_comment", + "Working on this.\n\n" + MARKER + "\n\nAlso: please look at CI.", + True), + ("MCP marker first, prose after, does not disclose", + "mcp__github__add_issue_comment", + MARKER + "\n\nWorking on this, and a long human paragraph.", True), ("MCP discussion comment WITH marker", "mcp__github__discussion_comment_write", "Reply.\n\n" + MARKER, False), @@ -485,7 +780,7 @@ def run(): print(f"{'PASS' if ok else 'FAIL'}: {label} " f"(warned={got}, expected={expect})") - total = len(CASES) + 2 + len(INDIRECT_CASES) + 1 + 4 + 11 + total = len(CASES) + 2 + len(INDIRECT_CASES) + 1 + 4 + 13 print(f"\n{total - failed} passed, {failed} failed") return 1 if failed else 0 diff --git a/memories/github.md b/memories/github.md index bdc2d1290..a04963303 100644 --- a/memories/github.md +++ b/memories/github.md @@ -623,6 +623,10 @@ in [`github-repo-transfers.md`](github-repo-transfers.md). `7b006485`, whose `review / claude-review` check run carries `conclusion: cancelled` while its dependent `review / require-review` carries `conclusion: failure`.) +- **`gh run list -w "Workflow Name"` fails if multiple workflow files share the same `name:` field in their YAML.** + The command exits 1 with `could not resolve to a unique workflow; found: workflow-a.yml workflow-b.yml`. + This happens commonly when reusable workflows (like a review action) are called by multiple caller workflows, or when two different files just happen to use the same `name`. + The fix is to query by the **exact filename** instead of the display name: `gh run list -w workflow-b.yml`. ## gh — stale remote URL causes cryptic `gh pr create` failure - `gh pr create` fails with `Head sha can't be blank, Base sha can't be blank, No commits between :main and :` when `origin` points to an **old repo URL** (e.g. after a GitHub repo transfer/rename). diff --git a/memories/preferences.md b/memories/preferences.md index d6b64fc07..dcc9d626f 100644 --- a/memories/preferences.md +++ b/memories/preferences.md @@ -195,7 +195,6 @@ Never finish a turn leaving in-flight PRs unmonitored without an active scheduled timer. (User directive / CAI, 2026-08-17.) - - When there's a well-scoped next step --- a filed follow-up issue, a sequenced item, an obvious continuation of the current work --- just start it; don't pause to ask "want me to keep going?" first. The answer is a standing yes. This removes the extra "should I continue?" pause between already-scoped steps; it does NOT override holding for genuinely ambiguous or architecturally significant decisions. @@ -320,6 +319,8 @@ The existing instruction already covered this; the gap was execution discipline in a fast multi-merge loop, not missing guidance --- re-read this bullet at the top of every "pick the next backlog item" cycle. In a multi-AGENT pipeline, UMS runs at BOTH levels: each subagent runs UMS once ITS PR merges (it stops after reporting CLEAN, so the coordinator resumes it post-merge with a "your PR merged, run UMS" nudge --- or the agent-launch spec bakes in a final UMS step), and the coordinator runs its own UMS for the cross-PR orchestration learnings no single subagent can see (merge-order sequencing, conflict-cascade handling, pipeline mechanics). Each agent writes its OWN memory file plus one MEMORY.md index line to keep the conflict surface small; avoid rewriting shared memory bodies concurrently. (Learned on sparta 2026-07-01.) +- After ANY PR merges to main (under mwc, post-merge, or manual merge), IMMEDIATELY and autonomously sweep all open PRs in the repository for merge conflicts (`gh pr list --state open --json number,title,headRefName,mergeable,mergeStateStatus`). + For any PR reporting `CONFLICTING` or `UNKNOWN`, fetch main, test the merge, resolve the conflict in an isolated worktree, and push the sync commit proactively without waiting for the user to point it out or ask for it. (Learned on ai-config, 2026-08-24: "cai: you should have checked PR conflicts on your own".) - Keep it simple. Don't over-explain or ask permission for straightforward fixes --- just do them. - Don't re-ask a decision that's already settled and built. @@ -440,7 +441,6 @@ (Measured 2026-08-21 on ai-config#1884: two `memories/` files were treated for hours as a peer session's in-flight work. Both additions were already on `main` in fuller form, and the diff had also rewritten three *correct* relative links into broken ones --- the `check-links.py` failure being blamed on that session all along.) - - **Don't touch anyone else's branch.** **Do:** only push to or modify branches I created in my own worktree. **Don't:** push commits, force-push, checkout, or edit branches belonging to another session or user --- even if the content looks worth keeping or the branch looks abandoned. diff --git a/scripts/sync-codex-skill-wrappers.py b/scripts/sync-codex-skill-wrappers.py index 1afa7f0d9..79ec8bc9c 100755 --- a/scripts/sync-codex-skill-wrappers.py +++ b/scripts/sync-codex-skill-wrappers.py @@ -165,7 +165,15 @@ def reference_doc(mappings: dict) -> str: > [!IMPORTANT] > **Every comment-posting operation below carries the agent-disclosure marker in > its body**, on its own line after a blank line: -> `_Posted by Claude Code (AI agent) --- not written by a human._` +> `_Posted by (AI agent) --- not written by a human._` +> +> Substitute your own agent's name --- this registry is read by every model, so a +> hard-coded `Claude Code` would have a Codex or Gemini session misattribute its +> own comment. +> Keep the rest of the line verbatim. +> Check the substituted name against `scripts/check-pr-fully-clean.py`'s +> `REVIEW_BODY_MARKERS` too --- `code review` is one of them, so an agent named +> for code review would reintroduce the false-clean the emoji ban prevents. > > This registry is the substitution point for remote/web sessions, which have no > `gh` at all --- so a marker-free template here is a marker-free comment there, in diff --git a/shared/workflow/disclose-agent-authorship.md b/shared/workflow/disclose-agent-authorship.md index c12e655a7..5323a47d3 100644 --- a/shared/workflow/disclose-agent-authorship.md +++ b/shared/workflow/disclose-agent-authorship.md @@ -12,9 +12,14 @@ _Posted by Claude Code (AI agent) --- not written by a human._ ``` **It deliberately does not use the robot emoji.** -That looks like the obvious choice, and it is the one thing the marker must avoid: `scripts/check-pr-fully-clean.py` matches the bare emoji as a `REVIEW_BODY_MARKERS` entry, so any comment carrying it is admitted into the verdict scan as a review item. -A disclosure footer on every agent comment would therefore turn every claim, every status note, and every deferral into something the fully-clean checker reads as a review --- and a claim comment carries no findings, so it would scan as a **clean** one. -That is the false-clean failure [`fully-clean`](fully-clean.md) already describes for a human-authored self-review, arriving through the very mechanism added to make authorship legible. +That looks like the obvious choice, and it is the one thing the marker must avoid: `scripts/check-pr-fully-clean.py` matches the bare emoji as a `REVIEW_BODY_MARKERS` entry, so a comment carrying it is *admitted* into the verdict scan as a review item. +A disclosure footer on every agent comment would therefore turn every claim, every status note, and every deferral into something the fully-clean checker can read as a review --- and a claim comment carries no findings, so it would scan as a **clean** one. + +**Admission is necessary and not sufficient, which is worth stating precisely.** +The comment must ALSO name the current HEAD SHA to count toward criterion 2. +A synthetic `Working on this 🤖` with no SHA returns `No review comment has been posted evaluating HEAD SHA`, not a clean verdict. +So the emoji does not single-handedly manufacture a false clean --- it removes the one filter standing between a claim comment and the verdict scan, and leaves a SHA mention as the only thing still separating them. +A claim comment that quotes the head SHA is ordinary, which is why the gap is worth closing at the marker rather than relying on the second gate. The marker above collides with none of the checker's `REVIEW_BODY_MARKERS` (the robot emoji, `### ` plus that emoji, `code review`, `**claude finished`, `### verdict`, `verdict:`) nor with any `REVIEW_AGENT_MARKERS` entry, verified against `scripts/check-pr-fully-clean.py` on 2026-08-24. Check a replacement marker against both tuples before changing it. @@ -86,7 +91,7 @@ The literal `--body "@...` grep finds the two Dependabot sites and misses the th An earlier draft of this passage gave a different and wrong reason --- that the handle is never spelled contiguously in a source file, because a diff view would summon the bot. That is false twice over. -The handle appears 248 times across this corpus's markdown, counting every file but this one --- the command below is itself an occurrence, so a figure that included this file would move each time the file was edited, and both earlier drafts of this sentence were wrong for exactly that reason. +The handle appears in the hundreds across this corpus's markdown --- run the command below for the figure at your commit, rather than trusting one written here, since it moves whenever any file mentioning the handle changes. And [`memories/mention-triggers.md`](../../memories/mention-triggers.md) states the gate as `contains(github.event.comment.body, '@claude')`, over comment, review and issue bodies --- file contents are not among them. The practice of not spelling it applies to text that becomes a comment, which is what that file scopes it to. diff --git a/skills/gi/SKILL.md b/skills/gi/SKILL.md index f1bc47cb7..3f552b4f7 100644 --- a/skills/gi/SKILL.md +++ b/skills/gi/SKILL.md @@ -80,13 +80,29 @@ issue. Two signals must **both** be clear (`gh issue list` in step 1 returns titles, labels, and assignees but neither comment text nor linked PRs, so check both explicitly here). -**(1) No "Working on this" claim in the most recent comment:** +**(1) No live "Working on this" claim on the issue:** ```bash -# GitHub — read the issue's latest comment: -gh issue view --json comments --jq '.comments | last | .body' | cat # READ_ISSUE_COMMENTS +# GitHub -- read the claim/release exchange, not just the newest comment: +gh issue view --json comments \ + --jq '[.comments[] | select(.body | test("hold off|paws off|back off|unclaim|released|PR is free|now mergeable"; "i"))] | last | "\(.createdAt) \(.author.login): \(.body)"' # READ_ISSUE_COMMENTS +gh issue view --json updatedAt --jq .updatedAt # VIEW_ISSUE -- latest activity ``` +**Reading only `.comments | last` is the bug this replaces.** +A claim is live for two hours from the most recent *activity*, so any unrelated comment posted after it --- a status note, a bot's build result, a question --- becomes the last comment while the claim is still binding. +The claim then goes invisible and this check reports the issue free, which is the parallel-session collision the whole convention exists to prevent. +Filter to the exchange and take the last member of *that*. + +**Both timestamps are needed, which is why the second command is there.** +The claim's own `createdAt` says when it was made; the issue's `updatedAt` says +when the thread last saw activity, and the 2-hour rule expires on *activity*, +not on the claim's age. +A day-old claim followed by a comment thirty minutes ago is **live**; the same +claim with nothing after it is **expired**. +Reading only the claim's body cannot tell those apart, so it cannot decide the +question the step is asking. + Match the two-word invariant `hold off`, or either retired wording `paws off` / `back off`, case-insensitively --- then **exclude the comment if it also carries a release term** (`unclaim|released|PR is free|now mergeable`), because the retired release wording `... done --- paws off released.` contains `paws off` and would otherwise read as a live claim. See [`claim-pr`](../../shared/workflow/claim-pr.md)'s "Match the two-word invariant". If a live claim stands, skip the issue --- unless the claim has expired: no push or comment on the issue in over 2 hours, per [`claim-pr`](../../shared/workflow/claim-pr.md)'s expiration rule. @@ -115,8 +131,8 @@ If an open PR already exists for the issue: ### 5. Check history -Before implementing, invoke the `check-history` skill to review merged -MRs/PRs that touched the same area. Don't undo past progress. +Before implementing, invoke the `check-history` skill to review merged MRs/PRs that touched the same area. +Don't undo past progress. ### 6. Claim the issue diff --git a/skills/post-merge/SKILL.md b/skills/post-merge/SKILL.md index b287bcedd..9a95fb777 100644 --- a/skills/post-merge/SKILL.md +++ b/skills/post-merge/SKILL.md @@ -312,7 +312,15 @@ conflicting PR can sit in `UNKNOWN` and get missed if you filter for "A conflict your sweep found is not a conflict your merge caused" and "A stacked PR is the one conflict that intersection cannot attribute". 3. **Check claim status.** - Read the most recent comment. + Read the most recent comment **of the claim/release exchange**, not the most recent comment overall: + + ```bash + gh pr view --json comments \ + --jq '[.comments[] | select(.body | test("hold off|paws off|back off|unclaim|released|PR is free|now mergeable"; "i"))] | last | .body' # READ_PR_COMMENTS + ``` + + Any unrelated comment posted after a claim --- a status note, a bot result --- becomes the newest comment while the claim is still live, since a claim expires on activity rather than on age. + Reading the newest comment alone therefore reports a claimed PR as free. Match the two-word invariant, `hold off` or either retired wording `paws off` / `back off`, case-insensitively --- never a whole sentence. The PR and issue claims differ after those two words, and the dash between them is an em-dash in this file's own claim emitter (step 4), so a quoted prefix misses claims this very skill posts. See [`claim-pr`](../../shared/workflow/claim-pr.md)'s "Match the two-word invariant". diff --git a/skills/push/SKILL.md b/skills/push/SKILL.md index c80123175..a30d3b3d6 100644 --- a/skills/push/SKILL.md +++ b/skills/push/SKILL.md @@ -109,7 +109,7 @@ A released PR would read as live-claimed, and this skill would refuse a legitima Derive the release terms rather than copying this list, which is a snapshot of what the corpus posts today: `grep -rn "unclaim\|released\|PR is free\|now mergeable" skills/ commands/`. A matcher narrowed to the new phrase returns nothing on such a thread, which reads exactly like an unclaimed one --- see [`claim-pr`](../../shared/workflow/claim-pr.md). -If the latest claim comment is from someone **other than you**, hasn't been unclaimed, and is still live --- the PR shows a push or comment within the last 2 hours, per [`claim-pr`](../../shared/workflow/claim-pr.md)'s expiration rule --- **do not push.** +The query returns the whole claim/release exchange, newest last, so read its **last** member: if that is a *claim* rather than a release, and it is from someone **other than you**, and it is still live --- the PR shows a push or comment within the last 2 hours, per [`claim-pr`](../../shared/workflow/claim-pr.md)'s expiration rule --- **do not push.** Ask the user. An expired claim (over 2 idle hours) no longer blocks on its own, but take it over with a fresh claim comment and run this skill's other checks (branch-head advance, `@claude` run in flight) before pushing. diff --git a/skills/rescue-closed/SKILL.md b/skills/rescue-closed/SKILL.md index f5c16b5e5..3d8af8cc4 100644 --- a/skills/rescue-closed/SKILL.md +++ b/skills/rescue-closed/SKILL.md @@ -108,7 +108,9 @@ Before touching any item, **claim it** (`claim-pr`) so parallel sessions or the **Issue:** ```bash -gh issue reopen --comment "Reviving: ." # REOPEN_ISSUE +gh issue reopen --comment "Reviving: . + +_Posted by Claude Code (AI agent) --- not written by a human._" # REOPEN_ISSUE ``` If reopening is wrong — a messy thread, or scope has shifted — file a fresh issue diff --git a/tool-mappings.md b/tool-mappings.md index d64ab8f78..1b57de16c 100644 --- a/tool-mappings.md +++ b/tool-mappings.md @@ -13,7 +13,15 @@ operation to the equivalent GitHub MCP tool so any model can run a skill. > [!IMPORTANT] > **Every comment-posting operation below carries the agent-disclosure marker in > its body**, on its own line after a blank line: -> `_Posted by Claude Code (AI agent) --- not written by a human._` +> `_Posted by (AI agent) --- not written by a human._` +> +> Substitute your own agent's name --- this registry is read by every model, so a +> hard-coded `Claude Code` would have a Codex or Gemini session misattribute its +> own comment. +> Keep the rest of the line verbatim. +> Check the substituted name against `scripts/check-pr-fully-clean.py`'s +> `REVIEW_BODY_MARKERS` too --- `code review` is one of them, so an agent named +> for code review would reintroduce the false-clean the emoji ban prevents. > > This registry is the substitution point for remote/web sessions, which have no > `gh` at all --- so a marker-free template here is a marker-free comment there, in @@ -61,7 +69,7 @@ operation to the equivalent GitHub MCP tool so any model can run a skill. | `CREATE_ISSUE` | Open a new issue. | `gh issue create` | `mcp__github__issue_write (method=create)` | | `COMMENT_ISSUE` | Post a comment on an issue. **The body ends with the agent-disclosure marker** --- see [`disclose-agent-authorship`](shared/workflow/disclose-agent-authorship.md). | `gh issue comment "" --body "..."` | `mcp__github__add_issue_comment` | | `CLOSE_ISSUE` | Close an issue with a reason. | `gh issue close "" --reason "..."` | `mcp__github__issue_write (method=update, state=closed, state_reason=...)` | -| `REOPEN_ISSUE` | Reopen a closed issue. | `gh issue reopen "" --comment "..."` | `mcp__github__issue_write (method=update, state=open)` | +| `REOPEN_ISSUE` | Reopen a closed issue. **The body ends with the agent-disclosure marker** --- see [`disclose-agent-authorship`](shared/workflow/disclose-agent-authorship.md). | `gh issue reopen "" --comment "..."` | `mcp__github__issue_write (method=update, state=open)` | | `LABEL_ISSUE` | Set an issue's labels. The two behave differently and are not interchangeable: `--add-label` ADDS to the existing set, while the MCP path REPLACES the whole set, so pass the union of existing and new labels there. The MCP path also silently creates an unknown label name instead of rejecting it. | `gh issue edit "" --add-label "..."` | `mcp__github__issue_write (method=update, labels=[...])` | | `GET_LABEL` | Read a single label's name, color, and description. There is no MCP tool to create or update a label; use gh label create/edit, or gh api from a workflow. | `gh api "repos///labels/"` | `mcp__github__get_label` | | `LIST_DISCUSSIONS` | List a repository's discussions. Readable over REST; writes are GraphQL-only. | `gh api repos/{owner}/{repo}/discussions` | `mcp__github__list_discussions` | diff --git a/tool-mappings.yml b/tool-mappings.yml index 12a1758f8..77153a8b9 100644 --- a/tool-mappings.yml +++ b/tool-mappings.yml @@ -155,7 +155,7 @@ operations: cli: gh issue close "" --reason "..." github_mcp: mcp__github__issue_write (method=update, state=closed, state_reason=...) - id: REOPEN_ISSUE - description: Reopen a closed issue. + description: Reopen a closed issue. **The body ends with the agent-disclosure marker** --- see [`disclose-agent-authorship`](shared/workflow/disclose-agent-authorship.md). cli: gh issue reopen "" --comment "..." github_mcp: mcp__github__issue_write (method=update, state=open) - id: LABEL_ISSUE