Repository navigation
feat(runtime-compat): the engine side of the desktop repair card (#5672 slice 2, E1) - #5700
Conversation
… needs (red) Regression tests for slice 2's engine additions (E1), stacked on #5672. They fail at #5672's head 5e19931 and pass once the implementation lands: - repair --json: one progress line per phase, then one result line - consent recorded from the app before the repair acts; success closes the incident, tells the owner in the DM thread, and answers late replies - SUTANDO_RUNTIME_CONSENT_SURFACE=app: no wait, no DM (module and real launcher) - doctor --json schema 1 with selected_reason, plan_hash and auth - the chosen binary and the reason, in doctor --json and every launch log - notify-owner: at most one DM per incident, none once usable - app-surface time bounds: 10 s per binary, 60 s per discovery, 30 s retries Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
When the desktop app launches the core it is the consent surface, so the engine must answer fast, never DM on its own, and give the app a stable contract to render and act on: - repair --json streams checking-disk, downloading (attempt n of N), verifying, switching and done, then one result line with the version. - repair --consent-source app --consent-by MXID [--plan-hash H] records the consent on the incident before acting and refuses a changed plan. Success closes the incident, sends one "fixed from the app" follow-up when a DM went out, and answers pending or late install replies instead of repairing twice. - SUTANDO_RUNTIME_CONSENT_SURFACE=app: ensure never waits and never queues the DM; probes get 10 s per binary and 60 s per discovery (later candidates are "unknown", never a pass); no download attempt starts 30 s after the first, and npm's own fetch retries are off. - doctor --json entries carry schema 1, selected_reason, plan_hash and auth. - Every ensure and which logs the chosen binary and why; selection itself is unchanged (the first usable one wins). - notify-owner queues the incident's one DM for an unanswered card, and nothing once the CLI works. Review follow-ups from the contract review: the no-wait test asserts no installer ran, too-old and incompatible prefer-managed cases, candidate keys and auth.login values pinned, a done phase, and the Codex launcher case. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/feat/runtime-compat-repair...HEAD, staged and unstaged changes
Summary
src/runtime_compat.pyLines 1250-1258 1250 def out(queued: bool, reason: str) -> int:
1251 if a.json:
1252 print(json.dumps({"queued": queued, "reason": reason}))
1253 else:
! 1254 _say(f"owner DM {'queued' if queued else 'not queued'}: {reason}")
1255 return 0
1256 report = discover(a.runtime, env=env)
1257 if report.selected is not None:
1258 return out(False, "usable")Lines 1257-1265 1257 if report.selected is not None:
1258 return out(False, "usable")
1259 ws = _workspace()
1260 if ws is None:
! 1261 return out(False, "no-workspace")
1262 inc = Incident(ws, a.runtime)
1263 if inc.notify(notice_body(a.runtime, plan(report, env=env))):
1264 return out(True, "queued")
1265 return out(False, "already-sent" if inc.rec.get("notice_queued") else "not-queued") |
…r resumable REPAIR_PHASES is the only phase set repair --json may emit; an undeclared phase raises. Tests pin the set, the order on success and failure runs, done exactly once and last, that done is never written to an on-disk record, and that a swap journal carrying it is rolled back, not finished. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…budget is spent The retry loop checked the budget only after a failure, so a backoff clipped to the remaining budget, plus any scheduler delay, could still start another attempt past the deadline (attempt 2 at 30.01 s). The budget is now checked again before each later attempt is dispatched; when it is spent, the repair stops with the last attempt's failure. Tests (fake clock): an attempt failing at 29 s whose 1 s backoff lands exactly on the deadline, and overslept backoffs (29 s + 5 s, 1 s + 40 s). Each starts only the first attempt. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Code review at 5a94103a0: no change requested. Not approving yet because there is no expert approval at this head.
What I checked
- Read the whole diff against the base
feat/runtime-compat-repair(5e19931; the branch is up to date with it):src/runtime_compat.py, the three test files, anddocs/runtime-compat-repair-matrix.md. tests/runtime-compat-app-surface.test.pyat this head:Ran 40 tests ... OK. Withsrc/runtime_compat.pyswapped back to the base version, the same file givesFAILED (failures=5, errors=32). So the tests exercise the real module, not a copy.- The budget fix in
5a94103: withsrc/from4d01b67,AppRepairBudgetfails exactly the two new cases (test_no_attempt_starts_at_the_exact_deadline,test_an_overslept_backoff_does_not_start_another_attempt). At this head they pass._run_with_retrynow checks the budget again before each later attempt is dispatched, and clips the backoff to what is left. tests/runtime-compat.test.py(the existing suite) is still OK at this head.- None of the new tests assert on module source. Their
read_text()calls read only output files: records, notices, results. - No added line has a hardcoded
~/.sutandopath or a liveSUTANDO_WORKSPACEdefault. CI is all green, and the commit status haslicense/cla: success(1 context). There are no unresolved review threads. - E1-c is applied where the launchers enter.
runtime-compat-gate.shcallsensure, andmain()forceswait_s=0andnotify_owner=FalseunderSUTANDO_RUNTIME_CONSENT_SURFACE=app. The real-launcher gate testAppLaunchedCoreNeverWaitsOrDmspins this.
Non-blocking notes
_repair_cmd: when--consent-source appis given but_workspace()returns None, the repair still runs. In that case no consent is recorded andresolve_from_appis skipped. E1-b says consent is recorded before acting. Either refuse there, or say in the docs that this is the degraded path.ensure()applies the app-surface rules only throughmain(). A future Python caller ofensure()would not get them. Moving the check into_ensurewould keep the contract in one place.- The early refusals (
no-consent,plan-changed) emit a result line with nodoneprogress line before it. That is fine, but the desktop parser should not wait fordone.
What's missing before this can be approved
- At least one substantive APPROVED review at this head from a maintainer with write access.
- It is stacked on #5672, which is still open, so the merge order in the body applies.
- Owed, not blocking: one real round trip through the desktop repair card (doctor, then consent, then
repair --jsonprogress, then the incident closed in the DM). This is a startup path, and every test here uses stub CLIs.
— Sutando (susans-MacBook-Pro)
An app --consent-source with no --plan-hash skipped the changed-plan
check entirely, so a stale app card could trigger whatever repair plan
is current. A workspace that failed to resolve also let the install
run without ever recording the consent that authorized it.
Both paths now refuse before any install: a missing or mismatched
--plan-hash under --consent-source app ("plan-hash-required" /
"plan-changed"), and an unresolvable workspace under any consent
source ("workspace-unavailable").
Red-first in tests/runtime-compat-app-surface.test.py:
test_app_consent_without_a_plan_hash_is_refused and
test_missing_workspace_refuses_before_installing fail on the prior
head and pass after the fix.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
|
Fixed the app-consent plan-hash gap in
Red-first tests added in
Updated three existing tests that relied on the old (buggy) no-plan-hash shortcut to now pass a real plan hash: Full New head: 67e4eae |
marklysze
left a comment
There was a problem hiding this comment.
Reviewed at 67e4eae (stacked on #5672; CI: 33 pass, nothing failing or pending). Scope: the two new refusals in _repair_cmd and their tests, as asked. Approving; two low findings.
What I checked
- Both refusals sit before
assisted_install, so nothing installs on either path:plan-hash-required(src/runtime_compat.py:1214) is ahead of the plan computation, andworkspace-unavailable(:1228) is ahead ofIncident(...)andrecord_consent. The mismatched-hash refusal that was already there still comes first when a hash is supplied, and each refusal is aRepairResultwith exit 2,--jsonresult line and auser_action, the same shape the other refusals use. - Tests:
runtime-compat-app-surface42/42 andruntime-compat-gate10/10 pass here.codex-core-launcherhas 8 failures (thenotifier_*tests); the identical 8 fail on the base branch on this host (run side by side), so the PR adds none, and its newtest_app_surface_broken_codex_stops_at_once_without_a_dmpasses. - Mutants (each alone,
AppConsent/RepairJson/AppRepairBudget): removing the plan-hash refusal, moving it tonot consent_source and not plan_hash, removing the workspace refusal, and either refusal exiting 0 are all caught.
Findings
- Low: "any consent source" is not what the workspace test pins. The commit and PR say an unresolvable workspace refuses under any consent source, but
test_missing_workspace_refuses_before_installinguses--consent-source apponly. A mutant that narrows the check toa.consent_source == "app" and ws is Nonepasses the whole suite, so a future non-app source could go back to installing with no consent recorded. A second case with another source (for example--consent-source test) pins it. - Low, design note: a non-app source with no
--plan-hashis still accepted and records the consent against the current plan hash (consent_hash = current), i.e. the same stale-card gap this PR closes forapp. Nothing sends such a source today (the desktop usesapp), and a mutant requiring the hash for every source also survives, so the contract is unspecified either way. Worth deciding which one is intended and pinning it, or limiting--consent-sourceto the values the engine knows.
Not blockers. The fix does what the description says on both paths.
liususan091219
left a comment
There was a problem hiding this comment.
Approve at 67e4eae6. The bar is met: @marklysze's substantive approval at this head, CI green (33 success), license/cla: success (1 context), and no unresolved review threads.
What changed since my comment at 5a94103a (one commit, src/runtime_compat.py +12/−1):
src/runtime_compat.py:1214-1217:--consent-source appwithout--plan-hashnow refuses withplan-hash-required(exit 2). The check runs before the plan is computed, so a stale card can no longer consent to whatever the plan has become.src/runtime_compat.py:1228-1232: a consent source with no resolvable workspace refuses withworkspace-unavailable, ahead ofIncident(...)andrecord_consent. This closes the gap I noted last round, where the repair ran with no consent recorded.
What I ran (scratch worktree, SUTANDO_WORKSPACE* unset):
- At head:
runtime-compat-app-surface42/42,runtime-compat-gate10/10,runtime-compat139/139, all OK. - Revert-to-fail: with
src/runtime_compat.pyfrom5a94103aand the tests from head, exactly the two new cases fail,test_app_consent_without_a_plan_hash_is_refusedandtest_missing_workspace_refuses_before_installing(tests/runtime-compat-app-surface.test.py:220,:227). So the tests exercise the real refusals. scripts/review-checks.shon the PR diff: PASS. The new tests'read_text()calls read output records and notices, never module source.
No change-request condition fires. The base feat/runtime-compat-repair is the intended stack on #5672, not a wrong base.
Non-blocking
- I agree with Mark's first finding: the workspace refusal is tested only with
--consent-source app, though the code refuses for any source. A second case with another source would pin that.
Still owed, not blocking: one real round trip through the desktop repair card (doctor, consent, repair --json progress, incident closed in the DM). Merge order stays #5672 first, then this PR.
— Sutando (susans-MacBook-Pro)
Stacked on #5672 (base
feat/runtime-compat-repair, head5e19931). Merge order: #5672 first, then this PR, then the desktop D0 pin bump.Slice 2 of #5672, engine part "E1", from the plan page "#5672 slice 2 — desktop repair plan" (§2 E1-a..d, as amended by §7 D-1..D-3, R-2, R-3).
Status
134376c(draft checkpoint): the regression tests only, red on feat(runtime-compat): detect and repair an unusable Claude Code or Codex CLI before a launch #5672's head.a165967: the implementation insrc/runtime_compat.py, the review follow-ups below, and the docs. All green locally.4d01b67: one declared phase set (REPAIR_PHASES). An undeclared phase raises.doneis a stdout progress line only: it is never written to a record, and a swap journal carrying it is rolled back, not finished (RepairPhases, 5 tests).5a94103: fix, from review. The retry budget was checked only after a failure, so a backoff clipped to the remaining budget plus scheduler delay could start attempt 2 past the deadline (at 30.01 s). The budget is now checked again before each later attempt is dispatched. Fake-clock tests cover an exact-deadline case (fail at 29 s, 1 s backoff) and overslept backoffs (29 s + 5 s, 1 s + 40 s).python3 tests/runtime-compat-app-surface.test.py AppRepairBudgetwith the new tests, before the fix (4d01b67'ssrc/):after (
5a94103):What it adds (contract the desktop parses)
repair --json:{"type":"progress","phase":"checking-disk"|"downloading"|"verifying"|"switching"|"done", "attempt", "attempts"}lines, then one{"type":"result", ok, reason, detail, user_action, attempts, path, version}RepairJsonrepair --consent-source app --consent-by <mxid> [--plan-hash H]: recordsconsent{source,by,at,plan_hash,runtime}instate/runtime-compat-<cli>.jsonbefore acting; refuses a changed plan (plan-changed, exit 2); success closes the incident (resolved_by: app), tells the owner in the DM thread, and answers pending or late "install compatible …" replies with "already fixed from the app"AppConsentSUTANDO_RUNTIME_CONSENT_SURFACE=app:ensurenever waits and never queues the DM, whatever--wait/--notify-ownersayConsentSurfaceApp, gateAppLaunchedCoreNeverWaitsOrDms(real launcher)doctor --jsonschema: 1,selected_reason,plan_hash,auth{login,remedy,detail}; key sets pinnedDoctorJsonSchemaselected_reasone.g.chose <runtimes>/bin/claude (managed) 2.1.0 because ~/.npm-global/bin/claude (PATH) is broken: --version exited 1, indoctor --jsonand oneruntime-compat: chose …line on everyensure/whichPreferManagednotify-owner --runtime R [--json]→{queued, reason}; once per incident viaIncident.notify; nothing when the CLI is usable nowNotifyOwnerunknown: not probed in timeand unusableTimeoutsAppRepairBudgetContract notes (from Mini's review of
134376c):test_app_surface_blocked_download_gives_up_within_budgetpinsattempts < 3for 20 s attempts). That is the contract the card's attempt counter shows. A single attempt still runs to completion, so a slow but working download is never cut off.donephase: chose to emit it in code, as §2 E1-a lists it.{"type":"progress","phase":"done"}comes right before the result line, on success and failure. The plan page is not changed.test_app_surface_broken_codex_stops_at_once_without_a_dmintests/codex-core-launcher.test.py.candidates[]entry's keys, andauth.loginis one ofok,logged-outorunknown(AUTH_LOGIN_VALUES;expiredis item 7/E3, not this PR).Two tests already pass at the base and stay as pins:
test_repair_without_json_keeps_stdout_empty(prose stays on stderr) andtest_missing_cli_plans_npm_install_via_bundled_node(R-3 fresh machine).Evidence: red at #5672's head
5e19931(tests from this branch)python3 tests/runtime-compat-app-surface.test.pytests/runtime-compat-gate.test.pyAppLaunchedCoreNeverWaitsOrDms(realstart-cli.sh, broken stubclaude,SUTANDO_RUNTIME_WAIT_Sunset):CI at
134376cwas red only for those two new files:python standalone tests (6/9)failed ontests/runtime-compat-app-surface.test.pyand(7/9)ontests/runtime-compat-gate.test.py.tsc + tests (clean install)is the aggregate job that requires them.Evidence: green at
a165967(and4d01b67)Run locally with the host's
SUTANDO_*andTMUX*variables unset:codex-core-launcher: the same two tests (test_managed_notifier_waits_for_each_result_before_next_task,test_launches_codex_and_managed_task_notifier: "managed core-input monitor did not start") also fail at the base5e19931on this host. They are host-specific and not touched by this diff. The three other notifier tests that failed in the full run pass in isolation, both here and at the base. CI is the arbiter.At
5a94103:Also:
ruffclean,scripts/review-checks.shPASS on the diff against the base, andgen-src-map --checkup to date.Stub CLIs and temp dirs only; no real credentials.
🤖 Generated with Claude Code