fix: resolve 3 flaky integration tests blocking merge queue

[sergio-sisternes-epam]

Problem

The merge queue has a 0% success rate since May 24 (5 consecutive failures across PRs #1473 and #1465). Three independent bugs cause integration test failures that only manifest in the merge queue environment.

Fixes

Bug 1: config.py race condition (production code)

• Root cause: os.makedirs(CONFIG_DIR) without exist_ok=True causes FileExistsError when parallel xdist workers initialise ~/.apm simultaneously.
• Fix: Use exist_ok=True for makedirs and atomic O_CREAT|O_EXCL for config file creation to eliminate the TOCTOU race.

Bug 2: Copilot CLI rejects merge queue token (100% repro)

• Root cause: RUNTIME_ENV_VARS['copilot'] only maps to GH_TOKEN and GITHUB_PERSONAL_ACCESS_TOKEN, missing COPILOT_GITHUB_TOKEN which is Copilot CLI's preferred env var.
• Fix: Add COPILOT_GITHUB_TOKEN to RUNTIME_ENV_VARS['copilot'] in token_manager.py. Also set it explicitly in the golden scenario test from GITHUB_APM_PAT or GITHUB_TOKEN.

Bug 3: Skill install timeouts (intermittent)

• Root cause: 120s timeout is too tight for CI with network latency and GitHub API rate limiting.
• Fix: Increase subprocess timeout from 120s to 300s in test_skill_install.py and test_skill_integration.py.

Validation

• All lints pass (ruff check, ruff format, pylint R0801, auth-signals)
• All 14,981 unit tests pass
• 270 token-related tests pass

Files changed

• src/apm_cli/config.py -- race-safe config initialisation
• src/apm_cli/core/token_manager.py -- add COPILOT_GITHUB_TOKEN to runtime env vars
• tests/integration/test_golden_scenario_e2e.py -- explicit Copilot token env var
• tests/integration/test_skill_install.py -- timeout 120s -> 300s
• tests/integration/test_skill_integration.py -- timeout 120s -> 300s

Fixes merge queue failures for #1473 and #1465.

[Copilot]

Pull request overview

This PR aims to restore merge-queue stability by fixing three merge-queue-specific integration test failures: a config initialization race under parallelism, Copilot CLI token env-var compatibility, and CI timeouts during skill installs.

Changes:

• Make ~/.apm config initialization race-safe under parallel xdist workers.
• Teach token setup to export the Copilot CLI-preferred COPILOT_GITHUB_TOKEN, and set it explicitly in the golden E2E scenario.
• Increase integration test subprocess timeouts from 120s to 300s to reduce CI flakiness.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/apm_cli/config.py	Makes config directory/file creation safe under concurrent initialization.
src/apm_cli/core/token_manager.py	Adds COPILOT_GITHUB_TOKEN to Copilot runtime env-var mappings.
tests/integration/test_golden_scenario_e2e.py	Ensures Copilot CLI sees a token under COPILOT_GITHUB_TOKEN during the golden scenario.
tests/integration/test_skill_install.py	Extends install-related subprocess timeouts to reduce CI flakes.
tests/integration/test_skill_integration.py	Extends integration-related subprocess timeouts to reduce CI flakes.
apm.lock.yaml	Updates the repo lockfile contents/version metadata.

[github-actions]

APM Review Panel: ship_with_followups

Three correct bug fixes restore the merge queue from 0% success rate; the lockfile change that carried the supply-chain blocking signal was removed in a second commit -- all remaining panel signals are non-blocking.

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

The three code fixes (config.py atomic init, COPILOT_GITHUB_TOKEN in RUNTIME_ENV_VARS, 300s test timeouts) are narrow, correct, and urgent. All specialists agree on the code changes. Python-architect, cli-logging-expert, devx-ux-expert, and auth-expert found only nits against those three files.

The supply-chain-security expert raised a blocking finding on the initial diff (removal of local_deployed_file_hashes from apm.lock.yaml, which would have silently degraded SHA-256 provenance for all locally deployed agent and instruction files). A second commit removed the lockfile change entirely, resolving that concern. The lockfile finding is now moot; the current diff is clean from a supply-chain standpoint.

Two remaining signals worth tracking before merge closes: (1) the test-coverage-expert found no unit test asserting COPILOT_GITHUB_TOKEN lands in the child-process env dict after resolution -- a silent regression trap if the RUNTIME_ENV_VARS prepend is ever reverted; and (2) the FileExistsError-swallow path in ensure_config_exists has no test, meaning a future refactor removing the try/except would silently regress parallel xdist runs. Both are quick unit tests recommended as immediate follow-ups.

The auth-expert surfaced a pre-existing gap: COPILOT_GITHUB_TOKEN is correct in RUNTIME_ENV_VARS (destination side) but absent from TOKEN_PRECEDENCE['copilot'] (source side). A user who sets COPILOT_GITHUB_TOKEN directly without any TOKEN_PRECEDENCE var will get a silent "no token found" from APM while Copilot CLI proceeds anyway. Not introduced by this PR; recommended as a follow-up patch.

Aligned with: Pragmatic-as-npm (atomic dir init, correct env var propagation, and adjusted CI timeouts are pragmatic correctness fixes with no user-facing complexity cost); Secure-by-default (improved, now that the lockfile integrity concern is resolved by removing the lockfile change from scope).

Growth signal. The oss-growth-hacker notes that a 0% merge queue success rate since May 24 is a top-of-funnel contributor trust event -- contributors who opened PRs in that window and watched them fail repeatedly may have churned silently. The fix is urgent and correct. A CHANGELOG entry under 0.14.2 naming the outage window (May 24-25) converts an invisible fix into a visible trust signal. The COPILOT_GITHUB_TOKEN addition is also worth one sentence in the release notes: "apm now works out of the box in Copilot CLI environments without manual token wiring."

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	3	Three independent bug fixes; all narrow and correct. Atomic config init pattern is sound for xdist multi-process case.
CLI Logging Expert	0	0	2	No CLI output regressions. FileExistsError swallow is semantically correct; debug log on race branch would improve agent-mode traceability.
DevX UX Expert	0	0	2	No UX regressions. Config race fix preserves first-run UX. COPILOT_GITHUB_TOKEN addition is a positive discoverability improvement.
Supply Chain Security Expert	0	1	1	Lockfile blocking finding resolved by second commit. One remaining: test subprocess output may expose token value on failure -- scrub before print.
OSS Growth Hacker	0	1	2	Merge queue restoration is a contributor-trust event. CHANGELOG entry and release note recommended.
Auth Expert	0	1	1	COPILOT_GITHUB_TOKEN placement in RUNTIME_ENV_VARS is correct. TOKEN_PRECEDENCE['copilot'] gap is pre-existing and worth a follow-up patch.
Doc Writer	--	--	--	inactive -- no documentation files in diff; COPILOT_GITHUB_TOKEN is an internal runtime mapping with no user-facing CLI surface requiring docs update.
Test Coverage Expert	0	2	1	Two missing unit tests: COPILOT_GITHUB_TOKEN env dict population and FileExistsError-swallow path. Both recommended as immediate follow-ups.

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

Top 5 follow-ups

1. [OSS Growth Hacker] Add CHANGELOG entry under 0.14.2 naming the merge queue outage window (May 24-25) and confirming resolution -- contributor trust requires a visible post-mortem record; external contributors blocked during that window deserve acknowledgment.
2. [Auth Expert] Add COPILOT_GITHUB_TOKEN to TOKEN_PRECEDENCE['copilot'] as a low-priority fallback -- a user who sets this var directly without any TOKEN_PRECEDENCE var currently gets a silent "no token found" from APM; not introduced by this PR but surfaced by it.
3. [Test Coverage Expert] Add unit test asserting COPILOT_GITHUB_TOKEN is populated in the copilot runtime env dict after token resolution -- if the RUNTIME_ENV_VARS prepend is ever reverted, no test alarm fires and copilot skill runs silently fail to receive the token.
4. [Test Coverage Expert] Add unit test for the FileExistsError-swallow path in ensure_config_exists -- a future refactor removing the try/except would silently regress parallel xdist init runs; one-liner with patch('os.open', side_effect=FileExistsError).
5. [Supply Chain Security Expert] Scrub subprocess output against known token values before printing on test failure in test_golden_scenario_e2e.py -- COPILOT_GITHUB_TOKEN is injected from GITHUB_APM_PAT and any Copilot CLI debug output that echoes the token would appear in CI logs.

Architecture

classDiagram
    direction LR

    class config {
        <<Module>>
        +CONFIG_DIR str
        +CONFIG_FILE str
        +ensure_config_exists() None
        +get_config() dict
        +set_config(data) None
    }

    class GitHubTokenManager {
        <<Strategy>>
        +RUNTIME_ENV_VARS dict
        +preserve_existing bool
        +get_token_for_purpose(purpose, tokens) str
        +_setup_copilot_tokens(env, tokens) None
        +_setup_codex_tokens(env, tokens) None
    }

    class RUNTIME_ENV_VARS {
        <<ValueObject>>
        +copilot list
        +codex list
        +llm list
    }

    GitHubTokenManager *-- RUNTIME_ENV_VARS : owns
    note for config "Bug 1 fix: makedirs(exist_ok=True)\n+ O_CREAT|O_EXCL atomic init"
    note for RUNTIME_ENV_VARS "Bug 2 fix: prepend COPILOT_GITHUB_TOKEN\nto copilot list"

    class config:::touched
    class RUNTIME_ENV_VARS:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A([xdist worker N starts]) --> B[ensure_config_exists]
    B --> C["[FS] os.makedirs(CONFIG_DIR, exist_ok=True)"]
    C --> D{os.path.exists CONFIG_FILE?}
    D -- exists --> E([return])
    D -- absent --> F["[FS] os.open(O_WRONLY|O_CREAT|O_EXCL, 0o644)"]
    F -- success --> G["[FS] fdopen + json.dump default config"]
    G --> E
    F -- FileExistsError --> H[another worker won race]
    H --> E

    subgraph token_manager.py
        I([_setup_copilot_tokens called]) --> J[get_token_for_purpose copilot]
        J --> K{token found?}
        K -- no --> L([return])
        K -- yes --> M["iterate RUNTIME_ENV_VARS copilot\nCOPILOT_GITHUB_TOKEN, GH_TOKEN,\nGITHUB_PERSONAL_ACCESS_TOKEN"]
        M --> N["[ENV] set each var = token"]
        N --> O([env dict ready for subprocess])
    end

Loading

Recommendation

Ship it. The three code fixes are correct, narrow, and urgent -- a 0% merge queue success rate since May 24 is a blocker on every contributor PR in flight. The second commit cleanly removed the lockfile change that carried the supply-chain blocking signal. Track the five recommended follow-ups as issues: the TOKEN_PRECEDENCE gap and the two missing unit tests are the highest-signal items and can land in the next patch without blocking this fix.

---

Full per-persona findings

Python Architect

• [nit] Outer os.path.exists check is a benign TOCTOU remnant but not the actual guard at src/apm_cli/config.py:17
  The real atomic guard is O_CREAT|O_EXCL inside the try block. The outer if not os.path.exists(CONFIG_FILE) is a non-atomic fast-path optimisation -- harmless because O_EXCL catches any race, but worth a one-line comment so the next reader does not remove the outer check thinking it is the guard, or remove the inner try thinking the outer check is sufficient.
  Suggested: # Fast-path: skip the syscall on the common case. O_EXCL below is the actual atomic guard.

• [nit] get_config/set_config write paths remain non-atomic; out of scope but worth tracking at src/apm_cli/config.py:59
  Not introduced by this PR. Flagging for a future hardening pass if parallel workers ever mutate config post-init. Consider filelock if that scenario materialises.

• [nit] Design patterns note: changes are procedural and appropriate. RUNTIME_ENV_VARS ValueObject scales cleanly to additional runtime entries; no abstraction change needed.

CLI Logging Expert

• [nit] except FileExistsError: pass at src/apm_cli/config.py is semantically correct. Adding logger.debug("Config file already created by concurrent process, skipping") would surface this event in --verbose mode for AI agents diagnosing initialization races. Currently invisible even to debug output.

• [nit] No debug log records which source var won the copilot token resolution at src/apm_cli/core/token_manager.py. A logger.debug("Resolved copilot token from %s", winning_var) would make the --verbose chain auditable. Pre-existing gap; not introduced here.

DevX UX Expert

• [nit] The atomic O_CREAT|O_EXCL write initializes config with a hardcoded default_client value of vscode. Pre-existing issue, not introduced by this PR. Now more visible through the race path.

• [nit] COPILOT_GITHUB_TOKEN is now probed first in the copilot chain. If a user has both set with different values, the precedence shift is silent. A debug-level log noting which token source was resolved would help diagnose auth surprises.

Supply Chain Security Expert

• [recommended] Test code at tests/integration/test_golden_scenario_e2e.py:387 prints full subprocess output on failure after injecting COPILOT_GITHUB_TOKEN from GITHUB_APM_PAT/GITHUB_TOKEN. If the Copilot CLI or any tool it invokes echoes the token value into stdout/stderr, the token would appear in pytest output and CI logs. Scrub output against known token values before printing on failure, consistent with the no-token-in-logs contract.

• [nit] COPILOT_GITHUB_TOKEN placement in RUNTIME_ENV_VARS['copilot'] is correct; no scoping concern.

OSS Growth Hacker

• [recommended] A 0% merge queue success rate since May 24 is a contributor-trust event. Any contributor who opened a PR in that window and watched it fail repeatedly will have churned silently. Add a CHANGELOG entry under the next release naming the outage window and confirming it is resolved. This converts an invisible fix into a visible trust signal.

• [nit] COPILOT_GITHUB_TOKEN is now the preferred token lookup for copilot runtime. The PR body buries this as bug fix Integrate copilot runtime #2, but it is a first-class compatibility story. Consider a one-liner in the auth/token docs noting that COPILOT_GITHUB_TOKEN is recognized automatically in Copilot CLI contexts.

• [nit] 120s -> 300s timeout across 13 test call sites masks a latency signal. If installs are routinely taking more than 2 minutes, that is a user-facing performance story too. File a follow-up to instrument install timing and surface a progress indicator for long installs.

Auth Expert

• [recommended] COPILOT_GITHUB_TOKEN is not in TOKEN_PRECEDENCE['copilot'], creating a one-way blind spot at src/apm_cli/core/token_manager.py:100. TOKEN_PRECEDENCE['copilot'] resolves ['GITHUB_COPILOT_PAT', 'GITHUB_TOKEN', 'GITHUB_APM_PAT']. If a user sets COPILOT_GITHUB_TOKEN directly and has none of the TOKEN_PRECEDENCE vars set, get_token_for_purpose('copilot') returns None and APM may emit confusing warnings while Copilot CLI proceeds anyway. Consider adding COPILOT_GITHUB_TOKEN to TOKEN_PRECEDENCE['copilot'] as a low-priority fallback. Not introduced by this PR but surfaced by it.

• [nit] Test at tests/integration/test_golden_scenario_e2e.py:350 bypasses APM's own token resolution when GITHUB_APM_PAT is set (injects COPILOT_GITHUB_TOKEN directly; preserve_existing=True means APM does not overwrite it). Fine for CI. Document the intent with a comment so future authors do not assume APM's resolution drives the final value in this test path.

Doc Writer -- inactive

No documentation files in diff; COPILOT_GITHUB_TOKEN is an internal runtime env var mapping with no user-facing CLI surface requiring a docs update.

Test Coverage Expert

• [recommended] No test asserts COPILOT_GITHUB_TOKEN is set in the copilot runtime env dict after token resolution at tests/unit/core/test_token_manager_lifecycle.py
token_manager.py prepends COPILOT_GITHUB_TOKEN to RUNTIME_ENV_VARS['copilot']. _setup_copilot_tokens() iterates this list and writes the resolved token into each key. grep of tests/ for RUNTIME_ENV_VARS, _setup_copilot_tokens, and COPILOT_GITHUB_TOKEN returned zero hits in test files (the golden scenario e2e sets it as input, not as an assertion on the output env dict). A copilot skill run that silently fails to receive the token would be a user-visible regression with no test alarm.
  Proof (missing): tests/unit/core/test_token_manager_lifecycle.py::test_setup_copilot_runtime_env_includes_copilot_github_token -- proves: When a copilot token is available, COPILOT_GITHUB_TOKEN is populated in the runtime env dict passed to the skill process [secure-by-default, devx]
  assert 'COPILOT_GITHUB_TOKEN' in env and env['COPILOT_GITHUB_TOKEN'] == 'tok'

• [recommended] No test exercises the FileExistsError-swallow path in ensure_config_exists at tests/unit/test_config.py
tests/unit/test_config.py has test_ensure_config_exists_uses_utf8 (happy-path creation) but no test calls ensure_config_exists() when CONFIG_FILE already exists AND was created between the os.path.exists check and the os.open call. grep for FileExistsError and O_EXCL in tests/ returned zero hits in test_config.py. A future refactor removing the try/except would silently regress parallel xdist init invocations.
  Proof (missing): tests/unit/test_config.py::test_ensure_config_exists_swallows_file_exists_error -- proves: ensure_config_exists() is idempotent when another process creates CONFIG_FILE between the exists check and the open call [devx]
  with patch('os.open', side_effect=FileExistsError): config_mod.ensure_config_exists() # must not raise

• [nit] Timeout bumps in integration tests have no regression-trap implications. Purely operational; assertions unchanged.

_{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 #1477 · ● 2.2M · ◷