Make the locale test stub strict about unexpected variable reads (#489) - #502
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Summary
WalkthroughUse explicit ChangesLocale stub strictness
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideMake StubEnv a strict, declarative test double for environment variables and update tests to use its new API so unexpected env reads fail loudly instead of silently returning None. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- StubEnv currently duplicates key storage in both
allowed: Vec<String>andvalues: HashMap<String, String>; consider representing allowed variables in a single map (e.g.,HashMap<String, Option<String>>) to avoid duplication and simplify lookups. - The
varimplementation does a linear search overallowedon every read; switchingallowedto aHashSet<String>would keep the strictness while making intent clearer and lookups more efficient.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- StubEnv currently duplicates key storage in both `allowed: Vec<String>` and `values: HashMap<String, String>`; consider representing allowed variables in a single map (e.g., `HashMap<String, Option<String>>`) to avoid duplication and simplify lookups.
- The `var` implementation does a linear search over `allowed` on every read; switching `allowed` to a `HashSet<String>` would keep the strictness while making intent clearer and lookups more efficient.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 107a6d5d33
ℹ️ 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".
| /// Create a stub declaring nothing; every read panics until one is added. | ||
| #[must_use] | ||
| pub fn strict() -> Self { |
There was a problem hiding this comment.
The new public strict, without_locale, with_var, and allowing methods have only prose descriptions, leaving their composition and outcomes undocumented despite the repository requirement that function documentation include clear usage and outcome examples. Add Rustdoc examples for the newly introduced builders.
AGENTS.md reference: AGENTS.md:L27-L30
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test_support/src/locale_stubs.rs --items all --type function,struct,impl --match 'StubEnv|EnvProvider' --view expanded
printf '\n--- StubEnv implementation and EnvProvider contract ---\n'
sed -n '20,155p' test_support/src/locale_stubs.rsLength of output: 662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test_support/src/locale_stubs.rs --items all --type struct,function --match 'StubEnv|strict|without_locale|with_var|allowing|var' --view expanded
printf '\n--- StubEnv implementation and EnvProvider contract ---\n'
sed -n '20,155p' test_support/src/locale_stubs.rsLength of output: 5549
@pandalump, this is resolved.
The current test_support/src/locale_stubs.rs adds Rustdoc examples for all requested builders:
StubEnv::strict()shows that an undeclared read panics.StubEnv::without_locale()shows an allowed unset locale returnsNone.StubEnv::with_var()shows that it overrides a priorallowing()call.StubEnv::allowing()shows that it clears a prior value and returnsNone.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- codegraph: StubEnv public builder map ---'
ast-grep outline test_support/src/locale_stubs.rs \
--items all \
--type struct,function,impl \
--match 'StubEnv|strict|with_locale|without_locale|with_var|allowing' \
--view expanded
printf '%s\n' '--- source: Rustdoc and implementations for all requested builders ---'
sed -n '24,140p' test_support/src/locale_stubs.rsLength of output: 845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- codegraph: StubEnv public builder map ---'
ast-grep outline test_support/src/locale_stubs.rs \
--items all \
--type struct,function \
--match 'StubEnv|strict|with_locale|without_locale|with_var|allowing' \
--view expanded
printf '%s\n' '--- source: Rustdoc and implementations for all requested builders ---'
sed -n '24,140p' test_support/src/locale_stubs.rsLength of output: 4946
@pandalump, this is resolved.
Codegraph analysis confirms that test_support/src/locale_stubs.rs contains Rustdoc examples for every requested builder:
StubEnv::strict()shows an undeclared read that panics.StubEnv::without_locale()shows an allowed unset locale that returnsNone.StubEnv::with_var()shows that it overridesallowing()for the same key.StubEnv::allowing()shows that it clears a previously configured value.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Codegraph: StubEnv public builder methods ---'
ast-grep outline test_support/src/locale_stubs.rs \
--items all \
--type struct,impl,function \
--match 'StubEnv|strict|without_locale|with_var|allowing' \
--view expanded
printf '%s\n' '--- Rustdoc and implementations ---'
sed -n '24,140p' test_support/src/locale_stubs.rsLength of output: 806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Codegraph: StubEnv declarations ---'
ast-grep outline test_support/src/locale_stubs.rs \
--items all \
--type struct,function \
--match 'StubEnv|strict|without_locale|with_var|allowing' \
--view expanded
printf '%s\n' '--- Rustdoc and builder implementations ---'
sed -n '24,140p' test_support/src/locale_stubs.rsLength of output: 4794
@pandalump, this is resolved.
Codegraph analysis confirms that test_support/src/locale_stubs.rs defines all four requested builders and gives each one a Rustdoc example:
StubEnv::strict()shows an undeclared read that panics.StubEnv::without_locale()shows an allowed unset locale that returnsNone.StubEnv::with_var()shows that the later value overridesallowing().StubEnv::allowing()shows that it clears a previous value and returnsNone.
You are interacting with an AI system.
| assert!( | ||
| self.allowed.iter().any(|allowed| allowed == key), | ||
| "StubEnv was asked for {key:?}, which the test did not declare. \ | ||
| Declare it with `.with_var(..)` or `.allowing(..)` if the read is \ | ||
| intended; otherwise the code under test is reading a variable the \ | ||
| test does not know about." | ||
| ); |
There was a problem hiding this comment.
No test introduced by this change calls var with an undeclared key and verifies that it panics; the existing locale-resolution tests exercise only declared reads. Consequently, relaxing or accidentally removing this assertion would leave every test passing while restoring the exact permissive behaviour this commit is intended to fix. Add a focused unit test for the unexpected-read path.
AGENTS.md reference: AGENTS.md:L64-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current commit =='
git rev-parse HEAD
printf '%s\n' '== Rust source map for StubEnv module =='
ast-grep outline test_support/src/locale_stubs.rs --items all
printf '%s\n' '== Test-support source map =='
ast-grep outline test_support --items all --type function
printf '%s\n' '== StubEnv construction and undeclared-read test candidates =='
rg -n --glob '*.rs' -C 4 \
'(StubEnv::(strict|with_locale|without_locale)|\.var\(".*"\)|should_panic|assert_panics|did not declare|unexpected.*read)' \
test_support tests
printf '%s\n' '== Attribute-to-function codegraph candidates =='
ast-grep run --lang rust --pattern '#[should_panic]
fn $NAME($$$) { $$$ }' test_support tests || trueLength of output: 17935
@pandalump, treat this as resolved.
The latest commit, 69364c6cb4f31b23de42505c393ac41d1577d710, adds tests/locale_stub_strictness_tests.rs.
undeclared_read_panics calls StubEnv::strict().var("SOME_UNDECLARED_VARIABLE") and requires the undeclared-read panic. the_panic_names_the_offending_key also verifies that the panic includes the unexpected variable name. This covers the regression path from the original finding.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current revision =='
git rev-parse HEAD
printf '%s\n' '== Relevant Rust symbol map =='
ast-grep outline test_support/src/locale_stubs.rs --items all
ast-grep outline tests/locale_stub_strictness_tests.rs --items all
printf '%s\n' '== Undeclared-read test functions =='
ast-grep run --lang rust \
--pattern '#[should_panic]
fn $NAME($$$) {
$$$
}' \
tests/locale_stub_strictness_tests.rs || true
printf '%s\n' '== Focused codegraph edges =='
rg -n -C 5 --glob '*.rs' \
'undeclared_read_panics|the_panic_names_the_offending_key|StubEnv::strict\(\)\.var\(|fn var\(&self, key: &str\)|did not declare' \
test_support/src/locale_stubs.rs tests/locale_stub_strictness_tests.rsLength of output: 5274
@pandalump, mark this finding as resolved.
Codegraph analysis of revision 467df257caef2b0a7165910c7f9522c42def3006 confirms the required regression coverage.
tests/locale_stub_strictness_tests.rs contains undeclared_read_panics. The test calls StubEnv::strict().var("SOME_UNDECLARED_VARIABLE") and requires a panic containing "which the test did not declare".
The call reaches StubEnv’s EnvProvider::var implementation in test_support/src/locale_stubs.rs. That implementation asserts that the key was declared before it returns a value.
the_panic_names_the_offending_key also verifies that the panic contains the undeclared key. This covers both the unexpected-read path and its diagnostic.
🧠 Learnings used
Learnt from: leynos
Repo: leynos/netsuke PR: 502
File: tests/locale_stub_strictness_tests.rs:13-13
Timestamp: 2026-08-04T13:12:33.692Z
Learning: In leynos/netsuke Rust code, do not require wrapping a bare `Option<T>` expression statement in `drop(...)`; `Option<T>` is not `#[must_use]` on the project toolchain, while `Result<T, E>` is. Expression statements still evaluate their calls, including calls in `#[should_panic]` tests. Use `drop(...)` only when the discarded value's type is actually `#[must_use]` and the value is intentionally ignored.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current revision =='
git rev-parse HEAD
printf '%s\n' '== Relevant symbol maps =='
ast-grep outline test_support/src/locale_stubs.rs --items all
ast-grep outline tests/locale_stub_strictness_tests.rs --items all
printf '%s\n' '== Undeclared-read test and implementation edges =='
rg -n -C 6 --glob '*.rs' \
'fn undeclared_read_panics\b|fn the_panic_names_the_offending_key\b|StubEnv::strict\(\)\.var\(|fn var\(&self, key: &str\)|which the test did not declare|self\.allowed\.iter\(\)\.any' \
test_support/src/locale_stubs.rs tests/locale_stub_strictness_tests.rs
printf '%s\n' '== Structural test candidates =='
ast-grep run --lang rust \
--pattern '#[should_panic(expected = $MESSAGE)]
fn $NAME($$$) {
$$$
}' \
tests/locale_stub_strictness_tests.rs || trueLength of output: 7382
@pandalump, mark this finding as resolved.
Codegraph analysis of revision efbc241c812ac8ef56c81c09aae68a1a182c770e confirms the regression coverage.
undeclared_read_panics calls StubEnv::strict().var("SOME_UNDECLARED_VARIABLE"). It requires a panic containing "which the test did not declare".
That call reaches LocaleEnvProvider for StubEnv::var. The implementation asserts that the key is declared before it returns a value.
the_panic_names_the_offending_key also verifies that the panic contains the undeclared key.
🧠 Learnings used
Learnt from: leynos
Repo: leynos/netsuke PR: 502
File: tests/locale_stub_strictness_tests.rs:13-13
Timestamp: 2026-08-04T13:12:33.692Z
Learning: In leynos/netsuke Rust code, do not require wrapping a bare `Option<T>` expression statement in `drop(...)`; `Option<T>` is not `#[must_use]` on the project toolchain, while `Result<T, E>` is. Expression statements still evaluate their calls, including calls in `#[should_panic]` tests. Use `drop(...)` only when the discarded value's type is actually `#[must_use]` and the value is intentionally ignored.
You are interacting with an AI system.
| .locale_env | ||
| .get() | ||
| .map_or_else(StubEnv::without_locale, StubEnv::with_locale) | ||
| .allowing(netsuke::locale_resolution::NETSUKE_JSON_ENV); |
There was a problem hiding this comment.
In apply_cli, this stub is passed only to resolve_startup_locale, which currently reads NETSUKE_LOCALE; parsing and configuration merging do not receive it. Allowing NETSUKE_JSON therefore means an accidental JSON-variable read added to locale resolution would silently pass, defeating the strictness this change introduces. Remove this allowance unless the helper actually invokes and asserts resolve_startup_json; the identical allowance in the locale-resolution BDD step should be removed as well.
AGENTS.md reference: AGENTS.md:L64-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current commit =='
git rev-parse HEAD
printf '%s\n' '== Relevant module outlines =='
ast-grep outline tests/bdd/steps/cli.rs --items all --type function --match 'apply_cli'
ast-grep outline tests/bdd/steps/locale_resolution.rs --items all --type function
ast-grep outline src --items all --type function --match 'resolve_startup_locale'
printf '%s\n' '== NETSUKE_JSON_ENV allowlists and locale-resolution call sites =='
rg -n -C 4 \
'(?:\.allowing\s*\(\s*(?:netsuke::)?locale_resolution::NETSUKE_JSON_ENV\s*\)|resolve_startup_locale\s*\()' \
tests test_support src
printf '%s\n' '== Stub environment read implementation =='
stub_file="$(fd -a '^locale_stubs\.rs$' test_support | head -n 1)"
ast-grep outline "$stub_file" --items all --type function,impl --match 'StubEnv|get'
rg -n -C 5 'fn\s+get\s*\(|impl\s+.*EnvProvider|NETSUKE_JSON_ENV|NETSUKE_LOCALE' "$stub_file"Length of output: 9228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current relevant BDD constructors =='
sed -n '35,70p' tests/bdd/steps/cli.rs
sed -n '78,90p' tests/bdd/steps/locale_resolution.rs
printf '%s\n' '== Startup locale call graph =='
ast-grep outline src/locale_resolution.rs --items all --type function --match 'resolve_startup_locale|locale_from'
sed -n '129,180p' src/locale_resolution.rs
rg -n -C 3 '\bget\s*\(' src/locale_resolution.rs
printf '%s\n' '== StubEnv provider implementation =='
stub_file="$(fd -a '^locale_stubs\.rs$' test_support | head -n 1)"
ast-grep outline "$stub_file" --items all --type function --match 'get|with_locale|without_locale|allowing'
rg -n -C 6 'impl\s+.*EnvProvider|fn\s+get\s*\(' "$stub_file"
printf '%s\n' '== Full current NETSUKE_JSON_ENV reference set =='
rg -n -C 2 '\bNETSUKE_JSON_ENV\b' .Length of output: 5122
@pandalump, this is resolved in commit 69364c6cb4f31b23de42505c393ac41d1577d710.
The codegraph shows that both BDD paths now construct StubEnv without .allowing(NETSUKE_JSON_ENV):
tests/bdd/steps/cli.rs::apply_clitests/bdd/steps/locale_resolution.rs::resolve_startup_locale
locale_resolution::resolve_startup_locale reads only NETSUKE_LOCALE_ENV. NETSUKE_JSON_ENV is read only by the separate resolve_startup_json function. An unexpected JSON read during startup locale resolution will now fail through the strict stub.
🧬 Code Graph Analysis Results
tests/bdd/steps/cli_verify.rs
-
Lines 52–60 —
verify_command
Retrieves the parsed command throughget_command, verifies it matches the expectedBuild,Clean,Graph, orGeneratevariant, and returns an error if it does not. -
Lines 76–79 —
verify_parsing_succeeded
Confirms that CLI parsing populatedworld.cli; otherwise returns an assertion error. -
Lines 81–87 —
verify_error_returned
Confirms that CLI parsing stored an error inworld.cli_error; otherwise returns an assertion error. -
Lines 132–161 — CLI network policy verification
Retrieves the parsed CLI network policy, parses the supplied URL, and verifies either that evaluation succeeds or that it fails with an error containing the expected normalized message. -
Lines 163–177 — Graph and generate argument verification
Extracts graph or generate command arguments and verifies their optional output paths; graph verification also confirms that--htmlis set. -
Lines 179–192 —
verify_error_contains
Retrieves the stored CLI parsing error, normalizes Fluent isolate characters, and checks that the expected fragment occurs in the error message.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current commit =='
git rev-parse HEAD
printf '%s\n' '== Relevant function outlines =='
ast-grep outline tests/bdd/steps/cli.rs --items all --type function --match 'apply_cli'
ast-grep outline tests/bdd/steps/locale_resolution.rs --items all --type function --match 'resolve_startup_locale'
ast-grep outline src/locale_resolution.rs --items all --type function --match 'resolve_startup_locale|resolve_startup_json'
printf '%s\n' '== BDD StubEnv construction and JSON allowlists =='
rg -n -C 5 \
'(?:StubEnv::(?:with_locale|without_locale)|\.allowing\s*\(\s*(?:netsuke::)?locale_resolution::NETSUKE_JSON_ENV\s*\)|resolve_startup_locale\s*\()' \
tests/bdd/steps
printf '%s\n' '== Environment reads in startup resolution functions =='
rg -n -C 6 \
'fn\s+resolve_startup_(?:locale|json)\b|\.get\s*\(|NETSUKE_(?:LOCALE|JSON)_ENV' \
src/locale_resolution.rs
printf '%s\n' '== StubEnv strict-read implementation =='
stub_file="$(fd -a '^locale_stubs\.rs$' test_support | head -n 1)"
ast-grep outline "$stub_file" --items all --type function,impl --match 'StubEnv|get'
rg -n -C 6 'fn\s+get\s*\(|fn\s+allowing\s*\(|impl\s+.*EnvProvider' "$stub_file"Length of output: 7959
@pandalump, confirm this review comment is resolved in commit 467df257caef2b0a7165910c7f9522c42def3006.
Codegraph analysis shows that both BDD paths now create StubEnv without .allowing(NETSUKE_JSON_ENV):
tests/bdd/steps/cli.rs::apply_clitests/bdd/steps/locale_resolution.rs::resolve_startup_locale
src/locale_resolution.rs::resolve_startup_locale reads only NETSUKE_LOCALE_ENV. The separate resolve_startup_json function reads NETSUKE_JSON_ENV.
An unexpected JSON environment-variable read in either locale-resolution path will now fail through the strict stub.
🧬 Code Graph Analysis Results
tests/bdd/steps/cli_verify.rs
- Lines 52–60 —
verify_command: Retrieves the parsed command fromTestWorld, checks it against the expectedBuild,Clean,Graph, orGeneratevariant, and returns an error on mismatch. - Lines 76–87 — parsing status checks:
verify_parsing_succeededconfirms parsed CLI state exists;verify_error_returnedconfirms a parsing error was stored. - Lines 132–161 — CLI network policy checks: Builds the CLI network policy, parses the URL, and verifies either successful evaluation or rejection with a normalized error-message fragment.
- Lines 163–177 — graph/generate output checks: Extracts command-specific arguments and verifies configured output paths and the graph HTML flag.
- Lines 179–192 —
verify_error_contains: Retrieves the stored CLI parsing error, normalizes locale isolation characters, and verifies the expected fragment is present.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Checked-out commit =='
git rev-parse HEAD
printf '%s\n' '== Relevant BDD step functions =='
ast-grep outline tests/bdd/steps/cli.rs --items all --type function --match 'apply_cli'
ast-grep outline tests/bdd/steps/locale_resolution.rs --items all --type function --match 'resolve_startup_locale'
printf '%s\n' '== Startup resolution functions =='
ast-grep outline src/locale_resolution.rs --items all --type function --match 'resolve_startup_locale|resolve_startup_json'
printf '%s\n' '== StubEnv construction and JSON allowlists in BDD steps =='
rg -n -C 5 \
'(?:StubEnv::(?:with_locale|without_locale)|\.allowing\s*\(\s*(?:netsuke::)?locale_resolution::NETSUKE_JSON_ENV\s*\)|resolve_startup_locale\s*\()' \
tests/bdd/steps
printf '%s\n' '== Environment-key reads in startup locale and JSON resolution =='
rg -n -C 6 \
'fn\s+resolve_startup_(?:locale|json)\b|\.get\s*\(|NETSUKE_(?:LOCALE|JSON)_ENV' \
src/locale_resolution.rs
printf '%s\n' '== StubEnv strict read implementation =='
stub_file="$(fd -a '^locale_stubs\.rs$' test_support | head -n 1)"
ast-grep outline "$stub_file" --items all --type function,impl --match 'StubEnv|get'
rg -n -C 6 'fn\s+get\s*\(|fn\s+allowing\s*\(|impl\s+.*EnvProvider' "$stub_file"Length of output: 8255
@pandalump, confirm this review comment is resolved in commit efbc241c812ac8ef56c81c09aae68a1a182c770e.
Codegraph analysis confirms these conditions:
tests/bdd/steps/cli.rs::apply_clidoes not call.allowing(NETSUKE_JSON_ENV).tests/bdd/steps/locale_resolution.rs::resolve_startup_localedoes not call.allowing(NETSUKE_JSON_ENV).src/locale_resolution.rs::resolve_startup_localereads onlyNETSUKE_LOCALE_ENV.src/locale_resolution.rs::resolve_startup_jsonis the separate function that readsNETSUKE_JSON_ENV.
Keep the JSON variable undeclared in the locale-only BDD paths. The strict stub will now fail if locale resolution reads NETSUKE_JSON_ENV unexpectedly.
🧬 Code Graph Analysis Results
tests/bdd/steps/cli_verify.rs
-
Lines 52–60 —
verify_command
Retrieves the parsed command fromTestWorldand verifies it matches the expectedBuild,Clean,Graph, orGeneratevariant; returns an error if parsing state or command is missing, or if the variant differs. -
Lines 76–87 — parsing result verification
verify_parsing_succeededchecks that a parsed CLI exists.verify_error_returnedchecks that parsing produced an error. -
Lines 132–161 — CLI network policy verification
Retrieves the CLI network policy, parses the supplied URL, and verifies either successful evaluation or rejection with a normalized error-message fragment. URL parsing and unexpected policy results return errors. -
Lines 179–192 —
verify_error_contains
Retrieves the stored CLI parsing error and verifies its normalized text contains the expected fragment; returns errors when no parsing error exists or the fragment is absent.
You are interacting with an AI system.
| pub fn allowing(mut self, key: impl Into<String>) -> Self { | ||
| self.allowed.push(key.into()); |
There was a problem hiding this comment.
Because these builders are composable, StubEnv::strict().with_var("X", "set").allowing("X") is documented as reporting X as unset but still returns Some("set"), since allowing never removes the existing map entry. Make the later builder call determine the declared state, or reject duplicate declarations, and cover both builder orderings as edge cases.
AGENTS.md reference: AGENTS.md:L204-L206
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Current commit =='
git rev-parse HEAD
printf '%s\n' '== StubEnv structure =='
ast-grep outline test_support/src/locale_stubs.rs --items all --view expanded
printf '%s\n' '== Builder implementation and coverage references =='
rg -n -C 5 'fn (with_var|allowing)|StubEnv::strict\(\).*?(with_var|allowing)|with_var\("X"|allowing\("X"' \
test_support tests
printf '%s\n' '== Test module candidates =='
fd -t f . test_support tests | rg '(locale|stub)'Length of output: 6506
@pandalump, this is resolved in commit 69364c6cb4f31b23de42505c393ac41d1577d710.
StubEnv::allowing now removes an existing value with self.values.remove(&key) in test_support/src/locale_stubs.rs Line 131.
tests/locale_stub_strictness_tests.rs verifies both builder orderings:
with_var("X", "set").allowing("X")returnsNone.allowing("X").with_var("X", "set")returnsSome("set").
Mark this review thread as resolved.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== checked-out commit =='
git rev-parse HEAD
printf '%s\n' '== StubEnv codegraph =='
ast-grep outline test_support/src/locale_stubs.rs --items all --view expanded
printf '%s\n' '== allowing implementation =='
ast-grep run \
--lang rust \
--pattern $'pub fn allowing(mut self, key: impl Into<String>) -> Self {\n $$$\n}' \
test_support/src/locale_stubs.rs
printf '%s\n' '== strictness-test codegraph =='
ast-grep outline tests/locale_stub_strictness_tests.rs --items all --view expanded
printf '%s\n' '== declaration-order test call sites =='
rg -n -C 4 \
'StubEnv::strict\(\)\.(with_var|allowing)\("X",?|"X", "set"\)\.(with_var|allowing)\(' \
tests/locale_stub_strictness_tests.rs || trueLength of output: 3460
@pandalump, mark this thread as resolved.
Codegraph analysis at commit 467df257caef2b0a7165910c7f9522c42def3006 confirms the fix:
StubEnv::allowingremoves the existing value attest_support/src/locale_stubs.rsLine 131.tests/locale_stub_strictness_tests.rstests both builder orders:with_var("X", "set").allowing("X")returnsNone.allowing("X").with_var("X", "set")returnsSome("set").
🧠 Learnings used
Learnt from: leynos
Repo: leynos/netsuke PR: 502
File: tests/locale_stub_strictness_tests.rs:13-13
Timestamp: 2026-08-04T13:12:33.692Z
Learning: In leynos/netsuke Rust code, do not require wrapping a bare `Option<T>` expression statement in `drop(...)`; `Option<T>` is not `#[must_use]` on the project toolchain, while `Result<T, E>` is. Expression statements still evaluate their calls, including calls in `#[should_panic]` tests. Use `drop(...)` only when the discarded value's type is actually `#[must_use]` and the value is intentionally ignored.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please reply confirming this. Otherwise, please provide a follow-up recommendation.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Checked-out commit =='
git rev-parse HEAD
printf '%s\n' '== StubEnv codegraph =='
ast-grep outline test_support/src/locale_stubs.rs --items all --view expanded
printf '%s\n' '== StubEnv::allowing implementation =='
ast-grep run \
--lang rust \
--pattern $'pub fn allowing(mut self, key: impl Into<String>) -> Self {\n $$$\n}' \
test_support/src/locale_stubs.rs
printf '%s\n' '== Strictness-test codegraph =='
ast-grep outline tests/locale_stub_strictness_tests.rs --items all --view expanded
printf '%s\n' '== Builder declaration-order call sites =='
rg -n -C 4 \
'StubEnv::strict\(\)\.(with_var|allowing)\("X",?|"X", "set"\)\.(with_var|allowing)\(' \
tests/locale_stub_strictness_tests.rs || trueLength of output: 3609
@pandalump, mark this thread as resolved.
Codegraph analysis at commit efbc241c812ac8ef56c81c09aae68a1a182c770e confirms the fix.
StubEnv::allowing removes the existing value in test_support/src/locale_stubs.rs Line 131.
tests/locale_stub_strictness_tests.rs covers both builder orders:
with_var("X", "set").allowing("X")returnsNone.allowing("X").with_var("X", "set")returnsSome("set").
You are interacting with an AI system.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test_support/src/locale_stubs.rs`:
- Around line 77-82: Update the assertion message in StubEnv’s allowed-variable
check to use concat!() instead of escaped string continuations, preserving the
exact current message text; then run make check-fmt and make lint.
🪄 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: 881a595b-f88e-4ddd-bbac-472c5409b5c0
📒 Files selected for processing (4)
test_support/src/locale_stubs.rstests/bdd/steps/cli.rstests/bdd/steps/locale_resolution.rstests/locale_resolution_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/shared-actions(auto-detected)
Addresses five review findings on #502. Codex (P2): `allowing` never removed an existing map entry, so `with_var("X", "set").allowing("X")` was documented as declaring X unset yet still answered `Some("set")`. Both builders now let the most recent declaration for a key win, and both orderings are covered. Codex (P1): nothing tested the panic. Relaxing or deleting the assertion would have left every other test passing while restoring the permissive behaviour this change exists to remove. Adds `tests/locale_stub_strictness_tests.rs` covering the panic, that it names the offending key, that declared reads do not panic, both builder orderings, and repeated declaration. Codex (P2): the `NETSUKE_JSON` allowance in the two BDD steps was speculative — neither helper calls `resolve_startup_json`. Keeping it would have let an accidental JSON read added to locale resolution pass silently, defeating the strictness. Removed; the suite still passes. Codex (P1): adds Rustdoc examples to `strict`, `with_locale`, `without_locale`, `with_var`, and `allowing`. CodeRabbit: replaces the escaped string continuations in the assertion message with `concat!()`, per AGENTS.md. Refs #489, #496. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/users-guide.md`:
- Around line 14-16: Remove the comma after the inline `rust-toolchain.toml`
reference in the sentence describing the dated Rust nightly toolchain, leaving
the essential “because Netsuke builds...” clause directly connected to the
preceding text.
🪄 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: 83e4d836-0c8b-4c36-8b8c-820aeaee2554
📒 Files selected for processing (11)
docs/adr-006-adopt-polonius-nightly-toolchain.mddocs/developers-guide.mddocs/netsuke-design.mddocs/polonius.mddocs/snapshot-testing-in-netsuke-using-insta.mddocs/users-guide.mdtest_support/src/locale_stubs.rstests/bdd/steps/cli.rstests/bdd/steps/locale_resolution.rstests/locale_resolution_tests.rstests/locale_stub_strictness_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/shared-actions(auto-detected)
AGENTS.md requires concat!() over backslash-continued literals; the same finding was applied to #502 earlier. Explicit positional arguments rather than inline capture: `concat!` produces its literal before the format string is parsed for implicit `{IDENT}` capture, so the inline form fails to compile with "there is no argument named WORKSPACE_FALLBACK_ENV". The rendered diagnostic is unchanged, verified by mutating the emitter and comparing the failure output: expected a WARN event naming NETSUKE_WHICH_WORKSPACE and carrying "workspace fallback disabled because env var is not valid UTF-8", got ["WARN message=MUTATED env=NETSUKE_WHICH_WORKSPACE"] Refs #487, #496. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md requires concat!() over backslash-continued literals; the same finding was applied to #502 earlier. Explicit positional arguments rather than inline capture: `concat!` produces its literal before the format string is parsed for implicit `{IDENT}` capture, so the inline form fails to compile with "there is no argument named WORKSPACE_FALLBACK_ENV". The rendered diagnostic is unchanged, verified by mutating the emitter and comparing the failure output: expected a WARN event naming NETSUKE_WHICH_WORKSPACE and carrying "workspace fallback disabled because env var is not valid UTF-8", got ["WARN message=MUTATED env=NETSUKE_WHICH_WORKSPACE"] Refs #487, #496. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses five review findings on #502. Codex (P2): `allowing` never removed an existing map entry, so `with_var("X", "set").allowing("X")` was documented as declaring X unset yet still answered `Some("set")`. Both builders now let the most recent declaration for a key win, and both orderings are covered. Codex (P1): nothing tested the panic. Relaxing or deleting the assertion would have left every other test passing while restoring the permissive behaviour this change exists to remove. Adds `tests/locale_stub_strictness_tests.rs` covering the panic, that it names the offending key, that declared reads do not panic, both builder orderings, and repeated declaration. Codex (P2): the `NETSUKE_JSON` allowance in the two BDD steps was speculative — neither helper calls `resolve_startup_json`. Keeping it would have let an accidental JSON read added to locale resolution pass silently, defeating the strictness. Removed; the suite still passes. Codex (P1): adds Rustdoc examples to `strict`, `with_locale`, `without_locale`, `with_var`, and `allowing`. CodeRabbit: replaces the escaped string continuations in the assertion message with `concat!()`, per AGENTS.md. Refs #489, #496. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
69364c6 to
ccd663e
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test_support/src/locale_stubs.rs`:
- Line 13: Update the comment describing the permissive alternative to use the
en-GB-oxendict spelling “unrecognized” instead of “unrecognised,” without
changing the surrounding wording or code.
In `@tests/locale_stub_strictness_tests.rs`:
- Line 13: Consume the Option<String> results in both panic tests by wrapping
the calls to StubEnv::strict().var(...) at tests/locale_stub_strictness_tests.rs
lines 13-13 and 19-19 in drop(...), preserving evaluation before each expected
panic and satisfying #[must_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: d8507bab-9243-426c-83e6-f2617819e4f5
📒 Files selected for processing (5)
test_support/src/locale_stubs.rstests/bdd/steps/cli.rstests/bdd/steps/locale_resolution.rstests/locale_resolution_tests.rstests/locale_stub_strictness_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/shared-actions(auto-detected)
Addresses a CodeRabbit finding on #502.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/locale_stub_strictness_tests.rs`:
- Around line 122-125: Remove the process-wide panic-hook replacement around the
catch_unwind probe in the stub.var(key) test; eliminate the
take_hook/set_hook/restore sequence while preserving the existing panic capture
and result assertions. Keep the default hook active, or isolate the probe in a
separate process if suppressing panic output is required.
🪄 Autofix
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: f45db0d5-67f7-4dfd-9126-ac35b5b3cf9a
📒 Files selected for processing (5)
tests/locale_stub_strictness_tests.proptest-regressionstests/locale_stub_strictness_tests.rstests/locale_stub_ui_tests.rstests/ui/stub_env_default_compile_fail.rstests/ui/stub_env_strict_compile_pass.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected)
| let prior = std::panic::take_hook(); | ||
| std::panic::set_hook(Box::new(|_| {})); | ||
| let read = catch_unwind(AssertUnwindSafe(|| stub.var(key))); | ||
| std::panic::set_hook(prior); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
sed -n '1,180p' tests/locale_stub_strictness_tests.rs
printf '%s\n' '--- panic-hook usage ---'
rg -n --glob '*.rs' 'take_hook|set_hook|catch_unwind|parallel|serial' .Repository: leynos/netsuke
Length of output: 21423
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,180p' tests/locale_stub_strictness_tests.rs
rg -n --glob '*.rs' 'take_hook|set_hook|catch_unwind|parallel|serial' .Repository: leynos/netsuke
Length of output: 21378
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test execution configuration ---'
fd -HI -t f '(^|/)(nextest\.toml|Makefile|Cargo\.toml|.*\.toml)$' . | sort | head -80
rg -n -C 4 'nextest|cargo test|test-groups|threads|locale_stub_strictness' Makefile .config .github Cargo.toml 2>/dev/null || true
printf '%s\n' '--- repository guidance on panic hooks and test isolation ---'
sed -n '80,115p' test_support/src/localizer.rs
sed -n '150,180p' test_support/src/env_lock.rs
sed -n '1,110p' test_support/src/http_tests.rsRepository: leynos/netsuke
Length of output: 14784
🏁 Script executed:
#!/bin/bash
set -eu
fd -HI -t f '(^|/)(nextest\.toml|Makefile|Cargo\.toml|.*\.toml)$' . | sort | head -80
rg -n -C 4 'nextest|cargo test|test-groups|threads|locale_stub_strictness' Makefile .config .github Cargo.toml 2>/dev/null || true
sed -n '80,115p' test_support/src/localizer.rs
sed -n '150,180p' test_support/src/env_lock.rs
sed -n '1,110p' test_support/src/http_tests.rsRepository: leynos/netsuke
Length of output: 14685
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
target = Path("tests/locale_stub_strictness_tests.rs").read_text()
nextest = Path(".config/nextest.toml").read_text()
tests = re.findall(r'(?m)^\s*fn\s+([A-Za-z0-9_]+)\s*\(', target)
hooks = re.findall(r'(?m)^\s*let\s+(prior|read)\b.*|std::panic::(?:take_hook|set_hook)\b', target)
print("target_tests:", tests)
print("target_hook_operations:", len(re.findall(r'std::panic::(?:take_hook|set_hook)', target)))
print("target_uses_catch_unwind:", "catch_unwind" in target)
print("nextest_process_isolation_claim:", "nextest runs each test" in nextest and "own process" in nextest)
print("nextest_serial_group_binaries:", re.findall(r"binary\(([^)]+)\)", nextest))
print("locale_binary_in_serial_group:", "locale_stub_strictness_tests" in nextest)
PYRepository: leynos/netsuke
Length of output: 623
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
target = Path("tests/locale_stub_strictness_tests.rs").read_text()
nextest = Path(".config/nextest.toml").read_text()
print("target_tests:", re.findall(r'(?m)^\s*fn\s+([A-Za-z0-9_]+)\s*\(', target))
print("target_hook_operations:", len(re.findall(r'std::panic::(?:take_hook|set_hook)', target)))
print("target_uses_catch_unwind:", "catch_unwind" in target)
print("nextest_process_isolation_claim:", "nextest runs each test" in nextest and "own process" in nextest)
print("nextest_serial_group_binaries:", re.findall(r"binary\(([^)]+)\)", nextest))
print("locale_binary_in_serial_group:", "locale_stub_strictness_tests" in nextest)
PYRepository: leynos/netsuke
Length of output: 623
Remove the process-wide panic-hook replacement.
std::panic::set_hook affects all threads in the test process. A parallel panic can lose its diagnostics, and restoring prior can overwrite a concurrent hook change. Keep the default hook or run the probe in an isolated process.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/locale_stub_strictness_tests.rs` around lines 122 - 125, Remove the
process-wide panic-hook replacement around the catch_unwind probe in the
stub.var(key) test; eliminate the take_hook/set_hook/restore sequence while
preserving the existing panic capture and result assertions. Keep the default
hook active, or isolate the probe in a separate process if suppressing panic
output is required.
There was a problem hiding this comment.
Fixed in 6fa22ce, with a different remedy than suggested: rather than keeping the default hook (which buries genuine failures under 256 cases × 3 keys of panic output) or forking a process per probe (disproportionate for a property test), the hook is now installed exactly once as a wrapper that delegates to the prior hook unless a thread-local flag marks the current thread as inside a probe. That removes both hazards you identified: other threads' panics always reach the original hook (nothing is process-wide silenced), and nothing is ever restored, so there is no take/set window to race a concurrent hook change.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- current commit ---'
git rev-parse --short HEAD
printf '%s\n' '--- target test structure ---'
ast-grep outline tests/locale_stub_strictness_tests.rs --items all --view expanded || true
printf '%s\n' '--- relevant target section ---'
rg -n -C 12 'thread_local|take_hook|set_hook|catch_unwind|panic_hook|Hook' tests/locale_stub_strictness_tests.rs
printf '%s\n' '--- repository hook and synchronization patterns ---'
rg -n -C 5 --glob '*.rs' 'thread_local!|OnceLock|Once::|take_hook|set_hook|catch_unwind' tests test_support srcLength of output: 17798
@leynos, keep this finding unresolved.
6fa22ce still mutates the process-wide panic hook in install_gated_hook().
Once prevents repeated installation in this module. It does not coordinate with other tests that call std::panic::set_hook. A concurrent test can replace the gated hook after installation. This test can also replace a hook that another test installed.
The thread-local gate prevents suppression of panics from other threads after the wrapper is active. It does not remove global hook ownership or hook-installation conflicts.
Remove the hook replacement and retain the default hook. The existing project guidance in test_support/src/localizer.rs makes the same trade-off: accept deliberate-panic output rather than mutate the process-wide hook.
🧠 Learnings used
Learnt from: leynos
Repo: leynos/netsuke PR: 502
File: tests/locale_stub_strictness_tests.rs:13-13
Timestamp: 2026-08-04T13:12:33.692Z
Learning: In leynos/netsuke Rust code, do not require wrapping a bare `Option<T>` expression statement in `drop(...)`; `Option<T>` is not `#[must_use]` on the project toolchain, while `Result<T, E>` is. Expression statements still evaluate their calls, including calls in `#[should_panic]` tests. Use `drop(...)` only when the discarded value's type is actually `#[must_use]` and the value is intentionally ignored.
You are interacting with an AI system.
`StubEnv` answered `None` for every key but `NETSUKE_LOCALE`. Had the code under test been changed to read a differently-named variable — through a rename, a typo, or a new precedence rung — the stub would have quietly answered `None` and the test would still have passed, asserting nothing about the new read. That is the failure mode a test double exists to prevent. The stub now declares which variables it answers and panics on anything else, naming the unexpected key. Unset-but-expected is a distinct, declarable state, because an absent variable is a legitimate case to exercise and must be distinguishable from one the test never anticipated. `Default` is removed rather than retained. On a strict stub it would mean "deny every read", so `StubEnv::default()` would compile and then panic at run time for the common "no locale set" case; requiring `without_locale()` moves that to a compile error. One call site in `tests/locale_resolution_tests.rs` relied on it and is updated. This replaces the originally proposed convergence onto `mockable::Env`. That would have required moving `mockable` — and `mockall` with it — from dev-dependencies into the production dependency tree, which is a poor exchange for deleting a two-method trait. `locale_resolution`'s bespoke `EnvProvider` is a narrow seam of exactly the shape AGENTS.md permits and was never in violation. Closes #489. Refs #496. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses five review findings on #502. Codex (P2): `allowing` never removed an existing map entry, so `with_var("X", "set").allowing("X")` was documented as declaring X unset yet still answered `Some("set")`. Both builders now let the most recent declaration for a key win, and both orderings are covered. Codex (P1): nothing tested the panic. Relaxing or deleting the assertion would have left every other test passing while restoring the permissive behaviour this change exists to remove. Adds `tests/locale_stub_strictness_tests.rs` covering the panic, that it names the offending key, that declared reads do not panic, both builder orderings, and repeated declaration. Codex (P2): the `NETSUKE_JSON` allowance in the two BDD steps was speculative — neither helper calls `resolve_startup_json`. Keeping it would have let an accidental JSON read added to locale resolution pass silently, defeating the strictness. Removed; the suite still passes. Codex (P1): adds Rustdoc examples to `strict`, `with_locale`, `without_locale`, `with_var`, and `allowing`. CodeRabbit: replaces the escaped string continuations in the assertion message with `concat!()`, per AGENTS.md. Refs #489, #496. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses a CodeRabbit finding on #502.
Two commitments made on the review thread are now tests rather than prose. A property test states the invariant the fixed cases were instances of: over any sequence of with_var and allowing declarations, each key answers per its last declaration and undeclared keys panic, checked against an independent last-write-wins model so a bookkeeping slip between the stub's values and allowed collections cannot agree with itself. The mutation used to validate the property (disabling allowing's value clear) is recorded in the regression seed file. A compile-fail test proves StubEnv::default() does not compile. Trybuild cannot drive it: it removes ambient RUSTFLAGS and overrides workspace build.rustflags, so it would rebuild netsuke without -Zpolonius=next and reject the POLONIUS() sites (dtolnay/trybuild issues #315 and #333, both open). Instead the test_support rlib is built by Cargo, which inherits the ambient flags, and the fixtures are type-checked directly with rustc against that rlib. A control fixture using the sanctioned builders guards the wiring: were the --extern or -L dependency plumbing broken, the rejection would happen for the wrong reason and the compile-fail case would pass vacuously. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Round feedback on #502, both points taken. The undeclared-key probes now silence the default panic hook for exactly the catch_unwind call and restore it immediately: 256 cases times three keys of expected panic output was burying genuine failures. The two compile-fixture tests now draw the built rlib from a single #[once] rstest fixture rather than each invoking Cargo, which contended on the target lock and repeated finished work; the expectation on the fixture's expect is scoped to the one statement, since a once fixture cannot return Result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Post-rebase reconciliation: main renamed the locale trait to LocaleEnvProvider and the hardened lint suite now denies shadowed bindings, so the builder's key parameters rebind as name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
91fe043 to
efbc241
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/locale_stub_ui_tests.rs`:
- Around line 48-51: Update the error construction in the test rejection path to
use concat!() for the multi-line message, with an explicit {} placeholder and
stderr supplied as the format argument. Preserve the existing message content
and io::Error::other behavior.
🪄 Autofix
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: c5fa284f-830d-45e9-8c10-bcb7e3aed6cc
📒 Files selected for processing (9)
test_support/src/locale_stubs.rstests/bdd/steps/cli.rstests/bdd/steps/locale_resolution.rstests/locale_resolution_tests.rstests/locale_stub_strictness_tests.proptest-regressionstests/locale_stub_strictness_tests.rstests/locale_stub_ui_tests.rstests/ui/stub_env_default_compile_fail.rstests/ui/stub_env_strict_compile_pass.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected)
Round feedback on #502, all three points taken. The undeclared-key probes' hook swap is serialized behind a lock: nextest isolates each test in a process, but the in-process runner used for coverage runs tests as threads, and an unsynchronized take/set pair could strand one thread's no-op hook as another's restored state. The UI harness message uses concat! with an explicit placeholder rather than an escaped continuation. The developers' guide gains a StubEnv strictness section covering the trichotomy, last-declaration-wins, and the compile-time refusal of Default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
The probe silenced the default hook by taking it, installing a no-op, and restoring the original afterwards. Serializing that swap behind a lock kept the probes from stranding each other's hook, but the swap is still process-wide: under the threaded in-process coverage runner a concurrent test's panic during the window is silenced, and the restore can overwrite a hook installed by someone else in the meantime. Install a wrapper exactly once instead, delegating to the prior hook unless a thread-local flag marks the current thread as inside a probe. Other threads' panics always reach the original hook, and nothing is ever restored, so there is no window and no race. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Both warning rows are addressed on the pushed head:
A fresh review round is queued against this head. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files relevant to the warnings ==='
git diff --name-status origin/main...HEAD | rg '(^|/)(developers-guide\.md|locale_stubs\.rs|locale_stub_strictness_tests\.rs|locale_stub_ui_tests\.rs)$' || true
echo
echo '=== Documentation section locations ==='
rg -n -C 4 'StubEnv|strict|declared|undeclared|last.declaration|Default' docs/developers-guide.md || true
echo
echo '=== Rust file outlines ==='
ast-grep outline test_support/src/locale_stubs.rs --items all --view expanded || true
ast-grep outline tests/locale_stub_strictness_tests.rs --items all --view expanded || true
echo
echo '=== Panic-hook and synchronization-related code ==='
rg -n -C 8 'set_hook|take_hook|panic::|thread_local|thread_local!|Mutex|OnceLock|PROBE|probe' \
tests/locale_stub_strictness_tests.rs tests/locale_stub_ui_tests.rs test_support/src/locale_stubs.rs || true
echo
echo '=== Focused strictness test source ==='
cat -n tests/locale_stub_strictness_tests.rs
echo
echo '=== Focused developer guide excerpt ==='
line=$(rg -n -m1 'StubEnv' docs/developers-guide.md | cut -d: -f1 || true)
if [ -n "$line" ]; then
start=$(( line > 12 ? line - 12 : 1 ))
end=$(( line + 100 ))
sed -n "${start},${end}p" docs/developers-guide.md | nl -ba -v "$start"
fiLength of output: 24199
Do not create a follow-up issue. Do not perform further work for these two warnings. You are interacting with an AI system. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3 participants
Summary
test_support::locale_stubs::StubEnvansweredNonefor every key but one:Had the code under test been changed to read a differently-named variable — a rename, a typo, or a new precedence rung — the stub would have quietly answered
Noneand the test would still have passed, asserting nothing whatever about the new read. That is precisely the failure mode a test double exists to prevent.The stub now declares which variables it answers and panics on anything else, naming the unexpected key.
Design points
Unset is a declarable state.
without_locale()permits the read and reports it unset, distinct from a variable the test never anticipated. An absent variable is a legitimate case to exercise, and conflating the two would make the stub unusable for it.Defaultis removed, not retained. On a strict stub it would mean "deny every read", soStubEnv::default()would compile and then panic at run time for the common "no locale set" case. Requiringwithout_locale()turns that into a compile error. One call site intests/locale_resolution_tests.rsrelied on it — and did indeed fail at run time before this change, which is how the trap was found.Why not
mockable::EnvThis issue originally proposed converging
locale_resolution::EnvProviderontomockable::Env. On attempting it, that is the wrong trade.mockablesits in[dev-dependencies]and reaches the crate only throughtest_support.locale_resolutionis production code with a public API, so re-exportingmockable::Envfrom it requires movingmockableinto[dependencies]— andcargo treeshows it pullingmockall:Shipping a mocking framework in release builds of a build tool, to delete a two-method trait, is not a good exchange. The bespoke
EnvProviderexists precisely so that does not happen — a sound reason, not the duplication the issue characterised it as. The AGENTS.md mandate already permits this shape, solocale_resolutionwas never in violation.The one thing
MockEnv's expectations would genuinely have bought is strictness about unexpected reads. This PR takes that without the dependency.Verification
All gates pass:
check-fmt,lint,typecheck,test(1188 nextest),markdownlint,nixie. CodeScene delta: no issues.Closes #489.
Refs #496.
🤖 Generated with Claude Code
Summary by Sourcery
Tighten the locale environment test stub so tests explicitly declare expected environment variable reads and fail on unexpected ones.
New Features:
StubEnv::with_var,StubEnv::with_locale,StubEnv::without_locale, andStubEnv::allowing.Enhancements:
StubEnva strict stub that panics when code under test reads undeclared environment variables, improving test fidelity and catching unintended variable usage.Defaultimplementation fromStubEnvto prevent implicit, runtime-failing configurations and require explicit setup of the stub state.Tests:
StubEnvAPI and to explicitly declareNETSUKE_JSONas a legitimately readable-but-unset variable.