Skip to content

refactor: pure moves — test tails, the clap surface, and eight submodules - #501

Merged
schickling-assistant merged 12 commits into
mainfrom
schickling-assistant/2026-09-07-clean-3
Sep 7, 2026
Merged

refactor: pure moves — test tails, the clap surface, and eight submodules#501
schickling-assistant merged 12 commits into
mainfrom
schickling-assistant/2026-09-07-clean-3

Conversation

@schickling-assistant

Copy link
Copy Markdown
Contributor

What

Pure moves only: 14,145 lines relocated, plus two one-line fixes to tests that could not fail. Implements T1-GATE, T1-TAILS, T1-CLI and T1-SPLIT of reports/deepclean-st2.md §2.C.

what lines
T1-GATE #[cfg(test)] on src/run.rs's mod tests — the only ungated test module of 50 in src, so its tail was compiled by cargo build --release. Plus #[test] restored on hooks::tests::pi_launch_classification_is_exact, which had silently never run (its omp twin beside it is attributed; cargo check reported it as dead code). +2
T1-TAILS four test tails → sibling tests.rs 12,906
T1-CLI main.rs's clap surface → src/cli.rs 1,239
T1-SPLIT eight submodules out of five parents 4,226

The splits, each a contiguous-span move with the minimum markers:

  • codex_app_server/{protocol,context,process_group}.rs — 692/264/120 lines, 8/5/5 markers
  • resync/{watch_set,read}.rs — 463/170 lines, 14/2 markers
  • run/process.rs — 264 lines, 7 markers
  • catalog_transaction/capability.rs — 310 lines, 5 markers. Not fs.rs: the parent does use std::fs::{self, ..}, so mod fs; is E0255.
  • agent_author/{markers,stream,resource,desired_state,declared_field}.rs — 1,949 lines across 21 spans, 6 markers

What deliberately did not move

  • The three do-not-split verdicts stand: run.rs's reconcile core (2,045 LOC / ~30 markers on exactly the state INVARIANTS guards), main.rs's verb impls (a common.rs would be main.rs renamed), codex's state-dir + io core (313 LOC / 22 markers, atomic_json alone has 6 callsites).
  • agent_author's shared declaration writer stays in the parent for the same reason, as do its two #[cfg(test)] *_for_test scaffolds — which is what buys pub(super) instead of editing test code.
  • pub const CODEX_CONTEXT_VERIFIED_VERSION stays in codex_app_server.rs: moving a pub item into a private child deletes it from st2::codex_app_server's API with no compiler complaint.

Every parent keeps re-export glue at each item's own visibility, so no file outside a parent/child pair was edited — all 21 external crate::catalog_transaction::* / crate::run::* paths, st2::agent_author::*, st2::resync::watch_set_for* and every sibling tests.rs's use super::* resolve unchanged.

