Address PR #515 review follow-ups

[leynos]

Summary

Works through the code-review comments left on #515 (Harden workspace lint
configuration) after it merged. Every finding was verified against the current
tree before any change; one was found to be stale and is left alone with a
reason below.

Correctness and isolation

• BDD IR steps read the scenario environment. compile_manifest_impl called
  manifest::from_path, bypassing the injected environment, so IR scenarios read
  host variables while the manifest scenarios read scenario values. It now uses
  from_path_with_policy_and_env with the same manifest_env_reader seam.
• tests/cli_tests/merge.rs now sandboxes both merge sites. with_config_file
  and the precedence test called merge_in_child directly, letting host XDG
  configuration reach the child. Both now go through isolated_environment.
• Merge-probe allowlist forwards the dynamic-loader paths. LD_LIBRARY_PATH
  and DYLD_FALLBACK_LIBRARY_PATH are Cargo-supplied; clearing them leaves the
  re-executed worker unable to start on Linux and macOS.
• Project-scope discovery gets its own home directory instead of aliasing the
  project directory, so the assertions can only pass through project scope.
• UndefinedError is asserted before the downcast in the
  missing-environment-variable manifest test, so the cause is checked whatever
  the outer error type; the ManifestError::Parse check is retained for wrapped
  values.
• Explicit-config assertion pinned to the stable diagnostic as well as the
  missing path, so a generic I/O failure cannot satisfy it.
• Whitaker boundary contract parses arguments. Substring matching could not
  tell --package test_support from --package test_support-extra, nor spot a
  second selection; a small tokenizer now requires exactly one selection per
  invocation.
• Workflow YAML root validated structurally. _load pattern-matches the
  yaml.safe_load result and rejects scalar, list, and None roots. Note that
  GitHub Actions' on: key is a YAML 1.1 boolean word, so PyYAML returns it as
  True; the key is restored to "on" before the string-key check.
• Non-UTF-8 ninja override fails loudly rather than converting lossily, and
  the packaging assertion now names the offending path.
• ENV_LOCK poison flag cleared before asserting, so a failing assertion
  cannot leak process-global poisoning; the test carries a note that its direct
  global-mutex probe depends on nextest's per-test process isolation.

Structure and documentation

• Replaced the cfg-gated bare return in EnvSnapshot::capture_with_env with
  per-platform wrappers over the two divergent capture_impl arities.
• Kept the compiler-wrapper marker sidecar in camino's UTF-8 domain, converting
  once at the std::path boundary.
• Split the installation, release-detail, and Windows setup contracts out of
  tests/documentation_examples_tests.rs (495 lines) into
  tests/documentation_examples_installation_tests.rs, following the existing
  convention of one test binary per concern sharing the documentation_examples
  loader module. The original file is now 354 lines.
• Corrected initialise/initialisation to Oxford spelling in the macro
  initialization message (both English locales) and the fetch-cache Rustdoc,
  updating the inline insta snapshot in lockstep.
• Documented the native-path contract for the injected Ninja executable, and
  filled the Rustdoc gaps in `fake_ninja_check_build_file_in`,
  `real_utility_with_env`, `stdlib_output_or_error`, and the non-Unix
  `make_executable` (whose `#[cfg]` also sat inside its doc comment).

Not actioned

One comment asked to rewrite the `run_netsuke_in` environment-contract comment
to say no process environment variables are inherited. That helper does not call
`env_clear` — it inherits the parent environment except for the selectors it
explicitly sets or removes — so the existing comment is accurate and the
suggested wording describes `run_netsuke_in_with_env` instead. Applying it
would have made the comment wrong.

Validation

All deterministic commit gates pass on a clean tree: `check-fmt`, `lint`
(rustdoc, Clippy with `-D warnings`, Whitaker for both crates), `typecheck`,
`test` (1652 nextest tests, 83 doctests), `markdownlint`, `nixie`, and
`test-workflow-contracts` (12/12).

References

