Repository navigation
update-shape.py: --accept-local re-pins a both row from the merged file, as AGENTS.md promised, and --branch may name the branch you are on (#84) - #85
Merged
Conversation
…file, as AGENTS.md promised, and --branch may name the branch you are on (#84) `apply` refused every `both` row before it ever consulted `--accept-local`, so the exit AGENTS.md already documented — merge by hand, commit, re-run with `--accept-local <path>` on the file you merged — could never actually be taken. Found running S4 of #77 on InkRouter/IRRS and IRSS (their #13s): both adopted an in-place Makefile that appends a CONTRACTS_DIR block and keeps its pin row, so `classify()`'s "not a verbatim copy at the pinned commit" branch reads it as `both` the moment upstream touches the template, forever, regardless of any further local edit. `cmd_apply`'s conflict refusal now excludes a `both` row whose path `--accept-local` names: upstream bytes are never copied over it, and `classify()` precomputes its digest from the working file the same way it already does for `locally-modified`, so the row is recomputed from whatever is on disk — the person's word, not a check that a merge actually happened. An unaccepted `both` row still refuses with the existing wording; `check`'s per-row detail for `both` gains one sentence naming `--accept-local <path>` as the exit (kept out of `row.detail` itself, so the refusal's own wording does not change). `commit_on_branch` no longer assumes `--branch <name>` names a branch that does not exist yet: `<name>` naming the root's tracking branch is refused outright, current or not; naming the CURRENT branch (so, by that rule, not the tracking branch) commits onto it directly — exactly the checkout the documented `both` exit leaves behind once the hand merge is committed; an existing branch that is not current is refused by name (`update-branch-exists`); anything else is created fresh, as before. Tests in test_update_shape.py: the existing CHANGED-file `both` case now also asserts `--accept-local` succeeds and re-pins from the local, unmerged bytes (`test_accept_local_on_a_both_row_repins_from_the_local_bytes`). A new `adopted_root` fixture mirrors an in-place adoption's Makefile append (`ADOPT_MAKEFILE_BLOCK`, hand-patching the pin row's digest the way `_materialize` itself computes it) in its own upstream clone, so its second commit moving the Makefile TEMPLATE (and, alongside it, scripts/validate-manifest.py) cannot move any other test's default target. Four tests cover `check` naming the exit, the unaccepted refusal, the hand-merge-then-`--branch`-onto-that-branch exit end to end (re-pin, the other changed file copied, green validate-pins.py/validate-manifest.py), and an unmerged `--accept-local` still succeeding. Three more cover `--branch`'s postures: current non-tracking branch, existing non-current branch, and the tracking-branch refusal (checked on `main`, which is both this root's tracking AND current branch, so it proves the tracking check runs first). AGENTS.md point 4 now says the refusal has this exception and that the hand-merge commit goes on the branch `apply` is then pointed at with `--branch`; its cap moves 342 -> 344 with a dated entry in test_repo_hygiene.py's own style. 82 passed (test_update_shape.py, test_update_shape_add.py, test_repo_hygiene.py); 738 passed, 1 skipped for the full suite. Re-ran the fix against a scratch clone of InkRouter/IRRS on branch shape/update-3abeb8a4a27e, where the hand-merged Makefile from #13 is already committed: `apply --accept-local Makefile --branch shape/update-3abeb8a4a27e` re-pinned from that merged file, copied AGENTS-shape.md, committed onto the same branch, and `make validate` is green. Lane: xfactory-2 (xFactory-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 21 hours and 37 minutes by commenting @sourcery-ai review. Upgrade to get a review now.
Reviewer's GuideThis PR fixes the documented hand-merge path for Flow diagram for accepting a hand-merged shape rowflowchart TD
A["classify rows"] --> B{"both row named by --accept-local?"}
B -- No --> C["Refuse with update-conflict"]
B -- Yes --> D["Keep working-file bytes"]
D --> E["Recompute row digest"]
E --> F["Copy other accepted updates"]
F --> G["Commit updated pins and manifest"]
Flow diagram for --branch posture selectionflowchart TD
A["Validate --branch"] --> B{"Tracking branch?"}
B -- Yes --> C["Refuse update-branch-tracking"]
B -- No --> D{"Current branch?"}
D -- Yes --> E["Commit on current branch"]
D -- No --> F{"Existing local branch?"}
F -- Yes --> G["Refuse update-branch-exists"]
F -- No --> H["Create branch and commit"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
🟡 Changes recommended
The new tracking-branch helper is used for commits/PRs, but the printed NEXT command guidance still hardcodes --base main, which can mislead users when tracking_branch is not main.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes update-shape.py apply so the documented hand-merge workflow for both (changed on both sides) is actually usable: --accept-local <path> can now exempt an otherwise-conflicting both row from the conflict refusal and re-pin it from the working-tree bytes, and --branch <name> can target the currently checked-out non-tracking branch rather than always trying to create a new branch.
Changes:
- Honor
--accept-localforbothrows by precomputing local digests during classification and excluding acceptedbothpaths from the conflict refusal, then re-pinning from on-disk bytes. - Add
--branch“postures” to safely commit onto the current non-tracking branch, refuse the tracking branch, and refuse existing non-current branches. - Expand/update tests to cover adopted-root Makefile conflicts (
bothforever), accept-local re-pinning behavior, and the--branchpostures; update AGENTS.md wording and the hygiene line cap accordingly.
File summaries
| File | Description |
|---|---|
| update-shape.py | Implements both + --accept-local workflow, adds branch-selection logic, and improves check output for both. |
| tests/test_update_shape.py | Adds coverage for the fixed both/adopted-Makefile workflow and --branch behavior. |
| tests/test_repo_hygiene.py | Adjusts the AGENTS.md line-count cap for the small doc expansion. |
| AGENTS.md | Updates the documented workflow to match the corrected apply behavior and branch posture. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1026
to
+1034
| def tracking_branch_of(manifest: dict | None) -> str: | ||
| """This root's tracking branch, out of an already-loaded manifest — the | ||
| same field `open_pull_request` lands its pull request on. `--branch` may | ||
| never commit onto it directly: these organisations are pull-request only, | ||
| and a commit whose only next step is a push to the default branch is not | ||
| one this tool will make. | ||
| """ | ||
| return str((manifest or {}).get("tracking_branch") or "main") | ||
|
|
… one (SonarCloud duplication gate on #85) (#84) PR #85's quality gate failed on one metric: 4.4% duplication on new code (required <= 3%), no issues raised. `adopted_upstream_and_project` had retyped `upstream_and_project`'s clone-then-scaffold-then-recursive-clone block verbatim, and three tests each closed with the same "exit 2, refusal id in stderr, pin's commit: untouched" tail. `scaffold_upstream_and_project(tmp_path_factory, subdir)` is the part both fixtures share: a fresh clone of this repository, `PROJECT` scaffolded from it, and a recursive clone of that. Returns the pieces rather than a finished fixture dict on purpose — one shared return shape across two fixtures is how the second one drifts from the first — so each fixture still makes its own commit(s) on top and returns its own dict. `commit_upstream_changes(upstream, message, {source: text})` folds the append-write-then-commit-then-capture-the-hash steps `upstream_and_project`'s commit B and `adopted_upstream_and_project`'s commit B' both took; the appended comment the two share for `CHANGED_SOURCE` is now the one constant `UPSTREAM_FIX_LINE` rather than two copies of the same string. `assert_refused_and_unpinned(result, root, upstream_and_project, refusal_id, *needles)` is the repeated tail, now used by both `--branch` refusal tests, the `both`-row conflict test, and the adopted-Makefile conflict test. Behaviour and assertions are unchanged — same fixtures returned, same refusals, same messages checked — and every docstring survives. Two clones stay two clones: `adopted_upstream_and_project` still gets its OWN clone rather than a further commit on `upstream_and_project`'s, because that one is shared by every bare `check()` call in this module and a further commit on it would move their implicit target commit out from under them. 82 passed (test_update_shape.py, test_update_shape_add.py, test_repo_hygiene.py); 738 passed, 1 skipped for the full suite. Lane: xfactory-2 (xFactory-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f8d57331a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1157
to
+1159
| conflicts = [row for row in rows | ||
| if row.is_conflict | ||
| and not (row.state == BOTH and row.path in accepted)] |
There was a problem hiding this comment.
When a both row is accepted here, the documented workflow retains a manually merged file, but commit_on_branch and open_pull_request still state that the copies were “not hand-edited” and that “No copy was hand-edited.” Thus the exact merge/commit/apply flow produces misleading commit and PR provenance that hides the accepted local merge from reviewers; pass the accepted paths into those messages and disclose them, or omit the no-hand-edits assertion.
AGENTS.md reference: AGENTS.md:L199-L202
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟢 Approval recommended
The changes align the tool with documented workflow, are narrowly scoped, and are backed by targeted new tests covering the reported failure modes.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
brettheap
added a commit
that referenced
this pull request
Sep 10, 2026
…S.md and the hygiene caps A merge rather than a rebase because the organisation's ruleset refuses a force-push to any branch. AGENTS.md merged cleanly; the one conflict was tests/test_repo_hygiene.py's AGENTS.md size cap, which #85 and this branch both raised with commentary — README.md's cap did not conflict, since #85 never touched README.md. Both dated entries are kept, #85's first, and the cap is the merged file's exact count (370). The squash on merge folds this commit away. Lane: xfactory-2 (xFactory-2) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Sep 12, 2026
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.
Lane: xfactory-2 (xFactory-2)
Claim: #84 (CLAIMED comment, 2026-09-10; found running S4 of #77 on InkRouter/IRRS#13 and IRSS#13 on Brett Heap's "apply the shape update to the holder and both roots")
Closes #84.
update-shape.py applyrefused anybothrow before it consulted--accept-local, so the exit AGENTS.md documents forboth— merge by hand, commit, re-run with--accept-local <path>— could not be taken, and the refusal's own remediation was the command just refused. For an adopted root the Makefile is never verbatim (adoption appends to it), so this fired the first time upstream touched the template (#80). Secondary:--branch <name>always created the branch, so it could not be pointed at the branch the hand merge sits on.The fix.
classify()precomputes the local digest for abothrow;cmd_applyexcludes abothrow named by--accept-localfrom the conflict refusal, copies nothing over it, and recomputes its pin row from the working file — exactly what the flag already meant forlocally-modified. An unacceptedbothrow still refuses with the same wording;check'sbothdetail gains one sentence naming the exit.--branch <name>now has three postures: the tracking branch → refuse (update-branch-tracking); the current branch → commit onto it; an existing non-current branch → refuse by name (update-branch-exists); otherwise create it as before. AGENTS.md point 4 says the hand-merge commit goes on the branchapplyis then pointed at.Tests (nine new/changed in
tests/test_update_shape.py): a fixture that mirrors an in-place adoption (the adoption's appended Makefile block, pin digest patched);checkreportsbothand names--accept-local;applywithout the flag still refusesupdate-conflict;apply --accept-local Makefile --branch <the merge's branch>re-pins from the merged bytes, copies the other changed file, moves the pin and the manifest'sshape:block, and the root's ownvalidate-pins.pyexits 0;--accept-localon unmerged bytes is still accepted (the flag is the person's word, and the row then records those bytes); the three--branchpostures. Full suite: 738 passed, 1 skipped.Proven on the real case, offline: a clone of InkRouter/IRRS at its
shape/update-3abeb8a4a27e(the hand-merged Makefile commit), submodules pointed at the sibling checkouts,apply --at 3abeb8a --yes --accept-local Makefile --branch shape/update-3abeb8a4a27e→kept Makefile (local bytes; row recomputed from them because --accept-local named it),copied AGENTS-shape.md, re-pinned 6fe09a4 → 3abeb8a, validators green, one commit on the existing branch,make validateexit 0. The two real roots rerun this the moment it lands.🤖 Generated with Claude Code
Summary by Sourcery
Enable safe completion of hand-merged shape updates while preserving local files and enforcing explicit branch selection.
New Features:
apply --accept-local <path>to acceptbothrows and recompute their pins from the working-tree bytes.--branchto commit on the current non-tracking branch while safely refusing tracking or other existing branches.Bug Fixes:
Enhancements:
checkoutput and AGENTS.md guidance to describe the acceptedbothworkflow and branch requirement.Documentation:
apply.Tests:
--branchpostures.