refactor(footnotes): separate diagnostics from mutation in collect_scan_updates (CQS)

[lodyai]

Background

collect_scan_updates (src/footnotes/renumber/definitions.rs:178-225) both
mutates the scan state (the command) and emits tracing diagnostics for
skipped candidates (lines ~206-211) in the same pass.

Problem

Mixing the state mutation with observability entangles two responsibilities and
makes the core mutation harder to unit-test in isolation: a test of the scan
logic cannot exercise it without also driving the logging path, and the logging
cannot be reasoned about independently of the mutation order.

Objective

Separate the command from the diagnostics.

• Have the collection pass record skipped candidates into the state (or a
  returned collection) without logging.
• Emit diagnostics in a distinct log_skipped_candidates step after collection
  completes.

This restores command/query separation and lets the mutation be tested without
the tracing side effect.

Acceptance criteria

• collect_scan_updates performs no logging; skipped candidates are recorded in
  state.
• A separate function owns the tracing output.
• A unit test exercises the collection logic and asserts the recorded skipped
  candidates without depending on log capture.
• Existing renumber tests continue to pass; run the full make commit gate.

References

• Requested by @leynos

[leynos]

@CodeRabbit plan

Require property tests for the mutation if appropriate. proptest is an approved dependency.

[coderabbitai]

Coding Plan

Summary

• Introduce a SkippedCandidate enum capturing the two skip reasons with their associated data, extending DefinitionScanState to accumulate these records during the scan pass.
• Strip the two trace! calls from collect_scan_updates, replacing them with pushes to the new skipped_candidates vector.
• Provide a separate log_skipped_candidates function that iterates over the recorded skips and emits the original diagnostics, called from collect_definition_updates after collection completes.
• Add a direct unit test of collect_scan_updates that asserts on the recorded skipped candidates without log capture dependencies.

Implementation Steps

Phase 1: Refactor Diagnostics Out of Mutation Path

This phase introduces a data type to represent skip reasons, extends the scan state to accumulate them, removes inline logging from the scan loop, and provides a dedicated function to emit diagnostics after collection completes.

Task 1: Introduce SkippedCandidate Type and Extend DefinitionScanState

Add a type to capture the two distinct skip reasons with their associated data, then extend the scan state to accumulate these records.

• Create a SkippedCandidate enum in src/footnotes/renumber/definitions.rs near the existing NumericCandidate struct (around line 55)
• Define two variants matching the existing skip scenarios:
  • OutsideConversionRange — carries index, numeric_list_range, and skip_numeric_conversion flag
  • NoMappingExists — carries index only
• Derive Debug on the enum to support diagnostic formatting
• Add skipped_candidates: Vec<SkippedCandidate> field to DefinitionScanState (around line 82)
• Initialise the new field as an empty Vec in collect_definition_updates where the state is constructed (around line 271)

Task 2: Replace Inline Logging with State Recording

Modify the scan loop to record skip reasons rather than emitting diagnostics directly.

• In collect_scan_updates (lines 178–225), locate the first trace! call (lines ~206–211) that logs "skipping numeric footnote candidate conversion"
• Replace this trace! invocation with a push of SkippedCandidate::OutsideConversionRange { index, numeric_list_range, skip_numeric_conversion } onto state.skipped_candidates
• Locate the second trace! call (lines ~212–215) that logs "skipping numeric footnote candidate before any reference mapping exists"
• Replace it with a push of SkippedCandidate::NoMappingExists { index } onto state.skipped_candidates
• Remove the tracing::trace import if it becomes unused in this module (check for other usages first)

Task 3: Create log_skipped_candidates and Wire It Up

Provide a dedicated function to emit diagnostics from the recorded skip data, called after the scan completes.

• Create a private function fn log_skipped_candidates(skipped: &[SkippedCandidate]) in src/footnotes/renumber/definitions.rs
• Iterate over the slice and emit a trace! event for each variant, reproducing the original log messages and fields:
  • For OutsideConversionRange: emit trace!(index, numeric_list_range = ?range, skip_numeric_conversion, "skipping numeric footnote candidate conversion")
  • For NoMappingExists: emit trace!(index, "skipping numeric footnote candidate before any reference mapping exists")
• In collect_definition_updates, insert a call to log_skipped_candidates(&state.skipped_candidates) immediately after collect_scan_updates(lines, &mut state) returns and before finalize_numeric_candidates(&mut state) is called (around line 277)

🤖 Prompt for AI agents