• Follows up on Harden workspace lint configuration #515 — Harden workspace lint configuration
• Lody session: https://lody.ai/leynos/sessions/44745a2f-e479-493e-95e3-4862edefc48d

🤖 Generated with Claude Code

Summary by Sourcery

Address post-merge review feedback to harden lint-related tests, environment handling, and documentation contracts across manifests, workflows, packaging, and installation flows.

Bug Fixes:

• Ensure BDD IR compilation and manifest steps both read the injected scenario environment instead of the host environment.
• Sandbox CLI merge tests so child processes cannot see host XDG configuration and preserve dynamic loader paths needed to start re-executed workers.
• Tighten Whitaker boundary tests to require exactly one package selection per invocation and avoid substring-based false positives.
• Validate workflow YAML roots structurally as string-keyed mappings to surface malformed documents with clear assertions.
• Fail loudly on non-UTF-8 Ninja override paths instead of performing lossy conversions, and improve packaging assertions by naming offending manifest entries.
• Prevent global ENV_LOCK poisoning from leaking between tests by clearing the poison flag before asserting state.
• Assert MiniJinja UndefinedError causes for missing env vars regardless of the outer error type, and pin explicit-config error diagnostics to their stable messages.
• Ensure project-scope config discovery tests use a distinct home directory so they only pass via the intended scope.

Enhancements:

• Refactor EnvSnapshot environment capture into platform-specific wrappers to avoid cfg-gated early returns and clarify PATHEXT handling.
• Document and re-export test helpers for resolving real utilities via injected environments and clarify behaviour of stdlib_output_or_error, fake Ninja helpers, and make_executable on different platforms.
• Keep compiler-wrapper marker paths within camino's UTF-8 domain by converting at the std::path boundary and tightening associated tests.
• Adjust manifest macro error messaging and documentation to consistent spelling and update associated snapshots.
• Expose manifest environment reader helpers for reuse and add documentation clarifying scenario-based environment handling.

Documentation:

• Split installation and release documentation example tests into a dedicated binary to keep documentation_examples_tests within size limits while preserving installation and Windows setup contracts.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Summary

• Isolate tests from host configuration and environment state.
• Preserve native, non-UTF-8 paths for Ninja and compiler-wrapper handling.
• Strengthen manifest, workflow, packaging, configuration-selection, and Whitaker contract assertions.
• Add IR environment regression coverage, including unset-variable failures.
• Centralize and test bounded, redacted telemetry for template rendering and macro invocation.
• Redact caller-provided duration values from logs.
• Document telemetry boundaries, metric contracts, and redaction rules in ADR-008 and developer documentation.
• Improve platform-specific environment capture, mutex-poisoning assertions, helper documentation, spelling consistency, and Rustdoc coverage.

Validation

• Pass formatting, linting, type checking, Markdown linting, Nixie, 1,668 nextest tests, 84 doctests, and 12 workflow contract tests.

Walkthrough

The change centralizes Jinja telemetry, preserves native executable paths, injects environments into manifest loading, redacts duration diagnostics, and strengthens integration-test contracts.

Changes

Manifest and test contracts

