feat: settings toggle for AGENT_NOTIFY_SUMMARY contract injection - #1376
Conversation
There was a problem hiding this comment.
Findings
- [Major] Runtime settings update can lose unrelated settings under concurrent writers — the new endpoint performs an unlocked read/merge/write against the same
settings.jsonused by CLI auth/machine persistence and relay-key refresh. The CLI already holds${settingsFile}.lockacross updates, but this path ignores that lock, and hub writes reuse a fixed.tmppath. Concurrent writes can overwritecliApiToken,machineId, orrelayAuthKey, fail with a missing temp file, or return success for a toggle value that was not persisted. Evidence:hub/src/config/sessionSummaryContract.ts:27; related contextcli/src/persistence.ts:94,hub/src/config/settings.ts:73.
Suggested fix:Implementconst next = await updateSettings(settingsFile, (current) => ({ ...current, sessionSummaryContract: enabled })) return next.sessionSummaryContract === true
updateSettingsas a shared-lock read-modify-write using the existing${settingsFile}.lockconvention and a unique temporary file; migrate runtime relay writers to the same helper.
Questions
- None.
Summary
- Review mode: initial
- One Major finding: the new hub-wide runtime setting is not concurrency-safe with existing settings writers.
Testing
- Not run (automation).
- GitHub CI: CLI typecheck passed; web typecheck failed in unchanged
web/src/router.tsx:773, so hub typecheck and the test suite were skipped. - Add a concurrency test that races/serializes settings updates and verifies unrelated fields remain intact and the returned toggle matches the persisted value.
HAPI Bot
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No findings at the current head. The prior settings concurrency finding is addressed by the shared lock/unique-temp update path in hub/src/config/settings.ts:97, and the runtime writers introduced or affected by this PR now use it in hub/src/config/sessionSummaryContract.ts:26 and hub/src/tunnel/relayAuth.ts:46.
Residual risk: flavor integration is covered primarily through the shared instruction helper; no launcher-level test asserts the hub-enabled contract reaches each Claude/Codex/OpenCode/Grok launch seam.
Testing
- Not run (automation); PR content was not executed.
- GitHub
testcheck pending at review time. - Suggested coverage: one bootstrap-to-launch assertion per supported flavor, plus a cross-process settings update test using both the CLI and hub lock implementations.
HAPI Bot
0d11563 to
eb3f9ff
Compare
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No findings at the current head. The latest commit supplies the missing QueryClientProvider in the settings-page test harness; the full 30-file PR diff was reviewed again, including hub persistence/auth, CLI bootstrap propagation, and each supported launcher seam.
Residual risk: flavor integration remains covered mainly through the shared instruction helper, and the web test verifies rendering but not toggle persistence, loading, or failure behavior.
Testing
- Not run (automation); PR content was not executed.
- GitHub
testcheck passed at review time. - Suggested coverage: one bootstrap-to-launch assertion per Claude/Codex/OpenCode/Grok path, plus a web mutation test for enabled, pending, and failed states.
HAPI Bot
Add a hub-persisted, default-off Settings control so operators can opt agents into emitting the trailing AGENT_NOTIFY_SUMMARY line. Propagate the resolved flag on CLI session bootstrap and inject at call time for Claude, Codex, OpenCode, and Grok (Cursor still unsupported). Closes tiann#1375 Co-authored-by: Cursor <cursoragent@cursor.com>
Serialize settings.json updates with the shared .lock protocol and unique temp files so the new hub toggle cannot clobber CLI/relay fields under concurrency. Also supply deliveryMode on abort send-error restore so web typecheck (and CI) pass. Co-authored-by: Cursor <cursoragent@cursor.com>
The hub-settings toggle uses TanStack Query; the settings page suite was rendering without a QueryClient and blew up CI. Co-authored-by: Cursor <cursoragent@cursor.com>
eb3f9ff to
4e4a15a
Compare
There was a problem hiding this comment.
Findings
- [Minor] Toggle can persist the wrong value before settings load — missing query data is rendered as
falsewhile the control remains interactive, so clicking during the initial request can overwrite an enabled hub preference. Evidenceweb/src/routes/settings/general.tsx:63
Suggested fix:{hubSettingsQuery.data ? ( <SettingsSwitch checked={hubSettingsQuery.data.sessionSummaryContract} // existing label, description, and onChange /> ) : null}
Questions
- None.
Summary
Review mode: follow-up after new commits
One Minor finding at the current head. The previous reviewed commit and current head have diverged, so the complete 30-file diff from the current main base was reviewed again; the incremental compare was used only as follow-up context.
Testing
- Not run (automation); PR code was not executed.
- Suggested coverage: hold
getHubSettings()pending, verify the toggle cannot mutate, then resolvetrueand verify the rendered state and PUT payload.
HAPI Bot
Avoid rendering an interactive false switch while the hub GET is still in flight, which could overwrite an enabled preference on early click. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Findings
-
[Major] Settings overstate Grok coverage — the new copy says Grok sessions receive the summary contract, but injection exists only in
grokRemoteLauncher.ts; default terminal-created Grok sessions run locally, andgrokLocal.tshas no instruction path. The toggle and environment override therefore have no effect for the defaulthapi grokflow. Evidence:web/src/lib/locales/en.ts:721,cli/src/grok/grokRemoteLauncher.ts:309,cli/src/grok/grokLocal.ts:14,docs/guide/agents.md:193.Suggested fix:
'settings.general.sessionSummaryContract.desc': 'When on, Claude, Codex, OpenCode, and remote Grok sessions are asked to end each turn with an AGENT_NOTIFY_SUMMARY line for denser ready notifications. Off by default. Local Grok and Cursor are not covered yet. Applies to new/resumed sessions.',
Update the Chinese copy equivalently, or wire a supported local Grok instruction mechanism before advertising full Grok coverage.
Questions
- None.
Summary
Review mode: follow-up after new commits. One Major issue remains: the UI documents broader Grok support than the implementation provides.
Testing
GitHub test check passed. Not run locally (automation review; PR code was not executed).
HAPI Bot
Local Grok has no instruction inject path; settings copy now matches remote-only Grok support (and still excludes Cursor). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits. No issues found in the latest full diff. The prior Grok coverage finding is resolved. Residual risk: contract delivery across the supported agent flavors is covered mainly through helper/config tests rather than end-to-end agent launches.
Testing
Not run (automation). GitHub test check: IN_PROGRESS (pending).
HAPI Bot
Rebase onto main brought tiann#1376 unique tmp + outcome-shaped writers; wire sessionSummaryContract and the write-failure credential test to match. Co-authored-by: Cursor <cursoragent@cursor.com>
Absorb tiann#1376 session-summary hub toggle (opt-in) + tiann#1388/tiann#1390/tiann#1391. Keep soup Overseer/FUE/PR-awareness/runners on Settings → General. Co-authored-by: Cursor <cursoragent@cursor.com>
Rebase onto main brought tiann#1376 unique tmp + outcome-shaped writers; wire sessionSummaryContract and the write-failure credential test to match. Co-authored-by: Cursor <cursoragent@cursor.com>
…ice (#1392) * feat(settings): onboard hub transcription provider credentials in UI Env-only keys made dictation invisible; Settings can now add/edit/clear hub-side credentials (masked), with env still winning as override. Refs #1384. Co-authored-by: Cursor <cursoragent@cursor.com> * feat(settings): onboard voice-assistant backends alongside dictation Same Settings credential surface now covers ElevenLabs, Gemini Live, and Qwen Realtime (alias env pairs), not only transcription providers. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): address PR #1392 Major credential onboard findings Alias env locks, non-destructive Save (omit empty fields), and owner-only settings.json permissions for hub-stored provider secrets. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): harden credential onboard for second-pass Majors Owner-namespace gate, stage-then-sync env after persist, and per-field OpenAI-compatible editability under mixed env locks. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): serialize settings RMW and clear partial compatible creds Per-file settings lock for concurrent credential PUTs, and Clear shown for partial OpenAI-compatible entries (key/url/model alone). Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): serialize all settings writers via updateSettings Route credentials, relay auth, generators, server settings, and CLI token persistence through a locked RMW helper; reset Clear form state. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): share cross-process settings lock with CLI Extract withSettingsFileLock for hub+CLI, keep owner-only 0o600 rewrites, and race hub credential updates against CLI-style writers. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): keep UI secrets out of process.env; PID-own settings locks Settings-backed provider credentials now live in an in-memory overlay (getProviderEnvironment) so tunnel/ACP/Codex children do not inherit them. Settings file locks record pid+token and only reclaim dead or legacy locks. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): never reclaim ownerless settings lock sidecars wx creates the lock path before the owner JSON is visible; unlinking null owners let a waiter steal a live acquisition and collide on settings.json.tmp (CI ENOENT). Only reclaim parsed owners with dead PIDs. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): reclaim dead locks via rename; clean up failed publishes Stale reclaim renames the sidecar to a unique break path and re-verifies the expected dead owner before deleting it, so a loser cannot unlink a successor's live lock. Failed owner writes unlink the wx sidecar. Reclaim uses a sync owner read so contenders do not all observe one dead owner across an await and race the exclusive create. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): reclaim dead locks under exclusive reaper sidecar Stale reclaim now takes a fixed settings.json.lock.reap lock, re-validates pid+token, then unlinks — so a delayed contender cannot move a successor's live lock aside. Also document providerCredentials in settings.schema.json. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): fail closed on corrupt CLI settings; backoff busy reaper CLI updateSettings now uses a strict read that rejects invalid JSON instead of treating errors as {}, which could wipe providerCredentials. Settings lock reclaim sleeps when another process holds .reap so retries are not burned synchronously. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): publish locks via candidate+link; fix CLI vitest hoist Acquire settings locks by writing a complete candidate then linkSync to the fixed path so a crash cannot leave an empty live sidecar. Fix the CLI persistence regression test to create its temp dir inside vi.hoisted. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): replace bespoke lock with proper-lockfile; hide tenant creds UI Codex kept finding crash windows in hand-rolled lock sidecars. Switch the shared settings lock to proper-lockfile's mkdir + mtime lease. Hide the owner-only credentials editor from non-default namespaces on the voice page. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(settings): adapt sessionSummaryContract to outcome updateSettings Rebase onto main brought #1376 unique tmp + outcome-shaped writers; wire sessionSummaryContract and the write-failure credential test to match. Co-authored-by: Cursor <cursoragent@cursor.com> * chore: retrigger CI after rebase onto upstream/main Empty commit — Meta reported no checks on da0c6c2 after tip-forward rebase. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
AGENT_NOTIFY_SUMMARYemission stays optional indefinitely.POST/GET /cli/sessionsbootstrap (sessionSummaryContract) and injects the short contract at call time into Claude / Codex / OpenCode / Grok system or developer instructions.HAPI_SESSION_SUMMARY_CONTRACT=0|1still overrides the hub preference on the CLI process. Cursor ACP is intentionally not advertised (no upstream overlay seam yet). Parse/FCM path unchanged (feat(hub): native companion (FCM) push channel + device registry + pairing QR #803).Test plan
bun testhub:sessionSummaryContract+/api/hub-settings+ cli route smokebun run testcli:sessionSummaryInstruction+ claudeLocal + appServerConfig + relatedbun run typecheck:cli/typecheck:hub(web has a pre-existingrouter.tsxRawSendError error on upstream/main)Issues
Fixes #1375