Skip to content

fix: preserve managed sections in distributed compile#1772

Closed
forcewake wants to merge 3 commits into
microsoft:mainfrom
forcewake:fix/1764-managed-section-distributed
Closed

fix: preserve managed sections in distributed compile#1772
forcewake wants to merge 3 commits into
microsoft:mainfrom
forcewake:fix/1764-managed-section-distributed

Conversation

@forcewake

Copy link
Copy Markdown

Summary

  • Fixes the default distributed apm compile path so root AGENTS.md honors compilation.agents_md.mode: managed_section instead of overwriting the whole file.
  • Routes the --single-agents CLI write path through the same managed-section helper.
  • Adds regression coverage for root preservation, subdirectory overwrite behavior, missing root markers, CLI --single-agents, and path normalization.

Closes #1764.

Changes

  • src/apm_cli/compilation/agents_compiler.py
    • Detect root AGENTS.md in the distributed writer.
    • Delegate root managed-section writes to the existing _write_output_file_with_config() helper.
    • Preserve full-overwrite behavior for distributed subdirectory AGENTS.md outputs.
  • src/apm_cli/commands/compile/cli.py
    • Route --single-agents + managed_section through the same helper.
  • tests/unit/compilation/test_managed_section.py
  • CHANGELOG.md
    • Adds an Unreleased fixed entry.

Validation

Ran locally before commit/push:

uv run --extra dev pytest tests/unit/compilation/test_managed_section.py -q
# 35 passed in 0.74s

uv run --extra dev pytest tests/unit/compilation -q
# 1169 passed in 2.82s

uv run --extra dev ruff check \
  src/apm_cli/compilation/agents_compiler.py \
  src/apm_cli/commands/compile/cli.py \
  tests/unit/compilation/test_managed_section.py
# All checks passed!

git diff --check
# clean

Additional review:

  • Claude Code + Ponytail /ponytail ultra produced the implementation.
  • GitHub Copilot CLI claude-opus-4.8 independently reviewed the diff and returned APPROVED; Copilot's sandbox could not execute uv/pytest/ruff, so the commands above were rerun directly afterward.

Notes

  • This deliberately reuses the existing managed-section helper instead of adding a second implementation.
  • Subdirectory distributed AGENTS.md files remain generated/full-overwrite files.
  • No dependency changes.

Copilot AI review requested due to automatic review settings June 14, 2026 16:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes #1764 by ensuring managed_section mode preserves hand-written content in the project-root AGENTS.md for both the default distributed strategy and the --single-agents CLI path.

Changes:

  • Route root AGENTS.md writes through _write_output_file_with_config() when agents_md_mode == "managed_section" in the distributed write path.
  • Route --single-agents final write through _write_output_file_with_config() when agents_md_mode == "managed_section", and broaden error handling.
  • Add regression tests covering preservation/overwrite behavior, missing-file errors, and symlink root detection; update changelog.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/unit/compilation/test_managed_section.py Adds regression coverage for managed-section behavior on distributed + --single-agents paths.
src/apm_cli/compilation/agents_compiler.py Delegates root distributed writes to the managed-section helper; catches ManagedSectionError during distributed compilation.
src/apm_cli/commands/compile/cli.py Uses the managed-section helper for --single-agents writes when configured.
CHANGELOG.md Documents the fix under “Unreleased”.
.hermes/rdd/2026-06-14-apm-managed-section-distributed/07-pr.md Adds PR handoff notes (process artifact).
.hermes/rdd/2026-06-14-apm-managed-section-distributed/06-review.md Adds review notes (process artifact).
.hermes/rdd/2026-06-14-apm-managed-section-distributed/05-implementation.md Adds implementation log (process artifact).
.hermes/rdd/2026-06-14-apm-managed-section-distributed/04-plan.md Adds plan doc (process artifact).
.hermes/rdd/2026-06-14-apm-managed-section-distributed/03-spec.md Adds spec doc (process artifact).
.hermes/rdd/2026-06-14-apm-managed-section-distributed/02-brainstorm.md Adds brainstorming notes (process artifact).
.hermes/rdd/2026-06-14-apm-managed-section-distributed/01-research.md Adds research notes (process artifact).

