Derive version-sensitive test contracts from the crate version - #532
Merged
Conversation
The v0.1.0-beta1 bump broke the `diagnostic_json` snapshots, which baked the generator version into the stored JSON, and exposed two hardcoded literals in the documentation contract: - The PowerShell help-directory fragment asserted `Netsuke\0.1.0` while the docs correctly moved to `Netsuke\0.1.0-beta1`. - The release-tag assertion was a `contains` check on `releases/tag/v0.1.0`, which only kept passing because `v0.1.0` is a prefix of `v0.1.0-beta1` — it would have silently asserted the wrong release forever. Redact the generator version in the snapshots with an insta filter (enabling the `filters` feature), and derive both documentation contract literals from `CARGO_PKG_VERSION` so `Cargo.toml` remains the single source of truth and future bumps cannot reproduce the drift. Also teach `test_support`'s binary locator to fall back to `CARGO_TARGET_DIR` when Cargo's `build.build-dir` splits intermediate artefacts from final ones: test executables then run from the build dir while the uplifted `netsuke` binary is placed under the target dir, so the current-exe-derived path alone cannot find it.
Contributor
|
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:
Summary
Formatting, linting, workspace tests, and doctests pass. WalkthroughStabilize diagnostic JSON snapshots, add fallback lookup for test executables, document the lookup behaviour, and replace fixed release versions in documentation tests with ChangesTest stability
Possibly related PRs
Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideUpdates tests and support utilities to make version-dependent contracts derive from the crate version and to improve robustness of binary discovery in versioned builds, including redacting generator versions from JSON snapshots and handling split build/target directories. Flow diagram for netsuke binary discovery with CARGO_TARGET_DIR fallbackflowchart TD
A[test_binary_launcher] --> B["netsuke_binary_path(env)"]
B --> C[compute_build_dir_path]
C --> D{binary_exists_at_build_dir}
D -- yes --> E[return_build_dir_path]
D -- no --> F{CARGO_TARGET_DIR_set}
F -- no --> G[return_build_dir_path]
F -- yes --> H[compute_target_dir_path]
H --> I{binary_exists_at_target_dir}
I -- yes --> J[return_target_dir_path]
I -- no --> K[return_build_dir_path]
File-Level Changes
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.
leynos
marked this pull request as ready for review
August 5, 2026 23:18
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Narrow the diagnostic-JSON snapshot filter so it anchors on the generator block's `"name": "netsuke"` line: only the generator's version is redacted, and any other `version` field that later appears in a diagnostic document stays visible in snapshot diffs. Rework the `test_support` binary locator for injectability and robustness: - Split the lookup into `netsuke_executable_from(env, current_exe)` with an injected `mockable::Env`, mirroring the `compile_rust_helper_with_env` pattern, so the fallback logic is unit-testable with `MockEnv`. - Use `camino::Utf8PathBuf` throughout, converting at the `current_exe()` boundary, consistent with the rest of the crate. - Add a third candidate, `CARGO_TARGET_DIR/<triple>/<profile>/`, so `--target` builds resolve; the reviewer's suggestion to insert the triple unconditionally would have broken the ordinary no-`--target` layout, so the triple path is an additional candidate instead. - Surface filesystem errors other than not-found through a new `test_support::fs::try_is_file` wrapper (the crate's sanctioned ambient boundary under the Whitaker `no_std_fs_operations` policy), and list every attempted candidate when the binary is missing. Add unit tests covering the primary, profile-fallback, triple-fallback, and missing-binary paths, and document the locator and the snapshot-redaction policy in the developers' guide and the insta guide.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on lines +196 to +210 fn locates_binary_beside_the_test_executable() -> Result<()> {
let temp = tempfile::tempdir().context("create temp dir")?;
let root = utf8_root(&temp)?;
let exe = root.join("build/debug/deps/test-exe");
touch(&exe)?;
let binary = root.join("build/debug").join(binary_name());
touch(&binary)?;
let located = netsuke_executable_from(&env_with_target_dir(None), &exe)?;
ensure!(
located == binary,
"primary lookup should win; got {located}"
);
Ok(())
}❌ New issue: Code Duplication |
Owner
Author
|
@coderabbitai resume |
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
✅ Action performedReviews resumed. |
The three happy-path locator tests repeated the same stage-and-assert choreography. Extract an `assert_locates` helper that stages the scenario from root-relative paths (test executable, optional `CARGO_TARGET_DIR`, expected binary) and asserts the resolved path, keeping each test to a single scenario-specific call. Coverage, lookup order, and the missing-candidate test are unchanged.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Address the third review round: - Assert every candidate the locator generates in the missing-binary test, including the `CARGO_TARGET_DIR/<triple>/<profile>/` path. - Add a both-present priority test so inverting the candidate order can no longer pass unnoticed. - Cover `test_support::fs::try_is_file` directly: a directory reports `Ok(false)`, and traversal through a regular file propagates the error rather than being folded into absence. - Pass the located `Utf8PathBuf` straight to `assert_cmd::Command::new`, completing the camino migration. - Split the installation and release-contract tests (149 lines with no entanglement) into `tests/documentation_installation_tests.rs`, bringing `documentation_examples_tests.rs` from 505 lines to 355, under the repository's 400-line file cap. The new binary reuses the shared `documentation_examples` directory module via the same `pub mod` pattern as the loader tests; behaviour is unchanged. - Document `try_is_file` in the developers' guide under `test_support::fs`, with a cross-reference from the binary-locator section.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat o11y, code safety, documentation and validation coverage as in scope). ❌ Failed checks (1 error, 1 warning)
|
Owner
Author
|
@coderabbitai please create a new GitHub issue for the following: Testing (Property / Proof) |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- current branch and changed files ---'
git status --short
git diff --name-only "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || true
printf '%s\n' '--- source outlines ---'
ast-grep outline test_support/src/netsuke.rs --items all --view expanded
ast-grep outline test_support/src/fs.rs --items all --view expanded
printf '%s\n' '--- locator implementation and tests ---'
rg -n -A45 -B12 \
'fn (netsuke_executable_from|locates_binary_beside_the_test_executable|falls_back_to_cargo_target_dir_profile|falls_back_to_target_triple_directory|reports_all_attempted_paths_when_binary_is_missing|assert_locates_binary)\s*\(' \
test_support/src/netsuke.rs
printf '%s\n' '--- try_is_file implementation and tests ---'
rg -n -A45 -B12 \
'fn (try_is_file|.*try_is_file.*)\s*\(' \
test_support/src/fs.rs
printf '%s\n' '--- developer guide references ---'
rg -n -i -A12 -B6 'try_is_file|test_support::fs|netsuke_executable' docs/developers-guide.md
printf '%s\n' '--- open issues potentially covering these checks ---'
gh issue list --repo leynos/netsuke --state open --limit 100 \
--search 'try_is_file OR locator OR CARGO_TARGET_DIR OR binary lookup' \
--json number,title,url,body \
--jq '.[] | {number,title,url,body}'Length of output: 40310
Both failed checks are resolved in the current codegraph.
No remaining work is required for these failures. No AI coding-agent prompt or follow-up GitHub issue is required. This validation inspected the current source graph. The sandbox does not support running the Rust test commands. You are interacting with an AI system. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/netsuke"
printf '%s\n' '--- potentially matching open issues ---'
gh issue list --repo "$repo" --state open --limit 100 \
--search '"property-based" OR proptest OR "candidate_paths" OR "snapshot filtering"' \
--json number,title,url,body \
--jq '.[] | {number,title,url,body}'
printf '%s\n' '--- originating PR comment ---'
gh api "repos/$repo/issues/532/comments?per_page=100" --paginate \
--jq '.[] | select(.body | contains("Testing (Property / Proof)")) | {url: .html_url, author: .user.login, body: .body}'Length of output: 30064 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="leynos/netsuke"
requester="leynos"
body=$(cat <<'EOF'
## Summary
Add property-based coverage for executable candidate resolution in `test_support/src/netsuke.rs`. Validate the selective diagnostic JSON snapshot filter in `src/diagnostic_json_tests.rs`, or document why finite example coverage is sufficient.
## Rationale
`candidate_paths` combines optional `CARGO_TARGET_DIR` values, profiles, target triples, and ordered candidates. The current example tests verify selected layouts but do not prove the ordering and candidate-generation invariants across varied valid UTF-8 path components and candidate-presence combinations.
The snapshot filter modifies generated JSON before comparison. A focused test must show that it redacts only the Netsuke generator version and preserves unrelated version fields. If existing finite tests already establish that invariant completely, document the finite-domain rationale in the test or developer documentation.
## Required work
- Add substantive `proptest` coverage for `candidate_paths` and/or `netsuke_executable_from` in `test_support/src/netsuke.rs`.
- Generate arbitrary valid UTF-8 roots, profiles, and target triples.
- Cover an absent and present `CARGO_TARGET_DIR`.
- Cover candidate-presence combinations and assert the first existing candidate wins according to the documented lookup order.
- Assert that the generated candidate list has the intended paths and order for every generated input.
- Add a focused test for selective diagnostic JSON snapshot filtering in `src/diagnostic_json_tests.rs`.
- Alternatively, document and justify why finite coverage fully proves the snapshot-filter behaviour.
## Acceptance criteria
- The test suite contains `proptest` coverage of candidate generation and lookup ordering.
- Tests cover primary, profile-fallback, and target-triple-fallback candidate selection under generated inputs.
- Tests cover missing candidates and preserve diagnostics that list all attempted candidates.
- A focused test proves that only the Netsuke generator version is redacted, while unrelated `version` fields remain unchanged; or the codebase contains a clear finite-coverage justification.
- Run formatting, linting, focused tests, workspace tests, and doctests.
## Backlinks
- PR: https://github.com/leynos/netsuke/pull/532
- Request comment: https://github.com/leynos/netsuke/pull/532#issuecomment-5198956465
- Requested by: `@leynos`
EOF
)
gh issue create \
--repo "$repo" \
--title "Add property-based coverage for executable candidate resolution" \
--body "$body" \
--assignee "$requester"Length of output: 2838
The issue includes the requested You are interacting with an AI system. |
lodyai Bot
pushed a commit
that referenced
this pull request
Aug 6, 2026
Work through the code-review comments left on #515 after merge, verifying each against the current tree before changing anything. Correctness and isolation: - Compile IR from BDD manifests through `from_path_with_policy_and_env` with the scenario environment reader, matching the manifest steps instead of reading host variables. - Wrap both `tests/cli_tests/merge.rs` merge sites in `isolated_environment` so host XDG configuration cannot perturb the precedence assertions. - Forward `LD_LIBRARY_PATH` and `DYLD_FALLBACK_LIBRARY_PATH` through the merge-probe allowlist; Cargo supplies them and the re-executed worker cannot start without them. - Give the project-scope discovery test its own home directory so it can only pass through project-scope discovery. - Assert the `UndefinedError` cause before downcasting in the missing-environment-variable manifest test, so the check applies whatever the outer error type. - Strengthen the explicit-config assertion to require the stable diagnostic alongside the missing path. - Parse `--package` selections into arguments in the Whitaker boundary contract, rejecting extra flags and prefixed names. - Validate the workflow YAML root structurally in `_load`, restoring the `on:` key that PyYAML coerces to a boolean. - Fail loudly on a non-UTF-8 ninja override instead of converting lossily, and name the offending path in the packaging assertion. - Clear the `ENV_LOCK` poison flag before asserting, so a failing assertion cannot leak process-global poisoning. Structure and documentation: - Replace the `cfg`-gated bare `return` in `EnvSnapshot::capture_with_env` with per-platform wrappers. - Keep the compiler-wrapper marker sidecar in camino's UTF-8 domain. - Brought `documentation_examples_tests.rs` back under the 400-line limit by moving the installation, release, and Windows setup contracts into their own test binary. Main reached the same split independently in #532, so this branch adopts `tests/documentation_installation_tests.rs` from there rather than carrying a duplicate; its version literals derive from `CARGO_PKG_VERSION`. - Fix `initialise`/`initialisation` to Oxford spelling in the macro initialization message and the fetch-cache Rustdoc, updating the inline snapshot in lockstep. - Document the native-path contract for the injected Ninja executable, and fill the Rustdoc gaps in `fake_ninja_check_build_file_in`, `real_utility_with_env`, `stdlib_output_or_error`, and the non-Unix `make_executable`. One comment is not actioned: the `run_netsuke_in` environment-contract comment already matches the implementation. That helper does not call `env_clear`, so the suggested wording describes `run_netsuke_in_with_env` and would have made the comment wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lodyai Bot
pushed a commit
that referenced
this pull request
Aug 6, 2026
Work through the code-review comments left on #515 after merge, verifying each against the current tree before changing anything. Correctness and isolation: - Compile IR from BDD manifests through `from_path_with_policy_and_env` with the scenario environment reader, matching the manifest steps instead of reading host variables. - Wrap both `tests/cli_tests/merge.rs` merge sites in `isolated_environment` so host XDG configuration cannot perturb the precedence assertions. - Forward `LD_LIBRARY_PATH` and `DYLD_FALLBACK_LIBRARY_PATH` through the merge-probe allowlist; Cargo supplies them and the re-executed worker cannot start without them. - Give the project-scope discovery test its own home directory so it can only pass through project-scope discovery. - Assert the `UndefinedError` cause before downcasting in the missing-environment-variable manifest test, so the check applies whatever the outer error type. - Strengthen the explicit-config assertion to require the stable diagnostic alongside the missing path. - Parse `--package` selections into arguments in the Whitaker boundary contract, rejecting extra flags and prefixed names. - Validate the workflow YAML root structurally in `_load`, restoring the `on:` key that PyYAML coerces to a boolean. - Fail loudly on a non-UTF-8 ninja override instead of converting lossily, and name the offending path in the packaging assertion. - Clear the `ENV_LOCK` poison flag before asserting, so a failing assertion cannot leak process-global poisoning. Structure and documentation: - Replace the `cfg`-gated bare `return` in `EnvSnapshot::capture_with_env` with per-platform wrappers. - Keep the compiler-wrapper marker sidecar in camino's UTF-8 domain. - Brought `documentation_examples_tests.rs` back under the 400-line limit by moving the installation, release, and Windows setup contracts into their own test binary. Main reached the same split independently in #532, so this branch adopts `tests/documentation_installation_tests.rs` from there rather than carrying a duplicate; its version literals derive from `CARGO_PKG_VERSION`. - Fix `initialise`/`initialisation` to Oxford spelling in the macro initialization message and the fetch-cache Rustdoc, updating the inline snapshot in lockstep. - Document the native-path contract for the injected Ninja executable, and fill the Rustdoc gaps in `fake_ninja_check_build_file_in`, `real_utility_with_env`, `stdlib_output_or_error`, and the non-Unix `make_executable`. One comment is not actioned: the `run_netsuke_in` environment-contract comment already matches the implementation. That helper does not call `env_clear`, so the suggested wording describes `run_netsuke_in_with_env` and would have made the comment wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lodyai Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
Work through the code-review comments left on #515 after merge, verifying each against the current tree before changing anything. Correctness and isolation: - Compile IR from BDD manifests through `from_path_with_policy_and_env` with the scenario environment reader, matching the manifest steps instead of reading host variables. - Wrap both `tests/cli_tests/merge.rs` merge sites in `isolated_environment` so host XDG configuration cannot perturb the precedence assertions. - Forward `LD_LIBRARY_PATH` and `DYLD_FALLBACK_LIBRARY_PATH` through the merge-probe allowlist; Cargo supplies them and the re-executed worker cannot start without them. - Give the project-scope discovery test its own home directory so it can only pass through project-scope discovery. - Assert the `UndefinedError` cause before downcasting in the missing-environment-variable manifest test, so the check applies whatever the outer error type. - Strengthen the explicit-config assertion to require the stable diagnostic alongside the missing path. - Parse `--package` selections into arguments in the Whitaker boundary contract, rejecting extra flags and prefixed names. - Validate the workflow YAML root structurally in `_load`, restoring the `on:` key that PyYAML coerces to a boolean. - Fail loudly on a non-UTF-8 ninja override instead of converting lossily, and name the offending path in the packaging assertion. - Clear the `ENV_LOCK` poison flag before asserting, so a failing assertion cannot leak process-global poisoning. Structure and documentation: - Replace the `cfg`-gated bare `return` in `EnvSnapshot::capture_with_env` with per-platform wrappers. - Keep the compiler-wrapper marker sidecar in camino's UTF-8 domain. - Brought `documentation_examples_tests.rs` back under the 400-line limit by moving the installation, release, and Windows setup contracts into their own test binary. Main reached the same split independently in #532, so this branch adopts `tests/documentation_installation_tests.rs` from there rather than carrying a duplicate; its version literals derive from `CARGO_PKG_VERSION`. - Fix `initialise`/`initialisation` to Oxford spelling in the macro initialization message and the fetch-cache Rustdoc, updating the inline snapshot in lockstep. - Document the native-path contract for the injected Ninja executable, and fill the Rustdoc gaps in `fake_ninja_check_build_file_in`, `real_utility_with_env`, `stdlib_output_or_error`, and the non-Unix `make_executable`. One comment is not actioned: the `run_netsuke_in` environment-contract comment already matches the implementation. That helper does not call `env_clear`, so the suggested wording describes `run_netsuke_in_with_env` and would have made the comment wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lodyai Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
Work through the code-review comments left on #515 after merge, verifying each against the current tree before changing anything. Correctness and isolation: - Compile IR from BDD manifests through `from_path_with_policy_and_env` with the scenario environment reader, matching the manifest steps instead of reading host variables. - Wrap both `tests/cli_tests/merge.rs` merge sites in `isolated_environment` so host XDG configuration cannot perturb the precedence assertions. - Forward `LD_LIBRARY_PATH` and `DYLD_FALLBACK_LIBRARY_PATH` through the merge-probe allowlist; Cargo supplies them and the re-executed worker cannot start without them. - Give the project-scope discovery test its own home directory so it can only pass through project-scope discovery. - Assert the `UndefinedError` cause before downcasting in the missing-environment-variable manifest test, so the check applies whatever the outer error type. - Strengthen the explicit-config assertion to require the stable diagnostic alongside the missing path. - Parse `--package` selections into arguments in the Whitaker boundary contract, rejecting extra flags and prefixed names. - Validate the workflow YAML root structurally in `_load`, restoring the `on:` key that PyYAML coerces to a boolean. - Fail loudly on a non-UTF-8 ninja override instead of converting lossily, and name the offending path in the packaging assertion. - Clear the `ENV_LOCK` poison flag before asserting, so a failing assertion cannot leak process-global poisoning. Structure and documentation: - Replace the `cfg`-gated bare `return` in `EnvSnapshot::capture_with_env` with per-platform wrappers. - Keep the compiler-wrapper marker sidecar in camino's UTF-8 domain. - Brought `documentation_examples_tests.rs` back under the 400-line limit by moving the installation, release, and Windows setup contracts into their own test binary. Main reached the same split independently in #532, so this branch adopts `tests/documentation_installation_tests.rs` from there rather than carrying a duplicate; its version literals derive from `CARGO_PKG_VERSION`. - Fix `initialise`/`initialisation` to Oxford spelling in the macro initialization message and the fetch-cache Rustdoc, updating the inline snapshot in lockstep. - Document the native-path contract for the injected Ninja executable, and fill the Rustdoc gaps in `fake_ninja_check_build_file_in`, `real_utility_with_env`, `stdlib_output_or_error`, and the non-Unix `make_executable`. One comment is not actioned: the `run_netsuke_in` environment-contract comment already matches the implementation. That helper does not call `env_clear`, so the suggested wording describes `run_netsuke_in_with_env` and would have made the comment wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
leynos
added a commit
that referenced
this pull request
Aug 7, 2026
* Address PR #515 review follow-ups Work through the code-review comments left on #515 after merge, verifying each against the current tree before changing anything. Correctness and isolation: - Compile IR from BDD manifests through `from_path_with_policy_and_env` with the scenario environment reader, matching the manifest steps instead of reading host variables. - Wrap both `tests/cli_tests/merge.rs` merge sites in `isolated_environment` so host XDG configuration cannot perturb the precedence assertions. - Forward `LD_LIBRARY_PATH` and `DYLD_FALLBACK_LIBRARY_PATH` through the merge-probe allowlist; Cargo supplies them and the re-executed worker cannot start without them. - Give the project-scope discovery test its own home directory so it can only pass through project-scope discovery. - Assert the `UndefinedError` cause before downcasting in the missing-environment-variable manifest test, so the check applies whatever the outer error type. - Strengthen the explicit-config assertion to require the stable diagnostic alongside the missing path. - Parse `--package` selections into arguments in the Whitaker boundary contract, rejecting extra flags and prefixed names. - Validate the workflow YAML root structurally in `_load`, restoring the `on:` key that PyYAML coerces to a boolean. - Fail loudly on a non-UTF-8 ninja override instead of converting lossily, and name the offending path in the packaging assertion. - Clear the `ENV_LOCK` poison flag before asserting, so a failing assertion cannot leak process-global poisoning. Structure and documentation: - Replace the `cfg`-gated bare `return` in `EnvSnapshot::capture_with_env` with per-platform wrappers. - Keep the compiler-wrapper marker sidecar in camino's UTF-8 domain. - Brought `documentation_examples_tests.rs` back under the 400-line limit by moving the installation, release, and Windows setup contracts into their own test binary. Main reached the same split independently in #532, so this branch adopts `tests/documentation_installation_tests.rs` from there rather than carrying a duplicate; its version literals derive from `CARGO_PKG_VERSION`. - Fix `initialise`/`initialisation` to Oxford spelling in the macro initialization message and the fetch-cache Rustdoc, updating the inline snapshot in lockstep. - Document the native-path contract for the injected Ninja executable, and fill the Rustdoc gaps in `fake_ninja_check_build_file_in`, `real_utility_with_env`, `stdlib_output_or_error`, and the non-Unix `make_executable`. One comment is not actioned: the `run_netsuke_in` environment-contract comment already matches the implementation. That helper does not call `env_clear`, so the suggested wording describes `run_netsuke_in_with_env` and would have made the comment wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Separate manifest telemetry from macro evaluation Address the second review round on #515. Six of the ten findings had already been fixed or never held; the four below were verified against the current tree first. Separate observability from evaluation. `make_macro_fn` created a span, timed with `Instant::now()`, and emitted metrics inside the Jinja callback, mixing instrumentation into what is a query. `render_template` did the same at the template boundary. Both now compose a pure evaluation closure with an instrumentation wrapper in a new `jinja_macros::telemetry` module, which also gives the emitted fields' privacy contract a single place to be reviewed. Cover the macro-invocation boundary with tests. Its counter, histogram, and span appeared only in production code, so deleting them left the suite green. `macro_invocation_telemetry` now pins the success and failure paths and asserts the failure event carries the bounded error category without the macro's name or arguments. Confirmed by deleting the metrics and watching both tests fail. Redact the fixture duration value in `test_support`'s HTTP server. An unparsable override was logged verbatim, putting caller-controlled environment content into the log. The warning now carries the variable name, the bounded parse error, and the value's byte length; the tests assert on that category and additionally assert the value does not appear. An empty-value case joins the table, which the length now distinguishes. Correct two stale developer-guide entries: the executable helpers take `&Path`/`PathBuf` and there is no `exec::utf8_path` conversion point, and `push_file_layers` is really `push_file_layers_with_env`. Not actioned, with reasons: - Imported-macro renders were said to lack instrumentation. They do not: `render_template` has always had its own span and metrics, tested in `macros_telemetry`. A regression test now pins the boundary split. - `env_reader` was said to leak manifest-controlled variable names. It does not; the diagnostics are fixed text and a test asserts the name is absent, which is the documented decision. - The `merge_probe` clone already uses `merged.command.take()`. - `EnvProvider` was said to be renamed to `LocaleEnvProvider`. They are unrelated traits that both still exist, and the distinction is already documented in the developers' guide. - The claimed module cycles do not exist: `ENV_PREFIX` lives in `cli::constants` and `call_macro_value` in the sibling `call` module, which is where the finding asked for them. - Proptests for keyword forwarding and `EnvLock` acquire/drop interleavings already exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Harden telemetry redaction with property coverage Address the third review round on #515. Four findings held; one was half right, and the reasons for each are below. Stop the poisoning-recovery test leaking poison on failure. `env_lock_recovers_after_mutex_poisoning` asserted the held state while `recovered_guard` was still live. Had that assertion failed, unwinding would have dropped the guard mid-panic and poisoned `ENV_LOCK` afresh — the very leak the test's own doc comment claimed to have covered, since clearing the flag earlier only guards the deliberate poisoning. The state is now read through a non-asserting `current_thread_lock_is_held`, the guard dropped, and only then asserted; the doc comment no longer overclaims. Add property coverage for the redaction invariants. Both were pinned only by fixed sentinels, so they held for the sentinel and said nothing about the range: - `macro_telemetry_stays_bounded_for_arbitrary_macros` generates macro names, arguments, and undefined-variable names, then asserts the outcome labels stay within `{success, error}` and that none of the generated identifiers reach a tracing event. Verified by making the failure event log the full `MiniJinja` error instead of its kind: the test fails, naming the leaked macro. - `invalid_duration_warnings_are_composed_only_of_bounded_parts` generates unparsable overrides and asserts the rendered warning equals a message rebuilt from bounded parts alone. Exact match rather than a `!contains` check, which a value of `bytes` would satisfy while sitting in plain sight. Pin the redacted value's byte length in the duration table via a new `expected_warning_len`, measured after trimming to match the call site. It is the one piece of shape the warning still surfaces, so it should not go missing — or revert to the value — unnoticed. Parse the workflow with a YAML 1.2 boolean resolver. Mapping `True` back to `"on"` conflated GitHub Actions' `on:` with a literal `yes:` or `true:` key; under YAML 1.1 those three collapse into a single key, silently dropping entries. Narrowing the resolver to `true`/`false` leaves `on` a string, so the normalization disappears rather than being patched. Use `camino::Utf8Path` in the packaging forbidden-root check, per the project's stated preference over `std::path`. The finding also asked for an exact root match to keep names such as `test_support-extra` allowed; that already held, because comparing whole path components is exact already — only the type changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Cover IR environment injection and document telemetry Address the fourth review round on #515. All three findings held. Finish the job started last round in `env_lock`. Capturing the lock state before asserting fixed only the poisoning-recovery test; the two reentrant tests still called `assert_current_thread_lock_is_held` with guards live, so a failure there would drop a guard mid-unwind and poison `ENV_LOCK`, burying the real cause. Both now capture their observations, release the guards, then assert, preserving the original messages. The helper had no remaining callers and is gone; `current_thread_lock_is_held` stays. Test the IR environment injection. Compiling a manifest to IR was switched to `from_path_with_policy_and_env`, but no scenario combined a forwarded variable with `compiled to IR`, so reverting to `from_path` left the suite green. A new fixture derives both a target name and an input path from `env(...)`, and two scenarios pin the outcome: one asserts the resulting graph edge, the other that generation fails when the variable is unset. Confirmed by reverting the step to `from_path` — the first scenario fails because the graph never builds. The value had to travel through a target name rather than a command: `BuildEdge` carries no recipe, so `ir.rs` exposes no step that can assert command text. Cover the non-UTF-8 `NETSUKE_NINJA` branch too, Unix-only since building such a path needs POSIX byte semantics. Document the telemetry boundary. A new module arrived with a published metric contract and a redaction policy but no record of either. ADR-008 records the decision to keep observability out of manifest evaluation and telemetry bounded and redacted, with the alternatives; the developers' guide now describes both instrumentation boundaries, the label vocabulary, the redaction rule, and where the contract is tested. The design document and documentation index reference the ADR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Correct the poisoning test's safety note Address the fifth review round on #515. Both findings were prose. The comment above `env_lock_recovers_after_mutex_poisoning` claimed every assertion runs with the poison flag already cleared. That is wrong for the first one: the `join` assertion precedes `ENV_LOCK.clear_poison()`, and at that point the mutex is still poisoned — deliberately, since that is the state under test. It is nonetheless safe, because the poisoned guard belonged to the spawned thread and this thread holds none, so a failure has no live guard to drop. The note now separates the two cases and keeps the explanation of why the held state is captured before its guard is dropped. Drop the comma before "because" in the two ADR sentences named by the review. The second needed more than deleting the comma: "covers the compiled-expression fallback only because imports evaluate" reads as "merely because", so "only" moves ahead of the noun phrase to keep the meaning that the counter covers that path alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Keep the metric registry out of the render path `render_template` called `describe_render_metrics()` on every render, so the query function reached for the global metric registry itself. The call moves into `instrument_template_render`, behind the same one-time guard, leaving rendering to name only the instrumentation boundary it composes with. The macro-side registration stays where it is: it runs when a macro is registered, which is setup rather than evaluation. This is a partial response to a review finding asking for a clock and telemetry sink to be injected instead. That part is not actioned; see ADR-008, which records the decision and the reasoning, and the pull request discussion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Realign the telemetry guide with the render path Moving `describe_render_metrics` into `instrument_template_render` left the developers' guide asserting it "runs at the top of every `render_template` call". Describe where the registration actually happens, and why neither description call sits in a query function. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Gate the Unix-only bail import `bail!` is reached only from `non_utf8_ninja_override_is_rejected`, which is `#[cfg(unix)]` because building a non-UTF-8 path needs POSIX byte semantics. The import was unconditional, so on any other target it became an unused import — and the workspace builds with `-D warnings`, making that a hard error rather than a warning. Gate it alongside the `PathBuf` import that the same test needs. Verified rather than assumed: the pre-fix shape (unconditional import whose sole user is gated out) fails `rustc -D warnings` with `error: unused import`, and the gated shape compiles clean. A direct Windows cross-check was not possible here because a build dependency needs MSVC's `lib.exe`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: leynos <leynos@rohga> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
This branch repairs the test failures introduced by the v0.1.0-beta1
version bump and removes the version literals that caused them, so that
future bumps cannot break the suite again.
The bump surfaced three distinct problems:
diagnostic_jsoninsta snapshots baked the generator version intothe stored JSON, so any version change failed them.
Netsuke\0.1.0for thePowerShell help directory while the documentation had correctly moved
to
Netsuke\0.1.0-beta1.containscheck onreleases/tag/v0.1.0, which only kept passing becausev0.1.0is aprefix of
v0.1.0-beta1; it would have gone green on the bump andthen silently asserted the wrong release indefinitely.
The generator version is now redacted from the snapshots with an insta
filter, and both documentation contract literals are derived from
CARGO_PKG_VERSION, keepingCargo.tomlas the single source of truth.The branch also includes the pending
Cargo.lockbump commit andteaches the
test_supportbinary locator to fall back toCARGO_TARGET_DIRwhen Cargo'sbuild.build-dirsplits intermediateartefacts from final ones (the test executable then runs from the build
directory while the uplifted
netsukebinary lands in the targetdirectory).
Review walkthrough
CARGO_PKG_VERSION.CARGO_TARGET_DIRfallback in the binary locator, which uses the injected-environment pattern required by the workspace lint configuration.filtersfeature.Validation
make check-fmt: passmake lint(Clippy + Whitaker, warnings denied): passmake test(nextest full workspace + doctests): passNotes
The hardcoded
ModuleVersion = "0.1.0"in the release-help test fixture(tests/release_help/mod.rs) is a self-contained synthetic fixture rather than a copy of the crate version, so it is left unchanged.
Summary by Sourcery
Align tests and diagnostics with crate version and improve test binary discovery.
Bug Fixes:
Enhancements:
Build:
Tests: