test(auth): preserve streaming Git shim commands

[Daniel Meppiel (danielmeppiel)]

test(auth): preserve streaming Git shim commands

TL;DR

This follows up merged PR #2534 after the review panel found that its updated
installed-CLI Git shim buffered a long-lived git cat-file --batch-check
process. The patch captures output only for finite remote operations and adds
a cross-platform regression test proving bidirectional Git responses remain
available before stdin closes.

Note

The production locale-normalization fix is already on main. This PR is the
bounded test-infrastructure follow-up discovered during post-merge review.

Problem (WHY)

• PR fix(auth): normalize git message locale so auth failures stay classifiable #2534 changed the Git test shim to capture_output=True for every Git
  command so it could replace remote authentication stderr.
• GitPython also invokes long-lived git cat-file --batch-check processes.
  Capturing their stdout inside the shim withholds responses until process
  exit, while GitPython waits for a response before sending the next request.
• The installed-CLI lifecycle therefore timed out before reaching its
  private-repository credential-retry assertions.
• [!] The original lifecycle test detects the stall, but its packaged-binary
  setup makes the streaming contract slower and less direct to diagnose.

The panel treated the failed E2E as load-bearing evidence rather than an
opinion: "Grounding outputs in deterministic tool execution transforms probabilistic generation into verifiable action."

Approach (WHAT)

1. Build the rewritten Git command once inside the shim.
2. Route invocations without a rewritten remote directly through
   subprocess.run with inherited stdin, stdout, and stderr.
3. Retain captured text output only for finite remote invocations, where the
   fixture may replace localized unauthenticated stderr.
4. Add one focused cross-platform test that sends HEAD to
   cat-file --batch-check and requires the response before closing stdin.

Implementation (HOW)

File	Intent
tests/utils/git_credential_shim.py	Separates streaming local Git commands from finite remote commands. Remote stderr synthesis remains unchanged.
tests/unit/test_git_credential_shim.py	Creates a real repository and generated shim, starts cat-file --batch-check, writes one request, and enforces a five-second response deadline before cleanup.

Diagrams

Legend: The dashed streaming path is the new branch; finite remote operations
keep the existing captured-output path needed by the locale fixture.

flowchart LR
    G1[Git shim invocation] --> D1{Remote URL rewritten}
    D1 -->|yes| R1[Capture finite command output]
    R1 --> R2[Substitute localized auth stderr when needed]
    D1 -->|no| S1[Inherit stdin stdout and stderr]
    S1 --> S2[Bidirectional Git response remains live]
    classDef new stroke-dasharray: 5 5;
    class S1,S2 new;

Loading

Trade-offs

• Branch on rewritten remotes, not command names. The shim already computes
  remotes; reusing that fact avoids a fragile allowlist of current
  bidirectional commands.
• Keep subprocess.run for both branches. Replacing the local path with
  exec would be more transparent but would skip the shim's normal exit-code
  handling and complicate Windows behavior.
• Use a real Git subprocess in the unit test. A mocked pipe would not prove
  the buffering failure; the real cat-file protocol is fast and hermetic.
• Retain the broader installed-CLI scenario. The focused unit guard
  complements rather than replaces the full private-repository lifecycle.

Benefits

1. Long-lived Git commands return their first response before stdin closes.
2. The localized-auth remote fixture still controls finite command stderr.
3. One five-second unit guard localizes future stream-buffering regressions.
4. The regression guard runs in the existing Windows compatibility selection.
5. The patch changes one helper branch and adds one test file.

Validation

Exact-head targeted tests:

..                                                                       [100%]
2 passed in 1.56s

Mutation-break proof: removing the if not remotes streaming branch makes
tests/unit/test_git_credential_shim.py::test_git_shim_streams_bidirectional_commands
fail with _queue.Empty after five seconds; restoring it returns the test to
green.

Local lint and guard evidence

