Skip to content

feat(onboarding): disclose and gate agent status hooks on first run - #19974

Closed
brennanb2025 wants to merge 7 commits into
mainfrom
brennanb2025/enable-hooks-onboarding
Closed

brennanb2025 wants to merge 7 commits into
mainfrom
brennanb2025/enable-hooks-onboarding

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 22 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2429 $\color{#cf222e}{\Huge{\mathbf{−}}}$​15 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2414
Prod 36 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1211 $\color{#cf222e}{\Huge{\mathbf{−}}}$​255 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​956

ELI5

Orca quietly edits your agent CLIs' own config files every time it starts, so it can tell when an agent is working, needs you, or is done. Nobody ever told you. This puts a checkbox on the first-run screen that explains it, leaves it on by default, and — for brand-new profiles — waits for you to actually see it before touching anything.

What Changed

Onboarding step 1 gains an Enable agent status hooks checkbox, default on, in a single card beside the yolo-permissions control. It carries a one-line why, a collapsed What Orca changes, and when dropdown listing only the agents detected on that machine with their config paths, an off-ramp to Settings → Agents, and a consequence line shown only when unchecked.

First-run gate. A one-shot latch managedAgentHookFirstRunGate is frozen at load. While a genuinely fresh profile has not passed step 1, the startup install is not called — deferral means not calling the writer, never calling it with false.

An unchecked box can never install, from any caller. installManagedAgentHooks now returns hooks_disabled skips when the preference is off. That holds for every existing caller and every caller added later, including across restarts. There is deliberately no mirror guard that removes: on a fresh profile nothing of ours is installed, and removeManagedAgentHooks() deletes user-global entries another Orca profile may own (STA-5679). Declined means call nothing.

Consent is transactional. The checkbox value rides along with onboarding:update; main persists the preference before advancing onboarding, so a failed write aborts the advance, the latch, and the install together. No new IPC channel — ipc/agent-hooks.ts:22-25 forbids exposing install/remove to the renderer and this does not need it.

Why

Hooks install at startup with no prompt, writing into ~/.claude/settings.json, ~/.cursor/hooks.json and eleven other user-global configs. The only off switch is in Settings and nothing points at it. Without the gate the checkbox would be decorative, since startup runs before onboarding paints.

The preference persists on change rather than on Continue because dismissOnboarding persists nothing and skipToRepo saves only defaultTuiAgent — a Continue-only write loses an uncheck when the user presses Esc.

Review round — what the first revision got wrong

An adversarial review rejected the first commit. Fixed here:

  • The startup gate's only test was non-discriminating. Its anchor string matched twice at identical indentation, so deleting the real conjunct left all four assertions green. Deleted; the decision is now a pure function with a table test.
  • The renderer half had zero tests. Deleting the single line implementing the feature's central claim left the whole repo green.
  • shouldContinue was missing from the onboarding install — a mid-install uncheck could complete a removal while the in-flight loop re-added entries. Net: hooks on disk, preference off.
  • userInitiated: true was a regression. Grok treats an empty hooks.json as an opt-out unless that flag is set; the startup path this replaces passes no userInitiated, so true would have started overriding it for every fresh profile. Dropped.
  • The cohort signal counted only the primary state file, while the loader's own recovery path counts backups. A user whose primary was gone but backups survived was classified fresh and would silently stop having hooks maintained.
  • The codex disclosure was false — it said "Orca-managed Codex home" while the real-home lane writes ~/.codex/hooks.json and mutates config.toml.
  • The "When" line was false for existing users re-opening the wizard; rewritten cohort-neutral.

A pre-existing data-loss gap this PR closes (STA-5679, Codex real home)

~/.codex/hooks.json and config.toml are user-global, and the sweep matches Orca-managed entries by script filename, not by which profile installed them. Passing the off switch into the per-pane ensure at codex-launch-preparation.ts:41 therefore let one profile delete another profile's hook entry and trust records.

Startup was already made install-only for exactly this reason (main-process-ready-runtime.ts:108-117 gates on the preference, then passes a hard hooksEnabled: true). The per-pane twin was left behind — same function, same user-global files, two opposite policies. The commit that exempted Codex justified it with "its hooks live in an Orca-owned runtime home" (4ff07da23c, 2026-08-27), which had stopped being true six weeks earlier when the system-default lane moved onto the real ~/.codex (c3f37e5500, 2026-07-14).

Why it is fixed here rather than filed. The path is pre-existing and untouched by this branch, but the onboarding checkbox is what makes it easy to reach, in the worst direction: it fires only on a genuinely fresh profile, and a fresh profile owns nothing and — having no managed Codex accounts — defaults onto the real-home lane. Concretely: install a second Orca profile, uncheck the box at first run, open one Codex pane, and the primary profile's entries are gone. Four lines closes it.

Nothing legitimate is lost: routing is unchanged (the lane reads as usable whether 'pending' or 'removed'), and opt-out convergence is already owned by the Settings toggle, which is user-initiated — where STA-5679 says removal belongs. Adds the first test for codex-launch-preparation.ts, which had none; the off-switch case was ablated and fails without the guard.

Follow-ups, and what was decided for each

Issue Disposition
Authorization refactor — isAgentStatusHooksEnabled serves both "is the feature on" and "may I write user-global config", indistinguishably; false at the writer means delete, so "not yet asked" is unrepresentable Deferred, with evidence. Four passes counted 5 → 7 → 14 → ~20 writers, each finding sites the last missed. This PR also leaves three divergent predicates for the deferral axis. A delta review concluded the PR reduces the destructive surface, so this is debt for the next author, not a regression here.
Cross-profile ~/.codex sweep Fixed in this PR — see above. Investigated and confirmed a gap, not deliberate.
Disclosure fold — only the first rows fit; "Your hooks" and "Turning off" sit below an uncued fold Deferred, measured. The outer cap is the wrong lever: live ablation gave 160px → 6 agent cards / 1 disclosure row, 224px → 3 cards / 2 rows, 288px → 0 cards / all rows. Zero-sum, and it only engages on crowded machines. The fix is capping the variable-height agent list, not the container.
CLI defeats the gate — orca agent hooks enable on a fresh profile installs while the latch is still 'pending', so the next launch defers over hooks that already exist Deferred. One line (the CLI's enable path should retire the latch). Needs the CLI used before first app launch.
Windows quit removes Grok hooks unconditionally Dropped — not a bug. Deliberate and correct: the Windows bare-path Grok hook has no ORCA_PANE_KEY guard, so it must be cleaned up on exit, and cross-profile safety comes from the grok-hook-owners peer-liveness registry rather than the preference. That registry is precisely the ownership mechanism Codex lacks — which is why the same pattern is safe for Grok and was not for Codex.

Architecture note

A separate architecture round considered replacing the authorization shape with a permit type threaded through the writers. Deliberately not done here. It also produced a finding worth recording: at codex-real-home-hook-install.ts:101 and managed-agent-hook-controls.ts:257, false means delete, not don't write — so routing the Codex launch/resume paths through a shared authorization and mapping "awaiting consent" to false would sweep the user's real ~/.codex. Those three call sites are deliberately untouched, and are unreachable before step 1 (modal wizard, no repos, no panes).

--serve hosts are exempt and get no consent step. A serve host paints no wizard — paired clients keep onboarding in browser storage and there is no onboarding RPC — so it would otherwise defer forever. That means a fresh remote/SSH host still installs into its user-global agent configs with no disclosure. Correct, but it narrows the "waits for you to see it" claim above to desktop profiles.

The round inventoried ~20 surfaces that write user-global agent config. Most are out of reach of this change. Filing that inventory as a follow-up rather than widening this PR.

Linked Issue

None — direct product request rather than a filed issue.

Visual Proof

Captured from the real first-run wizard (dialog "Orca onboarding", step pips reading 1 of 3 — the array has five steps but the Windows-terminal step is skipped on macOS). Hidden renderer, CDP screenshots, isolated ORCA_DEV_USER_DATA_PATH, ORCA_BACKGROUND_LAUNCH=1. No computer-use.

Before: this row did not exist — step 1 ended at the Yolo checkbox.

Step 1, checked by default, disclosure collapsed — caret at rest, pointing right
collapsed

Disclosure expanded — caret rotated down
expanded

Unchecked — the consequence line appears, muted rather than amber (turning hooks off is a legitimate choice, not an error)
unchecked dark

Light theme, checked
checked light

Light theme, unchecked
unchecked light

All five images are shot against the shipping commit. An earlier set was discarded rather than reused: three predated the caret and three were taken at a disclosure height that was subsequently reverted, so they depicted two states that do not exist. Light theme was reached through the real wizard (step 2 -> Light -> back to step 1), not by forcing a class.

If you test the caret rotation: assert on the computed rotate property, not transform. Tailwind v4's rotate-90 sets the standalone rotate (none -> 90deg); computed transform reads "none" in both states, so a working rotation looks broken if you assert on transform.

Verified programmatically rather than by eye: the card is the only bordered element in the control (every descendant queried for non-zero border width returns [], checkbox excluded), and the consequence line's computed colors are neutral in both themes — dark rgb(161,161,161) / lead rgb(250,250,250), light rgb(115,115,115) / lead rgb(10,10,10), R=G=B throughout.

Two defects the capture found, which two adversarial code reviews had not:

  1. The disclosure trigger rendered as plain static text with no caret, while its sibling six pixels above it carries an arrow. Fixed in 29c9da5dd2.
  2. The disclosure's height cap strands "Your hooks" and "Turning off" below a fold with no visible cue. Not fixed — a live ablation in the running wizard measured the cap as a zero-sum dial against the agent grid: 160px gives six full agent cards and clips the third disclosure row; 224px gains one disclosure row and drops the grid to three full cards plus a row of sliced card-tops; 288px fits the disclosure and leaves no fully visible agent card. Raising it is also inert when few CLIs are detected, so it can only act in the case where it hurts. Getting all four rows above the fold requires capping the variable-height agent list instead of the whole disclosure — filed as follow-up rather than done here.

Testing

  • I manually tested these changes locally (first-run wizard, isolated profile, dark + light)
  • Automated tests added/updated

All gates re-run independently at the final commit, not taken from the implementing agent's report:

Gate Result
pnpm tc exit 0
pnpm test (touched paths) 103 files, 969 passed, 9 skipped
oxlint exit 0 (verified non-vacuous with a deliberate-violation canary)
check:code-quality:changed 0 findings across 35 files, incl. React Doctor
check:max-lines-ratchet OK, no new bypasses
4× verify:localization-* all exit 0

Wiring ablations, actually run (revision 3). A second review found the first revision's defect class one layer up: bypassing the startup gate left 526 test files / 5165 tests green, and deleting the three props wiring the checkbox to state left all 115 onboarding tests green. Both now fail — I re-ran the exact ablation that was previously green and it fails 2 of 4; deleting the props fails 2 of 3. The consent ref's freshness is covered too.

Earlier ablation: removing the on-change updateSettings line — the feature's central claim — fails 2 renderer tests. Restored and re-verified green. The previous revision had no test that could detect that removal.

Renderer tests: 17 cases across 2 new files, covering default-checked render, uncheck fires once, collapsed-by-default dropdown, the detected ∩ targets − disabled set (fixture carries a detected non-target, a disabled target and a kept target so the set algebra is load-bearing), the detecting state, immediate persistence before any Continue, uncheck-then-Esc, and that async hydration does not re-check a cleared box.

Known control arms, kept but not counted as coverage: "reconciles once the latch has been retired", "stays deferred while before step 1", "leaves a pre-release profile alone", and the wire-guard file's absence assertions (that file does not import the gate module).

Platforms: macOS only. Not tested on Linux, Windows, or SSH.

Review

  • onboarding-flow-state.ts is deliberately untouched — the on-change write already covers skip and dismiss. Please don't "fix" it.
  • The gate lifts on closedAt !== null even when lastCompletedStep < 1, i.e. dismissed without answering. Intended: an accidental Esc must not leave status permanently broken. Named hasPassedStepOneOrLeft so the rule reads honestly.
  • use-onboarding-settings-draft.ts exists because use-onboarding-flow.ts sat at exactly 300/300 against the max-lines cap; the extraction takes it to ~209.

Small adjacent change, flagged: updateEnabledOnDisk in cli/handlers/agent-hooks.ts now returns agentStatusHooksEnabled explicitly, so the offline CLI path exercises the new installer guard deliberately rather than passing it by omission. Behaviour is unchanged today. Say the word and I'll drop it.

Agent skill upstream boundary

N/A — no agent skills touched.

Orca installs managed agent status hooks at every launch, writing into each
detected CLI's own user-global config (~/.claude/settings.json and friends)
plus scripts in ~/.orca/agent-hooks/. Until now that happened silently, before
onboarding ever painted, with the only off switch buried in Settings.

Add an "Enable agent status hooks" checkbox to onboarding step 1, default on,
with a collapsed dropdown disclosing what Orca changes and when, and a pointer
to Settings -> Agents. On a genuinely fresh profile the startup install is now
deferred until the user passes that step, so the checkbox means something.

The preference persists on change rather than on Continue: dismissOnboarding
persists nothing and skipToRepo saves only defaultTuiAgent, so a Continue-only
write silently loses an uncheck when the user presses Esc.

While deferred, settings writes persist the preference but skip the reconcile.
Without that, uncheck-then-recheck would install before step 1 was passed, and
an uncheck would run removeManagedAgentHooks() against user-global files this
profile never wrote, deleting hooks another Orca profile owns (STA-5679).

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one consent-semantics point worth a deliberate call, plus a note on verification.

Reviewed changes

  • First-run latch (managedAgentHookFirstRunGate) — a one-shot 'pending' | 'done' setting frozen at load from fileExistedOnLoad, modelled on tabSwitchKeybindingSeed, so a genuinely fresh profile defers the managed-hook install until it passes onboarding step 1 (or dismisses/skips), and a re-opened wizard can never re-arm it.
  • Startup gate (main-process-ready-runtime.ts) — !isServeMode && isManagedHookInstallDeferredForFirstRun(...) skips the startup reconcile on a fresh profile and retires the latch on any non-deferring launch.
  • onboarding:update install — fires the install once (fire-and-forget) when the deferral lifts, gated on isAgentStatusHooksEnabled; idempotency comes from the latch.
  • Reconcile suppression (settings.ts, runtime-client-settings.ts) — skips the applyAgentStatusHooksEnabled reconcile while the first-run window is open, so an uncheck-then-recheck can't install before step 1 and an uncheck can't delete another profile's user-global hooks (STA-5679).
  • Renderer control — AgentStatusHooksControl checkbox + collapsed disclosure on the agent step, use-onboarding-settings-draft extraction (takes use-onboarding-flow back under the max-lines cap), and on-change persistence of the choice.

ℹ️ Manual verification is the merge gate here

The author is right to hold this open on visual proof and a packaged first-run pass. The two specific things that only a running app can confirm are (1) that a packaged fresh profile actually reaches onboarding:update (the install-once path) before renderer settings hydration completes — usePersistCurrentStep bails when settings is null — and (2) the settings:set-then-onboarding:update ordering that keeps the belt-and-braces write from ever reconciling prematurely. The code ordering is sound on paper, but this is exactly the kind of sequencing that deserves one real run before shipping.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ipc/onboarding.ts Resolved
@coderabbitai

coderabbitai Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 65c6fae0-f67a-4fb4-a634-643da6f998f1

📥 Commits

Reviewing files that changed from the base of the PR and between 29c9da5 and 4333db6.

📒 Files selected for processing (2)
  • src/main/startup/codex-launch-preparation.test.ts
  • src/main/startup/codex-launch-preparation.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

Adds a main-owned first-run latch that defers managed-agent-hook reconciliation until onboarding advances or closes. Centralizes hook enablement checks and adds cancellation during shutdown or preference changes. Extends onboarding IPC and preload APIs to carry consent with state updates. Adds persistence migration and backup recovery handling. Adds runtime guards, onboarding draft state, agent status-hook controls, affected-agent disclosure data, locale strings, and comprehensive tests.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 4333d

The first-run gate behavior is covered broadly, but the startup wiring test can still miss an unconditional latch-retirement regression. Address the test assertion before merge or accept this bounded coverage risk.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 51 functions across 42 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description gives detailed scope, rationale, testing, visual proof, and known limitations. It does not provide the required linked issue, and the repository checklist is omitted. Add a valid issue reference after Fixes #, restore the required checklist, and complete any applicable checklist items.
Linked Issues check ⚠️ Warning The description explicitly states None for the linked issue, while the template requires an issue link for every pull request. Add the issue reference that this pull request fixes or update the repository metadata with the required linked issue.
✅ Passed checks (2 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The Codex real-home and CLI changes are adjacent to the hook preference and first-run gating behavior. The description explains their safety rationale and test coverage, so no unrelated change is evid…
Title check ✅ Passed The title clearly and concisely describes the main changes: onboarding disclosure and first-run gating for agent status hooks.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 656b3f9a-9ef6-4341-90b0-894eeb0a66fe

📥 Commits

Reviewing files that changed from the base of the PR and between 58ff95b and e32b4c8.

📒 Files selected for processing (25)
  • src/main/agent-hooks/managed-hook-first-run-gate.test.ts
  • src/main/agent-hooks/managed-hook-first-run-gate.ts
  • src/main/ipc/onboarding-first-run-hooks.test.ts
  • src/main/ipc/onboarding.ts
  • src/main/ipc/settings.test.ts
  • src/main/ipc/settings.ts
  • src/main/persistence-cohort-and-identity-migration.test.ts
  • src/main/persistence/loading-store/loaded-cohort-migrations.ts
  • src/main/persistence/loading-store/loaded-state-parsing.ts
  • src/main/runtime/managed-hook-first-run-gate-wire-guard.test.ts
  • src/main/runtime/runtime-client-settings.ts
  • src/main/runtime/runtime-store-contract.ts
  • src/main/startup/main-process-ready-runtime.ts
  • src/main/startup/managed-hook-first-run-gate-startup-wiring.test.ts
  • src/renderer/src/components/onboarding/AgentStatusHooksControl.tsx
  • src/renderer/src/components/onboarding/AgentStep.tsx
  • src/renderer/src/components/onboarding/OnboardingFlow.tsx
  • src/renderer/src/components/onboarding/agent-status-hook-disclosure-rows.test.ts
  • src/renderer/src/components/onboarding/agent-status-hook-disclosure-rows.ts
  • src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts
  • src/renderer/src/components/onboarding/use-onboarding-flow.ts
  • src/renderer/src/components/onboarding/use-onboarding-settings-draft.ts
  • src/renderer/src/i18n/locales/en.json
  • src/shared/agent-hook-config-locations.ts
  • src/shared/global-settings-types.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Comment thread src/main/ipc/onboarding.ts Resolved
Comment on lines +37 to +39
entryBody.split("store.updateSettings({ managedAgentHookFirstRunGate: 'done' })").length - 1
).toBe(1)
expect(entryBody).toContain('isManagedHookFirstRunGatePending(startupManagedHookSettings)')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert that the pending check guards the retirement write.

These assertions only prove that both expressions exist. An unconditional store.updateSettings() call would still pass this test and could retire the gate before onboarding.

Assert the surrounding conditional structure, or extract the retirement decision into a testable function.

Comment thread src/renderer/src/i18n/locales/en.json Resolved
Adversarial review found the startup gate's only test was non-discriminating
(its anchor matched twice, so deleting the real conjunct left it green) and the
entire renderer half untested — removing the one line that implements the
feature left the repo green.

Guard the installer itself: installManagedAgentHooks now returns hooks_disabled
skips when the preference is off, so an unchecked box cannot install from any
caller or any later launch. No mirror guard that removes: on a fresh profile
nothing of ours is installed, and removeManagedAgentHooks deletes user-global
entries another Orca profile may own (STA-5679). Declined means call nothing.

Make step-1 consent transactional. The checkbox write was fire-and-forget and
the renderer store swallows failures, so a failed write plus an advancing wizard
installed using the default-on value — for a user who had unchecked. Consent now
rides along with onboarding:update and main persists it before advancing.

Also: the cohort signal counted only the primary state file while the loader's
own recovery path counts backups, so a user whose primary was gone but backups
survived was classified fresh; pass shouldContinue and isQuitting to the
onboarding install; drop userInitiated, which would have overridden Grok's
opt-out for every fresh profile relative to the path it replaces; correct the
codex disclosure, which claimed Orca does not touch ~/.codex.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found. The prior userInitiated consent concern is cleanly resolved and the revision adds the transactional step-1 consent it was missing.

Reviewed changes

  • Dropped userInitiated: true — the first-run install no longer asserts a flag the untouched default-on box never earned, so a deliberate empty Grok orca-status.json opt-out is left alone, matching the startup path this replaces; a regression test pins it.
  • Made step-1 consent transactional — onboarding:update now carries an OnboardingConsent payload; main persists agentStatusHooksEnabled before advancing, so a failed write aborts the advance, the latch, and the install together, and a stale default-on stored value can't install against an unchecked box.
  • Added install-loop cancellation — a shouldContinue callback wired to shouldContinueManagedHookStartup stops the fire-and-forget install mid-loop on a Settings uncheck, a disabled agent, or app quit.
  • Made the off-switch caller-independent — installManagedAgentHooks returns hooks_disabled skips for every agent before any presence probing, with no mirror remove (STA-5679).
  • Refactored the startup decision into a pure resolveStartupManagedHookPlan with a 10-case table test, replacing the non-discriminating source-string wiring test, and gated the codex real-home chain on the codex-specific predicate.
  • Backup-aware cohort classification — profileExistedOnLoad counts a surviving backup as a pre-existing profile, matching the loader's recovery evidence.
  • Renderer polish — detecting/empty disclosure states, a truthful Codex dual-lane disclosure, cohort-neutral "When" copy, and a muted (non-amber) uncheck consequence, with consent carried on both the advance and the close path.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

…ision

A second review found the same defect class as the first, one layer up:
bypassing the startup gate entirely left 526 test files green, and deleting the
three props connecting the checkbox to any state left all 115 onboarding
renderer tests green. The previous round deleted a vacuous grep test and
replaced it with a pure-function table test, which made the decision well
covered and the wiring not covered at all. Fixing the anchor was the answer;
removing the layer was not.

Add behavioural wiring tests at both seams, each verified by ablation: driving
the real ready-phase with only the installer and store faked, and driving the
checkbox through OnboardingFlow rather than AgentStep, since an AgentStep-level
test cannot see the props go missing. Also cover the consent ref's freshness,
which is what makes uncheck-then-Esc carry the right value.

Drop the disclosure's "isn't uploaded anywhere". Agent-hook envelopes are
published to paired devices, and hook ingestion fires telemetry, so both that
claim and the narrower "not sent as analytics" are falsifiable by grep. On a
consent surface an absolute a reviewer can disprove is worse than the silence it
replaces, so the line now states only what was traced.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found. The new commit is test-focused (behavioral wiring tests at both the startup and renderer seams) plus a defensive CLI echo and a more accurate consent disclosure.

Reviewed changes

  • Startup-gate wiring test — main-process-ready-managed-hook-gate.test.ts drives the real initializeReadyRuntimeServices with only the installer, Codex real-home, and store faked, asserting a fresh profile installs nothing and keeps the latch armed, a step-1 pass reconciles once and retires the latch, a serve host never defers, and the off switch still wins post-retirement.
  • Renderer consent-wiring test — onboarding-agent-hooks-consent-wiring.test.tsx drives the checkbox through OnboardingFlow rather than AgentStep, asserting an uncheck reaches the settings write, the disclosure excludes disabled agents, and a dismiss carries the current box state via the consent ref.
  • Disclosure copy corrected — the "isn't uploaded anywhere" claim is replaced with "goes to Orca on this machine and to the devices you've paired with it", removing a falsifiable absolute from a consent surface.
  • CLI offline echo — updateEnabledOnDisk now echoes agentStatusHooksEnabled so the offline install's authorization guard reads the value just written instead of falling through to the default.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

…ckable

Visual capture showed "What Orca changes, and when" rendering as plain static
text in both states, while its own sibling six pixels above it ("Show N more
agents") carries an arrow. Nothing signalled the row was interactive, which
makes a disclosure nobody opens.

Not raising the disclosure's height cap along with it. A live ablation in the
running wizard measured the cap as a zero-sum dial against the agent grid: at
160px the grid shows six full cards and the disclosure clips its third row; at
224px the disclosure gains one row and the grid drops to three full cards plus
a row of sliced card-tops; at 288px the disclosure finally fits and the grid
has no fully visible card at all. The cap is also inert on machines with few
detected CLIs, so raising it can only act in the case where it hurts. Getting
all four rows above the fold needs the variable-height agent list capped rather
than the whole disclosure, which is more than a polish change.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Added a caret to the disclosure trigger — AgentStatusHooksControl's "What Orca changes, and when" trigger now renders a ChevronRight that rotates down on open via [&[data-state=open]>svg]:rotate-90, so the collapsed row reads as clickable rather than static text. The icon is aria-hidden (decorative) and the trigger text remains the accessible name, so no test or label semantics change.

The fix is a single presentational line, scoped to the one defect the author's screenshot pass caught (the disclosure reading as inert text). The rotate-on-open pattern matches the existing shadcn accordion convention ([&[data-state=open]>svg]:rotate-180) and no existing renderer test asserts on the trigger's text-only content, so the suite is unaffected.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Made per-pane Codex real-home hook prep install-only (STA-5679) — ensureRealHomeHooksIfSelected in codex-launch-preparation.ts now returns early when isAgentStatusHooksEnabled is false and passes a hard hooksEnabled: true when it is true, so a second profile's first hooks-off Codex spawn can no longer sweep the primary profile's user-global ~/.codex/hooks.json and its trust records. The managed-home lane still passes the off switch through, which is correct since that home is Orca-owned.
  • Added the first test for codex-launch-preparation.ts — three cases drive the real prepareCodexRuntimeHomeForLaunch with the installer, hook-service, trust, WSL, and store faked while the off-switch predicate stays real, asserting the real home is untouched when hooks are off, hard-enabled when on, and skipped on WSL launches.

The off-switch ablation is discriminating (ensureRealHomeCodexHookState not called), and the hard-enable case pins hooksEnabled: true, so a regression in either direction fails the suite.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one stale comment left behind by the refactor.

Reviewed changes

  • Removed the render-phase ref writes that the changed-lines react-doctor gate flagged. setSelectedAgentInteractive now closes over selectedAgent, detectedAgentIds, isDetectingAgents, pathSource, and pathFailureReason instead of five mirror refs; the mount-once auto-select uses a functional setSelectedAgent updater instead of reading selectedAgentRef inside the async then; and useCloseWith takes a consent value (a useMemo keyed on the boolean) rather than a ref cell, so closeWith re-identifies only when the checkbox answer changes.
  • Updated the persistence probe in use-onboarding-flow-persistence.test.ts to pass consent by value, keeping the existing dismiss-freshness test as the behavioral pin.

I ran the three touched tests (use-onboarding-flow-persistence, onboarding-agent-hooks-consent-wiring, use-onboarding-settings-draft): 16 passed. The refactor is behavior-preserving — the functional-update path reads the same latest state the mirrors did, and the consent useMemo keeps the Escape/close path fresh.

ℹ️ Nitpicks

  • onboarding-agent-hooks-consent-wiring.test.tsx:14 — the file doc still reads "the ref closeWith reads tracks the box". consent is now a memoized value, not a ref.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +46 to +47
// Why a ref: closing also lifts the first-run hook deferral, so the close write has to carry the
// same consent the advance does — without re-identifying closeWith on every checkbox click.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This rationale is now stale: consent is a memoized value, not a ref cell, so the "Why a ref" framing no longer matches the code.

Suggested change
// Why a ref: closing also lifts the first-run hook deferral, so the close write has to carry the
// same consent the advance does — without re-identifying closeWith on every checkbox click.
// Why a memoized value: closing also lifts the first-run hook deferral, so the close write has to
// carry the same consent the advance does — re-keying only on the flag keeps closeWith stable.

…pressing them

CI's static analysis runs react-doctor with --scope lines --blocking error, which
is a different gate from the oxlint react-doctor plugin that check:code-quality:changed
runs -- that one reported zero findings while this one failed with six.

Six "ref mutated during render" errors. One was new here: the consent ref added
for the transactional first-run write. The other five were the agent-pick
telemetry mirrors, byte-identical to main -- the max-lines extraction relocated
them into a new file, which is what pulled them into changed-lines scope.

Rather than suppress, remove the refs:

- The five mirrors are gone. setSelectedAgentInteractive now depends on the
  values it reads. Its identity churns, which costs nothing: every dep is
  already a prop of the step that renders the handler.
- The mount-once auto-select read selectedAgentRef inside an async then(); it
  now uses a functional state update, which reads the latest value with no ref.
- The consent ref is a useMemo keyed on the boolean, and useCloseWith takes the
  value rather than a ref cell. closeWith is re-created only when the answer
  changes, which is what makes the Escape path carry the fresh value.

Behaviour is pinned: freezing the consent memo at mount fails
"sends the box state at dismiss time, not the one it was first rendered with".
No react-doctor-disable anywhere in this branch.
The disclosure listed devin as ~/.config/devin/config.json unconditionally, but
getDevinConfigPath (src/main/devin/hook-settings.ts) resolves
%APPDATA%\devin\config.json on win32. The map's own header admitted the
divergence instead of handling it, which is not good enough on a surface whose
entire job is telling the user which files get written.

The map becomes a function of platform rather than devin being special-cased at
the call site, so the next target that diverges has one obvious place to go.
Ablated: pinning devin back to the POSIX string fails the win32 case.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — the new commit is correct; the description's STA-5679 claim is now stale.

Reviewed changes

  • Made hook config locations platform-aware — AGENT_HOOK_CONFIG_LOCATIONS became a getAgentHookConfigLocations(platform) function, resolved at the disclosure call site from getRendererAppPlatform(), so devin shows %APPDATA%\devin\config.json on Windows and ~/.config/devin/config.json elsewhere. Three new cases pin the win32 branch, the POSIX branch, and that the platform-independent targets are untouched.
  • Dropped the STA-5679 per-pane Codex install-only guard from the branch — codex-launch-preparation.ts is back to passing the raw off switch into the real-home writer, and its new test file is gone. The same fix now lives in #20345 (a superset that also covers codex-session-resume-launch.ts).

ℹ️ The description still says this PR ships the STA-5679 Codex fix

The "Cross-profile ~/.codex sweep" row reads Fixed in this PR, and the "Why it is fixed here rather than filed" section still stands, but the rebased head contains no Codex change. Since the mitigation moved to #20345, updating the description and pointing at that PR keeps reviewers from expecting it to land here.

Technical details
# Description claims a STA-5679 Codex fix that is not in the head

## Affected sites
- PR #19974 description — the "Cross-profile `~/.codex` sweep | Fixed in this PR" row and the "A pre-existing data-loss gap this PR closes (STA-5679, Codex real home)" section.
- `src/main/startup/codex-launch-preparation.ts` — no longer contains the install-only early-return / hard `hooksEnabled: true`; `codex-launch-preparation.test.ts` was deleted (net delta vs the prior reviewed head `ea17e03`).

## Required outcome
- The description reflects that the Codex fix is split into #20345 rather than shipped here.

## Open questions for the human (optional)
- Is the #20345 cross-link wanted in the body, or is that PR's own description enough?

Pullfrog  | Fix it ➔ | View workflow run | Using openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

The gate this replaces decided "is this profile new?" by inspecting the disk at
load: a state file or any backup meant pre-existing. That inference was wrong
three separate times. Telemetry seeding writes a file for a brand-new profile,
so a second profile installed hooks globally before its own checkbox appeared,
undoing the first profile's opt-out. A user who quit onboarding halfway read as
consented on relaunch. Two candidate redesigns died on the same inference.

Freshness is now recorded when it is a fact rather than discovered later. A
marker under the user-data root, established in preflight before any profile or
Store exists, says whether this installation predates the change. Absence means
pre-change, so an upgrading user is never deferred and nothing is written for
them at all.

Every ambiguity installs. A corrupt marker, a malformed partial record, an
unreadable root, or a failed write all resolve to install-as-today; a deferral
requires no marker, none of Orca's own artifacts on disk, and a successful
write. Deferring a user who already has hooks would stop maintaining them, so
that direction is the one that must never happen by accident.

Consent is installation-scoped, not per-profile, so a second profile inherits
the answer instead of being asked again -- hooks are user-global and the person
has already seen the disclosure. Profile seeding now carries
agentStatusHooksEnabled too; previously a parent's opt-out was silently lost and
the child reinstalled.

The policy is threaded through every writer, not just startup. SSH, the WSL
relay, Codex launch and resume, and the offline CLI all bypass
installManagedAgentHooks and each wrote user-global config on its own.

Also required, not incidental: the Codex refresh no longer runs the legacy
system cleanup. That is a profile-local operation reaching into the real
~/.codex, which "existing users untouched" forbids outright.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Unchecking the onboarding box on a fresh installation sweeps user-global hook config. Both reconcile gates evaluate the install policy against the value just written, so an agentStatusHooksEnabled: false write resolves to deny rather than defer, the !== 'defer' guard passes, and removeManagedAgentHooks() runs. The previous head keyed this gate on the first-run latch (which ignored the preference) and did not have the bug. Details inline.

Reviewed changes

  • Replaced the per-profile latch with an installation-scoped marker — managed-hook-installation.json at the user-data root records installCohort/onboardingDecision; it is minted post-change/pending only when the root holds no Orca state, existing installs persist nothing, and any torn/malformed record reads as pre-change so ambiguity installs as before.
  • Introduced a tri-state install policy — allow/defer/deny in managed-hook-install-policy.ts, with deny outranking everything, a resolver installed during preflight from the marker plus host mode, and an onboarding_pending skip reason distinct from hooks_disabled.
  • Routed every writer through the policy — startup plan, settings:set, the runtime-client update path, Codex launch and session-resume, WSL relay/guest install, and the SSH remote hook/plugin installs.
  • Made new profiles inherit consent — seedNewOrcaProfileInheritedConsent replaces the telemetry-only seeder and carries agentStatusHooksEnabled (including false) into a new profile's data file.
  • Decoupled the Codex managed-home refresh from the legacy real-home sweep — refreshCodexRuntimeUserHooksExclusively no longer calls cleanupLegacyManagedHookRepresentations; explicit removal still does.
  • Added coverage — policy decision table, marker parse/mint/pass and bootstrap ordering, a writer-boundary scan, WSL/SSH policy tests, settings/runtime gate tests, onboarding, and profile-consent seeding.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread src/main/ipc/settings.ts
// Why only the reconcile is skipped: the preference above already persisted. Reconciling here
// would install before the first-run question is answered, or remove user-global hooks a
// different Orca profile owns (STA-5679).
if (hookSettingChanged && resolveManagedHookInstallDecision(result).kind !== 'defer') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

resolveManagedHookInstallDecision(result) reads the settings after the write, so an agentStatusHooksEnabled: false update resolves to deny, not defer, and the guard passes. The onboarding checkbox persists its uncheck through this handler (use-onboarding-settings-draft.ts:125), so on a fresh (pending) installation unchecking the box reaches applyAgentStatusHooksEnabled(false, …) → removeManagedAgentHooks() and sweeps user-global ~/.claude/~/.cursor/~/.grok entries — the STA-5679 deletion this comment says it prevents.

Technical details
# Pending installations sweep user-global hook config on an uncheck write

## Affected sites
- `src/main/ipc/settings.ts:238` — the decision is computed from `result` (post-write), so `agentStatusHooksEnabled: false` returns `deny` and satisfies the `!== 'defer'` guard.
- `src/main/agent-hooks/managed-hook-install-policy.ts:51-61` — `isExplicitlyDisabled` is checked before the cohort/pending branch by design; this caller feeds it a value that has already collapsed `defer` into `deny`.
- `src/main/agent-hooks/managed-agent-hook-controls.ts:244-245` — `applyAgentStatusHooksEnabled(false, …)` returns `removeManagedAgentHooks()`, deleting Orca-managed entries from user-global agent configs.
- `src/renderer/src/components/onboarding/use-onboarding-settings-draft.ts:125` — every checkbox toggle persists through `updateSettings` → `window.api.settings.set` → `settings:set`.

## Concrete sequence
1. A fresh user-data root has no Orca state, so preflight mints `{ installCohort: 'post-change', onboardingDecision: 'pending' }` (`main-process-preflight.ts:272-279`).
2. The wizard's box is on by default; the user unchecks it before Continue/Esc.
3. `settings:set({ agentStatusHooksEnabled: false })` → `hookSettingChanged` true → `resolveManagedHookInstallDecision(result)` is `deny` → reconcile runs → `removeManagedAgentHooks()`.

The prior head's gate used `isManagedHookInstallDeferredForFirstRun(...)`, which keyed on the latch and onboarding progress and ignored the preference, so it skipped the reconcile on this write.

## Required outcome
- A reconcile must never run while the installation's onboarding answer is still `pending`, regardless of the value written. Opt-out convergence stays on the post-onboarding Settings toggle (and on startup, which already never removes).

## Suggested approach (optional)
- Key the gate on the installation state (`isManagedHookOnboardingPending()` or the marker's `onboardingDecision`) rather than a policy decision recomputed from the post-write value; or resolve it against the pre-write `before` snapshot.

## Note on coverage
- `src/main/ipc/settings.test.ts:197-221` asserts the skip with a static `() => ({ kind: 'defer' })` resolver that ignores its argument, so the production `{false + pending} → deny` combination is never exercised. Driving the real `getManagedHookInstallDecision` (or the real resolver) would turn this test red today.

// reconcile with the off switch set would remove hooks another Orca profile owns (STA-5679).
if (
(typeof updates.agentStatusHooksEnabled === 'boolean' &&
resolveManagedHookInstallDecision(settings).kind !== 'defer' &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same defect on the paired-client/runtime-server path: settings is the post-update snapshot, so a client writing agentStatusHooksEnabled: false while the host's marker is still pending resolves to deny and reaches reconcileManagedAgentHooks() → removeManagedAgentHooks(). Gate this on the installation pending state alongside settings.ts:238 rather than on the value just written.

Technical details
# Runtime-client reconcile removes user-global hooks while the first-run answer is pending

## Affected sites
- `src/main/runtime/runtime-client-settings.ts:143` — `resolveManagedHookInstallDecision(settings)` is evaluated after `store.updateSettings(updates)`, so a `false` update yields `deny`.
- `src/main/runtime/runtime-client-settings.ts:203-224` — `reconcileManagedAgentHooks()` calls `applyAgentStatusHooksEnabled(settings.agentStatusHooksEnabled !== false, …)`, i.e. `removeManagedAgentHooks()` for the `false` case.

## Required outcome
- A pending installation does not reconcile/remove on a client settings update; explicit opt-out still converges once onboarding has been answered.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing this older onboarding implementation and rebuilding the choice on the current startup paths. Create a dedicated issue for explaining hooks and honoring the first-run choice before closing.

The remaining problem is tracked in #27669.

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.

1 participant