feat: AppShell application platform layer#8
Merged
Conversation
Phase 0 of the app platform plan (docs/learned/app-platform-plan.md): - gpui-component-storage: platform paths, atomic/durable writes, schema-versioned envelopes with four-way load outcomes, single-writer advisory locking (lock owned by the store worker thread), debounced stores with backup rotation and corrupt-primary recovery - gpui-component-manifest: app identity schema and validation, MSIX/CFBundle version derivation, app-local build.rs codegen (emit_identity + include_identity!) with a downstream-workspace fixture proving identity resolves from the consuming app's manifest - CI: mandatory 3-OS platform-crate test step; native-launch smoke placeholder for phase 1 - workspace: shared package version; docs/COMPATIBILITY.md matrix; update-gpui skill owns version/tag/compat bookkeeping - scaffold gpui-component-app crate for phase 1 - fix app_assets example missing gpui-component-assets dependency Reviewed: codex gpt-5.6-sol structured review, two rounds, 14 findings fixed (settings-file loss on failed commit, pending-update loss, writer lock lifetime, backup recovery outcomes, UTF-8 corruption classing, path traversal validation, MSIX store rules, CFBundleIdentifier grammar).
Phase 1 of the app platform plan (docs/learned/app-platform-plan.md): gpui-component-app, the application shell that lets a new app's main.rs be ~20 lines while supporting multi-window, single-window, and tray-first zero-window shapes. Core: - AppShell builder over a sealed plugin/phase mechanism with a central startup sequencer and reverse-order shutdown (partial-init unwind) - lifecycle as an AppEvent stream: early platform-listener registration, queue-until-ready delivery, reentrancy-safe dispatch, single request_quit path with accurate ShutdownReason attribution - explicit thread affinity: AppInfo/AppProxy (Send+Sync; proxy closes at the shutdown boundary), main-thread ShellState global via AppShellExt - liveness as hold/release leases; activation policy (Passive for tray-first apps); WhenIdle/Explicit exit policies; startup-idle exit - PlatformCapabilities with honest per-target judgements for fork stubs Services (always compiled, builder-activated): - windows: WindowSpec/WindowKey, auto-Root wrap via RootPolicy, singleton state machine, per-title numbering, overlay surfaces, close-driven registry reconciliation, standard window command handlers - settings: typed named stores on gpui-component-storage with stepwise migrations, FutureVersionPolicy::RefuseToWrite, debounced saves with shutdown flush; platform-owned ShellPreferences store - commands: registry with stable IDs; native app/edit/window menus, dock menu, and theme sections as projections; precedence-preserving keymap rebuilds for dynamic registration - theme: ThemeSource (bundled sync / registry watch / custom palettes), selection persistence via ShellPreferences, menu integration Proof and tooling: - examples/app_shell and examples/app_shell_tray conformance apps with --smoke CI mode (exit 0 iff a window opened and quit was clean) - gpui-app-doctor: verifies packaging artifacts against the manifest - CI native-launch-smoke job enabled (macOS launch; Linux/Windows documented-off pending display/GPU verification) - fix pre-existing workspace breakage (system_monitor disallowed timer, tiles.rs Menu field) Reviewed: codex gpt-5.6-sol structured review, five cycles, 24 findings fixed, 2 rejected with fork-source evidence (fork's global_mut notifies; no Copy derive existed). Implementation by opus + codex agent team.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe change adds manifest, storage, and application-shell crates. It introduces identity generation and verification, persistent storage, lifecycle and service APIs, menus, themes, windows, conformance examples, downstream tests, and CI smoke coverage. ChangesApp Platform Foundation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/update-gpui/SKILL.md:
- Around line 50-53: Update the version and compatibility bookkeeping procedure
in .claude/skills/update-gpui/SKILL.md to include creating and pushing the
annotated gpui-vYYYY.MM.DD-<shortsha> fork tag alongside the workspace tag, or
reference the automation that performs it. Align the fork-tag requirement in
docs/COMPATIBILITY.md with this executable procedure; update both sites as
needed.
In @.github/workflows/ci.yml:
- Around line 101-102: Harden the CI action usage in the affected job by pinning
both actions/checkout and actions-rust-lang/setup-rust-toolchain to immutable
commit SHAs. Configure checkout with persist-credentials disabled and disable
automatic caching in setup-rust-toolchain, matching the required security
posture and applying the same fix to the duplicate action usage in the test job.
In `@crates/app-manifest/src/doctor.rs`:
- Around line 246-283: Update verify_desktop and its desktop_value lookups so
Name, Exec, StartupWMClass, and Icon are read exclusively from the [Desktop
Entry] section, preventing Desktop Action sections from satisfying launcher
fields. Apply the same restriction to the additional affected block and preserve
the existing comparison and presence-reporting behavior.
- Around line 109-147: Update the entitlement handling in the manifest scan to
read and parse every candidate entitlements.plist file rather than treating its
filename as sufficient. Aggregate the entitlement keys across matching files and
compare them against every entry in identity.macos.entitlements, reporting each
declared entitlement as present only when found; preserve the missing-report
behavior when no candidate satisfies it. Add coverage for empty and unrelated
entitlement files, including equivalent platform-specific branching tests where
applicable.
- Around line 204-208: Update the icon validation in the bundle manifest parsing
flow to accept only nonblank string values. Replace the current array acceptance
in the icon filter with validation that rejects arrays, empty strings, and
whitespace-only strings while preserving valid nonblank string entries.
In `@crates/app-manifest/src/schema.rs`:
- Around line 140-157: Update AppManifest::validate to reject an explicitly
provided empty data_namespace, while preserving the existing handling for
omitted namespaces; return the appropriate manifest validation error and add a
regression test covering data_namespace = "" at the manifest boundary.
In `@crates/app-manifest/src/versions.rs`:
- Around line 70-79: Add boundary-focused tests for the MSIX component
validation loop in the relevant manifest version tests: verify 65535 is accepted
and 65536 returns ManifestError::InvalidMsixComponent for each component,
including the sideload build value. Ensure the cases exercise both the normal
version fields and the build component branch.
In `@crates/app-manifest/tests/downstream.rs`:
- Around line 8-16: Make the downstream fixture dependency resolution
reproducible: in crates/app-manifest/tests/downstream.rs lines 8-16, remove the
fresh-resolution rationale and add --locked to the cargo run arguments; in
crates/app-manifest/tests/fixtures/downstream-app/.gitignore line 1, remove the
/Cargo.lock entry and commit the generated fixture lockfile.
In `@crates/app-storage/src/atomic.rs`:
- Around line 75-92: The write_impl staging flow must preserve the target’s
restrictive permissions during atomic replacement. Before renaming the staged
file, read the existing path’s permissions when available, otherwise default new
files to 0600, then apply those permissions to the staging file before it
replaces the target.
In `@crates/app-storage/src/envelope.rs`:
- Around line 180-208: Make archive_corrupt in
crates/app-storage/src/envelope.rs require collision-safe quarantine: avoid
overwriting an existing archive, and propagate a failure when the rename cannot
succeed so corruption handling prevents subsequent writes. Update the rotation
logic around the affected store path in crates/app-storage/src/store.rs (lines
419-449) to never copy an unvalidated primary into .bak and to account for a
prior failed quarantine before writing or rotating files.
In `@crates/app-storage/src/paths.rs`:
- Around line 255-259: Update Paths::sub to validate name with validate_relative
before joining it to the selected base directory, preventing absolute paths and
parent traversal from escaping the base. Change the return type to
Result<PathBuf, StorageError> and propagate validation errors while returning
the joined path on success.
- Around line 294-299: Update platform_default_creates_nothing to use a unique
test namespace, assert the corresponding config directory is absent before
constructing AppPaths, and assert it remains absent after construction. Remove
the current exists/read_dir condition, while preserving the
PathLayout::PlatformDefault setup and cleanup expectations.
In `@crates/app-storage/src/store.rs`:
- Around line 203-227: Update the backup recovery branch in the store load
method around load_envelope so a corrupt primary archived to archived_to does
not leave subsequent loads unable to find valid backups. Either treat a Missing
result from a backup probe as eligible for recovery where appropriate, or
atomically restore the selected usable backup to the primary path before
returning; preserve migration and future-version outcomes.
- Around line 214-216: Update the backup-probing match in the store recovery
flow so the wildcard arm no longer handles I/O errors. In the surrounding
backup-generation logic, continue only for Missing and Corrupt outcomes, and
propagate genuine I/O errors to the caller instead of initializing defaults over
inaccessible data.
In `@crates/app-storage/tests/process_lock.rs`:
- Around line 110-129: The child-process wait around the locked-state reader
must enforce the timeout and cleanup on every exit path. Replace the blocking
read_line loop with a reader thread and channel consumed via recv_timeout, and
add a cleanup guard for the holder process that signals or kills it and always
reaps it, including assertion and early-break paths.
In `@crates/app-storage/tests/store.rs`:
- Around line 171-207: Replace the permission-based failure setup in
worker_write_error_is_surfaced at crates/app-storage/tests/store.rs:171-207 with
deterministic, portable write-failure injection, preserving the assertions that
flush returns an error and last_error retains it. Apply the same
failure-injection mechanism to the primary-preservation test at
crates/app-storage/tests/store.rs:209-246, removing its chmod-based setup and
keeping its existing preservation assertions unchanged.
- Around line 98-117: The backup rotation test backup_generations_rotate
currently checks only file existence, so it must validate generation ordering
and contents. Deserialize each store file and assert the primary contains
document generation 3, .bak contains 2, .bak.1 contains 1, and .bak.2 contains
0, preserving the existing rotation setup and assertions.
In `@crates/app/src/capabilities.rs`:
- Around line 61-76: Update the capability documentation in the judgments
section and the corresponding capability definitions around the reported range
to include actionable fallback guidance for every unsupported capability. For
each platform limitation, state the practical workaround, such as using regular
windows instead of overlays, manually registering URL schemes, using an
alternative tray or credential flow, and restoring windows without absolute
positioning on Wayland; preserve the existing support classifications.
In `@crates/app/src/commands/plugin.rs`:
- Around line 143-158: Make command binding validation fallible before
load_binding calls KeyBinding::load: update the command builder/registration
flow, including with_binding and register_command, to parse or validate the
keystroke and return a structured AppShellError for malformed bindings.
Propagate this error through initial menu setup and post-launch rebuilds, and
remove the expect-based panic in load_binding while preserving valid binding
registration.
In `@crates/app/src/defaults.rs`:
- Around line 21-41: The new AppShellBuilder entry points lack focused coverage.
Add test_settings_builder, test_theme_builder, and test_menus_builder tests that
verify SettingsPlugin registration, persisted theme selection through
read_theme_preference/write_theme_preference, and ThemeMenuBridge menu-section
projection respectively, using the public builder methods settings, theme, and
menus.
In `@crates/app/src/error.rs`:
- Around line 54-58: Remove the blanket
From<gpui_component_storage::StorageError> for AppShellError implementation in
error.rs. At the AppPaths::new call in the shell initialization flow, explicitly
map only that path-resolution error to AppShellError::Paths while preserving
other StorageError propagation and classifications.
In `@crates/app/src/handles.rs`:
- Around line 329-354: Update spawn_drain_loop to cap the number of commands
processed in each foreground drain pass, then yield or wait for the next poll
interval before continuing even when commands remain queued. Also add explicit
backpressure or command coalescing at the ProxyCommand producer boundary so the
unbounded channel cannot grow without limit, while preserving closed-proxy
checks between callbacks and before sleeping.
In `@crates/app/src/lifecycle.rs`:
- Around line 109-121: Update Lifecycle::push to return the AppEvent when the
shell is ready, preserving ownership for immediate delivery, and return no event
when buffering it in pending. Adjust the method’s return type, ready and
not-ready branches, and documentation to describe the returned event/absence
instead of a boolean.
In `@crates/app/src/settings/runtime.rs`:
- Around line 130-160: Update flush_for_exit so exit_flushed is set only after
the flush result succeeds, never immediately after spawning the worker. Ensure
timeout, worker failure, and flush errors leave the operation retryable while
preventing concurrent retries; use an explicit lifecycle state such as
NotStarted/InFlight/Completed if needed, and preserve the existing
lifecycle_error reporting.
In `@crates/app/src/windows/spec.rs`:
- Around line 18-29: Remove the Raw variant from RootPolicy in
crates/app/src/windows/spec.rs. In crates/app/src/windows.rs, remove
WindowManager::open_raw, ensure overlay content is wrapped in
gpui_component::Root, and adjust the creation method’s return type to expose
both the Root window handle and content entity; every window path must use Root
as its first-level view.
- Around line 59-155: Add test_window_spec_builder and test_overlay_spec_builder
for the two public specification builders. Verify default values, every mutator
including title, sizing, root/background policies, and hooks, plus invalid and
boundary sizing inputs; assert hook registration and resulting builder state
without invoking platform-specific behavior.
In `@docs/learned/app-platform-plan.md`:
- Around line 328-330: Update the conformance-example references in the plan
from examples/app-shell and examples/app-shell-tray to the actual repository
paths examples/app_shell and examples/app_shell_tray, preserving the surrounding
descriptions and documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 218e12e4-656e-4eba-9095-406661da1b6d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
.claude/skills/update-gpui/SKILL.md.github/workflows/ci.ymlCargo.tomlcrates/app-manifest/Cargo.tomlcrates/app-manifest/src/bin/gpui-app-doctor.rscrates/app-manifest/src/build.rscrates/app-manifest/src/doctor.rscrates/app-manifest/src/error.rscrates/app-manifest/src/lib.rscrates/app-manifest/src/parse.rscrates/app-manifest/src/schema.rscrates/app-manifest/src/versions.rscrates/app-manifest/tests/downstream.rscrates/app-manifest/tests/fixtures/downstream-app/.gitignorecrates/app-manifest/tests/fixtures/downstream-app/Cargo.tomlcrates/app-manifest/tests/fixtures/downstream-app/build.rscrates/app-manifest/tests/fixtures/downstream-app/src/main.rscrates/app-storage/Cargo.tomlcrates/app-storage/src/atomic.rscrates/app-storage/src/envelope.rscrates/app-storage/src/error.rscrates/app-storage/src/lib.rscrates/app-storage/src/lock.rscrates/app-storage/src/paths.rscrates/app-storage/src/store.rscrates/app-storage/tests/process_lock.rscrates/app-storage/tests/store.rscrates/app/Cargo.tomlcrates/app/src/capabilities.rscrates/app/src/commands/menu.rscrates/app/src/commands/mod.rscrates/app/src/commands/plugin.rscrates/app/src/commands/standard.rscrates/app/src/defaults.rscrates/app/src/error.rscrates/app/src/handles.rscrates/app/src/lib.rscrates/app/src/lifecycle.rscrates/app/src/liveness.rscrates/app/src/phases.rscrates/app/src/plugin.rscrates/app/src/settings.rscrates/app/src/settings/runtime.rscrates/app/src/settings/tests.rscrates/app/src/shell.rscrates/app/src/theme.rscrates/app/src/windows.rscrates/app/src/windows/key.rscrates/app/src/windows/registry.rscrates/app/src/windows/spec.rscrates/app/tests/headless.rscrates/story/examples/tiles.rsdocs/COMPATIBILITY.mddocs/learned/app-platform-plan.mdexamples/app_assets/Cargo.tomlexamples/app_shell/Cargo.tomlexamples/app_shell/build.rsexamples/app_shell/src/main.rsexamples/app_shell_tray/Cargo.tomlexamples/app_shell_tray/build.rsexamples/app_shell_tray/src/main.rsexamples/system_monitor/src/main.rs
Comment on lines
+50
to
+53
| 8. **Version + compatibility bookkeeping** (this skill is the ONLY place a gpui rev changes — see docs/learned/app-platform-plan.md D6): | ||
| - Bump `[workspace.package] version` in the root `Cargo.toml` (minor bump for a gpui rev change or breaking API change while pre-1.0; patch otherwise). Keep the `version = "X.Y.Z"` values inside `[workspace.dependencies]` for the path crates (`gpui-component`, `gpui-component-*`) in sync. | ||
| - Add a row to `docs/COMPATIBILITY.md` pairing the new workspace version with the new gpui rev. | ||
| - After the change lands on `main`, create the annotated tag: `git tag -a v<version> -m "gpui-component v<version> (gpui <shortsha>)"` and push it. Apps pin these tags — never bare revs. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files '.claude/skills/update-gpui/SKILL.md' 'docs/COMPATIBILITY.md' 'docs/learned/app-platform-plan.md'
printf '\n== update-gpui skill (relevant lines) ==\n'
sed -n '40,70p' .claude/skills/update-gpui/SKILL.md | cat -n
printf '\n== compatibility doc (relevant lines) ==\n'
sed -n '1,60p' docs/COMPATIBILITY.md | cat -n
printf '\n== search for gpui tag workflow ==\n'
rg -n --hidden --no-messages 'gpui-vYYYY\.MM\.DD|annotated tag|create the annotated tag|push it|tag creation|release automation|update-gpui' .claude docs Cargo.tomlRepository: BumpyClock/gpui-component
Length of output: 5187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline .claude/skills/update-gpui/SKILL.md || true
printf '\n== surrounding lines in skill ==\n'
sed -n '1,120p' .claude/skills/update-gpui/SKILL.md | cat -n
printf '\n== related docs/learned reference ==\n'
sed -n '1,220p' docs/learned/app-platform-plan.md | cat -nRepository: BumpyClock/gpui-component
Length of output: 18664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '40,70p' .claude/skills/update-gpui/SKILL.md | cat -n
printf '\n---\n'
sed -n '1,60p' docs/COMPATIBILITY.md | cat -n
printf '\n---\n'
rg -n 'gpui-vYYYY\.MM\.DD|annotated tag|update-gpui|release automation' .claude docs Cargo.tomlRepository: BumpyClock/gpui-component
Length of output: 4968
Complete the gpui fork tag step in the update flow.
The release discipline expects both the fork tag and the workspace tag, but this procedure only documents the workspace tag.
.claude/skills/update-gpui/SKILL.md#L50-L53: add thegpui-vYYYY.MM.DD-<shortsha>tag creation/push step, or point to the release automation that creates it.docs/COMPATIBILITY.md#L24-L27: keep the fork-tag requirement aligned with the executable procedure.
🧰 Tools
🪛 LanguageTool
[grammar] ~50-~50: Ensure spelling is correct
Context: ...eping** (this skill is the ONLY place a gpui rev changes — see docs/learned/app-plat...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~51-~51: Ensure spelling is correct
Context: ...the root Cargo.toml (minor bump for a gpui rev change or breaking API change while...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~52-~52: Ensure spelling is correct
Context: ... the new workspace version with the new gpui rev. - After the change lands on main,...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
📍 Affects 2 files
.claude/skills/update-gpui/SKILL.md#L50-L53(this comment)docs/COMPATIBILITY.md#L24-L27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/update-gpui/SKILL.md around lines 50 - 53, Update the version
and compatibility bookkeeping procedure in .claude/skills/update-gpui/SKILL.md
to include creating and pushing the annotated gpui-vYYYY.MM.DD-<shortsha> fork
tag alongside the workspace tag, or reference the automation that performs it.
Align the fork-tag requirement in docs/COMPATIBILITY.md with this executable
procedure; update both sites as needed.
Owner
Author
There was a problem hiding this comment.
Real but out of scope here (release-process docs in the update-gpui skill + COMPATIBILITY.md, not code touched by this PR). Tracking as a follow-up to add the fork-tag step / point at the automation that creates it.
There was a problem hiding this comment.
@BumpyClock, acknowledged. This is a valid release-process follow-up, but it does not need to block this App Platform PR. If no follow-up issue exists yet, would you like me to open one for the fork-tag/automation documentation alignment?
Comment on lines
+101
to
+102
| - uses: actions/checkout@v4 | ||
| - uses: actions-rust-lang/setup-rust-toolchain@v1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Unpinned actions + no persist-credentials: false + cache-poisoning surface.
zizmor flags actions/checkout@v4 (no persist-credentials: false, unpinned) and actions-rust-lang/setup-rust-toolchain@v1 (unpinned, auto-caching enabled by default → cache-poisoning surface) on these lines. This duplicates the same unpinned pattern already present in the test job above (lines 29-30), so it's a repo-wide gap being extended rather than a new regression, but worth addressing here since it's touched by this PR.
🔒 Suggested hardening
- - uses: actions/checkout@v4
- - uses: actions-rust-lang/setup-rust-toolchain@v1
+ - uses: actions/checkout@v4 # pin to a commit SHA
+ with:
+ persist-credentials: false
+ - uses: actions-rust-lang/setup-rust-toolchain@v1 # pin to a commit SHA
+ with:
+ cache: false🧰 Tools
🪛 zizmor (1.26.1)
[warning] 101-101: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 101-101: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 102-102: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 102-102: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 101 - 102, Harden the CI action usage
in the affected job by pinning both actions/checkout and
actions-rust-lang/setup-rust-toolchain to immutable commit SHAs. Configure
checkout with persist-credentials disabled and disable automatic caching in
setup-rust-toolchain, matching the required security posture and applying the
same fix to the duplicate action usage in the test job.
Source: Linters/SAST tools
Owner
Author
There was a problem hiding this comment.
Out of scope for this PR. As noted, this is a repo-wide gap — the same unpinned pattern is in the pre-existing test job. Pinning actions to commit SHAs plus persist-credentials: false should be a dedicated repo-wide CI-hardening change; tracking as a follow-up rather than partially applying it here.
…forms The headless bootstrap test drove a quit through `request_quit()` to end the run loop after its assertions passed. That only terminates the process on macOS, where the headless platform's `quit()` routes through `NSApp terminate`. On Linux and Windows the headless run loop neither terminates the process nor returns to `main` after `cx.quit()`, so the test printed its success line and then hung until the 30s watchdog fired the job red. Terminate with an explicit `std::process::exit(0)` at the point success is fully known instead of relying on platform loop-return semantics. Quit and teardown ordering is already covered by the pure unit tests; this test only asserts the boot lifecycle reaches `on_launch` with a working shell global, so the launched-flag bookkeeping that verified that after `run` returned is now redundant and removed.
… joins Addresses CodeRabbit findings on PR #8, grouped for the storage crate: - atomic.rs no longer imports `File` unconditionally. It was only used inside the `#[cfg(unix)]` directory-fsync path, so the Windows `-D warnings` build failed on an unused import. Qualify it at the use site instead. This is the Windows CI failure (Lint and native-launch-smoke both tripped on it). - Atomic replacement now preserves the target's permissions. The staging file was created under the process umask (typically 0644), so renaming it over a 0600 settings/secrets file silently widened it to world-readable. New files get a restrictive 0600 default; genuine stat failures are propagated. - Backup probing during corrupt-primary recovery no longer swallows I/O errors. The `_ => continue` arm treated a permission/device failure like an absent backup, which could let a caller initialize defaults over still-recoverable data. Only Missing/Corrupt are skipped; real errors surface. - `AppPaths::sub` validates its `name` (rejecting empty, absolute, and `..`) and returns a `Result`, so a caller-supplied component can no longer discard or escape the selected base directory. Tests cover permission preservation on overwrite, sub-path rejection, a unique-namespace check that actually detects on-disk creation, and backup generation contents/order (not just existence).
Addresses CodeRabbit findings on PR #8, grouped for the manifest crate: - doctor now verifies each declared macOS entitlement key against the union of every entitlements file's contents, rather than reporting success for a matching filename alone. An empty or wrong-keyed helper entitlement no longer satisfies a required entitlement. - doctor's `.desktop` reader restricts launcher-field lookups to the `[Desktop Entry]` group. Keys in `[Desktop Action ...]` groups could otherwise satisfy a missing Name/Exec/Icon and produce a false success. - doctor's bundle-icon check rejects blank strings and arrays containing empty or non-string entries (e.g. `[""]`, `[1]`), which previously passed. - `AppIdentity::validate` rejects an explicitly empty `data_namespace` at the manifest boundary. An omitted namespace still derives from `app_id`; only an explicit empty value is rejected, instead of passing here and failing later in `AppShell`. Adds unit tests for the desktop-group gating, entitlement key presence, the explicit-empty namespace case, and MSIX component boundaries (65535 accepted, 65536 rejected for both patch and sideload build).
Addresses CodeRabbit findings on PR #8, grouped for the app crate: - Drop the blanket `From<StorageError> for AppShellError`, which mapped every storage failure to `AppShellError::Paths`. Only path resolution is a Paths error; the one call site (`AppPaths::new`) now maps explicitly, so a future write/lock/corruption error reached via `?` cannot be misclassified. - `EventQueue::push` returns `Some(event)` / `None` instead of a `bool`. The bool signalled "deliver now" but consumed the event, so a ready caller could not actually perform the documented immediate delivery. Handing the event back makes the ready path usable. - `flush_for_exit` marks the exit flush complete only after a flush actually succeeds. It previously set the flag before awaiting the result, so a timeout or worker error left the shell believing the flush had completed and suppressed the persistence failure on the next call — violating the "report failure" half of the shutdown persistence contract. Adds `test_window_spec_builder` / `test_overlay_spec_builder` covering the two public window specifications (defaults, mutators, hooks, sizing guards).
The plan referenced `examples/app-shell` and `examples/app-shell-tray`, but the crates and directories are `app_shell` and `app_shell_tray`. Align the doc so the copy/paste build and CI commands resolve.
The `typos` CI check flags "unparseable" in two envelope.rs doc comments. Use "unparsable" so the gate passes. (Note: the macOS Typo-check job also failed at exit 127 because `cargo install typos-cli` did not put the binary on PATH and the `|| echo` masked it — that is a separate workflow-infra issue, tracked for the lead.)
…arge_err CI's Windows lint (newer clippy than the local toolchain) rejects `DoctorError` under `-D warnings` for `clippy::result_large_err`: the `Err` variant rode in every `Result<_, DoctorError>` on the doctor happy path at ~120 bytes, driven by the inlined `ManifestError` (120) and `toml::de::Error` (88). This surfaced only now that the earlier unused-import error stopped compilation before the doctor bin was linted. Box both heavy payloads so the error is ~40 bytes. `Manifest` drops its `#[from]` (the boxed field would derive `From<Box<..>>`, not `From<..>`), so the one call site maps the error explicitly. A `const` size assertion pins the type small to keep the lint from regressing on toolchains where it fires.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/app-storage/src/atomic.rs (1)
90-112: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply permissions before the durable
sync_all.
write_atomic_durablesyncs the staging file beforeset_permissions, so the restrictive mode change is outside the durability barrier. A crash can leave the renamed inode with its original umask-derived mode despite a successful durable write. Movepreserve_permissionsinside the staging closure beforefile.sync_all().Proposed fix
let mut file = OpenOptions::new() .write(true) .create_new(true) .open(&temp) .map_err(|e| StorageError::io(&temp, e))?; + #[cfg(unix)] + preserve_permissions(path, &temp)?; file.write_all(bytes) .map_err(|e| StorageError::io(&temp, e))?; if durable { file.sync_all().map_err(|e| StorageError::io(&temp, e))?; } Ok(()) })(); - #[cfg(unix)] - if let Err(e) = preserve_permissions(path, &temp) { - let _ = fs::remove_file(&temp); - return Err(e); - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/app-storage/src/atomic.rs` around lines 90 - 112, Move the Unix-only preserve_permissions call into the staging closure in write_atomic_durable, after writing and before the durable file.sync_all() barrier. Remove the later post-closure permission block, while retaining temp-file cleanup and error propagation for permission failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/app-storage/src/atomic.rs`:
- Around line 90-112: Move the Unix-only preserve_permissions call into the
staging closure in write_atomic_durable, after writing and before the durable
file.sync_all() barrier. Remove the later post-closure permission block, while
retaining temp-file cleanup and error propagation for permission failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eda5452c-6a7a-4b1a-ab87-68f8e1c6d733
📒 Files selected for processing (17)
crates/app-manifest/src/bin/gpui-app-doctor.rscrates/app-manifest/src/doctor.rscrates/app-manifest/src/error.rscrates/app-manifest/src/schema.rscrates/app-manifest/src/versions.rscrates/app-storage/src/atomic.rscrates/app-storage/src/envelope.rscrates/app-storage/src/paths.rscrates/app-storage/src/store.rscrates/app-storage/tests/store.rscrates/app/src/error.rscrates/app/src/lifecycle.rscrates/app/src/settings/runtime.rscrates/app/src/shell.rscrates/app/src/windows/spec.rscrates/app/tests/headless.rsdocs/learned/app-platform-plan.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the app platform layer from
docs/learned/app-platform-plan.md: three new crates that let a gpui-component app'smain.rsbe ~20 lines.[package.metadata.gpui-app]with app-local build.rs codegen (include_identity!), MSIX/CFBundle version derivation, and agpui-app-doctorpackaging verifierAppShellbuilder: lifecycle event stream (queue-until-ready, reentrancy-safe), hold/release liveness, Send+SyncAppInfo/AppProxy, window manager, typed settings +ShellPreferences, command registry driving native menus, theme service, honestPlatformCapabilitiesapp_shell,app_shell_tray) wired into a new CI launch-smoke job;COMPATIBILITY.md+ versioning disciplineAlso fixes pre-existing workspace breakage (
app_assetsdep,system_monitortimer lint,tiles.rsMenu field). Nocrates/uiAPI changes.Entire change is AI-generated (Claude Fable 5 orchestrating Opus + Codex gpt-5.6-sol agents; five structured review cycles, 24 findings fixed).