Skip to content

Capture PR #969 UMS lessons - #1021

Merged
d-morrison merged 4 commits into
mainfrom
ums/merge-created-duplication
Aug 1, 2026
Merged

Capture PR #969 UMS lessons#1021
d-morrison merged 4 commits into
mainfrom
ums/merge-created-duplication

Conversation

@d-morrison

@d-morrison d-morrison commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Closes #1020

Summary

  • Add a gh pr list --state merged gotcha to memories/github.md.
  • Record that a routine main merge can create cross-file duplication in a prose PR.
  • Point redundancy review at the existing normalized overlap instrument instead of literal grep.
  • Record that a reviewer's corrected citation is itself a factual provenance claim.

Existing coverage checked

  • memories/github.md had related mergedAt guidance, but not the gh pr list --state merged ordering trap.
  • shared/workflow/sync-with-main.md had adjacent merge-collision guidance, but not clean merges creating a DRY violation across files.
  • scripts/find-near-duplicates.py already provides normalized shingle-style overlap detection, so the new entry says to use the existing instrument.
  • shared/workflow/address-every-comment.md covered wrong suggestions, but not corrected citation provenance.

Verification

  • python3 scripts/validate-skills.py -> valid, with the pre-existing uninitialized sembr-skills submodule warning.
  • python3 scripts/check-memory-file-size.py -> no memory file exceeds 1200 lines.
  • NLB_BASE_REF=origin/main python3 /Users/ezramorrison/Documents/GitHub/gha/check-new-line-breaks/check-new-line-breaks.py -> no lines missing semantic breaks.
  • Banned-punctuation scan over git diff -U0 origin/main...HEAD added lines -> examined 113 added lines, 0 hits.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@d-morrison
d-morrison marked this pull request as ready for review August 1, 2026 19:46
Copilot AI review requested due to automatic review settings August 1, 2026 19:46
@github-actions
github-actions Bot removed the request for review from Copilot August 1, 2026 19:46
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@d-morrison
d-morrison force-pushed the ums/merge-created-duplication branch from e9de329 to 3743b4b Compare August 1, 2026 19:49
Copilot AI review requested due to automatic review settings August 1, 2026 19:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Records reusable lessons from PR #969’s review lifecycle.

Changes:

  • Documents merge-created prose duplication and normalized overlap checks.
  • Adds citation-provenance verification guidance.
  • Records merged-PR query ordering pitfalls.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
shared/workflow/sync-with-main.md Covers duplication introduced by clean merges.
shared/workflow/challenge-redundant-content.md Adds normalized overlap-check guidance.
shared/workflow/address-every-comment.md Requires verification of replacement citations.
memories/github.md Documents merged-PR listing pitfalls.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread shared/workflow/challenge-redundant-content.md Outdated
Comment thread memories/github.md Outdated
Copilot AI review requested due to automatic review settings August 1, 2026 19:50
@github-actions
github-actions Bot removed the request for review from Copilot August 1, 2026 19:50
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 1, 2026 19:55
Comment thread memories/github.md Outdated
@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Code review

Reviewed at head 3743b4b1000095c527dd84fd6752562ad5583127. This PR adds four documentation-only entries to memories/github.md, shared/workflow/address-every-comment.md, shared/workflow/challenge-redundant-content.md, and shared/workflow/sync-with-main.md, capturing UMS lessons from PR #969's review lifecycle (issue #1020).

Verification performed:

One finding posted inline (memories/github.md:95-97): the recommended replacement command gh search prs --merged=">=<date>" doesn't work — --merged is a boolean flag ("Filter based on merged state"), not a date filter. The correct flag is --merged-at date. Confirmed by running both forms against the live repo. This is the one place in the diff where a claim wasn't independently verifiable by inspection alone and turned out to be wrong — everything else checked out.

Verdict

