diff --git a/memories/github.md b/memories/github.md index 59a008fd0..2d2756f82 100644 --- a/memories/github.md +++ b/memories/github.md @@ -88,6 +88,17 @@ The GitHub MCP tool surface used in remote/web sessions lives in So this needs no external `jq` and no `date -d`, which is GNU-only and absent on macOS. (Morrison-Lab/ai-config#908, 2026-07-30: the `clean-worktrees` merged-PR guard shipped the string comparison. Review caught it, and the repro above confirmed the failure direction before the fix went in.) +- **`gh pr list --state merged` plus a low `--limit` can miss recent merges:** + The list is ordered by PR list order, effectively number/creation, before your `--jq` filter runs. + That means an old, low-numbered PR that merged recently can sit below a page of higher-numbered PRs and never reach the filter. + The result looks scoped by time while silently excluding the very merge checkpoint you were polling for. + Use a query whose filter matches the question, such as `gh search prs --repo / --merged-at ">="`, or query each PR of interest directly. + If you use `gh pr list --state merged`, set `--limit` far beyond the expected count and report how many merged PRs the command examined, not only how many passed the `mergedAt` filter. + - **Do:** use `gh search prs --repo / --merged-at ">="`, direct `gh pr view `, or an intentionally over-wide list with an examined count when answering "what merged since T". + - **Don't:** trust `gh pr list --state merged --limit N --json mergedAt --jq '.[] | select(.mergedAt > T)'` as a time-window query. + (Morrison-Lab/ai-config#969, 2026-08-01: `gh pr list --state merged --limit 15 --json number,mergedAt` plus a `mergedAt > 2026-08-01T08:00:00Z` filter returned only #1019, merged at `09:03:13Z`, and missed #969, merged at `09:14:38Z`. + Raw `--limit 6` output showed #1013 at `05:36Z` before #1012 at `05:45Z`, proving the page was not sorted by merge time. + Raising the limit to 30 returned both #1019 and #969.) - **`gh pr edit` exits 1 on repos with Projects Classic — use `gh api` to update PR body.** `gh pr edit --body "..."` / `--body-file ` returns exit code 1 with a GraphQL deprecation warning (`Projects (classic) is being deprecated…`). Sometimes the edit lands anyway; **sometimes it does not apply at all** (seen on sparta 2026-06-30: three `gh pr edit --body-file` attempts left the body unchanged with the `SHA_PLACEHOLDER` still in place). Either way, don't trust it — verify with `gh api repos///pulls/ --jq .body`, and just use the REST PATCH directly, which always exits 0 and applies: `gh api -X PATCH repos///pulls/ -f body="..."`. For a multi-line body, read it from a file with `-F body=@` (capital `-F` to pull the field value from the file) rather than cramming it into `-f body="..."`. - **PR description image embeds: use `raw.githubusercontent.com`, not `github.com/.../raw/...`.** Embedding a committed file in a PR body with `![](https://github.com///raw//)` may not render — the reviewer will flag it. The correct raw-content domain is `https://raw.githubusercontent.com////`. Reference the full commit SHA so the image keeps rendering after the branch is deleted on merge. - **`raw.githubusercontent.com` FOLLOWS repository-rename redirects, so a `200` under the OLD owner proves nothing — only a `200` under the NEW owner is decisive.** To test whether a repo has moved, probe the *new* name and treat `404` there as "did not move". Run a known-moved repo as a control first, or the probe silently answers backwards: `d-morrison/gha` still returned `200` on `raw.githubusercontent.com` well after it became `Morrison-Lab/gha`, so an old-name probe reports every repo as "not moved". The REST API is not a substitute — behind an agent proxy `api.github.com/repos//` can return `403` for every repo regardless of existence, which answers nothing in either direction. This matters before any blanket owner rewrite: probing all nine `d-morrison/*` references in ucdavis/bcs under the new owner showed only `gha` and `ai-config` had moved, so a find-and-replace would have broken `macros`, `altdoc`, `snapr`, `stats-allowlist`, `diffviewer`, `equation-anchors`, and `rme`. Note the bare `d-morrison` *username* (a `reviewer:` input, author metadata) is unaffected by a repo/org rename and must not be swept along. The Actions-side consequences of the same rename are in `github-actions.md` ("A repo/org rename breaks Actions `uses:` refs"). (2026-07-28.) diff --git a/shared/workflow/address-every-comment.md b/shared/workflow/address-every-comment.md index 82ba9e97d..9f5237fe2 100644 --- a/shared/workflow/address-every-comment.md +++ b/shared/workflow/address-every-comment.md @@ -513,6 +513,38 @@ Matching whole lines against the tag -- how bash itself ends a heredoc -- removed the whole lazy-quantifier/anchor failure mode instead of narrowing it; the reply carried the failing output of the suggested form.) +**A reviewer's corrected citation is another factual claim, so verify the +replacement before adopting it.** +The finding can be right: the citation in the PR can name the wrong source. +That does not make the reviewer's proposed source right. +A replacement issue or PR number is a fresh provenance claim, and it needs the +same check as the original citation. +For text provenance, prefer history over word association: +`git log -S "" -- ` asks which commit introduced the line, +while matching a word in another PR plus a nearby merge time only builds a +story. +Keep the review's conclusion when it is right, but set aside the replacement +when the evidence points elsewhere, and say which query decided it. + +- **Do:** verify a proposed replacement citation with the source's own history + before editing the PR to use it. +- **Do:** use `git log -S "" -- ` or an equivalent + provenance query when the question is which PR introduced text. +- **Don't:** adopt a reviewer's corrected issue or PR number because the + original was wrong. +- **Don't:** use word overlap and same-day timing as a substitute for source + history. + +(Morrison-Lab/ai-config#971 round 2, 2026-08-01: a review correctly found that +PR #955 did not cover a "default nobody chose" case record. +It then proposed #951 as the source because #951's `memories/tools.md` entry +used the word "default" and merged the same day. +That was the wrong default and the wrong file: #951 did not touch +`shared/workflow/metacognitive-monitoring.md`. +`git log -S "An unexamined default" -- shared/workflow/metacognitive-monitoring.md` +identified #947 as the source for the default half, while #955 supplied the +handed-premise half in the same fragment.) + **The highest-yield version of that check: when a comment names an edge case in its own prose and also supplies a fix, run the fix against that edge case.** diff --git a/shared/workflow/challenge-redundant-content.md b/shared/workflow/challenge-redundant-content.md index 58fed559b..cf0b6d9f0 100644 --- a/shared/workflow/challenge-redundant-content.md +++ b/shared/workflow/challenge-redundant-content.md @@ -42,3 +42,46 @@ the same procedure copied into several unrelated places), say so and route it to `find-overlap` (or `consolidate-skills`/`consolidate-memory` for skills/memories), rather than trying to fix everything found along the way in the current review. + +## Use the overlap instrument, not literal grep + +A redundancy finding is exactly where phrase grep is most tempting, and where +it is least trustworthy. +This corpus uses semantic line breaks and inline markup, so a copied claim can +span a newline or differ only by backticks while still being the same prose. +Use the existing overlap instrument for the broad sweep: +`scripts/find-near-duplicates.py` ranks corpus pairs by normalized word +shingles and reports the number of units and pairs it examined. +Its default target set and top-pair threshold are tuned for near-duplicate +files, not for proving that a short copied run between long `shared/` files is +absent. +So treat it as the reusable starting point and corpus-wide reading list, not as +a clean-result certificate for a narrow review finding. +When the question is a narrower single-file audit, use the same algorithmic +shape with a run-reporting matcher: +normalize whitespace and markup on both sides, lower-case, compare word +n-grams, and report the examined scope and every overlapping run. + +- **Do:** run `scripts/find-near-duplicates.py` for the broad candidate list, + or run an equivalent normalized n-gram matcher that reports overlapping runs + for the files under review. +- **Do:** report the examined files, pairs, or runs alongside any overlap found, + then read the overlapping bodies before deciding whether consolidation loses + nothing. +- **Don't:** use literal grep as evidence that no matching prose exists in a + semantic-line-break corpus. +- **Don't:** treat `scripts/find-near-duplicates.py`'s default no-finding result + as proof a short copied passage is absent from long shared files. +- **Don't:** treat a high shingle score as the disposition; it is a pointer to + read, not a duplicate verdict. + +(Morrison-Lab/ai-config#969, 2026-08-01: after review flagged one uncited +copy between `batch-merge-and-resolve.md` and `sync-with-main.md`, a +10-gram shingle sweep normalized backticks, asterisks, underscores, whitespace, +and case across `shared/workflow/`, `shared/writing/`, and +`shared/principles/`. +It found four overlapping runs: the flagged generalization, a second uncited +copy about `markdownlint`'s `blanks-around-lists` explanation that review had +missed, and two already-cited overlaps. +A literal grep for the `blanks-around-lists` wording in `sync-with-main.md` +returned nothing because the phrase crossed a semantic line break.) diff --git a/shared/workflow/sync-with-main.md b/shared/workflow/sync-with-main.md index 18c46bf3d..8376b54c9 100644 --- a/shared/workflow/sync-with-main.md +++ b/shared/workflow/sync-with-main.md @@ -195,6 +195,40 @@ Dropped before implementation, with the reasoning recorded in both the issue and the PR body, and the neighbouring fragments cross-linked to the skill instead.) +**A routine merge from `main` can create the duplicate inside your own diff.** +The collision above lands before you write, so the duplicate is redundant on +arrival. +A later `main` merge is quieter: both branches were non-duplicative when they +were written, and the duplicate appears only when you bring the other branch's +text into yours. +Git reports a clean merge because the two copies sit in different files. +Diff-scoped added-line checks do not help either, because the duplicated lines +already existed on one side or the other. +So after merging `main` into a prose branch, run the duplicate check against +the branch's full current diff and the neighbouring corpus, not only against +lines added by the merge commit. + +- **Do:** after a `main` merge, re-run a cross-file duplication check over the + merged branch's whole prose diff. +- **Do:** treat a reviewer finding on such duplication as correct even when + each copy was independently right before the merge. +- **Don't:** assume a conflict-free `main` merge preserved DRY, or that the + duplicate would have appeared in an added-lines-only scan. +- **Don't:** answer by asking which branch "introduced" the duplication; + the merge introduced the state that made both copies coexist. + +(Morrison-Lab/ai-config#969, 2026-08-01: #969 added +`shared/workflow/batch-merge-and-resolve.md` with a blockquote generalizing +that an added-lines-only instrument is unsound when a defect can be introduced +by deleting a line. +PR #966 independently added the same generalization to +`shared/workflow/sync-with-main.md` and merged after #969's branch was written. +`git show 50afe818:shared/workflow/sync-with-main.md`, normalized for +whitespace and markup, did not contain the phrase, so the duplication did not +exist at #969's pre-merge head. +The round-2 merge from `main` brought #966's copy in, and the round-3 review +correctly flagged the two uncited copies.) + **Two PRs that each append a new terminal numbered subsection to the same file (e.g. `### 5. ...` in a `CLAUDE.md` review-guidelines list) will conflict on merge even when neither side's content actually disagrees.**