fix: preserve managed sections in distributed compile#1772
Conversation
There was a problem hiding this comment.
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.mdwrites through_write_output_file_with_config()whenagents_md_mode == "managed_section"in the distributed write path. - Route
--single-agentsfinal write through_write_output_file_with_config()whenagents_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). |
|
@microsoft-github-policy-service agree |
7d30581 to
577b5a0
Compare
577b5a0 to
9d95d2b
Compare
|
@copilot review please one more time |
APM Review Panel:
|
| 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
-
[Supply Chain Security Expert] Guard
managed_section.py: raiseManagedSectionErrorifstart_markerorend_markerappears innew_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 raisesManagedSectionError, 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. -
[Doc Writer] Add root-only scope qualifier to the managed_section constraint lists in
compile.mdandmanifest-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. -
[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_filedirectly; a refactor that bypasses it in_compile_distributedwould leave AC-1 green while silently re-introducing the data-loss bug. A fullcompiler.compile()end-to-end test with markers and surrounding prose closes this regression-trap gap permanently. -
[Supply Chain Security Expert] Run
SecurityGate.scan_text()on the full merged string after_write_output_file_with_configreturns 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 incli.py; should land in the same patch as the marker-injection guard. -
[CLI Logging Expert] Split the except clause in
cli.pyinto separateManagedSectionErrorandOSErrorhandlers with distinct error messages -- The unifiedFailed to write final AGENTS.mdheadline 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
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"]
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
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_configtwo incompatible error-reporting personalities atsrc/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_configalways 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_fileouter handler atsrc/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 toexcept (OSError, ManagedSectionError)and re-raise unmodified (bareraise), reserving the wrapping message only for the non-root CompiledOutputWriter.write path. -
[nit]
cli.pymode-dispatch duplicates logic already encapsulated in_write_output_file_with_configatsrc/apm_cli/commands/compile/cli.py:706
_write_output_file_with_confighandles 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.pyusesValueError,_compile_distributedusesManagedSectionErroratsrc/apm_cli/commands/compile/cli.py:717
Both are correct sinceManagedSectionError(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.errorheadline misframes managed-section failures as I/O write errors atsrc/apm_cli/commands/compile/cli.py:718
cli.pyline 718 emitsFailed 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}")andexcept 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 atsrc/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 atsrc/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-agentstest exercises ManagedSectionError specifically (only OSError is patched) attests/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, invokecompile --single-agentswith 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_configembeds the target path in its ManagedSectionError messages. When recorded in_compile_distributedas '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_configis 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_configreturns, runSecurityGate.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_sectioncounts start and end marker occurrences only inexisting_content, not innew_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: ifstart_marker in new_section_content or end_marker in new_section_content: raise ManagedSectionError(...). -
[recommended]
_write_distributed_fileintroduces a new file READ without callingensure_path_within, extending a pre-existing containment gap to reads atsrc/apm_cli/compilation/agents_compiler.py
The is_root check comparesagents_path.parent.resolve() == self.base_dir.resolve(). A symlink at<base_dir>/AGENTS.mdpointing outside base_dir passes is_root=True, then the method reads and writes that external file.ensure_path_withinalready exists and handles this case.
Suggested: Addensure_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. UsePath(output_path).nameorportable_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-agentsis user-recognizable.
Suggested: Replace with: 'in both plainapm compileandapm compile --single-agents. Previously the project-rootAGENTS.mdwas silently overwritten on every compile run in both modes.' -
[nit]
compile.mdManaged-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: 'Bothapm compileandapm compile --single-agentshonour 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.mdmanaged-section constraints omit the distributed root-only scope atdocs/src/content/docs/producer/compile.md
The fix gates managed-section behaviour onis_root. Subdirectory AGENTS.md files are always fully replaced, even whenmode: managed_sectionis 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.mdmakes same unqualified claim about managed_section preserving surrounding content atdocs/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_errorsdocstring understates exception surface (ManagedSectionError also propagates unconditionally) atsrc/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 attests/unit/compilation/test_managed_section.py
test_distributed_root_agents_md_preserves_human_contentcalls_write_distributed_filedirectly with real file I/O on tmp_path. A future refactor that bypasses_write_distributed_filein_compile_distributedwould leave this test green while silently re-introducing the data-loss bug. No test exercises the fullcompiler.compile()->_compile_distributed->_write_distributed_filechain with valid root AGENTS.md and surrounding content surviving. Tests 4 and 5 DO callcompiler.compile()with distributed + managed_section but test error paths only.
Suggested: Addtest_distributed_compile_root_preserves_managed_section: write root AGENTS.md with markers and surrounding prose, write one.instructions.mdunder.apm/instructions/, callcompiler.compile(CompilationConfig(target='agents', strategy='distributed', agents_md_mode='managed_section', ...)), assertresult.successand surrounding content survives in the writtenAGENTS.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.allowedlist 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 · ◷
# Conflicts: # CHANGELOG.md
|
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. |
Summary
apm compilepath so rootAGENTS.mdhonorscompilation.agents_md.mode: managed_sectioninstead of overwriting the whole file.--single-agentsCLI write path through the same managed-section helper.--single-agents, and path normalization.Closes #1764.
Changes
src/apm_cli/compilation/agents_compiler.pyAGENTS.mdin the distributed writer._write_output_file_with_config()helper.AGENTS.mdoutputs.src/apm_cli/commands/compile/cli.py--single-agents+managed_sectionthrough the same helper.tests/unit/compilation/test_managed_section.pyCHANGELOG.mdValidation
Ran locally before commit/push:
Additional review:
/ponytail ultraproduced the implementation.claude-opus-4.8independently reviewed the diff and returnedAPPROVED; Copilot's sandbox could not executeuv/pytest/ruff, so the commands above were rerun directly afterward.Notes
AGENTS.mdfiles remain generated/full-overwrite files.