Skip to content

tests/test_value_pattern_line_break.py: ID_READERS gives adopt-project.py's argv --id the reason that holds (match, then validate-manifest.py refuses a line feed), a row proves it, and the guard says a new reader needs a row, fullmatch too - #1243

Draft
brettheap wants to merge 1 commit into
fix/1139-anchored-patternsfrom
fix/1229-id-readers-reasons

Conversation

@brettheap

@brettheap brettheap commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Goal

tests/test_value_pattern_line_break.py says only what is true of the readers of PROJECT_ID_RE: no row of ID_READERS gives a reason that does not hold for the tool it names, and the guard test_every_reader_of_the_id_pattern_is_in_the_table tells the author of a new reader, in its docstrings and in its failure message, that the one way to pass is a row in ID_READERS, whichever method the reader calls.

This is #1229, the follow-up of #1166 (#1139): rc1166b's NIT 1, NIT 2 and NIT 4 (#1166 issuecomment-6371408915). Wording, one assert message and one row, in the test file only. No production file changes.

Stacked on #1166 at 5102b42 (branch fix/1139-anchored-patterns, full sha 5102b42945baf3682bc51244a64050e654d25e32, read again at the push). The test file is not on main, so this branch is built on #1166's head, and its one commit is f92338ef6. Until #1166 lands, this PR's diff against main also carries #1166's eight files. Against #1166's head it is one file, 56 lines added and 16 removed. After #1166 lands, restack with three merges, never a rebase: #1166's final head, then its squash commit on main (keeping THIS branch's side, ours, for tests/test_value_pattern_line_break.py, the one add/add conflict: the squash's side would drop every #1229 edit without a marker), then main.

What held at #1166's head (5102b4294)

  1. A false reason (rc1166b NIT 2). The module docstring's "LEFT AS THEY ARE" bullet and the ID_READERS rows said every argv --id of scaffold-project.py, family.py init and adopt-project.py "holds the id to checked_value after the pattern". That holds for the first two, and tests/test_derived_id_refusals.py's TOOLS pins it for those two only. adopt-project.py never calls checked_value on --id: _refuse_bad_id asks PROJECT_ID_RE.match(project_id) of the id plan takes, passed or derived, and args.id reaches no checked_value call (git grep -n 'args\.id\b' -- adopt-project.py; its calls are for --project, the tracking branch, --pin-owner, the leg paths and --org). A line feed is still refused, by another door, the project's own validate-manifest.py.
  2. "fullmatch is enough" (rc1166b NIT 1). Both docstrings said a new reader "asks with fullmatch, or it is listed in ID_READERS". The guard is assert id_pattern_calls() == expected, a dict equality over every (module, method), exact both ways, so a new fullmatch reader fails it too, and so does a listed reader taken away. The assert had no message, so the author of a sibling PR saw only a dict diff.
  3. The guard reads spellings (rc1166b NIT 4, optional). _ID = PROJECT_ID_RE then _ID.match(value) passes it, and so does from repo_shape import PROJECT_ID_RE as X. Nothing in validate-manifest.py, validate-family.py, family.py and update-shape.py: an id:, a date or a repository: ending in a line feed is refused by name, and a pin's source_repository: that is no path or owner/repo name is named as the pin's (#1139) #1166 says that is false, and nothing says it.

What changed

All in tests/test_value_pattern_line_break.py.

  • Item 1. The module docstring's bullet names scaffold-project.py and family.py init only, with TOOLS as the proof, and a new bullet says adopt-project.py's argv --id is NOT held to checked_value: plan asks it with match and calls checked_value on --project and never on the id, so northwind\n passes the pattern, and the project's own validate-manifest.py refuses it as adopt-manifest-invalid naming --id, in manifest-id's words, exit 2, no plan written (the row below, "measured for plan only"). The adopt-project.py row of ID_READERS begins with that reason in place of "argv --id, as above". The scripts/family.py and scaffold-project.py rows are as they were.
  • Item 1's optional row. test_adopt_plan_names_a_passed_id_that_ends_in_a_line_break_as_an_id runs adopt-project.py plan --id with a trailing line feed (conftest's write_plan over its source_repo, one plan run of about a second) and asserts exit 2, the first stderr line REFUSED adopt-manifest-invalid: --id is 'northwind\n', which the project's own validate-manifest.py refuses: manifest-id: id is 'northwind\n'; it must match ^[a-z0-9][a-z0-9-]*$., and that no plan file was written. It also imports write_plan from conftest. --id and --id=... gave the same lines when measured.
  • Item 2. Both docstrings drop "it asks with fullmatch, or": a new reader, fullmatch included, is listed in ID_READERS with the reason a line feed cannot reach it, or the test fails; the table is exact both ways, so a reader taken away loses its row. The comment above ID_READERS says a reader added later needs its row whichever method it calls.
  • Item 3. The assert carries a message: "ID_READERS lists every call of a method of PROJECT_ID_RE outside tests/, fullmatch included, by module and method. Add, change or drop the row of the module and method the diff below shows, and give a match or search reader the reason a line feed cannot reach it." Pytest prints it above the dict diff.
  • Item 4 (aliases). Left as it is, and said: both docstrings say the guard reads spellings (a call whose receiver is the name PROJECT_ID_RE or an attribute of that name) and stops there, with the two aliases named as outside it. S1 to S3 below measure exactly that.

