From ad216c119583435d689211e4b331918297089781 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Sun, 9 Aug 2026 19:02:05 -0700 Subject: [PATCH 1/7] memory: confirm permissions-cascade gotcha recurs in Morrison-Lab/psw Hit the same startup_failure shape this file already documents (a caller under-granting a permission the nested job requests), this time actions: read rather than issues: read/write, in Morrison-Lab/psw's claude-code-review.yml caller. Confirms the pattern isn't d-morrison/qwt-specific and that a plain WebFetch on the run's html_url reliably surfaces the Annotations text. --- memories/gha-reusable-workflow-permissions.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/memories/gha-reusable-workflow-permissions.md b/memories/gha-reusable-workflow-permissions.md index e4d4c0ed..7d288516 100644 --- a/memories/gha-reusable-workflow-permissions.md +++ b/memories/gha-reusable-workflow-permissions.md @@ -43,6 +43,18 @@ completes with `startup_failure` and an empty `jobs` array. - **Do:** see `github-actions.md`'s "GitHub Actions workflow authoring gotchas" for the permission-grant mechanism and remedy; this only adds where to find the error text once you've hit it. +- **Confirmed again, different repo, different permission**: + `Morrison-Lab/psw`'s `claude-code-review.yml` caller granted `contents`, + `pull-requests`, `issues`, `id-token` but omitted `actions`, while the + callee's `claude-review` job requests `actions: read` (to let + `claude-code-action` install its `github_ci` MCP server). Same + `startup_failure`, zero jobs, nothing in `pull_request_read` + `get_check_runs` or `get_job_logs`. `WebFetch` on the run's `html_url` + reliably surfaced the Annotations text verbatim -- "The nested job + 'claude-review' is requesting 'actions: read', but is only allowed + 'actions: none'." -- confirming this isn't `d-morrison/qwt`-specific and + that a plain `WebFetch` (not just a dedicated `get_page_text` browser + tool) is sufficient to read it. (Morrison-Lab/psw#43/#44, 2026-08-10.) ## A caller-level `concurrency:` group with the same name as a nested job's own group deadlocks the run From 903c98d005c720ad1b7224e215e97e645d69162b Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Sun, 9 Aug 2026 19:06:02 -0700 Subject: [PATCH 2/7] memory: confirm base64 content-param gotcha with a new symptom Hit the already-documented create_or_update_file base64 mistake again on Morrison-Lab/psw#44, this time producing an inflated size (not a truncated one) and a different downstream GitHub Actions symptom (dispatch-time "no workflow_dispatch trigger" plus a generic failure conclusion, rather than startup_failure). Appends to the existing bullet rather than duplicating it. --- memories/github-mcp-tools.md | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/memories/github-mcp-tools.md b/memories/github-mcp-tools.md index ad4f6d82..776d1de8 100644 --- a/memories/github-mcp-tools.md +++ b/memories/github-mcp-tools.md @@ -381,6 +381,23 @@ See ai-config#694 for the precedent. leave a broken file on the branch waiting for the next review round to catch it. (Hit on lab-manual#376: an editing slip sent a truncated placeholder instead of the real fragment text; caught by checking the returned `size`.) + **Confirmed again with a different downstream symptom, and it defeats a + naive round-trip check.** Passing an already-base64-encoded string produced + a `size` of 2310 bytes for content that should have been 1710 -- a ~4/3 + inflation, the base64 expansion ratio, rather than a suspiciously *small* + number this time. The GitHub Actions symptom was different too: since the + stored blob was a bare base64 scalar rather than a YAML mapping, the + workflow read as having no triggers at all -- a dispatch-time `422 + Workflow does not have 'workflow_dispatch' trigger` on that ref (dispatch + to the unmodified default branch worked fine), and the push itself + produced a generic `failure` conclusion with zero jobs (not + `startup_failure`, which is the permissions-cascade shape covered + elsewhere in this file). A naive "does it decode without erroring" + round-trip check does not catch this: base64-decoding what + `get_file_contents` reads back just undoes your own accidental encoding + and returns the intended text, which looks like confirmation. The `size` + comparison against the source's real byte length is the check that + actually discriminates. (Morrison-Lab/psw#44, 2026-08-10.) - **Issue *writes* 404 while *reads* succeed → the issue was transferred to another repo, not a permissions gap.** If `mcp__github__add_issue_comment` / `issue_write` to `owner/repo#` fail (`404 Not Found`, or `Could not resolve @@ -708,4 +725,3 @@ See ai-config#694 for the precedent. where the table actually lives. (Caught in ai-config#137 review: the gip skill referenced a table ai-config didn't have at the time; ai-config#327 later added `tool-mappings.md` to close that gap.) - From 8e7a25260d9ac403a1f0d6aee7cdefd343a7c52e Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Sun, 9 Aug 2026 21:16:13 -0700 Subject: [PATCH 3/7] fix: correct false self-reference in github-mcp-tools.md Review finding: "covered elsewhere in this file" was false --- the permissions-cascade startup_failure shape is documented in the sibling file, memories/gha-reusable-workflow-permissions.md, not in this file. Point at it explicitly, matching this corpus's existing cross-file-link convention. --- memories/github-mcp-tools.md | 728 +---------------------------------- 1 file changed, 1 insertion(+), 727 deletions(-) diff --git a/memories/github-mcp-tools.md b/memories/github-mcp-tools.md index 776d1de8..0c7666f9 100644 --- a/memories/github-mcp-tools.md +++ b/memories/github-mcp-tools.md @@ -1,727 +1 @@ -# GitHub MCP tools (Claude Code remote/web sessions) - -The GitHub MCP tool surface used in remote/web sessions where the `gh` -CLI is unavailable --- tool selection, scope and owner-string quirks, -review/comment/thread mechanics, and the specific failure modes each -tool has shown in practice. -Split out of `github.md` pre-emptively at 1199 lines, just under -`scripts/check-memory-file-size.py`'s gate --- that check fires strictly -above 1200 lines, so the file never actually tripped it. -See ai-config#694 for the precedent. - -- In remote/web sessions the authenticated GitHub identity is the repo owner - (`d-morrison`), so requesting `d-morrison` as a PR reviewer fails with - `422 Review cannot be requested from pull request author`. Harmless — the PR - is still created; the reviewer just isn't added. Don't treat the 422 as a - failure to retry (it's expected per the standing request-pr-review rule when - the author == the requested reviewer). -- `gh` is NOT available in these sessions — use the `mcp__github__*` tools for - all GitHub interactions (PRs, issues, comments, reviews). CI status is always - available via `mcp__github__pull_request_read` (`get_check_runs` / `get_status`) - and the `mcp__github__actions_*` tools. Some environments may *also* expose a - separate `github_ci` MCP server (`mcp__github_ci__*`, e.g. `get_ci_status`), - which can connect asynchronously after session start. Don't conclude a tool is - absent from one check — `ToolSearch` for what you need before deciding it's - missing (and don't assume the `github_ci` server is present either). -- **An angle-bracket placeholder can vanish from a PR or issue body posted - through these tools, and no markdown construct protects it.** - A PR body written with `` `git ls-remote https://github.com//` `` - came back from the API as `git ls-remote https://github.com//`, with both - placeholders gone. - **No markdown construct protects them.** - A controlled test posted the same string four ways in one body --- plain - prose, an inline code span, an indented code block, and a fenced code block - --- and all four came back stripped, so the removal happens to the raw body - text and never reaches markdown parsing. - The instinct that backticks make text literal is therefore wrong twice over - here: neither the span nor the fence helps. - What is lost is the *stored* body, not one rendering of it, so re-reading or - re-rendering will not bring it back. - The blast radius is narrower than it first looks, and worth knowing precisely: - only text sent as a body through the API is affected. - A `` inside a file committed in the same PR is untouched, so a - memory entry documenting a command survives while the PR description quoting - that same command does not. - Write placeholders in a body without brackets --- `OWNER/REPO`, `PATH`, `N` - --- and re-read the body after posting whenever the exact text matters. - This is the "Postcondition gate" bullet in [`github.md`](github.md) made concrete: - nothing errors, the object is created exactly as asked, and only reading the - stored result back shows the content is not what was sent. - (ai-config#734, 2026-07-26: caught only because the mangled URL happened to be - re-read during an unrelated check.) - **A later observation narrows what "angle-bracket" means here, and its - mechanism is unconfirmed.** - A PR body posted through `create_pull_request` on 2026-07-31 came back - missing ``, ``, and ``, leaving a documented - command with no path and a `git reset --hard origin/` with no branch --- so - the stored body carried instructions a reader would run and get wrong. - Two contrasts stop that from reducing to "angle brackets are stripped". - In the same body `<=` survived, stored as the escaped entity `<=`, so - only tag-shaped tokens went. - And `` survived intact in a PR comment posted through - `add_issue_comment` in the same session (ai-config#965), inside backticks in - a blockquote, so the two write surfaces did not behave alike. - Which layer strips --- the MCP tool, GitHub's sanitizer, or the two - composed --- was not established, so read this as an observed effect rather - than as a mechanism, and do not generalize either surface's behaviour to the - other. - The mitigation is unchanged and cheap: spell a placeholder in caps - (`BRANCH`, `GHA-CHECKOUT`, `BASE`) in any body, where nothing can read it as - a tag. - Files in the diff were unaffected, as above --- the angle-bracket form - inside a fenced code block is correct there and should stay. -- **`mcp__github__actions_run_trigger` can't re-run CI jobs in these sessions — - it 403s.** `method: rerun_failed_jobs` (and `rerun_workflow_run`, and - `cancel_workflow_run` -- the whole `actions: write` family, so you can neither - restart a run nor stop one) returns - `403 Resource not accessible by integration`: the integration token lacks the - `actions: write` the re-run API needs. So a flaky CI failure can't be re-kicked - via MCP — **push a commit to re-trigger the whole workflow** (the normal path - during an iterate loop anyway), or ask the user to click Re-run. (Hit - re-running a flaky `link-checker` timeout on a lab-manual PR.) **`method: - run_workflow` (a fresh `workflow_dispatch`, not a rerun) 403s the same way** — - the token lacks `actions: write` for dispatch too, not just for reruns, so - don't expect a direct-dispatch workaround to succeed where rerun failed - (confirmed on UCD-SERG/serodynamics#193, and again on `d-morrison/rme#1017` - trying to dispatch `publish.yml` — same `403 Resource not accessible by - integration`). Prefer folding - the retry into a real, already-pending fix (e.g. a reviewer's requested - wording tweak) over pushing a bare `--allow-empty` commit — same retrigger, - no throwaway commit in history. Only use an empty commit when no real fix is - pending. (ai-config#403.) **When the failing workflow only triggers on - `push: main` / `workflow_dispatch` (no `pull_request` trigger), there's no - "push a commit to re-trigger" fallback either** — nothing exercises the - actual failing job pre-merge. Ask the user to dispatch it manually from the - Actions UI (share the exact workflow filename + branch), and get their - explicit go-ahead first if the workflow has a real side effect (e.g. a - gh-pages deploy step not gated to `main`) — dispatching isn't just a status - check in that case, it's a live action. -- **`issue_write`'s `labels` REPLACES the issue's whole label set, and a name - that does not exist yet is silently CREATED rather than rejected.** Two - independent surprises in one parameter, pulling in opposite directions. - The replace semantics come from the underlying REST "update an issue" - endpoint, so passing `["needs-data"]` to an issue already carrying - `["bug","tech-debt"]` drops both, with no warning and nothing in the - response to notice --- always pass the **union** of existing plus new. - Read the current labels first; `list_issues` already returns them, so a - bulk pass needs no extra call per issue. - The auto-creation runs the other way: it means a typo becomes a real label - rather than an error, so a misspelling silently splits a set in two. - Confirmed on `ucdavis/bcs`, 2026-07-29: applying `needs-data` to an issue - in a repo that had no such label created it, and `get_label` then returned - it with the default grey `#ededed` and an empty description. - **Nothing in the MCP tool set can set a label's color or description** --- - there is only `get_label` (`GET_LABEL` in - [`tool-mappings.md`](../tool-mappings.md)), no create/update --- so a label - born this way stays grey and undescribed until a human with **write** - access fixes it, or a workflow with `issues: write` does it via `gh api`. - Write, not admin: the Labels REST API's create/update endpoints need push - access, while admin governs repository settings, branch protection, and - webhooks. - Note that the Triage role can *apply* an existing label but cannot create - or edit one, so it is not sufficient here. - Say so when handing off, rather than leaving someone to wonder why the new - labels look unstyled. -- **Comments/replies you post via the GitHub MCP tools echo back into the - session's `` events under the human account's - identity, not a bot identity.** `add_reply_to_pull_request_comment` and - `add_issue_comment` authenticate as the human who owns the session (e.g. - `d-morrison`), so a webhook event for your own just-posted reply shows - `Author: d-morrison` (or whichever human), never a recognizable bot name - like `claude[bot]`. Don't use the author field to decide "is this my own - echo, skip it." This is easy to get wrong at a glance since a same-author - event looks exactly like a genuine human reply demanding a response. - **Check for the Claude Code attribution footer instead of fuzzy-matching - body text/timing** --- every comment posted from these sessions ends with - `_Generated by [Claude Code](https://claude.ai/code)_` per the system - prompt's attribution-footer requirement, so a webhook event whose body - ends with that footer is a much sharper signal than eyeballing whether the - wording looks familiar. (Hit repeatedly on - `UCD-SERG/serocalculator#503`, 2026-07-24: several - `add_reply_to_pull_request_comment` calls immediately produced a webhook - event attributed to `d-morrison` quoting the reply verbatim --- each one - a self-echo, not a new human comment, confirmed each time by re-reading - the body rather than checking for the footer directly.) - - **Scope that to the question it answers: is this a self-echo or a - *human* reply.** - Against a human it is decisive, because a human reply does not carry the - footer. - It is **not** proof the comment is this session's own post, and reading - it that way fails in two directions. - The footer is body text, so anyone who can comment can paste it --- which - matters the moment the surrounding question is adversarial rather than - merely a self-echo check. - And it identifies a **class** ("some Claude Code session"), not an - **instance**: a PR Steward or another session watching the same PR - carries the identical footer, so a peer agent's comment reads as yours. - Distinguishing *this* session needs something the body cannot forge --- - match the comment id against a call you made, or the run URL against a - run you own. - - **The absence of a footer survives both objections**, which is what keeps - the check useful. - Neither a paste nor a peer session *removes* a footer from a comment that - would otherwise carry one, so a body ending without one is near-conclusive - evidence it is not agent-posted at all. - Read the signal one-directionally: absence rules agent authorship out, - presence does not rule it in. - (`Morrison-Lab/wai#54`, 2026-08-09: this bullet's "mechanically, - unambiguously your own post" was transplanted into a chapter that states a - threat model, and the review's finding 5 correctly called it undercut --- - "the footer is part of untrusted comment data, and doesn't distinguish - this session from a peer agent session". - Note the claim never changed; only the question around it did. - See - [`check-purpose-before-reusing`](../shared/workflow/check-purpose-before-reusing.md)'s - "Reusing a CLAIM" section.) -- **A sustained run of `503` responses across every endpoint (not just PR - reads) is a GitHub-side outage, not a per-call glitch — confirm with the - cheapest possible probe, then stop retrying and back off.** When - `pull_request_read`/`list_pull_requests` both 503, don't keep hammering the - same call — call `mcp__github__get_me` (no arguments, smallest possible - request) once: if that 503s too, it's a broad outage rather than something - scoped to one repo, PR, or endpoint, and no amount of retrying the original - call will help. Report the outage plainly, use whatever was last confirmed - before it started, and re-check later rather than looping. (ai-config#583/ - #585 session, 2026-07-16: `pull_request_read`, `list_pull_requests`, and - `get_me` all 503'd for roughly an hour across several separate check-ins; - confirmed via `get_me` that it wasn't scoped to the two PRs being watched.) -- `mcp__github__pull_request_read` `method:` enum: `get` · `get_diff` (PR - unified diff — equivalent to `gh pr diff`) · `get_status` · `get_files` · - `get_commits` · `get_review_comments` · `get_reviews` · `get_comments` · - `get_check_runs`. -- **`mcp__github__request_copilot_review` is a real, separate tool** (not a - `pull_request_read` method) -- requests a Copilot code review on a PR, - equivalent to `gh api .../requested_reviewers -X POST -f - "reviewers[]=copilot-pull-request-reviewer[bot]"`. Verified directly - against `github/github-mcp-server`'s own source - (`pkg/github/copilot.go`'s `RequestCopilotReview`), registered in the - **default** toolset (`pkg/github/tools.go`), not behind an opt-in flag -- - don't assume a tool is a hallucination just because it's absent from this - file, which is a running collection of quirks encountered, not an - exhaustive registry. -- **`request_copilot_review` returns success even when Copilot's quota is - exhausted -- the refusal arrives later, as a posted review.** - The tool reports no error and no output whether or not Copilot will - actually review; what comes back minutes later is a `COMMENTED` review - whose entire body is *"Copilot was unable to review this pull request - because the user who requested the review has reached their quota - limit"*. - So a clean return is **not** evidence the quota is back, and neither is - the absence of an error --- only the posted review body settles it. - Two further specifics: - - The quota is **per requesting user**, not per repo or per PR, so every - request from the same account keeps refusing until it resets, however - many different PRs it's spread across. - - **Latency is a weak tell, and an untested one.** - Every refusal came back within roughly a minute of the request. - A later request was still pending when last checked about ten minutes - in, which is the only reason to suspect a long-pending request may be - a real review rather than a slow refusal -- but its outcome was never - observed, because the PR merged first. - So treat a long wait as weak grounds for holding off on re-requesting, - not as evidence a review is coming, and read the posted review either - way. - Copilot and the `@claude` reviewer fail **independently**: Copilot can be - quota-dead while `claude-review` posts genuine verdicts at the same head, - so a Copilot refusal is never a reason to stop checking the other one. - (`ucdavis/rampp#111`, 2026-07-24/25: three refusals across two heads while - `claude-review` reviewed both normally, and Copilot itself had worked on - the same PR two days earlier.) -- **A branch ruleset can block Copilot from pushing a fix while leaving my - own push to the same branch unaffected.** - When Copilot reports it prepared a change but could not apply it --- - e.g. *"Cannot update this protected ref"* --- don't infer the branch is - write-protected for this session too: try the push. - The corollary matters more for review triage: a Copilot-identified issue - still sitting unfixed may be unfixed because its push was rejected, - **not** because the fix was wrong, disputed, or deliberately dropped. - Re-check such a finding on its own merits rather than reading "Copilot - left it alone" as a signal it was already settled. - (`ucdavis/rampp#111`: Copilot had prepared the `DESCRIPTION` version bump - that `version-check` was failing on and was rejected with that error; the - identical fix pushed fine from this session as `0c72d81`.) -- **`get_status` can return "pending / 0 checks" even after CI has finished.** - Use `get_check_runs` for the real job conclusions (`success`, `failure`, - `skipped`) --- but see the bullet below: it is the more reliable of the two, - not an authoritative source. - `get_status` aggregates - across check suites and can lag or show a stale "pending" when all runs have - actually completed; `get_status` is unreliable for CI state. - (Hit during the ai-config #275 GII session — `get_status` showed - `total_count: 0` / `pending` while `get_check_runs` correctly showed all 5 - checks `success`.) - **Given this, don't call `get_status` at all when checking CI state** — - go straight to `get_check_runs`; calling both in parallel "to be safe" - just spends a call on a field you already know not to trust. (Repeated - on `Lacaedemon/sparta` PR #780, 2026-07-12: called both in parallel to - confirm a canceled-review race, when `get_check_runs` alone — or, when - the incoming webhook event already names the failing commit's SHA, a - single `pull_request_read` `get` compared against that SHA — would have - settled it in one call. See - [`efficient-pr-babysitting`](../shared/workflow/efficient-pr-babysitting.md).) -- **`get_check_runs` is the better of the two, but it is not authoritative: - it can report a job as `in_progress` minutes after that job finished.** - The entry above says to prefer it over `get_status`, which still holds --- - but read that as "less stale", not "correct". - `actions_get` `get_workflow_job` on the same job id returns the true - `status`/`conclusion`, and the two disagree often enough to matter. - The cross-check is cheap and decides it exactly, so run it rather than - reasoning about how long the job "should" have taken. - It is worth running in **both** directions. - Concluding "still running" from a stale `in_progress` only wastes a wait; - the dangerous inverse is a - rollup that has not yet caught up with a job that has since failed, which - is why the cross-check belongs in the declare-clean sweep - ([`fully-clean`](../shared/workflow/fully-clean.md) criterion 1) and not - only when something looks slow. - Do not over-correct, either: on the same PR minutes later, an - `in_progress` R-CMD-check was genuinely still running, and the runs - endpoint confirmed it. - The endpoint is unreliable, not wrong. - (`d-morrison/altdoc#61`, 2026-07-25: three instances in one afternoon --- - `test-coverage`, `docs-check` (completed `21:12:56`, still reported - `in_progress` after), and one true negative.) -- **`list_pull_requests` reports `merged: false` for every PR, merged ones - included; `merged_at` is the field that discriminates.** - The two bullets above are about *staleness*, where a field is sometimes - wrong; here it is **constant**, so it is wrong for every merged PR while - looking correct on any unmerged one you spot-check it against. - A constant carries no information, the argument - [`fully-clean`](../shared/workflow/fully-clean.md) also makes for `.state`. - Measured on `d-morrison/ai-config`, 2026-08-01, over 101 rows all `false`: - - | field | open (#1006) | merged (#1005) | closed unmerged (#505) | - |---|---|---|---| - | `list` `merged` | `false` | `false` | `false` | - | `list` `merged_at` | absent | present | absent | - | `get` `merged` | `false` | `true` | `false` | - - **It is not the `fields` projection**, the first thing to suspect and a - different remedy: passing no `fields` argument at all returns the same value. - `merged_by` is no fallback either, never served in a list response even when - named in `fields` -- consistent with the list endpoint returning GitHub's - smaller representation, though that is inferred rather than read from source. - - **Do:** decide merged-versus-closed from `merged_at`, and call - `pull_request_read` `get` when you need `merged` itself. - - **Don't:** report a PR as closed-unmerged on a list response's `merged` - field -- it says that about every PR in the repo. -- **`mcp__github__actions_list` (`list_workflow_runs`) returns a full repository - object per run -- budget accordingly, and prefer a cheaper call.** Each run in - the response carries `repository`, `head_repository`, `actor`, and - `triggering_actor` in full, so even `per_page: 1` runs ~30-60KB and a - `per_page: 3` call costs several thousand tokens; a large enough response - blows the tool-output cap and gets spilled to a file instead of returned. - When the question is "did CI/the review run, and how did it end", - `pull_request_read` `get_check_runs` answers it for a fraction of that, and - `actions_get` `get_workflow_run` (a single run by ID) is the right call when - you need one run's event/trigger/conclusion. Reserve `list_workflow_runs` for - when you genuinely need to enumerate runs the check-runs view can't see -- the - `action_required`/zero-job case in - [`fully-clean`](../shared/workflow/fully-clean.md) -- and when a call has - already spilled to a file, parse that file - (`python3 -c "json.load(...)"`) rather than re-listing. - (ai-config#687, 2026-07-24: - a two-run `list_workflow_runs` call to check whether a draft PR's review had - fired cost ~6k tokens; `get_check_runs` gave the same answer.) -- **`gh pr view --json checks` is not a valid field.** When you need the - combined status/check rollup from `gh pr view`, ask for `statusCheckRollup` - instead; when you need the actual CI conclusions, use `gh pr checks` or the - REST check-runs endpoint. -- **`mcp__github__push_files` strips executable bits** — files pushed via this - tool always land with mode `100644`, regardless of their original mode. Scripts - that were `100755` become non-executable. This is harmless when the workflow - invokes them via `bash