Skip to content

fix(driver-diagnostic): exclusive owner-only publication, and delete six unreachable items - #502

Merged
schickling-assistant merged 1 commit into
mainfrom
schickling-assistant/2026-09-07-clean-4
Sep 7, 2026
Merged

fix(driver-diagnostic): exclusive owner-only publication, and delete six unreachable items#502
schickling-assistant merged 1 commit into
mainfrom
schickling-assistant/2026-09-07-clean-4

Conversation

@schickling-assistant

Copy link
Copy Markdown
Contributor

The defect

driver_diagnostic::atomic_json was the only one of the ten publication helpers in the tree that used File::create on a predictable temp name in an agent-writable directory — while its sibling in delivery_ledger documents, thirty lines of prose away, exactly why each of those matters. Four defects in five lines:

before after
staging open File::create — no O_EXCL, follows a symlink and truncates its target create_new — refuses with AlreadyExists
mode 0o666 & ~umask, so world-readable 0o600
staging name .driver-diagnostic.<pid>.tmp — one name per process, two writes to one agent dir interleave .driver-diagnostic.tmp-<pid>-<counter>
directory entry never synced best-effort sync_all, exactly as delivery_ledger does

Pinned by driver_diagnostic::tests::a_planted_staging_symlink_is_refused_and_the_record_is_owner_only, which plants a symlink at a staging path, asserts the refusal and that the victim's bytes survive, then asserts the published record's mode and that no staging residue is left.

The parent-directory sync stays best-effort here deliberately: this is a defect fix and must not also change the helper's success/failure contract. Making the sync strict across every helper is T1-FSAT's job.

T1-DEAD

Six deletions, each backed by an exhaustive search over src, tests and crates:

  • pretrust::pretrust_codex — 0 callers; its own doc says reconciliation does not call it. pretrust_codex_at is the live entry.
  • context::append_decision — a one-line forwarder with no production caller (the CLI already calls append_decision_to_dir). Its 5 unit-test callers now call that directly.
  • agent_author::resource::add_resource — an 8-argument forwarder superseded by add_resource_with_selector, which production already uses. Its 17 unit-test callers pass the None the forwarder passed, so the tests exercise the same call.
  • identity::AddressBookEntry::bus_address — 0 callers. Every other bus_address in the tree is AgentSpec::bus_address(this_host), a different method that takes an argument.
  • ding::observed_poke — the repo's only #[allow(dead_code)] in src, superseded by observed_poke_with_window.
  • resource_profile::relative_path — 0 callers anywhere. reports/deepclean-st2.md filed it as merely test-only; it is dead.

Two tests that could not fail

  • hooks::tests::omp_launch_classification_is_exact carried a duplicated #[test], which made the harness report it twice — visible in the gate's test count, which is why --lib reads 743 here and 743 on main while this PR adds a test: +1 real test, −1 phantom.
  • Above it sat the doc comment belonging to pi_launch_classification_is_exact — the test whose missing #[test] refactor: pure moves — test tails, the clap surface, and eight submodules #501 restored. The two defects are one mistake: a test was pasted in and its attribute and doc landed on its neighbour. Attribute de-duplicated, doc moved onto the test it describes.

Visibility narrowing: only where it is warning-free

CodexHoldReason and CodexTerminalErrorpub(crate); codex_app_server::{load_current_binding, load_current_control_state}#[cfg(test)] fn.

reports/deepclean-st2.md also lists CodexRuntime, CodexThreadBinding, CodexControlState, CODEX_CONTEXT_VERIFIED_VERSION, CodexObservedState, resource_profile::last_commit and flapping::is_parked. Each was tried and reverted, for a reason worth recording:

  • The first four wake dead_code on their test-only accessors the moment crate-external reachability stops suppressing the lint. That is why they are pub today, and an #[allow(dead_code)] to hide it would be a worse trade than the wide visibility.
  • CodexObservedState appears in a pub method of the still-pub CodexControlState, so narrowing it trips private_interfaces.
  • flapping::is_parked has two integration-test callers (tests/run.rs), which link the lib as an external crate — narrowing is a hard E0624, not a warning. The report's row is wrong.

