Surface config flags in clap help output (including locale)#103
Open
Surface config flags in clap help output (including locale)#103
Conversation
|
This PR implements ExecPlan 3-2-1 (docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md) to surface six Weaver runtime configuration flags in clap-generated help and manpages — --config-path, --daemon-socket, --log-filter, --log-format, --capability-overrides and --locale — for both weaver --help and weaver daemon start --help, while preserving runtime parsing semantics and precedence (defaults < files < environment < CLI). Core changes
Implementation safety notes
Outstanding review/CI follow-ups
Files of note
This summary links the new ExecPlan to the implemented code and documentation changes and notes the remaining review hygiene items to close the PR. WalkthroughSummarise the added help rendering path that builds an augmented clap Command exposing shared config flags (including --locale), a preflight early‑exit guidance boundary, a runner glue layer for daemon RPC I/O, and tests/docs to ensure help/manpage outputs advertise config flags while runtime semantics require flags before the command domain. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Parser
participant Help as Help Module
participant Preflight as Preflight Check
participant Config as Config System
participant Daemon as Daemon (socket)
User->>CLI: Invoke weaver [argv]
CLI->>Help: Inspect argv for --help/--version
alt Help requested
Help->>Help: Build augmented Command (Cli::command + config metadata + --config-path)
Help->>User: write_help_for_args() -> stdout
CLI-->>User: Exit SUCCESS
else Normal execution
CLI->>Preflight: handle_preflight(parsed_cli, stderr, localizer)
alt Bare invocation or domain guidance needed
Preflight->>User: Emit guidance -> stderr
CLI-->>User: Exit preflight error
else Proceed
CLI->>Config: Load config (defaults < file < env < CLI)
Config-->>CLI: Resolved config (includes locale)
CLI->>Daemon: execute_daemon_command (connect or auto‑start)
Daemon-->>CLI: Stream responses (JSONL)
CLI-->>User: Command output / ExitCode
end
end
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 5 warnings, 2 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideSurfaces six runtime configuration flags (including the new locale field) in clap-generated help and manpages via a shared augmented Command, while keeping the existing config precedence and flag-ordering semantics unchanged, and factors preflight/help logic into dedicated modules with corresponding tests and docs updates. Sequence diagram for CLI help handling with shared augmented CommandsequenceDiagram
actor Operator
participant CliRunner as CliRunner
participant ClapCli as Cli
participant Help as help_command
participant ConfigMeta as Config_doc_metadata
participant Stdout as stdout
Operator->>CliRunner: run_with_handler(argv)
CliRunner->>CliRunner: split_config_arguments(argv)
CliRunner->>CliRunner: prepare_cli_arguments(argv, split)
CliRunner->>ClapCli: try_parse_from(cli_arguments)
ClapCli-->>CliRunner: Error(DisplayHelp)
CliRunner->>CliRunner: write_help_for_args(argv, stdout)
CliRunner->>Help: command()
Help->>ConfigMeta: Config::get_doc_metadata()
ConfigMeta-->>Help: FieldMetadata for config fields
Help-->>CliRunner: augmented Command
CliRunner->>Help: try_get_matches_from(argv)
Help-->>CliRunner: Error(DisplayHelp)
CliRunner->>Stdout: write(help_text)
Stdout-->>Operator: rendered help with 6 config flags
CliRunner-->>Operator: exit(0) without starting daemon
Updated class diagram for Config, Locale, and help/preflight modulesclassDiagram
class Config {
+SocketEndpoint daemon_socket
+String log_filter
+LogFormat log_format
+Vec~CapabilityDirective~ capability_overrides
+Locale locale
+Config default()
+capability_matrix() CapabilityMatrix
+locale() &Locale
-normalise_capability_overrides() void
}
class Locale {
-LanguageIdentifier 0
+Locale en_us()
+Locale default()
+to_string() String
+from_str(value: &str) Result~Locale,LocaleParseError~
}
class LocaleParseError {
-String input
+to_string() String
}
class HelpModule {
+command() Command
-config_path_arg() Arg
-config_field_arg(field: FieldMetadata) Option~Arg~
-apply_arg_shape(arg: Arg, cli: CliMetadata) Arg
}
class PreflightModule {
+handle_preflight(cli: Cli, split: ConfigArgumentSplit, stderr: ErrWriter, localizer: Localizer) Result~(),AppError~
-emit_domain_guidance(cli: Cli, stderr: ErrWriter, localizer: Localizer, raw_domain: &str) Result~(),AppError~
-preflight_result(written: bool) Result~(),AppError~
}
class AppError {
<<enum>>
PreflightGuidance
BareInvocation
EmitBareHelp(io::Error)
EmitHelp(io::Error)
CliUsage(clap::Error)
}
class CliRunner {
+run_with_handler(args: &[OsString]) ExitCode
-write_help_for_args(args: &[OsString], writer: &mut Write) io::Result~()~
}
Config --> Locale : has
Locale --> LocaleParseError : error
HelpModule --> Config : uses
HelpModule --> CliRunner : used_by
PreflightModule --> CliRunner : used_by
PreflightModule --> AppError : returns
CliRunner --> AppError : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
force-pushed
the
surface-config-flags-in-help-taop69
branch
from
775be84 to
e368e50
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e368e50bd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/weaver-cli/src/lib.rs (1)
35-60: 🛠️ Refactor suggestion | 🟠 MajorCollapse the config-flag catalogue to one source of truth.
help::command()now discovers config flags fromConfig::get_doc_metadata().fields, but runtime splitting still depends on this hand-maintained slice. The next config field will show up in--helpand remain inert at runtime until somebody editsCONFIG_CLI_FLAGS, which recreates the contract drift this PR is meant to remove. Source both paths from the same metadata, or centralize the catalogue in one shared helper.Based on learnings, "Define the shared configuration schema for
weaver-cliandweaverdinweaver-config, usingortho-configto merge config files, environment overrides, and CLI flags for daemon sockets, logging, and the capability matrix defaults."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/src/lib.rs` around lines 35 - 60, Replace the hand-maintained CONFIG_CLI_FLAGS slice with a single source of truth derived from the configuration metadata: remove or stop using the CONFIG_CLI_FLAGS constant and instead obtain the CLI flag names from Config::get_doc_metadata().fields (the same place help::command() reads from), then pass that generated list into split_config_arguments/prepare_cli_arguments where CONFIG_CLI_FLAGS was used; ensure the extraction maps metadata field names to the exact flag strings (e.g. "--config-path", "--daemon-socket", etc.) and preserve any ordering/escaping logic previously assumed by split_config_arguments so runtime flag-splitting and help generation stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaver-cli/src/help.rs`:
- Around line 17-24: The generated help currently exposes config flags as global
clap options but doesn't state the parsing order caveat; update the help text
for the Command built in command() (the code that iterates
Config::get_doc_metadata().fields and calls config_field_arg()) to append a
clear note in long_about or after_help stating that config flags must appear
before the domain or structured subcommand (e.g., "Config flags are only honored
when placed before the domain/structured subcommand, e.g., `weaver daemon start
--flag` must place --flag before the subcommand"); apply the same addition to
the other help augmentation site referenced around lines 48-51 so both places
document the ordering rule.
In `@crates/weaver-cli/src/lib.rs`:
- Around line 251-268: The file exceeds the 400-line limit; extract the
help/runner plumbing by moving the write_help_for_args function and any related
runner/help helpers into a new module (e.g., help.rs or runner_help.rs) and
re-export or call it from lib.rs; specifically, relocate the function
write_help_for_args and any closely related helper functions or types used by
the runner/help code into that module, update lib.rs to use mod help; use
pub(crate) or pub as needed and adjust imports (help::write_help_for_args or
re-export) so callers continue to compile, then run cargo check to verify no
unresolved imports remain.
In `@crates/weaver-cli/tests/main_entry.rs`:
- Around line 146-157: The test daemon_start_help_lists_all_config_flags must
also assert that invoking "weaver daemon start --help" does not proceed into the
daemon startup path; update the test function
daemon_start_help_lists_all_config_flags to add negative assertions that the
startup log/message ("Waiting for daemon start...") is not present on stdout or
stderr by appending .stdout(predicates::str::contains("Waiting for daemon
start...").not()) and .stderr(predicates::str::contains("Waiting for daemon
start...").not()) to the existing assert chain (use predicates::str::contains to
build the negative checks).
In `@crates/weaver-config/src/lib.rs`:
- Around line 101-108: Update the crate-level Rustdoc to reflect the new public
field by adding "locale" to the opening schema summary and ensure the module
begins with a module-level (`//!`) comment; specifically edit the top-of-file
crate docs in lib.rs to include "locale" alongside the existing configuration
concerns and add or update the leading //! comment block so it documents the
module's purpose and lists the fields (including the public pub locale: Locale
field and reference to default_locale()).
In `@crates/weaver-config/tests/configuration_precedence.rs`:
- Around line 136-139: The Harness::set_env call performs unguarded global env
mutation (uses unsafe std::env::set_var) and races under parallel tests; wrap
all test environment mutations behind a shared synchronization primitive: add a
static Mutex<()> (in the shared test_utils/test_helpers crate) and modify
Harness::set_env to acquire that mutex before calling std::env::set_var and to
return a guard-style RAII object that holds the mutex and restores the previous
value in its Drop (or alternatively switch Harness to use dependency
injection/mocking via the mockable crate); ensure all tests call the new guarded
API so env sets are serialized and restored safely.
In `@docs/roadmap.md`:
- Around line 294-299: Mark the roadmap parent task for item "3.2.1" as complete
by updating its headline checkbox to checked (e.g., change the parent checklist
item for 3.2.1 to [x]) so the document no longer shows the feature as open;
ensure the headline references the same sub-items and the six flags
(--config-path, --daemon-socket, --log-filter, --log-format,
--capability-overrides, --locale) and acceptance criteria remain unchanged.
In `@docs/weaver-design.md`:
- Around line 697-699: Change the British spelling "localisation" to the Oxford
-ize form "localization" in the phrase under Roadmap item `3.2.1` (replace the
token "localisation" where it appears in the sentence referencing clap
localisation/Command usage) so the documentation matches the repository's
en-GB-oxendict (-ize) spelling policy.
---
Outside diff comments:
In `@crates/weaver-cli/src/lib.rs`:
- Around line 35-60: Replace the hand-maintained CONFIG_CLI_FLAGS slice with a
single source of truth derived from the configuration metadata: remove or stop
using the CONFIG_CLI_FLAGS constant and instead obtain the CLI flag names from
Config::get_doc_metadata().fields (the same place help::command() reads from),
then pass that generated list into split_config_arguments/prepare_cli_arguments
where CONFIG_CLI_FLAGS was used; ensure the extraction maps metadata field names
to the exact flag strings (e.g. "--config-path", "--daemon-socket", etc.) and
preserve any ordering/escaping logic previously assumed by
split_config_arguments so runtime flag-splitting and help generation stay in
sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f24dcff-e524-4859-b140-2de6468dbfae
📒 Files selected for processing (18)
crates/weaver-cli/Cargo.tomlcrates/weaver-cli/build.rscrates/weaver-cli/src/errors.rscrates/weaver-cli/src/help.rscrates/weaver-cli/src/lib.rscrates/weaver-cli/src/preflight.rscrates/weaver-cli/src/tests/unit.rscrates/weaver-cli/src/tests/unit/help_output.rscrates/weaver-cli/tests/features/weaver_cli.featurecrates/weaver-cli/tests/main_entry.rscrates/weaver-config/src/lib.rscrates/weaver-config/src/locale.rscrates/weaver-config/tests/configuration_precedence.rscrates/weaver-config/tests/features/configuration_precedence.featuredocs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.mddocs/roadmap.mddocs/users-guide.mddocs/weaver-design.md
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
docs/roadmap.md (1)
285-299:⚠️ Potential issue | 🟡 MinorMark roadmap item
3.2.1complete.Update the parent checkbox as well. The sub-task and acceptance criteria underneath it are already marked done, so the headline item now reports shipped work as still open.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/roadmap.md` around lines 285 - 299, The roadmap entry 3.2.1 still lists the top-level checkbox as open despite its subitems being completed; edit the "3.2.1. Surface configuration flags in clap help output." headline checkbox to checked (mark the parent checkbox complete) so it matches the already-checked subtask lines for `--config-path`, `--daemon-socket`, `--log-filter`, `--log-format`, `--capability-overrides`, and `--locale` and leave the acceptance criteria text unchanged.crates/weaver-cli/src/preflight.rs (1)
109-185: 🧹 Nitpick | 🔵 TrivialCollapse the repeated preflight cases into
rstesttables.Duplicate the setup once, then parameterize the scenario inputs and expected guidance. These tests repeat the same arrange/act/assert flow, and the unknown-domain and missing-operation cases differ only by the input tuple and assertion needle. Fold them into
#[rstest]cases with a shared fixture for the localizer and buffers so later guidance paths stay aligned.As per coding guidelines, "Use
rstestfixtures for shared setup in Rust tests." and "Replace duplicated tests with#[rstest(...)]parameterised cases in Rust."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/src/preflight.rs` around lines 109 - 185, Collapse the six near-duplicate tests into a single parameterized rstest: create a shared fixture (using rstest) that sets up test_localizer() and a mutable stderr Vec<u8>, then add a #[rstest] table with columns for domain (Option<&str>), operation (Option<&str>), expected_result (Result variant or AppError discriminant), and optional expected_stderr_substring; for each row call handle_preflight(&cli(domain, operation), &split(...)) using the appropriate split flag, assert matches!(result, expected_result) and, if expected_stderr_substring is Some, convert stderr to String and assert it contains the needle, otherwise assert stderr.is_empty(); reference the existing helpers test_localizer, cli, split, and handle_preflight when implementing the fixture and parameterized cases.crates/weaver-cli/src/help.rs (1)
20-30:⚠️ Potential issue | 🟠 MajorSurface the ordering caveat on every help screen that shows these flags.
Add the note in the augmented builder itself.
help::command()now exposes the shared config flags on nested screens such asweaver daemon start --help, but that screen does not carry the top-level warning that the runtime only honours those flags before the domain or structured subcommand. Without that caveat, the help output advertises placements the runtime still ignores.Based on learnings, "CLI flags must appear before the command domain or structured subcommand to take effect."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/src/help.rs` around lines 20 - 30, The help builder returned by help::command() should append the ordering caveat to every generated help screen: after constructing the Command from Cli::command() and adding config_path_arg() and config_field_arg(...) entries, call the Command help-extender (e.g., after_help or before_help) on the Command returned by command() to append the text "CLI flags must appear before the command domain or structured subcommand to take effect." so every nested help (like when using Cli::command() for subcommands) shows this warning; update the function that returns Command (help::command) to attach that message to the Command before returning it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md`:
- Line 336: Change the British spelling "recognises" to the repository's Oxford
-ize form "recognizes" in the documentation instances that mention
split_config_arguments(...); update both occurrences (the one describing
`split_config_arguments(...)` recognising `--locale` and the other occurrence
noted) so the docs use "recognizes" consistently.
- Around line 264-268: Remove the duplicated fragment in the retrospective
bullet so it reads as one coherent sentence: delete the repeated
"crates/weaver-cli/src/help.rs" clause and ensure the sentence mentions only
once that `cargo orthohelp` will replace the `clap_mangen` infrastructure in CI,
keeping the runtime parser strict and referencing
`crates/weaver-cli/src/help.rs` and `crates/weaver-cli/build.rs` only once each
as needed.
---
Duplicate comments:
In `@crates/weaver-cli/src/help.rs`:
- Around line 20-30: The help builder returned by help::command() should append
the ordering caveat to every generated help screen: after constructing the
Command from Cli::command() and adding config_path_arg() and
config_field_arg(...) entries, call the Command help-extender (e.g., after_help
or before_help) on the Command returned by command() to append the text "CLI
flags must appear before the command domain or structured subcommand to take
effect." so every nested help (like when using Cli::command() for subcommands)
shows this warning; update the function that returns Command (help::command) to
attach that message to the Command before returning it.
In `@crates/weaver-cli/src/preflight.rs`:
- Around line 109-185: Collapse the six near-duplicate tests into a single
parameterized rstest: create a shared fixture (using rstest) that sets up
test_localizer() and a mutable stderr Vec<u8>, then add a #[rstest] table with
columns for domain (Option<&str>), operation (Option<&str>), expected_result
(Result variant or AppError discriminant), and optional
expected_stderr_substring; for each row call handle_preflight(&cli(domain,
operation), &split(...)) using the appropriate split flag, assert
matches!(result, expected_result) and, if expected_stderr_substring is Some,
convert stderr to String and assert it contains the needle, otherwise assert
stderr.is_empty(); reference the existing helpers test_localizer, cli, split,
and handle_preflight when implementing the fixture and parameterized cases.
In `@docs/roadmap.md`:
- Around line 285-299: The roadmap entry 3.2.1 still lists the top-level
checkbox as open despite its subitems being completed; edit the "3.2.1. Surface
configuration flags in clap help output." headline checkbox to checked (mark the
parent checkbox complete) so it matches the already-checked subtask lines for
`--config-path`, `--daemon-socket`, `--log-filter`, `--log-format`,
`--capability-overrides`, and `--locale` and leave the acceptance criteria text
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c2aeff5-0d4f-4544-9bce-f44160f28254
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcrates/weaver-cli/src/tests/unit/snapshots/weaver_cli__tests__unit__help_output__daemon_start_augmented_help.snapis excluded by!**/*.snapcrates/weaver-cli/src/tests/unit/snapshots/weaver_cli__tests__unit__help_output__top_level_augmented_help.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
crates/weaver-cli/Cargo.tomlcrates/weaver-cli/src/help.rscrates/weaver-cli/src/preflight.rscrates/weaver-cli/src/tests/unit/help_output.rscrates/weaver-config/Cargo.tomlcrates/weaver-config/src/lib.rscrates/weaver-config/src/locale.rsdocs/developers-guide.mddocs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.mddocs/roadmap.md
lodyai
Bot
force-pushed
the
surface-config-flags-in-help-taop69
branch
from
a6b72ad to
d0909bf
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
crates/weaver-cli/tests/main_entry.rs (1)
146-157:⚠️ Potential issue | 🟡 MinorAssert daemon startup does not run on
daemon start --help.Add negative assertions for the startup message on both streams. Line 146 currently verifies flag visibility only, so a side-effect regression can still pass this test.
Patch
#[test] fn daemon_start_help_lists_all_config_flags() { let mut command = cargo_bin_cmd!("weaver"); command.args(["daemon", "start", "--help"]); command .assert() .success() .stdout(contains("--config-path <PATH>")) .stdout(contains("--daemon-socket <ENDPOINT>")) .stdout(contains("--log-filter <FILTER>")) .stdout(contains("--log-format <FORMAT>")) .stdout(contains("--capability-overrides <DIRECTIVE>")) - .stdout(contains("--locale <LOCALE>")); + .stdout(contains("--locale <LOCALE>")) + .stdout(contains("Waiting for daemon start...").not()) + .stderr(contains("Waiting for daemon start...").not()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/tests/main_entry.rs` around lines 146 - 157, The test daemon_start_help_lists_all_config_flags currently only asserts help flags and misses verifying no daemon startup side effects; update the assertion chain on the command created by cargo_bin_cmd!("weaver") in function daemon_start_help_lists_all_config_flags to also assert that neither stdout nor stderr contain any startup messages (e.g., "starting", "started", "launch", or "daemon socket opened") by adding negative assertions such as .stdout(contains("Starting").not()) and .stderr(contains("Starting").not()) (or the test framework's equivalent) to prevent help from triggering daemon startup.docs/weaver-design.md (1)
697-699:⚠️ Potential issue | 🟡 MinorReplace Oxford spelling at this locale note.
Replace
localisationwithlocalizationto match repository spelling policy.Triage:
[type:spelling][type:docstyle]As per coding guidelines, "Use British English spelling based on the Oxford English Dictionary (en-GB-oxendict), including: -ize suffixes ..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/weaver-design.md` around lines 697 - 699, The phrase "localisation" in the roadmap note (referenced by roadmap item "3.2.1" and the surrounding sentence mentioning the shared augmented clap `Command`) should be changed to the repository's preferred spelling "localization"; update that word in the sentence and scan the same paragraph for any other occurrences of "localisation" to make them consistent with the repository spelling policy.crates/weaver-config/tests/configuration_precedence.rs (1)
136-139:⚠️ Potential issue | 🟠 MajorSerialise environment mutation across tests.
Stop extending unguarded global env mutation through
Harness::set_env; gate env writes behind a shared mutex-backed guard intest_utils/test_helpersand restore with RAII to prevent cross-test races.Run this read-only check to confirm the unguarded mutation path and missing shared guard:
#!/bin/bash set -euo pipefail echo "Inspect env mutation sites and nearby guard usage in configuration precedence tests:" rg -n -C3 'fn set_env|std::env::set_var|std::env::remove_var|Mutex|OnceLock|LazyLock' \ crates/weaver-config/tests/configuration_precedence.rs echo echo "Locate shared test utility crates that should host env guards:" fd -i 'test_utils|test_helpers' crates echo echo "Scan test code for env mutation patterns and guard constructs:" rg -n -C2 'set_var|remove_var|Mutex|OnceLock|LazyLock' crates --type rust -g '*test*'Expected result: env mutation sites appear in this harness without a shared guard held across mutation and restoration.
As per coding guidelines, "Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 ... env mutations in tests MUST be wrapped in shared guards and mutexes placed in a sharedtest_utilsortest_helperscrate. Direct environment mutation is FORBIDDEN in tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-config/tests/configuration_precedence.rs` around lines 136 - 139, The test mutates global environment via Harness::set_env in the given_environment_locale_override BDD step; change Harness::set_env to use a shared mutex-backed guard from the test utilities (e.g., a guard type in test_utils/test_helpers like EnvGuard) so every env write is serialized and restored with RAII, update given_environment_locale_override to acquire that guard before calling the set method (or call a new guarded API such as Harness::set_env_guarded) and ensure the guard restores the previous value on drop to avoid cross-test races.docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md (3)
336-336:⚠️ Potential issue | 🟡 MinorUse Oxford -ize spelling for "recognizes".
Replace
recogniseswithrecognizesto match repository spelling standards.Triage:
[type:spelling]Based on learnings, "Adhere to the repository's en-GB Oxford spelling convention in documentation files. Use -ize spellings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md` at line 336, Replace the British spelling "recognises" with the Oxford -ize form "recognizes" in the sentence mentioning split_config_arguments(...); update the phrase "`split_config_arguments(...)` recognises `--locale` in both separate-value" so it reads "`split_config_arguments(...)` recognizes `--locale` in both separate-value" to match the repository's -ize spelling convention.
264-268:⚠️ Potential issue | 🟡 MinorRemove the duplicated fragment from the retrospective bullet.
Delete the repeated
crates/weaver-cli/src/help.rssentence fragment so the bullet reads as one coherent statement.Triage:
[type:grammar][type:docstyle]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md` around lines 264 - 268, The retrospective bullet duplicates a fragment; remove the repeated "`crates/weaver-cli/src/help.rs`" clause so the sentence reads smoothly: keep the note that item `3.2.6` tracks adoption of `cargo orthohelp` in CI to replace `clap_mangen` once help metadata/locales are stable, and mention `crates/weaver-cli/build.rs` and the runtime help path without repeating `crates/weaver-cli/src/help.rs`; update the sentence containing `3.2.6`, `cargo orthohelp`, `clap_mangen`, `crates/weaver-cli/src/help.rs`, and `crates/weaver-cli/build.rs` to remove the duplicated fragment.
374-374:⚠️ Potential issue | 🟡 MinorUse Oxford -ize spelling for "recognizes".
Replace
recogniseswithrecognizesto match repository spelling standards.Triage:
[type:spelling]Based on learnings, "Adhere to the repository's en-GB Oxford spelling convention in documentation files. Use -ize spellings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md` at line 374, Change the British spelling "recognises" to the repository's Oxford -ize form "recognizes" in the sentence "Update any loader-facing constants or metadata so the runtime recognises" within the document; locate that exact phrase in docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md and update the word to "recognizes" so it matches the repo's en-GB Oxford -ize convention.crates/weaver-cli/src/help.rs (1)
17-31:⚠️ Potential issue | 🟡 MinorAdd an ordering caveat to the augmented help output.
These flags render as global clap options, but runtime parsing only honours config flags that appear before the domain or structured subcommand. State that rule in
after_helporlong_about; otherwiseweaver daemon start --log-filter debugappears supported when it is ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/src/help.rs` around lines 17 - 31, The augmented Command in function command() currently adds config_path_arg() and config_field_arg(...) as global options but doesn't warn users that runtime parsing only honors config flags placed before the domain/structured subcommand; update the Command's help text (use command.after_help() or command.long_about()) to append a clear one-line caveat stating that config flags must appear before the domain/subcommand to be effective (mention examples like "weaver daemon start --log-filter debug" being ignored if placed after), so readers of the generated help from Config::get_doc_metadata() understand the ordering constraint.crates/weaver-cli/src/lib.rs (1)
251-268: 🛠️ Refactor suggestion | 🟠 MajorExtract
write_help_for_argsintohelp.rsto satisfy the 400-line limit.The file stands at 406 lines, breaching the repository's 400-line gate. Move
write_help_for_argsintocrates/weaver-cli/src/help.rsalongside the existingcommand()builder, then re-export or call it fromlib.rs.As per coding guidelines, "Files must not exceed 400 lines in length".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/src/lib.rs` around lines 251 - 268, The function write_help_for_args should be moved out of lib.rs into a new or existing help module to keep lib.rs under 400 lines: create or update crates/weaver-cli/src/help.rs, cut the write_help_for_args definition into that file, ensure it imports std::ffi::OsString, std::io::Write and clap::error types (or refer to help::command()), and then in lib.rs either call help::write_help_for_args or add pub use help::write_help_for_args; also update any use statements in lib.rs (or module declaration) so the function is reachable and the code compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaver-config/src/lib.rs`:
- Line 11: Update the crate-level Rustdoc bullet that currently reads "Locale
identifier for internationalisation surfaces." to use Oxford -ize spelling by
replacing "internationalisation" with "internationalization"; locate the
module-level Rustdoc comment (the top-of-file //! doc comment in lib.rs) and
change that single word so the sentence reads "Locale identifier for
internationalization surfaces."
In `@docs/developers-guide.md`:
- Around line 400-421: The sentence in the documentation paragraph describing
handle_preflight (the paragraph starting with "handle_preflight runs after argv
splitting and before configuration loading") is missing a comma after the
introductory phrase; change "At that point the CLI has enough" to "At that
point, the CLI has enough" so the clause reads correctly and improves clarity
when referencing handle_preflight and its relation to AppError::BareInvocation
and AppError::PreflightGuidance.
---
Duplicate comments:
In `@crates/weaver-cli/src/help.rs`:
- Around line 17-31: The augmented Command in function command() currently adds
config_path_arg() and config_field_arg(...) as global options but doesn't warn
users that runtime parsing only honors config flags placed before the
domain/structured subcommand; update the Command's help text (use
command.after_help() or command.long_about()) to append a clear one-line caveat
stating that config flags must appear before the domain/subcommand to be
effective (mention examples like "weaver daemon start --log-filter debug" being
ignored if placed after), so readers of the generated help from
Config::get_doc_metadata() understand the ordering constraint.
In `@crates/weaver-cli/src/lib.rs`:
- Around line 251-268: The function write_help_for_args should be moved out of
lib.rs into a new or existing help module to keep lib.rs under 400 lines: create
or update crates/weaver-cli/src/help.rs, cut the write_help_for_args definition
into that file, ensure it imports std::ffi::OsString, std::io::Write and
clap::error types (or refer to help::command()), and then in lib.rs either call
help::write_help_for_args or add pub use help::write_help_for_args; also update
any use statements in lib.rs (or module declaration) so the function is
reachable and the code compiles.
In `@crates/weaver-cli/tests/main_entry.rs`:
- Around line 146-157: The test daemon_start_help_lists_all_config_flags
currently only asserts help flags and misses verifying no daemon startup side
effects; update the assertion chain on the command created by
cargo_bin_cmd!("weaver") in function daemon_start_help_lists_all_config_flags to
also assert that neither stdout nor stderr contain any startup messages (e.g.,
"starting", "started", "launch", or "daemon socket opened") by adding negative
assertions such as .stdout(contains("Starting").not()) and
.stderr(contains("Starting").not()) (or the test framework's equivalent) to
prevent help from triggering daemon startup.
In `@crates/weaver-config/tests/configuration_precedence.rs`:
- Around line 136-139: The test mutates global environment via Harness::set_env
in the given_environment_locale_override BDD step; change Harness::set_env to
use a shared mutex-backed guard from the test utilities (e.g., a guard type in
test_utils/test_helpers like EnvGuard) so every env write is serialized and
restored with RAII, update given_environment_locale_override to acquire that
guard before calling the set method (or call a new guarded API such as
Harness::set_env_guarded) and ensure the guard restores the previous value on
drop to avoid cross-test races.
In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md`:
- Line 336: Replace the British spelling "recognises" with the Oxford -ize form
"recognizes" in the sentence mentioning split_config_arguments(...); update the
phrase "`split_config_arguments(...)` recognises `--locale` in both
separate-value" so it reads "`split_config_arguments(...)` recognizes `--locale`
in both separate-value" to match the repository's -ize spelling convention.
- Around line 264-268: The retrospective bullet duplicates a fragment; remove
the repeated "`crates/weaver-cli/src/help.rs`" clause so the sentence reads
smoothly: keep the note that item `3.2.6` tracks adoption of `cargo orthohelp`
in CI to replace `clap_mangen` once help metadata/locales are stable, and
mention `crates/weaver-cli/build.rs` and the runtime help path without repeating
`crates/weaver-cli/src/help.rs`; update the sentence containing `3.2.6`, `cargo
orthohelp`, `clap_mangen`, `crates/weaver-cli/src/help.rs`, and
`crates/weaver-cli/build.rs` to remove the duplicated fragment.
- Line 374: Change the British spelling "recognises" to the repository's Oxford
-ize form "recognizes" in the sentence "Update any loader-facing constants or
metadata so the runtime recognises" within the document; locate that exact
phrase in
docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md and
update the word to "recognizes" so it matches the repo's en-GB Oxford -ize
convention.
In `@docs/weaver-design.md`:
- Around line 697-699: The phrase "localisation" in the roadmap note (referenced
by roadmap item "3.2.1" and the surrounding sentence mentioning the shared
augmented clap `Command`) should be changed to the repository's preferred
spelling "localization"; update that word in the sentence and scan the same
paragraph for any other occurrences of "localisation" to make them consistent
with the repository spelling policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 910349e1-9636-4913-8c85-64a7e25f6a74
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcrates/weaver-cli/src/tests/unit/snapshots/weaver_cli__tests__unit__help_output__daemon_start_augmented_help.snapis excluded by!**/*.snapcrates/weaver-cli/src/tests/unit/snapshots/weaver_cli__tests__unit__help_output__top_level_augmented_help.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
Makefilecrates/weaver-cli/Cargo.tomlcrates/weaver-cli/build.rscrates/weaver-cli/src/errors.rscrates/weaver-cli/src/help.rscrates/weaver-cli/src/lib.rscrates/weaver-cli/src/preflight.rscrates/weaver-cli/src/tests/unit.rscrates/weaver-cli/src/tests/unit/help_output.rscrates/weaver-cli/tests/features/weaver_cli.featurecrates/weaver-cli/tests/main_entry.rscrates/weaver-config/Cargo.tomlcrates/weaver-config/src/lib.rscrates/weaver-config/src/locale.rscrates/weaver-config/tests/configuration_precedence.rscrates/weaver-config/tests/features/configuration_precedence.featuredocs/developers-guide.mddocs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.mddocs/roadmap.mddocs/users-guide.mddocs/weaver-design.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the current state of the relevant files before giving a verdict. 🏁 Script executed: Length of output: 10499
Annulled requirements
Status of remaining comments
All live comments are resolved. All remaining comments from the previous review are now closed. |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationcrates/weaver-cli/src/preflight.rs: What lead to degradation?The module contains 2 functions with similar structure: tests.known_domain_without_operation_returns_preflight_guidance,tests.unknown_domain_returns_preflight_guidance Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add the missing blank line before the next top-level heading so the developer guide satisfies the Markdown lint rules after the rebase.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Make bare invocations consistently emit preflight guidance even when leading configuration flags are present, and update the affected tests. Move daemon execution helpers into `runner_glue` so `lib.rs` stays below the repository file-size limit. Also align `insta` with the workspace dependency, rerun manpage generation when `src/help.rs` changes, and refresh the roadmap and execplan notes for the shipped locale/help architecture.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/weaver-cli/Cargo.toml (1)
45-45:⚠️ Potential issue | 🟡 MinorSwitch
instaon Line 45 to the workspace dependency.Replace the pinned
insta = "1.39"with the workspace-managed dependency to remove version skew against the root workspace (1.41).Patch
-insta = "1.39" +insta = { workspace = true }Run this read-only check to verify the mismatch before applying the patch:
#!/usr/bin/env bash set -euo pipefail echo "Root workspace insta entry:" rg -n '^\s*insta\s*=' Cargo.toml -n -C1 echo echo "weaver-cli insta entry:" rg -n '^\s*insta\s*=' crates/weaver-cli/Cargo.toml -n -C1As per coding guidelines, "Use explicit version ranges in Cargo.toml and keep dependencies up-to-date."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/Cargo.toml` at line 45, Replace the pinned insta = "1.39" entry in the weaver-cli crate's Cargo.toml with the workspace-managed dependency (insta = { workspace = true }) to align with the root workspace insta version (1.41); update the dependency line in crates/weaver-cli Cargo.toml and then run the provided read-only grep check to confirm the workspace entry is now used and the version skew is resolved.crates/weaver-cli/build.rs (1)
18-20:⚠️ Potential issue | 🟠 MajorAdd a rerun trigger for
src/help.rs.Track the help module explicitly. Once Line 19 opts into explicit
rerun-if-changedhandling, Cargo stops whole-package tracking; because Line 30 renders the man page fromhelp::command(), edits insrc/help.rscan leave the generated manual page stale.Patch
// Regenerate the manual page when the CLI or metadata changes. println!("cargo:rerun-if-changed=src/cli.rs"); + println!("cargo:rerun-if-changed=src/help.rs"); println!("cargo:rerun-if-env-changed=CARGO_PKG_VERSION");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/build.rs` around lines 18 - 20, Add an explicit Cargo rerun trigger for the help module so changes to src/help.rs regenerate the man page; in crates/weaver-cli build.rs next to the existing println!("cargo:rerun-if-changed=src/cli.rs") and before println!("cargo:rerun-if-env-changed=CARGO_PKG_VERSION"), add a similar println call for "cargo:rerun-if-changed=src/help.rs" to ensure edits to help::command() cause rebuilds.crates/weaver-cli/src/preflight.rs (1)
34-46:⚠️ Potential issue | 🟠 MajorRemove the config-flag exemption from bare invocation.
Drop the
&& !split.has_config_flags()guard on Line 40. As written,weaver --config-path broken.tomlskips preflight, loads configuration, and can fail on config I/O or parsing instead of emitting the bare-invocation guidance that this boundary is meant to own.Patch
pub(crate) fn handle_preflight<ErrWriter: Write>( cli: &Cli, - split: &ConfigArgumentSplit, + _split: &ConfigArgumentSplit, stderr: &mut ErrWriter, localizer: &dyn Localizer, ) -> Result<(), AppError> { - if cli.is_bare_invocation() && !split.has_config_flags() { + if cli.is_bare_invocation() { tracing::debug!("emitting bare invocation guidance"); actionable_guidance::write_bare_invocation_guidance(stderr, localizer) .map_err(AppError::EmitBareHelp)?; PREFLIGHT_GUIDANCE_EMISSIONS.fetch_add(1, Ordering::Relaxed); return Err(AppError::BareInvocation);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/src/preflight.rs` around lines 34 - 46, Remove the config-flag exemption in handle_preflight so bare invocations always emit guidance: update the function signature to accept but ignore the ConfigArgumentSplit (rename split to _split) and change the conditional from `if cli.is_bare_invocation() && !split.has_config_flags()` to `if cli.is_bare_invocation() { ... }`, leaving the body that calls actionable_guidance::write_bare_invocation_guidance, increments PREFLIGHT_GUIDANCE_EMISSIONS, and returns AppError::BareInvocation unchanged.crates/weaver-cli/src/lib.rs (1)
283-351: 🛠️ Refactor suggestion | 🟠 MajorExtract these daemon-dispatch helpers out of
lib.rs.Finish the decomposition here. The file still runs past the 400-line cap, and
execute_daemon_commandplusbuild_requestare now the remaining bulk that belongs with the command/dispatch plumbing rather than the crate root. As per coding guidelines, "Files must not exceed 400 lines in length".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/src/lib.rs` around lines 283 - 351, The file exceeds the 400-line limit because daemon-dispatch helpers remain in lib.rs; move execute_daemon_command, build_request, connect_or_start_daemon (and any small helpers they need, e.g., is_daemon_not_running or start_and_retry_daemon if referenced) into a new module (e.g., crate::daemon::dispatch) and re-export or adjust visibility so callers still find execute_daemon_command and build_request; update imports in the new file to bring in CommandInvocation, LifecycleContext, IoStreams, ResolvedOutputFormat, OutputContext, CommandRequest, AppError, Connection, ExitCode, read_daemon_messages, write_error_and_fail, connect, etc., add a mod declaration in lib.rs and a pub use if needed, and run cargo build to fix any missing visibility or use-path errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaver-config/src/locale.rs`:
- Line 4: Update the module doc comment that currently reads "the wider CLI
localisation bootstrap remains future work..." to use en-GB-oxendict -ize
spelling by replacing "localisation" with "localization" in the doc string in
locale.rs (the top-level //! comment), ensuring the rest of the sentence remains
unchanged.
---
Duplicate comments:
In `@crates/weaver-cli/build.rs`:
- Around line 18-20: Add an explicit Cargo rerun trigger for the help module so
changes to src/help.rs regenerate the man page; in crates/weaver-cli build.rs
next to the existing println!("cargo:rerun-if-changed=src/cli.rs") and before
println!("cargo:rerun-if-env-changed=CARGO_PKG_VERSION"), add a similar println
call for "cargo:rerun-if-changed=src/help.rs" to ensure edits to help::command()
cause rebuilds.
In `@crates/weaver-cli/Cargo.toml`:
- Line 45: Replace the pinned insta = "1.39" entry in the weaver-cli crate's
Cargo.toml with the workspace-managed dependency (insta = { workspace = true })
to align with the root workspace insta version (1.41); update the dependency
line in crates/weaver-cli Cargo.toml and then run the provided read-only grep
check to confirm the workspace entry is now used and the version skew is
resolved.
In `@crates/weaver-cli/src/lib.rs`:
- Around line 283-351: The file exceeds the 400-line limit because
daemon-dispatch helpers remain in lib.rs; move execute_daemon_command,
build_request, connect_or_start_daemon (and any small helpers they need, e.g.,
is_daemon_not_running or start_and_retry_daemon if referenced) into a new module
(e.g., crate::daemon::dispatch) and re-export or adjust visibility so callers
still find execute_daemon_command and build_request; update imports in the new
file to bring in CommandInvocation, LifecycleContext, IoStreams,
ResolvedOutputFormat, OutputContext, CommandRequest, AppError, Connection,
ExitCode, read_daemon_messages, write_error_and_fail, connect, etc., add a mod
declaration in lib.rs and a pub use if needed, and run cargo build to fix any
missing visibility or use-path errors.
In `@crates/weaver-cli/src/preflight.rs`:
- Around line 34-46: Remove the config-flag exemption in handle_preflight so
bare invocations always emit guidance: update the function signature to accept
but ignore the ConfigArgumentSplit (rename split to _split) and change the
conditional from `if cli.is_bare_invocation() && !split.has_config_flags()` to
`if cli.is_bare_invocation() { ... }`, leaving the body that calls
actionable_guidance::write_bare_invocation_guidance, increments
PREFLIGHT_GUIDANCE_EMISSIONS, and returns AppError::BareInvocation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f3a1c15-8225-4feb-a4dc-e45b57fbc3cb
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcrates/weaver-cli/src/tests/unit/snapshots/weaver_cli__tests__unit__help_output__daemon_start_augmented_help.snapis excluded by!**/*.snapcrates/weaver-cli/src/tests/unit/snapshots/weaver_cli__tests__unit__help_output__top_level_augmented_help.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
Cargo.tomlMakefilecrates/weaver-cli/Cargo.tomlcrates/weaver-cli/build.rscrates/weaver-cli/src/errors.rscrates/weaver-cli/src/help.rscrates/weaver-cli/src/lib.rscrates/weaver-cli/src/preflight.rscrates/weaver-cli/src/tests/behaviour.rscrates/weaver-cli/src/tests/unit/help_output.rscrates/weaver-cli/tests/features/weaver_cli.featurecrates/weaver-cli/tests/main_entry.rscrates/weaver-config/Cargo.tomlcrates/weaver-config/src/lib.rscrates/weaver-config/src/locale.rscrates/weaver-config/tests/configuration_precedence.rscrates/weaver-config/tests/features/configuration_precedence.feature
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaver-cli/src/lib.rs`:
- Around line 32-40: Add the same MAINTENANCE comment above
SHARED_CONFIG_HELP_FLAGS that exists above CONFIG_CLI_FLAGS: state that these
entries must be kept in sync with the `cli_long` attributes in
`weaver-config::Config` and reference `CONFIG_CLI_FLAGS` so future editors know
to update both places; place the comment immediately above the `pub const
SHARED_CONFIG_HELP_FLAGS` declaration and mirror the wording/format used for the
existing MAINTENANCE note on `CONFIG_CLI_FLAGS`.
In `@crates/weaver-cli/tests/main_entry.rs`:
- Around line 11-12: The test currently imports SHARED_CONFIG_HELP_FLAGS which
couples the integration test to production constants; instead define a local
constant EXPECTED_SHARED_CONFIG_HELP_FLAGS with the six expected flags and use
it in place of SHARED_CONFIG_HELP_FLAGS. Remove SHARED_CONFIG_HELP_FLAGS from
the use/import line (keep DOMAIN_OPERATIONS), add const
EXPECTED_SHARED_CONFIG_HELP_FLAGS: &[&str] = &["--config-path <PATH>",
"--daemon-socket <ENDPOINT>", "--log-filter <FILTER>", "--log-format <FORMAT>",
"--capability-overrides <DIRECTIVE>", "--locale <LOCALE>"], and update all loops
and assertions that iterate over SHARED_CONFIG_HELP_FLAGS to iterate over
EXPECTED_SHARED_CONFIG_HELP_FLAGS (including the combined.contains(flag) checks
and assert.stdout(contains(*flag)) calls).
In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md`:
- Line 13: Remove or rewrite the sentence "This plan must be approved before
implementation begins." from the ExecPlan (the pre-implementation approval gate)
so the document no longer contradicts its Status: COMPLETE; either delete that
line or replace it with a retrospective note explaining approval was obtained
prior to implementation, and make the same change for the duplicated content
around the retrospective section referenced in the file (the block currently
repeating the pre-implementation gate).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54f023e6-d337-4db6-a423-00c05f4b469d
📒 Files selected for processing (10)
crates/weaver-cli/Cargo.tomlcrates/weaver-cli/build.rscrates/weaver-cli/src/config.rscrates/weaver-cli/src/lib.rscrates/weaver-cli/src/preflight.rscrates/weaver-cli/src/runner_glue.rscrates/weaver-cli/src/tests/unit/bare_invocation.rscrates/weaver-cli/tests/main_entry.rsdocs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.mddocs/roadmap.md
💤 Files with no reviewable changes (1)
- crates/weaver-cli/src/config.rs
Use Oxford -ize spelling in the locale module documentation so the Rustdoc comment matches the repository style guide.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document that the rendered config help list must stay in sync with the config metadata and runtime stripping table. Keep the binary integration test independent of production constants, and turn the completed ExecPlan approval gate into retrospective wording.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. ❌ Failed checks (3 errors, 5 warnings, 1 inconclusive)
|
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md`:
- Around line 266-268: Replace the first occurrence of the acronym "CI" in the
sentence referencing roadmap item `3.2.6` (the line mentioning `cargo orthohelp`
and `crates/weaver-cli/build.rs` and `clap_mangen`) with the expanded form
"Continuous Integration (CI)"; keep subsequent uses as "CI" and ensure the
change appears where "in CI" currently reads so the document now says "in
Continuous Integration (CI)" on first use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a29d423c-7bbd-4881-bb6c-906552975a47
📒 Files selected for processing (4)
crates/weaver-cli/src/lib.rscrates/weaver-cli/tests/main_entry.rscrates/weaver-config/src/locale.rsdocs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md
Define Continuous Integration on first use in the completed 3.2.1 ExecPlan so the retrospective note follows the documentation acronym style.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document the runner glue APIs and add tracing around daemon connection and auto-start paths. Remove the remaining atomic observability counters so the runtime uses tracing for these paths. Add focused coverage for help writer failures, request construction, locale JSON round-trips, and the configuration-flag ordering invariant. Keep shared help flag expectations local to tests so production constants cannot mask a regression.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md`:
- Around line 67-117: Add three explicit, normative headings to the ExecPlan:
"Fail-fast discovery", "YAML 1.2 semantics", and "ortho-config v0.8.0 dependency
graph", and under each state the observable behaviour required (e.g.
configuration loading must fail immediately on invalid files, YAML parsing
follows YAML 1.2 semantics with any differences noted, and the ortho-config
v0.8.0 model and dependency graph that the project relies on), phrasing each as
a requirement tied to testable outcomes; ensure the wording is prescriptive,
short, and placed as top-level sections in the same document so they are
discovered by reviewers and tooling (triage: [type:docstyle]).
- Around line 153-171: Add GitHub-flavored numeric footnote citations for each
referenced document/contract in this ExecPlan by inserting [^n] markers at each
first-appearance claim (respecting first-appearance order) and then append a
numbered footnote block mapping each [^n] to the corresponding document or
contract; do this for the cited items in the checklist and the other affected
section (the block noted "Also applies to: 420-435"). Use the same numbering
convention throughout and ensure references to the code/contract mentions
(crates/weaver-cli/src/config.rs::split_config_arguments(...),
crates/weaver-cli/src/cli.rs::Cli, crates/weaver-config/src/lib.rs::Config, and
crates/weaver-cli/build.rs::help::command()) are each footnoted where they are
asserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 22fa3921-c684-43f0-a48a-898179d33ff2
📒 Files selected for processing (1)
docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/weaver-cli/src/help.rs`:
- Around line 93-103: The code is pulling short help text via
config_field_help() instead of carrying it through the metadata path; update the
construction of ConfigFieldArgMetadata (and any other metadata uses around the
other block at 129-137) to include the short help text from the field/source of
truth and remove the switch in config_field_help(), so that
ConfigFieldArgMetadata (used by config_arg_from_metadata) contains the canonical
help string (e.g. add/rename the metadata field used for help and populate it
from the original field.help/source) and render help exclusively from that
metadata everywhere.
In `@crates/weaver-cli/src/preflight.rs`:
- Around line 29-34: Remove the now-unused '_split: &ConfigArgumentSplit'
parameter from the handle_preflight signature and all call sites (the function
is pub(crate) fn handle_preflight<ErrWriter: Write>(cli: &Cli, stderr: &mut
ErrWriter, localizer: &dyn Localizer) -> Result<(), AppError>), then collapse
any duplicated match/if branches inside handle_preflight that only differed by
the boolean has_config_flags into a single path (compute has_config_flags where
needed but do not keep separate duplicated bare-invocation cases), and update
tests to stop constructing ConfigArgumentSplit for preflight calls; keep
references to ErrWriter, Cli, Localizer, and has_config_flags to help locate the
relevant code to change.
In `@crates/weaver-config/src/locale.rs`:
- Around line 18-23: The LocaleParseError currently embeds raw input into its
Display via the #[error("invalid locale `{input}`")] format, which can leak
control characters to stderr; update the error annotation on struct
LocaleParseError to use a debug-escaped representation of the input (use
{input:?} instead of `{input}`) so invalid locale strings are escaped when
formatted, keeping the field name input unchanged.
- Around line 10-16: Add unit tests that cover BCP 47 locale extensions (e.g.
"en-US-u-ca-gregory") by parsing them into the validated Locale wrapper and
asserting round-trip equality; specifically, write tests that use
LanguageIdentifier::from_str (or langid! helper) to produce a
LanguageIdentifier, construct or parse a Locale from that string, then assert
the Locale's inner LanguageIdentifier matches and that serializing+deserializing
(serde) the Locale yields an identical value. Place tests alongside existing
locale tests and include at least one example with a "-u-*" Unicode extension to
ensure parsing and serde round-trip stability for extensions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a2fcdf9-4997-468f-b87d-9c1ef0bb9647
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/weaver-cli/Cargo.tomlcrates/weaver-cli/src/config.rscrates/weaver-cli/src/help.rscrates/weaver-cli/src/preflight.rscrates/weaver-cli/src/runner_glue.rscrates/weaver-cli/src/tests/behaviour.rscrates/weaver-cli/src/tests/unit/help_output.rscrates/weaver-config/Cargo.tomlcrates/weaver-config/src/locale.rs
Carry configuration help text through the help metadata path instead of switching on field names, and remove the unused preflight split parameter. Validate full locale tags with the ICU locale parser so Unicode extension tags round-trip cleanly while preserving the base language identifier for existing comparisons. Escape invalid locale input in errors with debug formatting. Document the ExecPlan requirements and add footnotes for the referenced contracts so the completed plan remains auditable.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/weaver-cli/src/help.rs (1)
25-45: 🧹 Nitpick | 🔵 TrivialRemove duplicated help-text mapping and source help from a single metadata origin.
Delete the
CONFIG_FIELD_HELPlookup table andDEFAULT_CONFIG_FIELD_HELPfallback path. DriveArg::helpfrom one canonical metadata source so flag descriptions in--help, manpages, and config docs cannot drift when help IDs or fields change.Also applies to: 150-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/weaver-cli/src/help.rs` around lines 25 - 45, Remove the duplicate help-text mapping by deleting the CONFIG_FIELD_HELP array and DEFAULT_CONFIG_FIELD_HELP constant and update any code that uses them to instead call into the canonical metadata source when building Arg::help; specifically replace usages that lookup keys in CONFIG_FIELD_HELP or fall back to DEFAULT_CONFIG_FIELD_HELP with a single metadata fetch (e.g., a function or struct method that returns the help string for a given help ID) so Arg::help is populated from that single origin for flags, manpages, and config docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md`:
- Around line 389-394: The doc currently uses speculative language "likely
`en-US`" for the locale default; change that sentence to state the implemented
default explicitly (e.g., "the default is `en-US`") so the field description for
`--locale` / `WEAVER_LOCALE` records the actual default used by the ExecPlan
(marked COMPLETE) rather than speculative wording.
- Around line 230-233: Replace the stale type references: change all occurrences
of unic_langid::LanguageIdentifier in the ExecPlan text to the actual shipped
wrapper type ortho_config::LanguageIdentifier (matching the developers-guide),
and update the documented default (the phrase "likely `en-US`") to the actual
shipped default value used by the codebase (use the canonical default constant
or Config/default for locale, e.g., the value returned by
ortho_config::LanguageIdentifier::default or Config::DEFAULT_LOCALE) so the
documentation exactly matches the workspace boundary and defaults.
---
Duplicate comments:
In `@crates/weaver-cli/src/help.rs`:
- Around line 25-45: Remove the duplicate help-text mapping by deleting the
CONFIG_FIELD_HELP array and DEFAULT_CONFIG_FIELD_HELP constant and update any
code that uses them to instead call into the canonical metadata source when
building Arg::help; specifically replace usages that lookup keys in
CONFIG_FIELD_HELP or fall back to DEFAULT_CONFIG_FIELD_HELP with a single
metadata fetch (e.g., a function or struct method that returns the help string
for a given help ID) so Arg::help is populated from that single origin for
flags, manpages, and config docs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 090d217a-ef0e-40e5-b194-0ea00ab2a2ca
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/weaver-cli/src/help.rscrates/weaver-cli/src/lib.rscrates/weaver-cli/src/preflight.rscrates/weaver-cli/src/tests/unit.rscrates/weaver-cli/src/tests/unit/bare_invocation.rscrates/weaver-cli/src/tests/unit/help_output.rscrates/weaver-config/Cargo.tomlcrates/weaver-config/src/locale.rsdocs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md
Comment on lines
+230
to
+233
| - `unic_langid::LanguageIdentifier` is not serde-enabled through the current | ||
| workspace dependency surface, so a direct `Config::locale` field would not | ||
| compile. A small validated `Locale` newtype in `weaver-config` keeps the | ||
| config contract honest without adding new dependency churn. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for locale-related crate references and type names
rg -n "unic_langid|icu_locale_core|LanguageIdentifier" --type toml --type md
# Also check for the mentioned lines in the specific file
echo "---"
echo "Lines 230-233:"
sed -n '230,233p' docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md
echo "---"
echo "Lines 291-293:"
sed -n '291,293p' docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md
echo "---"
echo "Lines 389-394:"
sed -n '389,394p' docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.mdRepository: leynos/weaver
Length of output: 1820
Replace stale unic_langid references with the correct type the workspace actually ships.
The ExecPlan documentation at lines 230, 255, and 292 references unic_langid::LanguageIdentifier, but the workspace manifest specifies icu_locale_core as the dependency. The developers-guide correctly uses ortho_config::LanguageIdentifier for the wrapped type. Update these three locations to match the actual shipped boundary.
Additionally, line 389 specifies the default as "likely en-US" in a document marked Status: COMPLETE. Record the actual shipped default value rather than conjecture.
Triage: [type:docstyle]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md`
around lines 230 - 233, Replace the stale type references: change all
occurrences of unic_langid::LanguageIdentifier in the ExecPlan text to the
actual shipped wrapper type ortho_config::LanguageIdentifier (matching the
developers-guide), and update the documented default (the phrase "likely
`en-US`") to the actual shipped default value used by the codebase (use the
canonical default constant or Config/default for locale, e.g., the value
returned by ortho_config::LanguageIdentifier::default or Config::DEFAULT_LOCALE)
so the documentation exactly matches the workspace boundary and defaults.
Comment on lines
+389
to
+394
| The field should: | ||
|
|
||
| - participate in the same layered precedence as the existing config fields; | ||
| - be surfaced as `--locale` and `WEAVER_LOCALE`; | ||
| - have a documented default, likely `en-US`; and | ||
| - stop short of wiring the localizer bootstrap or translated help selection in |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for Default impl for Locale and locale-related configuration
rg -n "impl Default for Locale|en[-_]US|pub locale" \
crates/weaver-config/src/locale.rs \
crates/weaver-config/src/lib.rs \
crates/weaver-config/tests \
2>/dev/null || echo "Pattern search complete"Repository: leynos/weaver
Length of output: 922
🏁 Script executed:
sed -n '35,45p' crates/weaver-config/src/locale.rsRepository: leynos/weaver
Length of output: 309
🏁 Script executed:
sed -n '25,37p' crates/weaver-config/src/locale.rsRepository: leynos/weaver
Length of output: 350
Replace speculative language with the implemented locale default.
The documentation states the default is "likely en-US", but the implementation definitively uses en-US. Update line 393 to remove the speculative framing and record the actual default value, since the ExecPlan is marked COMPLETE.
Triage: [type:docstyle]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/execplans/3-2-1-surface-configuration-flags-in-clap-help-output.md`
around lines 389 - 394, The doc currently uses speculative language "likely
`en-US`" for the locale default; change that sentence to state the implemented
default explicitly (e.g., "the default is `en-US`") so the field description for
`--locale` / `WEAVER_LOCALE` records the actual default used by the ExecPlan
(marked COMPLETE) rather than speculative wording.
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
weaver --helpandweaver daemon start --helpwithout altering runtime parsing semantics.Changes
Rationale
Plan of work (Stage-based)
weaver --helpandweaver daemon start --helplist all six flags and that help does not trigger daemon startup.localeto the shared configuration contract (initial planning scope)localeas a real config field (subject to later bootstrap-localizer work).Validation plan
weaver --helpandweaver daemon start --helpshow all six flags in the Options section.Risks & constraints
localeinto the shared contract may require coordination with roadmap 3.3.1 for bootstrap-localizer work.Documentation impact
How to review
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/d72d1d9a-4972-4a9d-92c6-c3f8954fce45
Summary by Sourcery
Surface shared runtime configuration flags in CLI help and manpage output while adding a validated locale setting to the configuration contract and documenting the updated behaviour.
New Features:
Enhancements:
Documentation:
Tests: