Repository navigation
Run install CLI probes in the existing runtime worker - #3596
Merged
Merged
Conversation
trissim
enabled auto-merge
October 29, 2026 07:07
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
debt-src at 1d9253a, every finding: |
trissim
disabled auto-merge
October 29, 2026 07:15
trissim
enabled auto-merge
October 29, 2026 07:17
trissim
disabled auto-merge
October 29, 2026 07:20
trissim
enabled auto-merge
October 29, 2026 07:22
trissim
disabled auto-merge
October 29, 2026 07:29
trissim
force-pushed
the
work/install-speed--cli-worker
branch
from
October 29, 2026 10:32
b916a9d to
77a665d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
8 commits, 10 files, +196/-114. CLI probes use one persistent selected-runtime transport, deleting the per-command subprocess and the 33-line owner-action executable. Current head: b94cfe5. The final commit corrects only the two stale caller fixtures exposed by root's combined run.
This head is the parser prerequisite checkpoint, not P935 checker delivery. The prerequisite commit changes 8 files, +37/-38: parser output state and the partial-as-parser-class factory are deleted; parser exits carry owned text to the invocation's existing output. No author pytest, ratchet, application/provider input or install. The 13 reported type errors are not claimed gone; hosted debt-src must establish that.
Historical timing, bounded to checkpoint 98d8c6e: one comparable planned-reconcile probe per side, selected-runtime body 7.156 → 4.753 s, caller elapsed 8.527 → 6.055 s. This is neither current-head qualification nor a whole-install speedup. The commander's correction explicitly forbids another author rehearsal here.
1. One selected-runtime worker, not a worker launching another Python
Before:
After:
CliCommandProbe(CliInvocation, RuntimeProbe)parses and executes inside the selected runtime's existing worker.OwnerActionsProbe.executeretains only its batch-to-owner-result projection through that runtime's TargetBatchResult. Batch refusals remain RefusedOwner records; its projection returns status 0 with those entries, as the removed script did.Deleted: the 33-line executable, per-command subprocess.run, and probe entry()/source() overrides. No fallback subprocess, fresh-process name roster or second transport. CliProbePhase.arguments and CliOutput's wire fields remain unchanged.
2. Parser owns parsing; invocation owns output
The earlier instruction was wrong: argparse's stub declares
parser_class: type[_ArgumentParserT](argparse.pyi:242), whilepartial(type(self), output=...)supplies a callable object. The variadic object forwarding constructor also failed to honor argparse's actual typed signature. Casting that callable into a class would only paint over the defect.Before:
After:
Deleted from CliArgumentParser:
__init__, self.output, add_subparsers override, partial import and _print_message override. command_parsing constructstype(self)(add_help=False); argparse supplies its existing parser_class=type(self) default for children. build_parser and all seven production/test call sites no longer receive output. Unneeded StringIO imports disappear with those fixture arguments; five fixture files returned to their main-side content at 77a665d. Root's combined run then exposed two obsolete caller expectations, corrected separately below.CliParserExit has an explicit
(status: int, text: CliParserText)constructor: SystemExit owns code, and the exception owns its text payload. No catch-all argument forwarding or second code assignment. CliParserText now carries a live value; GivenCliParserText/NoCliParserText reuse GivenText/AbsentText's existing texts() behavior. There is no new None decoder or presence test.print_help and print_usage each accept
file: object = Noneas an unused foreign-API argument, so every base file value remains accepted without a runtime Protocol or a narrower TextIO override. Each raises a successful CliParserExit containing its format_help()/format_usage() result. exit has argparse's(status: int = 0, message: str | None = None)signature and raises the declared status with NoCliParserText. error still raises ValueError for Core's JSON failure boundary.Reachability, not a silent message discard
The source inventory finds no reachable Core call requiring print_help/print_usage to return, nor a message-bearing parser.exit caller:
Dynamic future plugins are not certified by the AST inventory. New actions still have to honor the invocation's output owner; no compatibility mechanism was added for hypothetical callers.
3. Console, worker and root lifetime remain distinct
stderr scope remains narrower than the removed subprocess pipe: redirect_stderr captures Python writes to current sys.stderr. Native fd-2 writes, subprocess output and pre-bound logging handlers remain worker diagnostics, not CliOutput.stderr. The wire shape is unchanged; this coverage difference is explicit.
4. Fixtures migrate to the invocation boundary
The root/child help controls already invoke CliInvocation and retain nonempty supplied output plus zero console leakage. Root-restoration, refusal/reuse, worker identity, interrupt propagation and stdin isolation controls remain.
The three absent/empty/present
_print_messagedecoder cases are deleted with that mechanism. Their replacement exercises an explicit parser exit through invoke, preserving status 2, empty supplied output and empty console capture. No always-None decoding test remains. Every build_parser fixture is migrated; no parser stream argument remains to mirror the invocation's output.At 8da2be7, the two additional caller corrections are:
Complete related-test search found no other parser caller still using args.command or a string-vs-Checkout worktree assertion. Remaining command/worktree matches belong to different owners, not this parser contract. No production code changed in this follow-up.
Source evidence and API closure
The retained original source map parsed 1,168 Python files before executable deletion. This correction's refactor-audit Package.load map at rebased prerequisite parent 2191a0b parses 1,167 files under src/tests/tools, zero omissions, with 9 directly related module maps. Evidence includes declarations, imports, inheritance, assignments, decisions and calls, not only a plural-name search.
Applied-source reproof records the 8 prerequisite-changed modules, the declaration/field-option/text-choice dependencies, and argparse's source/stubs. Retained evidence under
/home/ts/.cache/agent-scratch/install-speed:parser-correction-before.json,parser-correction-after.json,parser-correction-search.log,parser-correction-search-after.log,parser-correction-publish.log. AST is source evidence, not runtime or type qualification.Only #3596 overlaps the parser/output-owner files among the inspected open PRs. tools/debt_ratchet.py overlaps stopped #3566/#3538 and is not edited. P935's original checker-site check is unchanged.
The owned branch was rebased onto current main rather than merging main into it. Rebase made the former lock-refresh/revert pair obsolete; its revert would otherwise have undone main's current Toad pin, so that redundant commit was dropped. No net stack/ or uv.lock diff, no new lock refresh, no source discarded or stashed.
Verification limits and root-owned next checks
No author commands here execute pytest, mypy, a ratchet, the application, a provider input or an installer. Source parsing and git diff --check pass. Root's actual combined run at 77a665d records 24 passed, 3 failed in 13.70 s in
/home/ts/.cache/agent-scratch/codex-greentext/p935-worker-controls.log(read in full). Worker/root/help/refusal/holder controls passed. One command-family assertion expected a string instead of Checkout; both cursor cases read nonexistent args.command. The final fixture-only commit corrects those three cases. Root reruns only those failed selectors; no author run or reassuring repetition of the passed selection is requested. Current-head focused results and hosted debt-src remain unclaimed.Root's existing relevant selection is test_install_worker_holds.py plus the CLI failure-family controls. It must cover real root/child help output and no console leakage, JSON argument refusal, nonzero parser exit status, root restoration, same-worker reuse, interrupts and the worker's private stdin. Source review must also close successful owner-batch projection and maintenance-holder freshness. The actual installed native path and an answered-message qualification remain root-owned.
At 98d8c6e only, the real worker handled a runtime probe, root help, read-back child help and invalid-command refusal in one worker: output bytes 9,092 / 90 / 245, statuses 0 / 0 / 1. A direct control confirmed interrupt propagation. Receipt: review-37581-entry-fixed.log/.exit. That older run does not qualify the current parser.
Historical per-probe timing — not current-head proof
Explicitly requested rehearsal at 98d8c6e, exit 0:
Before: retained rehearsal at 6ab8812. Each side has one
--root /home/ts/.agent-comms planned-reconcile --carryingcall. Both are uncopied: pending empty, copied bytes 0, schema rewrites 0, inputs empty.Outer elapsed contains the worker row; do not sum them. N=1 at different times, without frozen bus contents or machine/cache state. This does not measure every command or a whole live installation.
Raw context: 42 phase rows per side; exclusive sums 22.662 / 22.065 s; after command wall 28.333 s. Receipt: cli-final-rehearsal.stdout/.stderr/.exit/.wall-ns and cli-final-comparison.log. Service diagnostics:
/home/ts/..agent-comms.installer-f1f38a209348494e90006ffbcb5d1eb9/. No owner stop/start, reconcile writes, service shutdown or post-carry read-back ran in these no-copy rehearsals. No later parser checkpoint has been re-timed.P935 remains owed
This is not P935 delivery. CheckedRevision still loses sites into Counter/int results. No typed checker result, cache-site propagation, cache-hit diagnostic proof or selected #3572 hosted log exists from this unit. The original checker-site order/check remains intact. Its implementation must join the #3566/#3538 integration batch before tools/debt_ratchet.py is edited; source stops after this prerequisite push.
The earlier sidebar head/main mypy attempts exited before checking because mypy was absent. Root later restored the declared dev extra. Those failed launches establish no inherited-error comparison; none was rerun here. Hosted debt-src must establish whether this prerequisite removes the reported 13 errors and exposes any remaining client issue.
Assumed: root reviews this exact head, owns the combined controls/hosted diagnostics and coordinates the checker integration batch. Auto-merge stays disabled until the rejected head's successor is accepted. No current-head type, runtime or live-install acceptance is claimed.
Root review of the final annotation correction
Root read final head
b94cfe573573f5fb8b49609b1479998ca43bcf93. Since8da2be796, the only change is the unused externalexitmessage argument fromstr | Nonetoobject; runtime behavior is unchanged. The earlier exact-head run had 24 passes and 3 stale caller-fixture failures. Those fixtures were corrected without weakening their assertions, and rerunning only the failed selectors at8da2be796gave 3 passed in 0.48 s. These overlapping results are not a whole-suite result.The native installer rehearsal at
8da2be796passed withpending=[]and an uncopied root, so no copied-root carry or live stop/start is claimed. The CLI probe body took 4.320 s, with 5.543 s caller elapsed; the outer measurement includes the body and must not be added to it. Hosted run 40859332357 reports zero parser TypeErrors; its remaining blocker was the nullable external argument annotation corrected in the final head. Required checks on that final head still decide landing. The root control and rehearsal artifacts are under/home/ts/.cache/agent-scratch/codex-greentext/p935-*.P935 checker-site implementation remains incomplete. Acceptance of this worker/parser prerequisite does not close P935 or claim a live install speedup.