Upgrade ortho-config to v0.8.0; add global-args loader and tests#188
Merged
Upgrade ortho-config to v0.8.0; add global-args loader and tests#188
Conversation
Add a comprehensive execplan document outlining the steps to upgrade this repository's ortho-config dependency from v0.6.0 to v0.8.0. The document includes goals, migration notes, implementation plans, validation checklists, and risks associated with the upgrade to guide maintainers through the process. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideUpgrade the project to ortho_config v0.8.0, adjust global configuration loading to preserve file/env precedence when the CLI provides no overrides, and refresh the ortho-config documentation set (user guide, migration guide, and execplan) plus README to match the new version and features. Sequence diagram for loading global configuration without empty CLI overridessequenceDiagram
actor User
participant VkMain as vk_main
participant GlobalArgs
participant OrthoConfig as ortho_config_runtime
participant LayerComp as LayerComposition
participant MergeLayer
User->>VkMain: run vk
VkMain->>VkMain: load_global_args_without_cli_overrides()
VkMain->>GlobalArgs: compose_layers_from_iter(["vk"])
GlobalArgs-->>VkMain: LayerComposition
VkMain->>LayerComp: into_parts()
LayerComp-->>VkMain: layers, errors
VkMain->>VkMain: create defaults MergeLayer from GlobalArgs::default()
loop for each layer in layers
VkMain->>MergeLayer: check provenance()
alt provenance is Cli
VkMain->>MergeLayer: into_value()
alt value is null
VkMain-->>VkMain: skip this CLI layer
else value is non-null
VkMain-->>VkMain: push MergeLayer::cli(value)
end
else provenance is not Cli
VkMain-->>VkMain: push existing layer
end
end
VkMain->>LayerComp: LayerComposition::new(filtered_layers, errors)
LayerComp-->>VkMain: composition
VkMain->>GlobalArgs: composition.into_merge_result(GlobalArgs::merge_from_layers)
GlobalArgs-->>VkMain: OrthoResult<GlobalArgs>
VkMain-->>User: run with merged GlobalArgs (file/env preserved when CLI empty)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughSummarise crate upgrade to ortho-config v0.8.0; add a dedicated config loader that composes layers while ignoring null CLI overrides; introduce environment-safe test utilities and separate test modules; add extensive docs and migration guides; remove in-file tests and expose standalone test modules. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI / argv"
participant Loader as "config_loader::load_global_args_without_cli_overrides()"
participant LayerComp as "LayerComposition"
participant Ortho as "ortho_config::merge"
participant App as "main (vk)"
CLI->>Loader: provide argv (including --config-path, subcommand tokens)
Loader->>LayerComp: compose_layers_from_iter(defaults, files, env, CLI)
LayerComp-->>Loader: composed layers
Loader->>LayerComp: filter out null/empty CLI layer
LayerComp->>Ortho: merge_from_layers(filtered)
Ortho-->>Loader: GlobalArgs result
Loader-->>App: supply GlobalArgs
App->>Ortho: perform SubcmdConfigMerge and execute command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
- Replace local ortho-config user guide with upstream v0.8.0 guide - Apply v0.7.0 and v0.8.0 migration notes relevant to this repo - Update docs to reflect ortho-config v0.8.0 migration - Reconcile local docs to avoid contradictions with imported guide - Outline detailed implementation plan for migration steps - Highlight risks and necessary validation checks This commit prepares project documentation and migration planning for upgrading ortho-config dependency from v0.6.0 to v0.8.0, ensuring alignment with upstream changes and smoother transition. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…LI flattening overrides - Extracted `load_global_args_without_cli_overrides` to compose global config layers and filter empty CLI layers. - Prevent empty CLI flatten group from overwriting file/env defaults during global config load. - Updated main to use this new loader for merging global and CLI args. - Added test ensuring defaults load cleanly without unwanted CLI overrides. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/ortho-config-users-guide.md" line_range="361" />
<code_context>
+
+### FAQ: should `figment`, `uncased`, or `xdg` be direct dependencies?
+
+No for derive-generated code. Yes, only when application code directly imports
+those crates without going through the `ortho_config::` re-exports.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The phrase "when application code directly imports" is fine, but nearby text in this new section later uses "let you"/"you"; those should be rephrased to avoid 2nd-person pronouns.
In this added block, later sentences such as "let you merge the chosen subcommand" and "let you attribute JSON parsing failures" introduce 2nd-person pronouns, which the style guide forbids. Please rephrase those occurrences, for example:
- "…that enable merging the chosen subcommand…" instead of "let you merge the chosen subcommand".
- "…that enable attribution of JSON parsing failures…" instead of "let you attribute JSON parsing failures…".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>
### Comment 2
<location path="docs/ortho-config-v0-7-0-migration-guide.md" line_range="51" />
<code_context>
+
+1. Update every `ortho_config` and `ortho_config_macros` dependency to `0.7.0`.
+1. Keep format features (`toml`, `json5`, `yaml`) on `ortho_config` as before.
+1. If you disable default features, enable `serde_json` explicitly when you
+ want `cli_default_as_absent` or selected-subcommand merging helpers. The
+ default feature set already enables it.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This list item uses "you" twice and needs to be rephrased to avoid 2nd-person pronouns.
To comply with the pronoun rule, consider rephrasing along the lines of:
> 1. When default features are disabled, enable `serde_json` explicitly whenever `cli_default_as_absent` or selected-subcommand merging helpers are required.
This keeps the instruction but removes direct address to the reader.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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
🤖 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/adopt-ortho-config-v0-8-0.md`:
- Around line 29-31: Update the paragraph to correct the Rust version reference
for ortho_config v0.8.0: replace the incorrect "Rust 1.88" phrasing with the
actual minimum Rust version required by ortho_config v0.8.0 (or explicitly mark
it as a forward-looking/unstable requirement pending the Rust 1.88 release), and
mention the repo already using a recent nightly via `rust-toolchain.toml` and
that the crate manifest currently lacks a `rust-version` field so readers know
where the requirement is enforced.
- Around line 101-103: Update the note about adding `rust-version = "1.88"` in
the Cargo.toml guidance: explicitly state that Rust 1.88 is not yet released and
either (A) change the recommendation to the current stable toolchain version
that meets requirements (e.g., the current stable release) or (B) keep
`rust-version` in the plan but document that the manifest entry `rust-version =
"1.88"` will be added only once Rust 1.88 is officially released; ensure this
clarification appears next to the existing Cargo.toml / `rust-version = "1.88"`
line in the adopt-ortho-config-v0-8-0.md plan.
In `@docs/ortho-config-users-guide.md`:
- Line 1015: Update the wording in the sentence containing "When a user passes
`--help` or `--version`, `clap` surfaces specialised" to use en-GB-oxendict
spelling: replace "specialised" with "specialized". Locate this exact phrase in
docs/ortho-config-users-guide.md and make the single-word substitution so the
sentence reads "...`clap` surfaces specialized".
In `@docs/ortho-config-v0-7-0-migration-guide.md`:
- Line 86: Change the heading text "Example: normalise a derived field" to use
American/Oxford spelling "normalize" (i.e., "Example: normalize a derived
field") and search the same document for any other occurrences of "normalise" to
update them to "normalize" for consistency.
- Line 12: Update the document to use the en-GB-oxendict spelling by replacing
instances of "Localise" with "Localize" in headings and body text (e.g., change
the heading "Localise CLI copy and errors" to "Localize CLI copy and errors")
and scan for any other occurrences of "localise" (case-insensitive) to correct
them consistently throughout the file.
- Around line 188-223: Footnote lines like "\[^deps-0-7\]:",
"\[^compose-layers\]:", "\[^post-merge\]:", etc. contain escaped brackets which
break GFM footnote rendering; remove the backslashes before the opening square
bracket for all footnote entries in this section (e.g. change "\[^deps-0-7\]:
v0.7.0 ..." to "[^deps-0-7]: v0.7.0 ..." and apply the same fix for each
footnote token shown in the diff such as ^compose-layers, ^post-merge,
^localizer, ^localizeclap, ^cli-defaults, ^selectedsubcommand, ^display-request,
^json-merge, and ^rstest-bdd).
In `@src/main.rs`:
- Around line 183-185: The call to
serde_json::to_value(GlobalArgs::default()).expect(...) must not use expect;
propagate the serialization error instead: change the surrounding function to
return a Result (or convert the error into the function's existing error type)
and replace the .expect(...) with the ? operator (or a map_err(...) to convert
serde_json::Error), then push the resulting Value into filtered_layers via
MergeLayer::defaults(Cow::Owned(...)); update the function signature and any
callers as needed to handle the propagated error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aeead8b7-375e-44c1-bda7-95885bded441
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlREADME.mddocs/execplans/adopt-ortho-config-v0-8-0.mddocs/ortho-config-users-guide.mddocs/ortho-config-v0-7-0-migration-guide.mdsrc/main.rs
![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: cd84b38b3d
ℹ️ 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".
Refined phrasing in the ortho-config users guide to enhance clarity and readability in sections describing CLI flag renaming, file discovery, and config path override behavior. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
- Added setup of temporary config sandbox directories in tests. - Preserved and restored original environment variables to prevent side effects. - Enhanced test isolation for loading global args without CLI overrides. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
…ocalization
- Refine v0.6.0 and v0.7.0 migration guides with corrected markdown and improved links
- Clarify Rust MSRV bump to 1.89 in Cargo.toml and docs
- Improve vk design and end-to-end testing guides formatting and content
- Fix typos and unify US/UK English ('localise' to 'localize') in migration guide
- Enhance github-token.md steps for clarity and consistency
- Add references and format work cited section in tests documentation
Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
docs/ortho-config-v0-7-0-migration-guide.md (1)
188-226:⚠️ Potential issue | 🟡 MinorUse real footnote definitions here.
Remove the escapes before each
[^…]:token. The current form breaks every footnote reference in the guide. As per coding guidelines,**/*.md: Use GitHub-flavoured Markdown style for footnotes and endnotes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ortho-config-v0-7-0-migration-guide.md` around lines 188 - 226, The footnote markers are escaped (e.g. \[^deps-0-7\], \[^compose-layers\], \[^post-merge\], etc.), which breaks GitHub-flavored Markdown footnotes; remove the leading backslashes before each [^…]: definition and any in-text references so the tokens are plain [^deps-0-7], [^compose-layers], [^post-merge], [^localizer], [^localizeclap], [^cli-defaults], [^selectedsubcommand], [^display-request], [^json-merge], [^rstest-bdd] to restore proper footnote rendering across the document.
🤖 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/adopt-ortho-config-v0-8-0.md`:
- Around line 74-76: Replace the British/Oxford misspelling in the sentence
"Post-merge hooks and localisation: the v0.7.0 guide makes these available…" by
changing "localisation" to "localization" (also update the same word at the
other occurrence referenced around lines 120-121); ensure both occurrences use
"localization" to conform to the repo's en-GB-oxendict (-ize) convention and
keep surrounding punctuation/capitalization unchanged.
In `@docs/github-token.md`:
- Around line 8-12: The ordered list in docs/github-token.md uses repeated "1."
which triggers markdownlint MD029; update the four list items so they are
numbered 2., 3., 4., and 5. (Keep the same item text: "Give the token a
note...", "Under Select scopes...", "Click Generate token...", "Export the token
as VK_GITHUB_TOKEN...") This will satisfy markdownlint and the repository's
Markdown validation.
In `@docs/ortho-config-users-guide.md`:
- Around line 586-618: The ordered lists in the documentation use repeated "1."
entries which triggers MD029; update each ordered list block so numbering is
sequential (1., 2., 3., ...) instead of repeating "1." — specifically fix the
list starting with "Attempts to load a configuration file." (and the subsequent
lists that begin "Adds an environment provider..." and "Adds a provider
containing the CLI values...") as well as the other occurrences noted (the
blocks referred to as 669-676, 961-964, 1190-1199) so every list item uses
correct incremental numbers to satisfy markdownlint.
- Around line 276-283: The derive macro path in the example is incorrect: change
uses of ortho_config_macros::SelectedSubcommandMerge to the re-exported path
ortho_config::SelectedSubcommandMerge so the derive matches the public API and
the listed dependency ortho_config; update all occurrences of the derive (e.g.,
the derive on the type that implements SelectedSubcommandMerge and any examples
between 856–889) to use ortho_config::SelectedSubcommandMerge and ensure
Cargo.toml only requires ortho_config (not ortho_config_macros).
- Around line 441-447: Update the documentation for the merge_strategy attribute
to list all supported strategies in v0.8.0 ("append", "replace", "keyed"),
explain their constraints and defaults: "append" concatenates Vec<T> values and
is the default for vector fields, "keyed" merges map-like types and is the
default for maps, and "replace" (available for both vectors and maps) makes the
later source override previous values; ensure the entry for merge_strategy
mentions these three options and that append is not the only strategy.
In `@docs/ortho-config-v0-6-0-migration-guide.md`:
- Around line 223-257: The footnote definitions are currently escaped (e.g.,
"\[^forwarded-features\]:", "\[^hello-world-cargo\]:", "\[^reexports\]:", etc.),
causing them to render as plain text; remove the leading backslashes before each
footnote token so they appear as real GitHub-flavored Markdown footnote
definitions (e.g., change "\[^forwarded-features\]:" to
"[^forwarded-features]:") and do the same for every other escaped footnote token
in that block to restore proper footnote rendering.
In `@docs/vk-design.md`:
- Around line 70-71: Remove the escaped backslashes from the footnote definition
so the GitHub-flavored footnote resolves: replace the literal
"\[^github-link\]:" with an unescaped "[^github-link]:" in the docs/vk-design.md
footnote block (the footnote label [^github-link] must exactly match the
reference used earlier), ensuring the URL and description remain on the
following line.
In `@docs/vk-end-to-end-testing-guide.md`:
- Around line 247-251: Update the ordered lists in
docs/vk-end-to-end-testing-guide.md to use sequential numbering instead of
repeated "1." for each item: renumber the block containing "Cargo's test runner
can find and compile the `tests/e2e.rs` file.", "`assert_cmd` is able to locate
the `vk` binary produced by the build process.", and "The basic assertion
mechanism is working as expected." to 1., 2., 3. respectively (and apply the
same fix to the other affected section referenced around lines 635-699). Ensure
the list prefixes are sequential so Markdownlint MD029 no longer triggers and
re-run make markdownlint to verify.
- Around line 1018-1101: Remove the erroneous backslashes before each footnote
definition (e.g., change "\[^1\]:" to "[^1]:") and rewrite any bare fenced code
URL blocks into the footnote line itself (e.g., replace the fenced
"```<https://docs.rs/termimad>```" and
"```<https://github.com/assert-rs/assert_cmd>```" with inline URLs in the
corresponding footnotes such as "[^4]: <https://docs.rs/termimad>" and "[^10]:
<https://github.com/assert-rs/assert_cmd>"). Ensure all footnotes [^1] through
[^25] use GitHub-flavored Markdown footnote syntax with the URL folded into the
footnote text and no surrounding triple-backtick fences.
In `@src/main.rs`:
- Around line 179-199: The helper load_global_args_without_cli_overrides
currently composes layers from a synthetic single-arg iterator
(OsString::from("vk")), which drops CLI-only discovery inputs (e.g. generated
config-path overrides); change it to pass the real argv (or ArgMatches) into
GlobalArgs::compose_layers_from_iter so the actual CLI-provided values are
included, then when iterating the composed layers only remove or skip the
empty/placeholder CLI layer (check MergeProvenance::Cli and use
layer.into_value().is_null() to detect emptiness) rather than constructing a
fake "vk" layer; this preserves CLI-only discovery inputs so
LayerComposition::new(...).into_merge_result(GlobalArgs::merge_from_layers) can
honor explicit config-path overrides.
---
Duplicate comments:
In `@docs/ortho-config-v0-7-0-migration-guide.md`:
- Around line 188-226: The footnote markers are escaped (e.g. \[^deps-0-7\],
\[^compose-layers\], \[^post-merge\], etc.), which breaks GitHub-flavored
Markdown footnotes; remove the leading backslashes before each [^…]: definition
and any in-text references so the tokens are plain [^deps-0-7],
[^compose-layers], [^post-merge], [^localizer], [^localizeclap],
[^cli-defaults], [^selectedsubcommand], [^display-request], [^json-merge],
[^rstest-bdd] to restore proper footnote rendering across the document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfb40dec-3804-469e-9fb5-ba21adc97db1
📒 Files selected for processing (9)
Cargo.tomldocs/execplans/adopt-ortho-config-v0-8-0.mddocs/github-token.mddocs/ortho-config-users-guide.mddocs/ortho-config-v0-6-0-migration-guide.mddocs/ortho-config-v0-7-0-migration-guide.mddocs/vk-design.mddocs/vk-end-to-end-testing-guide.mdsrc/main.rs
…th overrides This refactor updates the global configuration loading process to accurately parse and honor the --config-path discovery override from the process arguments. It excludes subcommand tokens that GlobalArgs cannot parse, preventing empty CLI flatten groups from overwriting file or environment values. Additional helper functions and tests enhance configuration precedence handling and maintain backward compatibility. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/ortho-config-v0-6-0-migration-guide.md (1)
51-60:⚠️ Potential issue | 🟡 MinorRenumber this ordered list.
Use sequential prefixes here. Repeating
1.trips MD029 in this repo, so
make markdownlintwill fail.As per coding guidelines, "
**/*.md: Validate Markdown with make markdownlint".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ortho-config-v0-6-0-migration-guide.md` around lines 51 - 60, The ordered list in the migration guide uses repeated "1." markers which triggers MD029; update the three list entries so their prefixes are sequential (1., 2., 3.) instead of all "1.", preserving the exact text for each item (references to `ortho_config`, `ortho_config_macros`, feature names like `json5`, `yaml`, `toml`, and the rebuild step) to satisfy markdownlint and keep content unchanged.
🤖 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/adopt-ortho-config-v0-8-0.md`:
- Around line 103-106: The docs currently duplicate the action to add
rust-version = "1.89"; change this paragraph so it is a verification step
instead of an add step (i.e., instruct the reader to confirm that rust-version =
"1.89" is present in Cargo.toml and matches ortho_config v0.8.0 MSRV), or
alternatively update the earlier "current state" text to state that rust-version
is not present so the later add step is valid; ensure the final text only
verifies presence or clearly states that the manifest needs to be updated to
rust-version = "1.89" to avoid duplicate instructions.
In `@docs/ortho-config-users-guide.md`:
- Around line 856-889: The example currently fails to compile because the
placeholder tuple types GreetArgs and RunArgs do not implement clap::Args
required by the #[derive(Subcommand)] on enum Commands; either add the clap
derive to those placeholders (e.g., derive Parser/Args on GreetArgs and RunArgs
so they satisfy Subcommand) or mark the code block as non-compiling by changing
the fence to rust,ignore to indicate it’s illustrative only; update whichever of
GreetArgs, RunArgs, or the code fence accordingly so the documentation builds or
clearly shows it’s non-compiling.
In `@docs/ortho-config-v0-6-0-migration-guide.md`:
- Around line 225-267: The footnote block references repository links tagged
v0.8.0 (e.g., URLs in the notes for [^forwarded-features], [^hello-world-cargo],
[^reexports], [^hello-world-figment], [^discovery-attr],
[^hello-world-discovery], [^discovery-errors], [^hello-world-discover-config],
[^saphyr], [^hello-world-yaml], [^changelog]) but the guide targets the v0.6.0
migration; update every occurrence of "v0.8.0" in these footnote URLs and link
texts to "v0.6.0" so the citations point to the correct tag, applying the same
tag change consistently across the entire footnote block.
In `@docs/ortho-config-v0-7-0-migration-guide.md`:
- Around line 49-56: Renumber the ordered list so the first item stays "1." and
the subsequent items use "2.", "3.", and "4." instead of repeating "1."; update
the lines that begin "Keep format features (`toml`, `json5`, `yaml`)...", "If
you disable default features, enable `serde_json` explicitly...", and "Expect
new transitive dependencies (`fluent-bundle`, `fluent-syntax`, `unic-langid`,
and `tracing`)..." to start with "2.", "3.", and "4." respectively to satisfy
MD029 and avoid markdownlint failures.
- Around line 188-226: Update the footnote links in the cited footnote block by
changing every reference that points to the v0.8.0 tag to the v0.7.0 tag;
specifically edit the URLs referenced in footnotes [^deps-0-7],
[^compose-layers], [^post-merge], [^localizer], [^localizeclap],
[^cli-defaults], [^selectedsubcommand], [^display-request], [^json-merge], and
[^rstest-bdd] so the GitHub links end with /blob/v0.7.0/... instead of
/blob/v0.8.0/.... Ensure all occurrences in that footnote block are changed
consistently.
In `@docs/vk-design.md`:
- Around line 79-82: The ordered list items are all numbered `1.` which trips
MD029; update the second and third entries so the list reads "1.
write_comment_body...", "2. write_comment...", "3. write_thread..." instead of
repeating `1.`; locate the list in docs/vk-design.md referencing the symbols
write_comment_body, write_comment, and write_thread and change the numeric
markers for the second and third lines accordingly so Markdown lint passes.
In `@src/main.rs`:
- Around line 233-236: Reformat the construction of filtered so it satisfies
rustfmt: break the long vec![args.next().map_or_else(|| OsString::from("vk"),
Into::into)] expression across lines and add the necessary commas/spacing so
cargo fmt passes; specifically adjust the expression that initializes filtered
(the vec![ ... ] containing args.next().map_or_else(...)]), keeping the same
logic (use map_or_else with OsString::from("vk") and Into::into) and then run
rustfmt (cargo fmt --all) to verify.
- Around line 204-223: The code is prepending a defaults layer despite
GlobalArgs::compose_layers_from_iter already including defaults; remove the
manual insertion of MergeLayer::defaults (the push of
MergeLayer::defaults(Cow::Owned(default_globals))) and rely on the composition
returned by GlobalArgs::compose_layers_from_iter, adjusting filtered_layers
initialization accordingly; also remove the now-unused OrthoJsonMergeExt import
to clean up unused imports referenced in the file.
---
Outside diff comments:
In `@docs/ortho-config-v0-6-0-migration-guide.md`:
- Around line 51-60: The ordered list in the migration guide uses repeated "1."
markers which triggers MD029; update the three list entries so their prefixes
are sequential (1., 2., 3.) instead of all "1.", preserving the exact text for
each item (references to `ortho_config`, `ortho_config_macros`, feature names
like `json5`, `yaml`, `toml`, and the rebuild step) to satisfy markdownlint and
keep content unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bebb91dd-4a3b-4bbf-92fe-9b152895b5ee
📒 Files selected for processing (8)
docs/execplans/adopt-ortho-config-v0-8-0.mddocs/github-token.mddocs/ortho-config-users-guide.mddocs/ortho-config-v0-6-0-migration-guide.mddocs/ortho-config-v0-7-0-migration-guide.mddocs/vk-design.mddocs/vk-end-to-end-testing-guide.mdsrc/main.rs
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 chain🏁 Script executed: Length of output: 2806
The text at line 51 of the migration guide still reads: Rephrase it to remove the second-person address, e.g.: 1. When default features are disabled, enable `serde_json` explicitly whenever
`cli_default_as_absent` or selected-subcommand merging helpers are required.As a side note, there are further second-person occurrences in the same file (lines 75, 159, 180, 184) that also breach the style rule — they fall outside the originally flagged range but warrant attention. |
Corrected the ordered lists numbering in v0.6.0 and v0.7.0 migration guides for clarity. Updated version number references from v0.8.0 to the appropriate v0.6.0 or v0.7.0 in documentation links to maintain accuracy. Also improved some wording for better clarity in execplans and user guide examples. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai review |
This comment was marked as resolved.
This comment was marked as resolved.
- Extracted parse_resolve_args helper to simplify CLI resolve command parsing in tests. - Rewrote existing resolve_subcommand_parses and resolve_subcommand_parses_message tests to use this helper. - Removed unused rstest dependency and EnvGuard struct. - Improved invalid VK_HTTP_TIMEOUT error test setup and cleanup with a closure-based helper. - Changed several tests to use a unified approach for creating and asserting errors. - Minor cleanup and formatting in vk-design.md unrelated to testing. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
docs/vk-design.md (2)
70-78:⚠️ Potential issue | 🟡 MinorRemove the escaped brackets in the footnote definition.
The escaped
\[^github-link\]:on line 72 renders as literal text, so the footnote reference on line 32 will not resolve. Remove the backslashes to enable GitHub-flavoured footnote linking.🛠️ Proposed fix
<!-- mdformat off --> -\[^github-link\]: GitHub Docs. "Linking to a pull request comment." +[^github-link]: GitHub Docs. "Linking to a pull request comment." <!-- markdownlint-disable-next-line MD013 -->As per coding guidelines, "
**/*.md: Use GitHub-flavoured Markdown footnotes ([^1]) for references".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/vk-design.md` around lines 70 - 78, The footnote definition is escaped as "\[^github-link\]:", preventing the footnote from resolving; update the footnote label by removing the backslashes so it reads "[^github-link]: GitHub Docs. \"Linking to a pull request comment.\"" (leave the URL and surrounding mdformat directives intact) so the inline reference [^github-link] resolves correctly.
86-91:⚠️ Potential issue | 🟡 MinorRenumber the ordered list to pass MD029.
The list items are all numbered
1.which triggers MD029 (ordered list item prefix). Markdownlint expects1/2/3style numbering.🛠️ Proposed fix
<!-- mdformat off --> 1. `write_comment_body` formats a single comment body to any `Write` implementation. -1. `write_comment` includes the diff for the first comment in a thread. -1. `write_thread` iterates over a thread and prints each comment body in turn. +2. `write_comment` includes the diff for the first comment in a thread. +3. `write_thread` iterates over a thread and prints each comment body in turn. <!-- mdformat on -->As per coding guidelines, "
**/*.md: Validate Markdown with make markdownlint".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/vk-design.md` around lines 86 - 91, The ordered list currently uses three repeated "1." entries and triggers MD029; update the list so the prefixes are sequential (e.g., "1. `write_comment_body`...", "2. `write_comment`...", "3. `write_thread`...") so markdownlint passes; edit the block containing the three functions (`write_comment_body`, `write_comment`, `write_thread`) to use sequential numbering and verify with the repo's markdownlint rules.src/main_tests.rs (1)
61-75: 🛠️ Refactor suggestion | 🟠 MajorWrap environment restoration in a guard to ensure panic-safety.
The closure-based helper does not guarantee cleanup if
f(err)panics—lines 71–74 will be skipped. An RAII guard withDropensures the original environment state is always restored.♻️ Proposed refactor using guard pattern
+struct EnvGuard { + key: &'static str, + original: Option<String>, +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.original { + Some(v) => set_var(self.key, v), + None => remove_var(self.key), + } + } +} + fn with_invalid_http_timeout_error<F>(f: F) where F: FnOnce(Arc<ortho_config::OrthoError>), { - let old_timeout = environment::var("VK_HTTP_TIMEOUT").ok(); - remove_var("VK_HTTP_TIMEOUT"); + let _guard = EnvGuard { + key: "VK_HTTP_TIMEOUT", + original: environment::var("VK_HTTP_TIMEOUT").ok(), + }; + remove_var("VK_HTTP_TIMEOUT"); set_var("VK_HTTP_TIMEOUT", "not-a-number"); let err = GlobalArgs::load_from_iter(std::iter::once(OsString::from("vk"))) .expect_err("invalid VK_HTTP_TIMEOUT should fail"); f(err); - match old_timeout { - Some(value) => set_var("VK_HTTP_TIMEOUT", value), - None => remove_var("VK_HTTP_TIMEOUT"), - } }As per coding guidelines, "Use
rstestfixtures for shared setup and to avoid repetition between tests" and "env mutations in tests MUST be wrapped in shared guards".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main_tests.rs` around lines 61 - 75, The helper with_invalid_http_timeout_error currently mutates VK_HTTP_TIMEOUT and restores it after calling f(err), but if f(err) panics the restoration is skipped; replace the manual restore with an RAII guard struct (e.g., EnvVarGuard) created before calling GlobalArgs::load_from_iter that captures old_timeout and implements Drop to restore or remove VK_HTTP_TIMEOUT, then remove the explicit post-call set_var/remove_var lines and invoke f(err) normally so cleanup is guaranteed even on panic; reference the helper function name with_invalid_http_timeout_error and the closure invocation f(err) when applying this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/vk-design.md`:
- Around line 70-78: The footnote definition is escaped as "\[^github-link\]:",
preventing the footnote from resolving; update the footnote label by removing
the backslashes so it reads "[^github-link]: GitHub Docs. \"Linking to a pull
request comment.\"" (leave the URL and surrounding mdformat directives intact)
so the inline reference [^github-link] resolves correctly.
- Around line 86-91: The ordered list currently uses three repeated "1." entries
and triggers MD029; update the list so the prefixes are sequential (e.g., "1.
`write_comment_body`...", "2. `write_comment`...", "3. `write_thread`...") so
markdownlint passes; edit the block containing the three functions
(`write_comment_body`, `write_comment`, `write_thread`) to use sequential
numbering and verify with the repo's markdownlint rules.
In `@src/main_tests.rs`:
- Around line 61-75: The helper with_invalid_http_timeout_error currently
mutates VK_HTTP_TIMEOUT and restores it after calling f(err), but if f(err)
panics the restoration is skipped; replace the manual restore with an RAII guard
struct (e.g., EnvVarGuard) created before calling GlobalArgs::load_from_iter
that captures old_timeout and implements Drop to restore or remove
VK_HTTP_TIMEOUT, then remove the explicit post-call set_var/remove_var lines and
invoke f(err) normally so cleanup is guaranteed even on panic; reference the
helper function name with_invalid_http_timeout_error and the closure invocation
f(err) when applying this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 888f1907-288c-4ef6-9f3a-75cdf025ad67
📒 Files selected for processing (2)
docs/vk-design.mdsrc/main_tests.rs
Refactored existing tests that verify invalid VK_HTTP_TIMEOUT handling by introducing rstest fixtures `invalid_http_timeout` and `invalid_http_timeout_error` for environment setup and error capturing. This improves test isolation and readability by cleaning up manual setup and teardown, and by utilizing rstest attributes to parametrize tests. Also added EnvGuard struct to manage environment variable restoration safely across test runs. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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 `@src/main_tests.rs`:
- Around line 42-52: Collapse the two tests cli_loads_repo_from_flag and
cli_loads_github_token_from_flag into a single parameterised rstest: create one
#[rstest(...)] test function (e.g., cli_loads_global_flags) that iterates over
tuples of (args, expected_field) for the two cases, call
Cli::try_parse_from(["vk", flag, value, "pr", "1"]) inside the test, then assert
the appropriate field on cli.global (use cli.global.repo.as_deref() and
cli.global.github_token.as_deref() depending on the parameter) to check the
expected value; keep the existing parse invocation (Cli::try_parse_from) and
assertions but drive them from the rstest parameters.
- Around line 56-78: Add a new function environment::var_os that mirrors the
existing environment::var but returns Option<OsString> (handling non-UTF8
values), then update EnvGuard and invalid_http_timeout to call
environment::var_os and store original as Option<OsString> (change
EnvGuard::original type accordingly); finally restore the original value in Drop
using environment::set_var_os when Some(OsString) or environment::remove_var
when None (add set_var_os if not present) so non-UTF8 env values are preserved.
🪄 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: 8695dad9-4b47-4110-8876-9153f4006434
📒 Files selected for processing (1)
src/main_tests.rs
…ocking Introduce `set_var_os` and `var_os` functions in the environment module to allow setting and reading environment variables using `OsStr`/`OsString` types while holding the global mutex lock. This improves safe and consistent environment variable access when dealing with non-UTF8 values. Also update tests to use these new functions for safer env var management. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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 `@src/environment.rs`:
- Around line 29-35: The two setter wrappers are duplicates: remove the
redundant set_var_os function and keep only set_var (both have signatures K:
AsRef<OsStr>, V: AsRef<OsStr>> and call the same locked unsafe env::set_var).
Before deleting, verify that signatures match and that all remaining set_var_os
calls are test-only; if any tests rely on the OS-specific name, change the
test-only re-export to alias set_var as set_var_os in the test utilities (e.g.,
update the test helper export in test_utils to pub use
crate::environment::set_var as set_var_os) and then delete the set_var_os
implementation from src/environment.rs.
In `@src/main_tests.rs`:
- Around line 201-213: Collapse the two tests into a single parameterised
rstest: replace the two #[test] functions resolve_subcommand_parses and
resolve_subcommand_parses_message with one #[rstest] fn
resolve_subcommand_parses that takes #[case] argv: &[&str] and #[case]
expected_message: Option<&str>, call parse_resolve_args(argv), assert
args.reference == "83#discussion_r1", and assert args.message.as_deref() ==
expected_message; add two cases (one with argv
["vk","resolve","83#discussion_r1"] and expected_message None, and one with argv
["vk","resolve","83#discussion_r1","-m","done"] and expected_message
Some("done")) so duplication around parse_resolve_args is removed.
- Around line 63-85: The current invalid_http_timeout fixture mutates the
environment in two separate critical sections causing race conditions; move the
fixture into test_utils and implement it using vk::environment::with_lock so
snapshotting the original value and setting "VK_HTTP_TIMEOUT" to "not-a-number"
happen inside the same locked section, and ensure EnvGuard's Drop also restores
or removes the variable inside with_lock to avoid restoring a stale value;
update the invalid_http_timeout fixture to construct and return this shared,
lock-wrapped EnvGuard from test_utils so other tests reuse the same guard/lock.
🪄 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: 7fd880c1-e339-4250-9bd0-d531695473b7
📒 Files selected for processing (3)
src/environment.rssrc/main.rssrc/main_tests.rs
…nd guards - Removed `set_var_os` from environment module and tests. - Replaced individual set/remove functions with `EnvGuard` struct that restores env vars on drop. - Added `invalid_http_timeout_guard` to safely manage VK_HTTP_TIMEOUT test setup. - Updated tests to use `EnvGuard` and `rstest` parametrized tests for clarity and safety. - Centralized environment variable locking for thread-safe test setups. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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 `@src/test_utils.rs`:
- Around line 225-243: In the Drop impl for EnvSandbox, swap the order so
environment::with_lock(...) runs first to restore original_env, then restore the
working directory; also make the env::set_current_dir call non-panicking (don’t
use expect) — handle or ignore the Result (e.g., match or log the Err) so a
failure to change dir won’t abort before env vars are restored. Keep changes
inside impl Drop for EnvSandbox and update references to env::set_current_dir
and environment::with_lock accordingly.
🪄 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: cd92eb7b-a4aa-4dc4-b5d4-021523221c20
📒 Files selected for processing (4)
src/environment.rssrc/main.rssrc/main_tests.rssrc/test_utils.rs
…ir errors In the Drop implementation for EnvSandbox, changed env::set_current_dir from expect() to ignoring its Result. This prevents potential panics during test cleanup and ensures environment restoration is attempted without aborting on failure. Also, fixed numbering in the migration guide documentation for clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/ortho-config-v0-6-0-migration-guide.md (2)
51-60:⚠️ Potential issue | 🟡 MinorRenumber this list.
Use
2.on Line 55 and3.on Line 59. The repeated1.prefixes still trigger MD029.As per coding guidelines,
**/*.md: Validate Markdown with make markdownlint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ortho-config-v0-6-0-migration-guide.md` around lines 51 - 60, The ordered list uses repeated "1." markers causing MD029; update the three list entries so they read "1. Change every `ortho_config` dependency...", "2. Retain any optional features (such as `json5`, `yaml`, or `toml`) on the main `ortho_config` dependency...", and "3. Rebuild the project to confirm the upgraded macro compiles cleanly..." so the second and third items use "2." and "3." respectively to fix the markdown lint failure.
225-269:⚠️ Potential issue | 🟡 MinorRemove the escapes from these footnote definitions.
Drop the leading backslash from each
[^...]:line in this block. The current form renders the notes as plain text instead of GitHub-flavoured footnotes.As per coding guidelines,
**/*.md: Use GitHub-flavoured Markdown footnotes ([^1]) for references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ortho-config-v0-6-0-migration-guide.md` around lines 225 - 269, The footnote definitions are escaped (leading backslashes) so GitHub Markdown renders them as plain text; remove the leading backslash from each footnote label (e.g. change \[^forwarded-features\], \[^hello-world-cargo\], \[^reexports\], \[^hello-world-figment\], \[^discovery-attr\], \[^hello-world-discovery\], \[^discovery-errors\], \[^hello-world-discover-config\], \[^saphyr\], \[^hello-world-yaml\], \[^changelog\] to [^forwarded-features], [^hello-world-cargo], [^reexports], [^hello-world-figment], [^discovery-attr], [^hello-world-discovery], [^discovery-errors], [^hello-world-discover-config], [^saphyr], [^hello-world-yaml], [^changelog]) so they become GitHub-flavoured footnotes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test_utils.rs`:
- Around line 98-132: EnvGuard currently releases the process-wide lock
immediately after setting VK_HTTP_TIMEOUT, allowing races; modify EnvGuard to
hold the env_sandbox_lock() MutexGuard for the guard's lifetime by adding a
field (e.g. guard: Option<MutexGuard<()>>) to EnvGuard, acquire
env_sandbox_lock() inside invalid_http_timeout_guard() before reading and
setting the variable and store the resulting MutexGuard in the EnvGuard struct
(keep original as before), and update Drop for EnvGuard to restore the
environment variable without reacquiring the lock (the held MutexGuard ensures
serialization) so the lock is only released when EnvGuard is dropped; reference
EnvGuard, invalid_http_timeout_guard(), and env_sandbox_lock() when making these
changes.
- Around line 192-208: The setup currently clears environment variables inside
environment::with_lock and then calls env::set_current_dir which may panic after
the env is mutated with no EnvSandbox to restore them; change the sequence and
error handling in the sandbox setup so the working directory is changed before
mutating environment OR wrap the env mutations in a fallible sequence that rolls
back on error and returns a Result instead of panicking. Concretely, ensure
env::set_current_dir(&sandbox_path) (or construction of the EnvSandbox that will
restore state) runs prior to the unsafe env::remove_var / env::set_var calls
inside environment::with_lock, or capture and restore previous env values on any
error path, and propagate errors via Result rather than calling expect/panic
(refer to env::set_current_dir, environment::with_lock and the
EnvSandbox/restore logic to locate where to adjust).
---
Duplicate comments:
In `@docs/ortho-config-v0-6-0-migration-guide.md`:
- Around line 51-60: The ordered list uses repeated "1." markers causing MD029;
update the three list entries so they read "1. Change every `ortho_config`
dependency...", "2. Retain any optional features (such as `json5`, `yaml`, or
`toml`) on the main `ortho_config` dependency...", and "3. Rebuild the project
to confirm the upgraded macro compiles cleanly..." so the second and third items
use "2." and "3." respectively to fix the markdown lint failure.
- Around line 225-269: The footnote definitions are escaped (leading
backslashes) so GitHub Markdown renders them as plain text; remove the leading
backslash from each footnote label (e.g. change \[^forwarded-features\],
\[^hello-world-cargo\], \[^reexports\], \[^hello-world-figment\],
\[^discovery-attr\], \[^hello-world-discovery\], \[^discovery-errors\],
\[^hello-world-discover-config\], \[^saphyr\], \[^hello-world-yaml\],
\[^changelog\] to [^forwarded-features], [^hello-world-cargo], [^reexports],
[^hello-world-figment], [^discovery-attr], [^hello-world-discovery],
[^discovery-errors], [^hello-world-discover-config], [^saphyr],
[^hello-world-yaml], [^changelog]) so they become GitHub-flavoured footnotes.
🪄 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: ebeab639-3d30-49d1-806e-c0f52d6fda62
📒 Files selected for processing (2)
docs/ortho-config-v0-6-0-migration-guide.mdsrc/test_utils.rs
- Added a MutexGuard field to EnvGuard to hold the env_sandbox_lock for the lifetime. - Removed environment::with_lock calls, relying on the held MutexGuard instead. - Ensures process-wide environment variable mutation is properly synchronized. - Moved `set_current_dir` call in EnvSandbox::new for clarity. This simplifies synchronization by tying env variable access to the MutexGuard's lifetime rather than explicit with_lock closures. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test_utils.rs (1)
179-184:⚠️ Potential issue | 🟠 MajorPropagate sandbox construction errors instead of panicking.
Replace
.expect(...)at Line 182, Line 184, and Line 192 with fallible propagation fromEnvSandbox::new(for example, returnstd::io::Result<Self>). Keep teardown deterministic without panic-driven control flow.Proposed fix
impl EnvSandbox { /// Create a new isolated environment and working-directory sandbox. #[must_use] - pub fn new() -> Self { + pub fn new() -> std::io::Result<Self> { let guard = env_sandbox_lock(); - let sandbox_dir = tempfile::tempdir().expect("create environment sandbox"); + let sandbox_dir = tempfile::tempdir()?; let sandbox_path = sandbox_dir.path().to_path_buf(); - let current_dir = environment::with_lock(env::current_dir).expect("capture current dir"); + let current_dir = environment::with_lock(env::current_dir)?; @@ - env::set_current_dir(&sandbox_path).expect("switch to environment sandbox"); + env::set_current_dir(&sandbox_path)?; @@ - Self { + Ok(Self { current_dir, original_env, sandbox_dir, _guard: guard, - } + }) } }#!/bin/bash # Verify panic-based error handling in non-test Rust modules. rg -n -C1 '\.expect\(' src/test_utils.rs rg -n -C1 'pub fn new\(\) -> Self' src/test_utils.rsAs per coding guidelines,
**/*.rs:.expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.Also applies to: 192-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test_utils.rs` around lines 179 - 184, Change EnvSandbox::new to return a fallible result (e.g., std::io::Result<Self>) and propagate errors instead of calling expect: replace the calls to tempfile::tempdir(), environment::with_lock(env::current_dir), and env::set_current_dir with propagated results using ? inside EnvSandbox::new; keep using env_sandbox_lock() and the same fields (current_dir, original_env, sandbox_dir, _guard) but return Ok(Self { ... }) on success so teardown remains deterministic without panic-driven control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test_utils.rs`:
- Around line 105-117: EnvGuard::drop currently performs unsafe
env::set_var/remove_var without synchronising with the shared environment lock;
change it to wrap the match that uses self.original.take() inside
environment::with_lock(...) so the unsafe env mutations are serialized (use
environment::with_lock to call env::set_var or env::remove_var), and similarly
modify invalid_http_timeout_guard() to perform reading and setting of
VK_HTTP_TIMEOUT inside environment::with_lock (returning the original value from
the closure) instead of calling env::var_os and unsafe env::set_var outside the
lock; keep env_sandbox_lock usage for the guard field but ensure all unsafe
env::set_var/remove_var calls run inside environment::with_lock to guarantee
process-wide serialization.
---
Duplicate comments:
In `@src/test_utils.rs`:
- Around line 179-184: Change EnvSandbox::new to return a fallible result (e.g.,
std::io::Result<Self>) and propagate errors instead of calling expect: replace
the calls to tempfile::tempdir(), environment::with_lock(env::current_dir), and
env::set_current_dir with propagated results using ? inside EnvSandbox::new;
keep using env_sandbox_lock() and the same fields (current_dir, original_env,
sandbox_dir, _guard) but return Ok(Self { ... }) on success so teardown remains
deterministic without panic-driven control flow.
🪄 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: 4291a297-c316-465e-919f-1b544faf2b52
📒 Files selected for processing (1)
src/test_utils.rs
The EnvSandbox::new constructor now returns Result<Self, std::io::Error> instead of unwrapping internally. Corresponding changes update usages to handle errors explicitly, improving error handling and robustness in tests that create environment sandboxes. Minor related cleanups include wrapping env var changes in locks for safety. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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 `@src/config_loader.rs`:
- Around line 107-127: Extract the duplicated sandbox and loader setup in the
tests into a shared helper (e.g., a new fn like
setup_global_args_without_cli_overrides(iter: impl IntoIterator<Item=OsString>)
-> (EnvSandbox, GlobalArgs)) that creates EnvSandbox::new(), writes any fixture
files if needed, and calls load_global_args_without_cli_overrides_from_iter;
update both test functions
load_global_args_without_cli_overrides_defaults_cleanly and
load_global_args_without_cli_overrides_honours_config_path_override to call this
helper and keep only their specific assertions (assert! checks and any
config_path-specific writes) so setup is centralized while assertions remain
per-test.
In `@src/test_utils.rs`:
- Around line 105-117: Extract a private helper (e.g., restore_env_key) that
takes (key: &str, original: Option<String>) and contains the match that calls
unsafe { env::set_var(key, value) } for Some and unsafe { env::remove_var(key) }
for None inside environment::with_lock closure; then replace the duplicate match
blocks in EnvGuard::drop and EnvSandbox::drop by calling this helper with
self.key and self.original.take() (or the equivalent field) so both drops reuse
the same restoration logic and remove duplication.
🪄 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: 2e029e15-558b-4642-865f-9a394e952964
📒 Files selected for processing (2)
src/config_loader.rssrc/test_utils.rs
Refactored test utilities by introducing a helper function to set up global args without CLI overrides, improving test clarity and reuse. Centralized environment variable restoration logic in EnvSandbox and EnvGuard drops with a dedicated restore_env_key function to reduce code duplication and improve safety in environment handling during tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
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
src/config_loader.rsexposingload_global_args_without_cli_overrides(), integrated into runtime to ignore empty CLI inputs.load_global_args_without_cli_overrides_defaults_cleanlyand tests insrc/main_tests.rsandsrc/test_utils.rs.Changes
load_global_args_without_cli_overrides().config_loader::load_global_args_without_cli_overrides()to compose and merge global layers while ignoring empty CLI inputs.Implementation plan (highlights)
Validation plan
Risks and open questions
Definition of done
📎 Task: https://www.devboxer.com/task/92612368-d50f-4dea-9386-d5106fc652b5