Remove full NetsukeManifest AST from graph-generation DEBUG logs

[coderabbitai]

Summary

Remove the DEBUG log in src/runner/graph_generation.rs that serialises and writes the complete user-supplied NetsukeManifest AST.

Rationale

A manifest can contain variables, macro bodies, and other user-supplied content. Writing the complete manifest to DEBUG logs can expose sensitive data and create unbounded log records.

Required changes

• Remove the complete AST DEBUG log and its serialisation.
• Keep the existing bounded graph-generation counter, duration histogram, trace span, and fixed error categories.
• If diagnostic context is required, log only fixed or bounded metadata. Do not log user-supplied manifest content.

Affected area

• src/runner/graph_generation.rs, Lines 55-59

Acceptance criteria

• The graph-generation path does not serialise or log the complete NetsukeManifest.
• Observability retains bounded metrics, tracing, and fixed error categories.
• No replacement log includes variables, macro bodies, or other unbounded user-supplied manifest content.

Backlinks

• Reported on PR Set v0.1.0-beta3 release version and status #624: Set v0.1.0-beta3 release version and status #624
• Source review comment: Set v0.1.0-beta3 release version and status #624 (comment)
• Requested by: @leynos

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Summary

• Delete the full-manifest AST DEBUG block and its unused debug import from src/runner/graph_generation.rs. Keep all bounded telemetry intact.
• Add no replacement log. The existing graph_generation_telemetry counter, histogram, span, and error categories already give bounded diagnostics.
• Remove the orphaned RUNNER_CONTEXT_SERIALISE_MANIFEST key atomically from keys.rs and all 35 locale catalogues to keep the l10n audit clean.
• Add a leak-check regression test in src/runner/tests.rs that proves a sentinel manifest string never appears in captured log output at DEBUG level.

Design Choices

Design Choice 1: Whether to add a bounded replacement diagnostic in place of the removed AST dump

Options Considered:

1. Add no replacement log. Rely on the existing bounded telemetry in graph_generation_telemetry.rs (counter, histogram, span, error categories).
2. Add a new bounded diagnostic event (for example target/action counts) following the ADR-009 pattern.

Chosen Option: 1

Rationale: The ticket permits, but does not require, a replacement that logs only fixed or bounded metadata. The existing bounded telemetry already covers diagnostic needs for this path, so add no new log and avoid unnecessary scope.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Remove the unbounded manifest log

This phase removes the raw AST dump and its now-unused Rust import. Preserve all bounded telemetry and the anyhow Context trait, which other call sites still use.

Task 1: Remove the full-manifest DEBUG block

Delete the log that serialises and writes the complete manifest.

• Remove the if tracing::enabled!(tracing::Level::DEBUG) { … debug!("AST:\n{ast_json}") } block in src/runner/graph_generation.rs (Lines 55-59).
• Remove the serde_json::to_string_pretty(&manifest) call and its .context(localization::message(keys::RUNNER_CONTEXT_SERIALISE_MANIFEST))? handling that belong to this block only.
• Keep the Context import from super::{…}; it is still used at the other .context(…) call sites in this file.
• Keep all delegation to graph_generation_telemetry::instrument_graph_generation(...) untouched.

Task 2: Remove the now-unused `debug` import

Clean up the import that only the removed block used.

• Remove use tracing::debug; (Line 15) from src/runner/graph_generation.rs.
• Do not remove serde_json from Cargo.toml; the crate uses it in many other files.
• Confirm the file still compiles with no unused-import warnings.

🤖 Prompt for AI agents

Implement Phase 1: remove the unbounded manifest log from
`src/runner/graph_generation.rs`. The goal is to delete the raw AST dump while
preserving all bounded telemetry and the anyhow `Context` trait usage.

Delete the full-manifest DEBUG block:
- Remove the `if tracing::enabled!(tracing::Level::DEBUG) { …
debug!("AST:\n{ast_json}") }` block (Lines 55-59).
- Remove the associated `serde_json::to_string_pretty(&manifest)` call and its
`.context(localization::message(keys::RUNNER_CONTEXT_SERIALISE_MANIFEST))?`
error handling, but only the parts that belong to this block.
- Keep the `Context` import from `super::{…}` intact, since other `.context(…)`
call sites in the file still use it.
- Keep the delegation to
`graph_generation_telemetry::instrument_graph_generation(...)` untouched.

Remove the now-unused `debug` import:
- Remove `use tracing::debug;` (Line 15) from `src/runner/graph_generation.rs`.
- Do not remove `serde_json` from `Cargo.toml`, since the crate uses it in many
other files.
- Verify the file compiles with no unused-import warnings after these removals.

Phase 2: Remove the orphaned localisation key

