fix(azure): fetch build-log content as text/plain to avoid ISecuredObject 500 (#192)

[MementoRC]

Summary

Fixes #192. azure_get_build_logs returned a 500 ISecuredObject error on every single-log content fetch for the conda-forge org.

Root cause

The single build-log content endpoint (GET .../build/builds/{id}/logs/{logId}?api-version=7.1) returns a bare List<String>. AzureClient.get hardcoded Accept: application/json, so Azure DevOps refused to serialize it and returned:

500 - Cannot return type System.Collections.Generic.List`1[System.String], from a public API as it doesn't implement ISecuredObject

A plain curl worked because its default Accept: */* yields text/plain. The log-list path (no log_id) was unaffected — it returns a proper JSON object.

Changes

• AzureClient.get gains an accept parameter (default application/json) that sets the Accept header; the list path is unchanged.
• azure_get_build_logs requests single-log content with accept="text/plain"; the existing text/plain response branch handles it.
• azure_get_failing_jobs: same text/plain fix on its inline log fetch, plus a NoneType crash fix — record.get("log", {}).get("id") raised 'NoneType' object has no attribute 'get' when a timeline record's log key was present but null (the secondary symptom reported in azure_get_build_logs: 500 ISecuredObject error on every log-content fetch (conda-forge org) #192); now (record.get("log") or {}).get("id").

Tests

Adds 4 tests (Accept-header override + default on AzureClient.get, the text/plain request for single-log content, and a regression test that azure_get_failing_jobs does not raise when a failed record has log=None). Full azure unit suite: 31 passed.

Closes #192

🤖 Generated with Claude Code

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Review

• Read changed files
• Review code quality and best practices
• Check for potential bugs
• Assess test coverage
• Post final review

---

Summary

This is a clean, targeted fix. The root cause analysis is accurate, the changes are minimal and well-scoped, and the test coverage is solid. A few observations below.

---

Code Quality

client.py — AzureClient.get accept parameter (client.py:59-61)

The design is good. Adding accept as a keyword argument with a sensible default is the right approach — it's non-breaking and self-documenting. The docstring explains the Azure quirk clearly.

One minor observation: the accept parameter propagation doesn't carry through the 401-retry path on client.py:93. On retry, the same headers dict is re-used (already constructed with the correct accept value at line 76-79), so this is actually fine — the retry correctly inherits the Accept header. Worth noting this works correctly even though it's not immediately obvious.

api.py — azure_get_failing_jobs NoneType fix (api.py:301)

if include_logs and (record.get("log") or {}).get("id"):

This is the correct idiomatic fix. The comment on lines 298-300 explaining the log=None trap is appropriately detailed and helpful.

Duplicated log-parsing logic (api.py:167-180 and api.py:310-322)

The content-type branching block that parses JSON vs. plain text log responses is now duplicated between azure_get_build_logs and azure_get_failing_jobs. This is a minor code smell — a small helper like _parse_log_response(response) would DRY this up, but it's not a blocker for this fix. Consider extracting it in a follow-up.

---

Potential Bugs / Edge Cases

api.py:165-180 — JSON branch is now unreachable for single-log content

Since the single-log endpoint now always requests text/plain, Azure will respond with text/plain. The application/json branch at api.py:168 will never be hit for this endpoint in practice. The code is harmless (it's defensive), but the comment on line 165-166 ("Azure DevOps returns log content as JSON with a value array") is now misleading — that was the old behavior we're explicitly avoiding. The same stale comment exists at api.py:309.

Consider updating the comment or simplifying to just the text branch if the JSON path is confirmed dead for this endpoint.

api.py:179-180 — Empty trailing newline from split("\n")

log_text = await response.text()
log_lines = log_text.split("\n")

If log_text ends with a newline (common for log files), split("\n") produces a trailing empty string. This means original_line_count will be inflated by 1, and the output may have a spurious blank line. Using log_text.splitlines() or log_text.rstrip("\n").split("\n") would be cleaner. The same issue exists at api.py:322.

---

Test Coverage

The 4 new tests are well-written and cover the right scenarios:

Test	What it covers
test_get_sends_text_plain_accept_header_when_accept_arg_passed	Accept header override on AzureClient.get
test_get_defaults_accept_header_to_application_json	Default Accept header remains application/json
test_get_specific_log_requests_text_plain_accept_header	azure_get_build_logs sends accept="text/plain"
test_get_failing_jobs_does_not_raise_when_log_is_none	Regression: log=None doesn't crash

The regression test at test_azure_api.py:305 is particularly well-constructed — using raise AssertionError inside the mock side-effect to assert that no log fetch is attempted when log=None is a clean, explicit approach.

Missing: There's no test verifying that azure_get_failing_jobs also sends accept="text/plain" on its inline log fetch (analogous to test_get_specific_log_requests_text_plain_accept_header for azure_get_build_logs). This would be a good addition.

---

Security

No concerns. No credentials are exposed or logged. The accept parameter does not introduce any injection risk — it's a controlled string passed directly as an HTTP header value.

---

Minor Nits

• client.py:78: The hardcoded "MCP-Git-Server/1.1.0" version string will drift. Not a bug, just a maintenance note.
• The post and patch methods (client.py:97-135) still hardcode "Accept": "application/json". If any write-path endpoint ever has the same Azure serialization problem, they'll need the same treatment. Low risk for now.

---

Overall: This is a solid, production-worthy fix. The two minor bugs (stale comments implying a JSON path that's now unreachable, and split("\n") trailing empty string) are worth addressing, but neither is a blocker. The NoneType crash fix is correct. Recommend approving pending the optional improvements.