git grep -n 'checked_value' -- tests/test_value_pattern_line_break.py now shows the checked_value reason only against scaffold-project.py and scripts/family.py (lines 43, 171, 173); the module docstring's adopt-project.py bullet and the adopt-project.py row name validate-manifest.py and adopt-manifest-invalid for it, and say adopt-project.py is not held to checked_value.

Rows

The new row fails on main (50869f384). The test file is #1166's, so it was copied from this head onto a detached worktree at main, and only the new row was run (git 2.43.0): it fails, because on main the manifest validator's id pattern still takes northwind and a line feed ($ matches before the line feed), and the line feed is refused only through the topic:

E  AssertionError: REFUSED adopt-manifest-invalid: --id is 'northwind\n', which the project's own validate-manifest.py refuses: manifest-topic: topic is 'xf-project-northwind', expected 'xf-project-northwind\n' derived from id 'northwind\n'.
E  -  manifest-id: id is 'northwind\n'; it must match ^[a-z0-9][a-z0-9-]*$.
E  +  manifest-topic: topic is 'xf-project-northwind', expected 'xf-project-northwind\n' derived from id 'northwind\n'.

So the row holds the claim that manifest-id is the refusal, which is #1166's own change to validate-manifest.py, and it passes at #1166's head plus this commit (31 passed below). That is why this PR is stacked.

Mutants and plants

Each applied alone to a scratch worktree (detached at f92338ef6), run, restored by writing the original bytes back, and the sha256 compared (restored=True each time, git status clean after each). Production files only; the test file is not touched by a plant. Node ids: the guard is test_every_reader_of_the_id_pattern_is_in_the_table, the row is the new one above. Baseline before any plant: both pass (2 passed).

plant file what result
G1 update-shape.py def _rc_probe(value): return PROJECT_ID_RE.fullmatch(value) appended guard fails, its message names ID_READERS; Left contains 1 more item: {('update-shape.py', 'fullmatch'): 1}
G2 update-shape.py the same with .match guard fails, message names ID_READERS
G3 update-shape.py the same with .search guard fails, message names ID_READERS
G4 adopt-project.py if False: in place of the plan-id: test at line 6791 guard fails, message names ID_READERS; {('adopt-project.py', 'match'): 2} != {('adopt-project.py', 'match'): 3}
R1 adopt-project.py fullmatch in place of match at line 5140 (_refuse_bad_id) guard fails (names ID_READERS) and the row fails: REFUSED adopt-bad-id: --id 'northwind\n' must match ^[a-z0-9][a-z0-9-]*$
R2 adopt-project.py checked_value('--id', project_id) added before line 5140 row fails: REFUSED unsafe-value: --id is 'northwind\n', which is not a value this tool will put on a git or gh command line
R3 adopt-project.py if False: in place of if values is not None: in _refuse_unholdable_values, so no manifest check row fails: the plan is written, exit 0
S1 update-shape.py _ID = PROJECT_ID_RE then _ID.match(value) guard PASSES (1 passed): an alias is outside it, as the docstrings now say
S2 update-shape.py from repo_shape import PROJECT_ID_RE as X then X.match(value) guard PASSES (1 passed)
S3 update-shape.py PROJECT_ID_RE.match(value) in a comment and in a string guard PASSES (1 passed): not counted, as intended