Refactor `src/footnotes/renumber/definitions.rs` to separate state mutation from
tracing diagnostics by introducing a `SkippedCandidate` enum, extending
`DefinitionScanState`, replacing inline `trace!` calls with state recording, and
wiring up a dedicated logging function.

**Task 1: Introduce `SkippedCandidate` and extend `DefinitionScanState`**
- Near the existing `NumericCandidate` struct (around line 55), define a new
`SkippedCandidate` enum with `#[derive(Debug)]`
- Add two variants:
- `OutsideConversionRange { index, numeric_list_range, skip_numeric_conversion
}`
  - `NoMappingExists { index }`
- Add a `skipped_candidates: Vec<SkippedCandidate>` field to
`DefinitionScanState` (around line 82)
- Initialise `skipped_candidates` as an empty `Vec` where `DefinitionScanState`
is constructed in `collect_definition_updates` (around line 271)

**Task 2: Replace inline `trace!` calls with state recording**
- In `collect_scan_updates` (lines 178–225), find the first `trace!` (lines
~206–211) logging `"skipping numeric footnote candidate conversion"` and replace
it with a push of `SkippedCandidate::OutsideConversionRange { index,
numeric_list_range, skip_numeric_conversion }` onto `state.skipped_candidates`
- Find the second `trace!` (lines ~212–215) logging `"skipping numeric footnote
candidate before any reference mapping exists"` and replace it with a push of
`SkippedCandidate::NoMappingExists { index }` onto `state.skipped_candidates`
- Check for any remaining usages of `tracing::trace` in this module; remove the
import only if it is no longer used

**Task 3: Create `log_skipped_candidates` and wire it up**
- Add a private function `fn log_skipped_candidates(skipped:
&[SkippedCandidate])` that iterates over the slice and emits a `trace!` event
per variant:
- `OutsideConversionRange`: `trace!(index, numeric_list_range = ?range,
skip_numeric_conversion, "skipping numeric footnote candidate conversion")`
- `NoMappingExists`: `trace!(index, "skipping numeric footnote candidate before
any reference mapping exists")`
- In `collect_definition_updates`, call
`log_skipped_candidates(&state.skipped_candidates)` immediately after
`collect_scan_updates(lines, &mut state)` returns and before
`finalize_numeric_candidates(&mut state)` is called (around line 277)

Phase 2: Add Unit Test for Recorded Skip Data

This phase adds a focused unit test that exercises the collection logic and asserts the recorded skipped candidates without depending on log capture.

Task 1: Add Unit Test for Skipped Candidate Recording

Write a test that triggers both skip conditions and verifies the accumulated records.

• Add a new test function in src/footnotes/renumber/definitions/tests.rs named collect_scan_updates_records_skipped_candidates
• Follow existing patterns: use the strings() helper for input lines, construct DefinitionScanState manually with controlled values
• Prepare input lines containing:
  • A numeric list item outside the numeric_list_range (or with skip_numeric_conversion = true) to trigger OutsideConversionRange
  • A numeric list item within range but before any mapping exists to trigger NoMappingExists
• Call collect_scan_updates directly (accessible via super:: from the tests submodule)
• Assert on state.skipped_candidates.len() and match on variant/field values using assert_eq! or pattern matching
• Confirm the test does not require log capture or tracing subscriber setup

Task 2: Verify Existing Tests Pass

Run the full test suite to confirm no regressions.

• Execute make (the commit gate) to run all tests including tests/footnotes.rs integration tests and the existing unit tests in definitions/tests.rs
• Confirm all renumber-related tests pass unchanged
• Check that the existing trace! behaviour is preserved by running with RUST_LOG=trace and verifying diagnostic output manually

🤖 Prompt for AI agents

Add a focused unit test in `src/footnotes/renumber/definitions/tests.rs` that
directly exercises `collect_scan_updates` and asserts on the recorded
`skipped_candidates` — no log capture required.

- Add a test function named `collect_scan_updates_records_skipped_candidates`
following existing patterns in the file (plain `#[test]` or `#[rstest]` as
appropriate)
- Use the `strings()` helper to construct input lines; construct
`DefinitionScanState` manually with controlled values
- Prepare input lines that trigger both skip conditions:
- A numeric list item outside the `numeric_list_range` (or with
`skip_numeric_conversion = true`) → `SkippedCandidate::OutsideConversionRange`
- A numeric list item within range but before any mapping exists →
`SkippedCandidate::NoMappingExists`
- Call `collect_scan_updates` via `super::collect_scan_updates`
- Assert `state.skipped_candidates.len()` is correct, and use `assert_eq!` or
pattern matching to verify each variant's field values
- Do NOT introduce any tracing subscriber setup; the test must be self-contained

