feat(preflight): --preflight-only flag (#100)#153
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a --preflight-only flag to the preflight command, enabling session-isolated audits that do not interfere with the authoritative events.jsonl log or state.json. The implementation includes a new run_preflight_with_options entry point in the engine and a sidecar event log (preflight-only.events.jsonl) that is truncated upon each new session. Feedback suggests simplifying the internal flush_events closure by capturing the path from the outer scope and optimizing event serialization by using serde_json::to_writer to avoid unnecessary string allocations.
| let mut flush_events = |log: &events::EventLog, path: &Path| -> Result<()> { | ||
| if fresh_audit_first_flush { | ||
| fresh_audit_first_flush = false; | ||
| log.write_to_file_truncating(path) | ||
| } else { | ||
| log.write_to_file(path) | ||
| } | ||
| }; |
There was a problem hiding this comment.
The path parameter in the flush_events closure is redundant as it always receives &events_path. Capturing events_path directly from the outer scope would simplify the closure definition and its call sites.
| let mut flush_events = |log: &events::EventLog, path: &Path| -> Result<()> { | |
| if fresh_audit_first_flush { | |
| fresh_audit_first_flush = false; | |
| log.write_to_file_truncating(path) | |
| } else { | |
| log.write_to_file(path) | |
| } | |
| }; | |
| let mut flush_events = |log: &events::EventLog| -> Result<()> { | |
| if fresh_audit_first_flush { | |
| fresh_audit_first_flush = false; | |
| log.write_to_file_truncating(&events_path) | |
| } else { | |
| log.write_to_file(&events_path) | |
| } | |
| }; |
| package: "all".to_string(), | ||
| }); | ||
| event_log.write_to_file(&events_path)?; | ||
| flush_events(&event_log, &events_path)?; |
There was a problem hiding this comment.
Update call site to match the simplified flush_events closure signature.
| flush_events(&event_log, &events_path)?; | |
| flush_events(&event_log)?; |
| package: "all".to_string(), | ||
| }); | ||
| event_log.write_to_file(&events_path)?; | ||
| flush_events(&event_log, &events_path)?; |
There was a problem hiding this comment.
Update call site to match the simplified flush_events closure signature.
| flush_events(&event_log, &events_path)?; | |
| flush_events(&event_log)?; |
| package: "all".to_string(), | ||
| }); | ||
| event_log.write_to_file(&events_path)?; | ||
| flush_events(&event_log, &events_path)?; |
There was a problem hiding this comment.
Update call site to match the simplified flush_events closure signature.
| flush_events(&event_log, &events_path)?; | |
| flush_events(&event_log)?; |
| for event in &self.events { | ||
| let line = serde_json::to_string(event).context("failed to serialize event to JSON")?; | ||
| writeln!(writer, "{}", line).context("failed to write event line")?; | ||
| } |
There was a problem hiding this comment.
Using serde_json::to_writer is more efficient than serde_json::to_string followed by writeln! as it avoids allocating an intermediate string for each event. This is particularly beneficial if the event log contains many entries or large payloads (e.g., captured command output).
| for event in &self.events { | |
| let line = serde_json::to_string(event).context("failed to serialize event to JSON")?; | |
| writeln!(writer, "{}", line).context("failed to write event line")?; | |
| } | |
| for event in &self.events { | |
| serde_json::to_writer(&mut writer, event).context("failed to serialize event to JSON")?; | |
| writeln!(writer).context("failed to write event line")?; | |
| } |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
3b885ac to
3623957
Compare
Closes the last gap in the Prove competency (#100): operators can now re-run a finishability assessment against the current workspace without disturbing any in-flight publish state. `shipper preflight --preflight-only` performs a session-isolated audit — it neither reads nor appends to the authoritative `events.jsonl`, never writes `state.json`, and instead emits its trace to a sidecar at `.shipper/preflight-only.events.jsonl` (truncated each invocation). The CLI flag is scoped to the `preflight` subcommand and threads through a new `engine::PreflightRunOptions` with `fresh_audit: bool`; the historical `run_preflight()` entry point is preserved as a back-compat wrapper. Three tests pin the behavior: CLI flag parses and defaults to `false`, fresh-audit ignores prior state.json + events.jsonl seeds, and repeated fresh audits reflect current registry reality (truncate-on-first-write, not accumulated).
3623957 to
74f0645
Compare
Summary
Closes the last open gap in the Prove competency (#100). Adds
shipper preflight --preflight-only, a session-isolated re-audit that lets operators get a freshFinishability(Proven / NotProven / Failed) signal without disturbing any in-flight publish state.--preflight-onlyflag, scoped to thepreflightsubcommand only (clap rejects it onpublish,resume, etc.). Defaults tofalsefor full back-compat.engine::run_preflight_with_options()entry point takingPreflightRunOptions { fresh_audit: bool }. The historicalrun_preflight()is preserved as a back-compat wrapper that delegates with defaults. Whenfresh_audit = true:events.jsonl(events-as-truth invariant for the publish flow stays intact).state.json(no publish state side-effect).<state_dir>/preflight-only.events.jsonl, truncated on first flush and appended thereafter (one fresh trace per invocation, not accumulated).EventLog::write_to_file_truncating()helper plus apreflight_only_events_path()resolver and aPREFLIGHT_ONLY_EVENTS_FILEconstant.Acceptance criteria (from the plan scout's #100 comment)
--preflight-onlyparses and is scoped topreflight(rejected on incompatible commands by clap).state.jsonand priorevents.jsonl.Finishabilityreflects current filesystem + registry state, not cached.run_preflightwrapper.Tests
Three new tests added, all passing:
preflight_only_flag_parses_and_defaults_to_false(shipper-cli) — pins clap surface and back-compat default.run_preflight_fresh_audit_ignores_prior_state(shipper-core) — seeds sentinelevents.jsonl+state.json, asserts both are byte-identical after a fresh audit and that the sidecar carries the full trace.run_preflight_with_dirty_git_fresh_audit(shipper-core) — runs fresh audit twice against differing registry realities (NotProven then Proven), asserts authoritative log stays absent and sidecar truncates between runs (exactly onePreflightStarted).Test plan
cargo fmt --all -- --checkcleancargo clippy --workspace --all-targets --all-features -- -D warningscleancargo test -p shipper-cli -p shipper-core(the only flakes I observed are the pre-existingpreflight_command_finishability_proven/preflight_command_snapshottests incli_e2e.rs, which fight overtarget/packagescratch space when run in parallel; both pass with--test-threads=1and neither test was touched by this PR)cargo run -p shipper -- preflight --preflight-onlyagainst a workspace with a stale.shipper/and confirmevents.jsonlis unchanged whilepreflight-only.events.jsonlis freshOut of scope
Per the plan scout's comment: multi-registry fresh-audit semantics, automated re-preflight after edits, and CI hook integration are deferred. Recommend closing #100 on merge.