From 8f01884b0953a99981e7b523ffe44a6d725e27e5 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Mon, 24 Aug 2026 17:10:12 -0700 Subject: [PATCH 01/16] fix(disclosure): the marker must end the BODY, plus five missed posting surfaces Closes #2177 #2131 merged with defects two same-vendor reviewers cleared: eleven local adversarial rounds and the repo's own claude-review (which ran properly once the token was restored -- $3.07, "Ready for merge", no findings). A cross-vendor review found 11 findings at that same commit, 8 blocking, every one real. The guard accepted a marker that was not in the body It searched the whole command for the marker phrase, so a marker in a trailing shell comment, a marker followed by more human prose, and a partial marker were all silent. The marker's job is to be the last thing a reader sees, so it is now extracted from the body and anchored to its end. Posting surfaces not named "comment" `gh issue close|reopen --comment` and `gh pr close|reopen --comment` post comments; none was detected, and skills/rescue-closed carried a live undisclosed one. Added, along with `gh api --input` and `--form body=`. NOT `gh pr merge`: its `-b` is the merge-commit body and it has no `--comment` at all, so an earlier draft's alternative for it could never have fired. The bot exemption admitted human-directed prose `[ \w-]{0,40}` let `@dependabot rebase please humans` through. Replaced with the closed command vocabulary those bots accept, and extended to every body-bearing spelling the detector accepts as a posting route -- `--body=`, `-b`, `--comment` were all missing, so compliant bot commands warned. Claim readers saw only the newest comment `gi` and `post-merge` read `.comments | last`. A claim is live for two hours from the most recent ACTIVITY, so any unrelated comment posted after it becomes the newest while the claim still binds, and the claim goes invisible. Both now filter to the claim/release exchange. Found by this branch's own push-gate review - The end-anchoring fix had landed on the Bash path only. `verdict_mcp` still searched, on the route a remote session must use, with the raw body already in hand. No MCP fixture covered it, which is how 121 green cases missed it. - Ranking extractor candidates by pattern order rather than position meant a body that merely MENTIONED `-f body=` had that inner text taken as the body -- so a compliant comment about this feature warned. Two of three realistic probes. Now ranked by position. - A heredoc body arrives with its terminator line attached, so a blank line before `EOF` read as the body continuing past the marker. - `tool-mappings`' `REOPEN_ISSUE` was the one comment-posting row unannotated. Overstated claims corrected The robot-emoji hazard is real and one gate narrower than stated: the emoji gets a comment ADMITTED to the verdict scan, and it must also name the head SHA to count. The fragment said so; the hook's user-facing warning and hooks.json still said "scans as a CLEAN one". All three agree now. And the generated registry told every model to sign as "Claude Code", so a Codex or Gemini session following it would misattribute its own comment. Not carried over: the stranded branch was behind main and would have reverted #2176's orchestrator changes. This branch is cut fresh from main and touches no orchestrator file. Filed separately: #2177 also records that `scripts/orchestrator/subagents.py`'s ReviewerSubagent posts an undisclosed comment on main -- it landed in #2176 after #2131 merged, so the rule shipped and the next merge violated it. Checks: 42/42 hook suites (131 in this one, up from 121), markdownlint 0, links, skills, hook-catalog, hook-output-shape, context-closure pin, and the real new-line-breaks gate against origin/main. --- hooks/hooks.json | 11 +- hooks/require-agent-disclosure.py | 197 +++++++++++++++++-- hooks/test-require-agent-disclosure.py | 79 +++++++- scripts/sync-codex-skill-wrappers.py | 10 +- shared/workflow/disclose-agent-authorship.md | 13 +- skills/gi/SKILL.md | 16 +- skills/post-merge/SKILL.md | 10 +- skills/push/SKILL.md | 2 +- skills/rescue-closed/SKILL.md | 4 +- tool-mappings.md | 12 +- tool-mappings.yml | 2 +- 11 files changed, 321 insertions(+), 35 deletions(-) diff --git a/hooks/hooks.json b/hooks/hooks.json index febd5d0c8..c02a8faef 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..13411653c 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -74,6 +74,9 @@ r"(?:[A-Za-z_][A-Za-z0-9_]*=\S*\s+)*" ) +# Tail of one command, for a lookahead that must cross a line continuation. +_SEG_TAIL = r"(?:[^\n;&|]|\\\n)*" + # The named CLI verbs, where the command word alone settles it. _POST_CMDS = ( r"gh\s+pr\s+comment", @@ -92,6 +95,15 @@ # `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)", + # `--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(?=" + _SEG_TAIL + r"(?:--comment\b|-c\s))", + # 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(?=" + _SEG_TAIL + r"(?:--comment\b|-c\s))", ) POST_RE = re.compile(_ANCHOR + r"(?:" + "|".join(_POST_CMDS) + r")", re.MULTILINE) @@ -116,7 +128,11 @@ # 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( + r"(?:-f|-F|--field|--raw-field|--form)\s+[\"']?body=" + # `--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 @@ -134,6 +150,19 @@ def is_api_post(segment): return False if not API_COMMENT_TARGET_RE.search(segment): 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 # would miss it. The earlier version of this branch keyed on the mutation @@ -214,14 +243,14 @@ 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 +322,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 +366,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 +449,108 @@ 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. +# 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=]+|-(?:b|m|c)\s+)") +_FIELD_QUOTED = re.compile( + r"(?:-f|-F|--field|--raw-field|--form)\s+([\"'])body=") +_FIELD_BARE = re.compile( + r"(?:-f|-F|--field|--raw-field|--form)\s+body=") + + +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 + _, kind, m = min(candidates, key=lambda c: c[0]) + 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. + text = re.sub(r"\n\s*[A-Z_][A-Z0-9_]*\s*$", "", text) + tail = text.rstrip().rstrip("\"'").rstrip() + m = None + for m in MARKER_RE.finditer(tail): + pass + if m is None: + return False + # Only the remainder of the marker line may follow. A blank line after it, + # or a long trailing run, means the body continues past the disclosure -- + # which reads as quoting the convention rather than as disclosing. + after = tail[m.end():].rstrip() + return "\n\n" not in after and len(after) <= 60 + + def judge_segment(segment, extra): """Return a warning for one command-position segment, or None. @@ -418,8 +563,16 @@ def judge_segment(segment, extra): text = segment + "\n" + extra if BOT_COMMAND_RE.search(segment): return None - if MARKER_RE.search(text): - 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 +582,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 +624,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..91be03e4d 100755 --- a/hooks/test-require-agent-disclosure.py +++ b/hooks/test-require-agent-disclosure.py @@ -322,6 +322,73 @@ 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), + # --- unreadable vs missing must not be confused (review finding 9) ------- ("gh pr comment -F is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), @@ -470,6 +537,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 +562,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/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 f77774ad0..69caf7689 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..2015ec3dc 100644 --- a/skills/gi/SKILL.md +++ b/skills/gi/SKILL.md @@ -80,13 +80,19 @@ 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 | "\(.author.login): \(.body)"' # READ_ISSUE_COMMENTS ``` +**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*. + 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 +121,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 From 17bfcb32f9249cc88ac1bba0ddfdeac9822467fb Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Mon, 24 Aug 2026 17:35:31 -0700 Subject: [PATCH 02/16] docs: correct the test-count delta in the previous commit message The previous commit said "131 in this one, up from 121". The 121 figure came from a commit on the branch that #2131 merged WITHOUT -- so it never reached `main`, and no reader can reproduce it from history. Measured against `origin/main`: 110 passed. So this branch's delta is +21 tests (110 -> 131), not +10. Recording it here rather than rewriting the message, since the merge commit already sits on top. The class of error is the one this branch is about: a number carried forward from a state that never shipped, published without the command that derives it. The command is: git show origin/main:hooks/test-require-agent-disclosure.py > /tmp/t.py git show origin/main:hooks/require-agent-disclosure.py > /tmp/require-agent-disclosure.py cd /tmp && python3 t.py From a667d2a2ee169e28d8b596f1bfec9da51bbdb597 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Mon, 24 Aug 2026 17:36:21 -0700 Subject: [PATCH 03/16] docs(hook): state what discloses() actually enforces, and whose flag --form is Two comment-only corrections from the push-gate review that missed the previous commit. `discloses()` said "only the remainder of the marker line may follow" while the rule is `"\n\n" not in after and len(after) <= 60` -- which permits a single newline plus a short run. That tolerance is deliberate (a trailing signature or a wrapped marker line is common, and refusing it would warn on a compliant comment), so the comment now describes the rule rather than a stricter one nobody implemented. `--form` is glab's flag, not gh's. The pattern is right, since the detector matches `(?:gh|glab) api`; only the comment read as if both had it. --- hooks/require-agent-disclosure.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index 13411653c..36cfcb828 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -130,6 +130,8 @@ # this change annotates was completely invisible to the guard. API_BODY_FIELD_RE = re.compile( r"(?:-f|-F|--field|--raw-field|--form)\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") @@ -544,9 +546,12 @@ def discloses(text): pass if m is None: return False - # Only the remainder of the marker line may follow. A blank line after it, - # or a long trailing run, means the body continues past the disclosure -- - # which reads as quoting the convention rather than as disclosing. + # 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. after = tail[m.end():].rstrip() return "\n\n" not in after and len(after) <= 60 From 10909ff18f01956abeaf12b76ea96c6e289f1cb1 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Mon, 24 Aug 2026 18:03:40 -0700 Subject: [PATCH 04/16] fix(hook): gate close/reopen over the whole segment, not a lookahead The #2185 review found the one new matcher that is not quote-aware, and it is the one this branch added. `gh issue|pr close|reopen` were gated by a lookahead over a raw `[^\n;&|]` tail. That class has no notion of quoting, so a `;` inside an EARLIER flag's quoted value ended the tail before `--comment` was reached and the command went undetected: gh issue close 5 -R o/r --duplicate-of "see issue #3; also #4" \ --comment "Closing without disclosure." `split_segments` keeps that whole command as one segment, correctly -- and then the lookahead threw that away. `gh issue close` really does take a free-text `--duplicate-of`, so this is a shape the corpus can produce, and it is exactly the surface the branch exists to detect. The fix is the pattern already used two lines above for `gh pr review`: match the verb, then test the body flag over the whole segment, which `split_segments` has already bounded with quote awareness. `_SEG_TAIL` is gone; nothing else used it. Worth naming why this slipped: every other matcher in the file routes through `split_segments`, and the file's own comments argue at length for doing so -- including round-4 fixtures pinning a semicolon and an ampersand inside a body. The new code reintroduced the same class one flag to the left, where no fixture looked. The 131 cases all exercised special characters inside the comment value and none in a flag before it. Seven fixtures added, covering `;`, `&` and `|` in an earlier flag, the disclosed counterpart, `-c`, and close/reopen with no comment flag at all (which posts nothing and must stay silent). Dropping the new gate turns the suite red. 138 tests, up from 131. Plus 41/41 hook suites, links, skills, hook-catalog, hook-output-shape, markdownlint, the real new-line-breaks gate, context-closure. --- hooks/require-agent-disclosure.py | 23 ++++++++++++++++++++--- hooks/test-require-agent-disclosure.py | 26 ++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index 36cfcb828..f6a250b2a 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -75,7 +75,13 @@ ) # Tail of one command, for a lookahead that must cross a line continuation. -_SEG_TAIL = r"(?:[^\n;&|]|\\\n)*" +# 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 = ( @@ -99,11 +105,11 @@ # 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(?=" + _SEG_TAIL + r"(?:--comment\b|-c\s))", + 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(?=" + _SEG_TAIL + r"(?:--comment\b|-c\s))", + r"gh\s+pr\s+(?:close|reopen)\b", ) POST_RE = re.compile(_ANCHOR + r"(?:" + "|".join(_POST_CMDS) + r")", re.MULTILINE) @@ -182,6 +188,13 @@ 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) +COMMENT_FLAG_RE = re.compile(r"--comment\b|--comment=|-c\s") ANY_BODY_FLAG_RE = re.compile( r"--body\b|--body=|--body-file|--message\b|--message=|-b\s|-m\s|-F\s" r"|(?:-f|-F|--field|--raw-field)\s+[\"']?body=") @@ -194,6 +207,10 @@ def is_post_segment(segment): if (REVIEW_ONLY_RE.search("\n" + segment) and not ANY_BODY_FLAG_RE.search(segment)): return False + # A close/reopen posts nothing unless `--comment` is present. + if (CLOSE_REOPEN_RE.search("\n" + segment) + and not COMMENT_FLAG_RE.search(segment)): + return False return True return is_api_post(segment) diff --git a/hooks/test-require-agent-disclosure.py b/hooks/test-require-agent-disclosure.py index 91be03e4d..905238d5c 100755 --- a/hooks/test-require-agent-disclosure.py +++ b/hooks/test-require-agent-disclosure.py @@ -389,6 +389,32 @@ def GQL(body): ("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"), + ("a pipe in an earlier flag does not hide --comment", + 'gh pr close 5 -R o/r --title "a|b" --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), + ("close with no --comment posts nothing", + 'gh issue close 5 -R o/r --duplicate-of "see #3"', False), + ("reopen with no --comment posts nothing", + 'gh issue reopen 5 -R o/r', False), + ("the -c short flag is a comment flag", + 'gh issue close 5 -R o/r -c "bare"', "missing"), + # --- unreadable vs missing must not be confused (review finding 9) ------- ("gh pr comment -F is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), From 3b8d04e6ded82200641a396c2eac543e13cea04e Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:23:32 -0700 Subject: [PATCH 05/16] fix(hook): recognize attached-flag shorthand and equals syntax for disclosure flags (closes #2185 findings) --- hooks/require-agent-disclosure.py | 12 ++++-------- hooks/test-require-agent-disclosure.py | 12 ++++++++++++ memories/preferences.md | 2 ++ 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index f6a250b2a..5ec1ab869 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -194,9 +194,10 @@ def is_api_post(segment): # 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) -COMMENT_FLAG_RE = re.compile(r"--comment\b|--comment=|-c\s") +COMMENT_FLAG_RE = re.compile(r"--comment\b|--comment=|-c(?:\s|=|\S)") ANY_BODY_FLAG_RE = re.compile( - r"--body\b|--body=|--body-file|--message\b|--message=|-b\s|-m\s|-F\s" + r"--body\b|--body=|--body-file\b|--message\b|--message=" + r"|-b(?:\s|=|\S)|-m(?:\s|=|\S)|-F(?:\s|=|\S)" r"|(?:-f|-F|--field|--raw-field)\s+[\"']?body=") @@ -468,11 +469,6 @@ 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. # 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 @@ -484,7 +480,7 @@ def bodies_for(segment, bodies): # `$'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=]+|-(?:b|m|c)\s+)") + r"(?:--(?:body|message|comment)[\s=]+|-(?:b|m|c)[\s=]*)") _FIELD_QUOTED = re.compile( r"(?:-f|-F|--field|--raw-field|--form)\s+([\"'])body=") _FIELD_BARE = re.compile( diff --git a/hooks/test-require-agent-disclosure.py b/hooks/test-require-agent-disclosure.py index 905238d5c..82fc78294 100755 --- a/hooks/test-require-agent-disclosure.py +++ b/hooks/test-require-agent-disclosure.py @@ -414,6 +414,18 @@ def GQL(body): 'gh issue reopen 5 -R o/r', False), ("the -c short flag is a comment flag", 'gh issue close 5 -R o/r -c "bare"', "missing"), + ("the -c short flag with equals syntax is a comment flag", + 'gh issue close 5 -R o/r -c="Closing without disclosure."', "missing"), + ("the -c short flag with attached quote is a comment flag", + 'gh issue close 5 -R o/r -c"Closing without disclosure."', "missing"), + ("the -c short flag with equals syntax and marker discloses", + 'gh issue close 5 -R o/r -c="Closing.\n\n' + MARKER + '"', False), + ("the -c short flag with attached quote and marker discloses", + 'gh issue close 5 -R o/r -c"Closing.\n\n' + MARKER + '"', False), + ("the -b short flag with equals syntax is a body flag", + 'gh pr comment 12 -b="bare"', "missing"), + ("the -b short flag with attached quote is a body flag", + 'gh pr comment 12 -b"bare"', "missing"), # --- unreadable vs missing must not be confused (review finding 9) ------- ("gh pr comment -F is a body-file, reported unreadable", diff --git a/memories/preferences.md b/memories/preferences.md index ade24fb94..4823df2e8 100644 --- a/memories/preferences.md +++ b/memories/preferences.md @@ -320,6 +320,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. From cf195e46ebdbc8bb97fbd6db2738ad11cfd6ef01 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Mon, 24 Aug 2026 18:28:20 -0700 Subject: [PATCH 06/16] fix(hook): match all three pflag short-flag spellings, and anchor them Second #2185 review round. Two findings, both real, plus the duplicated comment block that survived an earlier attempt to remove it. `gh` is a pflag CLI, so a shorthand flag attaches its value with no separator: `-cvalue` and `-c=value` are valid alongside `-c value`. Every short-flag pattern in this file required whitespace, so all of them missed the attached forms -- including the pre-existing `-b`/`-m` alternatives, which predate this branch. An undisclosed `gh issue close 5 -c"..."` drew no warning at all. Two repairs were needed, and the first was wrong Switching to `-c\S` fixed the attached forms and broke the spaced one, trading one gap for another; the suite caught it immediately. All three spellings now match. Then `inline_body` started returning `hanges`: with no left boundary, `-c` matched inside `--request-changes`, and the extractor read the rest of that token as the body. Every short-flag pattern now carries `(?"` so it could reuse the @@ -468,11 +475,6 @@ 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. # 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 @@ -484,7 +486,7 @@ def bodies_for(segment, bodies): # `$'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=]+|-(?:b|m|c)\s+)") + r"(?:--(?:body|message|comment)[\s=]+|(? is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), From 5f5aa20e2b5a73f635e48dcbc59b885c7a0f204d Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Mon, 24 Aug 2026 19:01:58 -0700 Subject: [PATCH 07/16] fix(hook): the marker was forgeable, and nine other cross-vendor findings A cross-vendor review of the merged head found ten findings, nine blocking. The same-vendor reviewer had cleared this file twice. The marker was forgeable `MARKER_RE` matched only the attribution prefix, and `discloses()` allowed sixty trailing characters for a signature -- so ` forged` and even `_Posted by Claude Code (AI agent) bogus` discharged the guard. A check that text-which-is-not-the-marker can satisfy is not a check. The matcher now requires the whole marker (agent name still substitutable, per AGENTS.md) and only the marker's own closing punctuation may follow. That tightening exposed a fixture asserting a laxness the rule forbids: an invented tail, `-- not a human._`, which AGENTS.md's "keep the rest of the line verbatim" rules out and the prefix matcher happened to accept. Corrected, with the invented form pinned as a non-disclosure. Misclassified commands - `--comment` is a value flag on `gh issue|pr close|reopen` and a BOOLEAN action flag on `gh pr review`, so a flag-name list cannot settle it. Extraction now rejects a "value" that is itself a flag: `gh pr review 12 --comment --body "..."` warned on a compliant comment while the same flags reversed passed. - `glab mr note list|resolve|delete|update` and `gh pr comment --delete-last` post nothing and all warned. - `/comments` was searched across the whole segment, so `gh api repos/o/r/issues -f body='use the /comments endpoint'` -- an issue creation -- read as a comment post. The endpoint is now tokenized out, quote- aware, and order-independent. - The bot exemption searched the segment, so a quoted bot command anywhere in it exempted the real body. It now tests the extracted body. Spellings and edges `-fbody=`, `--raw-field=body=`, `-XGET`, `--method=GET` were all missed; a lowercase heredoc delimiter made a compliant body warn, since only an uppercase terminator was stripped. `gh api graphql --input ` now reports an unreadable body rather than nothing, since the mutation name lives in the file. Two fixtures superseded rather than kept Both asserted silence for `gh api graphql --input p.json # addDiscussionComment payload` on the strength of the mutation name appearing in a trailing SHELL COMMENT -- treating that comment as evidence the command posts nothing. It is not evidence. Replaced with the honest verdict and with a genuine mention-only case that does stay silent. `skills/gi` could not decide the question it asks Its jq returned the claim's body with no timestamp, and the 2-hour rule expires on thread ACTIVITY. A day-old claim followed by a comment thirty minutes ago is live; the same claim with nothing after it is expired; the old output was identical for both. It now returns `createdAt` and reads the issue's `updatedAt` alongside. Also: a fixture used `gh pr close --title`, which gh rejects as an unknown flag. 168 tests, up from 153. Eight mutations of the new logic all turn the suite red. A sweep of every fenced bash block in the corpus gives 4 warnings, all the by-design unreadable-body note, 0 MISSING. --- hooks/require-agent-disclosure.py | 143 ++++++++++++++++++++++--- hooks/test-require-agent-disclosure.py | 66 ++++++++++-- skills/gi/SKILL.md | 12 ++- 3 files changed, 200 insertions(+), 21 deletions(-) diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index f6495ea18..00c8f1ca3 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -99,8 +99,10 @@ # 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` @@ -135,7 +137,7 @@ # (`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|--form)\s+[\"']?body=" + r"(?:-f|-F|--field|--raw-field|--form)[\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 @@ -152,11 +154,78 @@ 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 @@ -168,7 +237,7 @@ def is_api_post(segment): # 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", + 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 @@ -212,8 +281,14 @@ def is_api_post(segment): r"|(?:-f|-F|--field|--raw-field)\s+[\"']?body=") +# `gh pr comment --delete-last` deletes a comment rather than posting one. +DELETING_RE = re.compile(r"--delete-last\b|--delete\b") + + def is_post_segment(segment): """True when this segment posts a forge comment by any route.""" + if DELETING_RE.search(segment): + return False if POST_RE.search("\n" + segment): # `gh pr review` is the one named verb that may carry no body at all. if (REVIEW_ONLY_RE.search("\n" + segment) @@ -251,7 +326,13 @@ def is_post_segment(segment): # The required marker, matched loosely enough to survive an agent-name swap # ("Posted by Codex (AI agent) ...") but tightly enough not to match prose that # merely mentions agents. -MARKER_RE = re.compile(r"posted by .{0,40}\(ai agent\)", re.IGNORECASE) +# The WHOLE marker, not its prefix. Matching `posted by ... (ai agent)` alone +# meant `_Posted by Claude Code (AI agent) bogus` satisfied the check, and +# ` 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 @@ -495,9 +576,23 @@ def bodies_for(segment, bodies): _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) > 1 def _read_quoted(text, i): @@ -544,7 +639,16 @@ def inline_body(segment): candidates.append((m.start(), "flag", m)) if not candidates: return None - _, kind, m = min(candidates, key=lambda c: c[0]) + 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) @@ -565,7 +669,10 @@ def discloses(text): # 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. - text = re.sub(r"\n\s*[A-Z_][A-Z0-9_]*\s*$", "", text) + # 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): @@ -578,8 +685,12 @@ def discloses(text): # 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. - after = tail[m.end():].rstrip() - return "\n\n" not in after and len(after) <= 60 + # 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): @@ -592,7 +703,13 @@ 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 _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: diff --git a/hooks/test-require-agent-disclosure.py b/hooks/test-require-agent-disclosure.py index 985e22011..00bd72b52 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", @@ -403,8 +421,10 @@ def GQL(body): ("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 --title "a|b" --comment "bare"', "missing"), + '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), @@ -462,6 +482,38 @@ def GQL(body): 'gh pr review 12 --request-changes -b"Findings.\n\n' + MARKER + '"', False), + # --- #2185 cross-vendor round ------------------------------------------- + # + # A same-vendor reviewer cleared this file twice before these were found. + ("a forged suffix after the marker does not disclose", + 'gh pr comment 1 --body "Done.\n\n' + MARKER + ' forged"', "missing"), + ("gh pr review's --comment is BOOLEAN, so --body is not its value", + 'gh pr review 12 --comment --body "Done.\n\n' + MARKER + '"', False), + ("the same review flags in the other order", + 'gh pr review 12 --body "Done.\n\n' + MARKER + '" --comment', False), + ("a quoted bot example inside a human body is not exempt", + 'gh pr comment 1 --body "Tell humans to run --body \'@dependabot rebase\' ' + 'now."', "missing"), + ("glab mr note list posts nothing", 'glab mr note list 12', False), + ("glab mr note delete posts nothing", + 'glab mr note delete 12 --note-id 5', False), + ("gh pr comment --delete-last posts nothing", + 'gh pr comment 12 --delete-last --yes', False), + ("an issue create whose BODY mentions /comments is not a comment post", + "gh api repos/o/r/issues -f body='Please use the /comments endpoint.'", + False), + ("-fbody= attached is a body field", + 'gh api repos/o/r/issues/12/comments -fbody=bare', "missing"), + ("--raw-field=body= is a body field", + 'gh api repos/o/r/issues/12/comments --raw-field=body=bare', "missing"), + ("-XGET attached is still a read", + 'gh api repos/o/r/issues/12/comments -XGET -f per_page=100', False), + ("--method=GET is still a read", + 'gh api repos/o/r/issues/12/comments --method=GET -f per_page=100', False), + ("a lowercase heredoc delimiter still discloses", + "gh pr comment 1 --body-file - <<'eof'\nHi.\n\n" + MARKER + "\n\neof", + False), + # --- unreadable vs missing must not be confused (review finding 9) ------- ("gh pr comment -F is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), diff --git a/skills/gi/SKILL.md b/skills/gi/SKILL.md index 2015ec3dc..3f552b4f7 100644 --- a/skills/gi/SKILL.md +++ b/skills/gi/SKILL.md @@ -85,7 +85,8 @@ check both explicitly here). ```bash # 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 | "\(.author.login): \(.body)"' # READ_ISSUE_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.** @@ -93,6 +94,15 @@ A claim is live for two hours from the most recent *activity*, so any unrelated 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. From 93363481ba392fabd8f6e5831bf6b29fd5d14997 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Mon, 24 Aug 2026 19:18:51 -0700 Subject: [PATCH 08/16] fix(hook): a flag token is never preceded by a quote Third round on the same false-positive class, and the reviewer is right that it deserves a named fixture category rather than ad hoc rediscovery. `(? 'onfig issue, see #3' (matched inside the VALUE) verdict -> MISSING (on a command posting NO comment) Not contrived: any free-text value opening "-config", "-close this", "-basically", "-both PRs" trips it, and the reviewer's own test invocation fired this repo's hook spuriously while reproducing it. A real flag token is never immediately preceded by a quote, so the boundary now excludes quotes too. Eight fixtures added as their own category, four of them commands that post nothing at all, four confirming the real flags still fire. Both mutations -- dropping the quote from the boundary, and dropping the boundary entirely -- turn the suite red. 176 tests. Filed #2189 for the structural answer. Every one of the three rounds' bugs is a TOKENIZATION problem, and `shlex` is POSIX shell lexing in the standard library: measured, it parses all three of the failing inputs correctly, makes each bug structurally impossible rather than patched, and replaces three hand-rolled quote-aware helpers that each reimplement part of it. Its one gap, ANSI-C `$'...'`, raises rather than mis-parsing silently. Not in this PR -- it is under review and a second session is pushing to it -- but the 176-case suite is exactly the harness that makes that rewrite verifiable. --- hooks/require-agent-disclosure.py | 14 ++++++------ hooks/test-require-agent-disclosure.py | 31 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index 00c8f1ca3..48f43bdf9 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -267,17 +267,17 @@ def is_api_post(segment): # no separator (`-cvalue`) or with an equals (`-c=value`). Requiring whitespace # missed both, on the exact posting surface this file was extended to cover. # -# `(? is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), From 1f90cee548cd8f1fa7b180169b85cf702f8596f5 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:39:45 -0700 Subject: [PATCH 09/16] fix(hook): recognize --form body=@file and --form body=\ as unreadable in glab api --- hooks/require-agent-disclosure.py | 8 ++++---- hooks/test-require-agent-disclosure.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index 48f43bdf9..33f920327 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -278,7 +278,7 @@ def is_api_post(segment): ANY_BODY_FLAG_RE = re.compile( r"--body\b|--body=|--body-file\b|--message\b|--message=" r"|(? Date: Mon, 24 Aug 2026 19:47:36 -0700 Subject: [PATCH 10/16] fix(hook): one field-flag list, and a positive flag boundary Third #2185 round, plus a merge with the concurrent session's fix for the same first finding. Finding 1: `--form` missing from UNREADABLE_RE, flagged twice `--form` was added to three field patterns and not to `UNREADABLE_RE`, so `--form body=@file` -- content the check cannot see -- was reported as CONFIDENTLY missing its marker. The concurrent session patched each of the four lists inline; this merge keeps the structural version instead. Four independent lists had now drifted three separate times (`-F body=`, `--raw-field`, `--form`), so they are one `_FIELD_FLAGS` constant. The drift is impossible now rather than reviewable. Finding 2: enumerating excluded characters was always going to leak Round B excluded word characters. Round C added quotes. Comma, paren and slash still matched, so `--duplicate-of "foo,-cool"` warned on a command posting no comment at all. The boundary is now POSITIVE -- `(?`. +# 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. @@ -137,7 +146,7 @@ # (`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|--form)[\s=]*[\"']?body=" + _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 @@ -267,18 +276,18 @@ def is_api_post(segment): # no separator (`-cvalue`) or with an equals (`-c=value`). Requiring whitespace # missed both, on the exact posting surface this file was extended to cover. # -# `(? is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), From 501feb1fd27557c9f3f5dc879741706626478793 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:01:59 -0700 Subject: [PATCH 11/16] test: document remaining tokenization gap for #2189 Adds a regression test for the multi-field gh api shape where a short flag inside a prior field's value still false-positives. This clarifies that the previous boundary fix only narrowed the bug class rather than fully closing it, which aligns with the planned shlex rewrite in issue #2189. --- hooks/test-require-agent-disclosure.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hooks/test-require-agent-disclosure.py b/hooks/test-require-agent-disclosure.py index a7c54ce82..2c753955d 100755 --- a/hooks/test-require-agent-disclosure.py +++ b/hooks/test-require-agent-disclosure.py @@ -575,6 +575,15 @@ def GQL(body): ("a slash before a hyphenated value", 'gh issue close 5 -R o/r --duplicate-of "foo/-config"', False), + # --- #2185 round 4: multiple fields where one quotes a flag name --------- + # + # Tracked by #2189: the regex-based tokenization matches `-m` inside the + # title's value, extracting the word after it ("flag") as the body, and + # reporting the real (correctly disclosed) body as missing its marker. + # This is a known false positive to be fixed by the #2189 rewrite. + ("a short flag inside a prior field's value false-positives (tracked in #2189)", + 'gh api repos/o/r/issues/5/comments -f title="Fix -m flag parsing" -f body="' + MARKER + '"', "missing"), + # --- unreadable vs missing must not be confused (review finding 9) ------- ("gh pr comment -F is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), From 38a851c15a6b4b4b37a0f55bbc2d756ee02f9ee2 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:10:36 -0700 Subject: [PATCH 12/16] ums: document gh run list -w ambiguity on shared workflow names --- memories/github.md | 4 ++++ 1 file changed, 4 insertions(+) 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). From 5b2b03090a060d6c87e36bfb552c8df69394ab92 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:18:23 -0700 Subject: [PATCH 13/16] fix(hooks): tighten flag-shape check to avoid matching hyphenated bodies --- hooks/require-agent-disclosure.py | 2 +- hooks/test-require-agent-disclosure.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index f2033b713..5054221b0 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -601,7 +601,7 @@ def _looks_like_flag(value): `--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) > 1 + return bool(value) and value.startswith("--") and len(value) > 2 def _read_quoted(text, i): diff --git a/hooks/test-require-agent-disclosure.py b/hooks/test-require-agent-disclosure.py index 2c753955d..7f519970f 100755 --- a/hooks/test-require-agent-disclosure.py +++ b/hooks/test-require-agent-disclosure.py @@ -491,6 +491,10 @@ def GQL(body): 'gh pr review 12 --comment --body "Done.\n\n' + MARKER + '"', False), ("the same review flags in the other order", 'gh pr review 12 --body "Done.\n\n' + MARKER + '" --comment', False), + ("a body starting with a hyphen before another flag is not a flag", + 'gh pr comment --body "- bullet" --repo o/r', "missing"), + ("a body starting with a hyphen before another flag WITH marker", + 'gh pr comment --body "- bullet\n\n' + MARKER + '" --repo o/r', False), ("a quoted bot example inside a human body is not exempt", 'gh pr comment 1 --body "Tell humans to run --body \'@dependabot rebase\' ' 'now."', "missing"), From d4d7c555b5a77be97b47cbf244763d506e216518 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:30:44 -0700 Subject: [PATCH 14/16] fix(hooks): document em-dash literal gap in docstring and test The fix for the single-hyphen body false positive intentionally left the double-hyphen case alone, as a known regex gap tracked in #2189. A comment body legitimately starting with \--\ or \---\ (e.g., an em-dash substitute) is misclassified as a boolean flag. Add a regression test specifically pinning this shape as a known gap, and correct the docstring's claim that no comment body begins with \--\ to reflect this reality. --- hooks/require-agent-disclosure.py | 7 +++++-- hooks/test-require-agent-disclosure.py | 9 +++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/hooks/require-agent-disclosure.py b/hooks/require-agent-disclosure.py index 5054221b0..fb4ee94a1 100755 --- a/hooks/require-agent-disclosure.py +++ b/hooks/require-agent-disclosure.py @@ -595,8 +595,11 @@ def _looks_like_flag(value): `--comment` takes a value on `gh issue|pr close|reopen` and is a BOOLEAN action flag on `gh pr review`, so a flag-name list cannot settle it. What - settles it is the value: no comment body begins with `--`, so an extraction - that yields one has consumed a boolean flag and read the next token. + settles it is the value: while a comment body might legitimately start with + a literal `--` or `---` (an em-dash substitute), this is a known gap + (tracked in #2189) we accept to avoid worse false positives. An extraction + that yields a double-hyphen prefix is assumed to have consumed a boolean + flag and read the next token. Without this, `gh pr review 12 --comment --body "..."` extracted `--body` as the body and warned on a compliant comment, while the same flags in the other order passed. diff --git a/hooks/test-require-agent-disclosure.py b/hooks/test-require-agent-disclosure.py index 7f519970f..0b4565a5c 100755 --- a/hooks/test-require-agent-disclosure.py +++ b/hooks/test-require-agent-disclosure.py @@ -588,6 +588,15 @@ def GQL(body): ("a short flag inside a prior field's value false-positives (tracked in #2189)", 'gh api repos/o/r/issues/5/comments -f title="Fix -m flag parsing" -f body="' + MARKER + '"', "missing"), + # Tracked by #2189: a comment body legitimately opening with a literal `--` or `---` + # (e.g. an em-dash) is misclassified by `_looks_like_flag` as a boolean flag, which + # causes `inline_body` to drop it and fall back to the segment-wide check. If another + # flag follows it, the segment-wide check fails because the marker is no longer at + # the exact end of the command string. + ("a body starting with a double hyphen false-positives if a flag follows (tracked in #2189)", + 'gh pr comment 1 --body "--- see the linked issue for context.\\n\\n' + MARKER + '\" --repo o/r', "missing"), + + # --- unreadable vs missing must not be confused (review finding 9) ------- ("gh pr comment -F is a body-file, reported unreadable", 'gh pr comment 12 -F /tmp/body.md', None), From 5e7b053a5c820c35ed6f99f6ee74bc39808a0f0a Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:42:15 -0700 Subject: [PATCH 15/16] fix(docs): apply semantic line breaks to recent main merge The recent merge from main (521419d4) introduced a line packing two sentences in memories/preferences.md, which tripped the new-line-breaks CI check for this branch because the PR base diff encompasses it. Split the line to fix CI. --- memories/preferences.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/memories/preferences.md b/memories/preferences.md index d8239e4d5..3e23624ff 100644 --- a/memories/preferences.md +++ b/memories/preferences.md @@ -1195,6 +1195,7 @@ safer/preferred choice merely because the repo has external consumers. Only purge them from prose and generic placeholder flags. - **When reverting a merge, immediately reopen the corresponding issue(s).** - If you revert a PR or merge commit that previously closed one or more tracked issues, the bug or feature request is no longer solved on `main`. You must immediately locate the issues that were closed by the reverted merge and reopen them so the work is tracked again. + If you revert a PR or merge commit that previously closed one or more tracked issues, the bug or feature request is no longer solved on `main`. + You must immediately locate the issues that were closed by the reverted merge and reopen them so the work is tracked again. - **Do:** reopen the issues that were closed by the reverted merge. - **Don't:** leave issues closed when the fix that closed them has been reverted from `main`. From 6a32948feb0a5bedfacccc0784dd5bf10cc3b769 Mon Sep 17 00:00:00 2001 From: dem-extra1 <112029334+dem-extra1@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:49:11 -0700 Subject: [PATCH 16/16] fix(docs): trim a blank line from preferences.md to satisfy size check The previous commit's line split pushed preferences.md to 1201 lines, tripping scripts/check-memory-file-size.py's 1200-line threshold on the validate job. Removed a redundant blank line elsewhere in the file to bring it back to 1200. --- memories/preferences.md | 1 - 1 file changed, 1 deletion(-) diff --git a/memories/preferences.md b/memories/preferences.md index 3e23624ff..7a62cada4 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.