Two consequences of moving a file, both real

  1. include_str! is relative to its file. Five fixture paths in the moved codex tail needed ../tests/fixtures/../../tests/fixtures/.
  2. INVARIANTS.md cites proofs by file. tests/invariants.rs::qualified_proof_references_resolve reads every backticked <path>.rs::<test> span and asserts the test is declared there — and it went red, which is exactly what the newly-gated target is for (ci: run every hermetic test target in the st2 gate #499). 38 citations were repointed: 21 src/ding/mod.rs::, 7 src/resync.rs::, 5 src/run.rs::, 5 src/codex_app_server.rs::. Row text, guarantees and proof sets are unchanged; all 257 qualified proofs resolve.

Checks

  • Gate nix build .#st2 --no-link -L: EXIT=0 (pty st2.clean-3.gate3, log /tmp/st2-clean3-gate3.log).
  • Test counts, against refactor(delivery,identity,vrs): delete a label branch, give DELTA-006 a producer, fold two duplications #500's gate run: --lib 743 (was 742 — the +1 is the restored #[test]), run 60, hooks 11, codex_hooks 5, driver_expansion 13, discovery 75, validate 64, message 6, agent_publish 15 + 9 ignored, agent_address 12, agent_desired_state 8, catalog_graph 10, invariants 2, status_agents 12, vrs_ledger 3. No target lost, gained or renamed a test.
  • Extra gate for T1-CLI: st2 completions {bash,zsh,fish} | sha256sum is byte-identical between refactor(delivery,identity,vrs): delete a label branch, give DELTA-006 a producer, fold two duplications #500's binary and this one (3d8af97b4ff296ab / 066a2a70d3ae1d36 / 9bc9d828e7155e20).
  • Byte-identity proof of the moves, run before committing: re-indenting each new tests.rs (adding four spaces to exactly the lines that were dedented) reproduces the removed block byte-for-byte, for all four files; cli.rs equals main.rs:16-1254 after removing 28 pub(crate) markers, with zero residual differences.

Independent review

MoveReviewer (clean context, read-only, no build access) re-derived the tail extraction itself for all four files, ran its own quote-parity scan to locate all 83/76/40/9 in-string continuation lines and confirmed no string literal's content changed, exhaustively searched for include!/#[path]/file!()/module_path!()/concat!/env! hazards, and verified programmatically that each of the 28 cli.rs differences is exactly "pub(crate) " inserted after the leading whitespace with no clap attribute altered.

Verdict: "no semantic change". Two non-blocking notes, both recorded rather than fixed:

  1. Four intermediate commits do not compile on their own (src/run/tests.rs arrives one commit after mod tests;; the codex include_str! repoint arrives three commits after the tail move). Squash-merged, so main never sees them; worth knowing if anyone bisects this range.
  2. cli.rs:719's fn selection signature grows from 102 to 113 columns. cargo fmt --check is deliberately not gated (flake.nix), and the line was already over width on main.
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

`src/run.rs`'s `mod tests` was the only one of 50 test modules in `src`
without `#[cfg(test)]`, so its 3,700-LOC tail was compiled by
`cargo build --release`. It compiled because `tempfile` is a real
dependency.

`hooks::tests::pi_launch_classification_is_exact` had no `#[test]`, so
the pi half of the launch-classification contract has never run — the
omp twin beside it is attributed. `cargo check` reported it as dead code.

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
Pure move: 3700 lines, dedented by exactly four spaces on every line that
does not begin inside a multi-line string literal (verified by
re-indenting and comparing byte-for-byte with the original block). A
child module sees the parent's privates, so `use super::*;` is unchanged
and no visibility marker is added.

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
Pure move: 2265 lines, dedented by exactly four spaces on every line that
does not begin inside a multi-line string literal (verified by
re-indenting and comparing byte-for-byte with the original block). A
child module sees the parent's privates, so `use super::*;` is unchanged
and no visibility marker is added.

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
…erver/tests.rs

Pure move: 4193 lines, dedented by exactly four spaces on every line that
does not begin inside a multi-line string literal (verified by
re-indenting and comparing byte-for-byte with the original block). A
child module sees the parent's privates, so `use super::*;` is unchanged
and no visibility marker is added.

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
Pure move: 2748 lines, dedented by exactly four spaces on every line that
does not begin inside a multi-line string literal (verified by
re-indenting and comparing byte-for-byte with the original block). A
child module sees the parent's privates, so `use super::*;` is unchanged
and no visibility marker is added.

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
`main.rs` was 4,641 LOC with zero test LOC; 1,239 of them (27%) were pure
clap declaration. The declaration and the verb implementations are now
readable separately.

Pure move: the block is byte-identical apart from 16 item and 12
field/method `pub(crate)` markers, which a binary crate needs to see its
own module's items, and the import block cli.rs needs (`PathBuf`,
`anyhow::{Context, Result}`, `clap::{Args, Parser, Subcommand}`).
`main.rs` keeps `CommandFactory` and `Parser` for `Cli::command()` and
`Cli::parse()`.

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
…ex_app_server

Three pure moves of contiguous top-level spans out of the 4,298-line
parent, verbatim apart from the visibility markers the parent and the
sibling test module need:

- `protocol.rs` (692 lines, 8 markers) — the app-server version probe and
  the JSON-schema admission walk.
- `context.rs` (264 lines, 5 markers) — Codex's occupancy arithmetic and
  the harness-context producer.
- `process_group.rs` (120 lines, 5 markers) — the wrapper-owned process
  group and its close-on-exec spawn.

`pub const CODEX_CONTEXT_VERIFIED_VERSION` deliberately stays in the
parent: moving a `pub` item into a private child would delete it from
`st2::codex_app_server`'s API with no compiler complaint. The state-dir +
io core stays too (313 LOC / 22 markers, `atomic_json` alone has 6
callsites).