Comment thread src/apm_cli/commands/compile/cli.py
Comment thread src/apm_cli/compilation/agents_compiler.py
Comment thread tests/unit/compilation/test_managed_section.py Outdated
Comment thread .hermes/rdd/2026-06-14-apm-managed-section-distributed/07-pr.md Outdated
@forcewake

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@forcewake forcewake force-pushed the fix/1764-managed-section-distributed branch from 7d30581 to 577b5a0 Compare June 14, 2026 16:49
@forcewake forcewake force-pushed the fix/1764-managed-section-distributed branch from 577b5a0 to 9d95d2b Compare June 14, 2026 17:30
@forcewake

Copy link
Copy Markdown
Author

@copilot review please one more time

@sergio-sisternes-epam sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label Jun 15, 2026
@sergio-sisternes-epam sergio-sisternes-epam added panel-review Trigger the apm-review-panel gh-aw workflow and removed panel-review Trigger the apm-review-panel gh-aw workflow labels Jun 15, 2026
@github-actions

Copy link
Copy Markdown

APM Review Panel: ship_with_followups

Ship: silent data-loss bug correctly fixed in both apm compile and --single-agents paths; marker-injection, docs root-only scope, and integration test gap are the three highest-priority follow-ons.

The fix is correct and the two critical regression paths are covered at the appropriate test tiers. AC-1, the distributed managed_section happy path, is defended by a direct unit test against _write_distributed_file. AC-5, the --single-agents CLI path, is defended at the CLI end-to-end tier via CliRunner. The exception handling is technically sound: ManagedSectionError is a ValueError subclass, so the (OSError, ValueError) catch in cli.py captures both failure modes without a correctness gap. The raise_write_errors dual-mode flag and the cli.py if/else dispatch are real architectural roughness, but they do not introduce regressions and are appropriate for a follow-on cleanup, not a reason to delay the fix.

Three themes converge across panelists and each warrants a targeted follow-on issue in the same milestone. The supply-chain marker-injection finding is the highest-signal item in this panel: a consumed package whose instruction content contains the default end-marker string can survive into the human-authored region of AGENTS.md on the first compile, producing AI agent instructions that appear user-written. This is the only finding with a concrete attacker-controlled content path that crosses a trust boundary, and it requires a guard in managed_section.py before this feature is promoted in any onboarding material. The docs root-only scope gap is the most immediately actionable user-trust risk: doc-writer and devx-ux-expert converge independently on the observation that compile.md and manifest-schema.md both make unqualified claims that managed_section preserves surrounding content, which is false for subdirectory AGENTS.md files under the distributed strategy. Any user who reads those pages literally and has hand-written content in a subdirectory AGENTS.md will lose it silently today. The SecurityGate re-scan gap is a one-line defense-in-depth fix: the current scan runs only on the APM-compiled block before the managed_section merge, leaving human-authored sections with hidden Unicode overrides or other payloads unscanned in the final written file.

The test-coverage-expert's integration-with-fixtures gap carries weight above the remaining opinion-tier follow-ons because managed_section is a governed-by-policy surface and the evidence outcome is confirmed missing. The existing AC-1 unit test patches _write_distributed_file directly; a future refactor that bypasses it in _compile_distributed would leave that test green while silently re-introducing the data-loss bug. A full compiler.compile() -> _compile_distributed -> _write_distributed_file chain test with markers and surrounding prose in the root AGENTS.md closes this regression-trap gap permanently. The error UX improvements from cli-logging-expert and the CHANGELOG phrasing sharpening from the growth hacker are both correct observations, but they rank below the security and test-gap items on urgency and should be bundled into the nearest cleanup commit.

Dissent. cli-logging-expert flagged the (OSError, ValueError) catch in cli.py as catching the base class where ManagedSectionError is the specific expected type (nit severity). devx-ux-expert explicitly confirmed the catch is correct because ManagedSectionError IS a ValueError subclass -- this is not a defect. The cli-logging-expert's recommendation to split the except branches into separate handlers is valid as a user-message clarity improvement and survives as a follow-on, but it does not change program correctness and should not be read as a gap introduced by this PR.

Aligned with: Portable by Manifest -- The fix ensures that agents_md.mode: managed_section in apm.yml produces consistent behaviour across both compile strategies, closing the gap between what the manifest declares and what the tool delivers. Secure by Default -- Two supply-chain findings land on this surface: the marker-injection path and the SecurityGate re-scan gap. Neither was introduced by this PR; both were latent; the two follow-ons are net improvements. Governed by Policy -- managed_section is precisely the governed-by-policy surface for AGENTS.md writes. The fix correctly enforces that mode setting across both compile paths. OSS Community Driven -- Silent data loss on first real use of managed_section was the most likely driver of private abandonment from the highest-value adopter segment.

