Skip to content

Interpolate Maven ${...} properties through one formats::maven helper: five copies drifted on recursion, undefined names and the length cap #1887

Open

Description

[agent] Filed by the scheduled architecture audit (audit-core). Register: register.

Kind: refactor. Register ID: A30.

Problem

The ${name} substitution loop (find("${"), find('}'), lookup, push) is written five times. The copies have drifted:

Copy Recursive Undefined name Output length cap
maven_crawler::interpolate L738-L751 (settings.xml) no None none
formats::maven::interpolate / interpolate_once L329-L372 up to MAX_INTERPOLATION rounds kept verbatim MAX_INTERPOLATED_LEN
maven_scope::interpolate_at L491-L534 depth-limited None none
maven_reactor::interpolate_by L1002-L1021 depth-limited None none
maven_reactor::Reactor::interpolate_with L1689-L1710 depth-limited None none

The last three are the same loop body, differing only in how they call the lookup:

while let Some(at) = rest.find("${") {
    out.push_str(&rest[..at]);
    let after = &rest[at + 2..];
    let close = after.find('}')?;
    let raw = lookup(&after[..close])?;
    out.push_str(&interpolate_by(&raw, lookup, depth + 1)?);
    rest = &after[close + 1..];
}

Only formats::maven caps the output length. The depth-limited recursive copies can still grow exponentially within the depth limit (a=${b}${b}, b=${c}${c}, …), which is the "billion laughs" case that cap exists for. This is from reading the code; I did not execute it.

Proposed change

  • Add formats::maven::interpolate_by(value, lookup: &dyn Fn(&str) -> Option<String>) -> Option<String>. It recurses with MAX_INTERPOLATION as the depth limit and returns None past MAX_INTERPOLATED_LEN.
  • maven_scope, maven_reactor (both) and maven_crawler keep their lookup closures and call it.
  • maven_crawler's settings.xml use is non-recursive today. Maven itself interpolates settings.xml from env/system properties only, so recursion there is harmless, but keep a test that pins its current result.
  • formats::maven::interpolate's keep-unresolved mode becomes a lookup that echoes ${name} back, so it can share the loop too. If that changes its output, it can stay as the one separate wrapper.

Size

4 files, about −70 lines.

Acceptance criteria

  • Behavior is unchanged for in-cap inputs. These must stay green: the existing maven_scope, maven_reactor and formats::maven interpolation tests, and maven_pom_equivalence_tests.rs.
  • Add one test showing that a doubling property chain returns None (refused) in the scope and reactor paths instead of growing past MAX_INTERPOLATED_LEN.

Activity

  1. added
    arch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)
    refactorStructural change: duplicated code or logic, missing abstraction, layering, dead code
    on Oct 30, 2026
  2. mikolalysenko commented on Oct 30, 2026

    @mikolalysenko
    CollaboratorAuthor

    [agent] Triaged: priority:p3 (Maven). Confirmed on main: the five ${...} loops are at maven_crawler::interpolate, formats::maven::interpolate/interpolate_once, maven_scope::interpolate_at, and maven_reactor::interpolate_by / Reactor::interpolate_with. No duplicate or open PR found. Not clustered with #1809 (that one is about reactor detection, not property interpolation).


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent:triagedarch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)priority:p3refactorStructural change: duplicated code or logic, missing abstraction, layering, dead code

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions