deep cleanup tier 1: fsatomic, flock, and the driver dedupe (with apply sheets)

[schickling-assistant]

Tier 1 of reports/deepclean-st2.md landed in #499, #500, #501 and #502. Three items were deliberately held back: each is a dedupe whose diff also carries at least one deliberate behaviour change, and folding a behaviour change into a "no semantic change" PR is how those changes ship unreviewed. Each was scouted to an applicable plan first; the plans are below so the follow-up is implementation, not re-analysis.

All line numbers were re-measured after #501 (which moved ~14k lines) and #502.

---

T1-FSAT — one src/fsatomic.rs for the publication helpers

Census correction. The report lists ten helpers; counting functions rather than modules there are twelve, and it mis-attributes one. Missing from the report: park.rs::write_json_atomically, request.rs::atomic_create, harness_state::persist_floor, the second inline copy of the pretrust write, and message.rs contributes two (atomic_create_file and atomic_replace_file).

Absorb 9 fns in 8 modules, each mapped to its current durability level verbatim:

helper	level	note
delivery_ledger::atomic_json	FsyncFileAndDir	already both fsyncs; best-effort dir sync becomes strict (see below)
driver_diagnostic::atomic_json	FsyncFileAndDir	after #502's defect fix
context::write_atomic	Rename	fixes a latent bug: today's staging name is .ctx.tmp-{pid}-{now_ms()}, so two threads writing in the same millisecond share a staging path and the second fs::write truncates the first's staged bytes. A counter plus create_new makes that an impossible AlreadyExists.
status::write_atomic	Rename	deliberately NOT raised: rewritten per agent per refresh tick, and a stale status reads as unknown rather than wrong
message::atomic_create_file	Rename	6 callers; level left verbatim — raising it for "Exactly-once-safe native bus" is a separate question, not a dedupe
message::atomic_replace_file	Rename
harness_state::write_json_atomic	Rename	the one caller pair that needs staged_in: harness-state stages beside itself, harness-context stages in the control plane
park::write_json_atomically	FsyncFileAndDir	already the best of the set
request::atomic_create	Rename

Keep-set of 5, with the evidence-based reason (the report's reason is wrong for one):

• catalog_transaction::atomic_replace_file, agent_publish::{atomic_write_spec, atomic_publish_staged_bundle} — publish through a retained control-directory fd, plus EXDEV fault injection and control_plane_rename_error classification. Strictly stronger primitive; fsatomic has no fd to hold.
• resource_profile::atomic_replace_at — not in the report at all. openat/renameat relative to a directory fd, part of a generation/revision/digest compare-and-swap.
• codex_app_server::atomic_json — the report's stated reason is false: it does not publish through a control-dir fd, it is an ordinary stage+rename. The real reasons to keep it are secure_dir(parent) (chmods the state dir to 0700 on every write) and to_writer_pretty. Its doc comment should be rewritten to say that.
• pretrust::write_atomic — keep, against the report. It publishes ~/.claude.json / ~/.codex/config.toml: files st2 does not own. Tightening a foreign config to 0600 has no security argument behind it. The zero-risk dedupe the report missed is pretrust.rs's second, inline, uncited copy of the same six lines in the codex-TOML path — fold that callsite into the existing helper, −7 LOC, no behaviour change.

Hard constraint the report missed. The staging-name grammar is load-bearing, not decorative. .status.tmp- is matched by 6 catalog/publication walkers, .message.tmp- by 4 sent-record walkers, and .harness-context.tmp-<pid>-<counter> is parsed digit-by-digit by harness_context::is_legacy_staging_name and pinned by INVARIANTS row 29 ("Replicated-path discipline"); watch.rs writes .harness-state.tmp-1-0 and .status.tmp-1-0 as literals in the delivery-watcher ignore-list test. So the prefix must stay a caller argument and the suffix grammar must be exactly {prefix}.tmp-{pid}-{counter}.

Deliberate behaviour changes this PR must own, not hide:

1. Mode. Six absorbed callers go from 0644 & ~umask to 0600. The least certain one is harness-state/harness-context: both are carried by a replication transport (INVARIANTS row 29). If that transport ever runs as a different uid, the tightening breaks replication. There is no evidence either way in this repo — the include list is owned elsewhere. This is a question for a human, not an assumption.
2. Strict directory sync. delivery_ledger's dir sync is best-effort today; codex_app_server and park are already strict. Unifying to strict changes Ledger::persist's success contract.

Commit order. Characterization tests FIRST, in each helper's own module — a test inside fsatomic cannot prove the migration preserved anything, because fsatomic does not exist yet when it lands. Pin status and context's current mode and residue behaviour, and fix delivery_ledger's existing residue filter (it matches name.ends_with(".tmp"), which the new grammar does not end with, so it would silently stop testing). Then the module and the migration.

Observability of each level, stated honestly: Rename is fully observable (complete bytes, no residue, mode). FsyncFile is not observable in-process — there is no fsync fault injection in this repo — and would have zero callers, so either drop the variant or ship it documented-but-unexercised. FsyncFileAndDir is observable through its failure edge: a parent directory that cannot be opened for sync must make the call fail. That test needs a non-root uid to mean anything (mode 0300 does not deny root), so gate it or say plainly that the strictness change is review-pinned only.

A second, distinct defect found while counting. harness_state::persist_floor stages at the fixed literal .harness-state.seq.tmp — no pid, no counter — written with fs::write (truncating, symlink-following, 0644). Two processes writing the sequence floor for the same agent interleave into one staging path and one of them can rename a torn floor into place. The floor is the safety net for the record going unreadable, so a torn floor defeats exactly the failure it exists for. One caller is inside a lock; the other is the token-only virgin-record path where the lock coverage is unverified. This should land as its own defect commit, not inside the dedupe diff.

---

T1-LOCK — route the hand-rolled flock sites through a typed guard

The report's premise is wrong. CatalogLock cannot absorb these sites and must not be given the two axes as a propagation vehicle: it is a domain lock, not a file-lock primitive. acquire canonicalizes a catalog root, opens a root-dir capability, creates/validates .st2/, fsyncs the catalog root, checks the apply marker, checks the generation-intent marker and recovers a generation intent — all around a one-line flock. Its own module doc says the opposite of what the report assumes: "State-plane traffic (messages, context, resources, status) deliberately does not use this lock."

Correct shape: extract the transport into a new src/flock.rs (FileLock + Mode{Shared,Exclusive} + Wait{Block,Now} + a new Open{Create,CreateNew,Existing} axis + Drop-unlock), make CatalogLock its first consumer with byte-identical behaviour, then migrate 7 of the 8 hand-rolled sites. Design rules that make byte-identity achievable: the module returns std::io::Result (never anyhow), so every callsite keeps its exact existing context string; Ok(None) means contention and nothing else; the module does not create directories, fsync, or name lock files — that is protocol, not transport; and open and hold stay split so catalog_lock keeps its two debug checkpoints exactly between the open and the flock (five tests read ST2_TEST_CATALOG_LOCK_ATTEMPT).

Sites: catalog_transaction::initialize_bootstrap_control, codex_app_server::acquire_owner_lock, event::StreamLock (21 LOC + own Drop), harness_state::lock_exclusive (ripples through context::lock_now and 3 call sites), message::SentLock (49 LOC + own Drop, and its file: Option<File> is never None — dead shape), pretrust::ConfigLock (33 LOC + own Drop), resource_observe::lock_request_scope. Net −70 LOC of unsafe and hand-rolled Drop, four impl Drop deleted, one unsafe lock block left in the tree.

Do not migrate: resource_profile::lock_publication — it opens via openat against a retained directory capability, returns a CatchUpError taxonomy the publication state machine matches on, and proves the opened lock is a regular file between the open and the flock. It is also the one site already correct on every axis, so it is the proof that the new module's defaults are house style rather than invention. Also not a flock site at all: host_lock.rs (a pid file that must survive process death). And every test-side flock stays raw — those are adversaries that take the lock from outside.

Deliberate behaviour change: O_CLOEXEC is a no-op (Rust's OpenOptions::open already sets it), but O_NOFOLLOW newly hardens five lock files that set neither flag today (event, harness_state ×3 paths, message, pretrust, resource_observe), and four of them start being created 0600 instead of 0644. No current test and no live path reaches any of those lock files through a symlink, so nothing observes it — but it is a real change and needs its own pinning test (mirroring the existing catalog_lock_refuses_a_symlinked_lock_file). One site needs a decision: message::shared_existing opens O_RDONLY today and would become O_RDWR, so a doctor running as a different uid against a 0644 ledger lock would start failing where it read successfully.

Thinnest safety net: harness_state/harness_context/context have no dedicated lock test anywhere, and this item changes their open mode, permissions and symlink behaviour. pretrust and resource_observe have no contention test either. The regression oracle for all of them is cheap and deterministic: flock locks are per open-file-description, so a second open of the same lock file in the same process contends with a leaked guard — no threads needed. Use it for message::inspect_sent, whose two sequential guards are the one load-bearing lifetime in the set.

---

T1-DUP — pi/omp unify, adapter extraction, exit labels

pi/omp unify. omp_session.rs is 166 LOC of duplicate. Item-by-item: 7 shared items, 8 genuinely per-harness, 5 pure doc/style drifts, 2 whose resolution is observable. Shape: a new src/pi_family_session.rs holding run_for(…, kind: &HarnessKind) plus the five shared helpers, mirroring the ChannelKind + run_for(kind) fork pi_channel.rs already uses — so the same fork is solved the same way twice in the family rather than two different ways. The per-harness env-name constants must not move: pi_channel.rs reads both modules' CHANNEL_* by path, and the whole point of two sets is that an omp seat cannot adopt a stray pi configuration. The omp version gate rides along as a verify_version: Option<fn(&str) -> Result<()>> on the descriptor, called exactly where omp calls it today — after the empty-argv check and before harness_state::claim, so an unadmitted minor fails without claiming ownership.

Exit labels. 5 copies; they disagree on exactly one arm, (None, None): "exited" in pi/omp versus "exit unknown" elsewhere. Promote provider_session::describe_exit to pub(crate) and delete the other four. De-risking finding: (None, None) is unreachable for a reaped child on Linux — Child::wait/try_wait call waitpid with neither WUNTRACED nor WCONTINUED, so every status satisfies WIFEXITED or WIFSIGNALED; the arm is constructible only via ExitStatus::from_raw(0x7f) in a test. And nothing parses the field back: every reader either passes it to display or tests is_some(). Correction to the report: it claims the unification is "pinned by provider_session.rs:337". It is not — that assertion is fed by SessionObserver::launch_error()'s hardcoded literal and never calls describe_exit. The pin does not exist and should be created by this PR, as a (status, signal) -> label table test.

Ten extractable class-(a) adapters: the exit label (5 copies), with_channel_extension (2, byte-identical), offline_defaults (2, byte-identical incl. its const), channel_env (2, byte-identical modulo the per-harness const names), record_session_end (2), channel_content (2, byte-identical — claude_mcp and pi_channel), write_json (2, byte-identical), publish_provider_auth (2, identical modulo a hardcoded Driver), ProviderAuthEdge (2 byte-identical definitions — move one into driver_diagnostic, which already owns Driver/Stage/Reason/Source/Support), and opencode's completed (folding onto completed_provider("opencode", …) produces a byte-identical error string). ≈ −133 LOC on top of the unify's ≈ −160.

Two genuinely non-extractable: provider_auth_edge (disjoint input domains — a hook event name plus JSON payload versus a typed TurnResult; they share only the output enum, which the ProviderAuthEdge move already unifies) and stop_provider_group (the shared one writes the terminal record on two paths opencode's copy does not have, because opencode's observation ownership lives in a different type).

Three things that are behaviour changes wearing a dedupe costume — do not include:

1. compaction_trigger (claude vs pi_channel). Unifying widens Claude's decode vocabulary from {manual, auto} to {manual, auto, threshold, overflow, idle}; the narrower set is a documented decision, not an oversight.
2. Extracting the shared --version banner capture. harness_version's module doc states the divergence is intentional ("omp gates the launch, opencode degrades to no native delivery, and codex refuses semantic-version reasoning outright"), and extraction changes one of opencode's error strings.
3. Folding codex's completed_tui. Its message is "controlled Codex TUI exited with {status}" — folding changes an operator-visible error string for zero structural gain.

Also observable: omp's eprintln! → tracing::warn! for "recording session end failed". provider_session spawns with stderr(Stdio::inherit()), so today that diagnostic paints into the interactive harness's own terminal; pi already uses tracing::warn!. Right change, but it moves where a failure lands and no test asserts stderr.

Ordering: pi/omp unify before exit labels — the unify structurally deletes two of the five copies, so the "exited" → "exit unknown" flip then lands in exactly one place.

One irreversible loss to avoid: several of these pairs carry different rationales for identical code (the two None/Support::Unknown justifications in publish_provider_auth, the two OFFLINE_DEFAULTS docs — one carrying the open DQ-OMP-5 note, the two with_channel_extension docs). Merge both halves. The rationale is the part that is genuinely not duplicated.

Posted on behalf of @schickling

field	value
agent_identity	dev3.direct.omp.43sz6ujq
session	dev3.43sz6ujq
agent_persona	generalist
agent_supervisor	unavailable
agent_tool	OMP
agent_tool_version	18.1.7
agent_runtime	OMP 18.1.7
tooling_profile	dotfiles@39a19af