mirror of
https://github.com/lahfir/agent-desktop.git
synced 2026-08-19 05:25:43 +00:00
docs: capture learnings from todo resolution batch (stamping, fingerprints)
Two patterns worth keeping for future work: - deterministic-build-artifact-marker: have build.rs stamp an absolute path to its artifact at a stable location so CI never has to guess with `find | head -1`. Sibling anti-pattern called out: build.rs auto-copying the generated artifact back into the source tree. - identity-fingerprint-against-os-reorder: for OS APIs that return ordered lists whose ordering is defined by "current state" (NC is the canonical example), pair any index-based action with an optional (app, title) fingerprint and fail closed on mismatch. Both include "when NOT to use" sections to prevent cargo-culting.
This commit is contained in:
Lahfir
parent
f39e6a942a
commit
366f81cc04
2 changed files with 264 additions and 0 deletions
|
|
@ -0,0 +1,129 @@
|
|||
---
|
||||
title: Stamp build artifacts at a deterministic path for CI and scripts
|
||||
date: 2026-04-16
|
||||
category: best-practices
|
||||
module: crates/ffi
|
||||
problem_type: best_practice
|
||||
component: tooling
|
||||
severity: medium
|
||||
applies_when:
|
||||
- A build script (cbindgen, bindgen, codegen) writes an artifact to $OUT_DIR whose path includes a cargo-generated hash
|
||||
- CI or a developer script needs to locate that artifact after the build finishes
|
||||
- The obvious shell-out (`find target -path '*/out/<artifact>' | head -1`) silently picks the wrong file when multiple cached build dirs coexist
|
||||
- You want a drift check (`diff committed_copy generated_copy`) that reliably fails when the committed copy is stale
|
||||
tags:
|
||||
- build-rs
|
||||
- cbindgen
|
||||
- ci
|
||||
- reproducibility
|
||||
- determinism
|
||||
- cargo
|
||||
- rust-patterns
|
||||
---
|
||||
|
||||
## Problem
|
||||
|
||||
Cargo puts build-script artifacts under a hash-randomized directory:
|
||||
|
||||
```
|
||||
target/<profile>/build/<crate-name>-<hash>/out/<artifact>
|
||||
```
|
||||
|
||||
The `<hash>` is not documented, not stable across rebuilds of the same
|
||||
code on different toolchain minor versions, and you can end up with
|
||||
several `<crate-name>-<hash>/` dirs in the same warm `target/` cache
|
||||
(e.g. after a `cargo clean -p` and rebuild, or rustup flip). Any CI
|
||||
step or developer script that resolves the artifact with
|
||||
`find target -path '.../<artifact>' | head -1` is picking arbitrarily
|
||||
among those dirs. Under drift check:
|
||||
|
||||
- If `head -1` picks the current build's artifact → drift correctly
|
||||
surfaces when the committed copy is stale.
|
||||
- If `head -1` picks a stale leftover → you either self-heal (false
|
||||
green) or report stale-vs-stale (useless).
|
||||
|
||||
The failure mode is silent: a CI step says "OK: header in sync" while
|
||||
the committed header is actually out of date, and the bad ABI ships.
|
||||
|
||||
## Solution
|
||||
|
||||
Have `build.rs` write a stable marker file containing the absolute path
|
||||
of the just-generated artifact. Downstream consumers read the marker
|
||||
instead of guessing:
|
||||
|
||||
```rust
|
||||
// crates/<your-crate>/build.rs
|
||||
fn main() {
|
||||
// ... generate $OUT_DIR/artifact ...
|
||||
|
||||
if let Some(target_root) = target_root_from_out_dir(Path::new(&out_dir)) {
|
||||
let stamp = target_root.join("ffi-header-path.txt");
|
||||
let _ = std::fs::write(&stamp, out_path.to_string_lossy().as_bytes());
|
||||
}
|
||||
}
|
||||
|
||||
/// OUT_DIR = {target}/{profile}/build/{pkg-hash}/out
|
||||
fn target_root_from_out_dir(out_dir: &Path) -> Option<PathBuf> {
|
||||
let mut current = out_dir;
|
||||
for _ in 0..4 {
|
||||
current = current.parent()?;
|
||||
}
|
||||
Some(current.to_path_buf())
|
||||
}
|
||||
```
|
||||
|
||||
CI and scripts read the marker:
|
||||
|
||||
```yaml
|
||||
- name: Drift check
|
||||
run: |
|
||||
STAMP=target/ffi-header-path.txt
|
||||
test -f "$STAMP" || { echo "FAIL: stamp missing"; exit 1; }
|
||||
GENERATED=$(cat "$STAMP")
|
||||
test -f "$GENERATED" || { echo "FAIL: stamped path missing"; exit 1; }
|
||||
diff -u crates/ffi/include/artifact.h "$GENERATED"
|
||||
```
|
||||
|
||||
## Why this works
|
||||
|
||||
- **One writer**: the build script's own invocation knows exactly which
|
||||
`OUT_DIR` it ran in. Stamping the path at that moment captures it
|
||||
authoritatively; no later tool has to reconstruct it.
|
||||
- **Stable path**: `target/<marker>.txt` lives one directory above the
|
||||
hashed build dirs and is overwritten each build. CI's cache system
|
||||
sees it as content of `target/` — no special allowlisting needed.
|
||||
- **Fail-fast wrapper**: the marker file's absence is itself a signal
|
||||
that the build script didn't run (e.g. `cargo check` instead of
|
||||
`cargo build`). CI should fail rather than fall back to a wrong
|
||||
default.
|
||||
|
||||
## When NOT to use this
|
||||
|
||||
- If your build script generates artifacts deterministically **at a
|
||||
fixed location outside `OUT_DIR`** (e.g. directly into a committed
|
||||
dir), you don't need a marker — the path itself is stable.
|
||||
- If the artifact lifecycle is driven by `cargo metadata` queries
|
||||
(e.g. `cargo metadata --format-version=1 | jq .target_directory`),
|
||||
that's already deterministic. Stamping is only needed when the
|
||||
specific pkg-hash subdirectory matters.
|
||||
|
||||
## Sibling anti-pattern to avoid
|
||||
|
||||
Do **not** have `build.rs` copy the generated artifact into the source
|
||||
tree (e.g. `fs::copy(&out_path, &committed_path)`). That mutates the
|
||||
working tree during every build, which means:
|
||||
|
||||
- `git diff` is polluted by invisible copies.
|
||||
- The drift check (`git diff --exit-code committed_path`) can
|
||||
self-heal a stale committed copy instead of catching it.
|
||||
|
||||
The committed copy is the ABI contract; updating it should be an
|
||||
explicit developer action (dedicated script) or CI-only step.
|
||||
|
||||
## References
|
||||
|
||||
- `crates/ffi/build.rs` — the stamping logic
|
||||
- `.github/workflows/ci.yml` — "FFI header drift check" step
|
||||
- `scripts/update-ffi-header.sh` — developer-facing refresh script
|
||||
- Todo `006-ready-p2-deterministic-ffi-header-drift-path` — the bug this
|
||||
pattern resolves
|
||||
|
|
@ -0,0 +1,135 @@
|
|||
---
|
||||
title: Guard OS-reordered resources with an identity fingerprint, not a raw index
|
||||
date: 2026-04-16
|
||||
category: best-practices
|
||||
module: crates/core, crates/macos, crates/ffi
|
||||
problem_type: best_practice
|
||||
component: notifications
|
||||
severity: high
|
||||
applies_when:
|
||||
- The API exposes a numeric index (or handle) obtained from a list-then-act flow
|
||||
- The underlying OS can reorder, add, or remove entries between the list and act calls
|
||||
- Acting on the wrong entry has user-visible consequences (Reply to the wrong sender, Dismiss the wrong notification, Press the wrong button)
|
||||
- Existing design assumed lists are stable across calls and did not re-verify
|
||||
tags:
|
||||
- notification-center
|
||||
- confused-deputy
|
||||
- fingerprint
|
||||
- identity
|
||||
- ffi
|
||||
- reordering
|
||||
- fail-closed
|
||||
---
|
||||
|
||||
## Problem
|
||||
|
||||
macOS Notification Center reassigns the `index` of every visible
|
||||
notification on each listing. Between `list_notifications()` and
|
||||
`notification_action(index, ...)`:
|
||||
|
||||
- a new notification arrives → everything shifts down by 1
|
||||
- an unrelated notification is dismissed → everything shifts up by 1
|
||||
- the user opens the Notification Center sidebar → grouping changes
|
||||
can renumber entries entirely
|
||||
|
||||
Any tool that round-trips `(app, title) → index → "press Reply"` is a
|
||||
**confused deputy** at the OS boundary: the tool thinks it's acting on
|
||||
the notification it showed to the user, but by the time the action
|
||||
call reaches NC the slot points elsewhere.
|
||||
|
||||
This class of bug exists for any OS API that:
|
||||
|
||||
- returns an ordered list whose ordering is defined by "current state",
|
||||
not by a stable identity
|
||||
- accepts an index (or some other positional handle) as a subsequent
|
||||
parameter
|
||||
|
||||
Examples beyond NC: running-process lists by PID reuse, window lists
|
||||
after `raise()`, clipboard history, filesystem listings used for bulk
|
||||
operations.
|
||||
|
||||
## Solution
|
||||
|
||||
Pass an optional identity fingerprint alongside the index. Verify the
|
||||
row at that index against the fingerprint **before** acting. Fail
|
||||
closed if it doesn't match.
|
||||
|
||||
```rust
|
||||
pub struct NotificationIdentity {
|
||||
pub expected_app: Option<String>,
|
||||
pub expected_title: Option<String>,
|
||||
}
|
||||
|
||||
impl NotificationIdentity {
|
||||
pub fn matches(&self, info: &NotificationInfo) -> bool {
|
||||
if let Some(ref app) = self.expected_app {
|
||||
if app != &info.app_name { return false; }
|
||||
}
|
||||
if let Some(ref title) = self.expected_title {
|
||||
if title != &info.title { return false; }
|
||||
}
|
||||
true
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Adapter layer:
|
||||
|
||||
```rust
|
||||
let entry = list_entries(&filter)?.into_iter()
|
||||
.find(|e| e.info.index == index)
|
||||
.ok_or_else(|| AdapterError::notification_not_found(index))?;
|
||||
|
||||
if let Some(id) = identity {
|
||||
if !id.is_empty() && !id.matches(&entry.info) {
|
||||
return Err(AdapterError::new(
|
||||
ErrorCode::NotificationNotFound,
|
||||
"row at this index does not match the expected fingerprint — NC likely reordered",
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// safe to press
|
||||
```
|
||||
|
||||
## Design choices
|
||||
|
||||
**Optional fields, not mandatory.** The fingerprint is a safety feature,
|
||||
not a required parameter. Hosts that already reconcile (e.g. by
|
||||
re-listing just before acting) can leave both fields null and get
|
||||
the legacy behavior. This keeps the API ergonomic for simple scripts
|
||||
while making the safe path available.
|
||||
|
||||
**Re-use an existing error code** (`NotificationNotFound`) rather than
|
||||
adding a new one. From the host's perspective, "the notification I
|
||||
intended to act on is gone or moved" is semantically the same as "the
|
||||
notification at that index disappeared": in both cases the right
|
||||
recovery is re-list and retry. Introducing a distinct
|
||||
`IDENTITY_MISMATCH` code would fork callers' error handling without a
|
||||
real behavioral difference.
|
||||
|
||||
**Tri-state UTF-8 decoding at the FFI boundary.** The identity strings
|
||||
come in as `*const c_char`. Null means "no fingerprint"; invalid UTF-8
|
||||
must NOT be silently coerced to "no fingerprint" (that would defeat
|
||||
the guard). Use `try_c_to_string` which returns
|
||||
`Ok(None)` / `Ok(Some(_))` / `Err(())` and map `Err` to
|
||||
`InvalidArgs`.
|
||||
|
||||
## When NOT to use this
|
||||
|
||||
- If the OS API returns a stable opaque handle that the caller can
|
||||
hold across calls (e.g. `HWND` on Windows), plumb the handle
|
||||
through instead of an index. Fingerprinting is a fallback for
|
||||
APIs whose handles we can't or shouldn't persist.
|
||||
- If the operation is idempotent and harmless (e.g. "list children
|
||||
of this group"), mismatch handling adds cost without value.
|
||||
|
||||
## References
|
||||
|
||||
- `crates/core/src/notification.rs` — `NotificationIdentity` type and
|
||||
tests
|
||||
- `crates/macos/src/notifications/actions.rs` — adapter-layer check
|
||||
- `crates/ffi/src/notifications/action.rs` — C-ABI surface with
|
||||
optional app/title pointers
|
||||
- Todo `003-ready-p1-stable-notification-action-identity` — the bug
|
||||
this pattern resolves
|
||||
Loading…
Reference in a new issue