uv run --no-project --no-sync ruff check src/ tests/:

All checks passed!

uv run --no-project --no-sync ruff format --check src/ tests/:

1616 files already formatted

python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/:

Your code has been rated at 10.00/10

bash scripts/lint-architecture-boundaries.sh and
bash scripts/lint-auth-signals.sh:

[+] architecture boundary lint clean
[+] auth-signal lint clean

The driver watches the GitHub check set for this exact head before reporting
completion.

Scenario Evidence

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	Git operations used during installation keep responding when they require bidirectional streaming	DevX	tests/unit/test_git_credential_shim.py::test_git_shim_streams_bidirectional_commands (regression-trap for #2534)	unit
2	apm install of a private github.com dependency under a non-English locale still reaches the authenticated retry and completes	Secure by default, DevX	tests/integration/test_public_github_anonymous_lifecycle_e2e.py::test_private_github_fallback_normalizes_locale_and_completes_lifecycle	e2e

How to test

• Run python -m pytest -q tests/unit/test_git_credential_shim.py; expect
  one passing cross-platform streaming test.
• Remove the if not remotes branch and rerun that test; expect a
  five-second _queue.Empty failure, then restore the branch.
• Run the private localized lifecycle test with a current packaged APM
  binary; expect install success, one credential fill, and both observed
  locale variables equal to C.
• Run the canonical lint chain; expect ruff, duplication, architecture, and
  auth-signal guards to complete successfully.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

This PR makes Git authentication-failure detection locale-independent by pinning Git subprocess locale variables to C, ensuring AuthResolver.is_public_github_auth_failure continues to classify failures correctly and the credential retry path triggers reliably across non-English environments. It also updates the installed-test Git shim to avoid buffering long-lived bidirectional Git commands and adds regression coverage across unit and installed-CLI lifecycle tiers.

Changes:

• Pin LC_ALL=C and LANGUAGE=C in AuthResolver._build_git_env via an immutable mapping to prevent caller mutation.
• Extend unit + integration coverage for the translated-stderr failure/retry path and for shim streaming behavior.
• Update the Git credential shim to capture output only for finite remote operations and log locale observations; add a changelog entry for the user-visible fix.

Show a summary per file

File	Description
src/apm_cli/core/auth.py	Adds an immutable locale policy and applies it in the canonical auth-path Git environment builder.
tests/unit/core/test_public_github_anonymous_first.py	Adds unit tests asserting locale pinning/immutability and that translated auth failures still trigger a token retry.
tests/integration/test_public_github_anonymous_lifecycle_e2e.py	Seeds a non-English locale and verifies the installed CLI completes the private repo lifecycle with pinned locale variables.
tests/utils/git_credential_shim.py	Adjusts shim to stream non-remote commands and only capture output for finite remote operations; logs locale variables for assertions.
tests/unit/test_git_credential_shim.py	Adds a cross-platform regression test ensuring the shim doesn’t buffer bidirectional Git commands (cat-file --batch-check).
CHANGELOG.md	Records the fix under Unreleased Fixed with contributor attribution and issue closure reference.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (1)

tests/utils/git_credential_shim.py:126

• _uses_non_c_git_message_locale() treats an empty locale as non-C ("" does not startwith("C")), which can cause the shim to inject the translated auth stderr even when the effective locale is actually the default "C". It also ignores LC_MESSAGES/LANG, which are part of gettext locale resolution, so the shim's locale check can diverge from real git behavior.

def _uses_non_c_git_message_locale():
    locale = os.environ.get("LANGUAGE") or os.environ.get("LC_ALL") or ""
    return not locale.startswith("C")

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_now

PR #2673 preserves bidirectional Git I/O while capturing only finite rewritten-remote commands in the installed-test credential shim.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The panel converges on a sound, narrowly scoped follow-up to community PR
#2534. The inline proxy dispatch preserves Git's streaming behavior without
creating a parallel policy owner, while the real-Git Windows regression
exercises the streaming path and the existing lifecycle CI exercises remote
capture.

Mutation-break confirmation and green CI at
b094243fdf2dc5130d239b2472c3d3991b8fca42 provide strong confidence. No
specialist identified a correctness, security, UX, documentation,
authentication, or coverage concern.

Aligned with: Multi-harness and multi-host: the real-Git Windows regression
protects consistent credential-shim behavior across host environments. OSS
community driven: this focused follow-up completes the behavior introduced
through community PR #2534. Pragmatic as npm: transparent proxying keeps
ordinary Git commands interactive while capturing only commands with finite
output.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Sound, narrowly scoped shim fix that preserves bidirectional Git I/O without introducing a parallel policy owner.
CLI Logging Expert	0	0	0	No CLI output concerns; the test-only shim preserves streaming without changing user-facing logging.
DevX UX Expert	0	0	0	No DevX UX concerns; the shim fix preserves responsive Git behavior without changing the CLI surface.
Supply Chain Security Expert	0	0	0	Credential handling remains test-scoped without widening token exposure or changing production auth.
OSS Growth Hacker	0	0	0	No conversion surface is affected; the regression guard strengthens contributor confidence.
Auth Expert	0	0	0	The shim preserves private GitHub credential retry while streaming non-remote Git commands.
Test Coverage Expert	0	0	0	Focused real-Git regression and exact-head lifecycle CI cover streaming and remote-capture paths.

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

Architecture

flowchart TD
    A[Git shim invocation] --> B{Remote URL rewritten}
    B -- no --> C[Inherit stdin stdout and stderr]
    C --> D[Bidirectional Git response remains live]
    B -- yes --> E[Capture finite command output]
    E --> F{Localized unauthenticated failure}
    F -- yes --> G[Emit localized auth stderr fixture]
    F -- no --> H[Emit real Git stderr]

Loading

Folded in this run

• (panel) Preserve streaming for non-remote Git commands and add a focused
  cross-platform regression guard -- resolved in b094243fd.

Copilot signals reviewed

• tests/utils/git_credential_shim.py:126 -- NOT-LEGIT: the fixture
  deliberately exercises the two locale variables pinned by AuthResolver, and
  the relevant lifecycle seeds both variables explicitly; broader locale
  emulation is outside this streaming-only follow-up.

Regression-trap evidence (mutation-break gate)

• tests/unit/test_git_credential_shim.py::test_git_shim_streams_bidirectional_commands
  -- deleted the if not remotes streaming branch; test FAILED with
  _queue.Empty after five seconds; guard restored.

Lint contract

uv run --no-project --no-sync ruff check src/ tests/ and
uv run --no-project --no-sync ruff format --check src/ tests/ both passed.
The exact-head GitHub Lint job also passed.

CI

All checks passed for
https://github.com/microsoft/apm/actions/runs/32661154426 after 0 CI fix
iterations, including Windows Compatibility Gate, Lifecycle Smoke, both test
shards, architecture ratchets, CodeQL, and Merge Gate.

Mergeability status

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#2673	b094243	ship_now	2	1	0	2	green	MERGEABLE	BLOCKED	awaiting required review

Recommendation

Ship at the reviewed head; the regression coverage, mutation-break result, and
full green CI provide sufficient evidence, with no substantive follow-up
identified.

---

Full per-persona findings

Python Architect

• [nit] Keep the streaming and capture dispatch policy inline.
  The branch in tests/utils/git_credential_shim.py is the simplest correct
  proxy design at this scope; another abstraction would add indirection without
  a second policy owner.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert

No findings.

Doc Writer -- inactive

The diff only changes test infrastructure and does not change documentation or
user-facing source behavior.

Test Coverage Expert

No findings.

Performance Expert -- inactive

The diff only changes test infrastructure and does not touch production
performance paths or make performance claims.

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