This phase removes the localisation key that becomes dead after Phase 1. Land the removal atomically across keys.rs and all 35 locale catalogues, or the l10n audit fails the build.

Task 1: Remove the key from `keys.rs`

Delete the single macro entry for the removed key.

• Delete the line RUNNER_CONTEXT_SERIALISE_MANIFEST => "runner.context.serialise_manifest", from the define_keys! { … } body in src/localization/keys.rs.
• Do not edit ALL_KEYS by hand; the macro regenerates it from the remaining entries.
• Complete Phase 1 first. The constant is still referenced until then, and the crate does not compile otherwise.

Task 2: Remove the key from all 35 locale catalogues

Delete the translation line from every catalogue.

• Delete the runner.context.serialise_manifest = … line from each locales/*/messages.ftl file.
• No sync tool exists; edit each file, or apply one scripted deletion across all files.
• Land this task together with Task 2.1. If keys.rs and the catalogues drift, the audit reports missing or orphaned findings and fails cargo build and tests/build_l10n_audit_tests.rs.

Use this command to remove the line from every catalogue:

sed -i '/^runner\.context\.serialise_manifest = /d' locales/*/messages.ftl

🤖 Prompt for AI agents

Implement Phase 2: remove the orphaned localisation key that becomes dead after
Phase 1. Land this change atomically across `keys.rs` and all 35 locale
catalogues, or the l10n audit fails the build.

Remove the key from `keys.rs`:
- Delete the line `RUNNER_CONTEXT_SERIALISE_MANIFEST =>
"runner.context.serialise_manifest",` from the `define_keys! { … }` body in
`src/localization/keys.rs`.
- Do not edit `ALL_KEYS` by hand, since the macro regenerates it from the
remaining entries.
- Complete this task only after Phase 1, because the constant is still
referenced until then and the crate does not compile otherwise.

Remove the key from all 35 locale catalogues:
- Delete the `runner.context.serialise_manifest = …` line from each
`locales/*/messages.ftl` file.
- No sync tool exists. Edit each file individually, or apply one scripted
deletion across all files, for example:
~~~
sed -i '/^runner\.context\.serialise_manifest = /d' locales/*/messages.ftl
~~~
- Land the `keys.rs` removal and the catalogue removals together. If `keys.rs`
and the catalogues drift, the audit reports `missing` or `orphaned` findings and
fails `cargo build` and `tests/build_l10n_audit_tests.rs`.

Phase 3: Add a regression test

This phase pins the behaviour. It proves that manifest content never appears in emitted logs, following the established leak-check convention.

Add a leak-check test to `src/runner/tests.rs`

Prove that no manifest-derived content reaches the log output.

• Add the test next to runner_reports_the_complete_generation_stage_sequence in src/runner/tests.rs.
• Reuse the existing write_manifest(...) helper, GraphGenerationContext, and FixedMonotonicClock scaffolding.
• Embed a distinctive sentinel string (for example a target name or command) in the manifest under test.
• Wrap the generate_ninja_with_shell(...) call in crate::test_tracing_capture::with_test_subscriber(LevelFilter::DEBUG, |captured| { …; captured.snapshot() }).
• Assert that no captured event contains the sentinel string. Include the full event list in the panic message for diagnosis, matching the env_reader.rs style.
• Use LevelFilter::DEBUG to make the test maximally strict. Since the raw dump is removed, no manifest content must appear even at DEBUG.
• Do not add a new #[cfg(test)] mod tests inside graph_generation.rs; the runner unit tests live centrally in src/runner/tests.rs.

🤖 Prompt for AI agents

Implement Phase 3: add a regression test that pins the removal of manifest
content from emitted logs, following the established leak-check convention.

Add a leak-check test to `src/runner/tests.rs`:
- Place the test next to `runner_reports_the_complete_generation_stage_sequence`
in `src/runner/tests.rs`.
- Reuse the existing `write_manifest(...)` helper, `GraphGenerationContext`, and
`FixedMonotonicClock` scaffolding.
- Embed a distinctive sentinel string (for example a target name or command) in
the manifest under test.
- Wrap the `generate_ninja_with_shell(...)` call in
`crate::test_tracing_capture::with_test_subscriber(LevelFilter::DEBUG,
|captured| { …; captured.snapshot() })`.
- Assert that no captured event contains the sentinel string. Include the full
event list in the panic message for diagnosis, matching the style used in
`env_reader.rs`.
- Use `LevelFilter::DEBUG` to make the test maximally strict, since no manifest
content must appear even at DEBUG level after the raw dump removal.
- Do not add a new `#[cfg(test)] mod tests` inside `graph_generation.rs`. Keep
runner unit tests centralised in `src/runner/tests.rs`.

Research

The runner composes the Ninja build graph in src/runner/graph_generation.rs. Its function generate_ninja_with_shell loads the manifest, then emits a free-text debug!("AST:\n{ast_json}") event that serialises the entire manifest to pretty-printed JSON. This is the only log in src/ that writes raw, unbounded, user-controlled content. All bounded observability (counter, duration histogram, trace span, fixed error categories) lives separately in src/runner/graph_generation_telemetry.rs and is unaffected.

The serialisation uses the localisation key RUNNER_CONTEXT_SERIALISE_MANIFEST, defined in src/localization/keys.rs and translated in all 35 locales/*/messages.ftl files. The key is used at no other call site.