Growth signal. managed_section is the adoption gateway for the segment that already has hand-written AGENTS.md content and cannot afford silent overwrites on every compile. The bug meant this segment hit data loss on first real use and formed private conclusions before ever filing an issue. The release note and any accompanying post should lead with the before/after story: hand-written team rules survive above the APM block, APM content updates below, no manual merge required on every run. The quick-start should surface managed_section earlier with a one-liner callout aimed at teams already using AGENTS.md. The CHANGELOG phrasing fix -- replacing implementation-internal terms with plain command-language equivalents (apm compile and apm compile --single-agents) -- is worth taking in this same PR or immediately after, since the CHANGELOG is the first place power users look when something breaks.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 3 Fix is correct and well-tested; raise_write_errors dual-mode flag and cli.py mode-dispatch redundancy warrant follow-on cleanup but nothing blocks merge.
CLI Logging Expert 0 2 2 Error framing conflates I/O write failure with managed-section config errors; [target] bracket prefix mimics STATUS_SYMBOLS; no silent paths introduced.
DevX UX Expert 0 2 1 ManagedSectionError IS a ValueError subclass so the (OSError, ValueError) catch in cli.py is correct; two gaps remain: no ManagedSectionError-specific test for --single-agents, and managed_section silently skips sub-directory AGENTS.md with no user warning.
Supply Chain Security Expert 0 3 1 Three concerns: SecurityGate scans compiled output only (not final merged file); package end-marker injection persists poisoned AGENTS.md; distributed path lacks ensure_path_within on new read.
OSS Growth Hacker 0 1 1 Trust-critical fix for the managed_section power-user segment; CHANGELOG leaks internal implementation terms that blunt the user-impact story.
Doc Writer 0 2 2 Docs are not incorrect, but the managed-section page is missing the root-only scope constraint for distributed mode -- a silent data-loss risk for any user who reads the docs literally.
Test Coverage Expert 0 1 0 Both fix paths defended: AC-1 at unit tier (direct _write_distributed_file call), AC-5 at CLI e2e tier (CliRunner). One recommended gap: no compiler.compile() happy-path integration test for distributed managed_section.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Supply Chain Security Expert] Guard managed_section.py: raise ManagedSectionError if start_marker or end_marker appears in new_section_content -- This is the only finding in the panel with a concrete attacker-controlled content path crossing a trust boundary. A package author who controls consumed instruction content and knows the marker string can splice content into the human-authored region on first compile, producing AI agent instructions that appear user-written. The second compile then raises ManagedSectionError, creating a denial-of-service. Attack prerequisites are low: control a consumed package, know the marker string. Must land before managed_section is featured in any onboarding or quick-start material.

  2. [Doc Writer] Add root-only scope qualifier to the managed_section constraint lists in compile.md and manifest-schema.md -- doc-writer and devx-ux-expert converge independently: both pages make unqualified claims that managed_section preserves surrounding content, which is false for subdirectory AGENTS.md files under the distributed strategy. Any user who reads the docs literally and has hand-written content in a subdirectory file will lose it silently today. Requires only a single bullet addition in each page.

  3. [Test Coverage Expert] Add compiler.compile() integration test for distributed managed_section that asserts surrounding prose survives in the written root AGENTS.md -- Evidence outcome confirmed missing on a governed-by-policy surface. The existing AC-1 unit test calls _write_distributed_file directly; a refactor that bypasses it in _compile_distributed would leave AC-1 green while silently re-introducing the data-loss bug. A full compiler.compile() end-to-end test with markers and surrounding prose closes this regression-trap gap permanently.

  4. [Supply Chain Security Expert] Run SecurityGate.scan_text() on the full merged string after _write_output_file_with_config returns in managed_section mode -- The current scan runs only on the APM-compiled block before the managed_section merge. Human-authored sections in the preserved region -- which may contain hidden Unicode overrides -- pass unscanned into the final written file. One-line fix in cli.py; should land in the same patch as the marker-injection guard.

  5. [CLI Logging Expert] Split the except clause in cli.py into separate ManagedSectionError and OSError handlers with distinct error messages -- The unified Failed to write final AGENTS.md headline misframes managed-section config errors (missing markers, missing file) as I/O write failures, sending users toward disk and permissions remediation when the real cause is a manifest configuration issue.

Architecture

classDiagram
    direction LR

    class CompilationConfig {
        <<ValueObject>>
        +agents_md_mode str
        +agents_md_start_marker str
        +agents_md_end_marker str
        +strategy str
    }

    class AgentsCompiler {
        <<Orchestrator>>
        +base_dir Path
        +errors list
        +compile(config) CompilationResult
        -_compile_distributed(result, config)
        -_write_distributed_file(path, content, config)
        -_write_output_file_with_config(path, content, config, raise_write_errors)
        -_write_output_file(path, content)
    }

    class CompiledOutputWriter {
        <<IOBoundary>>
        +write(path, content) None
    }

    class managed_section_module {
        <<Module>>
        +apply_managed_section(existing, new, start, end) str
    }

    class ManagedSectionError {
        <<DomainError>>
    }

    class compile_cmd {
        <<CLIEntry>>
        +single_agents_write_path()
    }

    AgentsCompiler ..> CompilationConfig : reads
    AgentsCompiler ..> CompiledOutputWriter : delegates write
    AgentsCompiler ..> managed_section_module : calls
    AgentsCompiler ..> ManagedSectionError : catches
    compile_cmd ..> AgentsCompiler : delegates compile
    compile_cmd ..> CompiledOutputWriter : direct write full mode
    managed_section_module ..> ManagedSectionError : raises
    ManagedSectionError --|> ValueError

    class AgentsCompiler:::touched
    class compile_cmd:::touched

    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A([apm compile]) --> B{single_agents?}

    B -- no --> C[_compile_distributed loop\nagents_compiler.py:624]
    C --> D[_write_distributed_file\nagents_compiler.py:1557]
    D --> E{is_root AND mode\n== managed_section?\nline 1573}
    E -- yes --> F["_write_output_file_with_config\nline 1575, raise_write_errors=True"]
    E -- no --> G["CompiledOutputWriter.write\nline 1582"]

    B -- yes --> H[cli.py single-agents path\nline 705]
    H --> I{mode ==\nmanaged_section?\nline 706}
    I -- yes --> J["_write_output_file_with_config\nline 707, raise_write_errors=True"]
    I -- no --> K["CompiledOutputWriter.write\nline 714"]

    F --> L[apply_managed_section\nmanaged_section.py]
    J --> L
    L --> M{markers found?}
    M -- yes --> N["CompiledOutputWriter.write"]
    M -- no --> O[raise ManagedSectionError]

    O -- distributed --> P["except OSError,ManagedSectionError\nself.errors.append"]
    O -- cli path --> Q["except OSError,ValueError\nlogger.error + sys.exit 1"]
Loading
sequenceDiagram
    participant CLI as cli.py
    participant AC as AgentsCompiler
    participant WDF as _write_distributed_file
    participant WOWC as _write_output_file_with_config
    participant MS as managed_section.py
    participant COW as CompiledOutputWriter

    CLI->>AC: compile(config)
    AC->>AC: _compile_distributed(result, config)
    loop for each agents_path in content_map
        AC->>WDF: _write_distributed_file(path, content, config)
        WDF->>WDF: is_root = path.parent.resolve() == base_dir.resolve()
        alt is_root AND mode==managed_section
            WDF->>WOWC: _write_output_file_with_config(..., raise_write_errors=True)
            WOWC->>WOWC: target.read_text() existing content
            WOWC->>MS: apply_managed_section(existing, new, start, end)
            MS-->>WOWC: merged content OR raise ManagedSectionError
            WOWC->>COW: write(path, merged_content)
        else subdir OR mode==full
            WDF->>COW: write(agents_path, final_content)
        end
        alt OSError or ManagedSectionError raised
            AC->>AC: self.errors.append(msg)
        end
    end
    AC-->>CLI: CompilationResult
Loading

Recommendation

The fix is correct, the two critical acceptance-criteria paths are covered at the right test tiers, and no panelist raised a finding that requires changes before this lands. Open five follow-on issues before the next patch: (1) marker-injection guard in managed_section.py, (2) root-only scope qualifier in compile.md and manifest-schema.md, (3) compiler.compile() integration test for distributed managed_section, (4) SecurityGate re-scan on the merged file, (5) split except branches in cli.py for error message clarity. Items 1 and 4 are the two security follow-ons and should be prioritized in the same patch before managed_section is promoted in any quick-start or onboarding content. Item 2 can land as a docs-only commit immediately. Items 3 and 5 are cleanup and can ride the next regular cycle.


Full per-persona findings

Python Architect

  • [recommended] raise_write_errors boolean flag gives _write_output_file_with_config two incompatible error-reporting personalities at src/apm_cli/compilation/agents_compiler.py:1469
    The method now has two modes: when raise_write_errors=False (default), OSError is swallowed into self.errors; when raise_write_errors=True, OSError is raised to the caller. Boolean flags that invert error-handling semantics create dual-mode coupling: callers must know the flag to understand the method's contract, and a third caller would need to learn and choose a side. The cleaner long-term shape is to make the method always raise and push error accumulation into each call site's own except block.
    Suggested: In a follow-on PR, remove raise_write_errors, make _write_output_file_with_config always raise OSError or ManagedSectionError, and update the existing silent caller at line 677 to wrap with its own try/except self.errors.append. This removes the dual-mode coupling and makes each call site's error strategy explicit.

  • [nit] OSError from managed-section write gets double-wrapped when it travels through _write_distributed_file outer handler at src/apm_cli/compilation/agents_compiler.py:1584
    _write_output_file_with_config(raise_write_errors=True) raises OSError with a formatted message; that exception bubbles through _write_distributed_file's outer except OSError at line 1584, which re-wraps it. self.errors ends up with two 'Failed to write' phrases and the path twice.
    Suggested: Change the outer handler to except (OSError, ManagedSectionError) and re-raise unmodified (bare raise), reserving the wrapping message only for the non-root CompiledOutputWriter.write path.

  • [nit] cli.py mode-dispatch duplicates logic already encapsulated in _write_output_file_with_config at src/apm_cli/commands/compile/cli.py:706
    _write_output_file_with_config handles both full and managed_section internally. The if/else in cli.py rebuilds the same dispatch. When a third mode is added, cli.py becomes a third maintenance point.
    Suggested: Replace the if/else block with a single unconditional call: compiler._write_output_file_with_config(str(output_path), final_content, config, raise_write_errors=True).

  • [nit] Catch-clause inconsistency: cli.py uses ValueError, _compile_distributed uses ManagedSectionError at src/apm_cli/commands/compile/cli.py:717
    Both are correct since ManagedSectionError(ValueError), but the intent differs. Harmonising to (OSError, ManagedSectionError) makes handled types explicit.
    Suggested: from ...compilation.managed_section import ManagedSectionError; except (OSError, ManagedSectionError) as e:

CLI Logging Expert

  • [recommended] logger.error headline misframes managed-section failures as I/O write errors at src/apm_cli/commands/compile/cli.py:718
    cli.py line 718 emits Failed to write final AGENTS.md: {e} for every exception including ManagedSectionError. When the real cause is missing markers or a missing file, 'Failed to write' implies a disk/permissions problem and misdirects the user's remediation effort.
    Suggested: Split branches: except ManagedSectionError as e: logger.error(f"Managed-section write blocked: {e}") and except OSError as e: logger.error(f"Failed to write final AGENTS.md: {e}"). Add a logger.progress() hint before sys.exit(1) in the ManagedSectionError branch.

  • [recommended] [{target}] bracket prefix on re-raised ManagedSectionError mimics STATUS_SYMBOLS and duplicates path information at src/apm_cli/compilation/agents_compiler.py:1505
    When surfaced via logger.error, the full line reads: Failed to write final AGENTS.md: [AGENTS.md] Managed-section markers not found... -- the path appears twice and the bracket prefix looks like a status indicator rather than a scope prefix.
    Suggested: Drop the bracketed path prefix from the re-raise: raise ManagedSectionError(str(exc)) from exc.

  • [nit] except (OSError, ValueError) catches the base class where ManagedSectionError is the specific expected type at src/apm_cli/commands/compile/cli.py:717
    ManagedSectionError is the concrete exception expected; catching ValueError broadly could silently swallow unrelated ValueErrors.
    Suggested: from ...compilation.managed_section import ManagedSectionError; except (OSError, ManagedSectionError) as e:

  • [nit] Distributed write path uses identical 'Failed to write' framing for both OSError and ManagedSectionError at src/apm_cli/compilation/agents_compiler.py:629
    A user seeing this in result.errors cannot distinguish a disk failure from a marker configuration problem without reading the full exception body.