Each parent glue is `mod X;` + `use self::X::*;`, so every existing path
resolves unchanged and `codex_app_server/tests.rs` needs no edit beyond
repointing its five `include_str!` fixtures, which the earlier test-tail
move left one directory too shallow.

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
Two pure moves of contiguous top-level spans, verbatim apart from
visibility markers:

- `watch_set.rs` (463 lines, 14 markers) — carrier classification, the
  coalescing windows, the per-agent watch set, resource coverage.
- `read.rs` (170 lines, 2 markers) — the confined, symlink-refusing read
  and its error classification.

`pub use watch_set::*;` is load-bearing: `crate::resync::ResyncCoverage`,
`resource_coverage_with_profiles`, and the two `st2::resync::watch_set_for*`
paths the integration targets use all stay byte-identical, and the glob
re-globs the `pub(super)` items into the parent namespace so
`resync/tests.rs`'s `use super::*;` needs no edit. `read`'s glue is a
private `use read::*;` — nothing it holds is public enough for a
`pub(crate)` re-export, which rustc says out loud.

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
…ty primitives

- `run/process.rs` (264 lines, 7 new markers) — the capture cap, the
  bounded reader, and every deadline-bounded `Command` runner.
- `catalog_transaction/capability.rs` (310 lines, 5 new markers) — every
  `openat`/`renameat` the transaction makes through a retained directory
  descriptor. Named `capability`, not `fs`: the parent does
  `use std::fs::{self, ..}`, so `mod fs;` is E0255.

Both parents keep `pub(crate) use <child>::*;`, so all 21 external
references — `crate::run::CAPTURE_CAP_BYTES` and friends in `ding`, the
18 `crate::catalog_transaction::*` paths in `agent_author`,
`agent_publish`, `catalog_archive`, `catalog_lock`, `message` and
`event` — resolve unchanged and no third file is touched.

`run/tests.rs` declares the two imports the moved test tail needs
directly (`ChildStdin`, `AsRawFd`) rather than inheriting them from a
parent that no longer uses them.

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
…t_author

1,949 of 3,709 lines relocate across 21 spans into five modules named
after concepts, not sizes:

- `markers.rs` (113) — the `meta { managed-by }` ownership authority
  (#473/#486). The only extraction that fixes a real cross-file boundary:
  `agent_publish` was reaching into the middle of a 3,709-line file.
- `stream.rs` (403) and `resource.rs` (604) — the two verb families, zero
  new markers between them.
- `desired_state.rs` (305) — the lifecycle verb, one new marker.
- `declared_field.rs` (524) — presentation *and* address, which the
  file's own doc calls one machine; splitting address out would cost 8
  markers for a 141-line module.

Six new `pub(super)` markers in total. The shared declaration writer —
error vocabulary, `AgentTarget`/`resolve_target`/`authorize_actor`, the
KDL node locator, the span-edit primitives, `atomic_replace_checked` —
stays in the parent: extracting it would leave `agent_author.rs` as glue
plus a test module, i.e. a `common.rs` that is `agent_author.rs`
renamed. The two `#[cfg(test)] *_for_test` scaffolds stay too, which is
what buys `pub(super)` instead of editing test code.

Glue is one block of `mod` + re-export lines at each item's own
visibility, so `st2::agent_author::*` (main.rs, tests/event_e2e.rs),
`crate::agent_author::*` (agent_publish.rs) and the inline test module's
`use super::*` all resolve unchanged: zero edits outside parent+children.

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
`tests/invariants.rs::qualified_proof_references_resolve` reads every
backticked `<path>.rs::<test>` span in INVARIANTS.md and asserts the test
is declared in that file, so moving a test tail invalidates the citation
— and the gate said so. Row text and guarantee are unchanged; only the
path is.

Same repointing for the two `.experiments` citations of the codex and
ding test modules.

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
38 citations named a test by the file that used to hold it. Rewrote the
path only — 21 `src/ding/mod.rs::`, 7 `src/resync.rs::`, 5
`src/run.rs::`, 5 `src/codex_app_server.rs::` — so all 257 qualified
proofs resolve again. No row text, guarantee, or proof set changed.

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
@schickling-assistant
schickling-assistant merged commit e58f9f7 into main Sep 7, 2026
4 of 5 checks passed
schickling-assistant added a commit that referenced this pull request Sep 7, 2026
…elete five unreachable items (#502)

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