tests/test_git_output_decoding.py: the guard reads a text-mode call spelled `from subprocess import run`, `import subprocess as sp`, with only `encoding=` or `errors=`, through `getoutput`, with `**options` or passed uncalled, so none of them walks past #230's check any more (#321) by brettheap · Pull Request #335 · opensoft/openRepoShape

brettheap · 2026-10-12T11:23:19Z

Lane: openreposhape-3 (openRepoShape-3)
Claim: #321 (comment)

What

tests/test_git_output_decoding.py is the only file that changes. #230's TextModeCalls matched one spelling, subprocess.<spawner>(..., text=True); every other way to ask a child for text decodes with the locale's codec exactly as #217 describes, and walked past it. It now reads:

Every way to reach subprocess: from subprocess import run (also as, also *), import subprocess as sp, sp = subprocess and sp: object = subprocess (a plain and an annotated assignment alike), other_module.subprocess.run. A walrus, an unpacking or a for target that holds the module is not followed.
encoding= or errors= alone as a request for text (Popen puts the pipes in text mode on either); encoding=None and errors=None do not ask.
A literal encoding= that is not UTF-8 (None, "cp1252") and a literal errors=None as naming nothing, so text=True, encoding=None, errors="replace" is refused. A variable is taken at its word: a tree cannot say what it holds, and the shipped code has none.
getoutput / getstatusoutput: refused unless EXEMPT lists the function. They are always text and Python 3.9, this standard's floor, takes no codec for them.
**options in a spawner call: refused unless the new OPAQUE table lists the function with a reason (a blank reason is refused). A call that names encoding= and errors= itself needs no row, because Python rejects a keyword given twice.
A spawner passed around uncalled (functools.partial(subprocess.run, ...), runner = subprocess.run, run: Callable = subprocess.run, class Child(subprocess.Popen), map(run, argvs)): refused the same way. A type annotation is not a reference, but the value of an annotated assignment is.

What it did before is unchanged: a plain text=True call with no codec is still refused, and text=False and bytes calls are still let through.

EXEMPT drops two rows, env_commit and cross_leg_references, kept until #196 and #216 landed. env_commit's call names both codec keywords (since #231), and #216 (#228, on main as 346079d) made cross_leg_references' git grep read bytes, so neither exempts anything. The two gh api digest rows stay.

Why

The issue's premise held for the guard and not for the shipped code. Measured with main's own TextModeCalls (c7c02fa) against the head's, over the same synthetic modules, each written as the module the guard must flag:

module (a `read(argv)` function unless noted)	main's guard	head's guard
`from subprocess import run` + `run(argv, text=True)`; the `as`, `import *`, `import subprocess as sp`, `sp = subprocess`, `sp: object = subprocess`, `shape_materialize.subprocess.run` spellings	finds nothing	`asks for text and does not name ['encoding', 'errors']`
`subprocess.run(argv, encoding="utf-8")`	finds nothing	`does not name ['errors']`
`subprocess.run(argv, errors="replace")`	finds nothing	`does not name ['encoding']`
`text=True, encoding=None, errors="replace"`; `encoding="cp1252"`	finds nothing	`does not name ['encoding']`
`encoding="utf-8", errors=None`	finds nothing	`does not name ['errors']`
`subprocess.run(argv, capture_output=True, **options)`; the same into `Popen`	finds nothing	expands `**` into `subprocess.run(...)`, so the guard cannot see whether it asks for text
`subprocess.getoutput("git status")`, `getstatusoutput`, `from subprocess import getoutput`	finds nothing	calls `subprocess.getoutput`, which is always text and takes no `encoding=` before Python 3.11
`functools.partial(subprocess.run, text=True)`, `runner = subprocess.check_output`, `run: Callable = subprocess.run`, `map(run, argvs)`, `class Child(subprocess.Popen)`	finds nothing	refers to `subprocess.run` without calling it

24 modules in all; main's guard finds nothing wrong in any (the guard lives in the test file, so the head's tests cannot be pointed at main's guard directly). Each fixed module, spelled the same way with encoding="utf-8", errors="replace", passes the head's guard.

Over the shipped Python (20 files), at both 71b58a1 and c7c02fa (rows in place): main's guard flags 0, the head's flags 0, and the two found lists (what each treats as a text-mode call, with the keywords it names) are identical. The head sees the same 51 spawner calls (33 text-mode) an independent AST survey counts. At 346079d, with the two rows dropped, it sees 51 spawner calls, 32 text-mode, and flags 0; at c7c02fa the same guard without the cross_leg_references row flagged adopt-project.py:1080 asks for text and does not name ['encoding', 'errors'], so that row was needed until #216. So the guard finds no real offender, no shipped file changes, and OPAQUE starts empty.

Tests

