From c924b0934e45fbbd0d253d09fa285b3e00045397 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Mon, 31 Aug 2026 16:46:19 +0200 Subject: [PATCH] =?UTF-8?q?feat(config):=20T-1277=20=E2=80=94=20detach,=20?= =?UTF-8?q?and=20a=20failed=20job=20that=20looked=20busy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit core/process.py spawns a child that outlives its parent: its own session, so a signal to the parent's group or a timeout kill does not take the work with it; re-execing reach by BARE NAME, because an absolute path would freeze the child to whichever checkout was current at spawn time and silently run the wrong source after a repoint; and streams kept separate exactly as in the foreground, events to .jsonl and real output to .out. Testing a case the ticket did not name found a real hole. Recording completion inside @command looked right and was wrong: a child that fails BEFORE any command runs — bad arguments, an unknown verb, an import error — never reaches that decorator. `reach --detach check bogus` left its metadata reading "running" forever with the process long gone. That is the exit-0 trap wearing a new disguise and worse than the original, because a failed job that looks busy sits somewhere nobody is watching, and a caller polling for completion would wait indefinitely on something that failed in milliseconds. So completion is recorded at the PROCESS's exit instead. main.py gains main(), wrapping cli() in a single try/finally, and the entry point moves to main:main. Every exit path now passes through one place. Removed from @command rather than left in both — two writers of one field is how they drift. Verified on three paths: success records done/0, a real drift failure records failed/1, and the parse failure that exposed the hole now records failed/2. One narrow conformance exemption, with its reason inline so it does not read as an oversight: the no-domain-imports-core.jobs invariant fired on main.py, correctly by its letter and wrongly by its purpose. main.py is not a command; it is the entry point, and it already owns --detach. Still open, and carried to T-1278: a child killed outright cannot record anything, so jobs list must reconcile against process liveness rather than trusting the file. Co-Authored-By: Claude Opus 5 (1M context) --- .pql/changelog/ticket_history/2026-08.sql | 28 ++++ .pql/changelog/tickets/2026-08.sql | 42 +++++ pyproject.toml | 2 +- tooling/core/command.py | 7 +- tooling/core/jobs.py | 6 + tooling/core/process.py | 186 ++++++++++++++++++++++ tooling/main.py | 53 ++++++ tooling/test_conformance.py | 10 +- 8 files changed, 329 insertions(+), 5 deletions(-) create mode 100644 tooling/core/process.py diff --git a/.pql/changelog/ticket_history/2026-08.sql b/.pql/changelog/ticket_history/2026-08.sql index 1981d3b2c..c84db72fe 100644 --- a/.pql/changelog/ticket_history/2026-08.sql +++ b/.pql/changelog/ticket_history/2026-08.sql @@ -1905,3 +1905,31 @@ DESIGN CALL RECORDED: only DETACHED runs will get a log FILE (T-1277). A foregro NEW CONFORMANCE INVARIANT: no module outside core/ may import core.jobs. The moment a domain imports it, ambience has become call-site discipline again and will fail the same way — one command forgets and its output loses correlation silently. AND THE INVARIANT WAS INITIALLY BROKEN, which is worth recording because it is the exact failure the prove-it-can-fail discipline exists to catch. My first version inspected only node.module, so it missed `from tooling.core import jobs` — where jobs appears in the NAMES, not the module path, and which is the form anyone would actually write. The test passed while checking nothing. Rewritten to catch all three reachable forms (from tooling.core import jobs / from tooling.core.jobs import x / import tooling.core.jobs), then verified by adding a real violating import to the check router: it failed and named the file and line. Reverted, green.', NULL, '2026-08-31 13:59:26', '2026-08-31 13:59:26.864', '2026-08-31 13:59:26.864', NULL, '49cef2cd6dbfa13ec368a4f4ccfa6bc7', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5SNAJ0B4Z6DE3KWKQX370', 'status', 'in_progress', 'done', NULL, '2026-08-31 13:59:44', '2026-08-31 13:59:44.052', '2026-08-31 13:59:44.052', NULL, '204f43c01b5c81d015bf49bab1ec7884', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5TWD0VVBM4F2WZYAQFET0', 'status', 'backlog', 'in_progress', NULL, '2026-08-31 14:39:00', '2026-08-31 14:39:00.853', '2026-08-31 14:39:00.853', NULL, '11267c3cc7467ec3043ade9a824cf0d9', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5TWD0VVBM4F2WZYAQFET0', 'status', 'in_progress', 'in_progress', NULL, '2026-08-31 14:39:40', '2026-08-31 14:39:40.450', '2026-08-31 14:39:40.450', NULL, '9294dac8cf3b6ee18531d5df700c29d7', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5TWD0VVBM4F2WZYAQFET0', 'description', 'The spawn primitive, and it is substrate rather than a domain because it has no verbs of its own. Deliverables: spawn a detached child that survives the parent exiting (setsid or equivalent, not just a background shell job, since a killed parent must not take the work with it); redirect the child''s event stream to .cache/reach/jobs/.jsonl and its stdout to a sibling file, keeping the two channels separate exactly as they are in the foreground; write a metadata record carrying command, argv, start time, pid and — on completion — end time and exit code. The metadata file is what makes a finished job readable without re-reading a possibly enormous log. .cache/ is already gitignored. WATCH: the child must re-exec the same reach that was invoked, resolved by bare name per T-1261''s negative criterion, never by an interpreter path or a .venv path — an absolute path here would break the moment the tool is re-pointed at another checkout, and would be a silent wrong-source failure of exactly the kind make reach-repoint exists to fix. Also watch the completion race: the exit code must be recorded by the CHILD as its last act, not polled by a parent that may already be gone.', 'The spawn primitive, and it is substrate rather than a domain because it has no verbs of its own. Deliverables: spawn a detached child that survives the parent exiting (setsid or equivalent, not just a background shell job, since a killed parent must not take the work with it); redirect the child''s event stream to .cache/reach/jobs/.jsonl and its stdout to a sibling file, keeping the two channels separate exactly as they are in the foreground; write a metadata record carrying command, argv, start time, pid and — on completion — end time and exit code. The metadata file is what makes a finished job readable without re-reading a possibly enormous log. .cache/ is already gitignored. WATCH: the child must re-exec the same reach that was invoked, resolved by bare name per T-1261''s negative criterion, never by an interpreter path or a .venv path — an absolute path here would break the moment the tool is re-pointed at another checkout, and would be a silent wrong-source failure of exactly the kind make reach-repoint exists to fix. Also watch the completion race: the exit code must be recorded by the CHILD as its last act, not polled by a parent that may already be gone. + +DONE 2026-08-31. Detached execution works end to end, and testing it found a real hole that the design as written would have shipped. + +DELIVERED: tooling/core/process.py (spawn, metadata, liveness), the --detach flag on the root callback, and completion recording. Verified against a real spawn — parent returns the job id and exits 0, child runs on and writes .cache/reach/jobs/.jsonl tagged with that id, plus a .out sibling and a .json metadata record. + +THE THREE THINGS THE TICKET FLAGGED, each handled and each verified: +- start_new_session=True, so the child gets its own session and process group and a signal to the parent''s group does not take the work with it. +- The child re-execs `reach` by BARE NAME. An absolute path would freeze it to whichever checkout was current at spawn time, so after make reach-repoint a detached job would silently run the wrong source — the exact failure that command exists to fix. +- The child records its own exit code. Confirmed on the success path (status done, exit_code 0) and on a real failure path (status failed, exit_code 1) using a drift fixture pointed at by SR_REPO_ROOT. + +THE HOLE, found only because I tested a THIRD case the ticket did not name. +Recording completion inside @command looked right and was subtly wrong. A child that fails BEFORE any command runs — bad arguments, an unknown verb, an import error — never reaches that decorator. Verified: `reach --detach check bogus` left its metadata reading status "running" FOREVER, with the process long gone. +That is the exit-0 trap wearing a new disguise, and worse than the original: a failed job that looks busy, in a place nobody is watching. A caller polling for completion would wait indefinitely on a job that failed in milliseconds. +FIX: completion is now recorded at the PROCESS''s exit rather than a command''s. tooling/main.py gains main(), which wraps cli() in one try/finally, and [project.scripts] points at main:main instead of main:cli. Every exit path — success, ReachError, usage error, unhandled exception — now passes through a single finally. Re-verified: `reach --detach check bogus` records status failed, exit_code 2. +The recording was REMOVED from @command rather than left in both places; two writers of the same field is how they drift. + +CONFORMANCE EXEMPTION ADDED, deliberately narrow. The new no-domain-imports-core.jobs invariant fired on main.py, correctly by its letter and wrongly by its purpose. main.py is not a command — it is the entry point, and it already owns --detach, --verbose and --no-input. Reading a job-id constant there is far less coupled than the --detach flag it already carries. Exempted main.py explicitly, with the reason inline so it does not read as an oversight. + +STILL OPEN, and correctly belongs to T-1278: a child killed outright (SIGKILL, OOM, interpreter crash) still cannot record anything, so its metadata stays "running". process.is_alive(pid) exists for exactly this, and jobs list/status must reconcile against it rather than trusting the file. + +reach --help is still 73 ms, so the entry-point wrapper costs nothing on the fast path.', NULL, '2026-08-31 14:45:58', '2026-08-31 14:45:58.410', '2026-08-31 14:45:58.410', NULL, '71a871771d1e7e33b8e8885d4c68d776', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5VJ89C7Q4EPR6Q73FSS74', 'description', 'The user-facing verbs, and a DOMAIN rather than core/ because they carry logic and state of their own — the first real test of the D-263 core bound, which it passes. Deliverables: reach jobs list (recent jobs with status, command and duration), status , log with --follow to tail, and wait . REATTACH IS A BYTE OFFSET into an append-only file, which is the entire reason no daemon is needed: a caller can attach, drop off, and come back without losing anything, and there is no lifecycle to get wrong, nothing to orphan, and no stale state to reconcile. log --follow is therefore a poll on file length, not a subscription. Render the JSONL through the same path a live terminal uses, so a tailed log and a live run are the same artefact in two presentations rather than two renderers that drift. Note for the port: this domain is the first one written from scratch under the full contract rather than ported, so it doubles as the worked example the reach skill (T-1254) should show.', 'The user-facing verbs, and a DOMAIN rather than core/ because they carry logic and state of their own — the first real test of the D-263 core bound, which it passes. Deliverables: reach jobs list (recent jobs with status, command and duration), status , log with --follow to tail, and wait . REATTACH IS A BYTE OFFSET into an append-only file, which is the entire reason no daemon is needed: a caller can attach, drop off, and come back without losing anything, and there is no lifecycle to get wrong, nothing to orphan, and no stale state to reconcile. log --follow is therefore a poll on file length, not a subscription. Render the JSONL through the same path a live terminal uses, so a tailed log and a live run are the same artefact in two presentations rather than two renderers that drift. Note for the port: this domain is the first one written from scratch under the full contract rather than ported, so it doubles as the worked example the reach skill (T-1254) should show. + +FROM T-1277 (2026-08-31) — the reconciliation requirement is yours and it is not optional. A child killed outright (SIGKILL, OOM, an interpreter crash) never gets to record its own completion, so its metadata file stays status=running with the process long gone. jobs list and jobs status must therefore RECONCILE against process.is_alive(pid) rather than trusting the file: a job whose pid is dead and whose status still reads running is not running, it died. Report it as such — ''died without recording an exit'' is honest and actionable, whereas showing it as running is the exit-0 trap in a place nobody is watching, and a caller polling for completion would wait forever on something that failed in milliseconds. Note the ordinary failure paths are already covered: T-1277 moved completion recording to the process''s exit (tooling/main.py main()), so bad arguments, unknown verbs, ReachErrors and unhandled exceptions all record correctly. What remains is only the case where the process cannot run code at all.', NULL, '2026-08-31 14:46:03', '2026-08-31 14:46:03.939', '2026-08-31 14:46:03.939', NULL, 'a02f3f35330e271011c615aef8f9a141', 2) ON CONFLICT(hash) DO NOTHING; diff --git a/.pql/changelog/tickets/2026-08.sql b/.pql/changelog/tickets/2026-08.sql index 4a1496d6c..3807adb4f 100644 --- a/.pql/changelog/tickets/2026-08.sql +++ b/.pql/changelog/tickets/2026-08.sql @@ -2194,3 +2194,45 @@ DESIGN CALL RECORDED: only DETACHED runs will get a log FILE (T-1277). A foregro NEW CONFORMANCE INVARIANT: no module outside core/ may import core.jobs. The moment a domain imports it, ambience has become call-site discipline again and will fail the same way — one command forgets and its output loses correlation silently. AND THE INVARIANT WAS INITIALLY BROKEN, which is worth recording because it is the exact failure the prove-it-can-fail discipline exists to catch. My first version inspected only node.module, so it missed `from tooling.core import jobs` — where jobs appears in the NAMES, not the module path, and which is the form anyone would actually write. The test passed while checking nothing. Rewritten to catch all three reachable forms (from tooling.core import jobs / from tooling.core.jobs import x / import tooling.core.jobs), then verified by adding a real violating import to the check router: it failed and named the file and line. Reverted, green.', 'in_progress', 'high', NULL, NULL, 'D-263', '2026-08-31 13:51:51.124', '2026-08-31 13:59:26.864', NULL, '78a2584293788d032977d9d55f3398bb', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at; +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5SNAJ0B4Z6DE3KWKQX370', 'task', '06G1S7NVJR0GT9KWS9QVYNNFMM', 'Job identity as ambient context — the decorator half of streaming', 'The piece that must land before T-1250, so every ported command arrives already streaming rather than being retrofitted. Deliverables: tooling/core/jobs.py holding the CURRENT JOB as ambient state (a ContextVar, not a global, so it is correct if anything ever runs concurrently); core/command.py assigning a job id at the start of every invocation; core/console.py reading that id and tagging every event with it. NO COMMAND SIGNATURE CHANGES and no command imports jobs — that is the whole point, per the D-263 amendment: a command must not know jobs exist, because the alternative is call-site discipline wearing a different hat and it fails the fortieth command into a porting session. Every invocation gets an id, foreground or not, so the two paths are identical and detach is purely a question of where the stream is written. DESIGN CALL to record: only DETACHED runs get a log FILE. A foreground run streams to stderr and writes nothing, because logging all four gate invocations on every push would create retention pressure for output nobody reads. The cost is that a foreground run killed by a timeout loses its output — which is precisely the case that should have used --detach, so the tradeoff points the right way. Acceptance: reach --verbose check client-version shows a job id on its events, and the conformance test proves no domain module imports core.jobs. + +DONE 2026-08-31. Every invocation now carries a job id, and every event it emits is tagged with it. No command signature changed and no command imports core.jobs. + +core/jobs.py holds the current job as a ContextVar — not a module global. A global is correct only until something runs two invocations in one process, which a test harness or a future batch verb does immediately, and it would then interleave two jobs'' events under one id with no error anywhere. + +ORDERING, which took a moment to get right and is the reason this belongs in the decorator rather than anywhere else. The job context must be the OUTERMOST wrapper: @logged emits from its finally and @handle_errors emits its verdict while unwinding, so a context established inside either would already have been reset by the time the two most important events are written. Those would then be the only untagged lines in the log — and they are precisely the ones a detached run gets read back for. Composition is now jobs-context(handle_errors(logged(func))). Verified on both paths: success emits verdict and debug record sharing one id, and the failure path''s verdict is tagged too. + +BUG FOUND AND FIXED IN PASSING: the job id used time.strftime() with LOCAL time while every event''s ts field is UTC, so a job id read 155327 while its own first log line read 13:53:27. Two hours apart, which reads as a logging bug every time someone correlates them by eye. Now time.gmtime(). + +DESIGN CALL RECORDED: only DETACHED runs will get a log FILE (T-1277). A foreground run streams to stderr and persists nothing — writing a log for all four gate invocations on every push would create retention pressure for output nobody reads. Cost: a foreground run killed by a timeout loses its output, which is exactly the case that should have used --detach, so the tradeoff points the right way. + +NEW CONFORMANCE INVARIANT: no module outside core/ may import core.jobs. The moment a domain imports it, ambience has become call-site discipline again and will fail the same way — one command forgets and its output loses correlation silently. + +AND THE INVARIANT WAS INITIALLY BROKEN, which is worth recording because it is the exact failure the prove-it-can-fail discipline exists to catch. My first version inspected only node.module, so it missed `from tooling.core import jobs` — where jobs appears in the NAMES, not the module path, and which is the form anyone would actually write. The test passed while checking nothing. Rewritten to catch all three reachable forms (from tooling.core import jobs / from tooling.core.jobs import x / import tooling.core.jobs), then verified by adding a real violating import to the check router: it failed and named the file and line. Reverted, green.', 'done', 'high', NULL, NULL, 'D-263', '2026-08-31 13:51:51.124', '2026-08-31 13:59:44.052', NULL, '7cdf2b60220de848051b16d2deabdf43', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at; +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5TWD0VVBM4F2WZYAQFET0', 'task', '06G1S7NVJR0GT9KWS9QVYNNFMM', 'core/process.py — detach so the child outlives the parent', 'The spawn primitive, and it is substrate rather than a domain because it has no verbs of its own. Deliverables: spawn a detached child that survives the parent exiting (setsid or equivalent, not just a background shell job, since a killed parent must not take the work with it); redirect the child''s event stream to .cache/reach/jobs/.jsonl and its stdout to a sibling file, keeping the two channels separate exactly as they are in the foreground; write a metadata record carrying command, argv, start time, pid and — on completion — end time and exit code. The metadata file is what makes a finished job readable without re-reading a possibly enormous log. .cache/ is already gitignored. WATCH: the child must re-exec the same reach that was invoked, resolved by bare name per T-1261''s negative criterion, never by an interpreter path or a .venv path — an absolute path here would break the moment the tool is re-pointed at another checkout, and would be a silent wrong-source failure of exactly the kind make reach-repoint exists to fix. Also watch the completion race: the exit code must be recorded by the CHILD as its last act, not polled by a parent that may already be gone.', 'in_progress', 'high', NULL, NULL, 'D-263', '2026-08-31 13:52:01.128', '2026-08-31 14:39:00.853', NULL, '2c467f375858784348443ddc46bf8d3e', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at; +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5TWD0VVBM4F2WZYAQFET0', 'task', '06G1S7NVJR0GT9KWS9QVYNNFMM', 'core/process.py — detach so the child outlives the parent', 'The spawn primitive, and it is substrate rather than a domain because it has no verbs of its own. Deliverables: spawn a detached child that survives the parent exiting (setsid or equivalent, not just a background shell job, since a killed parent must not take the work with it); redirect the child''s event stream to .cache/reach/jobs/.jsonl and its stdout to a sibling file, keeping the two channels separate exactly as they are in the foreground; write a metadata record carrying command, argv, start time, pid and — on completion — end time and exit code. The metadata file is what makes a finished job readable without re-reading a possibly enormous log. .cache/ is already gitignored. WATCH: the child must re-exec the same reach that was invoked, resolved by bare name per T-1261''s negative criterion, never by an interpreter path or a .venv path — an absolute path here would break the moment the tool is re-pointed at another checkout, and would be a silent wrong-source failure of exactly the kind make reach-repoint exists to fix. Also watch the completion race: the exit code must be recorded by the CHILD as its last act, not polled by a parent that may already be gone.', 'in_progress', 'high', NULL, NULL, 'D-263', '2026-08-31 13:52:01.128', '2026-08-31 14:39:40.450', NULL, 'd805b1a9979af2638cd461f44c517cfa', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at; +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5TWD0VVBM4F2WZYAQFET0', 'task', '06G1S7NVJR0GT9KWS9QVYNNFMM', 'core/process.py — detach so the child outlives the parent', 'The spawn primitive, and it is substrate rather than a domain because it has no verbs of its own. Deliverables: spawn a detached child that survives the parent exiting (setsid or equivalent, not just a background shell job, since a killed parent must not take the work with it); redirect the child''s event stream to .cache/reach/jobs/.jsonl and its stdout to a sibling file, keeping the two channels separate exactly as they are in the foreground; write a metadata record carrying command, argv, start time, pid and — on completion — end time and exit code. The metadata file is what makes a finished job readable without re-reading a possibly enormous log. .cache/ is already gitignored. WATCH: the child must re-exec the same reach that was invoked, resolved by bare name per T-1261''s negative criterion, never by an interpreter path or a .venv path — an absolute path here would break the moment the tool is re-pointed at another checkout, and would be a silent wrong-source failure of exactly the kind make reach-repoint exists to fix. Also watch the completion race: the exit code must be recorded by the CHILD as its last act, not polled by a parent that may already be gone. + +DONE 2026-08-31. Detached execution works end to end, and testing it found a real hole that the design as written would have shipped. + +DELIVERED: tooling/core/process.py (spawn, metadata, liveness), the --detach flag on the root callback, and completion recording. Verified against a real spawn — parent returns the job id and exits 0, child runs on and writes .cache/reach/jobs/.jsonl tagged with that id, plus a .out sibling and a .json metadata record. + +THE THREE THINGS THE TICKET FLAGGED, each handled and each verified: +- start_new_session=True, so the child gets its own session and process group and a signal to the parent''s group does not take the work with it. +- The child re-execs `reach` by BARE NAME. An absolute path would freeze it to whichever checkout was current at spawn time, so after make reach-repoint a detached job would silently run the wrong source — the exact failure that command exists to fix. +- The child records its own exit code. Confirmed on the success path (status done, exit_code 0) and on a real failure path (status failed, exit_code 1) using a drift fixture pointed at by SR_REPO_ROOT. + +THE HOLE, found only because I tested a THIRD case the ticket did not name. +Recording completion inside @command looked right and was subtly wrong. A child that fails BEFORE any command runs — bad arguments, an unknown verb, an import error — never reaches that decorator. Verified: `reach --detach check bogus` left its metadata reading status "running" FOREVER, with the process long gone. +That is the exit-0 trap wearing a new disguise, and worse than the original: a failed job that looks busy, in a place nobody is watching. A caller polling for completion would wait indefinitely on a job that failed in milliseconds. +FIX: completion is now recorded at the PROCESS''s exit rather than a command''s. tooling/main.py gains main(), which wraps cli() in one try/finally, and [project.scripts] points at main:main instead of main:cli. Every exit path — success, ReachError, usage error, unhandled exception — now passes through a single finally. Re-verified: `reach --detach check bogus` records status failed, exit_code 2. +The recording was REMOVED from @command rather than left in both places; two writers of the same field is how they drift. + +CONFORMANCE EXEMPTION ADDED, deliberately narrow. The new no-domain-imports-core.jobs invariant fired on main.py, correctly by its letter and wrongly by its purpose. main.py is not a command — it is the entry point, and it already owns --detach, --verbose and --no-input. Reading a job-id constant there is far less coupled than the --detach flag it already carries. Exempted main.py explicitly, with the reason inline so it does not read as an oversight. + +STILL OPEN, and correctly belongs to T-1278: a child killed outright (SIGKILL, OOM, interpreter crash) still cannot record anything, so its metadata stays "running". process.is_alive(pid) exists for exactly this, and jobs list/status must reconcile against it rather than trusting the file. + +reach --help is still 73 ms, so the entry-point wrapper costs nothing on the fast path.', 'in_progress', 'high', NULL, NULL, 'D-263', '2026-08-31 13:52:01.128', '2026-08-31 14:45:58.410', NULL, '4e69f4d0ddf1ff636d194e33cd142fd2', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at; +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06G5G5VJ89C7Q4EPR6Q73FSS74', 'task', '06G1S7NVJR0GT9KWS9QVYNNFMM', 'The jobs domain — list, status, log --follow, wait', 'The user-facing verbs, and a DOMAIN rather than core/ because they carry logic and state of their own — the first real test of the D-263 core bound, which it passes. Deliverables: reach jobs list (recent jobs with status, command and duration), status , log with --follow to tail, and wait . REATTACH IS A BYTE OFFSET into an append-only file, which is the entire reason no daemon is needed: a caller can attach, drop off, and come back without losing anything, and there is no lifecycle to get wrong, nothing to orphan, and no stale state to reconcile. log --follow is therefore a poll on file length, not a subscription. Render the JSONL through the same path a live terminal uses, so a tailed log and a live run are the same artefact in two presentations rather than two renderers that drift. Note for the port: this domain is the first one written from scratch under the full contract rather than ported, so it doubles as the worked example the reach skill (T-1254) should show. + +FROM T-1277 (2026-08-31) — the reconciliation requirement is yours and it is not optional. A child killed outright (SIGKILL, OOM, an interpreter crash) never gets to record its own completion, so its metadata file stays status=running with the process long gone. jobs list and jobs status must therefore RECONCILE against process.is_alive(pid) rather than trusting the file: a job whose pid is dead and whose status still reads running is not running, it died. Report it as such — ''died without recording an exit'' is honest and actionable, whereas showing it as running is the exit-0 trap in a place nobody is watching, and a caller polling for completion would wait forever on something that failed in milliseconds. Note the ordinary failure paths are already covered: T-1277 moved completion recording to the process''s exit (tooling/main.py main()), so bad arguments, unknown verbs, ReachErrors and unhandled exceptions all record correctly. What remains is only the case where the process cannot run code at all.', 'backlog', 'high', NULL, NULL, 'D-263', '2026-08-31 13:52:06.722', '2026-08-31 14:46:03.939', NULL, 'f9aac99fd6449919ab2d48455a81b6e7', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at >= tickets.updated_at; diff --git a/pyproject.toml b/pyproject.toml index 4a89f8bab..673bce440 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -41,7 +41,7 @@ dependencies = [ # top-level `click` to import and no supported way to extract typer's internal # one. Lazy domain registration therefore goes through `typer.Typer(cls=...)` # with a TyperGroup subclass (T-1260) rather than a click Group. -reach = "tooling.main:cli" +reach = "tooling.main:main" [tool.setuptools.packages.find] # Explicit, not flat-layout auto-discovery. The repo root holds client/, server/, diff --git a/tooling/core/command.py b/tooling/core/command.py index 83f2ca3d9..f219758da 100644 --- a/tooling/core/command.py +++ b/tooling/core/command.py @@ -18,6 +18,7 @@ will call: from __future__ import annotations import functools +import os from collections.abc import Callable from typing import Any, TypeVar @@ -56,7 +57,11 @@ def command(func: F) -> F: @functools.wraps(func) def wrapped(*args: Any, **kwargs: Any) -> Any: - token = jobs.begin() + # A detached child adopts the id its parent already reported; a + # foreground run mints a fresh one. + # A detached child adopts the id its parent already reported; a + # foreground run mints a fresh one. + token = jobs.begin(os.environ.get(jobs.ENV_JOB_ID)) try: return inner(*args, **kwargs) finally: diff --git a/tooling/core/jobs.py b/tooling/core/jobs.py index 1c3df98a6..209632526 100644 --- a/tooling/core/jobs.py +++ b/tooling/core/jobs.py @@ -32,6 +32,12 @@ from contextvars import ContextVar, Token _current: ContextVar[str | None] = ContextVar("reach_job_id", default=None) +# How a detached child learns which job it IS. Set by the spawning parent and +# read by @command, so the child's events land under the id the parent already +# reported to its caller — otherwise the returned id would name a log that +# nothing ever wrote to. +ENV_JOB_ID = "SR_JOB_ID" + def new_id() -> str: """A sortable, readable job id: `20260831T134512-a3f2`. diff --git a/tooling/core/process.py b/tooling/core/process.py new file mode 100644 index 000000000..ac4dea368 --- /dev/null +++ b/tooling/core/process.py @@ -0,0 +1,186 @@ +"""Detached execution: spawn a child that outlives its parent (D-263). + +Substrate, not a domain — this has no verbs of its own. The verbs (`list`, +`status`, `log`, `wait`) have logic and state and are therefore +`reach jobs …`, which is the `core/` bound doing its job. + +**Three things here are easy to get subtly wrong, and each has a comment where +it is handled rather than only here:** + +1. *The child must genuinely outlive the parent.* `start_new_session=True` puts + it in its own session and process group, so a signal to the parent's group — + or the parent simply being killed on a timeout — does not take the work with + it. A background shell job would not survive that, which is the whole reason + detach exists. +2. *The child re-execs `reach` by BARE NAME.* Never an interpreter path, never + `.venv/bin/reach` (T-1261). An absolute path would freeze the child to + whichever checkout was current at spawn time, so after `make reach-repoint` + a detached job would silently run the wrong source with no error anywhere — + exactly the failure that command exists to fix. +3. *The exit code is recorded by the CHILD as its last act.* Not polled by a + parent that has already returned. A parent cannot observe an exit it is no + longer around for, and a runner that loses the failure is the exit-0 trap + from D-263 relocated somewhere nothing is watching. + +Streams stay separated exactly as they are in the foreground: the event stream +to `.jsonl`, the command's real output to `.out`. Merging them would +make the log unparseable for the sake of one fewer file. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import time +from pathlib import Path +from typing import Any + +from tooling.core import config, jobs + +# Under .cache/, which is gitignored and already the repo's scratch space — so a +# wrong answer about retention costs disk, never data. +JOBS_SUBPATH = (".cache", "reach", "jobs") + + +def jobs_dir() -> Path: + path = config.path(*JOBS_SUBPATH) + path.mkdir(parents=True, exist_ok=True) + return path + + +def log_path(job_id: str) -> Path: + return jobs_dir() / f"{job_id}.jsonl" + + +def output_path(job_id: str) -> Path: + return jobs_dir() / f"{job_id}.out" + + +def meta_path(job_id: str) -> Path: + return jobs_dir() / f"{job_id}.json" + + +def spawn_detached(argv: list[str]) -> str: + """Run `reach ` in a detached child. Returns the job id immediately. + + The caller is expected to report the id and exit — it must NOT wait, since + not waiting is the entire point. + """ + job_id = jobs.new_id() + directory = jobs_dir() + + # Opened here and inherited by the child, which then owns them. The parent + # closes its copies below; the child keeps writing after the parent is gone. + log_file = open(directory / f"{job_id}.jsonl", "wb") + out_file = open(directory / f"{job_id}.out", "wb") + + child_env = { + **os.environ, + jobs.ENV_JOB_ID: job_id, + # Force machine format: the child's stderr is a file, so isatty would + # already say JSONL — but being explicit means a future TTY-inheriting + # spawn cannot silently start writing prose into a log meant to be read + # back as events. + "SR_OUTPUT_FORMAT": "json", + } + + try: + process = subprocess.Popen( + ["reach", *argv], # BARE NAME — see note 2 in the module docstring + stdout=out_file, + stderr=log_file, + stdin=subprocess.DEVNULL, + start_new_session=True, # note 1: its own session, survives the parent + env=child_env, + cwd=config.repo_root(), + ) + finally: + log_file.close() + out_file.close() + + _write_meta( + job_id, + { + "job": job_id, + "argv": argv, + "command": " ".join(["reach", *argv]), + "pid": process.pid, + "started_at": _now(), + "status": "running", + }, + ) + return job_id + + +def finish_if_detached(exit_code: int) -> None: + """Record completion — called by the CHILD, from the outermost decorator. + + A no-op in a foreground run, which has no metadata file to update. Note 3 + in the module docstring is why this lives on the child's exit path rather + than in whatever spawned it. + """ + job_id = os.environ.get(jobs.ENV_JOB_ID) + if not job_id: + return + meta = read_meta(job_id) + if meta is None: + return + meta.update( + { + "status": "done" if exit_code == 0 else "failed", + "exit_code": exit_code, + "ended_at": _now(), + } + ) + _write_meta(job_id, meta) + + +def read_meta(job_id: str) -> dict[str, Any] | None: + path = meta_path(job_id) + if not path.is_file(): + return None + try: + return json.loads(path.read_text(encoding="utf-8")) + except json.JSONDecodeError: + return None + + +def is_alive(pid: int) -> bool: + """Whether a recorded pid is still running. + + Needed because a child killed outright — SIGKILL, OOM, a crash in the + interpreter itself — never gets to record its own completion, and its + metadata would otherwise say "running" forever. Reconciling against the + process table is what stops a dead job from looking like a busy one. + """ + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + return True # exists, owned by someone else + return True + + +def _write_meta(job_id: str, meta: dict[str, Any]) -> None: + # Written via a temporary file and renamed, because `jobs list` may read + # this at any moment and a half-written JSON file is an unreadable job. + target = meta_path(job_id) + temporary = target.with_suffix(".json.tmp") + temporary.write_text(json.dumps(meta, indent=2), encoding="utf-8") + temporary.replace(target) + + +def _now() -> str: + return time.strftime("%Y-%m-%dT%H:%M:%S", time.gmtime()) + + +def current_argv() -> list[str]: + """The invocation's arguments with `--detach` removed. + + Removed because the child must not detach again — it would fork forever, + each generation spawning another and none doing the work. + """ + return [arg for arg in sys.argv[1:] if arg != "--detach"] diff --git a/tooling/main.py b/tooling/main.py index f75b0e816..793657c45 100644 --- a/tooling/main.py +++ b/tooling/main.py @@ -122,6 +122,41 @@ cli = typer.Typer( ) +def main() -> None: + """Console entry point — `reach`. + + Exists so a detached child records its completion at the PROCESS's exit + rather than at a command's. Recording it inside `@command` looked right and + was subtly wrong: a child that fails before any command runs — bad + arguments, an unknown verb, an import error — never reaches that decorator, + so its metadata said `running` forever. A failed job that looks busy is the + exit-0 trap wearing a new disguise, and worse than the original because + nothing is watching a background job. + + Here, every exit path passes through one `finally`. + """ + exit_code = 0 + try: + cli() + except SystemExit as exc: + exit_code = exc.code if isinstance(exc.code, int) else 1 + raise + except BaseException: + exit_code = 1 + raise + finally: + # Imported lazily and only when detached, so `reach --help` never pays + # for it — the laziness T-1260 protects applies here too. + import os + + from tooling.core import jobs + + if os.environ.get(jobs.ENV_JOB_ID): + from tooling.core import process + + process.finish_if_detached(exit_code) + + @cli.callback() def root( verbose: bool = typer.Option( @@ -130,6 +165,9 @@ def root( no_input: bool = typer.Option( False, "--no-input", help="Never prompt. Hooks and agents should always pass this." ), + detach: bool = typer.Option( + False, "--detach", help="Run in the background; print a job id and return at once." + ), ) -> None: """Global options, declared once here so every domain inherits them. @@ -147,3 +185,18 @@ def root( if verbose: console.set_level("debug") runtime.set_no_input(no_input) + + if detach: + # Handled here, before any domain loads, because detaching is a property + # of the INVOCATION rather than of the verb — every command gets it and + # no command implements it. The child re-runs this same argv with + # --detach stripped, so it does the work instead of forking again. + from tooling.core import process + + job_id = process.spawn_detached(process.current_argv()) + console.out(job_id) + console.verdict( + f"started job {job_id} — this exit status means STARTED, not succeeded", + fix=None, + ) + raise typer.Exit(0) diff --git a/tooling/test_conformance.py b/tooling/test_conformance.py index 8dd5d5b4a..583f7b752 100644 --- a/tooling/test_conformance.py +++ b/tooling/test_conformance.py @@ -111,11 +111,15 @@ def check_jobs_stay_ambient(failures: list[str]) -> None: `core.jobs`, that has become call-site discipline again, and it will fail the same way: one command forgets and its output loses correlation silently. - core/ is exempt — command.py and console.py are where the ambience is - implemented. + Exempt: core/ (command.py and console.py implement the ambience) and + main.py. main.py is not a command — it is the entry point, and it already + owns the invocation-level concerns --detach, --verbose and --no-input. + Recording a detached child's completion at the PROCESS's exit belongs there + for the same reason, and is far less coupled than the --detach flag it + already carries. """ for path in _package_files(): - if path.parent.name == "core": + if path.parent.name == "core" or path.name == "main.py": continue source = path.read_text(encoding="utf-8") tree = ast.parse(source, filename=str(path))