refactor(E-21 tests): exercise GptService.GenerateImageAsync directly, remove MapImageException duplication

[cyberprophet]

Background

Follow-up to merged PR #56 (E-21 image generation exception mapping). Codex review flagged a test-design issue that was not addressed before merge.

Problem

agency.tests/GptServiceImageErrorTests.cs asserts against a local helper MapImageException that reimplements the catch/filter logic from GptService.Image.cs. Because the tests run through the duplicated helper rather than invoking GptService.GenerateImageAsync<T> itself, the suite can stay green even if the real production mapping drifts — for example, if a future edit in GptService.Image.cs changes or removes the 401/429 clauses, the tests will not catch it.

This weakens regression protection for a behavior P5 and P8 callers depend on (401 → UnauthorizedAccessException, 429 → ImageRateLimitedException).

Codex severity: P2.

Proposed fix

Either:

1. Extract the status-to-exception mapping into a shared, internal static method on GptService (e.g. GptService.MapImageException(ClientResultException) in a partial file), have both production and tests call it directly. Delete the test-local duplicate.
2. Or rewrite the tests to call GptService.GenerateImageAsync<T> through a fake OpenAIClient / HttpMessageHandler that returns specific ClientResultException status codes — exercising the full production code path.

Option 1 is smaller; option 2 is stricter. Pick based on how hard it is to stub OpenAIClient cleanly in the existing test setup.

Acceptance

• No duplicated catch/filter logic between production and tests.
• Tests fail if GptService.Image.cs:56-80 is changed in a way that alters observable exception mapping.
• All 12 existing E-21 tests still pass.

References

• PR fix(E-21): map HTTP 401 and 429 to typed exceptions in image generation #56 (the original merge)
• PR fix(P-07): enforce hard cap including truncation marker #58 (separate P-07 hard-cap follow-up, not a dependency)

[cyberchacha]

Triage Summary

• Type: enhancement
• Summary: The test file GptServiceImageErrorTests.cs (from PR fix(E-21): map HTTP 401 and 429 to typed exceptions in image generation #56) duplicates the catch/filter logic from GptService.Image.cs in a local MapImageException helper, meaning tests can pass even if the production mapping drifts.
• Why this label: This is a refactor / test-design improvement request, not a bug — current tests pass, but they test duplicated logic rather than the real production code path.
• Missing info: none

Findings

Duplicated catch/filter logic in test helper

agency.tests/GptServiceImageErrorTests.cs lines 36-55 define MapImageException() which replicates the exact same catch (ClientResultException ex) when (ex.Status == 400/401/429) pattern from agency/OpenAI/GptService.Image.cs lines 63-81. If production catch clauses are added, removed, or reordered, the test helper will not reflect those changes and tests will remain green.

// Test helper (duplicated logic)
static Exception MapImageException(ClientResultException ex)
{
    try { throw ex; }
    catch (ClientResultException e) when (e.Status == 400) { return new ImageGenerationModerationException(e.Message, e); }
    catch (ClientResultException e) when (e.Status == 401) { return new UnauthorizedAccessException(..., e); }
    catch (ClientResultException e) when (e.Status == 429) { return new ImageRateLimitedException(..., e); }
}

FakePipelineResponse already exists and can be reused

The test file already contains FakePipelineResponse (lines 150-175), a minimal PipelineResponse stub that creates ClientResultException with arbitrary status codes. This infrastructure is sufficient to drive the real GptService.GenerateImageAsync<T> if the ImageClient call can be intercepted.

GptService inherits OpenAIClient — stubbing requires constructor seam

GptService (in agency/OpenAI/GptService.cs line 14) extends OpenAIClient directly and calls GetImageClient(imageModel) at line 34 of the partial GptService.Image.cs. To inject a fake that throws ClientResultException with specific status codes, either: (a) make GetImageClient virtual/overridable, or (b) inject an HttpMessageHandler via an OpenAIClientOptions overload that returns canned error responses.

Suggested Approach

Option A — Extract shared mapping method (smaller change):

1. In agency/OpenAI/GptService.Image.cs, extract the three catch blocks into an internal static Exception MapImageException(ClientResultException ex) method.
2. Have the catch blocks in GenerateImageAsync<T> call this method and throw the result.
3. In GptServiceImageErrorTests.cs, delete the local MapImageException helper and call GptService.MapImageException() directly.
4. Verify all 12 tests pass.

Option B — Test through the real method (stricter, recommended by issue):

1. Add an internal constructor or OpenAIClientOptions parameter to GptService that accepts a custom HttpMessageHandler.
2. Create a test DelegatingHandler that returns HttpResponseMessage with status 400/401/429 for image endpoint calls.
3. Rewrite tests to instantiate GptService with the fake handler and call GenerateImageAsync<T> directly, asserting on the thrown exceptions.
4. Delete the MapImageException helper entirely.
5. Keep FakePipelineResponse only if still needed; otherwise remove it too.

Hybrid approach: Start with Option A for a quick fix, then follow up with Option B for full integration coverage.

Risks or trade-offs: Option A is minimal but still doesn't test the full method (e.g., option construction, logging). Option B requires exposing an internal constructor seam, which adds a small amount of test-only API surface. Neither option changes production behavior.
Estimated scope: small (Option A) / medium (Option B)