At 90ecef7 (fix round 2: main 346079d merged, two EXEMPT rows dropped; the guard code is round 1's, d578bd8): tests/test_git_output_decoding.py 67 passed in 99.14s (was 3), tests/test_repo_hygiene.py 50 passed in 1.89s, tests/test_cli_reference.py 13 passed in 76.04s (the machine was loaded; d578bd8 ran in 3.46s, 1.84s and 28.44s). tests/test_stdout_encoding.py is not run: nothing here prints.

SYNTHETIC is 24 modules; each gets one test that the guard flags it and says why, and one that the same module fixed passes (48).
4 refused and 8 allowed one-liners pin what the old guard did; their finding lists are identical under main's guard.
4 more: an annotation is not a reference, a function listed in EXEMPT may call getoutput, a function listed in OPAQUE passes (with a reason; a blank one and another function's row do not), every real OPAQUE row gives its reason.
Each detection was removed in a scratch copy (19 mutants: from import, import *, as, sp = subprocess, sp: object = subprocess, an annotated assignment's value read as an annotation, the through-another-module spelling, encoding/errors do not ask, any encoding is UTF-8, None counts as a policy, expansion not refused, every expansion refused, getoutput not recorded, references not recorded, annotations read as references, blank reason accepted, EXEMPT ignored for getoutput, text=False asks, the old call shape) and each failed the test named for it.
Nothing is skipped on any OS: the new tests parse source text and read files as UTF-8, with no process, path or codec of their own. The file parses as Python 3.9 syntax (ast.parse(feature_version=(3, 9)); no 3.9 interpreter here).

Composes with

git merge-tree --write-tree <sibling> HEAD for each wave-A sibling (re-run at 90ecef7), then the head's guard over the merged tree's shipped Python. No open fix/* branch touches tests/test_git_output_decoding.py.

sibling	tip	trial merge	head's guard over the merged tree
fix/adopt-plan-asks-unsafe-paths	49b8716	CLEAN	0 flagged
fix/adopt-refuse-existing-split-branch	a6ec523	CLEAN	0 flagged
fix/pin-rewrite-indentation	ef8c91e	CLEAN	0 flagged
fix/ruleset-hint-tracking-branch	c91d5ab	CLEAN	0 flagged
fix/update-shape-next-quoting	9faf93f	CLEAN	0 flagged
fix/doctor-contracts-manifests	fa4ec4f	CLEAN	0 flagged
fix/adopt-spelled-unambiguous	`51a6e06`	CLEAN	0 flagged
fix/family-add-member-id	71ffe5d	CLEAN	0 flagged
fix/landing-tests-windows	`6c93590`	CLEAN	0 flagged

Only fix/adopt-refuse-existing-split-branch adds a subprocess call so far, and it reads bytes (capture_output=True, no text=), so the guard does not flag it; the rest add none (the tips are local refs; none is pushed yet). A sibling that adds one will meet the guard: a text-mode call with no encoding="utf-8", errors=<policy>, a ** expansion or a getoutput fails its test until it is fixed or listed.

Left out

interpreter_runs in tests/test_validator_child_encoding.py reads only subprocess.run spelled that way, the same blind spot in a second guard.
os.popen is text with no codec to name and the guard does not read it; no shipped code uses it.
shipped_python() globs *.py, scripts/*.py, templates/**/*.py and bootstrap; a Python file added anywhere else is not scanned.
A check that each EXEMPT row still matches a text-mode call would have caught the two rows this change drops.

Closes #321

🤖 Generated with Claude Code

Summary by Sourcery

Strengthen the git output decoding guard so every statically detectable text-mode subprocess call uses an explicit UTF-8 codec and error policy.

Bug Fixes:

Expand the git output decoding guard to detect text-mode subprocess calls across imports, aliases, indirect module references, partial calls, and uncalled spawners.
Require UTF-8 encoding and an explicit error policy for all detectable text-mode subprocess calls, including calls using only one codec option or non-UTF-8 values.
Detect always-text getoutput/getstatusoutput calls and conservatively reject opaque keyword expansions unless documented in OPAQUE.

Enhancements:

Refactor subprocess-call analysis into a reusable module judge with explicit handling for text requests, always-text calls, references, annotations, and opaque calls.

Documentation:

Document the expanded guard coverage and the syntax forms intentionally outside static analysis.

Tests:

Add synthetic regression coverage for previously undetected subprocess spellings, codec combinations, opaque calls, always-text helpers, and passed-around spawners.
Preserve coverage for existing refused and allowed call behavior, exemptions, annotations, and opaque-row validation.

Chores:

Remove obsolete EXEMPT entries whose underlying calls now specify safe decoding.

…pelled `from subprocess import run`, `import subprocess as sp`, with only `encoding=` or `errors=`, through `getoutput`, with `**options` or passed uncalled, so none of them walks past #230's check any more (#321) #230's guard (`TextModeCalls`) matched one spelling, `subprocess.<spawner>(..., text=True)`, and checked that the call names `encoding` and `errors`. Every other way to ask a child for text decodes with the locale's codec exactly as #217 describes, and was invisible to it. Nothing but `tests/test_git_output_decoding.py` changes: the shipped code already passes the hardened guard (see below). - A call is a spawner call however `subprocess` is reached: `from subprocess import run` (also `as`, also `*`), `import subprocess as sp`, `sp = subprocess`, `other_module.subprocess.run`. - `encoding=` or `errors=` alone asks for text, because `Popen` puts the pipes in text mode on either. `encoding=None` and `errors=None` do not ask. - A literal `encoding=` that is not UTF-8 (`None`, `"cp1252"`) and a literal `errors=None` name nothing, so `text=True, encoding=None, errors="replace"` is refused. A variable is taken at its word: a tree cannot say what it holds, and the shipped code has none. - `subprocess.getoutput` and `getstatusoutput` are refused unless EXEMPT lists the function: they are always text, and Python 3.9 (this standard's floor) takes no codec for them. - A spawner call that expands `**options` without asking for text in a way the guard can see is refused unless the new `OPAQUE` table lists its function with a reason (a blank reason is refused too). A call that names `encoding=` and `errors=` itself needs no row: Python rejects a keyword given twice. - A spawner referred to without being called (`functools.partial(subprocess.run, ...)`, `runner = subprocess.run`, `class Child(subprocess.Popen)`, `map(run, argvs)`) is refused the same way. A type annotation is not a reference. Tests (`tests/test_git_output_decoding.py`, 3 -> 59): `SYNTHETIC` holds one small module per form above, 20 of them. Each gets two tests, one that the guard flags the module and names why, one that the same module fixed passes. Eight allowed and four refused one-liners pin what the old guard already did, and four more tests cover the annotation rule, a function listed in EXEMPT, a function listed in OPAQUE (with a reason, with a blank one, and for another function) and the reasons in the real table. Fails on main: `TextModeCalls` as main has it, run over the same 20 modules, finds nothing wrong in any of them (the guard lives in the test file, so the head's tests cannot be pointed at main's guard directly). Each detection was also removed in a scratch copy (17 mutants: from-import, star-import, `as`, `sp = subprocess`, `other_module.subprocess`, encoding/errors do not ask, any encoding is UTF-8, `None` counts as a policy, expansion ignored, every expansion refused, getoutput ignored, references ignored, annotations read as references, blank reason accepted, EXEMPT ignored for getoutput, `text=False` asks, the old call shape restored), and each failed the test named for it. Unchanged: for everything the old guard saw, main's and the head's `found` lists are identical over the shipped Python at 71b58a1 and at c7c02fa: 51 spawner calls, 33 of them text-mode, 0 offenders in either, so no shipped file changes and OPAQUE starts empty. Left out: `interpreter_runs` in `tests/test_validator_child_encoding.py` reads only `subprocess.run` too, and `os.popen` (text, no codec) is not read; none is used in the shipped code. EXEMPT's `env_commit` row exempts nothing now that #196 has landed (#231). Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

sourcery-ai · 2026-10-12T11:23:25Z

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 12 hours and 10 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

sourcery-ai · 2026-10-12T11:23:34Z

Reviewer's Guide

Expands the subprocess decoding guard in the test suite from one direct call spelling to the relevant import, alias, text-mode, always-text, opaque, and uncalled-reference forms, with extensive synthetic regression tests while preserving existing behavior and exemptions.

File-Level Changes

Change	Details	Files
Broaden the AST-based subprocess text-mode guard to recognize aliases, imports, indirect module bindings, always-text helpers, partial/uninvoked references, and opaque keyword expansions.	Track subprocess module aliases and assignments, imported spawners, wildcard imports, and indirect attribute spellings. Treat non-None encoding/errors as text-mode requests and require UTF-8 plus an explicit error policy. Flag getoutput/getstatusoutput unless exempted, and require documented OPAQUE entries for **options or uncalled spawner references. Avoid treating type annotations as executable spawner references and preserve the existing EXEMPT behavior.	`tests/test_git_output_decoding.py`
Add comprehensive synthetic and regression coverage for the expanded guard behavior.	Add bad/fixed paired cases for every previously missed call spelling and detection category. Verify unchanged refused and allowed forms remain stable. Test exemptions, opaque reasons, annotation handling, mutant coverage, and Python/OS-independent source parsing.	`tests/test_git_output_decoding.py`

Assessment against linked issues

Issue	Objective	Addressed
#321	Extend TextModeCalls to detect subprocess text-mode calls made through from-imports, module aliases or assignments, and equivalent subprocess module paths.	✅
#321	Treat encoding= or errors= alone as text mode, detect locale-codec risks such as missing, None, or non-UTF-8 encoding and missing errors policy, while preserving valid UTF-8/error-policy calls.	✅
#321	Detect getoutput/getstatusoutput, opaque **kwargs spawner calls, and uncalled spawner references, requiring explicit exemptions or justified opaque entries so these forms cannot bypass the guard.	✅

Possibly linked issues

tests/test_git_output_decoding.py: TextModeCalls sees only subprocess.<spawner>(..., text=True), so from subprocess import run, import subprocess as sp, a **kwargs call, a call naming only encoding= or errors=, and subprocess.getoutput each decode with the locale codec without failing the guard #217 added #321: The PR implements the issue’s requested TextModeCalls enhancements, including aliases, encoding/errors, getoutput, and **kwargs.

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.

brettheap · 2026-10-12T11:25:38Z

(lane openRepoShape-3, 2026-10-12T11:25:35Z) @codex review

chatgpt-codex-connector · 2026-10-12T11:25:52Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-12T11:55:16.122622Z	`90ecef7`	Manual request

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 960cd17807

ℹ️ 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".

chatgpt-codex-connector · 2026-10-12T11:28:38Z

+                if (isinstance(node, ast.Assign)
+                        and isinstance(node.value, ast.Name)
+                        and node.value.id in self.modules):


Track subprocess aliases created by annotated assignments

Extend alias binding to ast.AnnAssign as well as ast.Assign. A shipped module can validly write sp: object = subprocess and then call sp.run(..., text=True) without encoding or errors; judge_module currently reports no offender for that code, so this straightforward typed form bypasses the guard even though it is semantically identical to the sp = subprocess form this change intends to cover.

Useful? React with 👍 / 👎.

…lias made by an annotated assignment, as it follows `sp = subprocess` Codex's P2 on 960cd17: alias binding read `ast.Assign` only, so a shipped module that wrote `sp: object = subprocess` and then `sp.run(argv, text=True)` passed with no codec named. Measured on 960cd17, that module and `again: object = sp` (after `sp = subprocess`) both found nothing. - `TextModeCalls.bind` binds a name from an `ast.AnnAssign` that has a value exactly as from an `ast.Assign`: `sp: object = subprocess` makes `sp` the module, and `again: "ModuleType" = sp` makes `again` the module. - An annotated binding of a spawner (`run: Callable = subprocess.run`, `read: Callable = check_output`) was already refused by the uncalled reference rule, because only the annotation is skipped, not the value. That is now pinned by a case each. - The docstrings say what stays unfollowed: a walrus, an unpacking, or a `for` target that holds the module. Tests: `SYNTHETIC` 20 -> 24 modules (`module-assigned-with-an-annotation`, `module-annotated-after-a-plain-alias`, `spawner-assigned-with-an-annotation`, `imported-spawner-assigned-with-an-annotation`), each with the guard flags it and the fixed twin passes; the annotation test gains a bare `pending: subprocess.Popen` with no value. `test_git_output_decoding.py` 59 -> 67. Mutants 17 -> 19: removing the `AnnAssign` branch fails the two module cases, and reading an annotated assignment's value as an annotation fails the two spawner cases. Unchanged: the hardened guard still flags 0 offenders over the shipped Python at 71b58a1 and c7c02fa, 51 spawner calls, `found` lists identical to main's. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

brettheap · 2026-10-12T11:40:56Z

(lane openRepoShape-3, 2026-10-12T11:40:55Z) fix round 1: codex's P2 at 960cd17 is answered. TextModeCalls.bind now follows an ast.AnnAssign with a value exactly as it follows an ast.Assign, so sp: object = subprocess then sp.run(argv, text=True) is flagged (asks for text and does not name ['encoding', 'errors']); measured on 960cd17, that module and again: object = sp after sp = subprocess both found nothing. An annotated binding of a spawner (run: Callable = subprocess.run, read: Callable = check_output) was already refused by the uncalled-reference rule, because only the annotation is skipped and not the value; each is now pinned by a case. Four new synthetic modules with fixed twins (SYNTHETIC 20 -> 24); test_git_output_decoding.py 67 passed (was 59), test_repo_hygiene.py 50 passed, test_cli_reference.py 13 passed; 19 mutants (the 17 and two new ones), all caught: removing the AnnAssign branch fails the two module cases, reading an annotated assignment's value as an annotation fails the two spawner cases. The hardened guard still flags 0 shipped offenders at 71b58a1 and c7c02fa (51 spawner calls, found lists identical to main's), and the nine wave-A trial merges are still CLEAN with 0 flagged. A walrus, an unpacking or a for target holding the module is still not followed; the docstring says so. Head d578bd8, a fast-forward on 960cd17.

brettheap · 2026-10-12T11:41:01Z

(lane openRepoShape-3, 2026-10-12T11:41:01Z) @codex review

Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

chatgpt-codex-connector · 2026-10-12T11:45:58Z

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: d578bd884b

ℹ️ 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".

…rences` rows, which exempt nothing now that #196 and #216 are on main The branch merged main at 346079d (#228, issue #216; #196 landed earlier as 6f45bf9). The two rows were kept "until" those landed: - `scripts/shape_materialize.py` `env_commit`: its `subprocess.run` names `text=True, encoding="utf-8", errors="replace"`, so the default rule is met and the row exempted nothing (since #231). - `adopt-project.py` `cross_leg_references`: #216 made its `git grep` read BYTES (`capture_output=True`, no `text=`, the record split on NUL and each path decoded by hand), so it is no longer a text-mode call at all. Measured on c7c02fa the row was needed (`adopt-project.py:1080 asks for text and does not name ['encoding', 'errors']` without it); on 346079d the guard finds no text-mode call in that function. EXEMPT keeps the two `gh api` digest rows (`pin_digest_from_gh`, `tree_digest_from_gh`: UTF-8 yes, `replace` no), which still exempt `errors`. With both rows gone the guard flags 0 offenders over the shipped Python at 346079d: 51 spawner calls, 32 text-mode (33 before #216). Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

brettheap · 2026-10-12T11:51:11Z

(lane openRepoShape-3, 2026-10-12T11:51:11Z) fix round 2: merged main 346079d (#228, issue #216; #196 had landed as 6f45bf9), clean, two trailers on the merge commit; dropped EXEMPT rows ("scripts/shape_materialize.py", "env_commit") and ("adopt-project.py", "cross_leg_references"). Neither call offends on main: env_commit's subprocess.run names text=True, encoding="utf-8", errors="replace" (the row has exempted nothing since #231), and #216 made cross_leg_references' git grep read bytes (capture_output=True, no text=, the NUL-separated records decoded by hand), so the guard finds no text-mode call in that function. Evidence that the row was needed until now: at c7c02fa the same guard without it flagged adopt-project.py:1080 (cross_leg_references) asks for text and does not name ['encoding', 'errors']; at 346079d with both rows gone it flags 0 over the 20 shipped files (51 spawner calls, 32 text-mode, 33 before #216). EXEMPT keeps the two gh api digest rows (pin_digest_from_gh, tree_digest_from_gh), which still exempt errors. At 90ecef7: test_git_output_decoding.py 67 passed, test_repo_hygiene.py 50 passed, test_cli_reference.py 13 passed; the nine wave-A trial merges are still CLEAN with 0 flagged (only fix/adopt-refuse-existing-split-branch adds a subprocess call, and it reads bytes). Head 90ecef7, a fast-forward on d578bd8.

brettheap · 2026-10-12T11:51:16Z

(lane openRepoShape-3, 2026-10-12T11:51:15Z) @codex review

sonarqubecloud · 2026-10-12T11:51:30Z

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

chatgpt-codex-connector · 2026-10-12T11:55:14Z

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 90ecef70c8

ℹ️ 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".

…, #253 via #264, #302 via #318, #288 via #309, #279 via #310, #306 via #311, #334 via #336, #239 via #269, #243 via #268, #321 via #335) into fix/adopt-record-submodule-placement Brings in the twelve commits from c7c02fa to a1b3a4b on top of 71b58a1, merged by sha. No rebase. Among the files this branch changes: 346079d (#216) and e43fa95 (#243) change adopt-project.py; 34ecbc2 (#260) changes scripts/shape_materialize.py; 87373a6 (#306) changes shape-doctor.py and tests/test_shape_doctor.py; 7491530 (#288) changes AGENTS.md and tests/test_repo_hygiene.py. Two conflicts, each resolved keeping both sides: - adopt-project.py, the module docstring's `execute` paragraph. #243 ends "the root's only submodules are the two leg mounts, each at the commit pushed to its leg."; this branch follows "two leg mounts." with "The split's `project.yaml` records the leg each source submodule kept in a leg was answered into, under `submodules:`, for `shape-doctor.py` to check it against." The merge has #243's sentence, then this branch's, re-wrapped to the paragraph's width. The module docstring is not in `--help` (docs/cli.md holds neither sentence). - tests/test_repo_hygiene.py, `test_agents_md_is_short_enough_to_be_read`. Each side moved the AGENTS.md cap from 464 and wrote a paragraph saying why: #288 to 477, for thirteen lines, and this branch to 472, for eight. AGENTS.md itself merged cleanly and holds both: 485 lines, main's 477 plus this branch's +9 -1. The merge keeps #288's paragraph word for word, then this branch's, renumbered "2026-10-12: 477 -> 485" because it now follows #288's, and asserts `<= 485`. `git merge-tree --write-tree` of the two parents gives the same tree in every file but these two. Checked: - adopt-project.py: `_commit_the_split` was changed by both sides, in hunks that do not touch. #243 clones the assembly with `--template=`; this branch builds `append` with the `submodules:` record. The merged function holds both, and each side's text is unchanged. Every other definition one side changed is that side's text. - shape-doctor.py: #306 changes `validator_row` and `run_validator`. This branch's placement functions, F1's `gitmodules_the_record_answers` among them, are this branch's text. - No top-level name is bound twice in adopt-project.py, scaffold-project.py, scripts/repo_shape.py, scripts/shape_materialize.py, shape-doctor.py, validate-manifest.py, update-shape.py, scripts/family.py or scripts/bump-leg.py. Each merged module's names are the base's plus what either side added. - AGENTS.md is main's plus exactly this branch's changed lines. - The branch's diff against main is #189 alone: 12 files, +1694 -157. That is +1670 at 27d039b, plus F1's fix and test (+23), plus the one line the docstring re-wrap adds. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… of main's commits merge with no conflict, among them #243's template-less clone of the assembly and #216's literal `git grep` in adopt-project.py, and #260's one-pass `render` in shape_materialize.py origin/main moved 71b58a1 -> 8cdca5e. It was merged by sha, as the coordinator asked for e43fa95 or later: - c7c02fa (#299 via #315), scripts/family.py; - 346079d (#216 via #228), adopt-project.py: `plan`'s follow-up `git grep`; - 34ecbc2 (#260 via #316), scripts/shape_materialize.py: `render` substitutes in one pass; - 86da808 (#253 via #264), scripts/bump-leg.py; - 6f4ce47 (#302 via #318), scripts/family.py; - 7491530 (#288 via #309), AGENTS.md, README, docs and scaffold-project.py's --help; - 7591455 (#279 via #310), tests/conftest.py: `git()` takes its default identity as `-c` config; - 87373a6 (#306 via #311), shape-doctor.py; - 8a14ac9 (#334 via #336), tests/test_update_shape_landing.py; - c1b6732 (#239 via #269), update-shape.py; - e43fa95 (#243 via #268), adopt-project.py: the assembly root is cloned with `--template=`, and the verification refuses a mount whose gitlink is not the commit pushed to its leg (`adopt-mount-commit`); - a1b3a4b (#321 via #335), tests/test_git_output_decoding.py; - 8cdca5e (#295 via #312), the template's scripts/validate-pins.py. `git merge-tree` and this merge give the same tree, 07e6b49. adopt-project.py, scripts/shape_materialize.py and tests/test_adopt_ignored_shape_files.py auto-merge. In the last of these main's 346079d and this branch's 86a5e30 change different cases. Checked on the merged tree: - AST (fin183g/names.py, base 71b58a1): no top-level name is bound twice in adopt-project.py, scaffold-project.py, scripts/repo_shape.py or scripts/shape_materialize.py, and none is loaded but bound nowhere. Main changed the signature of no function this branch calls. - Every sentence 86a5e30 changed is still there: in `_through`, in `_link_parts`, in `_verify`'s `_resolve` (which e43fa95's `adopt-mount-commit` leaves as it was), and in the three test files. - `_commit_the_split` now clones the assembly with `--template=`. The next commit, F3, reads what that clone holds in its own `.git`, which is why F3 comes after this merge. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… writes its pin file as the scaffold does, so an adopted root passes `validate-pins.py` (#273); a `--pin-source` that cannot answer is `pin-source-unreadable` (#339) (#508) **Brought up to date after #409 squash-landed** by merging origin/main (9736424, #409's squash) as f9ed46f. Its `scaffold-project.py` conflict was resolved to this branch's side once `git diff b70a07d 9736424 -- scaffold-project.py` (#409's final head against its squash) came out empty. So the file is the one tested on this branch plus main's own later changes to it, and holds nothing of #409's; details below. This branch carries #275's adopt half. It was built on `fix/pin-source-env-collisions`, PR #409 (#275's scaffold half and its fix round), merged at 6494714 as 583f647, over the earlier 5690235 (34dbd86). After 6494714, #409 only merged main, five times, up to its final head b70a07d, and none of those merges has a hand edit (`git show --remerge-diff` is empty for each). Adopt's refusal of two products that share a source variable goes through #409's function, so this branch lands after #409. It was never rebased. `git diff b70a07d 9736424` is empty for each file #409 touched: `scaffold-project.py`, `scripts/repo_shape.py`, `templates/assembly-root/scripts/validate-pins.py` and `tests/test_pin_source_env_collisions.py`. What f9ed46f resolves, four files with one hunk each: - **`scaffold-project.py`.** - The conflict hunk is main's block from `PIN_COLLISION_REMEDIATION` through `_pin_collision_remediation`, which #409's squash edits: it adds `PIN_VARIABLE_REMEDIATION`, `_colliding_pin_variables` and `_pin_collision_remediation` and changes `_refuse_colliding_pins`. This branch moved all of that to `scripts/shape_materialize.py`. The hunk takes this branch's side. (The merge commit's message says "re-adds"; this is the exact account.) - #409's auto-merged import of `PIN_SOURCE_ENV_PREFIX, pin_source_env_collisions` is dropped, because nothing in the file uses either name any more. - It is not `git checkout --ours` of the whole file. Main changed the file beyond #409 since this branch last merged it: #355's `--tracking-branch` check (6c2a371) and a73b4e9's `RULESET_HINT`. Those lines stay. - The result is the file `git merge-tree` gives for this branch with main before the squash (518010b). Its diff from the file tested here has the same patch-id as main's own 3ef059c..518010b change to it. - **`templates/assembly-root/scripts/validate-pins.py`.** Main's side, which is #409's final blob, with main's #359 workflow reader in it. This branch had no change of its own to the file beyond #409. - **`adopt-project.py`, `_commit_the_split`.** - Main: #445 wraps the function in its late-failure `try`, and #394 builds an `append` with the plan's submodule record. - This branch: its only edits there were a comment and `neutral_pins=_recorded_pins(plan)` on the materializer call. - Both now sit on main's call inside the `try`: `append=append, neutral_pins=_recorded_pins(plan)`. - **`docs/cli.md`.** It is regenerated with `scripts/render-cli-reference.py`, and `--check` is ok. It differs from main's only in the `--pin`, `--pin-owner` and `--pin-source` help of `adopt-project.py plan`. `scripts/repo_shape.py`, `templates/assembly-root/scripts/validate-pins.py` and `tests/test_pin_source_env_collisions.py` are main's blobs, which are #409's: this branch adds nothing to them. Nothing of #409 is lost. Its scaffold behaviour is `shape_materialize.refuse_colliding_pins` and `pin_variable_collisions`, keyed by #409's `pin_product_key`. **73d65c6, after the merge.** - **The change.** `_read_pins` reads `plan.get("pins")` where it read `plan.data.get("pins")`. - **Why.** #445's `tests/test_adopt_late_failure.py` hands `_commit_the_split` a `SplitPlan` as its plan, a `dict` that adds `entries`. On f9ed46f, three of its tests stopped at "committing the split" with `AttributeError: 'SplitPlan' object has no attribute 'data'` (3 failed, 83 passed in that file). - **Nothing else changes.** `Plan.get` with no default returns `plan.data.get(key)` exactly, so a loaded plan reads as before. `adopt-project.py plan --pin` took a NAME only (`--pin openGlass`) and refused a trailing `@<commit>`. `plan`, `check` and `execute` each exited 0, and `execute` printed `adoption verified`. The manifest then listed the name in `neutral_product_pins:` with no `contracts/<product>-pin.yaml` beside it, and on its first run the adopted root's own `validate-pins.py` refused it as `neutral-pin-missing`. #273's table, reproduced on c7c02fa with the issue's own commands (local bare remotes, and a stub `gh` that exits 97): | `--pin` | c7c02fa | this branch | |---|---|---| | `openGlass` | `execute` exit 0, `- openGlass`; clone's `validate-pins.py` exit 2 `neutral-pin-missing` | `plan` exit 2 `pin-malformed`, with the scaffold's remediation (`--pin openGlass@<40 hex>`); no plan is written | | `opensoft/openGlass` | exit 0, `- opensoft/openGlass`; exit 2 `neutral-pin-missing` for `contracts/opensoft/openglass-pin.yaml` | `plan` exit 2 `pin-malformed` | | `spec` | exit 0; `validate-pins.py` exit 0, reading the leg's `contracts/spec-pin.yaml` as the neutral pin | `spec@<sha>` is `adopt-pin-collides` at `plan` | | `shape` | exit 0; `contracts/shape-pin.yaml` read as the neutral pin | `shape@<sha>` is `adopt-pin-collides` at `plan` | | `openGlass@<sha>`, `opensoft/openGlass@<sha>` | `plan` exit 2 `adopt-pin-malformed` | `plan`, `check` and `execute` exit 0, and `gh` is called 0 times. `contracts/openglass-pin.yaml` is written and the manifest lists `- openGlass`. In the clone, `validate-manifest.py` exits 0, and `validate-pins.py` exits 0 with `openGlass: pin commit … digest recomputes` | Brett ruled for decision 1's first option ("go with your recommendation on #273"): adopt takes the scaffold's pin and writes the scaffold's pin file. That reverses the NAME-only `--pin` from #6, and the comment that described it is replaced with the new contract. - **One implementation.** The scaffold's pin code moved into `scripts/shape_materialize.py`, next to `neutral_pin_path` and the materializer that writes the file. What moved: - `PIN_ARG_RE`, `parse_pin`, `pin_digest_from_source` and `pin_digest_from_gh`; - `declared_pin_values`, `declared_pin_products` and `PIN_COLLISION_REMEDIATION` (in #309's wording); - from #275's branch, `PIN_VARIABLE_REMEDIATION` and the variable check; - `_refuse_colliding_pins`, split into the pure `pin_collisions` and `pin_variable_collisions`, and `refuse_colliding_pins(refusal=...)`, which raises one refusal for both. The code did not move verbatim. `_refuse_colliding_pins` became the three functions above; `pin_digest_from_source` was rewritten for #339 (below); and fix round 1 changed three of them (below): `pin_variable_collisions` keys each pin by #275's `pin_product_key`, `pin_digest_from_source` spells a path that is not printable, and `pin_digest_from_gh` refuses a missing `gh`. The scaffold's behaviour is otherwise unchanged. `scaffold-project.py` imports what it calls under its old names. Lane 5's review ran 34 scaffold cases (every pin refusal, the `gh` stub's four modes, dry and real successes) on 34dbd86 and on this branch at 7a94b6b. rc, stdout, stderr, the `gh` log and the files made were identical, apart from the shape's own commit and digest and #339's row. Its pin tests and #275's pass as they are. `tests/test_git_output_decoding.py`'s exemption for `pin_digest_from_gh` follows the function to its new file. - **`plan`.** `--pin`, `--pin-owner` and `--pin-source` mean what they mean for the scaffold. `plan` reads the digest from `--pin-source` or `gh api`, and records product, repository, commit, digest and `digest_source` in the plan, so `check` and `execute` read no network for a pin. `plan` refuses: - a name with no `@<commit>`, as `pin-malformed`. That includes an empty `--pin ''`, which adopt used to drop and now refuses, as the scaffold always has; - `spec`, `code` and `shape`, in any case, as `adopt-pin-collides` (#273 decision 4), in the scaffold's sentence and remediation (#241, #309); - one product given twice, the same way, whether it is a repeat or two names that share one file (decision 5); - two products that one `SHAPE_PIN_SOURCE_<PRODUCT>` names, such as `open-glass` and `open_glass`, the same way, through #275's check; - a leg path over a pin's file, as `adopt-leg-path-collides`; - a `--pin-source` path that `check` would refuse in `digest_source:` (fix round 1), as `adopt-unwritable-value`. It is asked only when a `--pin` records the path; - with no `gh` on PATH and no `--pin-source`, `gh-not-found` (fix round 1). - **`check` and `execute`.** These are new `check` findings, and `execute` refuses each one before any leg exists. They come from `_leg_findings`, the list `check` and `_refuse_what_check_finds` share: - `plan-pin-unrecorded`: a name alone (an old plan's `pins: [openGlass]`, or a hand edit), or an entry missing its commit or digest; - `plan-pin-malformed`; - `plan-pin-collides`, which covers two pins at one file, a pin at a file the shape writes, and two products that share one source variable; - `plan-root-holds-pin`: a source path kept in the root at the pin's file. The materializer would write the shape's pin beside it under `shape/`, and `validate-pins.py` would read the source's file instead. How each finding ends: - `plan-pin-unrecorded` and `plan-pin-malformed` end with the `plan --pin <product>@<40 hex commit>` that records the pin whole. - `plan-pin-collides` ends with re-running `plan` with each product once, or, for two products that share a source variable, with one product of each such group. - `plan-root-holds-pin` ends with the kept path's own remedy: send that entry to a leg, `drop` it, or rename it in the source and re-run `plan`. A leg moved over a pin's file in the plan is `plan-leg-path-collides`. - **Also part of #386: its `pins:` row.** A hand-edited `pins:` that is not a list (a name such as `pins: openGlass`, a number, a mapping, a quoted string) is now `plan-pin-malformed` in `check` (`` `pins:` is 'openGlass', not a list of pins ``), and `execute` refuses it before any leg exists. - Where: `_read_pins` in `adopt-project.py`; the test row is `plan-pin-malformed-not-a-list` in `tests/test_adopt_pins.py`. - Before: on main, `check` said `plan ok` for the name, the mapping and the string, and gave a traceback for the number (reproduced by lane 5's review on cb7063f). #386 records that `execute` then wrote the name as eight one-letter pins. - Not fixed here: #386's other keys, `allow_empty_legs:`, `paths:`, `legs:` and `source:`, so #386 stays open. Lane 2's #435 takes all of #386, `pins:` included, and its `pins:` rule keeps only text items where this branch writes mappings; whichever lands second reconciles them (#435 (comment), and "Where the meaning meets" below). - **`execute`.** It writes each pin through `materialize_assembly_root(neutral_pins=...)`. The manifest lists the product with the owner stripped (decision 3), so the entry and the file agree. - **Docs.** `--help` is updated and `docs/cli.md` regenerated (`render-cli-reference.py --check` ok). `pin_digest_from_source` caught only `Refusal`. A directory that is no clone is git's own failure, so it was always `pin-source-unreadable`, exit 2. A path `subprocess.run` cannot enter never reaches git: it is a `FileNotFoundError`, `NotADirectoryError` or `PermissionError` out of `cwd=`, and nothing caught it. `scaffold-project.py --pin ... --pin-source <that>` ended `--dry-run` and the real run alike in a traceback, exit 1. Since the move above, `adopt-project.py plan --pin` reads its digest through the same function and ended the same way. Both reach it through `declared_pin_values`, and `check` and `execute` read no source, so that call is the only reader. The issue's table, run on ab623dd for the scaffold and on d806e8f for `adopt-project.py plan` (adopt has no `--pin-source` on ab623dd), then on this head. `gh` is a stub that logs and exits 97, with `GH_TOKEN` bogus and an empty `GH_CONFIG_DIR`; it was called 0 times in every run: | `--pin-source` | before | this branch | |---|---|---| | a directory that is not a clone | `pin-source-unreadable`, exit 2 | unchanged | | a path that does not exist | `FileNotFoundError` traceback, exit 1 | `pin-source-unreadable`, exit 2: `<path> could not answer for opensoft/openGlass @ <40 hex>: no such directory` | | a file | `NotADirectoryError`, exit 1 | exit 2, `... it is a file, not a directory` | | under a parent this user cannot enter | `PermissionError`, exit 1 | exit 2, `... Permission denied` | | a symlink loop (not in the issue) | `RuntimeError` out of `Path.resolve()` on 3.12, exit 1 | exit 2, `... Too many levels of symbolic links` | The real run and `--dry-run` give the same stdout and stderr, and nothing exists afterwards (no remote directory, no work directory; for adopt, no plan file). What changed, in `scripts/shape_materialize.py` only: - `pin_digest_from_source` catches `OSError` as well as `Refusal` and raises the existing `pin-source-unreadable` with its remediation unchanged. The message names the path and the reason. - `stat()` judges the source inside the guard, as #312 does in `validate-pins.py`, and the sentences are that file's: a missing path, or one beneath a file, is "no such directory"; a file is "it is a file, not a directory"; any other `OSError`, from `stat()` or from git's `cwd=`, is the system's own reason (`strerror`). It is not `exists()`/`is_dir()`, which before 3.14 swallow only a missing path. A source `stat()` can read but `cwd=` cannot enter, a directory with no search bit, fails in `tree_digest` and the same guard answers it. - `declared_pin_values` resolves the path through a guarded `_resolved_pin_source`. `Path.resolve()` raises a `RuntimeError` on a symlink loop before 3.13 (an `OSError` since), before the digest was asked for. As typed, `stat()` meets the loop and says so. - A source that answers is read exactly as before: a relative path resolves against the working directory, a symlink to a clone is followed, `digest_source` reads `local-clone (<path as typed>)`, and the digest is `tree_digest`'s. `tests/test_pin_source_unreadable.py` (46 tests in the #339 commit; fix round 1 adds 17 for S1 and moves one, below) runs each case through `scaffold-project.py --dry-run`, its real run and `adopt-project.py plan`: missing, a file, beneath a file, relative and missing, a parent and a source with no search bit (skipped on Windows and as root, by a mark, so before any fixture builds a directory), a 300-character name, a symlink loop (skipped on Windows) and a directory that is no clone. Each asserts exit 2, the refusal naming the path and the reason, its remediation, no traceback and nothing created. It also holds `--dry-run` and the real run to the same stdout and stderr, checks the path is printed escaped under an ASCII stderr and as a surrogate when it is not UTF-8, and checks that a relative source and a symlink to a clone read as before. The session `gh` guard fails any test whose run called `gh`. origin/main ab623dd reached this branch through implQ's branch: 34dbd86 is a30091f merged with ab623dd. One file conflicted there, `templates/assembly-root/scripts/validate-pins.py`, in seven hunks, because #312 (8cdca5e) replaced `keyed_sources` and `default_source` with `PinSources` and #275 added `env_shared` and `shared`. #312's blob is the same at its PR head, at the squash and at ab623dd, so it landed the file implQ resolved against. The resolution keeps #312's side in all seven and adds #275's on top: | # | hunk | resolution | |---|---|---| | 1 | `resolve_neutral_pin_source` signature | #312's `sources: PinSources` and 3-tuple return, plus #275's `env_shared: bool = False` | | 2 | its docstring tail | #312's `Returns (path, how, typed)` text, plus #275's "Rule 2 is skipped when `env_shared`" paragraph | | 3 | `_recompute_neutral_pin_digest` signature | #312's `sources`, plus #275's `shared: frozenset[str]` after `report` | | 4 | the `resolve_neutral_pin_source(...)` call inside it | #312's 3-tuple unpack, plus `env_shared = product.casefold() in shared` and the extra argument | | 5 | `_check_one_neutral_product_pin` signature | #312's `sources`, plus #275's `shared` | | 6 | its call of `_recompute_neutral_pin_digest` | `sources, report, shared` | | 7 | `_check_neutral_product_pins` docstring and body | #312's `PinSources(...)` and `_note_unused_pin_sources`, plus #275's paragraph, `_report_pin_source_collisions(products, report)` and `shared = frozenset(pin_source_env_shared(products))` | This branch's own merge of 34dbd86 (5690235) had no conflict, and neither had its merge of #409's fix round, 6494714 (583f647). That fix round made `neutral-pin-source-collides` a note and keyed "one product" by `repo_shape.pin_product_key`. At 421e4e5, `templates/assembly-root/scripts/validate-pins.py`, `scripts/repo_shape.py` and `tests/test_pin_source_env_collisions.py` were byte-identical to the tree `git merge-tree` gives for 6494714 with origin/main 3ef059c (blobs 5604007, 091966b, 5220787), so this branch added nothing to those three files beyond #409. The 34dbd86 merge brought in #335, #312, #317, #270 (f795cf7), #344 and #349 with no conflict. Earlier merges of origin/main (87373a6, for #309's reworded constant, and e43fa95) are unchanged. Fix round 1 merged origin/main three more times: - **cd1fc79 (2bcfa69).** It brought #245, #351, #350, #345, #372, #371, #354, #380, #377 and the rest since ab623dd, with no conflict. - **45e01f4 (4098b61).** It brought #396's `tests/test_agents_md_codes.py`. That test finds `scaffold-pin-collides` as a string constant of `scripts/shape_materialize.py`, which `SHIPPED` lists. - **3ef059c (421e4e5), #314.** One conflict, one hunk in `adopt-project.py`. This branch's `_refuse_an_unrecordable_pin_source` and #314's section "The project must be able to hold what `execute` writes into it (#261)" were added at the same place; both are kept. Where the two meet, lane 5's earlier recipe applies: - `_flag_plan` builds each pin as the `PIN_FIELDS` mapping `plan` writes, not `list(pins)` of products. So the plan `plan` asks the validator about is the plan it writes. - #314's `test_plan_asks_what_check_asks_of_the_same_keys` plans `--pin openGlass@<commit> --pin-source <stand-in>` where it planned the name alone, which `plan` now refuses. Then f9ed46f, after #409 squash-landed, and 73d65c6 after it (top of this description). On the merged tree exactly one test failed: `tests/test_adopt_template_values.py::test_check_finds_and_execute_refuses_a_hand_edited_value[pins-surrogate]`. It plans, then hand-edits the plan to `pins: ["openX\ud800"]`, a name-only pin with a lone surrogate. Before this branch the refusal's headline was `REFUSED plan-unwritable-value`. A name-only pin in a plan is now `plan-pin-unrecorded` (#273), so `check` has a second, independent finding for that very plan, and the headline is the first finding's: ``` REFUSED plan-pin-unrecorded: 2 finding(s) ... FINDING plan-pin-unrecorded: `pins:` entry 1, 'openX\ud800', is a name with no commit and no digest ... FINDING plan-unwritable-value: a `pins:` item is 'openX\ud800', which `execute` writes into the assembly root's `project.yaml`, and it holds a lone surrogate ... ``` Nothing about #270's contract changed. The lone surrogate is still found as `plan-unwritable-value`, listed under the refusal, and `execute` still refuses before any leg exists. `check` builds `_leg_findings`, where `_pin_findings` sits, before it calls `_value_findings`, so the pin finding comes first. d806e8f changes one assertion for that row, `pins` only: the headline is `REFUSED plan-pin-unrecorded: ` where it was `PLAN_REFUSED`. The other 15 rows and the finding assertions, including the `plan-unwritable-value` line, are untouched. Lane 5's review of 7a94b6b found 3 should-fix and 5 nits. Each is answered here: - **#409's `pin_product_key` (review N1).** The merge of #409's fix round could not carry it, because this branch had moved the function that used it out of `scaffold-project.py`. `pin_variable_collisions` now keys each pin by that function, where it casefolded. `openß` and `openss` are two pin files and one `SHAPE_PIN_SOURCE_OPENSS`, and a casefold named each of the two pins twice. No `--pin` or recorded `pins:` entry `PIN_ARG_RE` admits can spell either, so the test asks the function directly. `refuse_colliding_pins`' docstring now says, in #409's words, that the root's `validate-pins.py` only notes such a pair. - **S1, a `--pin-source` path that is not printable.** `pin_digest_from_source` names the path as typed unless it is not printable (`str.isprintable`), and then as Python's `repr` spells it (`shown_source`). That covers the refusal's head and git's own reason, which names the directory it ran in. Printed raw, an ESC reached the terminal, and a line break or a carriage return began a line of the path's choosing, a forged `REFUSED` line among them, from the scaffold and from adopt. The traceback #339 replaced had shown a `repr`. A printable path is named byte for byte as before. - **S2, no `gh` on PATH.** `pin_digest_from_gh` refuses as `gh-not-found`, exit 2, as `repo_shape.tree_digest_from_gh` names it, and offers `--pin-source`. It was a `FileNotFoundError` traceback, exit 1, from the scaffold and, new with this branch, from `adopt-project.py plan --pin`. - **S3, a hand-edited `commit:` or `tree_sha256:`.** The lane's decision is to accept it and document it (option (c)); no behaviour changed. A pair that disagrees passes `check` and `execute` (`adoption verified`), because neither reads a network for a pin, by design. The adopted root's own `validate-pins.py` then exits 1 with `neutral-pin-digest-mismatch` wherever it can read the product. `check` now says so once, after the pins it will write: "each pin as `plan` read it, not read again here: the root's own `validate-pins.py` recomputes its digest wherever it can read the product". Options (a), `execute` recomputes before it creates the legs, and (b), `check` recomputes from a clone still on the machine, are filed as #449. - **N2, a `--pin-source` that `plan` cannot record.** `plan` asks `check`'s question (`one_printable_line`) of the path it records in each pin's `digest_source:`, before any digest is read. It refuses one that fails as `adopt-unwritable-value`, in #270's words (`unwritable_reason`), or as "an invisible formatting character" where those name nothing. Before, it was planned, exit 0, and printed raw in the report. `check` then refused it as `plan-pin-malformed` and said to re-run the same `plan`. The report's `pin` line and `check`'s `write` line now spell the digest's source with `_shown`. The scaffold half is pre-existing (a line break in the path is written into the pin file; a byte that is not UTF-8 dies after all three repositories exist) and is filed as #450. - **N3, six hunks no test guarded.** Each now has a test that fails when it alone is reverted: - the two name guards (a `pins:` name holding an ESC and a forged line); - the lower-casing of the commit and the digest (an upper-cased hand edit); - the `digest_source` printability check; - the not-text check (`commit: 1234567` unquoted); - the gitlink filter (a submodule kept where the pin is written is `plan-submodule-root` alone). - **N4.** This body. - **N5, #339's third decision.** A `--pin-source` that nothing reads is still silent, now in adopt too. This PR does not decide it. #451, filed, lays out refuse, note and leave, so closing #339 loses nothing. Lane 5's second review, of 421e4e5, found no blocking item, 2 should-fix items and 3 nits. The two should-fix items are this body: the landing after #409 (above) and #386 (above). The code needed no change for them. f63d1ed adds two tests and no code: - **n1. `plan` asks nothing of a `--pin-source` no `--pin` records.** With the `args.pin and` guard dropped, such a path holding an ESC or a zero-width joiner was refused in a sentence naming no file ("`execute` writes into , and ..."), and every test still passed. `test_a_pin_source_no_pin_records_is_not_asked_about` holds the plan to exit 0 and `pins: []`, with the path named nowhere in the plan or the report. The ESC row is POSIX-only by the file's `POSIX_NAMES` mark; the zero-width joiner row runs everywhere. It holds #451's answer today, silence, so a decision there changes this test on purpose. - **n2. A printable `--pin-source` is printed as it is.** With `_spelled` in place of `_shown` in `plan`'s `pin` line, every test still passed. `test_a_printable_pin_source_is_printed_as_it_is` holds `plan`'s `pin` line, and `check`'s `write` line for the plan `plan` wrote, to their exact text, ending `(local-clone (<path>))` with no backtick. - **n3** needs a code change and is left out (below). **Fix round 2, on f63d1ed.** Each run had a `gh` stub first on PATH (it logs and exits 97), `GH_TOKEN` bogus, an empty `GH_CONFIG_DIR`, `git-filter-repo` 2.47.0 next on PATH, and the identity blanked with `GIT_CONFIG_COUNT`; the session `gh` guard came first inside each run. - **The pin set: 812 passed, 2 skipped.** It covers adopt pins, adopt plan, #339's file, #275's tests, the pin and neutral-pin validators, git output decoding, adopt template values, CLI reference, scaffold pin and reuse, scaffold pin collisions, #314's two files and #396's file. The skips are `test_adopt_plan.py:403` (no MedxEHR clone) and `test_adopt_template_values.py:223` (this host reads `git config user.name` from the repository, not from `GIT_CONFIG_GLOBAL`). That is fix round 1's 809 passed, plus the three new rows. - **Shape doctor and scaffold leg-path collisions: 369 passed, 3 skipped** (the PowerShell line exists only on a Windows host). - **Mutation probes**, each one line changed in a throwaway worktree, over `tests/test_adopt_pins.py` and `tests/test_pin_source_unreadable.py` less the five end-to-end adoptions: - the `args.pin and` guard dropped: 2 failed, both n1 rows; - `plan`'s `pin` line `_spelled`: 1 failed, the n2 test; - `check`'s `write` line `_spelled`: 2 failed, the n2 test and `test_an_upper_cased_hand_edit_is_written_lower_cased`; - `plan`'s `pin` line raw: 1 failed, the no-break-space test that already guarded that hunk. **On the merged tree, 73d65c6** (the merge f9ed46f and the `_read_pins` commit), with the same harness: - **The pin set** above, with #445's `test_adopt_late_failure.py`, `test_adopt_git_reads.py` and `test_adopt_filter_repo_failure.py` and #387's `test_adopt_empty_symlinks.py` and `test_adopt_root_manifest.py`: **988 passed, 2 skipped**. The skips are the two above. - **Shape doctor, scaffold leg-path collisions and the adopt suites:** **967 passed, 3 skipped** (the PowerShell line). The adopt suites are e2e, execute refusals, verify, root shape paths (#387's too), leg-path collisions, clone template, ignored shape files and submodules. - **On the merge alone, f9ed46f** (the same tree as before the `_read_pins` commit): `tests/test_adopt_late_failure.py` gave 3 failed, 83 passed, the three `AttributeError` rows above. - `render-cli-reference.py --check` is ok on the merged tree. pyflakes reports the same four warnings on `adopt-project.py`, `scaffold-project.py` and `scripts/shape_materialize.py` as before the merge. **Fix round 1, on 421e4e5.** Each run had `git-filter-repo` 2.47.0 on PATH and the identity blanked with `GIT_CONFIG_COUNT`. A `gh` stub that logs and exits 97 was first on PATH, with `GH_TOKEN` bogus and an empty `GH_CONFIG_DIR`, and the session `gh` guard came first inside each run. - **The pin set:** 809 passed, 2 skipped (the same files and skips as above). - **Doctor and scaffold:** 782 passed, 3 skipped (the PowerShell line exists only on a Windows host). It covers shape doctor, scaffold leg-path collisions, scaffold e2e, referent chain links, naming policy, repo hygiene and #351's free-text values. - **The adopt suites:** 882 passed. It covers e2e, execute refusals, verify, spec-only, leg paths, root shape paths, plan escapes, leg-path collisions, clone template and submodules. - **Before the #314 merge,** on 74ce4d9's tree: the first two sets gave 577 passed, 2 skipped and 782 passed, 3 skipped. On 2bcfa69's tree (the merges alone, before the fix) they gave 542 passed, 2 skipped and 782 passed, 3 skipped. After the 45e01f4 merge, `tests/test_agents_md_codes.py`, repo hygiene and CLI reference: 110 passed. - **Lane 5's second review** also ran 421e4e5 merged with main cb7063f and with 53bee5b: the pin set, the manifest-value set, and the doctor set with #357's `tests/test_doctor_validator_rows.py`. All passed, with only the skips above. Proof that fix round 1's tests test something: - **On 2bcfa69's tree,** with this head's two test files laid over it, 31 of the new rows fail: - S1: 17; - N2 and its report spelling: 7; - S2: 3; - the `pin_product_key` port: 2; - `check`'s S3 line: 1; - the not-UTF-8 `adopt plan` row: 1. The N3 rows pass there, as they must: they guard hunks this branch already had. - **Mutation probes.** Each hunk was reverted alone, in a throwaway worktree, and both test files were run: 18 of 18 were caught. - **The port:** the casefold keying (2 fail); the casefold lookup alone (2). - **S1:** the path named raw (13); git's reason named raw (2); every path `repr`'d, printable or not (27). - **S2:** no `except` (3). - **N2:** no `plan` check (11); only #270's question asked (1); the formatting-character reason said otherwise (1); the report's `pin` line raw (1); `check`'s `write` line raw (1). - **S3:** the line dropped (1). - **N3:** the `_pin_fix` name guard (1); the `_read_pins` path guard (1); the lower-casing (1); the `digest_source` check (1); the not-text check (1); the gitlink filter (1). - **The issue rows reproduced.** S1, S2 and N2 were reproduced on 2bcfa69's tree and shown gone on the fix (a probe driving both tools; `gh` was called 0 times). **Before fix round 1.** Runs were on 7a94b6b, with `git-filter-repo` 2.47.0 first on PATH and the repository-local `user.name` blanked with `GIT_CONFIG_COUNT` (a workstation whose standard checkout sets one fails `test_the_scaffold_refuses_with_no_elector` on main itself; CI sets none). 7a94b6b differs from ffd1297 in `tests/test_pin_source_unreadable.py` only, so the three suites that do not read that file ran on ffd1297: - the new file: 46 passed; - adopt pins, adopt plan, git output decoding: 172 passed, 1 skipped (the MedxEHR clone is absent); - scaffold pin and reuse, pin collisions, e2e, referent chain links, neutral pin and pin validators, #275's tests, naming policy, repo hygiene, CLI reference, scaffold leg-path collisions: 571 passed; - shape doctor: 254 passed, 3 skipped (the PowerShell line exists only on a Windows host); - the new file with repo hygiene, git output decoding and CLI reference, once more after the last edit: 176 passed. Earlier on 5690235 and d806e8f, before the #339 commit: the adopt suites (e2e, execute refusals, verify, spec-only, leg paths, root shape paths, plan escapes, leg-path collisions, clone template) 740 passed, and `tests/test_adopt_template_values.py` with #273's other adopt tests 489 passed, 2 skipped. They were not re-run on 7a94b6b: the #339 diff touches `pin_digest_from_source` and the call in `declared_pin_values`, which those suites reach through `test_adopt_pins.py` only. Nothing was run on macOS or Windows. Proof the new tests test something: - **On d806e8f.** 38 of the 46 fail, each on the traceback and its exit 1. The 8 that pass are the unchanged behaviour: the not-a-clone refusal (4 rows) and the valid-source rows (4). - **Mutation probes.** Ten hunks reverted one at a time, each failing at least one new test: the `OSError` catch (16 fail), the catch narrowed to `OSError` only (21), the missing-path clause (9), the clause narrowed to `FileNotFoundError` (3), the `S_ISDIR` judgement (3), the loop guard (4), no resolve at all (3), the old `.resolve()` call (4), the reason as `str(exc)` (12), and the file sentence as the OS's (3). - #273's own 52 tests still fail on c7c02fa (51 fail or error; the one that passes checks the test's own table), and its 18 mutation probes were each caught. **Re-verified after the merge.** An independent re-verification of f9ed46f and 73d65c6 found the code correct: - #409 is carried whole. A 30,000-case fuzz of `--pin` lists through #409's final refusal and this branch's port differed in 0 cases. - `scaffold-project.py` equals the tested file plus main's own changes that are not #409's (same patch-id). - `_commit_the_split` keeps #445's `try` and #394's `append`. - `_read_pins` reads a loaded plan as before. Its findings were all in this description (the #506 bullet and three stale facts), now fixed. Runs on the head and on its merge with main 48585e6: 988 passed, 2 skipped; and 967 passed, 3 skipped. `git merge-tree --write-tree` was run at 01:48Z on 2026-10-13, comparing hunks per file. Each PR head, fetched from `refs/pull/<n>/head` with no `gh`, was merged with 73d65c6 and with origin/main 9736424 alone. Two sets were asked about: - every PR this description listed before that main does not now contain; - every PR numbered above #468 whose head main does not contain: #473, #474, #490, #491, #493, #494, #495, #496, #497, #503, #504 and #505. **Open state, read with `gh` by the coordinator at 02:00Z and refreshed at 02:25Z after a re-verification on main 48585e6.** Every pull request named below is open. #184, #94, #96, #98, #100 and #161 are closed, and #465 landed as 48585e6. `fix/adopt-plan-quote-yaml-keywords` has no pull request. #506 (`fix/scaffold-reuse-seeded-legs`) opened after this list was made, and the re-verification reads it. - **Landed since the last draft of this list:** #343, #346, #355, #358, #373, #378, #382, #387, #394, #397, #401, #404, #409, #419, #452, #456, #465, #466 and #484. #394's `_commit_the_split` conflict and #401's `docs/cli.md` conflict are both resolved in f9ed46f, above. - **Clean, 20:** #266, #347, #356, #374, #376, #384, #395, #453, #467, #468, #473, #474, #490, #491, #493, #494, #495, #497, #503 and #505. #453, #468 and #491 were updated onto main since the first read and now merge clean. #507 (lane 5's #486-#488) shares only the generated `docs/cli.md` with this branch and merges clean with it. - **Conflicting with main alone, and this branch adds no hunk:** - #402: `shape-doctor.py` 5; - #496: `shape-doctor.py` 10 and `tests/test_shape_doctor.py` 1. **Conflicts this branch adds:** - **#435** (3efcc7a, lane 2's `fix/adopt-plan-field-types`, for #386). - It adds 2 hunks in `adopt-project.py`: 5 here, against 3 with main alone. They are `render_plan`'s `pins:` line and `Plan.pins`. - Keep this branch's `_pin_lines` and `_pin_products`. The rest is under "Where the meaning meets". - **#504** (37ef29a, `fix/adopt-root-kept-symlinks`). - It adds 1 hunk in `adopt-project.py`: 2 here, against 1 with main alone. It is the materializer call in `_commit_the_split`. - Keep both arguments and both comments: `append=append, neutral_pins=_recorded_pins(plan), spell=_spelled`. - **#506** (119d8c4, `fix/scaffold-reuse-seeded-legs`). - It adds 1 hunk in `scaffold-project.py`. With main alone, #506 conflicts only in `docs/handbook.html`. - #506 inserts `LEG_TEMPLATES` right after `declared_pin_products`, which this branch moved out, and rewrites `_reusable_remotes` further down. Keeping both sides breaks the file with an unterminated string literal. - Keep only #506's `LEG_TEMPLATES` block. Drop #506's `declared_pin_values`/`declared_pin_products`, which moved here, and this branch's three-line `_reusable_remotes` opener, which #506 rewrites below. Resolved that way, pyflakes is clean and #506's scaffold tests pass (88). - **`fix/adopt-plan-quote-yaml-keywords`** (ed9968f; a branch with no pull request). - It adds 2 hunks in `adopt-project.py`: 3 here, against 1 with main alone. - One is `render_plan`'s `pins:` line. Keep `_pin_lines`, which already sends product, repository and `digest_source` through `y`. - The other is `_refuse_unholdable_values`' signature. Keep `pins: dict`, with that branch's `_validator_judged` and `_named_as_read` above it. - `fix/adopt-late-failure-notice` is no longer on origin. #445, the late-failure notice, is on main and merged here. **Where the meaning meets:** - **#435 and #386.** The coordinator has said this on #435: #435 (comment) - #435's `_plan_key_findings` (`plan-bad-pins`) keeps only text items in `pins:`, and its `PINS_REMEDIATION` tells a person to write `pins: [openGlass]`. - This branch writes each pin as a mapping, and it makes a name alone `plan-pin-unrecorded`. - Read from #435's code, not run: on a tree with both, the plan `plan --pin` writes is `plan-bad-pins` in `check`. - Whichever lands second reconciles them, with three changes: - `_plan_key_findings` takes this branch's mappings, or leaves `pins:` items to `_read_pins`; - a `pins:` that is not a list gives one finding, not both `plan-bad-pins` and `plan-pin-malformed`; - `PINS_REMEDIATION` says the mapping `plan` writes. - Until that is done, `tests/test_adopt_pins.py::test_a_plan_with_a_whole_pin_checks_ok` fails. - **#314.** It landed, and it is merged here (above). - **`fix/adopt-plan-quote-yaml-keywords`.** Its pin tests are written against the name-only `--pin`. They need the `name@commit` form, and their expected `pins: [...]` line becomes `_pin_lines`'s mapping. - **Two unwritable values are refused in two runs (lane 5's second review, n3).** `plan --elected-by 'A<LF>B' --pin openGlass@<sha> --pin-source <a path holding an ESC>` refuses the elector alone, exit 2. Re-run with the elector retyped, it refuses the source; #270's refusal names every unwritable value at once. Reproduced on 421e4e5, with `gh` called 0 times. One refusal would need a code change, not made here: - `_refuse_an_unrecordable_pin_source` would split into a function that returns the sentence; - `_checked_plan_inputs` would ask it before #270's `unwritable_values` raise, and raise one `adopt-unwritable-value` that names both, with both remediations; - a test would add the two-value row. N2's question is `one_printable_line`, not `unwritable_reason`, so it cannot simply join #270's list. - **Docs.** The README, AGENTS.md and `docs/handbook.html` are unchanged, because none of them calls adopt's `--pin` a name. A sentence saying adopt now takes the scaffold's `--pin` is a follow-up. It costs an AGENTS.md line-cap raise, and a README edit forces the handbook's README blob to change. - **A product spelled `null`, `true` or `false`.** The scaffold already accepts `--pin null@<sha>`: it exits 0 and builds a root whose `validate-pins.py` exits 1 `neutral-pin-entry-malformed`. Adopt's `plan` writes `product: null` through `y`, and `check` then finds `plan-pin-malformed` and refuses it before any leg exists. The one fix for both tools is in `parse_pin`, and it would change the scaffold's behaviour. - **Wording shared with the scaffold.** `digest_source` is written bare into the pin file, so a `--pin-source` path holding ` #` is cut short there. `templates/assembly-root/contracts/neutral-product-pin.yaml` and `validate-pins.py`'s `neutral-pin-missing` remediation name only the scaffold as the writer. Two comments in `scripts/repo_shape.py` still name the scaffold's `pin_digest_from_gh`, in a digest-pinned shipped copy. - **Filed as follow-ups:** - #449: S3, a hand-edited `commit:` or `tree_sha256:` that disagrees, with options (a) and (b); - #450: the scaffold half of N2, a `--pin-source` path no file can hold; - #451: #339's third decision (N5), a `--pin-source` that nothing reads. - **`validate-pins.py` prints a `--pin-source` path raw** in `neutral-pin-source-unreadable`, the twin of S1. It is a shipped template that #409 and other open PRs edit, so it is left for its own change. No issue is filed for it yet. - **Windows and macOS** were not run here; CI ran both (below). Two assertions that depend on what Windows answers for a path (a name past the filesystem's limit, a path beneath a file) check only the refusal and its remediation there. Fix round 1's rows that need a control character or a byte that is not UTF-8 in a path, or a symlink to `git`, are skipped on Windows by a mark, and so is fix round 2's ESC row. The rows for a no-break space and a zero-width joiner run everywhere, and so does the in-process `gh-not-found` test. Reviewed independently by lane 5 at 7a94b6b (3 should-fix, 5 nits: fix round 1) and at 421e4e5 (0 blockers, 2 should-fix, 3 nits: fix round 2). After #409's squash, the re-verification of f9ed46f and 73d65c6 found the code correct; its 1 should-fix and 3 nits were in this description and are fixed. GitHub's update-branch then merged main twice (33bae3f, 2170526), with no hand edit. CI's tests, tests-macos and tests-windows and SonarCloud all passed at 2170526 (main cac6bb2 merged in). On that head merged with main 57c85e9 (#490, #474 and #468, none of which touches this branch's files), the pin and adopt set with `tests/test_update_shape_landing.py` gave 1112 passed, 2 skipped, and the doctor, adopt end-to-end, unfinished-merge and bootstrap set gave 1056 passed, 3 skipped (the doctor's PowerShell line, which exists only on a Windows host). Closes #273 Closes #339 Lane: openreposhape-5 (openRepoShape-5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

chatgpt-codex-connector Bot reviewed Oct 12, 2026

View reviewed changes

Merge origin/main (346079d) into fix/guard-call-forms (#321)

455456d

Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

brettheap merged commit a1b3a4b into main Oct 12, 2026
5 checks passed

brettheap deleted the fix/guard-call-forms branch October 12, 2026 12:39

Conversation

brettheap commented Oct 12, 2026 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Tests

Composes with

Left out

Summary by Sourcery

Uh oh!

sourcery-ai Bot commented Oct 12, 2026

Uh oh!

sourcery-ai Bot commented Oct 12, 2026

Reviewer's Guide

File-Level Changes

Assessment against linked issues

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

brettheap commented Oct 12, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

brettheap commented Oct 12, 2026

Uh oh!

brettheap commented Oct 12, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 12, 2026

Uh oh!

brettheap commented Oct 12, 2026

Uh oh!

brettheap commented Oct 12, 2026

Uh oh!

sonarqubecloud Bot commented Oct 12, 2026

Quality Gate passed

Uh oh!

chatgpt-codex-connector Bot commented Oct 12, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

brettheap commented Oct 12, 2026 •

edited by sourcery-ai Bot

Loading

chatgpt-codex-connector Bot commented Oct 12, 2026 •

edited

Loading