Skip to content

Unify Settings, fit Model Pad, and add global Skills controls - #97

Open
sambitcreate wants to merge 6 commits into
mainfrom
feature/settings-unification
Open

Unify Settings, fit Model Pad, and add global Skills controls#97
sambitcreate wants to merge 6 commits into
mainfrom
feature/settings-unification

Conversation

@sambitcreate

@sambitcreate sambitcreate commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Changes

Workspace folder paths are now hidden by default. Appearance settings can show them using beginning/end, last-folder, or beginning-only formatting; duplicate workspace names remain distinguishable.

Settings now shares Appearance’s page hierarchy, grouped surfaces, row spacing, and trailing controls. Model Pad fits its actual available space across narrow windows and zoom levels, Telegram switches stay on the right, and Memory uses an SD-card icon. Brain UI glyphs have been replaced. Onboarding image assets are unchanged from the base branch.

The global Skills switch preserves individual choices while preventing skill discovery, attachment, tool execution, and hidden journal replay in chats. It cancels in-flight chat and manual-compaction snapshots, revalidates queued Telegram commands, and suppresses saved Bot skill grants without blocking ordinary Bot chats or unrelated access edits. Skill identities survive disabled catalog reads and app restarts; re-enabling detects real content changes. The additive catalog flag is handled by both native clients. Bot-specific catalog requests now pass through the authenticated HTTP route, and iOS caches remain isolated per Bot while new-Bot creation uses generic inventory.

Validation

  • After reverting the onboarding artwork replacement: all 50 onboarding tests and 57 Settings design tests passed.
  • Full npm test, production build, lint, renderer/E2E type checks, and complete Electron E2E passed after the final review fixes (16 passed; production-profile case runs separately in CI).
  • Electron tests cover path persistence, every Settings destination at 390/600/1280px, Model Pad window/zoom/panel states, keyboard movement/save, and the actual provider request after disabling Skills.
  • Runtime regressions cover stale skill invocations, real JSONL compaction/reopen, Telegram queues/menu refresh, and Bot admission. Final Remote API suite: 356 passed, 1 expected skip, plus 7 LAN transport tests. An initial occupied-port failure in the unchanged legacy-port test cleared on two full reruns.
  • Three independent fresh-context GPT-5.6 Sol medium reviews completed; confirmed findings fixed and regression-tested. Runtime re-review found no remaining actionable issues.
  • Android: 33 focused Bot contract and networking tests passed, including real HTTP request routing and malformed target rejection. iOS app/test bundle compiled; physical test execution was blocked by locked/unavailable connected iPhones (simulators are prohibited by repository policy).

In very short or highly zoomed windows, Model Pad retains a usable 160px minimum canvas and normal vertical scrolling keeps its controls and labels reachable.

Plan: docs/plans/completed/settings-unification-plan.md. All checks passed on implementation revision d38fbeda79dc80bc64febb79f47f50a8c4cc2167, confirmed by the GPT-5.6 Luna max CI watcher. Automated review reported no new issues. CI will rerun for the subsequent onboarding-artwork revert.

@pullfrog pullfrog Bot 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.

Important

Two enforcement gaps keep the disabled-Skills gate from matching the PR's claims: manual/operator compaction still sends raw journal content to the provider while the gate is closed, and Custom-Bot surfaces that don't thread the new skillsEnabled flag fail closed (or churn saved grants) across a disable/enable cycle. Details below.

