Repository navigation
Conversation
PsiACE
left a comment
PsiACE
left a comment
There was a problem hiding this comment.
Two supported Linux paths remain blocked.
| - target: linux-x86_64 | ||
| runner: ubuntu-24.04 |
There was a problem hiding this comment.
The new Linux candidate fails on the locked CPython archive: extract_runtime() rejects its casefold-colliding terminfo paths (reproduced with the real, hash-verified archive; also fails in CI). After extraction, _server_probe, ResourceSpec.platform and ManagedLayout.current_user still exclude Linux. Please complete the producer/consumer path before enabling this target.
There was a problem hiding this comment.
Thanks for catching this.
Linux resource handling now preserves case-sensitive paths, so real interpreter archives with paths such as TermInfo and terminfo are accepted. Windows and macOS keep their existing case-insensitive validation.
I also completed the Linux producer/consumer path by adding Linux support to ResourceSpec.platform, resource extraction, current-platform selection, _server_probe, and ManagedLayout.current_user() with XDG locations. Regression tests cover case-distinct Linux paths and the Linux managed layout.
| } | ||
| pub fn binding(&self) -> Result<Binding, Error> { | ||
| if !cfg!(any(windows, target_os = "macos")) { | ||
| if !cfg!(any(windows, target_os = "macos", target_os = "linux")) { |
There was a problem hiding this comment.
Enabling Linux exposes a broken session environment: launch() supplies only HOME/PATH/TMPDIR, then process::run() calls env_clear(). On a real systemd user session, systemctl --user show-environment succeeds normally, fails with this exact allowlist, and succeeds again with XDG_RUNTIME_DIR restored. Desktop therefore reports an available user manager as unsupported. Please preserve or safely derive the required session/XDG values in the trusted native layer.
There was a problem hiding this comment.
Thanks for catching this.
The trusted Linux helper environment now preserves XDG_RUNTIME_DIR after validating that it is an absolute, non-symlink directory owned by the current user with private permissions. If it is not set, the trusted layer safely derives /run/user/<uid> when valid.
A regression test covers both the valid private runtime directory and rejection of unsafe permissions. The existing fixed environment allowlist remains in place; arbitrary renderer or process environment variables are not forwarded.
There was a problem hiding this comment.
[P2] The runtime-dir fix restores user-manager access, but launch() still drops XDG_CONFIG_HOME, XDG_DATA_HOME, XDG_STATE_HOME, and XDG_CACHE_HOME before env_clear(). On 911d6f8, I used the actual Trust::launch() environment with a real Ops subprocess: the same owned unit under a custom XDG_CONFIG_HOME changes from registration: installed to not_installed. ManagedLayout.current_user() also switches its program, configuration, data, cache, and journal paths back to the HOME defaults. Please preserve validated XDG home locations through this boundary and cover a non-default layout, so Desktop inspects/manages the same installation as the CLI.
There was a problem hiding this comment.
Thanks for catching this.
The trusted Linux launch environment now preserves validated XDG_CONFIG_HOME, XDG_DATA_HOME, XDG_STATE_HOME, and XDG_CACHE_HOME in addition to XDG_RUNTIME_DIR. Absolute paths without parent-directory traversal are accepted; invalid paths are rejected instead of silently falling back to $HOME.
This keeps ManagedLayout.current_user() consistent with the CLI for non-default XDG layouts. I also added regression coverage for the non-default layout and invalid path handling. Rust and Python focused tests pass locally.
There was a problem hiding this comment.
[P2] The non-default layout now round-trips correctly on 2e168b1, but empty XDG values introduce a regression. With XDG_CACHE_HOME="" (likewise any of the other three home variables), Trust::launch() returns Unavailable before starting Ops; leaving it unset succeeds. I reproduced this against the actual Rust trust layer and confirmed the previous head accepts the same empty value. ManagedLayout.current_user() already treats empty as unset, as required by the XDG specification. Please skip empty values before the absolute-path check so a valid default-layout session can still inspect/manage its service.
There was a problem hiding this comment.
Verified on c5b9a19a: the reported session-environment gap is closed, and the empty-value regression from 2e168b1a is gone.
launch()now insertsXDG_RUNTIME_DIRafter validating it throughtrusted_linux_runtime_dir()— absolute, non-symlink directory, matchinguid, mode& 0o077 == 0, with/run/user/<uid>as the derived fallback (desktop/src-tauri/src/ops_bridge/trust.rs:304-309,trust.rs:325-347).- The four home variables survive
env_clear()throughtrusted_linux_xdg_environment()(trust.rs:351-380). - Empty values are dropped before the absolute-path check (
trust.rs:369-372), soXDG_CACHE_HOME=""returns an environment instead ofUnavailable;linux_xdg_home_environment_preserves_a_non_default_layout(desktop/src-tauri/src/ops_bridge/tests.rs:978-1018) now asserts the all-empty map yields an empty environment, andlinux_runtime_environment_is_preserved_only_for_a_private_owned_directory(tests.rs:957-975) covers the0o755rejection and the unset/empty equivalence.
One residual asymmetry, which is not the original defect: an absolute value containing .. still returns Err(Error::Unavailable) (trust.rs:374-377), while trusted_linux_runtime_dir_from treats the same shape as ignore (returns None, trust.rs:327-333) and the XDG specification says to ignore. Because launch() propagates the error, a single ..-bearing XDG home keeps Local Server unavailable — the same outcome class this thread was about, for a rarer input. That branch is also the only untested one: relative and empty values are covered, .. is not.
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
Rechecked 6b2f53bd: the custom-XDG layout and empty-XDG failures are fixed. The native trust layer now preserves configured locations and treats empty values as unset; I found no new correctness issue in the changed paths.
Merge validation is still incomplete. The Python 3.13 unit job fails with ProcessLookupError while _process_identity() reads /proc/<pid>/stat in test_upgrade_command_owner_finishes_cleanup_after_gate_cancellation[parent-term]. That test is unchanged from the base, so this appears to be an existing test race rather than a defect introduced here. Desktop, native-service and managed-resource-candidate validation were consequently skipped. Please obtain a successful run including those checks before merging.
knqiufan
left a comment
knqiufan
left a comment
There was a problem hiding this comment.
Please keep the optional Linux candidate from blocking the required Windows/macOS pipeline.
| fail-fast: false | ||
| matrix: | ||
| include: | ||
| - target: linux-x86_64 |
There was a problem hiding this comment.
[P2] Keep the optional Linux candidate outside the required aggregate
Adding Linux to this shared matrix makes a Linux-only failure fail the candidate workflow and therefore the Main CI aggregate. The aggregation script returns failure for that simulated result. This conflicts with #2112's explicit requirement that an optional Linux failure must not block mandatory Windows/macOS checks. Please isolate the Linux result so its failure remains visible without failing the required aggregate.
There was a problem hiding this comment.
Confirmed on 6b2f53bd. The CI rerun is now green, including the Linux candidate, but the optional-failure handling remains incorrect. Running the actual ci-result script with modeled job results succeeds when all jobs succeed and exits 1 with CI checks did not succeed: {'managed-resource-candidates': 'failure'} when that workflow alone fails. The new Linux matrix member has no failure isolation, so a Linux-only failure reaches this required aggregate even when Windows/macOS succeed. Please keep that failure visible while preserving the required-platform-only outcome specified in #2112.
There was a problem hiding this comment.
Thanks for catching this.
I isolated the optional Linux candidate from the required aggregate. The required managed-resource-candidates call now runs only Windows and macOS targets and remains in ci-result.needs.
Linux runs through a separate managed-resource-candidates-linux call. Its diagnostics and artifacts remain visible, but it is intentionally excluded from the required aggregate, so a Linux-only failure cannot block the required Windows/macOS pipeline.
I also added regression tests covering the target split and terminal CI result behavior. The focused CI tests pass locally.
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
The target split is correct, but the two calls still share a cancellation group. The current run has already cancelled the required candidate workflow.
| ${{ !cancelled() && needs.quality.result == 'success' && | ||
| contains(fromJSON('["success","skipped"]'), needs.tests.result) && | ||
| (fromJSON(needs.quality.outputs.suites || '{}').managed_resources) }} | ||
| uses: ./.github/workflows/managed-resource-candidates.yml |
There was a problem hiding this comment.
[P2] Give the required and optional callers separate concurrency groups
Both calls inherit managed-resource-candidates-${{ github.event.pull_request.number || github.ref }}-${{ github.event_name }} with cancel-in-progress: true, so they cancel each other despite the target split. On 24018fb0, the required selector job is already cancelled with the annotation Canceling since a higher priority waiting request for managed-resource-candidates-2471-pull_request exists; its candidate matrix is cancelled too. Please distinguish the two callers in the concurrency key so optional Linux cannot preempt the required Windows/macOS qualification.
There was a problem hiding this comment.
Thanks for catching this.
The required and optional callers now use separate concurrency groups. The required Windows/macOS caller passes concurrency_suffix: required, while the optional Linux caller passes concurrency_suffix: linux. The suffix is included in the reusable workflow’s concurrency key, so the Linux run can no longer cancel the required candidate run.
I also added regression assertions for the two caller-specific groups. The focused CI tests pass locally.
There was a problem hiding this comment.
[P2 resolved at c5b9a19] The two callers now use distinct concurrency keys: the reusable group is managed-resource-candidates-<pr|ref>-<event>-<concurrency_suffix|default>, and master.yml passes concurrency_suffix: required for the windows/macos caller and concurrency_suffix: linux for the optional Linux caller. A Linux run can no longer cancel the required selection. Both callers still share github.event.pull_request.number || github.ref with cancel-in-progress: true, so a re-run of the same caller still supersedes its own earlier run, which is the intended behaviour.
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
Re-reviewed d52ec006. The required and optional resource callers now use separate concurrency groups, and all four candidate targets completed successfully on this head. The earlier XDG and optional-platform findings are addressed; I found no remaining correctness issue introduced by this PR. No new product API or database table is added.
The overall CI result is still red for OpenDAL typing and SQLite/OceanBase acceptance failures in unchanged paths. This code approval does not override those CI gates or the remaining review requirements.
|
The four red checks reduce to three unrelated CI issues:
The Linux Desktop, systemd-user, and all managed-resource candidate checks pass. |
hireview001
left a comment
hireview001
left a comment
There was a problem hiding this comment.
- Resolve merge conflicts with the target branch.
- CI failed: CI result (failure).
- CI failed: harness-validation / Acceptance (sqlite) (failure).
- CI failed: harness-validation / Acceptance (oceanbase) (failure).
- CI failed: opendal-package (failure).
- CI failed: Main (failure).
|
The only remaining failure is the unchanged dsh-package runtime test:setup validates native include trees observed by a real DSH startup. |
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
Re-reviewed dcf60c39. The master merge preserves the reviewed Linux changes, including XDG handling and separate concurrency groups for required and optional candidates. I found no new correctness defect introduced by this PR. No new product API or database table is added.
CI remains blocked by the DSH runtime setup test. Its code and job are unchanged from the base; I found no evidence connecting that failure to this PR. Linux Desktop, real systemd-user lifecycle, and all four managed-resource candidates passed on this head. This approval does not override the failing CI gate or other outstanding reviews.
hireview001
left a comment
hireview001
left a comment
There was a problem hiding this comment.
- Resolve merge conflicts with the target branch.
- CI failed: CI result (failure).
- CI failed: dsh-package (failure).
- CI failed: Main (failure).
There was a problem hiding this comment.
Re-reviewed ed2a2332. I found no new runtime correctness defect in the merge. One non-blocking documentation inconsistency remains below.
Current-head CI qualification is incomplete: the macOS ARM Desktop job was cancelled before acquiring a hosted runner. GitHub reports runner capacity constraints; this is not a demonstrated code regression. A successful run is still required.
| uv run --no-sync pytest -q tests/native/test_personal_service_lifecycle.py | ||
| ``` | ||
|
|
||
| The Desktop page currently performs a read-only status inspection through `ops.inspect`; it does not install, start, stop or restart a service. The product uses the current user's `systemctl --user` manager and does not enable linger, write system units or require root. A passing Rust, lifecycle or candidate-bundle test does not by itself qualify a signed package or a normal user's login-session configuration. |
There was a problem hiding this comment.
[P3] Update the obsolete read-only UI description
After the master merge, LocalServerSettings exposes install, repair, start, stop and restart, and useLocalServer.execute() dispatches the approved plan through opsApi.apply(). With a valid Linux binding, the page is therefore no longer limited to ops.inspect. Please distinguish the read-only inspection from the explicitly approved lifecycle operations here, while retaining the existing limits on installed-session and signed-package qualification.
There was a problem hiding this comment.
Thanks for catching this.
I updated desktop/VALIDATION.md to distinguish read-only status inspection through ops.inspect from explicitly approved lifecycle plans dispatched through ops.apply. The documentation now covers install, repair, start, stop, and restart for a valid Linux binding.
knqiufan
left a comment
knqiufan
left a comment
There was a problem hiding this comment.
Please address the two blocking findings before merging: keep Local Server operations available when an XDG home variable contains a relative path, and preserve the existing Linux Server CLI lifecycle gate. The Rust toolchain and README findings are non-blocking.
| .components() | ||
| .any(|component| matches!(component, Component::ParentDir)) | ||
| { | ||
| return Err(Error::Unavailable); |
There was a problem hiding this comment.
[P2 · Blocking] Ignore relative XDG home paths instead of disabling Local Server
With XDG_CACHE_HOME=relative-cache, this validation returns Unavailable before the Ops helper starts, so even read-only inspection fails despite a valid binding and an available user manager. I reproduced the rejection in this Rust helper and confirmed that ManagedLayout.current_user() in src/powercontext/ops/installer.py also rejects the same input. The XDG Base Directory Specification says to ignore relative values. Please use the default directory for relative values at both boundaries while preserving valid absolute overrides.
There was a problem hiding this comment.
[P2 partially resolved at c5b9a19] Relative values are now ignored at both boundaries as requested: trusted_linux_xdg_environment_from skips a non-absolute XDG_* value (continue), so the helper starts with the platform defaults, and ManagedLayout.current_user() keeps its fallback for the same input. The new linux_xdg_home_environment_preserves_a_non_default_layout test asserts that XDG_CONFIG_HOME=relative/config is absent from the forwarded environment.
One residual difference remains for an absolute value that contains .., e.g. XDG_CONFIG_HOME=/home/u/x/../.config: trusted_linux_xdg_environment_from returns Error::Unavailable, which fails the whole launch, while ManagedLayout.current_user() treats the same value as unusable and falls back to ~/.config. So the read-only inspection is unavailable through the bridge for an environment the Python consumer would have accepted with the default layout. If the intent is "never disable Local Server for an unusable XDG value", skipping that value here would match installer.py; if the intent is to fail closed on any traversal segment, the two boundaries should agree.
There was a problem hiding this comment.
[P2 · Still blocking] The Rust bridge now drops relative XDG home values, but the managed installer still reads a relative XDG_DATA_HOME when choosing the Server data directory. On c5b9a19a, I ran the managed install preflight with a simulated service manager and XDG_DATA_HOME=relative-data. Preflight succeeded, but target_summary.data_dir was <cwd>/relative-data/powercontext. With the variable unset or empty, the same preflight returned $HOME/.local/share/powercontext, which is also the expected path for a relative value.
ManagedLayout.current_user() uses the validated fallback for the managed program directory, but calls default_data_dir() separately for Server data, which reads the original environment again. Please use the validated XDG data base for the Server data directory too, and cover target_summary.data_dir in the regression test while preserving valid absolute overrides.
| runs-on: ubuntu-latest | ||
| # Linux native service qualification is optional under #2112; do not make | ||
| # it block the required macOS and Windows service checks. | ||
| continue-on-error: true |
There was a problem hiding this comment.
[P2 · Blocking] Keep the existing Linux Server CLI lifecycle gate required
This job already tests the supported powercontext service lifecycle against a real systemd --user manager. Applying continue-on-error to the whole job means a Linux CLI regression can now leave Main's required CI result green when the other platforms pass. Please preserve the pre-existing CLI gate and apply the optional policy only to the new Linux Desktop qualification.
There was a problem hiding this comment.
[P2 resolved at c5b9a19] This job no longer carries continue-on-error: linux-systemd-user (native-service.yml, ubuntu-24.04, POWERCONTEXT_RUN_NATIVE_SERVICE_TESTS=1) is a plain required job again, so a Linux CLI lifecycle regression still fails the aggregate. The optional policy now lives only on desktop.yml's linux job (continue-on-error: true).
Consequence worth stating for the record: that optional job is also the only place the newly Linux-enabled Rust suites run (#[cfg(all(test, any(target_os = "macos", target_os = "linux")))] in ops_bridge, plus the linux-only tests in ops_bridge/tests.rs). Under #2112 their results are advisory, so a Linux-only regression in the Ops Bridge still leaves Main green. That is consistent with the stated policy, not a request to change it.
| { | ||
| echo "bundle=$POWERCONTEXT_OPS_TEST_BUNDLE" | ||
| echo "bundle_manifest_sha256=$POWERCONTEXT_OPS_TEST_BUNDLE_SHA256" | ||
| CARGO_BUILD_JOBS=2 cargo test --locked --manifest-path desktop/src-tauri/Cargo.toml --lib actual_independent_bundle_runs_through_native_trust_and_read_only_bridge -- --ignored |
There was a problem hiding this comment.
[P3 · Non-blocking] Select the pinned Rust toolchain for the candidate test
This step runs cargo from the repository root. --manifest-path does not make rustup discover desktop/rust-toolchain.toml; I verified that the root selected the default toolchain, while desktop/ selected 1.95.0. Please select 1.95.0 explicitly or run the command from desktop/ so the test actually uses the toolchain installed above.
|
|
||
| On Linux, the Local Server settings page can inspect the PowerContext user service through the trusted Ops Bridge. This check is read-only: it reports supported, registration, ownership, manager, liveness, readiness and maintenance status, while keeping paths, commands, environment values and raw process output out of the UI. | ||
|
|
||
| The bridge only addresses PowerContext's registered service target and rejects untrusted windows, remote origins and arbitrary renderer input. A missing or unavailable local service does not disable remote Server connections. This workstream does not add install, start, stop or restart controls; those mutations remain in the service-management boundary and must use the same ownership and approval rules. |
There was a problem hiding this comment.
[P3 · Non-blocking] Describe the approved lifecycle actions in both READMEs
With a valid Linux binding, Local Server settings already offers install, repair, start, stop, and restart; approved plans go through ops.apply. This paragraph and its counterpart in desktop/README.zh.md still say the controls are absent. Please align both READMEs with desktop/VALIDATION.md: inspection is read-only, and lifecycle operations require explicit approval.
There was a problem hiding this comment.
Thanks for the detailed review. All four findings have been addressed.
- Relative XDG home values are now ignored and fall back to the default directories in both the trusted Rust launch environment and
ManagedLayout.current_user(). Valid absolute overrides are still preserved. - The existing Linux Server CLI lifecycle gate is required again. Optional handling remains limited to the new Linux Desktop qualification job.
- The Linux managed-resource native Ops test now explicitly runs with Rust 1.95.0.
- Both English and Chinese Desktop READMEs now distinguish read-only
ops.inspectfrom explicitly approved lifecycle plans dispatched throughops.apply, including install, repair, start, stop, and restart.
Which issue or RFC does this PR close?
Related to #2110
This PR implements the 08-E Linux personal service integration workstream. It does not close #2110 because that issue covers the complete optional Linux integration, including the already merged 08-A and 08-B workstreams.
Rationale for this change
Linux Desktop needs a safe and user-scoped way to provision and manage the local PowerContext Server.
This change integrates the shared structured Installer/Ops contracts with Linux
systemd --user, while preserving controller ownership, explicit-stop semantics, user-session lifetime, and the existing Windows/macOS behavior.The integration does not install system-wide services, require root privileges, or silently enable persistent services across logouts.
What changes are included in this PR?
systemd --user.loginctl enable-linger.Are there any user-facing changes?
Yes.
On supported Ubuntu 24.04 Linux Desktop environments, users can manage the local PowerContext Server from the Desktop settings UI.
The service is managed only within the current user session. Desktop exit does not silently stop the Server, and the application does not enable system-wide persistence or modify the user's existing linger configuration.
The feature requires a qualified local PowerContext runtime. Missing or damaged runtime resources produce structured actionable failures without affecting remote Server operations.
No public Server API or persisted data format is changed.
How was this change tested?
Local validation:
cargo test --locked --manifest-path desktop/src-tauri/Cargo.tomlcargo clippy --locked --manifest-path desktop/src-tauri/Cargo.toml --all-targets -- -D warningscargo fmt --manifest-path desktop/src-tauri/Cargo.toml --checkpnpm --dir desktop lintpnpm --dir desktop typecheckpnpm --dir desktop testpnpm --dir desktop buildgit diff --check7706 passed, 346 skippedReal Linux systemd validation:
Fork CI validation:
AI usage statement
This PR was developed with OpenAI Codex powered by GPT-5. The generated changes were reviewed, tested locally on Ubuntu 24.04, and validated through the fork's GitHub Actions workflows.