fix(advanced-git): correct the dropped-work check from #158, and make the recipes testable - #161
Merged
Merged
Conversation
…erge recipe Review of #158 found the dropped-upstream-work check scanning the wrong range. It listed what upstream changed AFTER the merge; the work a resolution can drop is what upstream changed BEFORE it. Reproduced on a purpose-built repo (upstream commit touching f.txt and g.txt, dropped by a -s ours merge, plus one unrelated later upstream commit): shipped: DIFFERS: h.txt <- unrelated; both dropped files missed corrected: DIFFERS: f.txt, g.txt Whenever upstream has not touched the dropped files again - the common case - the shipped loop printed nothing and read as an all-clear, which is the failure the section exists to prevent. The candidate set now comes from the merge's own parents, reusing the BASE the next snippet already computes. Also in the same section pair: - The reference-merge recipe could not run: git commit after a --no-commit merge exits 128 on unmerged paths. Adds git add -A, and pins REF as a tag so the only reference to it does not live in a prunable /tmp worktree. - The exemption from "Never git checkout <ref> -- <path>" was too broad. It holds only on a path still carrying conflict markers, BEFORE it is edited; a hand-written resolution is destroyed with no reflog entry, which was verified. The rule at the top of the file now points forward to the exemption. - Drops the --autosquash suggestion for empty commits: autosquash only reorders fixup!/squash! commits and cannot fold them. - The split-verification loop word-split paths containing spaces into bogus entries and printed fatal: noise on deletions. Uses -z with read -d ''. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…e two 403s Two review findings on this file: - The CI-disabled section was the only GitLab content in a file that is otherwise entirely gh, and its endpoint list was over-generalized from one observation: pipeline_schedules and variables 403 as well, and merge_requests 403s the same way when its own feature flag is off. Reduced to the platform-neutral lesson plus a pointer to the netresearch-gitlab skill, which now carries the corrected recipe. - Two sections merged minutes apart now explain a 403 during a watch differently - disabled feature here, secondary rate limit under "Watcher cost". Each now names its own signature: API rate limit exceeded on every endpoint versus a bare 403 Forbidden confined to the CI endpoints. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The runner is grep -qiE, case-insensitive and line-based, so several of the patterns added in #158 matched any plausible answer: (reference merge|REF|...) matches "prefer" and "refs/", (403|projects/) matches any GitLab answer, and (stale|no longer|delete the section|remove) matches "remove". rebase_long_branch_with_reference_merge scored 2 of 2 on an answer containing none of the guidance, so it measured nothing. Each of the five entries now asserts its distinctive claim and carries three assertions, matching the file's norm. Re-tested against the same naive answer: all five now score 0. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
force-pushed
the
fix/retro-review-followup
branch
from
August 10, 2026 08:27
669ce7d to
7b50326
Compare
github-actions
Bot
added
documentation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
An independent review found eight blockers in the three commits below it. Each fix here was re-verified by running it, not by reading the diff. advanced-git.md, dropped-upstream-work check: - The check has to run once PER MERGE. merge-base of a later merge's parents resolves to the earlier merge's upstream parent, so a single run only covers the window between them. Verified on a branch with two merges each dropping a different file: the single run reported only b.txt; the enumeration over git rev-list --merges reports both. - The reconstruction used today's tips as ours/theirs, so anything upstream changed AFTER the merge showed up as dropped work - a clean, convincing false positive. Verified: with today's main it reported up1 and up3, with the merge's own $m^1/$m^2 only the genuinely dropped up1. - The cheap-check loop still word-split paths; it now uses -z like its neighbour, and the second block no longer inherits an undefined $f. advanced-git.md, reference-merge recipe: - The block mixed two working directories: git -C .bare only resolves from the project root, where a bare git tag then dies with "not a git repository". Everything after the worktree add is now -C /tmp/ref. - The pin is a branch, not a lightweight tag: those fail under a global tag.gpgsign=true with "fatal: no tag message?", and this environment blocks them outright. Verified that git branch pins it and that git checkout <branch> -- <path> accepts it. - add -u, not add -A, which contradicted the staging rule 20 lines above and swept an untracked file into REF, making the step-3 assertion fail spuriously. Verified both halves. advanced-git.md, split verification: the comment claimed a deleted path "compares equal, which is the right answer". It does not - git rev-parse echoes its argument on failure, and two different echoes never match, so a deleted path was ALWAYS reported NOT CARRIED. Verified, then fixed with --verify -q plus an ABSENT sentinel, which reports nothing when the deletion was carried. merge-gate-watcher.md: the 403 signature is no longer presented as decisive - an insufficient scope, SAML, an IP allowlist and GitHub's secondary limit produce the same shape - and the self-contained flag check is back, so the section no longer depends on an unmerged section of another skill. The cross-platform "tell them apart" claim is gone: the rate-limit case is GitHub, the disabled-CI case is GitLab, and GitLab rate-limits with 429. evals: four assertions could not be satisfied by a correct answer. The canonical "git diff <target>...HEAD" failed its own pattern, and both CI phrases matched zero lines of the skill, so that eval failed in the with-skill arm too and the A/B comparison would have reported no benefit. All five now pass against the skill's own prose (3/3) and no naive answer passes any of them fully. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…osed Three review rounds found the same defect class each time: a recipe reasoned about at the edited line and never run end to end, from the cwd and in the shell the document prescribes. tests/test_advanced_git_recipes.sh now runs all three recipes against fixtures (18 assertions, auto-discovered by the tests workflow), so the next edit is checked by the suite rather than by a reader. Writing it exposed the third round's findings, all now fixed: - Step 4 of the reference-merge recipe could not run. worktree remove refuses a worktree holding the .orig files a resolution leaves behind, and the && chain then skipped the branch deletion; both commands also lacked the -C that the step-1 cwd requires. Now --force, two statements, explicit -C. - add -u silently skips paths a resolution creates (splitting a file, extracting a helper): the commit succeeds and REF is quietly wrong. Staging is by name, as the rule in this file already says, with status --porcelain to show what is left. - The dropped-work check reported a sibling-branch merge as six false positives and missed the real drop when a merge's parents were swapped. It now skips any merge whose ^2 is not an ancestor of the base, flags octopus merges as unchecked, and warns when the range is empty because the branch already landed - which otherwise reads as an all-clear. - Process substitution in the reconstruction was bash-only in a block with no bash note; it writes a temp file instead. - The split loop's branch-name check was prose. One wrong name credits a deleted path as carried and hides a genuine miss, so it is now an executable guard that exits. merge-gate-watcher: the 403 candidates are narrowed to the two that actually produce an endpoint-scoped 403 - the feature being off, and the token lacking that scope. Org-wide rejections and rate limits refuse everything. The claim about another skill's file was dropped rather than left asserting content that is still unmerged. evals: four assertions rejected correct answers - the pattern for the PR-body diff forbade any flag between "git diff" and the range, and the split entry required the reader to name their variable $f. Retested in both directions with answers written not to copy the skill's wording: all five score 3/3 on correct answers and no naive answer passes any of them. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The suite added in the previous commit was vacuous for the fix that commit led with: swapping "add -- <path>" back to "add -u" left it green, because the fixture's resolution never created a path. It also used "cmd && pass", which runs neither pass nor fail when the command fails, so a broken step vanished instead of failing. Both are fixed, and the suite is now mutation-tested: all six probed regressions - add -u, add -A, worktree remove without --force, dropping the swapped-parents branch, dropping the branch-name guard, dropping the || ABSENT sentinel - flip it to FAIL, while the unmutated suite passes. 25 assertions, up from 18, including three that were previously tautological (the empty range asserted a git identity; the tag.gpgsign case never ran a tag; the spaced path was carried so it was never printed) and step 3 of the reference-merge recipe, which the document calls the point of the exercise and the suite did not run at all - now checked in both directions, empty on matching trees and non-empty on divergent ones. Recipe fixes the suite exposed: - The parents-swapped case was skipped rather than examined. The loop now takes whichever parent is an ancestor of the base, and only skips when neither is, which is the sibling-merge case. Verified against a fixture where the merge was made from the upstream side and dropped an upstream change. - The octopus warning sat after the guard that skips, so it could never print. - The split-verification guard used exit inside a snippet meant to be pasted into a shell; the whole check is now a subshell. - The comment credited --verify -q for making deletions work. It does not: git rev-parse echoes its argument on failure, so the || ABSENT sentinel is what makes the two sides compare equal. --verify -q only keeps fatal: off stderr. merge-gate-watcher: the || fallback added last commit could never fire - the pipeline exits with jq's status and jq on empty input exits 0. It captures first now. The rate limit is back in the candidate set, since one budget can drain while the other is healthy, with the body test that separates it. evals: three more assertions rejected correct answers - one required the word "reference" where "the end state" is equally right, one required the reader to name their variable $f, one matched the prompt's own wording. Retested with answers written not to copy the skill: correct 3/3, no naive answer passes. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
changed the title
CI failed the one assertion that pinned an exact exit code: a lightweight tag under tag.gpgsign=true exits 128 here and 1 on the runner. The claim the recipe depends on is that it refuses at all, which is what is asserted now - the same correction already applied to the commit-before-staging case in this file. Still caught by mutation: forcing the tag to succeed flips the assertion to FAIL. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a blocker that reached
mainin #158, and turns the three recipes inadvanced-git.mdinto an executable, mutation-tested suite so this class of defect stops recurring.#158 merged before any review landed — both Copilot responses on it were
unable to review … quota limit. Four independent review rounds have since run against this branch; each found real defects, including in the fixes for the previous round. The commit history is that sequence and is kept unsquashed on purpose.The blocker in
mainadvanced-git.mdtold the reader to derive the dropped-upstream-work candidate set from what upstream changed after the merge. The work a merge resolution can drop is what upstream changed before it. Reproduced on a purpose-built repo — upstream commit touchingf.txtandg.txt, dropped by a-s oursmerge, plus one unrelated later upstream commit:When upstream has not touched the dropped files again — the usual case — the shipped loop prints nothing and reads as an all-clear. Worse than not having the section.
What else was wrong, by round
Round 1 → the fixes in
67bbba9,843dcbe,7b50326. The candidate-set range; a stale PR-body section; a watcher armed for a producer that was switched off; five new evals.Round 2 →
b62a12f. The check has to run once per merge (merge-baseof a later merge's parents is the earlier merge's upstream parent, so a single run misses the first merge's drop). The reconstruction used today's tips, so post-merge upstream work read as dropped work. The reference-merge recipe mixed two working directories and pinned REF with a lightweight tag, which dies under a globaltag.gpgsign=true. The split loop's comment claimed deletions "compare equal";git rev-parseechoes its argument on failure, so a deleted path was always reportedNOT CARRIED. Four eval assertions could not be satisfied by a correct answer — the skill's own canonicalgit diff <target>...HEADfailed its own pattern.Round 3 →
a627326. Step 4 of the recipe could not run (worktree removerefuses the.origfiles a resolution leaves, and the&&chain then skipped the pin deletion).add -usilently skips paths a resolution creates. A sibling-branch merge produced one false positive per file it touched. This is where the recipes becametests/test_advanced_git_recipes.sh.Round 4 →
8d77ea6. That suite was vacuous for theadd -ufix it led with, andcmd && passlet broken steps vanish instead of failing. Three assertions were tautologies. The parents-swapped merge was skipped rather than examined. The octopus warning sat after the guard that skips, so it could never print. The||fallback added tomerge-gate-watcher.mdcould never fire — the pipeline exits withjq's status andjqon empty input exits 0.The suite
25 assertions over the three recipes: bare-repo layout, a real conflict, two merges dropping different files, a sibling merge, a parents-swapped merge, post-merge churn, deleted paths, paths with spaces, an unresolvable branch name. Mutation-tested — each of these flips it to FAIL, and the unmutated suite passes:
add -uinstead of staging by nameREF holds exactly the resolutionadd -Ainstead of staging by namestatus --porcelain shows the leftoverworktree removewithout--forceworktree remove --force (command failed)parents-swapped merge is examinedunresolvable branch name abortsTest plan
tests/test_advanced_git_recipes.sh— 25 assertions, 0 failures, passes twice in a row, hermetic undermktemp -dshellcheckclean (info level, matching the pre-commit hook)Build/Scripts/validate-skill.sh— 0 errors, 0 warningsskill-repo/scripts/validate-evals.sh— 63 passedtests/test_validate_git_command.py,tests/test_pr_status_errored_review.sh— 0 failuresmerge-gate-watcher.mdcapture-then-parse form, which no test coversNot addressed
Section placement in
merge-gate-watcher.md, and the fact that no CI job ever executes eval patterns against a response — the eval file still has the "checked by a reader" property this suite was written to end. Both belong to whoever next touches those files.