ADR-009 and ADR-013 mandate that manifest content, values, and paths never reach a span field, metric label, or log event. Only fixed vocabularies, counts, and hashes are permitted.

Runner unit tests live in src/runner/tests.rs. A thread-local tracing-capture harness (crate::test_tracing_capture::with_test_subscriber) supports leak-check tests, as used in src/manifest/env_reader.rs and src/manifest/tests/expansion_telemetry.rs.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement Phase 1: remove the unbounded manifest log from
`src/runner/graph_generation.rs`. The goal is to delete the raw AST dump while
preserving all bounded telemetry and the anyhow `Context` trait usage.

Delete the full-manifest DEBUG block:
- Remove the `if tracing::enabled!(tracing::Level::DEBUG) { …
debug!("AST:\n{ast_json}") }` block (Lines 55-59).
- Remove the associated `serde_json::to_string_pretty(&manifest)` call and its
`.context(localization::message(keys::RUNNER_CONTEXT_SERIALISE_MANIFEST))?`
error handling, but only the parts that belong to this block.
- Keep the `Context` import from `super::{…}` intact, since other `.context(…)`
call sites in the file still use it.
- Keep the delegation to
`graph_generation_telemetry::instrument_graph_generation(...)` untouched.

Remove the now-unused `debug` import:
- Remove `use tracing::debug;` (Line 15) from `src/runner/graph_generation.rs`.
- Do not remove `serde_json` from `Cargo.toml`, since the crate uses it in many
other files.
- Verify the file compiles with no unused-import warnings after these removals.
===============================================================================

Task: 2

Implement Phase 2: remove the orphaned localisation key that becomes dead after
Phase 1. Land this change atomically across `keys.rs` and all 35 locale
catalogues, or the l10n audit fails the build.

Remove the key from `keys.rs`:
- Delete the line `RUNNER_CONTEXT_SERIALISE_MANIFEST =>
"runner.context.serialise_manifest",` from the `define_keys! { … }` body in
`src/localization/keys.rs`.
- Do not edit `ALL_KEYS` by hand, since the macro regenerates it from the
remaining entries.
- Complete this task only after Phase 1, because the constant is still
referenced until then and the crate does not compile otherwise.

Remove the key from all 35 locale catalogues:
- Delete the `runner.context.serialise_manifest = …` line from each
`locales/*/messages.ftl` file.
- No sync tool exists. Edit each file individually, or apply one scripted
deletion across all files, for example:
~~~
sed -i '/^runner\.context\.serialise_manifest = /d' locales/*/messages.ftl
~~~
- Land the `keys.rs` removal and the catalogue removals together. If `keys.rs`
and the catalogues drift, the audit reports `missing` or `orphaned` findings and
fails `cargo build` and `tests/build_l10n_audit_tests.rs`.
===============================================================================

Task: 3

Implement Phase 3: add a regression test that pins the removal of manifest
content from emitted logs, following the established leak-check convention.

Add a leak-check test to `src/runner/tests.rs`:
- Place the test next to `runner_reports_the_complete_generation_stage_sequence`
in `src/runner/tests.rs`.
- Reuse the existing `write_manifest(...)` helper, `GraphGenerationContext`, and
`FixedMonotonicClock` scaffolding.
- Embed a distinctive sentinel string (for example a target name or command) in
the manifest under test.
- Wrap the `generate_ninja_with_shell(...)` call in
`crate::test_tracing_capture::with_test_subscriber(LevelFilter::DEBUG,
|captured| { …; captured.snapshot() })`.
- Assert that no captured event contains the sentinel string. Include the full
event list in the panic message for diagnosis, matching the style used in
`env_reader.rs`.
- Use `LevelFilter::DEBUG` to make the test maximally strict, since no manifest
content must appear even at DEBUG level after the raw dump removal.
- Do not add a new `#[cfg(test)] mod tests` inside `graph_generation.rs`. Keep
runner unit tests centralised in `src/runner/tests.rs`.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---