[brettheap]

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

Closes #680.

What changed and why

The bash launcher openRepoShape, run_doctor. Installed, --doctor clones the standard into $TMPDIR/openRepoShape-doctor.XXXXXXXX, and workdir arms an EXIT trap that removes it (openRepoShape:201). The trap is disarmed once the doctor has run (#500). A TERM or HUP sent to the shell alone ended the shell at once. The EXIT trap then removed the clone while the doctor, which nobody had signalled, was still running from it.

• The shell outlives the doctor child (openRepoShape:697-716). INT, TERM and HUP are trapped around the doctor, and each trap only records the signal (caught, a local of run_doctor, :667). bash runs a trap for a foreground command only once that command has ended, so the EXIT trap cannot run while the doctor is alive.
• How the doctor ended decides what is left:
  • A doctor that ran to its end keeps and names its clone, as before.
  • A doctor ended by a signal (a status over 128) leaves nothing (:766, :792-795). This holds whether or not the shell was sent the signal too.
• The shell then ends by the signal it was sent (:783-799, kill -"$caught" "$$"). A calling shell reads 128 + the signal's number in $?.
• An INT counts only where the doctor was ended by one too (:776-782). This is bash's own rule for SIGINT, and what this shell already did before INT was trapped: an INT sent to the shell alone lets the doctor finish, and the shell exits with the doctor's status.
• The comment the issue names (now :729-738) is rewritten, and the RAN TO ITS END wording carries into the next paragraph (:749).

Design (a), and why

The doctor stays in the foreground, and the trap only records the signal. I did not use design (b), which runs the doctor in the background, forwards the signal and calls wait:

• A non-interactive bash starts a background job with SIGINT and SIGQUIT ignored. The doctor would then raise no KeyboardInterrupt on Ctrl-C.
• Every git and gh the doctor runs would inherit that ignored SIGINT. A Ctrl-C reaches the whole foreground process group, and they would ignore it. Forwarding TERM reaches only the doctor, never what it started.
• Under (a), a Ctrl-C or a terminal hang-up reaches the doctor and everything it started, as it always did.
• The cost of (a) is that a TERM to the shell alone waits for the doctor to finish. Before this PR that doctor ran to its end anyway, orphaned and over a deleted standard.

The exit: a death by the signal, which a shell reads as 128 + N

The issue says "Exit 128 + the signal number". Every calling shell reads exactly that in $?, measured below. I did this by re-raising the signal (kill -N $$ after trap -), not with exit $((128 + N)). The reason is measured, not argued:

• A bash loop running a command that traps INT and then does exit 130 goes on to its next turn at a Ctrl-C: after 1 status 130 | after 2 status 0 | after 3 status 0. With the re-raise, the loop stops: outer rc -2.
• bash's rule is that a command which EXITED 130, rather than being ended by SIGINT, handled the Ctrl-C itself.
• Main's launcher dies by the signal in the three "stated behaviour" rows (-2/-15/-1). An exit 130 would have been a regression for every for … do openRepoShape --doctor …; done.

The tests pin the death by the signal (returncode == -N). The mutant that uses exit 128+N fails five of them.

Measured: the issue's table, before (main de92614) and after (af75e37)

These measurements use the installed launcher, one file with nothing beside it, over a stub standard whose shape-doctor.py records its pid and blocks for 10 s. Signals go by recorded PID. The first five rows are the issue's; the last four are added.

Before, main de92614:

signal	sent to	clone 0.6 s later	child 0.6 s later	bash exit	the child at its end	"kept at" line	$TMPDIR after
TERM	bash alone	gone	running	-15	its own file gone	no	empty
HUP	bash alone	gone	running	-1	its own file gone	no	empty
INT	bash and child	gone	ended	-2	killed	no	empty
TERM	bash and child	gone	ended	-15	killed	no	empty
HUP	bash and child	gone	ended	-1	killed	no	empty
INT	bash alone	exists	running	0	its own file present	yes	clone kept
INT	child alone	exists	ended	130	killed	yes	clone kept
TERM	child alone	exists	ended	143	killed	yes	clone kept
HUP	child alone	exists	ended	129	killed	yes	clone kept

After, af75e37:

signal	sent to	clone 0.6 s later	child 0.6 s later	bash exit	the child at its end	"kept at" line	$TMPDIR after
TERM	bash alone	exists	running	-15	its own file present	yes	clone kept
HUP	bash alone	exists	running	-1	its own file present	yes	clone kept
INT	bash and child	gone	ended	-2	killed	no	empty
TERM	bash and child	gone	ended	-15	killed	no	empty
HUP	bash and child	gone	ended	-1	killed	no	empty
INT	bash alone	exists	running	0	its own file present	yes	clone kept
INT	child alone	gone	ended	130	killed	no	empty
TERM	child alone	gone	ended	143	killed	no	empty
HUP	child alone	gone	ended	129	killed	no	empty

What a calling sh -c 'bash openRepoShape --doctor …; echo $?' reads after this change: TERM to the shell and the doctor gives 143, INT gives 130, HUP gives 129; TERM to the shell alone gives 143, HUP gives 129.

Tests

All new tests are in tests/test_shape_doctor.py's installed-doctor section. They are POSIX only (WINDOWS_SKIP), like the tests around them.

• test_a_doctor_ended_by_a_signal_leaves_nothing (:2884) covers INT, TERM and HUP sent to the shell and the doctor, plus TERM sent to the doctor alone. In each case there is no clone in $TMPDIR, no "kept at" line, and the doctor never reached its end. The exit is -N when the shell was signalled, and 128 + N when only the doctor was.
• test_a_signal_to_the_installed_doctors_shell_alone_waits_for_it (:2917) covers TERM, HUP and INT sent to the shell alone:
  • 0.5 s later the shell is still running and the clone's shape-doctor.py is there;
  • once let go, the doctor finds its own file at its end and its line is printed;
  • the clone is kept and named on the last line;
  • the exit is -15, -1, or the doctor's 0 for INT.
• The harness for both:
  • HELD_DOCTOR (:2789) records its pid and its parent's. The test asserts that the parent is the shell it started, and signals only those two PIDs.
  • The stub waits for a release file, at most 60 s. The fixture held_run (:2848) lets it go at teardown, so a failed test leaves nothing running and needs no kill.
  • DEFAULT_SIGNALS (:2809) resets INT, TERM and HUP before exec bash, so a pytest run under nohup or as a background job cannot hand the shell an ignored signal it could neither trap nor reset.
• test_a_plain_repositorys_next_lines_outlive_the_installed_doctor (:2743, L8) runs a git repository with an origin through run_installed:
  • every path a NEXT names under the kept clone exists after the command exits;
  • the adoption line and the machine row's setup-project.py --preflight run as pasted, with no can't open file;
  • the setup.sh --org <your-org> line is checked for its path only.
• Shared helpers. The drifted-project test (:2697) now uses next_lines, named_in_the_clone and run_once_it_has_exited along with the new test, rather than a second copy of them. run_installed takes its environment from installed_env, which the new tests use too.

Mutation checks. Each mutant of the launcher fails at least one new test:

mutant	new tests that fail
main's launcher	3
exit 128+N in place of the re-raise	5
keep the clone whatever the status	4
no rm before the re-raise	3
EXIT trap disarmed before the doctor	1
no INT rule	1
no traps	2
clone never kept (pre-#500)	the plain-repository test

Runs. All runs used a stub gh exiting 97 first on PATH, GH_TOKEN=bogus and an empty GH_CONFIG_DIR, and every pytest command took /usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch> -o tmp_path_retention_policy=failed:

tests	bytes run against	result
tests/test_shape_doctor.py	the final launcher; sha256sum -c OK for both files after the run	413 passed, 3 skipped in 783.57s (0:13:03)
the plain-repository test, re-run after one assertion was added to it (PLAIN_NEXTS_RUN <= ran), with its section via -k "installed or shim or ended_by_a_signal or plain_repositorys"	final	26 passed, 390 deselected in 39.94s
tests/test_openreposhape_command.py tests/test_install_sh.py tests/test_repo_hygiene.py tests/test_cli_reference.py	the final launcher	243 passed in 104.86s (0:01:44)
tests/test_repo_hygiene.py tests/test_cli_reference.py	the final test file	99 passed in 47.90s

Git Bash. tests-git-bash's test_the_doctor_checks_its_clone_out_at_the_ref_in_force reaches run_doctor with a stub doctor and no signal. On that path the traps are set and reset and caught stays empty, so the run is unchanged. It passed here on Linux bash 5.2. It was not run under Git Bash.

Docs

None changed. README.md:1151, AGENTS.md:220 and docs/handbook.html:1568 say that the installed doctor keeps its clone for the NEXT lines. That still holds for every doctor that ran to its end, and none of them states the interrupted case. --help and docs/cli.md are unchanged.

Pinned copies

None touched. The launcher is not a pinned shape copy: it is not in FAMILY_COPIED_VERBATIM and is not under templates/.

Lands after

#656, which is already on main as 35133f2. The branch is cut from de92614. Main has since gained 30676c9 (#634), which touches neither file, and git merge-tree with it is clean.

🤖 Generated with Claude Code

Summary by Sourcery

Correct installed doctor signal handling so the shell waits safely, interrupted runs clean up, and completed runs retain usable clones.

Bug Fixes:

• Ensure the installed doctor’s temporary clone remains available while the doctor is running, even when the launcher shell receives a signal alone.
• Remove the temporary clone when the doctor is terminated by a signal, while preserving and reporting clones from doctors that finish normally.
• Propagate signals by terminating the launcher with the received signal so callers observe the expected signal-based status.

Enhancements:

• Expand installed-doctor coverage for signal handling, clone lifetime, interrupted runs, and executable NEXT report paths.

Tests:

• Add POSIX tests covering signals sent to the launcher, the doctor, or both, including cleanup and exit-status behavior.
• Verify plain repository NEXT commands remain usable after the doctor exits and its clone is kept.

[sourcery-ai]

Reviewer's Guide

The PR fixes installed --doctor lifecycle and signal handling by deferring shell signal handling until the foreground doctor finishes, preserving completed clones while removing clones from signal-terminated doctors, and re-raising shell-directed signals with the expected status semantics. Comprehensive POSIX tests cover cleanup, signal propagation, exit codes, and post-exit NEXT command usability.

File-Level Changes

Change	Details	Files
Changed installed doctor signal handling so the launcher remains alive through the foreground doctor run, preventing cleanup of the clone while the doctor is still using it.	Trap INT, TERM, and HUP around the doctor and defer handling until the foreground command ends. Preserve normal signal propagation to the doctor and its subprocesses. Re-raise a signal received by the shell after cleanup so callers observe signal-based termination semantics.	openRepoShape
Defined distinct cleanup behavior for completed and signal-terminated doctor runs.	Keep and report the clone when the doctor reaches its end, including after a shell-only TERM or HUP. Remove the clone and suppress the kept-clone output when the doctor exits due to INT, TERM, or HUP. Retain the existing SIGINT behavior when only the shell is interrupted and the doctor completes normally.	openRepoShape
Expanded installed-doctor regression coverage for interrupted runs, retained NEXT commands, and process lifecycle behavior.	Add POSIX tests for shell-only and doctor-directed INT, TERM, and HUP, including clone state, output, doctor completion, and exit codes. Use a held stub doctor plus PID recording, signal-default reset, isolated sessions, and teardown release handling to make signal tests deterministic. Verify NEXT paths from plain repositories remain available and executable after the launcher exits, and extract shared helpers for these assertions.	tests/test_shape_doctor.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#680	Ensure signals sent to the launcher shell alone do not remove the standard clone while the doctor is running, while runs where the doctor is terminated by a signal clean up the clone and propagate the expected signal status.	✅
#680	Add POSIX tests that pin the interrupted-run behavior, including shell-and-doctor signals and signals sent to the shell alone.	✅
#680	Test that NEXT commands from an installed doctor run on a plain repository continue to reference usable files in the retained clone.	✅

---

Tips and commands

Interacting with Sourcery

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

Customizing Your Experience

Access your dashboard to:

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

Getting Help

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

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[brettheap]

(lane openRepoShape-3, 2026-10-16T23:08:46Z) Lane 3's independent review of #740 at af75e374 (RV-d, ruling 8): READY. There is no MUST and no SHOULD, only three NITs, none of which blocks. Every measured row matches the PR body's tables. CI on the head is green (tests, SonarCloud, package and tests-git-bash).

Measured: the launcher alone in a directory over a held stub doctor, main against the head. Every signal went to a PID RV-d started.

case	main	head
TERM or HUP to the shell alone	clone gone while the doctor runs	clone kept and named; exit -15 / -1 (fixed)
INT, TERM or HUP to shell and doctor; Ctrl-C to the group	nothing left	same
INT to the shell alone	clone kept, exit 0	same
INT, TERM or HUP to the doctor alone	clone kept	nothing left, same exits (the announced change)
Ctrl-C in a calling loop, or in $(...)	-2	-2
set -e caller, or sh reading $? after TERM	143	143
a second TERM while held	dies at once	waits, then -15 with the clone kept
a doctor that lingers 2 s after TERM	clone gone while it runs	clone removed only when it ends

Re-raising instead of exit 128+N is right. Every calling shell still reads 128 + N, the -N main gave is kept, and a calling loop still stops at Ctrl-C. exit $((128 + caught)) at :798 covers a shell that does not die.

NITs (optional):

• N1, openRepoShape:714-716: PIPE, USR1 or ALRM sent to the shell alone still remove the clone. That is the same on main, and nothing normally sends these to the shell alone. One sentence in the :697 comment would say that only INT, TERM and HUP are held.
• N2, :714-717: a Ctrl-C in the microseconds between the traps and the doctor's start is swallowed (measured with a busy-wait put there). A [ -z "$caught" ] || … check before the doctor line would close the gap.
• N3, :766: a doctor that exits 129-255 on its own is read as signalled. shape-doctor.py exits only 0-3, so this cannot happen in practice.

The harness: it leaks nothing, and nothing was left running. The stub ends itself after 60 s, and teardown waits at most 120 s. RV-d found no load-sensitive assertion and had no flake at load averages of 25-85.

Mutants: 10 of 12 launcher mutants are killed:

• main's launcher;
• exit 128+N;
• keeping the clone at any status;
• no rm before the re-raise;
• no INT rule, or the INT rule at any status;
• no traps, or traps never reset;
• HUP recorded as TERM;
• the EXIT trap disarmed early.

The two survivors change nothing a run can see.

Tests on the committed merge with main 66b230ed (stub gh): test_shape_doctor.py and test_openreposhape_command.py give 533 passed, 3 skipped, 1 xfailed (#735).

Merges:

• Onto main 66b230ed: clean, tree 902d05f9.
• With shape-doctor.py: the placement row asks a leg's top-level single-repository.yaml the staying-single record's question only where kind: makes it the record, so one declaring release-config is asked the general question as plan asks it, and an answered entry names that rule (#691) #734, shape-doctor.py: a conflicted shape pin's NEXT reads both sides' pins, so a copy conflicted beside it that only the side not taken has a files: row for is named in its why and left_for_a_person for a person to merge, and never taken (#687) #736 and shape-doctor.py: an own-copy validator row whose crash is a root's lost scripts/repo_shape.py names git -C <root> checkout HEAD -- scripts/repo_shape.py as its NEXT (none where HEAD no longer holds it) and the validator in its WHY (#697) #737, one by one and all three in turn: clean.
• openRepoShape --doctor: a signal sent to the command's shell alone waits for the doctor, so its clone of the standard is never removed while shape-doctor.py runs from it, and a doctor ended by a signal leaves nothing (#680) #740 and shape-doctor.py and update-shape.py: run from a partial clone of the standard, the shape currency row asks without fetching whether the clone holds what the comparison reads, and is CANNOT ANSWER with a fetch for a person to run where it does not, and a read that fails is refused, never a copy called both (#683) #742 share no file.

Docs and pinned copies: README:1151, AGENTS.md:220 and the handbook's line 1568 stay true, and the launcher is not a pinned copy.

Lane: openreposhape-3 (openRepoShape-3)

[sourcery-ai]

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/test_shape_doctor.py" line_range="2915" />
<code_context>
+@pytest.mark.parametrize(("name", "ends_by_it"), [
+    pytest.param("SIGTERM", True, id="TERM"),
+    pytest.param("SIGHUP", True, id="HUP"),
+    pytest.param("SIGINT", False, id="INT"),
+])
+def test_a_signal_to_the_installed_doctors_shell_alone_waits_for_it(
</code_context>
<issue_to_address>
**Normal exit 130 masks shell SIGINT**

When the doctor exits normally with status 130 after only the shell receives SIGINT, `run_doctor` treats `STATUS == 130` as proof that the doctor was ended by SIGINT and clears `caught`; if the doctor instead exits normally with 130 after only the shell received SIGINT, the launcher returns 130 without re-raising the signal, so a caller’s loop continues and the completed clone is not kept or named.

Do not infer that SIGINT reached the doctor solely from its exit status; preserve the shell’s pending SIGINT when the child may have exited normally with 130.
</issue_to_address>

### Comment 2
<location path="tests/test_shape_doctor.py" line_range="2768" />
<code_context>
+        tools = {path.name for path in named_in_the_clone(line, shape)}
+        named |= tools
+        if placeholder not in line:
+            run_once_it_has_exited(line, tmp_path)
+            ran |= tools
+    assert PLAIN_NEXTS_NAMED <= named, named
</code_context>
<issue_to_address>
**Failed commands count as successful**

When a named NEXT command exits nonzero without printing “can't open file”, `run_once_it_has_exited` checks only for that missing-file text, and the caller ignores its returned process status before adding the tool to `ran`, so the test passes despite the command failing.

Assert the expected return status and result from `run_once_it_has_exited` before adding the tool names to `ran`.

Also at `tests/test_shape_doctor.py:2690-2691`.
</issue_to_address>

Sourcery assessment

Needs a human reviewer. 2 findings to address first, and if the signal handling is wrong, the doctor could lose its cloned standard while still using it, or a temporary clone could be left behind after an interrupted run. That clone is bounded and can be removed manually; reverting restores the previous signal behavior but does not clean up a clone already left behind.

Blocking findings: tests/test_shape_doctor.py:2915, tests/test_shape_doctor.py:2768

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

[sourcery-ai]

🟡 Medium · Normal exit 130 masks shell SIGINT

When the doctor exits normally with status 130 after only the shell receives SIGINT, run_doctor treats STATUS == 130 as proof that the doctor was ended by SIGINT and clears caught; if the doctor instead exits normally with 130 after only the shell received SIGINT, the launcher returns 130 without re-raising the signal, so a caller’s loop continues and the completed clone is not kept or named.

Do not infer that SIGINT reached the doctor solely from its exit status; preserve the shell’s pending SIGINT when the child may have exited normally with 130.

Prompt for AI agents

In `tests/test_shape_doctor.py` at line 2915:

**Normal exit 130 masks shell SIGINT**

When the doctor exits normally with status 130 after only the shell receives SIGINT, `run_doctor` treats `STATUS == 130` as proof that the doctor was ended by SIGINT and clears `caught`; if the doctor instead exits normally with 130 after only the shell received SIGINT, the launcher returns 130 without re-raising the signal, so a caller’s loop continues and the completed clone is not kept or named.

Do not infer that SIGINT reached the doctor solely from its exit status; preserve the shell’s pending SIGINT when the child may have exited normally with 130.

[sourcery-ai]

🟡 Medium · Failed commands count as successful

When a named NEXT command exits nonzero without printing “can't open file”, run_once_it_has_exited checks only for that missing-file text, and the caller ignores its returned process status before adding the tool to ran, so the test passes despite the command failing.

Assert the expected return status and result from run_once_it_has_exited before adding the tool names to ran.

Also at tests/test_shape_doctor.py:2690-2691.

Prompt for AI agents

In `tests/test_shape_doctor.py` at line 2768:

**Failed commands count as successful**

When a named NEXT command exits nonzero without printing “can't open file”, `run_once_it_has_exited` checks only for that missing-file text, and the caller ignores its returned process status before adding the tool to `ran`, so the test passes despite the command failing.

Assert the expected return status and result from `run_once_it_has_exited` before adding the tool names to `ran`.

Also at `tests/test_shape_doctor.py:2690-2691`.