Layer / File(s)	Summary
Centralize Jinja telemetry src/manifest/jinja_macros/*, src/manifest/tests/*, locales/*, docs/*	Move invocation and render metrics into telemetry. Record bounded outcomes and durations. Add telemetry tests and documentation.
Preserve native paths and inject environments docs/developers-guide.md, src/runner/mod.rs, src/stdlib/which/env.rs, test_support/src/*, tests/bdd/steps/*, tests/novice_flow_smoke_tests.rs, tests/data/ir_env.yml, tests/features/ir.feature	Use native executable paths. Pass injected environment providers to manifest loading. Add platform-specific utility wrappers. Reject invalid UTF-8 override paths.
Redact test-support diagnostics test_support/src/http/*, test_support/src/env_lock.rs, test_support/src/stdlib_assert.rs	Report duration parse categories and lengths without exposing supplied values. Clarify helper contracts and mutex recovery behaviour.
Isolate and strengthen integration contracts tests/cli_tests/*, tests/manifest_jinja_tests.rs, tests/packaging_smoke_tests.rs, tests/whitaker_boundary_contract.rs, tests/workflow_contracts/*	Use isolated child environments. Validate explicit configuration errors, structured package selections, packaged paths, manifest errors, and YAML workflow structure.

Sequence Diagram(s)

sequenceDiagram
  participant MacroInvocation
  participant Telemetry
  participant MiniJinja
  participant Metrics
  MacroInvocation->>Telemetry: instrument_macro_invocation
  Telemetry->>MiniJinja: evaluate macro
  MiniJinja-->>Telemetry: return result
  Telemetry->>Metrics: record bounded outcome and duration
  Telemetry-->>MacroInvocation: return original result

Loading

Possibly related PRs

• leynos/netsuke#285: Extends the related configuration-layer loading pipeline.
• leynos/netsuke#501: Introduces the injected EnvReader seam used by this change.
• leynos/netsuke#515: Introduces the Jinja telemetry paths centralised by this change.

Suggested reviewers: codescene-access

Poem

Preserve native paths.
Bound every metric.
Redact each value.
Isolate the environment.
Verify every contract.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Testing (Overall)	❌ Error	The new _WorkflowLoader and _load branches have no focused tests; all six existing tests only load the valid CI file, so removed root/key validation would still pass.	Add isolated tests for YAML 1.2 on and boolean resolution, plus empty, scalar, list, and non-string-keyed documents; assert each raises the documented AssertionError.
Testing (Unit And Behavioural)	⚠️ Warning	Reject the check: _load adds YAML 1.2 and root/key rejection branches, but ci_lint_test.py only exercises the valid workflow; no tests cover these edge or error paths.	Add focused tests for scalar, list, empty, and non-string-key YAML documents, plus YAML 1.2 boolean-key handling; add unit cases for package_selections forms and false positives.
Testing (Property / Proof)	⚠️ Warning	The new _WorkflowLoader and package_selections enforce invariants over YAML shapes and recipe tokens, but no Hypothesis or proptest tests cover those paths.	Add Hypothesis properties for YAML 1.2 resolution, valid and invalid roots, and non-string keys; add generated recipe-token properties for package_selections.
User-Facing Documentation	❓ Inconclusive	Investigation is still in progress; no final assessment submitted yet.	Inspect the net diff and user-facing impact before deciding.

✅ Passed checks (16 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately identifies the post-merge review follow-ups for PR #515 and matches the changeset scope.
Description check	✅ Passed	The description clearly explains the review follow-ups, implementation changes, validation, and deferred items.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Developer Documentation	✅ Passed	The guide documents manifest telemetry, injected configuration access, native executable paths, and test seams; ADR-008 and netsuke-design.md record the telemetry boundary.
Module-Level Documentation	✅ Passed	Treat the check as passed: all 357 Rust modules and both Python test modules have file-level documentation, and changed modules state their purpose and component relationships.
Testing (Compile-Time / Ui)	✅ Passed	No compile-time API behaviour was added, so trybuild is not applicable. invocation.rs updates an inline snapshot, while telemetry and redaction tests assert stable semantics.
Unit Architecture	✅ Passed	The PR isolates tracing, metrics, and timing in explicit telemetry wrappers; evaluation remains in fallible query functions, while environment and process dependencies use injected readers or child...
Domain Architecture	✅ Passed	Pass: keep evaluation separate from telemetry; invoke_macro has no metrics or tracing, and private telemetry.rs emits only bounded labels through permitted library instrumentation.
Observability	✅ Passed	Accept the check: manifest rendering and macro invocation emit counters, duration histograms, spans, and bounded error outcomes; tests verify redaction and cardinality limits.
Security And Privacy	✅ Passed	Keep the change: telemetry emits only bounded labels and MiniJinja error kinds, duration logs redact values, and workflow parsing uses SafeLoader with root/key validation.
Performance And Resource Use	✅ Passed	The runtime diff preserves existing telemetry work and adds no new unbounded production loop or I/O; new parsing and property tests use bounded inputs and linear scans.
Concurrency And State	✅ Passed	Keep the check passing: private Once guards protect metric descriptions, tests use scoped local recorders, and EnvLock tests cover contention, re-entry, arbitrary drops, and poison recovery.
Architectural Complexity And Maintainability	✅ Passed	Keep the explicit telemetry module: it centralises redaction and two real boundaries, uses existing metrics/tracing seams, and adds no traits, registries, dependencies, or circular module edges.
Rust Compiler Lint Integrity	✅ Passed	Keep the change: the PR adds no broad Rust lint suppressions or clone calls, and new helpers have real callers; platform-gated imports and wrappers preserve compiler lint visibility.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pr-515-follow-ups

---

_{Comment @coderabbitai help to get the list of available commands.}

[sourcery-ai]

Reviewer's Guide

This PR addresses follow-up review comments from #515 by tightening environment handling in tests and helpers, hardening workflow and packaging contracts, improving Whitaker lint boundary checks, documenting several test-support contracts, and splitting installation-related documentation tests into their own binary, while also correcting English locale wording and small platform-specific behaviours.

File-Level Changes

Change	Details	Files
Align manifest and IR BDD steps to use the same injected scenario environment.	Introduce and re-export manifest_env_reader so it can be used from IR steps Change IR compile_manifest_impl to call from_path_with_policy_and_env with NetworkPolicy::default and the injected EnvReader Update module visibility for the manifest environment step module so IR steps can access it	tests/bdd/steps/ir.rs tests/bdd/steps/manifest/environment.rs tests/bdd/steps/manifest/mod.rs
Improve CLI merge tests to run under an isolated environment and correctly forward dynamic loader paths.	Update merge tests to obtain an isolated_environment and pass it into merge_in_child so host XDG config cannot leak into the child Extend merge_probe merge_in_child to re-export critical environment variables like LD_LIBRARY_PATH and DYLD_FALLBACK_LIBRARY_PATH in addition to basic Windows and temp vars	tests/cli_tests/merge.rs tests/cli_tests/merge_probe.rs
Strengthen configuration discovery, explicit-config error diagnostics, and jinja manifest error assertions.	Use a separate temporary home directory in config discovery scope tests so project-scope assertions cannot pass via home scope In the config selection test, assert that the explicit-config error message includes both the missing path and a stable diagnostic string In jinja manifest tests, assert first that an UndefinedError is present in the error chain, then retain ManifestError::Parse checks when applicable	tests/cli_tests/config_discovery_scopes.rs tests/cli_tests/config_selection.rs tests/manifest_jinja_tests.rs
Refactor environment snapshot capture to avoid cfg-gated early returns and centralize platform divergence.	Replace cfg-gated bare return in EnvSnapshot::capture_with_env with a call to a new capture_for_platform helper Add Windows and non-Windows capture_for_platform implementations that call the appropriate capture_impl arity and document the PATHEXT divergence	src/stdlib/which/env.rs
Improve workflow contract tests by enforcing structural validation of YAML workflow roots and keys.	Change workflow _load helper to pattern-match the yaml.safe_load result and require a mapping root, raising clear assertions for other types Normalize the GitHub Actions 'on' key from YAML boolean True back to the string "on" Add a check that all workflow mapping keys are strings and error if non-string keys are present	tests/workflow_contracts/ci_lint_test.py
Tighten Whitaker boundary contract tests to parse package selections robustly rather than rely on substring matching.	Introduce package_selections tokenizer that recognizes --package=value, separated --package value, and -p value forms Update Whitaker boundary tests to assert that scoped and root invocations select exactly the expected package using package_selections and adjust diagnostics accordingly	tests/whitaker_boundary_contract.rs
Make Ninja override handling and packaging assertions more robust and better documented.	Change novice flow helper to fail when the Ninja override path is non-UTF-8 instead of converting lossily, including the offending path in the error Improve packaging smoke test forbidden-root assertion to locate and print the offending packaged path instead of reporting only the root Add Rustdoc describing the fake_ninja_check_build_file_in contract and its relation to non-UTF-8 parent-directory handling	tests/novice_flow_smoke_tests.rs tests/packaging_smoke_tests.rs test_support/src/check_ninja.rs
Clarify contracts and docs for test-support helpers involving environment, execution, and stdlib outputs.	Re-export real_utility_with_env from dev_fast with detailed Rustdoc explaining the injected environment seam and error behavior Document stdlib_output_or_error semantics with explicit Returns/Errors sections and clarify precedence of output over error text Add explicit documentation for make_executable on Unix and non-Unix, including error behavior and the no-op contract on non-Unix	test_support/src/dev_fast/mod.rs test_support/src/stdlib_assert.rs test_support/src/exec.rs
Ensure global ENV_LOCK poisoning is handled safely and tests are properly annotated for process isolation.	Update env_lock_recovers_after_mutex_poisoning test to capture ENV_LOCK.is_poisoned, clear the poison flag before assertions, and avoid leaving the mutex poisoned at the end Add doc comments explaining the dependence on cargo-nextest per-test process isolation and the risk of races under thread-parallel runners	test_support/src/env_lock.rs
Keep compiler-wrapper sidecar markers in camino's UTF-8 domain and convert once at the std::path boundary.	Adjust compiler wrapper test helper to convert the wrapper PathBuf into a Utf8PathBuf, generate the .invoked marker in UTF-8 space, and produce a native OsString only when wiring up the RUSTC env var	test_support/src/command_helper.rs
Document native-path contract for the runner’s Ninja executable.	Annotate ExecutionContext.ninja_program field to state that it must remain a native Path and must not be converted to String, preserving support for non-UTF-8 executable paths	src/runner/mod.rs
Split installation and release documentation example tests into a separate binary to keep file size within limits.	Extract installation, registry, release-detail, and Windows setup tests from documentation_examples_tests.rs into a new documentation_examples_installation_tests.rs module Wire the new module to share the documentation_examples loader via a public submodule and leave the original tests file slimmer	tests/documentation_examples_tests.rs tests/documentation_examples_installation_tests.rs
Correct wording and initialization spelling in English locales and associated tests/docstrings.	Update en-GB and en-US locale messages to use 'initialize' in the macro init_failed message Adjust jinja macro invocation snapshot expectation to match the new wording Fix Rustdoc spelling for fetch cache initialization in NetworkConfig	locales/en-GB/messages.ftl locales/en-US/messages.ftl src/manifest/jinja_macros/invocation.rs src/stdlib/config_types.rs

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Actionable comments posted: 5

🤖 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 `@locales/en-GB/messages.ftl`:
- Line 142: Restore en-GB spelling at all affected sites: update
locales/en-GB/messages.ftl lines 142-142 to “initialise”, update the en-GB
diagnostic snapshot in src/manifest/jinja_macros/invocation.rs lines 142-142 to
“initialise”, and change “initialization” to “initialisation” in the Rust
comment at src/stdlib/config_types.rs lines 34-34.

In `@test_support/src/env_lock.rs`:
- Around line 255-257: Update the recovery test around EnvLock::acquire so the
held-state check does not panic while recovered_guard is live: read
ENV_LOCK_STATE non-panically into a boolean, drop recovered_guard, then assert
the captured state. Keep the existing poison-state assertion and subsequent
release assertion unchanged.

In `@test_support/src/http/tests.rs`:
- Around line 59-63: Add an expected warning-length field to DurationCase and
populate it for every invalid-value case, including empty input. Update the
warning assertions in the affected test sections to verify the rendered byte
length alongside the variable name, parse-error text, and redaction check, while
leaving valid cases without warnings unchanged.

In `@tests/packaging_smoke_tests.rs`:
- Around line 98-109: Update assert_forbidden_roots_absent to use
camino::Utf8Path::new instead of std::path::Path::new for both path
constructions, and compare the first component via Utf8Component::as_str() for
an exact root match so names like test_support-extra remain allowed.

In `@tests/workflow_contracts/ci_lint_test.py`:
- Around line 53-58: Update the YAML parsing in the workflow document loading
path to avoid treating YAML 1.1 boolean aliases such as yes and true as the
GitHub Actions on key. Use a YAML 1.2-compatible loader, or normalize only a
literal on key while preserving all other keys, then keep the existing workflow
validation behavior unchanged.

🪄 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: 864e13f1-a0e1-4375-8c80-d89fca081940

📥 Commits

Reviewing files that changed from the base of the PR and between 8ac4103 and aa9ecba.

📒 Files selected for processing (31)

• docs/developers-guide.md
• locales/en-GB/messages.ftl
• locales/en-US/messages.ftl
• src/manifest/jinja_macros/invocation.rs
• src/manifest/jinja_macros/mod.rs
• src/manifest/jinja_macros/telemetry.rs
• src/manifest/tests/macro_invocation_telemetry.rs
• src/manifest/tests/mod.rs
• src/runner/mod.rs
• src/stdlib/config_types.rs
• src/stdlib/which/env.rs
• test_support/src/check_ninja.rs
• test_support/src/command_helper.rs
• test_support/src/dev_fast/mod.rs
• test_support/src/env_lock.rs
• test_support/src/exec.rs
• test_support/src/http/mod.rs
• test_support/src/http/tests.rs
• test_support/src/stdlib_assert.rs
• tests/bdd/steps/ir.rs
• tests/bdd/steps/manifest/environment.rs
• tests/bdd/steps/manifest/mod.rs
• tests/cli_tests/config_discovery_scopes.rs
• tests/cli_tests/config_selection.rs
• tests/cli_tests/merge.rs
• tests/cli_tests/merge_probe.rs
• tests/manifest_jinja_tests.rs
• tests/novice_flow_smoke_tests.rs
• tests/packaging_smoke_tests.rs
• tests/whitaker_boundary_contract.rs
• tests/workflow_contracts/ci_lint_test.py

🔗 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)

[leynos]

@coderabbitai resume

[coderabbitai]

✅ Action performed

Reviews resumed.

[coderabbitai]

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/env_lock.rs`:
- Around line 101-102: Update assert_current_thread_lock_is_held and its callers
to avoid asserting while an EnvLock guard remains live: capture the
current_thread_lock_is_held result, explicitly drop all relevant guards, then
perform the assertion using the captured value. Preserve the existing assertion
message and ensure both caller paths release their EnvLock guards before
assertion.

🪄 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: 5523864c-5336-4e13-ba98-628b8adf6853

📥 Commits

Reviewing files that changed from the base of the PR and between 8ac4103 and cadb529.

📒 Files selected for processing (31)

• docs/developers-guide.md
• locales/en-GB/messages.ftl
• locales/en-US/messages.ftl
• src/manifest/jinja_macros/invocation.rs
• src/manifest/jinja_macros/mod.rs
• src/manifest/jinja_macros/telemetry.rs
• src/manifest/tests/macro_invocation_telemetry.rs
• src/manifest/tests/mod.rs
• src/runner/mod.rs
• src/stdlib/config_types.rs
• src/stdlib/which/env.rs
• test_support/src/check_ninja.rs
• test_support/src/command_helper.rs
• test_support/src/dev_fast/mod.rs
• test_support/src/env_lock.rs
• test_support/src/exec.rs
• test_support/src/http/mod.rs
• test_support/src/http/tests.rs
• test_support/src/stdlib_assert.rs
• tests/bdd/steps/ir.rs
• tests/bdd/steps/manifest/environment.rs
• tests/bdd/steps/manifest/mod.rs
• tests/cli_tests/config_discovery_scopes.rs
• tests/cli_tests/config_selection.rs
• tests/cli_tests/merge.rs
• tests/cli_tests/merge_probe.rs
• tests/manifest_jinja_tests.rs
• tests/novice_flow_smoke_tests.rs
• tests/packaging_smoke_tests.rs
• tests/whitaker_boundary_contract.rs
• tests/workflow_contracts/ci_lint_test.py

🔗 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)

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test_support/src/env_lock.rs (1)

254-263: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the poisoning-test safety comment.

Line 271 asserts before ENV_LOCK.clear_poison() at Lines 272-273. Therefore, the statement that every assertion runs with the poison flag cleared is false. State that only the post-clear assertions have this guarantee. Keep the separate no-live-guard guarantee for the join assertion.

Proposed comment correction

-    /// Every assertion here is made with no guard live and the poison flag
-    /// already cleared, so a failure cannot leave process-global poisoning
-    /// behind: clearing the flag covers the deliberate poisoning, and
-    /// capturing the held state before dropping the guard covers the unwind
-    /// path, where a panic would otherwise drop the `MutexGuard` and poison
-    /// the mutex afresh.
+    /// The post-recovery assertions run with no guard live and the poison flag
+    /// already cleared. The join assertion runs before the clear and has no
+    /// live `EnvLock` guard in the current thread. Capturing the held state
+    /// before dropping the guard prevents an assertion unwind from poisoning
+    /// the mutex afresh.

🤖 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 `@test_support/src/env_lock.rs` around lines 254 - 263, Correct the safety
documentation above the poisoning test: in the comment covering the join and
recovery assertions, state that only post-ENV_LOCK.clear_poison() assertions
have the poison-cleared guarantee, while the join assertion occurs before
clearing and has no live EnvLock guard in the current thread. Preserve the
explanation that capturing held state before dropping the guard prevents
assertion unwinding from re-poisoning the mutex.

🤖 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/adr-008-bounded-redacted-manifest-telemetry.md`:
- Line 21: Remove the unnecessary commas before “because” in both affected
sentences in the ADR, including the sentence near “recording it in a trace” and
the one near “only because imports evaluate.”

---

Outside diff comments:
In `@test_support/src/env_lock.rs`:
- Around line 254-263: Correct the safety documentation above the poisoning
test: in the comment covering the join and recovery assertions, state that only
post-ENV_LOCK.clear_poison() assertions have the poison-cleared guarantee, while
the join assertion occurs before clearing and has no live EnvLock guard in the
current thread. Preserve the explanation that capturing held state before
dropping the guard prevents assertion unwinding from re-poisoning the mutex.

🪄 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: 144555cb-9caa-4da9-8487-7a29cb6b6d36

📥 Commits

Reviewing files that changed from the base of the PR and between cadb529 and 0cf6ddb.

📒 Files selected for processing (8)

• docs/adr-008-bounded-redacted-manifest-telemetry.md
• docs/contents.md
• docs/developers-guide.md
• docs/netsuke-design.md
• test_support/src/env_lock.rs
• tests/data/ir_env.yml
• tests/features/ir.feature
• tests/novice_flow_smoke_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/whitaker (auto-detected)
• leynos/shared-actions (auto-detected)

[buzzybee-df12]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/developers-guide.md (1)

275-280: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a caption for this table.

Place a concise caption immediately before the table. Document readers need its purpose without inferring it from the surrounding prose.

Triage: [type:docstyle]

As per coding guidelines, caption every table and diagram.

🤖 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 `@docs/developers-guide.md` around lines 275 - 280, Add a concise descriptive
caption immediately before the table listing workflows, jobs, shared actions,
and Rust flags in the developers guide. Keep the existing table content
unchanged.

Source: Coding guidelines

🤖 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/adr-008-bounded-redacted-manifest-telemetry.md`:
- Line 9: Update the ADR Date value to the bare YYYY-MM-DD date 2026-08-06,
removing the trailing full stop while preserving the rest of the document.

In `@tests/novice_flow_smoke_tests.rs`:
- Around line 3-7: Update the imports in the novice flow smoke tests so bail is
imported separately under #[cfg(unix)], alongside the existing Unix-only PathBuf
import. Keep Context, Result, and ensure available for all platforms, and
preserve the Unix-only test behavior.

---

Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 275-280: Add a concise descriptive caption immediately before the
table listing workflows, jobs, shared actions, and Rust flags in the developers
guide. Keep the existing table content unchanged.

🪄 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: ae0f8f31-f487-41d8-8ec6-ad2320cbe087

📥 Commits

Reviewing files that changed from the base of the PR and between cadb529 and 115b310.

📒 Files selected for processing (10)

• docs/adr-008-bounded-redacted-manifest-telemetry.md
• docs/contents.md
• docs/developers-guide.md
• docs/netsuke-design.md
• src/manifest/jinja_macros/mod.rs
• src/manifest/jinja_macros/telemetry.rs
• test_support/src/env_lock.rs
• tests/data/ir_env.yml
• tests/features/ir.feature
• tests/novice_flow_smoke_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/whitaker (auto-detected)
• leynos/shared-actions (auto-detected)

[leynos]

@coderabbitai please create a new GitHub issue for the following:

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Testing (Overall)	❌ Error	Telemetry, HTTP redaction, IR environment, path, and lock changes have substantive tests. However, the new _WorkflowLoader and _load validation have no tests for their YAML branches; existing t...	Add focused loader tests that assert on remains a string, true/false resolve, and _load rejects empty, scalar, list, and non-string-key mappings.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="leynos/netsuke"
pr_number="530"
needle='Add focused loader tests that assert `on` remains a string'

comment_url="$(
  gh api --paginate "repos/$repo/issues/$pr_number/comments?per_page=100" \
    --jq ".[] | select(.body | contains(\"$needle\")) | .html_url" \
  | head -n1
)"

if [ -z "$comment_url" ]; then
  echo "Could not locate the originating PR comment." >&2
  exit 1
fi

existing="$(
  gh issue list --repo "$repo" --state open --search 'WorkflowLoader _load YAML tests in:title,body' \
    --json number,title,url --jq '.[] | "\(.number)\t\(.title)\t\(.url)"'
)"

if [ -n "$existing" ]; then
  printf 'Existing matching issue:\n%s\n' "$existing"
  exit 0
fi

body_file="$(mktemp)"
trap 'rm -f "$body_file"' EXIT
cat >"$body_file" <<EOF
## Summary

Add focused unit tests for the YAML workflow loader validation added in \`tests/workflow_contracts/ci_lint_test.py\`.

## Rationale

The current change adds YAML 1.2 boolean handling and validates documents returned by \`_load\`. Existing tests do not cover these branches. Add direct coverage to prevent regressions in GitHub Actions workflow parsing and validation.

## Affected area

- \`tests/workflow_contracts/ci_lint_test.py\`
- Its associated Python test module

## Required changes

Add focused tests for \`_WorkflowLoader\` and \`_load\`.

## Acceptance criteria

- A workflow document keeps the \`on\` key as a string.
- YAML \`true\` and \`false\` values resolve as booleans.
- \`_load\` rejects an empty document.
- \`_load\` rejects scalar documents.
- \`_load\` rejects list documents.
- \`_load\` rejects mappings that contain non-string keys.
- The tests assert the intended error behaviour without depending on unrelated workflow contract tests.

## Backlinks

- Follow-up from PR `#530`: https://github.com/leynos/netsuke/pull/530
- Originating review comment: $comment_url
- Requested by: `@leynos`
EOF

issue_url="$(
  gh issue create \
    --repo "$repo" \
    --title "Add YAML loader validation tests for workflow contracts" \
    --body-file "$body_file" \
    --assignee leynos
)"

printf 'Created issue: %s\n' "$issue_url"

Length of output: 2367

---

@leynos Created and assigned #543: Add YAML loader validation tests for workflow contracts.

_{You are interacting with an AI system.}

[leynos]

@coderabbitai resume

[coderabbitai]

✅ Action performed

Reviews resumed.

[codescene-access]

No quality gates enabled for this code.