Skip to content

Run install CLI probes in the existing runtime worker - #3596

Merged
trissim merged 8 commits into
mainfrom
work/install-speed--cli-worker
Oct 29, 2026
Merged

trissim merged 8 commits into
mainfrom
work/install-speed--cli-worker

Conversation

@trissim

@trissim trissim commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

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:

RuntimeProbe → InterpreterWorker
CliCommandProbe(RuntimeProbe) → subprocess python -m agent_comms.cli
OwnerActionsProbe → executable probes/owner_actions.py → CLI

After:

CliInvocation                         RuntimeProbe[CliOutput]
             \                       /
                  CliCommandProbe
                         |
                  OwnerActionsProbe.execute

CliCommandProbe(CliInvocation, RuntimeProbe) parses and executes inside the selected runtime's existing worker. OwnerActionsProbe.execute retains 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), while partial(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:

CliInvocation(output)
  → build_parser(output)
    → CliArgumentParser.output
      → constructor forwarding + partial child factory
      → _print_message + codec guessing text versus None

After:

CliInvocation(output)
  → build_parser(arguments)
    → ordinary CliArgumentParser and argparse's own child construction
      → print_help / print_usage formats known text
      → CliParserExit(status, GivenCliParserText(text))
      → exit(status) supplies NoCliParserText
  ← one catch writes exited.text.texts() to output and returns status

Deleted from CliArgumentParser: __init__, self.output, add_subparsers override, partial import and _print_message override. command_parsing constructs type(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 = None as 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:

  • argparse's help action calls print_help then exit; raising the typed exit at print_help completes that same terminal path.
  • argparse's message-bearing exit is in its default error; CliArgumentParser overrides error to raise ValueError instead.
  • The stdlib version action writes through _print_message, but Core declares no version action. It is not a reason to keep a speculative output/decoder path.
  • Core's generic DeclaredCommand creates children through the supplied subparser action, adds declared fields and sets command_declaration. Context's extensions add ordinary arguments. Separate DeclaredVerb tools use plain argparse parsers, not this subclass, and retain their own behavior.

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

  • Console main alone converts KeyboardInterrupt to 130. invoke catches declared CLI failures and CliParserExit, not interrupts. Worker interrupts propagate to existing worker-death handling.
  • invoke borrows an unstarted LaunchRoot and restores the caller after success, refusal, help or exception. No process-global output binding or stdout redirection is introduced.
  • serve owns private protocol descriptors; operations see stdin EOF, not later requests. The existing InterpreterWorkerKey selects runtime/cwd/environment, and each ProbeRequest carries its maintenance holder.
  • Outgoing runtimes execute their installed implementation. The first upgrade carrying this PR still uses old subprocess behavior for outgoing-owner stop; incoming calls use the new worker path.

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_message decoder 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:

  • test_command_families compares the parsed UserSendCliCommand.worktree with Checkout.checked(tmp_path), not str(tmp_path). Checkout.checked requires a Git checkout (repo.py:391–395); the test's tmp_path was bare, so the existing Repo.git initializes only that disposable fixture after the missing-worktree refusal. The required-worktree refusal and all later enum/argument/decode assertions remain intact.
  • test_page_cursor uses args.command_declaration.from_arguments(args), not a second member_named lookup through deleted args.command. Its dead CliCommand import is removed; both cursor/JSON cases and every cursor/serialization assertion remain.

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.

  • IMPL-13: fact = selected-runtime execution/lifetime; at = removed CLI subprocess path; derive = RuntimeProbe; by = inherited CliInvocation in the existing worker.
  • BOUND-5: fact = owner-action implementation; at = deleted executable; derive = existing CLI/batch classes; by = OwnerActionsProbe.execute.
  • IDEN-5: fact = invocation output destination; at = parser.output beside invocation output and the old stdout redirect; derive = invocation's existing output; by = deleting parser stream state/factory and carrying text to one catch/writer.
  • BOUND-1: fact = parser text presence; at = removed _print_message decoder interrogating its unused exit message; derive = the actual help/usage or plain-exit producer; by = explicit GivenCliParserText/NoCliParserText carried by CliParserExit.

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:

nice -n 19 ionice -c3 timeout -s INT 600 \
  /home/ts/.local/bin/agent-comms --root /home/ts/.agent-comms install \
  --checkout /home/ts/wt/install-speed--agent-comms \
  --to 98d8c6ea35573f7f60c88561615d4e6cf5ecb87d \
  --installer install-speed --rehearse

Before: retained rehearsal at 6ab8812. Each side has one --root /home/ts/.agent-comms planned-reconcile --carrying call. Both are uncopied: pending empty, copied bytes 0, schema rewrites 0, inputs empty.

One comparable probe per side Before Measured 98d8c6e
Selected-runtime body 7.156 s 4.753 s
Caller elapsed, including worker 8.527 s 6.055 s

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.

argparse asked for a class
we handed it a callable wearing a fake moustache
now argparse constructs its own children; the invocation does the talking

Root review of the final annotation correction

Root read final head b94cfe573573f5fb8b49609b1479998ca43bcf93. Since 8da2be796, the only change is the unused external exit message argument from str | None to object; 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 at 8da2be796 gave 3 passed in 0.48 s. These overlapping results are not a whole-suite result.

The native installer rehearsal at 8da2be796 passed with pending=[] 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.

@trissim
trissim enabled auto-merge October 29, 2026 07:07
Copilot AI balanced review requested due to automatic review settings October 29, 2026 07:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

debt-src at 1d9253a, every finding:

::warning file=src/agent_comms/cli_arguments.py::LintFindings +1 (warns, never blocks)
::warning file=src/agent_comms/installer/probes/owners.py::LintFindings +1 (warns, never blocks)
grew NoneAbsence:src/agent_comms/cli_arguments.py +1
::warning file=src/agent_comms/cli.py::Unformatted +1 (warns, never blocks)
found NoneAbsence:src/agent_comms/cli_arguments.py at src/agent_comms/cli_arguments.py:99
introduced NoneAbsence:src/agent_comms/cli_arguments.py at lines [99]

@trissim
trissim force-pushed the work/install-speed--cli-worker branch from b916a9d to 77a665d Compare October 29, 2026 10:32
@trissim
trissim enabled auto-merge October 29, 2026 10:49
@trissim
trissim added this pull request to the merge queue Oct 29, 2026
Merged via the queue into main with commit 33bd05c Oct 29, 2026
9 checks passed
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.

2 participants