mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-07-27 01:22:16 +00:00
docs: record that core must not carry platform code CI cannot execute
Also corrects the documented test command: cargo test --lib --workspace skips the agent-desktop binary crate entirely, because that crate has a bin target and no lib target.
This commit is contained in:
parent
9944b4d5b0
commit
5904a36b2a
3 changed files with 121 additions and 11 deletions
17
CLAUDE.md
17
CLAUDE.md
|
|
@ -7,10 +7,14 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
|
|||
```bash
|
||||
cargo build # Debug build
|
||||
cargo build --release # Release build (<15MB target)
|
||||
cargo test --lib --workspace # Run all unit tests
|
||||
cargo test --workspace # Run every test (lib, binary, and integration targets)
|
||||
cargo test --lib --workspace # Library unit tests only; SKIPS the agent-desktop binary crate
|
||||
cargo test -p agent-desktop # Binary crate tests (CLI contract, dispatch, batch, policy)
|
||||
cargo test --lib -p agent-desktop-core # Test core crate only
|
||||
cargo test --lib -p agent-desktop-macos # Test macOS crate only
|
||||
cargo test test_name # Run a single test by name
|
||||
cargo check -p agent-desktop-core --all-targets --target x86_64-pc-windows-msvc # Core must cross-compile
|
||||
cargo check -p agent-desktop-core --all-targets --target x86_64-unknown-linux-gnu
|
||||
cargo clippy --all-targets -- -D warnings # Lint (must pass, zero warnings)
|
||||
cargo fmt --all -- --check # Format check
|
||||
cargo fmt --all # Auto-format
|
||||
|
|
@ -375,11 +379,22 @@ for the actionability preflight (`get_live_*`), and `is_protected_process`
|
|||
## CI Requirements
|
||||
|
||||
- GitHub Actions macOS runner executes full test suite on every PR
|
||||
- Windows and Linux runners execute the core unit tests plus their native platform crate on every PR
|
||||
- `cargo tree -p agent-desktop-core` must not contain platform crate names
|
||||
- `cargo clippy --all-targets -- -D warnings`
|
||||
- `cargo test --workspace`
|
||||
- Binary size check: fail if release binary exceeds 15MB
|
||||
|
||||
### Core platform-conditional code
|
||||
|
||||
`agent-desktop-core` must stay executable on every supported OS. A `#[cfg]` branch in core
|
||||
that no CI lane runs is a hypothesis, not shipped code — either add a lane that executes
|
||||
it or keep it out of core. Platform hardening belongs behind `PlatformAdapter` or in the
|
||||
platform crate, written on that platform against a lane that runs it. Core carried 1,062
|
||||
LOC of never-executed Win32 file I/O into v0.5.0; it failed 225 of 940 tests on first
|
||||
contact with Windows and was deleted. See
|
||||
`docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md`.
|
||||
|
||||
## Commands
|
||||
|
||||
58 commands spanning App/Window, Observation, Interaction, Scroll, Keyboard,
|
||||
|
|
|
|||
|
|
@ -165,7 +165,7 @@ agent-desktop/
|
|||
│ │ ├── snapshot_surface.rs # SnapshotSurface (predeclared cross-platform surface vocabulary)
|
||||
│ │ ├── snapshot.rs # SnapshotEngine (filter, allocate, serialize)
|
||||
│ │ ├── session/ # session start/end/list/gc, manifest, liveness
|
||||
│ │ ├── private_file*.rs # 0600-equivalent private artifact writing (Unix perms + Windows ACL family)
|
||||
│ │ ├── private_file*.rs # 0600-equivalent private artifact writing (portable std::fs on every target; Windows ACL hardening not yet built, see Phase 2.1)
|
||||
│ │ ├── trace.rs / trace_read/ # JSONL reliability trace, segment merge, HTML viewer
|
||||
│ │ ├── error_code.rs # ErrorCode enum
|
||||
│ │ ├── adapter_error.rs / app_error.rs # AdapterError, AppError
|
||||
|
|
@ -952,13 +952,13 @@ Every sub-phase 2.0–2.15 below is held to the same definition of done, stated
|
|||
|
||||
**Goal:** empirically map Windows accessibility reality with raw, no-Rust scripts before any adapter code exists, producing a committed evidence corpus the Rust sub-phases implement against — and feeding every contradiction back into this document.
|
||||
|
||||
**Scope:** a `probes/windows/` directory of raw scripts — PowerShell using .NET managed UIA (`System.Windows.Automation`, preinstalled with .NET Framework 4.8) plus small C# programs compiled with `csc.exe` where UIA3 COM specifics differ from the managed wrapper. The corpus must cover, each as a runnable script with captured JSON/text output committed beside it: (1) full-tree dumps of Notepad, Explorer, Settings, and one Electron app (VS Code or Slack) including every property read per node; (2) a pattern-availability census per ControlType (Invoke, Toggle, Value, RangeValue, ExpandCollapse, SelectionItem, Scroll, ScrollItem, Text, Window, LegacyIAccessible); (3) every interaction exercised raw — invoke, toggle, set value, select, expand/collapse, scroll via pattern AND wheel, text get/selection/caret/insert, focus; (4) SendInput synthesis experiments — keyboard incl. modifier chords and UTF-16 chunking limits, mouse click/move/wheel/drag; (5) `ElementFromPoint` hit-testing incl. deliberately occluded and zero-size targets; (6) `CacheRequest` batched reads timed against per-property reads; (7) AutomationId coverage census across Win32 / WinForms / WPF / Electron; (8) event-handler observations (which UIA events actually fire, ordering, MTA threading behavior); (9) elevation/UIPI behavior against an elevated process; (10) RDP-session and DPI/multi-monitor bounds behavior. Alongside the scripts: `probes/windows/FINDINGS.md` — a findings ledger mapping every experiment to observed behavior and a doc-alignment verdict (confirms this document / contradicts it / new edge case).
|
||||
**Scope:** a `probes/windows/` directory of raw scripts — PowerShell using .NET managed UIA (`System.Windows.Automation`, preinstalled with .NET Framework 4.8) plus small C# programs compiled with `csc.exe` where UIA3 COM specifics differ from the managed wrapper. The corpus must cover, each as a runnable script with captured JSON/text output committed beside it: (1) full-tree dumps of Notepad, Explorer, Settings, and one Electron app (VS Code or Slack) including every property read per node; (2) a pattern-availability census per ControlType (Invoke, Toggle, Value, RangeValue, ExpandCollapse, SelectionItem, Scroll, ScrollItem, Text, Window, LegacyIAccessible); (3) every interaction exercised raw — invoke, toggle, set value, select, expand/collapse, scroll via pattern AND wheel, text get/selection/caret/insert, focus; (4) SendInput synthesis experiments — keyboard incl. modifier chords and UTF-16 chunking limits, mouse click/move/wheel/drag; (5) `ElementFromPoint` hit-testing incl. deliberately occluded and zero-size targets; (6) `CacheRequest` batched reads timed against per-property reads; (7) AutomationId coverage census across Win32 / WinForms / WPF / Electron; (8) event-handler observations (which UIA events actually fire, ordering, MTA threading behavior); (9) elevation/UIPI behavior against an elevated process; (10) RDP-session and DPI/multi-monitor bounds behavior; (11) private-file I/O primitives — whether atomic rename over a concurrently-open handle requires `FILE_SHARE_DELETE`, whether an elevated process owns new objects as `TokenOwner` (e.g. `BUILTIN\Administrators`) rather than `TokenUser`, whether `GetFileInformationByHandleEx(FileRemoteProtocolInfo)` reliably distinguishes local from remote volumes, and what ancestor-vs-leaf ACL validation contract parity with the unix leaf-only rule actually needs (see `docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md`). Alongside the scripts: `probes/windows/FINDINGS.md` — a findings ledger mapping every experiment to observed behavior and a doc-alignment verdict (confirms this document / contradicts it / new edge case).
|
||||
|
||||
**Key APIs:** System.Windows.Automation, UIA3 COM (IUIAutomation) via csc.exe shims, SendInput, ElementFromPoint, CacheRequest.
|
||||
|
||||
**Depends on:** nothing — this is the entry point; the dev VM needs only its preinstalled .NET, PowerShell, and git.
|
||||
|
||||
**Exit criteria:** the script corpus and captured outputs are committed and re-runnable on the dev VM; the findings ledger covers tree, patterns, interactions, input, hit-testing, batching, identity, events, elevation, and session behavior with no open "unknown" rows; every ledger entry that contradicts this document has a matching amendment to this document landed in the same PR (see the source-of-truth feedback rule in the Platform Delivery Model); no Rust adapter sub-phase (2.2 onward) starts until this exit gate is green.
|
||||
**Exit criteria:** the script corpus and captured outputs are committed and re-runnable on the dev VM; the findings ledger covers tree, patterns, interactions, input, hit-testing, batching, identity, events, elevation, session, and private-file I/O behavior with no open "unknown" rows; every ledger entry that contradicts this document has a matching amendment to this document landed in the same PR (see the source-of-truth feedback rule in the Platform Delivery Model); no Rust adapter sub-phase (2.2 onward) starts until this exit gate is green.
|
||||
|
||||
**Est. PR size:** ~1.5k lines (scripts + ledger; no Rust).
|
||||
|
||||
|
|
@ -972,15 +972,15 @@ Every sub-phase 2.0–2.15 below is held to the same definition of done, stated
|
|||
- `CoInitializeEx(NULL, COINIT_MULTITHREADED)` + `SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2)` bootstrap at process start
|
||||
- `WindowsAdapterSession` implementing `AdapterSession` via `open_session` — owns COM apartment state so later sub-phases don't reinvent COM lifecycle
|
||||
- Re-verify the dependency pins below against crates.io + supply-chain policy (pinned at 2026-04 research time — see New Dependencies)
|
||||
- Fix the `private_file_windows_security` `AceSize` validation gap (`crates/core/src/private_file_windows_security.rs`, currently cfg-gated dead code on non-Windows CI) so the private-artifact-writing path is real before any Windows code writes a refmap or trace file
|
||||
- Implement Windows private-file hardening from scratch, behind `PlatformAdapter` or as a Windows-gated dependency of the `agent-desktop-windows` crate — never as unconditional `agent-desktop-core` surface (see `docs/solutions/best-practices/never-ship-platform-code-that-ci-cannot-execute.md`) — satisfying, with evidence from 2.0's probes: `FILE_SHARE_DELETE` on every concurrently-open handle across an atomic replace; owner validation against `TokenOwner`, not `TokenUser`; no locality inference from `FileRemoteProtocolInfo`; and an ancestor-vs-leaf validation contract decided deliberately, matching or explicitly diverging from the unix leaf-only rule, so the private-artifact-writing path is real before any Windows code writes a refmap or trace file
|
||||
|
||||
**Key APIs:** `CoInitializeEx`, `SetProcessDpiAwarenessContext`, Win32 ACL / `AceSize` validation (private-file security)
|
||||
**Key APIs:** `CoInitializeEx`, `SetProcessDpiAwarenessContext`, Win32 ACL / `TokenOwner` validation, `FILE_SHARE_DELETE` (private-file hardening)
|
||||
|
||||
**Depends on:** nothing (opening sub-phase)
|
||||
|
||||
**Exit criteria:** workspace green on Windows CI; `WindowsAdapter` constructs and satisfies the trait; every command returns honest `PLATFORM_NOT_SUPPORTED` on Windows; the permission probe is unit-tested against mocked COM security state.
|
||||
**Exit criteria:** workspace green on Windows CI; `WindowsAdapter` constructs and satisfies the trait; every command returns honest `PLATFORM_NOT_SUPPORTED` on Windows; the permission probe is unit-tested against mocked COM security state; private-file hardening is unit-tested on the `windows-latest` CI lane, not merely `cargo check`-clean.
|
||||
|
||||
**Est. PR size:** ~0.8k LOC
|
||||
**Est. PR size:** ~1.3k LOC (bootstrap ~0.8k + from-scratch private-file hardening ~0.5k)
|
||||
|
||||
### 2.2 — UIA Element Wrapper & Tree Walk
|
||||
|
||||
|
|
@ -1170,11 +1170,11 @@ Every sub-phase 2.0–2.15 below is held to the same definition of done, stated
|
|||
**Scope:**
|
||||
- `ScreenshotBackend::Legacy` first (`PrintWindow(hwnd, hdc, PW_RENDERFULLCONTENT)` — mitigates DWM black frames), then `ScreenshotBackend::Modern` via `windows-capture`/WGC where the session supports it (P2-O13)
|
||||
- `screenshot --screen` honest display targeting (pairs with `list_displays` from 2.4)
|
||||
- Typed clipboard: `CF_UNICODETEXT` → `ClipboardContent::Text`, image via `CF_DIB`/PNG → `ClipboardContent::Image`, `CF_HDROP` file lists → `ClipboardContent::FileUrls`, all written through 0600-equivalent private files (see 2.1's ACL fix)
|
||||
- Typed clipboard: `CF_UNICODETEXT` → `ClipboardContent::Text`, image via `CF_DIB`/PNG → `ClipboardContent::Image`, `CF_HDROP` file lists → `ClipboardContent::FileUrls`, all written through 0600-equivalent private files (see 2.1's private-file hardening)
|
||||
|
||||
**Key APIs:** `PrintWindow`, `windows-capture`/`Windows.Graphics.Capture`, `OpenClipboard`/`GetClipboardData`/`SetClipboardData`
|
||||
|
||||
**Depends on:** 2.1 (private-file ACL fix), 2.4 (displays)
|
||||
**Depends on:** 2.1 (private-file hardening), 2.4 (displays)
|
||||
|
||||
**Exit criteria:** screenshot + clipboard e2e pass (clipboard tests hermetic, no real-desktop clipboard pollution); WGC gracefully degrades (falls back to `Legacy`, does not fail) in RDP-limited sessions.
|
||||
|
||||
|
|
@ -1652,7 +1652,7 @@ Same rendering shape, same [Cross-cutting sub-phase DoD](#cross-cutting-sub-phas
|
|||
|
||||
**Key APIs:** `org.freedesktop.portal.ScreenCast`, `org.freedesktop.portal.RemoteDesktop`, `XGetImage`, `wl-clipboard`/`xclip`
|
||||
|
||||
**Depends on:** 3.1 (private-file handling — Unix permissions already real, unlike the Windows ACL gap), 3.4 (displays)
|
||||
**Depends on:** 3.1 (private-file handling — Unix permissions already real; Windows private-file hardening is still to be built from scratch in 2.1), 3.4 (displays)
|
||||
|
||||
**Exit criteria:** screenshot + clipboard e2e pass (clipboard tests hermetic); the PipeWire portal flow is proven — the user approves via the XDG portal dialog once, subsequent calls bypass the dialog within the session grant window.
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,95 @@
|
|||
---
|
||||
title: Never ship platform code that CI cannot execute
|
||||
date: 2026-07-25
|
||||
category: best-practices
|
||||
module: crates/core private file I/O
|
||||
problem_type: process_gap
|
||||
component: tooling
|
||||
symptoms:
|
||||
- "A platform-conditional branch in core compiles everywhere but has only ever run on one OS."
|
||||
- "225 of 940 core unit tests fail the first time the suite is run on Windows."
|
||||
- "status fails closed on a plain local NTFS disk with 'cannot verify that the Windows storage is local'."
|
||||
- "A platform unit test compares two constants and can never fail."
|
||||
root_cause: process_gap
|
||||
resolution_type: code_removal
|
||||
severity: high
|
||||
tags: [platform-isolation, ci, windows, private-file, cfg, dead-code]
|
||||
---
|
||||
|
||||
# Never ship platform code that CI cannot execute
|
||||
|
||||
## Problem
|
||||
|
||||
`agent-desktop-core` is defined as platform-agnostic. Its domain logic honoured that —
|
||||
five `target_os` lines in the whole crate, every command test driven by `MockAdapter`.
|
||||
|
||||
Its filesystem layer did not. `private_file*` carried 1,062 LOC of Windows-only `unsafe`
|
||||
Win32: ACL construction, SID comparison, ancestor guard chains, reparse-point rejection,
|
||||
remote-volume detection. The Windows adapter it served was a 76-LOC stub with no
|
||||
`windows-sys` dependency and no implemented command — 14x more Windows code in
|
||||
"platform-agnostic" core than in the Windows crate.
|
||||
|
||||
CI ran `cargo check` on Windows and Linux, and `cargo test` on macOS alone. The Win32
|
||||
layer shipped in v0.5.0 type-checked and never once executed. Its first execution, on a
|
||||
real Windows machine, failed 225 of 940 core tests across four clusters:
|
||||
|
||||
- 122 sharing violations. `LEAF_SHARING` deliberately excluded `FILE_SHARE_DELETE`, so
|
||||
`SetFileInformationByHandle(FileRenameInfo)` collided with the validation handle the
|
||||
same code still held open. POSIX `rename(2)` ignores open descriptors; Windows does not.
|
||||
- 69 owner-only-DACL rejections. Owner validation compared against `TokenUser`; elevated
|
||||
tokens own new objects as `BUILTIN\Administrators`, which is how most CI runners execute.
|
||||
- 9 trace-writer access denials and 25 cascading assertion failures.
|
||||
- `status` dead on Windows: locality was inferred from
|
||||
`GetFileInformationByHandleEx(FileRemoteProtocolInfo)` failing with exactly
|
||||
`ERROR_INVALID_PARAMETER`. Local NTFS returns other codes, so the gate failed closed —
|
||||
and it ran on every ancestor from the volume root down.
|
||||
|
||||
The layer also diverged from the contract it claimed to implement. On unix, ownership and
|
||||
mode are checked on the leaf directory only; ancestors merely have to be non-symlink
|
||||
directories. The Windows path validated every ancestor and rejected any reparse point.
|
||||
Nobody decided that. It drifted, and no test could notice.
|
||||
|
||||
The single Windows behaviour test in the deleted code was:
|
||||
|
||||
```rust
|
||||
assert_eq!(LEAF_SHARING & FILE_SHARE_DELETE, 0);
|
||||
```
|
||||
|
||||
It asserted the defect was correct, compared constants to constants, and could never fail.
|
||||
|
||||
## Resolution
|
||||
|
||||
Deleted all 1,062 LOC and removed `windows-sys` from core entirely. Windows now uses the
|
||||
same portable `std::fs` path as every other non-unix target. This is honest: the hardening
|
||||
guarded refmap, trace, and session artifacts on a platform where no command can produce
|
||||
them.
|
||||
|
||||
Added real `cargo test` lanes for Windows and Linux so every platform-conditional branch
|
||||
in core is executed on every PR. That lane, not the deleted code, is the actual fix — it
|
||||
is what stops the same mistake reaching the Linux adapter, whose
|
||||
`validate_local_filesystem` had been equally unrun.
|
||||
|
||||
## Rules
|
||||
|
||||
- A `#[cfg]` branch that CI cannot execute is not shipped code, it is a hypothesis. Either
|
||||
add a lane that runs it, or do not merge it.
|
||||
- Do not write platform hardening ahead of the platform adapter it protects. Write it on
|
||||
that platform, against a lane that runs it, informed by probes.
|
||||
- A test that compares constants to constants is not a test. Every test must be able to
|
||||
fail.
|
||||
- When a platform layer needs a stricter policy than the shared contract, that is a
|
||||
contract change to make deliberately and document — not a detail to bury in one OS.
|
||||
|
||||
## Constraints for future Windows private-file hardening
|
||||
|
||||
When hardened Windows I/O returns, it belongs behind `PlatformAdapter` or in a
|
||||
Windows-gated dependency of the platform crate — never as unconditional core surface. It
|
||||
must satisfy, with evidence:
|
||||
|
||||
1. Atomic replace requires `FILE_SHARE_DELETE` on every concurrently-open handle to the
|
||||
target. POSIX rename semantics do not transfer.
|
||||
2. Owner validation compares against `TokenOwner`, not `TokenUser`.
|
||||
3. `FileRemoteProtocolInfo` cannot establish volume locality. Use a verified primitive or
|
||||
drop the gate rather than failing closed on local disks.
|
||||
4. Ancestor-wide validation must match the unix leaf-only contract, or the contract must
|
||||
change for both.
|
||||
Loading…
Reference in a new issue