fix: propagate target to intermediate CompilationConfig in single-file path#1355
fix: propagate target to intermediate CompilationConfig in single-file path#1355sergio-sisternes-epam wants to merge 2 commits into
Conversation
…e path When compiling with --target codex --single-agents, the intermediate CompilationConfig was missing target=config.target, defaulting to 'all'. This caused _compile_claude_md() to run and append Claude preview text to the AGENTS output. Add target=config.target to the constructor and a CLI-level regression test that exercises the fixed code path. Fixes microsoft#765 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes target propagation in the single-file compile path so apm compile --target codex --single-agents does not accidentally run default "all" target behavior and append Claude preview output.
Changes:
- Propagates
config.targetinto the intermediateCompilationConfig. - Adds a CLI regression test for issue #765 covering
--target codex --single-agents --dry-run.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/apm_cli/commands/compile/cli.py |
Preserves the selected target when building the intermediate single-file dry-run config. |
tests/unit/compilation/test_compile_target_flag.py |
Adds regression coverage for Codex single-agents output avoiding Claude preview artifacts. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | One-line fix is correct; root cause is a fragile manual copy-constructor pattern on CompilationConfig that will recur as fields are added. |
| CLI Logging Expert | 0 | 1 | 2 | Bug fix is correct and silent; regression test has a self-defeating assertion that bans the normal dry-run summary line. |
| DevX UX Expert | 0 | 0 | 1 | Bug fix correctly restores flag honesty: --target codex now suppresses Claude output as expected. No UX regressions. |
| Supply Chain Security Expert | 0 | 0 | 0 | No supply-chain, dependency, token, or path-safety concerns. 1-line config propagation fix with no security surface. |
| OSS Growth Hacker | 0 | 1 | 1 | Silent cross-harness pollution bug fixed; worth a release note callout to signal Codex/OpenAI target fidelity and reward an external contributor. |
| Doc Writer | 0 | 1 | 0 | Bug fix corrects user-visible compile output but no CHANGELOG entry was added; the Unreleased Fixed section has a clear pattern for this. |
| Test Coverage Expert | 0 | 1 | 1 | Regression trap present but at unit tier (CliRunner); CLI command surface floor is integration-with-fixtures. One tier-gap finding. |
| Auth Expert | -- | -- | -- | No auth surface touched; inactive. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 4 follow-ups
- [Doc Writer] Add CHANGELOG entry under [Unreleased] ### Fixed for the --target codex --single-agents fix -- Every user-visible behavior correction in this project is documented; omitting it leaves Codex users unable to find the fix in release history. One line, pre-merge.
- [CLI Logging Expert] Narrow or remove the 'Would generate' not-in-output assertion in test_compile_target_flag.py -- The assertion conflicts with the dry-run formatter's normal output string and is either self-defeating or vacuous; the CLAUDE.md Preview guard above it already covers the regression intent.
- [Python Architect] File a tracked issue to refactor intermediate CompilationConfig construction to use dataclasses.replace() -- The manual field-copy pattern is the root cause of this bug class; the same omission exists at agents_compiler.py:1259 and will recur with every new CompilationConfig field.
- [Test Coverage Expert] Add integration test in tests/integration/ for --target codex --single-agents using real subprocess -- The CliRunner unit test is a genuine trap but does not cover the real subprocess invocation path; the missing-coverage gap on a CLI surface floor is a regression risk.
Architecture
classDiagram
direction LR
class CompilationConfig {
<<Dataclass>>
+output_path str
+chatmode str
+resolve_links bool
+dry_run bool
+with_constitution bool
+target CompileTargetType
+strategy str
+single_agents bool
+trace bool
+local_only bool
+debug bool
+source_attribution bool
+exclude list
+from_apm_yml() CompilationConfig
}
class AgentsCompiler {
<<Service>>
+compile(config) CompilationResult
-_compile_agents_md(config, primitives) CompilationResult
-_compile_claude_md(config, primitives) CompilationResult
-_compile_gemini_md(config, primitives) CompilationResult
}
class CompilationResult {
<<ValueObject>>
+success bool
+content str
+output_path str
+errors list
+stats dict
}
class compile_cmd {
<<CLIEntryPoint>>
+invoke(target, single_agents, dry_run, ...) None
}
class ConstitutionInjector {
<<Service>>
+inject(content, with_constitution, output_path) tuple
}
compile_cmd ..> CompilationConfig : constructs (line 583 + line 699)
compile_cmd ..> AgentsCompiler : invokes
compile_cmd ..> ConstitutionInjector : uses
AgentsCompiler ..> CompilationConfig : reads target
AgentsCompiler ..> CompilationResult : returns
class CompilationConfig:::touched
class compile_cmd:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm compile --target codex --single-agents --dry-run"]) --> B["cli.py: compile_cmd()\nline 583: CompilationConfig.from_apm_yml()\\ config.target = 'codex'"];
B --> C{"config.single_agents or\nstrategy == single-file?"};
C -- yes --> D["line 699: intermediate_config = CompilationConfig(\n ...,\n dry_run=True,\n strategy='single-file',\n target=config.target <-- THIS PR FIX"];
D --> E["AgentsCompiler.compile(intermediate_config)\nagents_compiler.py line 213"];
E --> F{"should_compile_claude_md(\n routing_target='codex'\n)?"};
F -- no, codex routes agents only --> G["_compile_agents_md() --> AGENTS.md content"];
F -- BUG: 'all' default before fix --> H["_compile_claude_md() appends Claude preview text"];
G --> I["intermediate_result.content"];
I --> J["ConstitutionInjector.inject()"];
J --> K{"dry_run?"};
K -- yes --> L["_display_single_file_summary() -- no write"];
K -- no --> M["SecurityGate.scan_text() then write output_path"];
style H fill:#ffdddd,stroke:#cc0000
style D fill:#fff3b0,stroke:#d47600
Recommendation
Merge after: (1) adding the one-line CHANGELOG entry under [Unreleased] ### Fixed, and (2) narrowing the 'Would generate' assertion in the regression test. The dataclasses.replace() refactor and the integration-tier test gap should be filed as follow-up issues -- reference them in the merge commit or PR description so they are tracked. No panelist raised a blocking finding; the fix is correct, the regression test is genuine, and the contributor delivered cleanly.
Full per-persona findings
Python Architect
-
[recommended] Intermediate CompilationConfig uses manual field copy; missing-field bugs will recur as the dataclass grows at
src/apm_cli/commands/compile/cli.py:699
The intermediate_config in cli.py is built by manually re-listing most (but not all) of CompilationConfig's fields. This PR fixes the omitted target field, but the same pattern leaves other fields silently defaulted. The correct fix is dataclasses.replace(config, dry_run=True, strategy='single-file') which copies all fields by value and overrides only what changes.
Suggested: Replace with:from dataclasses import replace as dc_replacethenintermediate_config = dc_replace(config, dry_run=True, strategy="single-file") -
[nit] compile_agents_md() helper in agents_compiler.py also omits target from its CompilationConfig at
src/apm_cli/compilation/agents_compiler.py:1259
A separate call site with the same pattern.
Suggested: Thread target through as a parameter if the helper is ever called from a target-aware context. -
[nit] Regression test assertion for AGENTS.md reference is too loose at
tests/unit/compilation/test_compile_target_flag.py:196
'agents' in result.output.lower() will match incidental strings like 'single-agents' in the CLI flag echo.
Suggested: Change to:assert 'AGENTS.md' in result.output(drop theor 'agents' in result.output.lower()clause).
CLI Logging Expert
-
[recommended] Regression test asserts 'Would generate' must NOT appear, but that string IS the normal dry-run summary from formatters.py at
tests/unit/compilation/test_compile_target_flag.py
formatters.py emits '[DRY RUN] Would generate N codex file(s)' on every --dry-run run. The assertion 'assert Would generate not in result.output' therefore either causes the test to fail spuriously, or passes only because the test exits before reaching the formatter. Author's intent was to guard against 'CLAUDE.md Preview: Would generate ...' bleed-through. The fix should narrow the assertion to the specific Claude-preview substring.
Suggested: Replaceassert 'Would generate' not in result.outputwithassert 'CLAUDE.md Preview: Would generate' not in result.output(or remove it -- the CLAUDE.md Preview guard above already covers the regression). -
[nit] 'agents' in result.output.lower() fallback is vacuously loose -- will match unrelated words at
tests/unit/compilation/test_compile_target_flag.py
The fallback is so broad it will match almost any compile output.
Suggested: Pin to:assert 'AGENTS.md' in result.output. Drop theorbranch. -
[nit] Bug fix itself produces no change in user-visible CLI output -- correct and expected
Adding target=config.target to the intermediate CompilationConfig is a pure logic fix. The only output change is the absence of the erroneous Claude preview text, which is the desired outcome.
DevX UX Expert
- [nit] Positive test assertion is underspecified at
tests/unit/compilation/test_compile_target_flag.py
The assertion is so broad it would pass even if the command printed only a log line containing the word 'agents'. The negative assertion is the load-bearing check.
Suggested: Assert a more specific token that confirms codex output was produced, e.g. the expected filename pattern or a 'Would write' line that references AGENTS.md explicitly.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
-
[recommended] Call this out in release notes as a named fix for Codex/OpenAI users
Users hitting --target codex --single-agents would silently get Claude preview text appended to AGENTS.md. A named fix in the changelog directly counters 'this tool doesn't actually support Codex' word-of-mouth and signals multi-harness maturity.
Suggested: CHANGELOG entry should explicitly name both flags (--target codex --single-agents) and reference issueapm compile --target codex --single-agentsappendsCLAUDE.md Previewto the generated AGENTS output #765. Frame as 'multi-harness correctness' not just a config bug. -
[nit] Contributor angle: external EPAM contributor with a regression test is a funnel win worth amplifying
sergio-sisternes-epam filed the bug, isolated the root cause, and shipped a regression test with issue reference in the docstring. A maintainer thank-you on the PR costs 30 seconds and shows up in contributor analytics.
Suggested: Post a brief 'thank you + this is the pattern we want' reply. Consider adding this PR as an example in CONTRIBUTING.md.
Doc Writer
- [recommended] Missing CHANGELOG entry for the --target codex --single-agents bug fix at
CHANGELOG.md
CHANGELOG.md follows Keep a Changelog with an active [Unreleased] ### Fixed section. Every user-visible behavior correction in this project is documented there. This fix corrects wrong output observable by users.
Suggested: Add under [Unreleased] ### Fixed:- apm compile --target codex --single-agents no longer appends Claude preview text to AGENTS output; the intermediate CompilationConfig now correctly inherits the target from the parent config. (#1355, closes #765)
Test Coverage Expert
-
[recommended] Regression test uses CliRunner (unit tier); CLI command surface floor requires integration-with-fixtures at
tests/unit/compilation/test_compile_target_flag.py:181
CliRunner invokes the CLI function in-process without spawning a real subprocess. Existing integration tests in tests/integration/ use sys.executable -m apm_cli.cli as a real subprocess -- none cover --target codex --single-agents. The CliRunner test IS a genuine regression trap but the integration tier gap remains.
Suggested: Add an integration test intests/integration/usingsubprocess.run([sys.executable, '-m', 'apm_cli.cli', 'compile', '--target', 'codex', '--single-agents', '--dry-run'], ...)following the pattern intests/integration/test_compile_copilot_root_instructions.py.
Proof (passed):tests/unit/compilation/test_compile_target_flag.py::TestCompileTargetRouting::test_codex_single_agents_no_claude_preview_issue_765_cli-- proves: apm compile --target codex --single-agents must not append Claude preview text to AGENTS output -
[nit] Second negative assertion ('Would generate' not in output) may be fragile or vacuous at
tests/unit/compilation/test_compile_target_flag.py:221
Grepping cli.py for 'Would generate' returns no hits in the compile output path. If this string never appears in normal output, the assertion adds no signal. The 'CLAUDE.md Preview' assertion is the load-bearing one.
Suggested: Remove the'Would generate' not in result.outputassertion or replace with an assertion that the output_path references AGENTS.md and not CLAUDE.md.
Auth Expert -- inactive
The changed files (src/apm_cli/commands/compile/cli.py and tests/unit/compilation/test_compile_target_flag.py) touch only compile CLI config propagation and have no overlap with auth, token management, credential resolution, or remote-host fallback surfaces.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1355 · ● 1.4M · ◷
- Add CHANGELOG entry for target propagation fix - Tighten test assertions for compilation output - Add TODO for dataclasses.replace() refactor Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Panel feedback addressed in
|
| Finding | Status | Fix |
|---|---|---|
| [R] Missing CHANGELOG entry | Fixed | Added under [Unreleased] ### Fixed referencing both flags and issue #765 |
[R] 'Would generate' not in output assertion self-defeating |
Fixed | Narrowed to 'CLAUDE.md Preview: Would generate' not in result.output |
| [R] Integration test tier gap | Noted | Filed as follow-up (unit test IS a genuine regression trap) |
| [N] Inline comment explaining target propagation | Fixed | Added comment at line 699 explaining why target=config.target is needed |
[N] dataclasses.replace() refactor opportunity |
Noted | Valid follow-up; tracked separately |
All Copilot review threads resolved. CI green, lint clean.
Description
When compiling with
--target codex --single-agents, the intermediateCompilationConfigincli.pywas constructed withouttarget=config.target, defaulting to"all". This caused_compile_claude_md()to run and append Claude preview text to the AGENTS output.Root cause: The
intermediate_configatcli.py:699-706was missingtarget=config.target. Without it,CompilationConfig.targetdefaulted to"all", causingshould_compile_claude_md("all")to returnTrueand_compile_claude_md()to run even when targeting codex.Fix: Added
target=config.target,to theCompilationConfig(...)constructor in the single-file compilation path (line 706). Added CLI-level regression test usingCliRunnerthat exercises the exact fixed code path.Fixes #765
Type of change
Testing
77 tests in
test_compile_target_flag.pypass (76 existing + 1 new). Full unit suite passes. Lint clean (ruff check+ruff format --check).