Checks

  • Gate nix build .#st2 --no-link -L: EXIT=0 (pty st2.clean-4.gate, log /tmp/st2-clean4-gate.log).
  • Test-name set diffed against refactor: pure moves — test tails, the clap surface, and eight submodules #501's gate run: exactly one addition (the new symlink test) and one removal (the phantom duplicate). No other test moved, renamed or vanished.
  • cargo check --all-targets: 0 errors, and no warning that is not already on main (resource_profile_supervisor, ding/mod.rs, tests/resync.rs).

Deferred, with the analysis attached

T1-FSAT, T1-LOCK and T1-DUP are not in this PR. Each was scouted to an applicable apply-sheet — module source, per-caller mapping, per-site behavioural delta, test plan — and each carries at least one deliberate behaviour change that deserves its own reviewable PR rather than riding inside a "dedupe" diff:

  • T1-FSAT tightens six callers' record mode from 0644 & ~umask to 0600 and makes delivery_ledger's directory sync strict; it also uncovered a second, distinct defect (harness_state::persist_floor stages at the fixed literal .harness-state.seq.tmp, so two writers for one agent can rename a torn floor into place).
  • T1-LOCK's premise in the report is wrong: CatalogLock is a domain lock, not a file-lock primitive, and its own module doc says state-plane traffic deliberately does not use it. The correct shape is extracting the transport into src/flock.rs, which also hardens five lock files with O_NOFOLLOW.
  • T1-DUP changes the wire-visible (None, None) exit label from "exited" to "exit unknown" (unreachable for a reaped child on Linux, and pinned by nothing today) and moves omp's failure diagnostic off the agent's inherited stderr.

Tracked with the full apply sheets in the follow-up issue.

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

…elete five unreachable items

The diagnostic publisher was the only one of the ten publication helpers
in the tree that used `File::create` on a *predictable* temp name in an
agent-writable directory: no `O_EXCL`, no mode, symlink-followable, and
no parent-directory fsync — while its sibling in `delivery_ledger`
documents exactly why each of those matters. Four defects, one fix: the
staging sibling is now created exclusively at `0600` under
`.driver-diagnostic.tmp-<pid>-<counter>`, so a planted symlink is refused
instead of followed, the record is not world-readable, two writes to one
agent directory cannot collide, and the directory entry is synced.
Pinned by `a_planted_staging_symlink_is_refused_and_the_record_is_owner_only`.

T1-DEAD, each verified by an exhaustive search over `src`, `tests` and
`crates`:

- `pretrust::pretrust_codex` — 0 callers; its own doc says reconciliation
  does not call it. The live entry is `pretrust_codex_at`.
- `context::append_decision` — a one-line forwarder with no production
  caller; the CLI already calls `append_decision_to_dir`. Its 5 unit-test
  callers now call that directly.
- `agent_author::resource::add_resource` — an 8-argument forwarder
  superseded by `add_resource_with_selector`, which production already
  uses. Its 17 unit-test callers pass the `None` the forwarder passed.
- `identity::AddressBookEntry::bus_address` — 0 callers. Every other
  `bus_address` in the tree is `AgentSpec::bus_address(this_host)`, a
  different method with an argument.
- `ding::observed_poke` — the repo's only `#[allow(dead_code)]` in `src`,
  superseded by `observed_poke_with_window`.
- `resource_profile::relative_path` — 0 callers anywhere. The deep-clean
  report filed it as test-only; it is dead.

Narrowed only where it is warning-free: `CodexHoldReason` and
`CodexTerminalError` to `pub(crate)`, and the two codex control-state
loaders to `#[cfg(test)]`. Narrowing the other five codex items and
`resource_profile::last_commit` wakes `dead_code` on their test-only
accessors, which is exactly why they are `pub` today — reverted rather
than papered over with an allow.

Also: `hooks::tests::omp_launch_classification_is_exact` carried a
duplicated `#[test]` and, above it, the doc comment belonging to the pi
test whose `#[test]` was missing (#501 restored that). Attribute
de-duplicated, doc moved to the test it describes.

agent-identity: dev3.direct.omp.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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant