Skip to content

adopt-project.py: under a one-byte locale such as Latin-1, a clone check makes again for a plan's clone record that neither spelling names is compared with both, so it is kept and the next run reads it rather than cloning again (#1112) - #1153

Draft
brettheap wants to merge 2 commits into
l3-1078-one-byte-localefrom
l3-1112-at-the-record-both-spellings

Conversation

@brettheap

@brettheap brettheap commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #1106: its base is #1106's branch. Once #1106 lands, lane 3 brings main into this branch and moves its base to main.

What changed

A follow-up outside v1.2.0 (Brett Heap, 2026-10-21T10:32:04Z). It lands only after the v1.2.0 tag, on a later merge word.

The bug (#1112). Take a one-byte locale such as Latin-1, a plan written in UTF-8 whose record is a clone plan made, and a work directory outside ASCII (Thing plus U+00E9).

The fix (adopt-project.py), following the issue's Direction:

  • A new helper, _recorded_paths(recorded). It lists the paths _recorded_path asks, in its order, with ~ expanded: first the spelling _recorded_spelling gives, then the plan's UTF-8 bytes where this locale spells the text otherwise.
  • _recorded_path now asks its paths through _recorded_paths. Its behaviour is unchanged: the spelling as written is asked first, and where neither path is there, the path returned is the spelling as written.
  • A new helper beside them, _at_the_record(target, path, there, recorded), decides whether a clone _recorded_source makes again is at the record. It compares the target's real path with every path _recorded_path asked:
    • where neither spelling names anything, each path in _recorded_paths;
    • where a spelling is there, that one and each asked before it, which names nothing (paths[:paths.index(path) + 1]).
  • _recorded_source calls the helper, and its docstring has a paragraph for adopt-project.py: under a one-byte locale, check compares a clone it made again for a plan's clone record only with the spelling as written, so it removes that clone instead of keeping it and every run clones again #1112.

A decision the Direction leaves open: never a spelling asked after the one there. The Direction says to compare with both spellings. Where a spelling is there, I compare with that one and with any asked before it, but not with one asked after it.

Tests (tests/test_adopt_one_byte_locale.py). The three new tests run check twice in a child under the Latin-1 locale the file already builds. A new gh stand-in clones a repository on disk.

Fix round

RV1153, lane 3's review of 590ab96e, posted on this PR, found no MUST, one SHOULD and two NITs. This round, ff33ba12, takes all three:

Measurements

Each check ran under a Latin-1 locale built with localedef, where a child reports iso8859-1 ISO-8859-1 0. The plan was written in UTF-8 mode for the clone at <run>/Thing<U+00E9>/source-readonly/Thing, recording cloned: true and repository: testorg/Thing. The clone was then moved away. Both runs used check --work-dir <run>/Thing<U+00E9>, with a stub gh that clones a repository on disk.

Code Run 1 Run 2
#1106's head 504a1dd6 exit 0, NOTE "no longer exists ... cloned again", gh calls 1, clone removed at the end exit 0, the same NOTE, gh calls 2, clone removed again
590ab96e exit 0, the NOTE, gh calls 1, clone kept exit 0, plan ok, no NOTE, gh calls still 1, clone kept
This head ff33ba12 exit 0, the NOTE, gh calls 1, clone kept exit 0, plan ok, no NOTE, gh calls still 1, clone kept

The reverse case (N1): the same plan, a file at the UTF-8 bytes where the clone was, and check --work-dir <run>/Thing<0xE9>, the spelling as written, which is free.

Code Run 1 Run 2
590ab96e exit 0, NOTE "no longer whole", gh calls 1, clone removed at the end exit 0, the same NOTE, gh calls 2, clone removed again
This head ff33ba12 exit 0, the NOTE, gh calls 1, clone kept exit 0, plan ok, no NOTE, gh calls still 1, clone kept

With a file at the spelling as written (M2's case), #1106's head, 590ab96e and this head give the same result: each run clones again, gh calls reach 2, and nothing is left.

Out of scope

#1112 has no "Needs a ruling" or "Documented limit" section. Nothing else was changed.

Tests

Each file ran on its own at ff33ba12, under a stub gh that exits 97, GH_TOKEN=bogus, an empty GH_CONFIG_DIR, GIT_SSH_COMMAND=false, GIT_CONFIG_GLOBAL=/dev/null and EMAIL set, with /usr/bin/python3 -m pytest -q -p no:cacheprovider (Python 3.12.3):

Test file Result
tests/test_adopt_one_byte_locale.py (-rs) 13 passed in 17.83s, none skipped
tests/test_adopt_unenterable_source_and_locale.py 13 passed in 10.15s
tests/test_adopt_c_locale_bytes.py 17 passed in 39.87s
tests/test_adopt_source_clone_record.py 100 passed in 38.11s
tests/test_scaffold_leg_path_collisions.py 104 passed in 4.84s
tests/test_repo_hygiene.py 100 passed in 2.82s
tests/test_shape_doctor.py 445 passed, 3 skipped in 767.13s (the counts of the run at 590ab96e)
tests/test_path_guard_hygiene.py, at the merge of this head into origin/main 038dafba 49 passed in 11.75s
tests/test_adopt_one_byte_locale.py (-rs), at that merge 13 passed in 17.43s, none skipped
tests/test_adopt_unenterable_source_and_locale.py, at that merge 13 passed in 9.02s

Mutants

Each mutant was applied to adopt-project.py, and tests/test_adopt_one_byte_locale.py was run. The file was then restored, and cmp found it identical.

Mutant Result Killed by
M1: _at_the_record compares with the path _recorded_path gave alone (asked = [path]), as on #1106's head KILLED, 2 failed test_a_clone_made_again_at_the_record_is_kept_under_a_one_byte_locale and test_a_clone_made_again_at_the_spelling_asked_first_is_kept (each second run said "cloned again")
M2: every path compared even where one is there (asked = paths) KILLED, 1 failed test_a_clone_made_again_beside_the_spelling_as_written_is_not_kept (the second run gave exit 2, REFUSED adopt-work-dir-occupied)
M3: _recorded_path returns the last path, not the spelling as written, where nothing is there KILLED, 1 failed test_the_record_is_asked_as_written_then_as_utf8[neither-latin1]
M4 (N1): where a spelling is there, that one alone (asked = [path] if there else paths), as at 590ab96e KILLED, 1 failed test_a_clone_made_again_at_the_spelling_asked_first_is_kept (the second run said "cloned again")

#1106's M4 (_keep_the_clone_at_the_record) is unchanged by this PR.

Merges

git merge-tree --write-tree at this head, ff33ba12:

Closes #1112

Lane: openreposhape-3 (openRepoShape-3)

🤖 Generated with Claude Code

…heck` makes again for a plan's clone record that neither spelling names is compared with both spellings, so the clone at the plan's UTF-8 bytes is kept and the next run reads it rather than cloning again (#1112)

`_recorded_source` decided whether the clone it makes again is the plan's
record by comparing its target with the spelling as written alone. Under
Latin-1, a plan written in UTF-8 whose clone was in a work directory
outside ASCII was cloned again at the UTF-8 bytes `--work-dir` names, put
on the list of clones to remove, removed when `check` ended, and cloned
again by every run.

The new `_recorded_paths` lists the paths `_recorded_path` asks, in its
order, and `_recorded_path` now asks them through it. Where neither names
anything, the target's real path is compared with each of them. Where one
is there, the comparison is with that one alone, as before: a clone kept
beside a spelling as written that is there would be refused as
`adopt-work-dir-occupied` by the next run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sourcery-ai

sourcery-ai Bot commented Oct 22, 2026

Copy link
Copy Markdown

Reviewer's Guide

Fix repeated cloning under one-byte locales by recognizing either recorded path spelling when neither path exists, while preserving the established precedence and refusal behavior when the spelling-as-written path is occupied; add focused two-run integration tests for both cases.

Sequence diagram for locale-safe clone record detection

sequenceDiagram
    participant Check
    participant Plan_recorded_source
    participant Recorded_paths
    participant Filesystem
    participant Clone

    Check->>Plan_recorded_source: _recorded_source(local_path, work_root)
    Plan_recorded_source->>Recorded_paths: _recorded_paths(local_path)
    Recorded_paths->>Filesystem: Check spelling as written
    Recorded_paths->>Filesystem: Check UTF-8 spelling
    Filesystem-->>Recorded_paths: Neither path exists
    Plan_recorded_source->>Clone: Source.clone(repository, work_root, unwritable)
    Clone-->>Plan_recorded_source: Clone at UTF-8 path
    Plan_recorded_source->>Plan_recorded_source: Compare clone real path with each recorded path
    Plan_recorded_source-->>Check: Keep clone at the record
    Check->>Plan_recorded_source: _recorded_source(local_path, work_root)
    Plan_recorded_source-->>Check: Existing clone is reused
Loading

File-Level Changes

Change Details Files
Add a shared path-resolution helper and use both locale-dependent spellings when determining whether a recreated clone is at the recorded location.
  • Enumerate the spelling-as-written path first, followed by the UTF-8-byte path when distinct, with home-directory expansion.
  • Refactor _recorded_path to preserve its existing precedence and fallback behavior while using the shared enumeration.
  • Update clone-retention logic to compare the recreated target against all candidate paths only when neither spelling currently names an entry; retain the existing single-path behavior when one does.
  • Document the one-byte-locale edge case and the intentional behavior boundary.
adopt-project.py
Add end-to-end Latin-1 regression coverage for retaining recreated clones without changing occupied-record behavior.
  • Create a local gh stand-in that clones a repository and records invocations.
  • Verify a recreated clone at the UTF-8 path is kept and reused on the second check.
  • Verify a file at the spelling-as-written path causes the recreated clone to be removed and cloning to recur.
  • Reuse locale, repository, and plan helpers to exercise real subprocess behavior.
tests/test_adopt_one_byte_locale.py

Possibly linked issues


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

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-22T15:38:24Z) Lane 3's independent review of #1153 (RV1153), head 590ab96. PARTIAL: stopped at a usage checkpoint. No MUST so far.

SHOULD

  • S1. New CRITICAL Sonar issue python:S3776 on _recorded_source (adopt-project.py:6567), cognitive complexity 16 > 15, caused by the conditional at :6681. The gate still passes. Fix: move :6677-6681 into a module-level helper _at_the_record(target, path, there, recorded) that computes realpath(target) and compares it with [path] if there else _recorded_paths(recorded); _recorded_source calls it. SonarQube's snippet analysis reports S3776 at 16 for the method as it is, and 0 issues with the helper.

NIT

Checked

Lane 3's plan: S1 and N1 go into a fix round on #1153 in the next usage window: the _at_the_record helper, with N1's wider comparison folded in and a test for the reverse case if the measured behaviour holds there, and M1 and M2 re-pointed. N2: the Sourcery block comes off and #1154 joins the merge list. RV1153 then finishes test_shape_doctor.py and the merged-tree tests. #1153 lands only after #1106, after the v1.2.0 tag.

Lane: openreposhape-3 (openRepoShape-3)

…plan's UTF-8 bytes hold its clone no longer whole and `--work-dir` names the free spelling as written, the clone `check` makes again there is kept and the next run reads it, and the comparison moves into `_at_the_record` (#1112)

The fix round on #1153 from its review, RV1153:

- S1: the comparison of a clone made again with the record moves out of
  `Plan._recorded_source` into a module-level helper, `_at_the_record`,
  beside `_recorded_paths`, so the method's cognitive complexity falls
  from 16 back to 15 (SonarQube python:S3776).
- N1: the helper compares the clone's target with every path
  `_recorded_path` asked: where a spelling is there, that one and each
  asked before it, which names nothing. Under Latin-1, the UTF-8 bytes
  holding a file where the clone was and `--work-dir` naming the free
  byte 0xE9, the clone made at 0xE9 was removed when `check` ended and
  every run cloned it again; now it is kept and the next run reads it.
  A spelling asked after the one there is still not the record (M2's
  case, a file at 0xE9, is unchanged).
- A test of that reverse case: `check` run twice clones once and keeps
  the clone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-22T19:46:27Z) Lane 3's independent review of #1153, delta round (RV1153), head ff33ba1. Verdict: READY. No new MUST or SHOULD.

Lane 3's plan: #1153 is READY. It lands only after #1106, which lands after the v1.2.0 tag on a later word. Then lane 3 brings main into this branch, moves its base to main, and lands it by the stacked-PR merges.

Lane: openreposhape-3 (openRepoShape-3)

This was referenced Oct 22, 2026
@brettheap brettheap added this to the v1.2.1 milestone Oct 23, 2026
This was referenced Oct 23, 2026

This branch has not been deployed

No deployments
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.

1 participant