DevX UX Expert

  • [recommended] No --single-agents test exercises ManagedSectionError specifically (only OSError is patched) at tests/unit/compilation/test_managed_section.py:500
    ManagedSectionError IS caught today because it is a ValueError subclass, but the test gap means a future refactor of the exception clause would produce a silent regression on the original data-loss bug.
    Suggested: Add a test: write AGENTS.md without markers, invoke compile --single-agents with managed_section mode, assert exit_code != 0 and output contains the marker-absent hint string.

  • [recommended] managed_section silently bypasses sub-directory AGENTS.md files with no user-visible signal at src/apm_cli/compilation/agents_compiler.py:1573
    In distributed mode, managed_section is enforced only for the project-root AGENTS.md. Sub-directory AGENTS.md files are still fully overwritten. A user with hand-written content in a sub-directory AGENTS.md and managed_section mode set will silently lose that content.
    Suggested: When is_root is False and agents_md_mode is managed_section, emit at least a debug/verbose note: 'managed_section applies only to root AGENTS.md; {agents_path} will be fully overwritten.'

  • [nit] Distributed error message contains the file path twice when ManagedSectionError is raised at src/apm_cli/compilation/agents_compiler.py:1491
    _write_output_file_with_config embeds the target path in its ManagedSectionError messages. When recorded in _compile_distributed as 'Failed to write {agents_path}: {e!s}', the path appears twice.

Supply Chain Security Expert

  • [recommended] SecurityGate scans only compiled content, not the final merged file written in managed_section mode at src/apm_cli/commands/compile/cli.py
    In cli.py the SecurityGate scan runs on final_content (the APM-compiled block only) before _write_output_file_with_config is called. In managed_section mode that function reads the existing file and splices final_content into it. That combined output is never rescanned.
    Suggested: After _write_output_file_with_config returns, run SecurityGate.scan_text() on the full merged string rather than only on final_content.

  • [recommended] Package instruction content containing the end-marker string bypasses the duplicate-marker guard and can persist injected content in AGENTS.md at src/apm_cli/compilation/managed_section.py
    apply_managed_section counts start and end marker occurrences only in existing_content, not in new_section_content. If a package ships an instruction file whose body contains the default end-marker string, the first compile succeeds and splices it verbatim into the managed block. AI agents reading that file will see poisoned content as if it were human-authored. The second compile raises ManagedSectionError (denial-of-service).
    Suggested: Add a guard: if start_marker in new_section_content or end_marker in new_section_content: raise ManagedSectionError(...).

  • [recommended] _write_distributed_file introduces a new file READ without calling ensure_path_within, extending a pre-existing containment gap to reads at src/apm_cli/compilation/agents_compiler.py
    The is_root check compares agents_path.parent.resolve() == self.base_dir.resolve(). A symlink at <base_dir>/AGENTS.md pointing outside base_dir passes is_root=True, then the method reads and writes that external file. ensure_path_within already exists and handles this case.
    Suggested: Add ensure_path_within(agents_path, self.base_dir) as the first statement inside _write_distributed_file.

  • [nit] ManagedSectionError and OSError messages expose full absolute file paths verbatim in CI logs at src/apm_cli/compilation/agents_compiler.py
    In shared CI environments this reveals local filesystem layout. Use Path(output_path).name or portable_relpath() in error strings.

OSS Growth Hacker

  • [recommended] CHANGELOG uses two internal implementation terms instead of user-visible command language at CHANGELOG.md
    "the default distributed strategy" and "both write paths bypassed the managed-section helper" are code-level descriptions invisible to users. Only --single-agents is user-recognizable.
    Suggested: Replace with: 'in both plain apm compile and apm compile --single-agents. Previously the project-root AGENTS.md was silently overwritten on every compile run in both modes.'

  • [nit] compile.md Managed-section Constraints block carries no signal that both modes are now reliable
    A user who tried managed_section before this fix, got burned, and re-reads the docs still sees the same constraint list with no confidence signal.
    Suggested: Add: 'Both apm compile and apm compile --single-agents honour this setting as of v0.X.' Pin the version once the release tag is cut.

Auth Expert -- inactive

No auth surfaces touched; PR changes AGENTS.md write paths and managed_section logic only.

