Skip to content

fix: propagate target to intermediate CompilationConfig in single-file path#1355

Open
sergio-sisternes-epam wants to merge 2 commits into
microsoft:mainfrom
sergio-sisternes-epam:issue/765
Open

fix: propagate target to intermediate CompilationConfig in single-file path#1355
sergio-sisternes-epam wants to merge 2 commits into
microsoft:mainfrom
sergio-sisternes-epam:issue/765

Conversation

@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator

@sergio-sisternes-epam sergio-sisternes-epam commented May 16, 2026

Description

When compiling with --target codex --single-agents, the intermediate CompilationConfig in cli.py was constructed without target=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_config at cli.py:699-706 was missing target=config.target. Without it, CompilationConfig.target defaulted to "all", causing should_compile_claude_md("all") to return True and _compile_claude_md() to run even when targeting codex.

Fix: Added target=config.target, to the CompilationConfig(...) constructor in the single-file compilation path (line 706). Added CLI-level regression test using CliRunner that exercises the exact fixed code path.

Fixes #765

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added tests for new functionality (if applicable)

77 tests in test_compile_target_flag.py pass (76 existing + 1 new). Full unit suite passes. Lint clean (ruff check + ruff format --check).

…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>
Copilot AI review requested due to automatic review settings May 16, 2026 11:46
@sergio-sisternes-epam sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label May 16, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.target into the intermediate CompilationConfig.
  • 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.

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 16, 2026
@github-actions
Copy link
Copy Markdown

APM Review Panel: ship_with_followups

Correct one-line fix for #765; ship after CHANGELOG entry and two test repairs -- a dataclasses.replace() follow-up will close the recurrence surface permanently.

cc @sergio-sisternes-epam @danielmeppiel -- a fresh advisory pass is ready for your review.

The fix itself is unambiguously correct: propagating target=config.target to the intermediate CompilationConfig in the single-file path restores the user promise that --target codex --single-agents suppresses Claude preview output. Supply chain and auth surfaces are clean. The regression test is a genuine trap -- it exercises the actual fixed code path -- so the contribution clears the minimum bar for a bug fix with coverage.

Two test-quality issues need resolution before or immediately after merge. The cli-logging-expert identified that assert 'Would generate' not in result.output is self-defeating: formatters.py emits exactly that string on every --dry-run run, so the assertion either fails spuriously or passes vacuously depending on execution order. The fix is narrow: replace with assert 'CLAUDE.md Preview: Would generate' not in result.output or drop it entirely since the CLAUDE.md Preview guard above it already covers the regression. The test-coverage-expert's tier-gap finding (CliRunner vs. real subprocess) is non-blocking -- the unit test does exercise the fixed code path -- but an integration test following test_compile_copilot_root_instructions.py pattern should be filed as a follow-up issue rather than held as a blocker.

The python-architect's dataclasses.replace() recommendation is the highest-signal architectural finding in the panel: the manual field-copy pattern that caused this bug will silently recur every time a new field is added to CompilationConfig. That refactor is not a blocker for this PR, but it should be filed as a tracked issue and referenced in the CHANGELOG entry -- it transforms a point fix into a durable closure. The doc-writer's CHANGELOG finding is an easy pre-merge action: one line under [Unreleased] ### Fixed referencing both flags, the issue, and the PR.

Aligned with: Multi-harness/multi-host -- this fix is a direct fidelity signal for the Codex/OpenAI harness: users of --target codex received silently wrong output. Shipping with a named changelog entry closes a "does APM actually support Codex?" doubt loop.

Growth signal. An external EPAM contributor root-caused a cross-harness pollution bug and shipped with a regression test -- this is the canonical contributor arc worth amplifying. Name the fix in release notes as a Codex-target fidelity fix, credit the contributor explicitly, and consider referencing this PR in the contributing guide as a model "good bug fix" example. The oss-growth-hacker's note is concrete and low-cost to act on.

Panel summary

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

  1. [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.
  2. [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.
  3. [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.
  4. [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
Loading
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
Loading

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_replace then intermediate_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 the or '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: Replace assert 'Would generate' not in result.output with assert '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 the or branch.

  • [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 issue apm compile --target codex --single-agents appends CLAUDE.md Preview to 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 in tests/integration/ using subprocess.run([sys.executable, '-m', 'apm_cli.cli', 'compile', '--target', 'codex', '--single-agents', '--dry-run'], ...) following the pattern in tests/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.output assertion 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>
@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator Author

Panel feedback addressed in b763020c

All actionable findings from the review panel have been resolved:

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.

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.

apm compile --target codex --single-agents appends CLAUDE.md Preview to the generated AGENTS output

2 participants