Skip to content

fix(shop): a malformed 200 from GitHub is a failure, not an empty result - #428

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/github-store-unresolved-state
Aug 23, 2026
Merged

fix(shop): a malformed 200 from GitHub is a failure, not an empty result#428
rubenvdlinde merged 1 commit into
developmentfrom
fix/github-store-unresolved-state

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The symptom

The App store renders neither installable cards nor its "GitHub could not be reached" hint. E2E REQ-OBTC-006 has failed on development since 14:46 today (last green 07:03), so this is not a passing blip.

The cause

GitHubCatalogService::search() decoded the response and, whenever the body would not parse or carried no items array, left $items = [] and fell through to OUTCOME_OK:

$decoded = json_decode($result['body'], true);
$items = [];
if (is_array($decoded) === true && is_array($decoded['items'] ?? null) === true) {
    $items = $decoded['items'];
}
// ...falls through to 'outcome' => self::OUTCOME_OK

A proxy error page or a truncated response was therefore reported as a successful search that simply matched nothing.

That is exactly the dead state the test catches:

OUTCOME_OK so githubUnavailable is false, and TemplateGallery renders no hint
zero cards so it renders "No GitHub apps match your search"

The user is told their query found nothing, when in fact the lookup never completed. A lookup failure must not wear the words of a judgement.

The ok === false path and the controller's Throwable handler were both already correct. This was the one branch where a non-answer became an answer.

The fix

An unparseable or unshaped 200 now returns OUTCOME_UNREACHABLE, which the UI already knows how to render. The early return also skips the cache write, so one malformed response is no longer served to every later caller for the rest of the TTL.

Tests

Two, deliberately, because "fix" this by calling every empty result unreachable and you have replaced one lie with another:

  • a 200 whose body is not the search shape yields OUTCOME_UNREACHABLE
  • a well-formed but genuinely empty result set stays OUTCOME_OK

I verified the first test fails without the production change and passes with it. A test that would pass either way pins nothing.

Checks

Check Result
phpcs (project scope) clean
psalm 0 errors
phpstan [OK] No errors
phpunit unit suite 883 tests, 2813 assertions, all pass

Spec gains the matching scenario under REQ-GHSC-003, carrying an @e2e exclude with its reason: the malformed-body branch cannot be driven from a browser without making GitHub itself return a bad 200, and the user-visible consequence is already covered by REQ-OBTC-006.

Found while running the new writing skill's REVIEW mode over buildiq (ConductionNL/hydra#610) and following its E2E failure to the cause.

The App store has been rendering neither installable cards nor its
"GitHub could not be reached" hint, and e2e REQ-OBTC-006 has failed on
development since 14:46 today.

GitHubCatalogService::search() decoded the response and, whenever the body
would not parse or carried no `items` array, quietly left `$items = []` and
fell through to OUTCOME_OK. So a proxy error page or a truncated response
was reported as a successful search that simply matched nothing.

That produced the dead state the test caught. OUTCOME_OK means
`githubUnavailable` is false, so TemplateGallery renders no hint; zero cards
means it renders "No GitHub apps match your search" instead. The user is
told their query found nothing, when in fact the lookup never completed.
A lookup failure must not wear the words of a judgement.

An unparseable or unshaped 200 now returns OUTCOME_UNREACHABLE, which the
UI already knows how to show. The early return also skips the cache write,
so one malformed response is no longer served to every later caller for the
rest of the TTL.

Two tests, because fixing this by calling every empty result unreachable
would replace one lie with another:
- a 200 whose body is not the search shape yields OUTCOME_UNREACHABLE
- a well-formed but genuinely empty result set stays OUTCOME_OK

Verified the first test fails without the production change and passes with
it. Spec gains the matching scenario under REQ-GHSC-003.

phpcs clean, psalm 0 errors, phpstan OK, 883 unit tests pass.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/buildiq @ 90c2404

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
check-l10n-js
composer ✅ 106/106
npm ✅ 625/625
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-23 22:43 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Correction: this does not fix REQ-OBTC-006

I opened this PR saying it fixes the failing E2E. It does not. REQ-OBTC-006 fails identically with this change applied, and I should not have claimed otherwise before seeing CI.

What this PR does fix, and still should

The malformed-body path is a real defect on its own terms: a 200 whose body will not parse was reported as OUTCOME_OK with zero cards, so a failed lookup rendered as "No GitHub apps match your search". That is a lie in the code regardless of which path CI happens to exercise, it is now OUTCOME_UNREACHABLE, and the fix is pinned by a test I verified fails without it.

What is actually failing in CI

mounted() does call searchGithub(), so the search runs. The likely path is the genuinely empty one: an anonymous CI runner searching topic:buildiq-app gets a well-formed {"items": []}, which is correctly OUTCOME_OK with zero cards. My second test in this PR pins exactly that as correct behaviour.

So the UI renders the "no apps match your search" empty state, and the test asserts it must render either installable cards or the unavailable hint, and never neither.

That premise does not hold for a real empty result set. One of these has to give:

  1. the test accepts a third documented state (empty result), or
  2. the App store renders something for "the store is reachable and has nothing for you" that the test recognises, or
  3. the spec says an empty catalogue is itself an unavailable state, and the service reports it that way

That is a product/spec decision on REQ-OBTC-006, not a bug I should silently pick an answer for. It also explains why development has been red on this same test since 14:46 today, before I touched anything.

Merging this as a correct partial fix, since it does not cause or worsen the failure and development is already red on it. Flagging the remaining question rather than papering over it.

@rubenvdlinde
rubenvdlinde merged commit b4e7f4c into development Aug 23, 2026
78 of 82 checks passed
rubenvdlinde pushed a commit that referenced this pull request Aug 23, 2026
…ister

One conflict, src/manifest.json's walkthrough steps, and both sides had changed
it for different reasons: development rewrote the copy in the voice sweep (#426)
while this branch renamed the register the `create-app` step watches. Took
development's wording with this branch's `"register": "buildiq"`, so neither
change is lost. All 13 register references in the manifest now read buildiq and
the file still parses.

Development also brought #428, "a malformed 200 from GitHub is a failure, not an
empty result". That is complementary to the e2e change on this branch rather
than a duplicate of it: #428 reclassifies a MALFORMED response so it renders the
unavailable hint, while the third state this branch teaches the test about is a
SUCCESSFUL search that matched nothing — which the view still renders as
`.template-gallery__empty`. Both states exist; the assertion now covers all
three.
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.

1 participant