After writing the test, run `make` to execute the full test suite (including
`tests/footnotes.rs` integration tests and all unit tests in
`definitions/tests.rs`). Verify all renumber-related tests pass. Optionally run
with `RUST_LOG=trace` to confirm diagnostic output is preserved.

Research

The footnotes renumber functionality lives in src/footnotes/renumber/, with definitions.rs containing the scan logic. The private collect_scan_updates function iterates over lines, populating DefinitionScanState with definitions and numeric candidates whilst currently emitting two trace! diagnostics for skipped lines. DefinitionScanState is a stack-local struct constructed and consumed within collect_definition_updates. Unit tests reside in src/footnotes/renumber/definitions/tests.rs, using plain #[test] and #[rstest] patterns with a local strings() helper.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Refactor `src/footnotes/renumber/definitions.rs` to separate state mutation from
tracing diagnostics by introducing a `SkippedCandidate` enum, extending
`DefinitionScanState`, replacing inline `trace!` calls with state recording, and
wiring up a dedicated logging function.

**Task 1: Introduce `SkippedCandidate` and extend `DefinitionScanState`**
- Near the existing `NumericCandidate` struct (around line 55), define a new
`SkippedCandidate` enum with `#[derive(Debug)]`
- Add two variants:
- `OutsideConversionRange { index, numeric_list_range, skip_numeric_conversion
}`
  - `NoMappingExists { index }`
- Add a `skipped_candidates: Vec<SkippedCandidate>` field to
`DefinitionScanState` (around line 82)
- Initialise `skipped_candidates` as an empty `Vec` where `DefinitionScanState`
is constructed in `collect_definition_updates` (around line 271)

**Task 2: Replace inline `trace!` calls with state recording**
- In `collect_scan_updates` (lines 178–225), find the first `trace!` (lines
~206–211) logging `"skipping numeric footnote candidate conversion"` and replace
it with a push of `SkippedCandidate::OutsideConversionRange { index,
numeric_list_range, skip_numeric_conversion }` onto `state.skipped_candidates`
- Find the second `trace!` (lines ~212–215) logging `"skipping numeric footnote
candidate before any reference mapping exists"` and replace it with a push of
`SkippedCandidate::NoMappingExists { index }` onto `state.skipped_candidates`
- Check for any remaining usages of `tracing::trace` in this module; remove the
import only if it is no longer used

**Task 3: Create `log_skipped_candidates` and wire it up**
- Add a private function `fn log_skipped_candidates(skipped:
&[SkippedCandidate])` that iterates over the slice and emits a `trace!` event
per variant:
- `OutsideConversionRange`: `trace!(index, numeric_list_range = ?range,
skip_numeric_conversion, "skipping numeric footnote candidate conversion")`
- `NoMappingExists`: `trace!(index, "skipping numeric footnote candidate before
any reference mapping exists")`
- In `collect_definition_updates`, call
`log_skipped_candidates(&state.skipped_candidates)` immediately after
`collect_scan_updates(lines, &mut state)` returns and before
`finalize_numeric_candidates(&mut state)` is called (around line 277)
===============================================================================

Task: 2

Add a focused unit test in `src/footnotes/renumber/definitions/tests.rs` that
directly exercises `collect_scan_updates` and asserts on the recorded
`skipped_candidates` — no log capture required.

- Add a test function named `collect_scan_updates_records_skipped_candidates`
following existing patterns in the file (plain `#[test]` or `#[rstest]` as
appropriate)
- Use the `strings()` helper to construct input lines; construct
`DefinitionScanState` manually with controlled values
- Prepare input lines that trigger both skip conditions:
- A numeric list item outside the `numeric_list_range` (or with
`skip_numeric_conversion = true`) → `SkippedCandidate::OutsideConversionRange`
- A numeric list item within range but before any mapping exists →
`SkippedCandidate::NoMappingExists`
- Call `collect_scan_updates` via `super::collect_scan_updates`
- Assert `state.skipped_candidates.len()` is correct, and use `assert_eq!` or
pattern matching to verify each variant's field values
- Do NOT introduce any tracing subscriber setup; the test must be self-contained

After writing the test, run `make` to execute the full test suite (including
`tests/footnotes.rs` integration tests and all unit tests in
`definitions/tests.rs`). Verify all renumber-related tests pass. Optionally run
with `RUST_LOG=trace` to confirm diagnostic output is preserved.

💡 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.

---

💬 Have feedback or questions? Drop into our discord!