[brettheap]

Lane: openreposhape-4 (openRepoShape-4)
Claim: #604 (comment)

What changed and why

#562 (#536) asked one direction only: a copy this run WRITES that imports from a module --accept-local KEEPS. Nothing asked the reverse. A project whose committed edit had scripts/bootstrap.py import a name from scripts/repo_shape.py (row locally-modified) ran apply --at <a commit that dropped the name> --accept-local scripts/bootstrap.py. It got exit 0, pins ok and manifest ok, and its next bootstrap.py --help died with ImportError: cannot import name .... The validators caught it only where the kept copy is itself a validator, and then only by rolling back after the writes. Upstream does drop public names: cac6bb2 took link_manifest_path and declared_neutral_pins out of repo_shape.py.

_refuse_kept_copies_missing_names keeps its one call site in cmd_apply, before confirm. It now asks both directions and raises one refusal.

• A written importer and a kept module (update-shape.py apply --accept-local scripts/repo_shape.py can keep an old helper that a newly copied file imports from, and make bootstrap then fails with ImportError #536, _kept_copies_lacking): what it asks is unchanged.
• A kept importer and a written module (update-shape.py apply --accept-local: a kept .py copy that imports a name the run's new repo_shape.py dropped passes, and make bootstrap then dies on an ImportError #604, _kept_copies_importing_dropped):
  • Each kept .py copy is read from its bytes on disk with _names_imported. A kept copy is a both or locally-modified row that --accept-local named.
  • Each module it imports is resolved with _module_beside.
  • Where that path is a copy this run writes (an upstream-changed row's target bytes, or an --add file's), a name is refused only when the module's bytes on disk define it now and the bytes the run writes do not (_names_dropped).
• Each pair is one Disagreement NamedTuple (module, importer, names, side), so the refusal and its remediation read it by field name.
• The bytes-level reader: _names_defined_in(data) is split out of _names_defined(path), so the written module is read from its target bytes under the same rules. _names_defined(path) answers exactly as before: None where the read raises OSError, the ValueError of a path holding a NUL, or MemoryError.

Only what this run takes away is refused.

• A name the bytes on disk lack too already stops the kept copy today. That is the project's doing, not this run's.
• A module with no file yet (an --add file) defines nothing to take away.

One refusal, both directions.

• The code stays update-accept-local-missing-name. AGENTS.md lists no update-shape refusal code, so it needs no line.

• The lines: update-shape.py apply --accept-local scripts/repo_shape.py can keep an old helper that a newly copied file imports from, and make bootstrap then fails with ImportError #536's lines are unchanged. Every pair gets one line, sorted by module and then importer. A reverse line reads:
  scripts/repo_shape.py (written) no longer defines shell_line, which scripts/bootstrap.py (kept) imports from it

• The opening sentence is true of every line in either direction: "--accept-local keeps this project's own bytes of a copy that disagrees with a copy this run writes beside it: one of the two imports a name from the other that the other's bytes do not define, so the importer would stop on an ImportError the first time it runs".

• The validators clause ("though the validators, which never import it, pass") is said only when no importer named, written or kept, is one of kind.validators.

• The remediation gives the human's two exits, and each exit names the lines it settles:

  • keep the edit and change the kept copy:

    • merge the upstream's version by hand into a kept copy that lacks a name, "so it defines every name its (kept) lacks line names";
    • change a kept copy that imports a name the module written beside it no longer defines, "so it no longer takes any name its (written) no longer defines line names".

    Then "commit each", and run again with the same --accept-local. A refusal that holds one kind of line gives that kind's exit alone.

  • or drop the edit, so the upstream's copy is taken. For a both row, that is update-conflict's merge again, settled on the upstream's side.

  It names git show <target>:<source> of every kept copy listed. For a kept importer, that is the upstream's own copy, which shows what it imports instead. It names --accept-local in prose and prints no NEXT line.

• Exit 2. Nothing is written, and stdout has no copied line.

Docs:

• No --help text changed, so docs/cli.md is unchanged.
• No README.md or AGENTS.md statement became false:
  • README (:1056-1066) and the handbook's "What it refuses" list some of apply's refusals and the --accept-local exits. None of them names update-shape.py apply --accept-local scripts/repo_shape.py can keep an old helper that a newly copied file imports from, and make bootstrap then fails with ImportError #536's code, and none promises that a re-run succeeds.
  • AGENTS.md rules 3, 4 and 6 still hold.

Tests

tests/test_update_shape_kept_helper.py, appended on #562's harness: 76 tests before, 108 now. The module docstring says it covers both directions. The reverse direction has a second Atlas and InkRouter, cut at kept-base. That is a branch off A whose repo_shape.py gains two names no shipped copy imports. Off kept-base:

• dropped takes one of the names away again;
• both-ways does the same, and also has the holder's siblings.py import a name its bootstrap.py gains.

End to end:

• update-shape.py apply --accept-local: a kept .py copy that imports a name the run's new repo_shape.py dropped passes, and make bootstrap then dies on an ImportError #604's case: check reads bootstrap.py as locally-modified and repo_shape.py as upstream-changed. Then apply --at dropped --yes --accept-local scripts/bootstrap.py gives:

  • exit 2 under the code;
  • the line naming scripts/repo_shape.py (written), scripts/bootstrap.py (kept) and the name;
  • every file, ref and HEAD unchanged, and git status --porcelain empty;
  • no copied line and no NEXT line;
  • only the reverse exit's words, and no update-conflict (the row is not both).

  The premise is measured too: the kept bootstrap.py beside the target's repo_shape.py stops on that ImportError at --help.

• With --branch: refused before any branch exists.

• A kept validator (scripts/validate-pins.py) importing the dropped name: refused, without the validators clause.

• Controls:

  • a name the module lacked before too: applies, exit 0;
  • a name the written module still defines: applies, exit 0.

  In each, the kept copy starts after the run exactly where it started before.

• A family holder keeping a bootstrap.py that imports the dropped name: refused.

• Both directions in one run (the holder at both-ways, its bootstrap.py both and accepted): one refusal, with both kinds of line, sorted, each exit's words, and "commit each".

In process, over pairs of bytes:

• the reverse line, its remediation, and update-conflict named only for a both row;
• none refuses for each case ruling 3 lists:
  • a kept importer that does not parse;
  • a star import, a plain import M, or the importer's own fallback;
  • written bytes that do not parse (a null byte included), or that hold a star import or a module __getattr__;
  • bytes on disk that do not parse (non-UTF-8 included), or that hold a star import or a module __getattr__;
• none refuses for the two "not taken away" cases, or for an --add module with no file on disk;
• the scope: an importer in another directory, a module the run leaves alone, a module no written copy is, and a kept copy that is not .py. The last sits under scripts/, where the .py filter alone decides it, and so does update-shape.py: apply refuses to keep an --accept-local copy of a helper that lacks a name a copy it writes imports from it, before anything is written, so a kept scripts/repo_shape.py no longer leaves make bootstrap dying on an ImportError after the validators passed (#536) #562's forward twin;
• both directions in one refusal, sorted, each exit's words and "commit each", with and without a kept validator among the importers;
• a one-direction refusal of each kind gives only its own exit;
• _names_defined answers None for a missing file, a path holding a NUL, and a MemoryError on the read.

Run at ece91f6 (Linux, py-bench), as the brief listed, with -rs: tests/test_update_shape_kept_helper.py, the eight other test_update_shape*.py files named, test_family_printed_quoting.py, test_repo_hygiene.py, test_cli_reference.py and test_agents_md_codes.py. Result: 623 passed, 0 failed, 0 skipped, in 373 s. At 90aba6a the same set gave 620 passed.

Mutants at ece91f6, each run against the whole new file:

mutant	result
the reverse check removed	killed, 9 failed: the issue's case, --branch, the kept validator (update-validators-red and a rollback after the writes), the holder, both-ways, and four in process
the "defined on disk now" condition dropped	killed, 7 failed: the lacked-before control end to end (refused with "no longer defines kept_importer_never_defined", a name it never defined), and six in process, the --add module and the five bytes-on-disk cases among them
the written module read from disk, not from its target bytes	killed, 9 failed, #604's end-to-end case (test_keeping_a_copy_that_imports_a_name_the_run_drops_is_refused) among them
no .py filter on a kept copy (R604's r6)	killed, 1 failed: the kept scripts/notes.txt case
no .py filter on a written copy (r6's forward twin)	killed, 1 failed: the written scripts/notes.txt case
the remediation's old words ("every name above", "that name", "commit it")	killed, 6 failed
except OSError alone in _names_defined	killed, 1 failed: the path holding a NUL

With the reverse check removed, #604's case applies with exit 0, pins ok and manifest ok: Atlas (atlas), 3 legs. check then says unchanged 11 and nothing to do. Then scripts/bootstrap.py --help in that tree exits 1:

Traceback (most recent call last):
  File "<tmp>/Atlas/scripts/bootstrap.py", line 71, in <module>
    from repo_shape import kept_importer_dropped_upstream  # noqa: E402
ImportError: cannot import name 'kept_importer_dropped_upstream' from 'repo_shape' (<tmp>/Atlas/scripts/repo_shape.py)

What it cannot tell

It does not refuse, in either direction, in these cases:

• a kept importer whose bytes do not parse;
• a star import of the module, or import M and an attribute;
• a module that does not parse, or that holds a star import or a module __getattr__. That covers its bytes on disk, the bytes the run writes, and update-shape.py apply --accept-local scripts/repo_shape.py can keep an old helper that a newly copied file imports from, and make bootstrap then fails with ImportError #536's kept bytes.

There the validators answer after the writes, as before. They roll the run back only where a validator is the copy that cannot import.

Known limits:

• Names bound in unusual ways. A name bound only through globals(), setattr on the module or exec reads as missing. The reverse refuses it only where the written bytes bind it that way and the bytes on disk bound it plainly. A name bound and then deleted reads as defined.
• The validators clause looks only at whether an importer named IS a validator. The same is true of update-shape.py apply --accept-local scripts/repo_shape.py can keep an old helper that a newly copied file imports from, and make bootstrap then fails with ImportError #536's. A kept copy that is not a validator, but that a validator imports, would make the clause false. Today no shipped repo_shape.py imports a sibling copy, so no shipped file is in that position.

Review

R604 reviewed 90aba6a and approved with nits. Fix round 1 (ece91f6) made the four changes the coordinator ruled:

• F1: each exit names the lines it settles, and the remediation says "commit each".
• F2: the two non-.py cases moved under scripts/, and R604's surviving mutant r6 is now killed.
• F3: _names_defined answers a path holding a NUL as base did.
• F4: one Disagreement NamedTuple for the pair.

R604's three out-of-scope findings go to #458 by the coordinator:

• the printed git show commands are not shell-quoted;
• the validators clause;
• update-shape.py apply --accept-local scripts/repo_shape.py can keep an old helper that a newly copied file imports from, and make bootstrap then fails with ImportError #536's readers in the reverse direction.

Shared files

update-shape.py is also edited by lane 4's #588 (fix/posix-paste-lines), in accept_local_exit, keep_local_exits, cmd_check, commit_on_branch, _refuse_local_drift and _note_untaken_additions. This PR touches none of them. Its first changed line is about 20 lines below _note_untaken_additions. Its one other change outside its own functions is an import line, from typing import NamedTuple.

git merge-tree --write-tree, with the heads read by git ls-remote after the fix round:

other	head	result
main	1d809eaf	tree f92154208a9a2828e40673fb86698e889d9638a7, exit 0, no conflict. Main has not touched either file since this PR's base.
#588	8f5ce8a4	tree e5ffa0f8b854c275da564d44cc3d1db509d457db, exit 0, no conflict. The merged update-shape.py parses and differs from this head by exactly #588's 28 insertions and 12 deletions.

#588's earlier head, c2e19022, conflicted in docs/handbook.html against main whether or not this PR was in the merge. Its merge of main (8f5ce8a4) has since cleared that.

Closes #604

🤖 Generated with Claude Code

Summary by Sourcery

Prevent apply --accept-local from keeping Python importers that would reference names removed by modules in the same update.

New Features:

• Extend apply --accept-local validation to detect kept importers that depend on names removed from modules written by the update.

Bug Fixes:

• Prevent updates from completing successfully while leaving kept Python copies that fail with an ImportError after upstream names are removed.
• Ensure these incompatibilities are rejected before confirmation or any files, refs, or branches are changed.

Enhancements:

• Unify forward and reverse missing-name conflicts into one refusal with direction-specific diagnostics and remediation guidance.
• Read module definitions from both on-disk and pending target bytes while limiting checks to names actually removed by the current update.

Tests:

• Expand update-shape coverage for reverse-direction conflicts, combined conflicts, validators, family holders, no-op controls, unsupported syntax, unreadable files, and write-safety guarantees.

[sourcery-ai]

Reviewer's Guide

This PR hardens apply --accept-local by refusing, before any writes or branch creation, to keep a Python importer whose adjacent module bytes will lose a name during the update, while retaining the existing opposite-direction check. It reads kept files from disk and written modules from target bytes, reports both mismatch types through the existing refusal code with tailored remediation, and adds extensive end-to-end and in-process coverage for valid controls, unsupported static-analysis cases, validators, family holders, and atomicity.

Sequence diagram for reverse accept-local name refusal

sequenceDiagram
    participant Apply as apply
    participant Kept as Kept importer on disk
    participant Written as Written module target bytes
    participant Check as Name validation

    Apply->>Check: _refuse_kept_copies_missing_names()
    Check->>Kept: _imports_on_disk(root, path)
    Kept-->>Check: Imported names
    Check->>Written: _names_dropped(root, path, data)
    Written-->>Check: Names defined on disk but absent from target bytes
    Check->>Check: Compare imported names with dropped names
    alt Name was dropped
        Check-->>Apply: _missing_name_refusal()
        Apply-->>Apply: Refuse before confirm and writes
    else No dropped imported name
        Check-->>Apply: Continue
    end

Loading

Flow diagram for pre-write accept-local name validation

flowchart TD
    A["apply --accept-local"] --> B["Collect kept copies and written target bytes"]
    B --> C["Check written importers against kept module names"]
    C --> D["Check kept importers against names dropped by written modules"]
    D --> E{"Missing imported name?"}
    E -->|No| F["Continue to confirmation and writes"]
    E -->|Yes| G["Raise update-accept-local-missing-name"]
    G --> H["No branch creation or file writes"]

Loading

File-Level Changes

Change	Details	Files
Extend the pre-write --accept-local safety refusal to detect import/name mismatches in both directions.	Track written target bytes for upstream-changed and added files. Preserve the existing written-importer/kept-module check. Add reverse checking for kept Python importers whose adjacent written modules drop names defined on disk. Skip cases that cannot be statically determined, including unparsable bytes, star imports, module imports/attributes, dynamic __getattr__, and newly added modules. Combine both mismatch directions into one sorted refusal with context-sensitive validator messaging and remediation.	update-shape.py
Expand the kept-copy test harness and end-to-end coverage for reverse mismatches and combined refusals.	Add separate project and family fixtures based on a commit where modules define names later removed upstream. Test refusal before confirmation, writes, branch creation, and validator execution, including the reported bootstrap ImportError case. Verify controls where names were already missing, remain defined, or belong to added/untouched/out-of-scope files. Cover both mismatch directions in one refusal, deterministic ordering, remediation text, validator clauses, and update-conflict behavior. Add in-process tests for parser limitations, dynamic imports/definitions, byte-source handling, and scope rules.	tests/test_update_shape_kept_helper.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#604	Before writing anything, refuse apply --accept-local when a kept .py copy imports a name from a module the run will write, if that module defines the name on disk now but the bytes the run will write do not.	✅

Possibly linked issues

• update-shape.py apply --accept-local: a kept .py copy that imports a name the run's new repo_shape.py dropped passes, and make bootstrap then dies on an ImportError #604: The PR adds the kept-importer versus written-module check requested: it detects names present on disk but dropped in target bytes and refuses before writes.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sourcery-ai]

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 1 day and 3 hours by commenting @sourcery-ai review. Upgrade to get a review now.