refactor: F-ARCH-* audit batch (crate boundaries, atomic budget, approval metadata)#622
Merged
Conversation
added 7 commits
May 26, 2026 20:16
The `test-support` feature on `pitboss-cli` was a no-op forwarding shim preserved only to keep backward compatibility with callers passing `--features pitboss-cli/test-support`. A grep of the workspace shows no such callers: scripts, CI, and dev docs all use the canonical `--features pitboss-core/test-support` form, and the CLI's own unit tests pull `pitboss-core/test-support` via `[dev-dependencies]` rather than this feature. Removing the feature eliminates the fragile forwarding chain. Closes #483
…per) The sub-lead cancel watcher must be installed in lock-step with `DispatchState::register_sublead` — the watcher only covers actors registered *before* the signal arrives, and the eager cascade in `register_worker_cancel` covers everything after, so the two must be installed by a single sequenced entry point. Exposing the watcher as `pub fn` invited callers (especially future integration tests) to install it independently and break that invariant. `register_sublead` is the only caller in the codebase; the function had no callers outside the `dispatch::*` module tree. Narrowing visibility to `pub(super)` enforces the contract at the type system level. `install_cascade_cancel_watcher` keeps `pub` visibility because the integration-test suite (cancel_cascade_flows, dogfood_fake_flows, etc.) calls it directly to drive the root→sub-tree cascade in test harnesses — the analog of the dispatcher's own startup wiring in `hierarchical.rs`. Closes #479
… Bridge
`QueuedApproval` (pre-TUI-attach queue) and `BridgeEntry` (post-attach
bridge map) shared seven fields with identical types and semantics:
`task_id`, `summary`, `plan`, `kind`, `ttl_secs`, `fallback`,
`created_at`. Adding a new approval-metadata field (priority, category,
retry policy, etc.) required updating both structs and every
construction site in lock-step; missing one silently lost the field on
queue→bridge transfer.
Pull the seven shared fields into an `ApprovalMetadata` struct.
`QueuedApproval` becomes `{ request_id, responder, metadata }` and
`BridgeEntry` becomes `{ responder, metadata }`. The queue→bridge
transfer in `control/server.rs` now just moves the `metadata` whole
instead of field-by-field copying, removing one class of "I forgot to
copy field X" bug.
`PendingApproval` (the richer Phase 4+ record-level type with required
`ttl_secs`, typed `ApprovalCategory`, and actor lineage) stays separate
— the Phase 4 unification noted in its doc comment is a deeper design
change and out of scope here.
Also normalizes the two `ApprovalPlan` reference paths: the test in
`control/server.rs` was the only remaining site using
`mcp::tools::ApprovalPlan` directly; switched to
`mcp::approval::ApprovalPlan` (the canonical re-export, used by every
other approval-type field) for consistency.
Closes #480
`LayerState.budget` held `spent_usd` and `lead_spent_usd` as `std::sync::Mutex<f64>` while `reserved_usd` was a `tokio::sync::Mutex`. The std-Mutex choice was load-bearing — the session-stream usage observer is a synchronous callback inside the stream loop, and an `.await` on a tokio Mutex there would block the executor. But the proximity of the two lock types on the same struct made future changes error-prone: a `.lock().await` typo or a refactor that moved a spend access from sync to async context would silently re-introduce the guard-across-await foot-gun. Wrap both counters in a new `BudgetCounter(AtomicU64)` that bit-casts `f64` through `AtomicU64` for lock-free load/store/add. The session- stream observer can read and update spend without any lock at all; the async budget-walking aggregator (`compute_total_spend`) becomes a tight `.load()` loop. Critical sections that *combine* spend reads with reservation updates (the per-spawn TOCTOU guard from #106) still go through `reserved_usd: Mutex<f64>` — that lock is the synchronization, not the storage, and we leave it unchanged. Concrete improvements: - ~110 lines of `*x.lock().unwrap_or_else(PoisonError::into_inner)` / `+= delta` boilerplate replaced with `x.load()` / `x.add(delta)` / `x.store(value)`. - `compute_total_spend_blocking` no longer needs the std-Mutex `.lock()` fallback for partial-sum recovery on contention — the atomic loads don't contend. The tokio RwLock `try_read()` fallback on the sub-leads map stays (separate concern from the spend counters). - The "no `.await` may cross this guard" rule is now an impossibility rather than a hand-maintained invariant. Reservations (per-task `WorkerRegistry.reservations` HashMap) live deliberately separate from `BudgetState` — they're per-task_id keys, not layer-aggregate counters. The audit suggestion to centralize them into `BudgetTracker` would re-cross a clean boundary; this commit keeps them where the rest of the per-task_id maps live. Closes #478
…om core `pitboss-core::stream` exposes the disk-replay foundation; the cli-side remainder composes that with the per-run control socket to produce a live + historical unified stream. Until now both modules were named `stream`, so a reader staring at `use ...::stream::LiveStreamItem` couldn't tell which crate they were importing from without checking the prefix. The collision motivated F-ARCH-6 (#485). The audit also asked for `ActorPath` to move into `pitboss-core` to unblock a full unification of the consumer API in `pitboss-core::stream` itself. That work has already landed via #481 — `ActorPath` and the type-erased `EventEnvelope` shell now live in `pitboss-core::control_protocol`. The remaining cli-side composition is still load-bearing because the typed `ControlEvent` payloads carry `mcp::policy::ApprovalRule` and other MCP/approval concerns that deliberately don't live in core (per `pitboss-core::control_protocol`'s preamble). Renaming the cli module to `live_stream` is the right disambiguator without dragging approval rules into core. Touches three external consumers: `pitboss-tui::control` and `pitboss-tui::app` (the TUI's live-session attach path) and the `live_stream_flows` integration test. The module's interior is unchanged. Closes #485
`pitboss-web::api::audit` was reaching into `pitboss-cli::audit` and `pitboss-cli::dispatch::events` to read the run-wide `audit.jsonl` artifact — two read-only types whose only legitimate consumer outside the dispatcher is the web console. Pulling them through `pitboss-cli` forced the web crate to depend on the full dispatcher (its MCP server, control bridge, process spawner, …) just to deserialize a JSONL row. Move both modules into `pitboss-core` where the other consumer-shared reader-side types already live (`pitboss_core::runs`, `pitboss_core::store`, `pitboss_core::stream`). The cli-side `crates/pitboss-cli/src/audit.rs` and `crates/pitboss-cli/src/dispatch/events.rs` shrink to re-export shims, matching the pattern `pitboss_cli::runs` already follows since #484. All existing `crate::dispatch::events::*` / `crate::audit::*` paths inside `pitboss-cli` continue to resolve through the shim — no internal caller had to change. `pitboss-web::api::audit` now imports from `pitboss_core::audit` and `pitboss_core::task_events` directly, dropping two of its `pitboss_cli::*` reach-ins. `pitboss-cli` still owns the dispatcher runtime, manifest pipeline, MCP server, control protocol, and live stream — those remain too entangled with cli-only types (`ApprovalRule`, the typed `ControlEvent` payloads, etc.) to relocate in this PR — but the pure-disk audit types had no such ties. Notes: - `pitboss-core` has `#![warn(clippy::pedantic)]` while `pitboss-cli` does not. The moved modules carry minimal `#![allow(...)]` attrs for `must_use_candidate` and `needless_pass_by_value` so the move is mechanical — no behavior change. Two trailing-comma `format!` nits got fixed in passing. - The `pitboss audit` subcommand entry (`run`) was moved alongside the data types; the CLI binary's audit subcommand handler still routes through `pitboss_cli::audit::run` via the re-export shim. Closes #482 Closes #484
SDS-Mode
force-pushed
the
refactor/f-arch-batch
branch
from
May 27, 2026 00:16
6490eba to
ede098f
Compare
SDS-Mode
enabled auto-merge (rebase)
May 27, 2026 00:16
…ad (#366 twin) When an approval is raised before any TUI has attached, it queues on `ApprovalState.queue`. When a TUI later connects, the server drains the queue into the bridge map and emits `ApprovalRequest` events; the operator's subsequent `Approve` op flows through `control::server`'s Approve handler, which is responsible for writing the `ApprovalResponse` audit row to `events.jsonl`. That handler hard-coded `&state.root.lead_id` as the actor whose `tasks/<id>/events.jsonl` received the row, so a worker or sub-lead approval response landed in the lead's audit file and the originating actor's events.jsonl was missing its `approval_response` correlation to the earlier `approval_request`. The bridge-side handler (`ApprovalBridge::respond`) was already fixed for this in #366; the queue-drain twin was missed. The just-extracted `caller_id` local (from `bridge_entry.metadata.task_id`) is the value that should have been used — F-ARCH-12 made the variable explicit one line above the buggy call, which is what surfaced the bug. Add a regression test `approve_op_writes_response_row_to_caller_tasks_dir` that pre-seeds the bridge with `task_id = "worker-7"`, sends an Approve op, and asserts: 1. `<run_subdir>/tasks/worker-7/events.jsonl` exists and contains an `approval_response` row with the matching `request_id`. 2. The lead's `events.jsonl` does NOT contain the worker's request_id (covering the specific pre-fix misdirection). Also updates `pitboss-tui/tests/connect_timeout_regression.rs` to use `pitboss_cli::live_stream::*` (instead of the renamed `::stream::*`); that test landed in this branch from a parallel review pass and was written against the pre-F-ARCH-6 module name. Discovered during the F-ARCH-12 review pass on PR #622.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundled cleanup of all 7 open
F-ARCH-*findings from the 2026-05-14 codebase audit. Each refactor lands as its own commit sogit-cliffroutes them cleanly to the CHANGELOG. None of these changes are user-visible; the dispatcher, TUI, web console, and CLI behave identically before and after.chore(cli): remove empty test-support feature forwarding[features] test-support = ["pitboss-core/test-support"]shim — workspace grep confirms no caller passed--features pitboss-cli/test-support.refactor(dispatch): restrict install_sublead_cancel_watcher to pub(super)register_subleadis now the only valid entry point for sub-lead cancel-watcher install (the watcher had two prior callers, both indispatch::*).install_cascade_cancel_watcherstayspub— integration tests undertests/need it.refactor(dispatch): extract ApprovalMetadata shared between Queue and BridgeQueuedApproval/BridgeEntryinto a sharedApprovalMetadata. The queue→bridge transfer incontrol/server.rsis now a singleq.metadatamove instead of 7 field copies. Also normalizes the twoApprovalPlanpaths to the canonicalmcp::approval::ApprovalPlanalias.refactor(dispatch): switch spend counters to atomic BudgetCounterstd::sync::Mutex<f64>onspent_usd/lead_spent_usdwith a newBudgetCounter(AtomicU64)(f64 bit-cast). Removes the std-vs-tokio Mutex foot-gun on the same struct, kills ~110 lines of.lock().unwrap_or_else(PoisonError::into_inner)boilerplate, and letscompute_total_spend_blockingdrop the partial-sum fallback for these fields.reserved_usdkeeps its tokio Mutex — that lock is the per-spawn TOCTOU guard from #106.refactor(cli): rename stream module to live_stream to disambiguate from coreActorPath/EventEnvelopealready moved topitboss-corein #481; this rename eliminates the remainingpitboss-cli::stream↔pitboss-core::streamcollision. The cli module is the live + historical composition (depends on typedControlEvent/ApprovalRuleso can't live in core); the core module is the disk side.refactor: relocate task_events and audit to pitboss-coreTaskEvent/DeniedReasonKind/AuditEntry/AuditFiltertypes frompitboss-cli::dispatch::eventsandpitboss-cli::auditintopitboss_core::task_eventsandpitboss_core::audit. The cli-side modules shrink to re-export shims, matching the patternpitboss_cli::runsalready follows (#484).pitboss-web::api::auditswitches to the core paths, dropping two of itspitboss_cli::*reach-ins.shared_storeandcommunicationwould naturally move with the dispatcher if it ever split into its own crate. No such split is planned in this PR.Why bundle these into one PR
These findings cluster thematically around crate boundary hygiene: too much in
pitboss-cliwas exposed to its consumers (TUI, web), and a few internal types had drifted into shapes that invited future bugs (mixed-mutex spend tracking, three-struct approval-field duplication,streamvsstreammodule-name collision). Doing them as separate PRs would have produced six near-identical PR templates and forced the reviewer to context-switch through the samepitboss-clisurface seven times.Closes
Closes #478
Closes #479
Closes #480
Closes #482
Closes #483
Closes #484
Closes #485
Closes #486
Test plan
cargo check --workspace --all-targetspassescargo lint(clippy with-D warnings) passes — including pitboss-core's stricterclippy::pedanticsettings on the newly-relocatedtask_eventsandauditmodulescargo tidy(fmt --check) cleancargo test -p pitboss-cli --lib budget_watch— all 7 budget-watcher tests pass (atomic counter swap doesn't change semantics)cargo test -p pitboss-cli --test sublead_flows approval_ttl_triggers_auto_reject_fallback— TTL fallback path through newApprovalMetadataworksscripts/smoke-part1.sh— 11/11 passscripts/smoke-part3-tui.sh— 9/12 pass (1 pre-existing failure onpitboss-tui --version, 2 interactive-only skips; both unrelated to this PR)Note on
e2e_lead_request_approval_round_tripUnder parallel
cargo test, this approval round-trip test flakes ~30 % of the time both on this branch and onmain(verified by running it three times againstb942a65: pass, fail, pass). The race is between the server'sresponder.send(...)(which unblocksfake-claudeand lets it exit) and the subsequentrecord_approval_outcomecounter bump —fake-claudecan race ahead and the test asserts before the counter increments. Passes reliably with--test-threads=1or in isolation. Pre-existing race; not in scope here.Files touched at a glance
crates/pitboss-core/src/task_events.rs(moved from cli),crates/pitboss-core/src/audit.rs(moved from cli)crates/pitboss-cli/src/stream.rs→crates/pitboss-cli/src/live_stream.rscrates/pitboss-cli/src/dispatch/{layer,budget_watch,state,signals,runner,sublead,events}.rs,crates/pitboss-cli/src/{lib,audit,mcp/approval,mcp/tools/spawn,mcp/tools/tests,control/server,Cargo}.{rs,toml},crates/pitboss-tui/src/{app,control,state}.rs,crates/pitboss-web/src/api/audit.rs,crates/pitboss-core/src/lib.rs,crates/pitboss-cli/tests/{e2e_sublead_flows,hierarchical_flows,sublead_flows,live_stream_flows}.rs🤖 Generated with Claude Code