No mutant survives: G1 to G4 and R1 to R3 each fail where they should. R2 shows the new row also fails if adopt-project.py ever does hold the id to checked_value, which makes the module docstring's sentence false, so whoever adds that call is sent to the wording.

Runs

All through the lane's runner run9.sh (stub gh first on PATH, GIT_SSH_COMMAND=false, GIT_CONFIG_GLOBAL=/dev/null, EMAIL exported), on the committed head f92338ef6 with dirty=0, Python 3.12.3, git --version printed into each log.

Merge-trees

Main 50869f384 (re-fetched just before the push), this head f92338ef6:

Of the 51 open PRs (gh pr list --json number,headRefName,headRefOid,files), only #1166 touches tests/test_value_pattern_line_break.py. Not checked: whether a PR that touches adopt-project.py (26 do) adds or removes a call of a method of PROJECT_ID_RE; that is the guard's own job once #1166 lands, and #1166's body found none among the open PRs. A PR that makes plan hold --id to checked_value would fail the new row (R2), which is the row doing its work.

Closes #1229

Lane: openreposhape-5 (openRepoShape-5)

🤖 Generated with Claude Code

Summary by Sourcery

Make the line-break reader tests accurately account for adopt-project.py's argv ID validation and require complete, justified coverage for all PROJECT_ID_RE readers.

Bug Fixes:

  • Correct the documented and tested handling of trailing line feeds in adopt-project.py's argv --id path, showing that validation is performed by validate-manifest.py rather than checked_value.

Enhancements:

  • Require every direct PROJECT_ID_RE reader, including fullmatch readers, to have an exact ID_READERS entry with an accurate safety rationale.
  • Improve the reader-table assertion failure message and document its intentional spelling-based alias limitations.

Documentation:

  • Update the test documentation and table rationale to accurately describe ID validation behavior across the affected tools.

Tests:

  • Add coverage proving adopt-project.py plan rejects a passed ID ending in a line feed without writing a plan.

…ject.py`'s argv `--id` the reason that holds (`match`, then validate-manifest.py refuses a line feed), a row proves it, and the guard says a new reader needs a row, `fullmatch` too

#1166's table and module docstring said every argv `--id` of the three
tools "holds the id to `checked_value`". It holds for `scaffold-project.py`
and `family.py init` (`tests/test_derived_id_refusals.py`'s `TOOLS` are
those two), and not for `adopt-project.py`: `plan` asks the id with
`match` (`PROJECT_ID_RE.match(project_id)` in `_refuse_bad_id`), which takes
`northwind` and a line feed, and calls `checked_value` on `--project` only.
The line feed is still refused, by another door: the project's own
validate-manifest.py, as `adopt-manifest-invalid` naming `--id`.

The module docstring's bullet and the `adopt-project.py` row of `ID_READERS`
say so now, and one new row runs `adopt-project.py plan --id` with a
trailing line feed and asserts exit 2, that refusal in its first line, and
no plan written. It is measured for `plan` only.

The docstrings also said a new reader that asks with `fullmatch` passes the
guard. It does not: the guard is a dict equality over every (module, method),
so any reader added needs a row and a reader taken away loses its row. Both
docstrings say that, and the assert has a message that names `ID_READERS`
and says to add, change or drop the row the diff shows. The docstrings also
say the guard reads spellings: an alias of the pattern is outside it.

No production file changes.

Closes #1229

Lane: openreposhape-5 (openRepoShape-5)

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

sourcery-ai Bot commented Oct 24, 2026

Copy link
Copy Markdown

Reviewer's Guide

Updates the line-break pattern tests so each PROJECT_ID_RE reader has an accurate, method-independent safety rationale, specifically covering adopt-project.py's manifest-validation path, and makes the reader table an exact, actionable guard with regression coverage.

File-Level Changes

Change Details Files
Correct the documented safety rationale for adopt-project.py's argv --id reader and add an executable regression test for its actual rejection path.
  • Replace the incorrect checked_value explanation with the match plus validate-manifest.py behavior.
  • Add a test proving a trailing line feed is rejected as adopt-manifest-invalid and no plan is written.
  • Import and use write_plan to exercise the CLI path.
tests/test_value_pattern_line_break.py
Make the PROJECT_ID_RE reader inventory an exact, actionable contract for all matching methods.
  • Require rows for new match, search, and fullmatch readers, while also detecting removed readers.
  • Document the guard's spelling-based scope and alias limitations.
  • Add an assertion failure message directing maintainers to update the corresponding table row and rationale.
tests/test_value_pattern_line_break.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

@sonarqubecloud

Copy link
Copy Markdown

@brettheap

Copy link
Copy Markdown
Contributor Author

Independent review of #1243 (#1229) for lane openRepoShape-5, by rv1243 (sonnet), at f92338e: READY

The review is complete except one cell. I did not re-measure the hygiene five on git 2.47.1, because LAST STEP arrived first. Those five did pass on git 2.43 (337 passed). The writer's own log ($Q/v131/n1229/gate-five-git247.log, same head, dirty=0) says 337 passed on 2.47.1. I read that log and did not run it.

$Q is /tmp/claude-1000/-workspace-projects-openRepoShape/50cede29-ab82-4ca3-9f67-d98e1a58c8d2/scratchpad. My logs are in $Q/v131/rv1243/. The CHECKPOINT and DONE blocks are in $Q/v131/rv1243/STATE.md.

Refs at the start. git ls-remote read back fix/1229-id-readers-reasons = f92338ef6, fix/1139-anchored-patterns = 5102b4294, and main = 50869f384. gh pr view shows #1243 as a DRAFT with base fix/1139-anchored-patterns. git diff 5102b4294 f92338ef6 touches only tests/test_value_pattern_line_break.py (+56/-16).

Run discipline. For about a minute I had three of my runs live at once (the five, the plant driver and a small probe), one over the limit of two. The probe was a single test of about 40 s and I let it finish. Nothing else was affected.

What I checked

1. Every sentence the PR changes is true.

  • --id and checked_value.
    • adopt-project.py has one --id parser, plan.add_argument("--id" at :17574, so "argv --id" means plan's.
    • Every checked_value( call in the file, and every args.id use, shows none that takes the id: --project (:5401), the tracking branch, --pin-owner, the leg paths, --org and the plan keys.
    • _refuse_bad_id asks PROJECT_ID_RE.match(project_id) (:5140).
    • checked_value("--id", ...) exists at scaffold-project.py:2517 and scripts/family.py:2626, and tests/test_derived_id_refusals.py's TOOLS is those two tools only.
  • My own plan probe, through run9.sh ($Q/v131/rv1243/probe/probe.out):
    • --id followed by northwind<LF>, and --id=northwind<LF>: both exit 2 with no plan written, and both give the same first stderr line: REFUSED adopt-manifest-invalid: --id is 'northwind\n', which the project's own validate-manifest.py refuses: manifest-id: id is 'northwind\n'; it must match ^[a-z0-9][a-z0-9-]*$.
    • Control --id northwind: exit 0 and the plan is written.
    • --project 'Northwind<LF>', with or without --id: REFUSED unsafe-value: --project is 'Northwind\n'.... So a derived id cannot carry a line feed, which backs "calls checked_value on --project".
    • northwind\r\n is adopt-bad-id, and north wind is adopt-bad-id.
    • On scripts/repo_shape.py, PROJECT_ID_RE.match("northwind\n") is true and fullmatch is false.
  • The table is exact in both directions (plant logs in $Q/v131/rv1243/plants/):
    • Reader added: G1, G2, G3, G3b and G6.
    • Reader removed: G5, where the Right contains 1 more item message shows the validator's fullmatch row.
    • Count changed: G4, {('adopt-project.py','match'): 2} != {...: 3}.
  • Issue Verify item 1: git grep -n checked_value on the file shows the reason against scaffold-project.py and scripts/family.py at :43, :171 and :173. The adopt-project.py bullet and row (:46-48, :176) name validate-manifest.py and adopt-manifest-invalid, as the issue asks.

2. The issue's Verify plants. Each ran against the guard plus the new row, via run9.sh, in the scratch worktree rv1243-plant (detached at f92338ef6). Each was restored by writing the original bytes back. The sha256 matched, restored=True each time, and git status --porcelain was empty afterwards.

plant what guard result
BASE none 2 passed
G1 PROJECT_ID_RE.fullmatch appended to update-shape.py fails; Left contains 1 more item: {('update-shape.py','fullmatch'): 1}
G2 the same with match fails
G3 the same with search fails
G3b repo_shape.PROJECT_ID_RE.fullmatch fails (the attribute form is counted)
G4 if False: at adopt-project.py:6791 fails
G5 validator's fullmatch reader removed fails
G6 search appended to scaffold-project.py fails
  • Every guard failure has an E AssertionError: \ID_READERS` lists every call of a method of `PROJECT_ID_RE`...line. I counted thoseElines specifically, because the traceback also prints the docstring, which namesID_READERS` anyway.
  • Row mutants:
    • R1 (fullmatch at :5140): both the guard and the row fail; the row gives adopt-bad-id.
    • R2 (checked_value('--id', ...) added): the row fails with unsafe-value.
    • R3 (if values is not None: replaced by if False:): the row fails with 0 == 2.
    • No mutant survives.

3. The new row.

4. Test runs (through run9.sh, EMAIL=service@opensoft.dev, head f92338ef6, dirty=0):

run result log
file, git 2.43.0 31 passed in 81.18s file-git243.log
file, git 2.47.1 31 passed in 79.57s file-git247.log
file plus hygiene five, git 2.43.0 337 passed in 405.94s five-git243.log
file plus hygiene five, git 2.47.1 NOT RUN writer's log reads 337 passed

The slow times were load from other lanes. The writer's runs took 26 s and 111 s unloaded.

5. Hygiene of the diff.

  • git diff --check exits 0.
  • No non-ASCII line in the whole file, and no line over 79 columns, in the file or in the added lines.
  • No tab or trailing whitespace in the added lines.
  • An ast scan of all 26 asserts, messages included, found no BoolOp, so none joins conditions with and or or.

6. Merge-trees (merge-trees.log; the base of main and #1166 is 038dafba9):

Findings

MUST: none.

SHOULD (PR body, not the code; the coordinator edits bodies)

NIT

  1. The body's "Of the 51 open PRs" is now 52. The conclusion still holds: only validate-manifest.py, validate-family.py, family.py and update-shape.py: an id:, a date or a repository: ending in a line feed is refused by name, and a pin's source_repository: that is no path or owner/repo name is named as the pin's (#1139) #1166 and tests/test_value_pattern_line_break.py: ID_READERS gives adopt-project.py's argv --id the reason that holds (match, then validate-manifest.py refuses a line feed), a row proves it, and the guard says a new reader needs a row, fullmatch too #1243 touch the file.
  2. The docstring says "measured for plan only". plan is the only subcommand with --id (adopt-project.py:17574), so the hedge could be dropped or made firmer. It is harmless as written.
  3. closingIssuesReferences for tests/test_value_pattern_line_break.py: ID_READERS gives adopt-project.py's argv --id the reason that holds (match, then validate-manifest.py refuses a line feed), a row proves it, and the guard says a new reader needs a row, fullmatch too #1243 is empty today, because the base is fix/1139-anchored-patterns and not main. The Closes #1229 line will count only after the restack onto main. This is expected for a stacked draft.
  4. id_pattern_calls' own docstring is unchanged. It names the two spellings it reads, which is true: G3b confirmed the attribute form is counted. The new text in the guard docstring and module docstring says aliases are outside the guard. I did not run the writer's alias plants S1 to S3, so those are unmeasured by me.

Lane: openreposhape-5 (openRepoShape-5)

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