Repository navigation
fix(settings): let clear-app-state take its app from --app and refuse two apps - #3211
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 19 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
The code looks sound at 926a093, and I found nothing that blocks it. One open thread still needs a fix, and the branch has merge conflicts with main. The 6 checks are green. Please change the retry hint at contracts/settings.ts:252 to use Not blocking, and you can take or leave these: adding Could the CLI admit These threads do not apply, so you can resolve them: r4178279217 (doctor never records an action, so only settings carries I did not run tests. The PR body mentions a live iOS simulator run, but I saw no transcript, so I could not confirm it. The Android pm path with a named app has unit tests only. |
926a093 to
600cda7
Compare
|
Size Report
Startup median (7 runs, lower is better):
|
|
Since 926a093, the new head 600cda7 fixes the earlier code findings, but live evidence is still missing. The The Coverage job fails in The live iOS run in the PR body was on 926a093 and has no transcript. Android was never run live. Please run against 600cda7 with no session app open and paste the transcript. On the iOS simulator, show these.
On the Android emulator, show these.
The "(parser)" refusal claim for wifi in the PR body is stale, because the refusal is now daemon-side. Only the body text is out of date. Of the open inline threads, these do not apply and can be resolved.
No open thread still applies. Before merge, move |
|
Rebased onto main ( Of the non-blocking list:
Behavioral consequence worth flagging: dropping Nine of thirteen threads were fixed in that commit; the other four are resolved with the reason from your read (doctor never records an action, the |
|
Coverage failed on Fixed in Verified: |
… two apps (#3179) `settings clear-app-state --app com.example.app` dropped the named app: the CLI only folded `--app` into the settings whose app has no positional, so the clear fell through to the session app, a destructive change to an app the caller never named. A positional app plus a different `--app` silently kept the positional. The clear-app-state parse now resolves its app from the positional or `--app`, and refuses a positional and a different `--app` as a contradiction instead of picking one.
c4f9ca6 to
4085ad6
Compare
|
The one Smoke Tests failure on |
|
Reviewed at 4085ad6: the code looks good to merge. The open question from the 600cda7 review is settled, because the change only maps argv to input and sends the same positionals as the existing route, so it needs no live run. Not blocking, take or leave: the two-app test in The one failing Smoke cell fails in "Preflight iOS runner through public CLI" with |
Summary
#3205 landed the explicit
app/--apptarget forsettings permissionand iOSsettings location on|off, including the scope table and thesetting_app_not_consumedrefusal. This PR is now reduced to the one gap left on top of it:settings clear-app-stateignored--app.On main,
settings clear-app-state --app com.example.app(orsettings clear-app-state clear --app com.example.app) dropped the named app, so the clear fell through to the app bound to the session. That is a destructive change to an app the caller never named. A positional app plus a different--appalso silently kept the positional.Now
clear-app-statetakes its app from the positional or from--app. A positional and a different--appare refused as a contradiction:The env and config default for
targetAppis still stripped forsettingsby the parser (#3205), so only a typed--appreaches this rule. Help text and the command reference are updated.Not carried over from the earlier version of this PR
Main settled these differently in #3205, so they were dropped instead of being layered on:
targetAppfrom env and config for every command (main strips it forsettingsonly, sodoctorkeeps its env default).flags.targetApp(main sends it as request input).wifi(main treats an empty app as no app, and leaves unclassified settings to the owner).--appin.adscripts. This needs the recorder and replay to carry the request input, which is a separate change.Validation
pnpm check:affected --base origin/main --run: format, lint, typecheck, layering, fallow, build, and command-docs pass. In the related tests, onlyremote-proxy-parity.test.ts > lease expiry tears the session down…failed. It is a provider proxy test that this diff does not touch, and it fails at the same point when run by itself.