Skip to content

fix(github-push): surface the real upstream status/body when a broker call fails - #262

Merged
rubenvdlinde merged 2 commits into
developmentfrom
fix/broker-error-detail-not-swallowed
Aug 19, 2026
Merged

fix(github-push): surface the real upstream status/body when a broker call fails#262
rubenvdlinde merged 2 commits into
developmentfrom
fix/broker-error-detail-not-swallowed

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Summary

GitHubPushService::brokerCall() — the single choke point every GitHub write in the export pipeline goes through (createRepo, and postJson() backing pushTree's blob/tree/commit/ref calls plus openPullRequest) — logged only a bare HTTP status on a non-2xx broker response (no body) and discarded the exception message on a transport failure, returning null either way. postJson()/createRepo() then threw a fixed, content-free RuntimeException ('GitHub API call failed: POST ' . $path / 'GitHub create-repo failed.') that RunExportJob::run() writes verbatim into the ExportJob's errorMessage — the only thing the user ever sees. This was observed live this session: a publish failure surfaced only the generic message, forcing a blind retry that happened to succeed without ever learning why the first attempt failed.

  • brokerCall() now captures the real failure detail on a $lastFailureDetail property: HTTP {status}: {truncated, scrubbed body} for a completed non-2xx response, or transport error: {scrubbed message} for a caught broker exception. Reset at the top of every call so a stale value can never leak into an unrelated failure.
  • postJson() and createRepo() append that detail to the RuntimeException they already throw.
  • Reuses the existing scrub() helper (GitHub PAT-shaped token redaction) already applied to broker exception messages, and caps the body excerpt at 300 chars. Verified against CredentialBrokerService::request()'s contract ({status, headers, body} returned verbatim from GitHub — GitHub's own response body cannot contain our secret) and the two exceptions it can throw (CredentialAccessDeniedException/CredentialUpstreamException, neither of which carries secret material) that no additional redaction is needed.
  • Spec delta folded into openbuild-exporter's "Export target — GitHub repository" requirement (new scenario generalising the existing auth-failure scenario to any upstream failure).

Test plan

  • tests/Unit/Service/GitHubPushServiceTest.php extended: the existing fail-closed regression now asserts the thrown message carries transport-failure detail, not the bare fixed string; 5 new focused tests pin failureDetailFromStatus() (status+body, empty-body, truncation), scrub() redacting a PAT-shaped token, and failureSuffix()'s exact formatting.
  • composer lint / phpcs / phpmd / phpstan / psalm — all clean on the full tree (psalm's 4 pre-existing ZipArchive-missing errors are environmental, unrelated, present on development too).
  • Full PHPUnit suite: 870 passed (was 865 on development), same 23 pre-existing errors (ZipArchive/FlowService missing from this environment) — parity confirmed against development.
  • hydra-gates: same 3 pre-existing failures as development (e2e-coverage, manifest-validation, effective-manifest-crossref — all ajv/node_modules environmental, unrelated) — parity confirmed, zero new gate failures introduced.

🤖 Generated with Claude Code

… call fails

GitHubPushService::brokerCall() logged only a bare HTTP status (no body)
on a non-2xx broker response and discarded the exception message on a
transport failure; postJson()/createRepo() then threw a fixed,
content-free RuntimeException that became the ExportJob's errorMessage
verbatim, leaving a failed publish undiagnosable without a blind retry.

Capture the real detail (HTTP status + a truncated, scrubbed body
excerpt, or the scrubbed transport-failure reason) and fold it into the
thrown message. Reuses the existing PAT-token scrub() already applied to
broker exception messages; verified against CredentialBrokerService's
contract that no other redaction is needed.
…ch it doesn't take

testPushFailsClosedWhenTheBrokerCannotServeTheCall assumed Server::get() would
throw in a bare PHPUnit environment, landing brokerCall() in its
transport-exception branch ("transport error: ..."). CI showed the real
behavior instead: the call falls through to the non-2xx branch and produces
"HTTP 0" — which already satisfies the actual requirement (diagnostic detail
present, not the bare fixed string), just via a different internal branch than
assumed. Drop the over-specific "transport error:" assertion; keep the two
that actually guard the requirement.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ b721792

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
test-l10n
check-gitignore
check-nc-floor
format
composer ✅ 106/106
npm ✅ 626/626
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-19 14:50 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ faf30dd

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
test-l10n
check-gitignore
check-nc-floor
format
composer ✅ 106/106
npm ✅ 626/626
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-19 15:34 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 76582b3 into development Aug 19, 2026
116 of 118 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/broker-error-detail-not-swallowed branch August 19, 2026 16:25
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ faf30dd

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
test-l10n
check-gitignore
check-nc-floor
format
composer ✅ 106/106
npm ✅ 626/626
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-19 16:29 UTC

Download the full PDF report from the workflow artifacts.

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.

2 participants