Doc Writer

  • [recommended] producer/compile.md managed-section constraints omit the distributed root-only scope at docs/src/content/docs/producer/compile.md
    The fix gates managed-section behaviour on is_root. Subdirectory AGENTS.md files are always fully replaced, even when mode: managed_section is set. The existing constraint list says 'Content outside the markers is preserved verbatim across every compile run' with no qualification. A user with hand-written content in a subdirectory AGENTS.md will have it silently overwritten.
    Suggested: Add a constraint bullet: 'In the distributed strategy, managed-section applies only to the project-root AGENTS.md. Subdirectory AGENTS.md files are always fully replaced on every compile, regardless of this setting. Hand-written content in subdirectory files is not preserved.'

  • [recommended] manifest-schema.md makes same unqualified claim about managed_section preserving surrounding content at docs/src/content/docs/reference/manifest-schema.md
    The manifest-schema table says managed_section 'replaces only the block between start_marker and end_marker, leaving surrounding content untouched' with no scope qualifier. Users who go straight to the schema reference will have no indication that the behaviour is root-only for distributed layouts.
    Suggested: Add scope qualifier: 'In the distributed strategy, managed-section is applied only to the project-root AGENTS.md; subdirectory AGENTS.md files are always fully replaced.'

  • [nit] raise_write_errors docstring understates exception surface (ManagedSectionError also propagates unconditionally) at src/apm_cli/compilation/agents_compiler.py
    The param docstring says 'Raise write OSError instead of recording it on self.errors'. However, ManagedSectionError also propagates regardless of the flag value.

  • [nit] CHANGELOG entry phrasing could mislead readers into thinking all AGENTS.md files respect managed_section at CHANGELOG.md
    The phrase 'both write paths bypassed' could be read as all AGENTS.md files now go through the helper, when only the root file does.

Test Coverage Expert

  • [recommended] AC-1 distributed managed-section happy path covered at unit tier only; integration-with-fixtures tier via compiler.compile() is absent at tests/unit/compilation/test_managed_section.py
    test_distributed_root_agents_md_preserves_human_content calls _write_distributed_file directly with real file I/O on tmp_path. A future refactor that bypasses _write_distributed_file in _compile_distributed would leave this test green while silently re-introducing the data-loss bug. No test exercises the full compiler.compile() -> _compile_distributed -> _write_distributed_file chain with valid root AGENTS.md and surrounding content surviving. Tests 4 and 5 DO call compiler.compile() with distributed + managed_section but test error paths only.
    Suggested: Add test_distributed_compile_root_preserves_managed_section: write root AGENTS.md with markers and surrounding prose, write one .instructions.md under .apm/instructions/, call compiler.compile(CompilationConfig(target='agents', strategy='distributed', agents_md_mode='managed_section', ...)), assert result.success and surrounding content survives in the written AGENTS.md.
    Proof (missing): tests/unit/compilation/test_managed_section.py::test_distributed_compile_root_preserves_managed_section -- proves: apm compile (distributed, managed_section) preserves human-written content through the full compiler.compile() orchestration layer. assert result.success; assert 'Hand-written intro.' in (tmp_path / 'AGENTS.md').read_text() [devx, portability-by-manifest]

Performance Expert -- inactive

No performance-sensitive surfaces touched; PR changes AGENTS.md write path logic only. The single new read_text() call is one-shot, sub-millisecond, and outside all four package-manager phases (resolve/fetch/materialize/verify).

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • pypi.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "pypi.org"

See Network Configuration for more information.

Generated by PR Review Panel for issue #1772 · sonnet46 16.5M ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label Jun 15, 2026
@sergio-sisternes-epam sergio-sisternes-epam added this pull request to the merge queue Jun 15, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 15, 2026
@danielmeppiel

Copy link
Copy Markdown
Collaborator

Thank you for this fix, @forcewake -- it correctly identified the managed_section regression in the distributed compile path, and the diagnosis here was sound.

We are consolidating the fix for #1764 on #1768, which landed full green CI and covers both root-cause paths (the distributed root AGENTS.md write and the --single-agents final-write path) with regression coverage on each. To avoid two PRs racing on the same files, closing this one as a duplicate of #1768.

Really appreciate the contribution -- please keep them coming.

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.

[BUG] managed_section ignored when compilation.strategy=distributed (default)

4 participants