Reviewed changes

  • Unified Settings design system — new SettingsPage wrapper with shared .settings-page-heading (owned by settings-view.tsx with a per-destination exclusion list), reworked FieldSet/Field in ui.tsx with settings-group/settings-field classes, new --settings-card-* token layer in styles.css, and content width widened to max-w-5xl; every destination now carries the Appearance heading/card/row language, with source-reading tests and a per-destination 390/600/1280px E2E sweep.
  • Workspace path preferencesshowWorkspacePaths (default off) + workspacePathFormat (middle|end|start) added to AppearanceConfig, with strict parse validation; new workspace-path-display.ts, grapheme-safe pathTextEdge truncation, canvas-measured WorkspacePathLabel, and a localStorage-cache hook; applied in the sidebar, picker, and accessible names, with destructive dialogs always showing the full target path.
  • Responsive Model Pad — a layout effect measures the actual scrollport (ResizeObserver up the ancestor chain) and sets --model-pad-available-size on the grid (160px floor, scrollTop-compensated); CSS moves to minmax(0,1fr) layouts; title/description folded into the shared page heading. Covered by a window/zoom/panel geometry E2E matrix.
  • Global Skills gateskillsEnabled setting with IPC boundary validation; skillRegistry gate checks before cache reuse and around disk scans plus an execution-time tool check; buildAgentTools withholds snapshots/tools when off; Bot inventory/runtime-authority gating with skillsEnabled drift suppression and tombstoned saved grants; projectVisibleHistoryWithoutSkills journal projection applied on the llmClient generation path (with current-turn prompt override blocked before injection); Telegram queue stores opaque skill provenance revalidated at dispatch and generation, with bounded setMyCommands refreshes; cancelForSkillsDisabled cancels streams and subagents on disable.
  • Icons/branding — SD-card MemoryCardIcon for Memory, brain glyphs removed from settings nav, onboarding bento, and voice setup; new lightbulb thinking-controls illustration.
  • Tests — new settings-design/workspace-path/skills source tests, bot-authority drift tests, skill-registry/tool disable tests, Telegram provenance tests, JSONL reopen compaction tests, two new E2E specs, mobile shared-contract tests for stale skill selections, and package.json script registrations.

⚠️ Operator/manual compaction still replays raw skill content while the gate is closed

context-lifecycle-service.compactChat (unchanged by this PR) opens the raw durable session and runs PiCompactionCoordinator against it with no skillsEnabled check and no projection, so an LLM-engine compaction builds its provider request from the full journal — including skill expansions, skill-tool results, and pre-disable compaction summaries that the disabled view exists to mask. It is reachable from desktop /compact (renderer/lib/slash-command-actions.tschats:compactcompactDesktopChat in context-lifecycle-adapters.ts) and from Telegram's /compact, and cancelForSkillsDisabled does not touch it. Repro: use a skill in a chat, disable Skills, then /compact that chat — the compaction request still contains the skill instructions, contradicting the Skills page copy "Hidden skill instructions won't be replayed" and the plan's claim that existing journals/compaction are covered.

Technical details
# Compaction bypasses the disabled-skills projection

## Affected sites
- main/services/context-lifecycle-service.ts:162-183 — opens the raw durable session via `deps.openSession(chat.id)`, then `syncChatMessagesToPiSession` + `new PiCompactionCoordinator({ session })` + `coordinator.compact()`; engine `"llm"` (the default, `renderer/shared/compaction.ts`) issues a provider request whose context is the full raw journal.
- main/services/context-lifecycle-service-main.ts:31-35 — production `openSession` resolves to `piCompactionSessionStore.openChat`, the same store the new projection wraps at generation time, but unwrapped here.
- Reachable from desktop `/compact` (renderer/lib/slash-command-actions.ts:291-295 → main handlers → context-lifecycle-adapters.ts:9-21) and Telegram `/compact` (telegram-service-core.ts ~1274 → telegram-service.ts compactChat → context-lifecycle-adapters.ts:41-43).
- The durable journal keeps the content: llmClient appends the expanded current-user message via `syncChatMessagesToPiSession(..., contentOverrides)` (main/services/llm-client.ts:2166-2174), and skill-tool results are appended by the harness. `projectVisibleHistoryWithoutSkills` only masks reads; it never deletes (pi-compaction-session-store.ts:282-327).

## Required outcome
- While `skillsEnabled === false`, every compaction whose context could contain old skill-expanded journal content must read through the same masked projection used at generation time — including the lifecycle/operator path — or be refused.

## Suggested approach
- In `context-lifecycle-service.compactChat` (after `openSession`, before `coordinator.compact()`), mirror llm-client.ts:2038-2041: when the effective settings read says skills are disabled, wrap the session with `projectVisibleHistoryWithoutSkills(session, chat.messages, model)` and compact the wrapper.
- Add an integration test that seeds a journal with an expanded skill turn, disables Skills, and asserts a manual/operator compaction's provider payload contains no skill content (analogous to the existing "skill-free visible context compacts" test at pi-compaction-core.test.ts:1652-1686).

⚠️ Custom-Bot flows that don't thread skillsEnabled drift-block, and grant churn across a disable→enable cycle

