Skip to content

fix: strip redundant ./ prefixes from source path in header comment#106

Merged
jamesadevine merged 1 commit intomainfrom
fix/source-path-dot-slash
Apr 1, 2026
Merged

fix: strip redundant ./ prefixes from source path in header comment#106
jamesadevine merged 1 commit intomainfrom
fix/source-path-dot-slash

Conversation

@jamesadevine
Copy link
Copy Markdown
Collaborator

Problem

The @ado-aw source= header in generated YAML files accumulates redundant ./ prefixes on each recompilation via compile_all_pipelines:

# @ado-aw source="././././agents/release-readiness.md" version=0.7.0

Root Cause

compile_all_pipelines does Path::new(".").join(&pipeline.source) where pipeline.source is read from a previously generated header. Each recompilation prepends another ./.

Fix

Strip leading ./ sequences in generate_header_comment before writing the source path into the header. Includes two new tests covering single and multiple ./ prefixes.

compile_all_pipelines joins Path(".") with the source path read from a
previously generated header. Each recompilation prepended another "./",
causing paths like ././././agents/release-readiness.md.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jamesadevine jamesadevine merged commit 689825c into main Apr 1, 2026
7 of 9 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

🔍 Rust PR Review

Summary: Correct fix for a real idempotency bug — looks good with one minor suggestion.

Findings

✅ What Looks Good

  • Fix is correct and self-healing. The while loop in generate_header_comment breaks the accumulation cycle cleanly. Even pre-existing headers with "././././agents/foo.md" get normalized on the next recompilation: compile_all_pipelines reads the old prefix-laden path → joins with Path::new(".") → passes to generate_header_comment → all ./ prefixes stripped → clean header written. Subsequent runs stay clean.

  • Sanitization order is correct. The ./ stripping runs after the \n/\r/" replacements, so there's no edge case where a stripped prefix re-exposes a character injection.

  • Tests cover the reported scenarios — both the "././././..." (multi-prefix) and "./..." (single-prefix) cases are exercised.

⚠️ Suggestions

  • src/compile/common.rs — while loop allocates on every iteration. Each iteration does source_path[2..].to_string() — a fresh heap allocation. For a compiler this is trivially fine, but the idiomatic Rust approach avoids it entirely and is clearer about intent:

    // instead of:
    while source_path.starts_with("./") {
        source_path = source_path[2..].to_string();
    }
    
    // prefer:
    let source_path = source_path.trim_start_matches("./").to_string();

    trim_start_matches with a &str pattern strips that exact substring from the start repeatedly, which is identical semantics in one call and zero intermediate allocations.

  • Root cause in compile_all_pipelines (line 142) is unfixed. The PR description correctly identifies root.join(&pipeline.source) as the source of the extra ./. The write-time fix is sufficient to stop accumulation going forward, but fixing the read-time join would be belt-and-suspenders:

    // src/compile/mod.rs:142
    let source_path = root.join(pipeline.source.trim_start_matches("./"));

    Not required for correctness (the current fix is idempotent), but it removes the latent path construction oddity.

Generated by Rust PR Reviewer for issue #106 ·

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant