fix(acp): accept conventional boolean spellings for env-backed flags - #4922
Open
Pratikkale26 wants to merge 1 commit into
Open
fix(acp): accept conventional boolean spellings for env-backed flags#4922Pratikkale26 wants to merge 1 commit into
Pratikkale26 wants to merge 1 commit into
Conversation
`BUZZ_ACP_NO_MENTION_FILTER=1` aborted argument parsing with exit code 2,
so the managed agent never started — it never reached the relay, never
subscribed, and never posted. `1` is the spelling most tools accept for a
boolean environment variable, but these flags are bare `bool` fields whose
parser accepts only the literal strings `true` and `false`.
The failure is also close to invisible from Buzz Desktop: `last_error` in
`managed-agents.json` is just `harness exited with status exit code: 2`,
and the clap line naming the offending variable goes only to the per-agent
log file. In the reported case the agent stayed down about 16 hours.
Give every env-backed boolean flag clap's `BoolishValueParser`, which
accepts `true/false`, `1/0`, `yes/no` and `on/off`, with
`num_args = 0..=1` and `default_missing_value = "true"` so the bare switch
form (`--no-presence`) keeps working unchanged. Nonsense values are still
rejected, and `--memory` / `--no-memory` still conflict.
Nine flags are affected, not the six listed in the issue: the reporter
verified only `--no-mention-filter` and explicitly did not claim the
others. `--memory`, `--relay-observer` and `--lazy-pool` have the same
bare-bool-plus-env shape and the same failure.
`lazy_pool_cli_flag_enables_deferred_startup` asserted that
`--lazy-pool=true` must be an error ("bool flags do not take an explicit
value"). That assertion characterised clap's behaviour for the flag as it
was introduced rather than a product invariant, and it is exactly the
behaviour this fixes, so it now asserts the boolish form instead.
The issue's second suggestion — surfacing the harness's stderr in Desktop's
`last_error` instead of a bare exit code — is not addressed here.
Adds coverage for all nine flags: conventional spellings resolve correctly,
the bare switch form still works, defaults are preserved when absent
(notably `--memory` staying on), nonsense values are still rejected, and
the memory conflict still holds. Following the convention documented in
this test module, values go through the CLI form rather than
`std::env::set_var`, which would race on process-global state; env values
flow through the same `value_parser`.
Fixes block#4881
Signed-off-by: pratikkale26 <pratikkale7661@gmail.com>
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
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
BUZZ_ACP_NO_MENTION_FILTER=1abortsbuzz-acpduring argument parsing with exit code 2, so the managed agent never starts — it never reaches the relay, never subscribes, and never posts.1is the spelling most tools accept for a boolean environment variable, but these flags are bareboolfields whose parser accepts only the literal stringstrueandfalse.Fixes #4881.
Reproduced
Probing the current parser for
BUZZ_ACP_NO_MENTION_FILTER:The fix
Every env-backed boolean flag now uses clap's
BoolishValueParser(true/false,1/0,yes/no,on/off), withnum_args = 0..=1anddefault_missing_value = "true"so the bare switch form keeps working:num_args = 0..=1is safe here:CliArgshas no positional arguments (43#[arg]attributes, 0 positional), so a value-less--flagcannot swallow anything.Nine flags, not six
The issue lists six and is explicit about not having tested them. Enumerating every
boolfield carrying anenvattribute turns up nine —--memory,--relay-observerand--lazy-poolhave the identical bare-bool-plus-env shape and the identical failure:--no-mention-filterBUZZ_ACP_NO_MENTION_FILTER--no-ignore-selfBUZZ_ACP_NO_IGNORE_SELF--no-presenceBUZZ_ACP_NO_PRESENCE--no-typingBUZZ_ACP_NO_TYPING--no-memoryBUZZ_ACP_NO_MEMORY--no-base-promptBUZZ_ACP_NO_BASE_PROMPT--memoryBUZZ_ACP_MEMORY--relay-observerBUZZ_ACP_RELAY_OBSERVER--lazy-poolBUZZ_ACP_LAZY_POOLOne existing assertion changed
lazy_pool_cli_flag_enables_deferred_startupasserted that--lazy-pool=truemust be an error:That came from #2122, the PR that introduced
--lazy-pool, and characterises clap's behaviour for the flag as it was written rather than a product invariant — and it is precisely the behaviour this issue asks to change. It now asserts the boolish form instead. Flagging it explicitly since it's a deliberate change to an existing assertion.Not addressed
The issue's second suggestion — surfacing the harness's stderr in Desktop's
last_errorinstead of a bare exit code — is a separate change in the Desktop process supervisor and isn't in this PR. Worth doing: it's what turns any future startup failure from a 16-hour outage into a readable message. Happy to follow up.Testing
Five new tests covering all nine flags:
1/yes/on→ true,0/no/off→ false)--memorystaying onmaybe) are still rejected — this widens accepted spellings, it doesn't accept anything--memory/--no-memorystill conflictValidated by running the new tests against the unfixed flags —
boolish_flags_accept_conventional_boolean_spellingsfails withunexpected value 'true' for '--no-mention-filter' found; no more were expected.Following the convention documented in this test module, values go through the CLI form rather than
std::env::set_var, which would race on process-global state across parallel tests; env values flow through the samevalue_parser.Related
no_mention_filter, but from the other side: that one is about the flag's state being unobservable when it works; this is about the failure being unobservable when it doesn't.