The new options.skillsEnabled drift suppression is threaded only through the runtime admission path; the pre-existing call sites that validate saved grants without the option still run skill checks against a gate-emptied catalog, so Custom Bots holding saved skill grants fail closed while Skills is disabled in ways the PR body says it avoids ("suppresses saved Bot skill grants without blocking ordinary Bot chats"): creating a chat for such a bot through the audience application flow, editing the bot in the access editor (any unrelated change is rejected unless the grants are first removed), and archived-bot reads. Separately, disable→enable windows can churn grants: if any disabled-window catalog read reconciles the gated-empty skill inventory (skills flip present:false), re-enable re-mints incarnations, so the first enabled admission reports changed_or_removed for a skill that never changed. The new runtime-authority tests exercise re-enable by swapping in a prebuilt snapshot, which never runs that reconcile, so this isn't covered.

Technical details
# Custom-Bot paths omit skillsEnabled; re-enable window may churn grants

## Affected sites
- main/services/bot-application-service.ts:1019-1028 — audience chat creation calls `assertAuthorityBindingsCurrent({ botId, snapshot })` with no `skillsEnabled`; while the gate is closed the catalog's skills list is empty and bound skills surface as tombstones (`available:false`), so drift throws (`unavailable`) and the chat cannot be created.
- renderer/main/bots-view.tsx:203-213 — `buildBotAccessUpdate` asserts every selected skill is `available` in the catalog; while the gate is closed any save of a Custom bot with saved skill grants (even unrelated provider/file changes) is rejected until the user deselects the grants.
- main/services/bot-capability-bindings.ts:1250, 1342 — the drift option defaults to `undefined` (checks still run); only the runtime admission path passes `{ skillsEnabled }`, so suppression is inconsistent across call sites.
- Suspected churn: bot-capability-store-core.ts:1558-1582 (skills reconciled as `present:false` during disabled reads can be deleted and re-minted with a new `resourceIncarnation` on re-enable, changing `exactFingerprint``changed_or_removed`). The PR's own re-enable test (bot-runtime-authority.test.ts:554-575) uses `fixture.setSnapshot(enabledSnapshot)` and `setSkillsEnabled(true)` — a prebuilt snapshot — so it cannot observe incarnation reconcile behavior.

## Required outcome
- While the gate is closed, saved Custom-Bot skill grants must suppress everywhere they surface (audience pre-flight, editor saves, chat reductions), not only in the runtime resolver — otherwise bot editing/chat creation breaks for exactly the users who disabled Skills.
- Re-enabling must not report genuine drift for skills that never changed; saved grants must be usable again without a forced re-save.

## Suggested approach
- Thread `skillsEnabled` (or a single "effective skills" predicate) into the drift/validate call sites listed above, matching the runtime admission behavior.
- Add an integration scenario through the real incarnation store: disable → one disabled-window catalog read → re-enable → admit, asserting no false `capability_changed` and that saved grants still bind.

Pullfrog  | Fix it ➔View workflow run | Using DeepSeek Flash𝕏

@pullfrog pullfrog Bot 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.

ℹ️ No new issues in this delta — it renames duplicate settings group headings and stabilizes the Model Pad E2E check. The PR is still not ready to approve: both findings from the previous review (operator/manual compaction bypassing the disabled-skills projection, and Custom-Bot paths that omit skillsEnabled) remain open at this head, since this commit touches neither path.

Reviewed changes

  • Renamed duplicate group headings — About→Application, Computer Use→Desktop control, Memory→Memory controls, Remote Access→Connection (loading and loaded states), Scheduled tasks→Background execution — so each destination page no longer repeats its own name as a group heading.
  • Strengthened heading coveragesettings-unification.spec.ts asserts exactly one heading per destination after navigation, settings-model-picker.spec.ts moves destination checks to level-1 headings, and settings-design.test.tsx bans FieldSet group titles duplicating the About/Memory/Remote Access/Scheduled tasks destination names.
  • Updated the workspace-actions selector in chat-shell-interactions.spec.ts to the exact accessible name, now that path suffixes are hidden by default.
  • Reworked the Model Pad reachability poll — re-centers the pad on each retry while the layout settles, samples the following frame, and asserts fit against the actual scrollport∩viewport intersection with geometry diagnostics included in failures.
  • Recorded CI context in .papercuts/troubleshooting.md, the unification plan, and AGENTS.md.

