Address PR #515 review follow-ups

docs/adr-009-bounded-redacted-manifest-telemetry.md

@@ -0,0 +1,156 @@
+# Architecture decision record (ADR): Bounded, redacted telemetry for manifest evaluation
+
+## Status
+
+Accepted.
+
+## Date
+
+2026-08-06.
+
+## Context and problem statement
+
+Manifest evaluation renders Jinja templates and invokes manifest-defined
+macros. Both are queries: they compute a value and were expected to stay free
+of ambient concerns such as timing and metric emission. Once observability was
+needed for these paths, two risks had to be weighed against each other:
+
+- Manifest content — template text, macro names, macro arguments, and context
+  values — is caller-controlled and unbounded. Recording it directly in a
+  metric label produces unbounded cardinality in the metric series, and
+  recording it in a trace risks leaking secrets because environment variable
+  names routinely identify credentials (`src/manifest/env_reader.rs` already
+  applies this rule to `env()` lookup failures).
+- Interleaving spans and metric emission with the evaluation logic in
+  `render_template` and the macro-invocation callback would make those
+  functions read as instrumentation rather than plain evaluation, and would
+  scatter the redaction contract across every call site instead of collecting
+  it in one place a reviewer can audit.
+
+`AGENTS.md` also constrains the shape of the answer: libraries may emit
+`metrics` and `tracing` instrumentation but must not install global recorders
+or subscribers; only the application initializes those once, at startup.
+
+## Decision
+
+Collect manifest telemetry in `src/manifest/jinja_macros/telemetry.rs`, kept
+separate from evaluation, with two distinct instrumentation boundaries:
+
+- **Template render** — `manifest::jinja_macros::render_template` composes
+  `telemetry::instrument_template_render`, which wraps the render in the
+  `manifest.template.render` span, increments the
+  `netsuke_manifest_template_renders_total` counter, and records the
+  `netsuke_manifest_template_render_duration_seconds` histogram.
+- **Macro invocation** — the compiled-expression fallback built by
+  `make_macro_fn` (`src/manifest/jinja_macros/invocation.rs`) composes
+  `telemetry::instrument_macro_invocation`, which wraps the invocation in the
+  `manifest.macro.invoke` span, increments the
+  `netsuke_manifest_macro_invocations_total` counter, and records the
+  `netsuke_manifest_macro_invocation_duration_seconds` histogram.
+
+Macros reached through a template import are metered only at the render
+boundary; the macro-invocation counter covers only the compiled-expression
+fallback because imports evaluate inside the render call and never reach
+`make_macro_fn`. `macro_invocation_telemetry.rs`'s
+`imported_macro_render_does_not_emit_invocation_metrics` test pins this split.
+
+The label and field vocabulary is bounded by construction:
+
+- `outcome` is always `"success"` or `"error"`.
+- The render boundary adds `has_macro_imports`, `"true"` or `"false"`.
+- On failure, both boundaries add `error_category`, the `Debug` form of
+  `minijinja::ErrorKind`, never the error's `Display` text, which can embed
+  manifest content.
+
+Template text, macro names, macro arguments, context values, and environment
+variable names never reach a span field or a metric label.
+
+## Rationale
+
+- **Queries stay pure.** Keeping instrumentation in one module lets
+  `render_template` and the macro-invocation callback compose an
+  instrumentation boundary explicitly rather than interleave timing and metric
+  code with evaluation.
+- **One place to audit the privacy contract.** Every field emitted by
+  `telemetry.rs` is enumerated in that module, so confirming that manifest
+  content cannot reach a subscriber is a single, small review rather than an
+  audit of every call site.
+- **Two boundaries, not one.** Template rendering and macro invocation are
+  different operations with different failure shapes and different call
+  patterns (one render can invoke many macros); merging them into a single
+  counter would hide the render/invocation split that
+  `imported_macro_render_does_not_emit_invocation_metrics` depends on.
+- **Bounded labels prevent cardinality blowups.** `outcome`,
+  `has_macro_imports`, and `error_category` are drawn from small, fixed
+  vocabularies, so the metric series stays bounded regardless of how many
+  distinct manifests, macros, or templates are evaluated.
+- **`error_category` uses `Debug`, not `Display`.** `minijinja::ErrorKind`'s
+  `Debug` form is a fixed enum variant name; the `Display` text of a
+  `minijinja::Error` can embed manifest content such as variable names.
+- **Matches the existing environment-name redaction rule.** `env_var_with` in
+  `src/manifest/env_reader.rs` already omits the variable name from both
+  tracing and the returned Jinja error, for the same reason: manifest-supplied
+  names routinely identify credentials.
+
+## Consequences
+
+- New telemetry fields for these two boundaries must be added to
+  `telemetry.rs` and reviewed against the redaction contract before merging;
+  they must not be added ad hoc at the render or invocation call sites.
+- `src/manifest/tests/macros_telemetry.rs` and
+  `src/manifest/tests/macro_invocation_telemetry.rs` pin the counter and
+  histogram names, the bounded label vocabulary, and the render/invocation
+  boundary split using a local `metrics_util::debugging::DebuggingRecorder` and
+  the workspace's tracing capture helper, so neither the global recorder nor
+  the global subscriber is touched by the test suite.
+  `macro_invocation_telemetry.rs` also runs a proptest,
+  `macro_telemetry_stays_bounded_for_arbitrary_macros`, asserting the redaction
+  contract for arbitrary generated macro names, arguments, and
+  undefined-variable names, not only the fixed sentinel cases.
+- The counter and histogram names
+  (`netsuke_manifest_template_renders_total`,
+  `netsuke_manifest_template_render_duration_seconds`,
+  `netsuke_manifest_macro_invocations_total`,
+  `netsuke_manifest_macro_invocation_duration_seconds`) are a published
+  contract for anything scraping these metrics; renaming them is a breaking
+  change.
+- Because the module only calls `metrics` and `tracing` macros and never
+  installs a recorder or subscriber, the application remains free to choose or
+  omit an exporter at startup without any change to manifest evaluation.
+
+## Alternatives considered
+
+- **Instrument inline in the evaluation path.** Rejected. Interleaving spans
+  and metric emission directly inside `render_template` and the macro-callback
+  closure would make the redaction contract implicit at each call site instead
+  of reviewable in one module, and would make `render_template` read as
+  instrumentation-plus-evaluation rather than plain evaluation composed with an
+  explicit instrumentation boundary.
+- **Inject a telemetry-sink trait.** Rejected. The `metrics` crate already
+  provides that abstraction: `counter!`/`histogram!`/`describe_*!` route
+  through whatever recorder the application installs, and `tracing` provides
+  the equivalent for spans and events through the subscriber. A bespoke sink
+  trait would duplicate that abstraction while still needing the same redaction
+  discipline, and `AGENTS.md` already forbids this module from installing a
+  recorder or subscriber itself.
+- **One shared counter and histogram for both boundaries.** Rejected. Merging
+  render and invocation telemetry would erase the render/invocation split that
+  distinguishes the import path from the compiled-expression fallback, making
+  it impossible to answer "did this render's macro imports run, or fall back to
+  compiled-expression invocation?" from the metrics alone.
+
+## Implementation references
+
+- Telemetry module:
+  [`src/manifest/jinja_macros/telemetry.rs`](../src/manifest/jinja_macros/telemetry.rs)
+- Render boundary composition:
+  [`src/manifest/jinja_macros/mod.rs`](../src/manifest/jinja_macros/mod.rs)
+- Macro-invocation boundary composition:
+  [`src/manifest/jinja_macros/invocation.rs`](../src/manifest/jinja_macros/invocation.rs)
+- Matching environment-name redaction rule:
+  [`src/manifest/env_reader.rs`](../src/manifest/env_reader.rs)
+- Tests:
+  [`src/manifest/tests/macros_telemetry.rs`](../src/manifest/tests/macros_telemetry.rs),
+  [`src/manifest/tests/macro_invocation_telemetry.rs`](../src/manifest/tests/macro_invocation_telemetry.rs)
+- Developer guide:
+  [`docs/developers-guide.md`](developers-guide.md#manifest-telemetry-template-render-and-macro-invocation)

docs/contents.md

@@ -49,6 +49,9 @@ operator, user, and contributor references are easier to find.
 - [adr-008-environment-seam-taxonomy.md](adr-008-environment-seam-taxonomy.md):
   Environment seam taxonomy decision record: three sanctioned shapes for
   injecting environment-dependent input instead of reading the process directly.
+- [adr-009-bounded-redacted-manifest-telemetry.md](adr-009-bounded-redacted-manifest-telemetry.md):
+  Manifest telemetry decision record, separating observability from evaluation
+  and bounding and redacting the emitted metrics and spans.
 
 ## User and operator guides
 

docs/developers-guide.md

@@ -1348,25 +1348,20 @@ writes that content and applies executable permissions only on Unix.
 `write_exec` is the minimal-script convenience wrapper;
 `write_exec_with_content` is the shared primitive for custom behaviour.
 
-The helpers take `&Utf8Path` and return `Utf8PathBuf`, matching the camino
-types used throughout Netsuke. Callers that already hold camino paths pass them
-directly. `tempfile::TempDir::path()` still yields an OS-native `&Path`, so
-callers convert at that boundary with `exec::utf8_path`, the single conversion
-point. `utf8_path` returns a `Result` rather than panicking: it names the
-offending path in the error (`path is not valid UTF-8: {path}`), and callers
-propagate it with their own context, as `fake_ninja` and
-`fake_ninja_check_build_file` do.
+The helpers take `&Path` and return `PathBuf`, the OS-native types that
+`tempfile::TempDir::path()` already yields. Because the helpers sit at the
+`tempfile`/OS boundary, there is no conversion step: callers pass the temporary
+directory's path straight through.
 
 ```rust
 let temp = TempDir::new()?;
-let root = exec::utf8_path(temp.path()).context("temporary directory")?;
-let stub = write_exec(root, "tool")?;
+let stub = write_exec(temp.path(), "tool")?;
 ```
 
-Because a camino path cannot represent a non-UTF-8 path, the fake-executable
-factories now fail on a temporary directory whose path is not valid UTF-8,
-rather than succeeding as they previously did. The `test_support` test
-`fake_ninja_helpers_reject_non_utf8_temp_directories` pins this behaviour.
+Because `write_exec` and `write_exec_with_content` operate on OS-native paths
+directly, the fake-executable factories accept a temporary directory whose path
+is not valid UTF-8. The `test_support` test
+`fake_ninja_helpers_support_non_utf8_temp_directories` pins this behaviour.
 
 ### User-facing documentation examples
 
@@ -2185,10 +2180,12 @@ Configuration merge helpers:
   `PathBuf`.
 - `explicit_config_path_with_env(cli, env) -> Option<PathBuf>` resolves explicit
   config selection from `--config` and `NETSUKE_CONFIG`.
-- `push_file_layers(cli, composer, errors) -> ()` pushes explicit or discovered
-  file layers onto a `MergeComposer`. Explicit load errors are pushed into
-  `errors`, and automatic discovery is not attempted after an explicit selector
-  fails.
+- `push_file_layers_with_env(cli, composer, errors, env) -> ()` pushes explicit
+  or discovered file layers onto a `MergeComposer`. The injected `env`
+  parameter follows the environment mandate: it supplies environment access
+  without requiring callers to mutate the process environment. Explicit load
+  errors are pushed into `errors`, and automatic discovery is not attempted
+  after an explicit selector fails.
 - `collect_diag_file_layers_with_env(cli, env)` reuses the same file-layer
   precedence for early JSON resolution.
 - `collect_file_layers(directory)` builds the fallback discovery layer chain,
@@ -2626,6 +2623,79 @@ MiniJinja state on each invocation, so it must not be treated as a reusable
 global template cache. Errors remain at the manifest boundary and retain their
 localized failure category.
 
+### Manifest telemetry: template render and macro invocation
+
+`src/manifest/jinja_macros/telemetry.rs` instruments the two boundaries above
+with `tracing` spans and `metrics` counters/histograms, kept out of the
+evaluation code so `render_template` and the macro-invocation callback stay
+plain queries. See [ADR-009](adr-009-bounded-redacted-manifest-telemetry.md)
+for the decision to separate observability from evaluation this way, and for
+the alternatives it rejected.
+
+There are two independent boundaries, because template rendering and macro
+invocation are different operations with different failure shapes:
+
+- **Template render.** `render_template` composes
+  `telemetry::instrument_template_render` around the render. It opens the
+  `manifest.template.render` span, increments the
+  `netsuke_manifest_template_renders_total` counter, and records the
+  `netsuke_manifest_template_render_duration_seconds` histogram.
+- **Macro invocation.** `make_macro_fn`'s compiled-expression fallback
+  composes `telemetry::instrument_macro_invocation` around the invocation. It
+  opens the `manifest.macro.invoke` span, increments the
+  `netsuke_manifest_macro_invocations_total` counter, and records the
+  `netsuke_manifest_macro_invocation_duration_seconds` histogram.
+
+Macros reached through a template import run inside the render call and never
+reach `make_macro_fn`, so they are metered at the render boundary only; the
+macro-invocation counter covers the compiled-expression fallback. The test
+`imported_macro_render_does_not_emit_invocation_metrics` in
+`src/manifest/tests/macro_invocation_telemetry.rs` pins this split.
+
+The label and field vocabulary is bounded by construction, never echoing
+manifest content into a span or a metric dimension:
+
+- `outcome` is always `"success"` or `"error"`.
+- The render boundary adds `has_macro_imports`, `"true"` or `"false"`,
+  distinguishing the import-prefixed render path from the plain one without
+  revealing which macros a manifest defines.
+- On failure, both boundaries add `error_category`, the `Debug` form of
+  `minijinja::ErrorKind` — never the error's `Display` text, which can embed
+  manifest content.
+
+**Redaction rule.** Template text, macro names, macro arguments, context
+values, and environment variable names must never reach telemetry. Manifest
+content is caller-controlled and unbounded, so recording it in a metric label
+would make the metric series unbounded, and recording it in a span or event
+risks leaking secrets — environment variable names routinely identify
+credentials. This mirrors the redaction rule `env_var_with` already applies to
+`env()` lookup failures; see [Manifest `env()` reader](#manifest-env-reader).
+
+`describe_macro_metrics` and `describe_render_metrics` register each metric's
+description exactly once, guarded by `std::sync::Once`. Neither is called from a
+query function: `describe_macro_metrics` runs when `make_macro_fn` builds a
+macro's registration, which is setup rather than evaluation, so the guard never
+sits on the invocation hot path; `describe_render_metrics` runs inside
+`instrument_template_render`, so `render_template` names only the
+instrumentation boundary it composes with and never reaches for the metric
+registry itself.
+
+Per `AGENTS.md`, this module emits through `metrics` and `tracing` but must not
+install a global recorder or subscriber; only the application does that, at
+startup. Tests follow the same rule: `src/manifest/tests/macros_telemetry.rs`
+(the render boundary) and `src/manifest/tests/macro_invocation_telemetry.rs`
+(the macro-invocation boundary) each drive a local
+`metrics_util::debugging::DebuggingRecorder` through
+`metrics::with_local_recorder`, and capture tracing events with the workspace's
+`with_test_subscriber` helper (see [`tracing_capture`](#tracing_capture)), so
+neither test touches process-wide state. Extend `macros_telemetry.rs` for
+render-boundary coverage and `macro_invocation_telemetry.rs` for
+invocation-boundary coverage. The latter also runs a proptest,
+`macro_telemetry_stays_bounded_for_arbitrary_macros`, which asserts the
+redaction contract holds for arbitrary generated macro names, arguments, and
+undefined-variable names, not just the fixed sentinel cases used by the other
+tests.
+
 ### Expansion helpers
 
 #### expand_foreach

docs/netsuke-design.md

@@ -1020,6 +1020,14 @@ do not depend on the lifetime of the manifest parsing state. This preserves
 MiniJinja's argument handling, including keyword parameters and `caller`
 support, while allowing later macros to override earlier ones.
 
+Template rendering and macro invocation are each wrapped in a bounded, redacted
+telemetry boundary, kept separate from evaluation so manifest content never
+reaches a span or a metric label.
+[ADR-009](adr-009-bounded-redacted-manifest-telemetry.md) records this
+decision; see the
+[developer's guide](developers-guide.md#manifest-telemetry-template-render-and-macro-invocation)
+for the metric names, bounded label vocabulary, and redaction rules.
+
 ### 4.4 Essential Custom Functions
 
 To transform `minijinja` from a general-purpose templating engine into a

locales/en-GB/messages.ftl

@@ -139,7 +139,7 @@ manifest.macro.register_failed = Failed to register manifest macros.
 manifest.macro.not_initialised = Macro environment is not initialised.
 manifest.macro.caller_invalid = Macro caller must be a string.
 manifest.macro.template_load_failed = Failed to load macro template.
-manifest.macro.init_failed = Failed to initialise macro environment.
+manifest.macro.init_failed = Failed to initialize macro environment.
 manifest.macro.missing = Macro { $name } is missing.
 
 # Manifest glob errors.

locales/en-US/messages.ftl

@@ -139,7 +139,7 @@ manifest.macro.register_failed = Failed to register manifest macros.
 manifest.macro.not_initialised = Macro environment is not initialised.
 manifest.macro.caller_invalid = Macro caller must be a string.
 manifest.macro.template_load_failed = Failed to load macro template.
-manifest.macro.init_failed = Failed to initialise macro environment.
+manifest.macro.init_failed = Failed to initialize macro environment.
 manifest.macro.missing = Macro { $name } is missing.
 
 # Manifest glob errors.