Needs minor changes — one real bug (a non-functional CLI flag in the new memory entry's own recommended fix) to correct; everything else in the diff is accurate, well-cited, and compliant with the repo's own conventions.

@claude

claude Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Code review

Reviewed at head 1909bd5f3aa985bc69b14b8c7bcd6d122508ec4e (previous review was at 3743b4b1). Since that review, commit e7ba1517 addressed both outstanding findings, plus a main merge with no conflicts.

Prior finding resolved: the previous review and a Copilot review both flagged that memories/github.md's recommended replacement command used --merged (a boolean flag) instead of --merged-at (the date-range flag). This is now fixed at both occurrences (lines 95 and 97), and I re-verified the corrected form actually works:

$ gh search prs --repo Morrison-Lab/ai-config --merged-at ">=2026-08-01" --limit 5 --json number,state
[{"number":1019,"state":"merged"}, ...]

Copilot's second finding also resolved: shared/workflow/challenge-redundant-content.md's new section originally implied scripts/find-near-duplicates.py's default run could certify a narrow single-file audit clean. The fixed text now correctly frames the script's default corpus (skills/*/SKILL.md) and default threshold/top-N (0.05 / 25) as tuned for broad candidate-listing, not as a clean-result certificate for a narrow finding -- verified against the script's own DEFAULT_CORPUS, DEFAULT_THRESHOLD, and --top values.

Re-verification performed:

  • Confirmed both flag-syntax fixes work against the live repo (above).
  • Confirmed scripts/find-near-duplicates.py's actual defaults match what the corrected prose now claims about it.
  • Re-ran python3 scripts/validate-skills.py and python3 scripts/check-memory-file-size.py at the current head -- both pass.
  • Re-ran python3 scripts/find-near-duplicates.py --corpus "shared/workflow/*.md" -- 0 pairs above threshold across 34 units, no new duplication introduced.
  • Scanned all added lines (full PR diff) for banned punctuation (em/en-dash, curly quotes, multiplication sign) -- none found.
  • Re-checked the address-every-comment.md and shared/workflow/sync-with-main.md additions, which the fix commit did not touch -- unchanged from the previously-verified content.
  • Confirmed issue Capture PR #969 ARDI lessons in ai-config #1020 (which this PR closes) matches the PR's stated scope.

No new findings.

Verdict: Ready for merge.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


Posted successfully.

Summary

I reviewed PR #1021 at head 1909bd5f3aa985bc69b14b8c7bcd6d122508ec4e, which is one commit ahead of the previously-reviewed head (3743b4b1). The intervening commit (e7ba1517) fixed both outstanding findings:

  1. My prior blocking findingmemories/github.md's recommended gh search prs --merged=">=<date>" command (a non-functional boolean flag) — is now corrected to --merged-at ">=<date>" at both occurrences. I re-ran the corrected command against the live repo and confirmed it works.
  2. Copilot's finding on shared/workflow/challenge-redundant-content.md (about find-near-duplicates.py's default run being oversold as a clean-result certificate) is also fixed, and I verified the corrected prose accurately reflects the script's actual defaults (DEFAULT_CORPUS, DEFAULT_THRESHOLD=0.05, --top=25).

I re-ran validate-skills.py, check-memory-file-size.py, and find-near-duplicates.py against the current head (all pass, no new duplication), scanned the full diff for banned punctuation (none found), and confirmed the untouched files (address-every-comment.md, sync-with-main.md) were already verified accurate in the prior round.

Verdict

Ready for merge.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

💰 Cost: $6.8083 (review) — run

@d-morrison

Copy link
Copy Markdown
Collaborator Author

Ready for merge.

Validation at current head 1909bd5f:

  • python3 scripts/validate-skills.py passed; existing sembr-skills uninitialized submodule warning only.
  • python3 scripts/check-memory-file-size.py passed.
  • NLB_BASE_REF=origin/main python3 /Users/ezramorrison/Documents/GitHub/gha/check-new-line-breaks/check-new-line-breaks.py passed.
  • Banned-punctuation scan examined 120 added lines and found 0 hits.
  • Targeted markdownlint passed.
  • GitHub checks are green, latest claude review at 1909bd5f says Ready for merge, and all review threads are resolved.

@d-morrison
d-morrison merged commit 18131bb into main Aug 1, 2026
8 checks passed
@d-morrison
d-morrison deleted the ums/merge-created-duplication branch August 1, 2026 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Capture PR #969 ARDI lessons in ai-config

2 participants