Pullfrog  | Fix it ➔View workflow run | Using DeepSeek Flash𝕏

@pullfrog pullfrog Bot 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.

Important

Both findings from the previous reviews are addressed with strong integration tests, but the new disabled-gate tombstone mechanism is unreachable from the mobile clients: while Skills is off the audience catalog (the only catalog iOS and Android ever fetch) is skill-free, so a Custom bot holding saved skill grants cannot be saved through the mobile access editor — and the new iOS/Android contract tests mask the deadlock by feeding the clients a tombstone-bearing catalog the Mac never sends.

Reviewed changes — commit 4ad6e5ec ("Enforce global Skills gate across compaction and saved Bot access") since the prior review at 6278ff4d:

  • Gated operator compactioncompactChat wraps the opened session in projectVisibleHistoryWithoutSkills when Skills is off and registers every compaction in an abortable map; disabling Skills cancels in-flight compactions via the settings side effect. Real-JSONL provider-payload tests prove masked context and no committed entry on cancel.
  • Per-bot tombstone catalogs — while disabled, targeted catalog reads retain the bot's saved skill IDs as available:false presentation choices, and retainedSkillIds validation preserves exact saved subsets while rejecting every new grant.
  • Consistent drift suppression — drift and currentness checks now fall back to the snapshot's skillsEnabled, so audience pre-flight, admission, archive reads, and unrelated editor saves no longer drift-block while the gate is closed.
  • Paused inventory is never reconciled as removed — skill discovery is double-gated and skips the incarnation store; the production-shape test drives disable → reads → archive/restore → restart → re-enable over the real store and proves incarnations and exact fingerprints survive, while a genuinely changed skill still fails admission.
  • Strict optional skillsEnabled wire flag — omitted means enabled; malformed values fail decoding across Mac/iOS/Android, with OpenAPI, docs, and shared fixtures updated in lockstep.
  • Targeted desktop catalog plumbingbots:getCapabilityCatalog(botId?) IPC, per-bot editor/summary reads, and a Skills section that explains the global-off state while keeping saved choices visible.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash𝕏

Comment thread main/services/bot-application-service.ts

@pullfrog pullfrog Bot 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.

✅ No new issues found.

The mobile-seam finding from the prior review is resolved: disabled-Skills tombstones now reach iOS and Android over the real wire, so existing-Bot access editors can preserve or remove saved grants instead of deadlocking Save — and the new transport tests exercise the exact request shapes both clients emit.

Reviewed changes — commit d38fbeda ("fix: route mobile Bot catalogs to scoped saved choices") since the prior review at 9104606b:

  • Routed targeted catalog reads through the authenticated HTTP routeGET /bot-capabilities?botId=… accepts one exact, charset/length-validated botId query (unknown Bots 404; duplicate, empty, malformed, and overlong values fail 400), with a matching BotIdQuery OpenAPI parameter, protocol tests, and API docs; the query-less generic path is unchanged.
  • Sent scoped requests from every existing-Bot iOS flow — custom-access load and save-conflict rebase, editor edit load and rebase, chat tools, and conversation files now call botCapabilityCatalog(botId:) with client-side identifier validation; only new-Bot creation still uses the generic inventory.
  • Isolated iOS offline catalogs per BotAidenBotCacheSnapshot/Segments gained catalogsByBotID with strict bounds and pruning on list refresh; catalog(forBotID:) never falls back to the generic slot, legacy snapshots without scoped data decode cleanly, and scoped fetches never overwrite the generic creation catalog.
  • Fixed the Android transport pathbotCapabilityCatalog(botId:) validates identifiers before sending, with tests asserting the exact generic-vs-targeted request paths and no network I/O for unsafe targets (this also unblocks Android's previously-400'd access-screen fetches).
  • Pinned the routing contract — the iOS shipping-target policy test statically asserts the scoped call-site and cache-scope layout, and router/bots tests drive real HTTP requests including per-target tombstone projections and the 400/404 rejection matrix.

Verified independently: both prior [!IMPORTANT] findings (compaction projection, bot drift/churn) remain correctly addressed at this head, and specialist iOS/Android source reviews plus the focused aiden-remote suites (router, bots, protocol, bot-application-service) found no regressions in this delta.

Pullfrog  | View workflow run | Using DeepSeek Flash𝕏

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