-
Notifications
You must be signed in to change notification settings - Fork 3
fix(cli): stop calling unsafe std::env::set_var in compare/summary #399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ce71db4
8e6c17e
ca8e0e6
79aff4c
5d2688c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,9 +104,8 @@ pub struct SummaryArgs { | |
| #[arg(long)] | ||
| pub quality: bool, | ||
|
|
||
| /// Bypass the archive sidecar and stream the ledger. Kept for parity | ||
| /// with the TS CLI and as an escape hatch for archive-corruption | ||
| /// debugging. | ||
| /// Accepted for TS CLI flag parity; a no-op against the Rust SDK, | ||
| /// which is SQLite-native and has no archive layer to bypass. | ||
| #[arg(long = "no-archive")] | ||
| pub no_archive: bool, | ||
| } | ||
|
|
@@ -206,7 +205,9 @@ fn run_inner(globals: &GlobalArgs, args: SummaryArgs) -> anyhow::Result<i32> { | |
| None | ||
| }; | ||
|
|
||
| let _archive_guard = ArchiveOverride::activate(args.no_archive); | ||
| // `--no-archive` is accepted for TS CLI flag parity but is a no-op: | ||
| // the Rust SDK is SQLite-native and has no archive layer to bypass. | ||
| let _ = args.no_archive; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| let progress = TaskProgress::new(globals, "summary"); | ||
|
Comment on lines
+209
to
211
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 SummaryArgs The PR's stated intent is to make Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| let opts = match globals.ledger_path.as_deref() { | ||
|
|
@@ -346,43 +347,6 @@ fn run_ingest( | |
| }) | ||
| } | ||
|
|
||
| /// Drop-in for `RELAYBURN_ARCHIVE=0`. The Rust SDK is already SQLite-native, | ||
| /// but this preserves the TS CLI flag contract for any lower layer that checks | ||
| /// the env escape hatch. | ||
| struct ArchiveOverride { | ||
| previous: Option<String>, | ||
| activated: bool, | ||
| } | ||
|
|
||
| impl ArchiveOverride { | ||
| fn activate(no_archive: bool) -> Self { | ||
| if !no_archive { | ||
| return Self { | ||
| previous: None, | ||
| activated: false, | ||
| }; | ||
| } | ||
| let previous = std::env::var("RELAYBURN_ARCHIVE").ok(); | ||
| std::env::set_var("RELAYBURN_ARCHIVE", "0"); | ||
| Self { | ||
| previous, | ||
| activated: true, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for ArchiveOverride { | ||
| fn drop(&mut self) { | ||
| if !self.activated { | ||
| return; | ||
| } | ||
| match self.previous.take() { | ||
| Some(v) => std::env::set_var("RELAYBURN_ARCHIVE", v), | ||
| None => std::env::remove_var("RELAYBURN_ARCHIVE"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const COVERAGE_FIELDS: [CoverageField; 5] = [ | ||
| CoverageField::Input, | ||
| CoverageField::Output, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
summary --no-archiveis ignoredrun_innernow dropsargs.no_archiveas a no-op, but theSummaryArgshelp text in this file still describes--no-archiveas a functional archive-bypass escape hatch. That creates a user-visible contract mismatch: scripts or debugging workflows can pass the flag believing behavior changed when it did not. Please either update the help text to match the new no-op behavior (as done forcompare) or emit an explicit warning when the flag is provided.Useful? React with 👍 / 👎.