Skip to content

fix(quality): a required additional app that will not enable must fail the job - #355

Draft
rubenvdlinde wants to merge 1 commit into
mainfrom
fix/additional-app-enable-must-be-fatal
Draft

fix(quality): a required additional app that will not enable must fail the job#355
rubenvdlinde wants to merge 1 commit into
mainfrom
fix/additional-app-enable-must-be-fatal

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What is wrong

php occ app:enable "$name" || echo "::warning::Failed to enable $name, continuing..."
lets a run proceed without the dependency the repository under test is built on.
The absence then resurfaces as application failures, which is indistinguishable from
real debt in the log — and a warning in a red job is invisible.

Measurement

ConductionNL/hermiq, full-scope run 31490144919, six PHPUnit cells. Perfect 6/6
correlation between grep -c 'Failed to enable openregister' and the error count:

job cell enable result
93774658402 8.4 / stable33 ok Tests: 1500, Errors: 5, Failures: 1
93774658407 8.3 / stable33 ok Tests: 1500, Errors: 5, Failures: 1
93774658415 8.4 / stable32 ok Tests: 1500, Errors: 5, Failures: 1
93774658413 8.3 / stable31 FAILED Tests: 1500, Errors: 12
93774658418 8.3 / stable32 FAILED Tests: 1500, Errors: 12
93774658420 8.4 / stable31 FAILED Tests: 1500, Errors: 12

The seven extra "errors" were Interface "OCA\OpenRegister\Service\Flow\IFlowNodeLogActions" not found and Call to undefined method MockObject_ToolRegistryFacade::describeTools().
Two causes, both of which deserve a red job:

  • deterministic — OpenRegister declares min-version="32", the cell tested stable31;
  • transientcurl error 60 … self-signed certificate during composer install left
    Twig missing, so app:enable crashed on Class "Twig\Extension\AbstractExtension" not found.

Why this is a DRAFT — please sequence it

Merging this today turns 6 repos red on a cell that is currently lying rather than
failing. Measured nextcloud-test-refs against additional-apps across the fleet — these
run a stable31 cell while requiring openregister (min-version 32), so every one of
their stable31 cells is today measuring an instance without OpenRegister:

app-versions · hermiq · hrmq · petstore · planix · nextcloud-app-template

hermiq is already fixed (ConductionNL/hermiq#180 drops its stable31 cell and
declares the truthful min-version="32"). The other five need the same one-line change
before this is merged. I have left it as a draft rather than merge it and hand five agents
an unexplained red cell mid-session — the sequencing is a fleet call, not mine.

Implementation notes

Applied to all four install sites (phpunit, newman, both playwright legs).

⚠️ The failure flag goes through a file, not a variable: the loop body runs in a
subshell because it is fed by a pipeline, so a variable set inside it is invisible after
done — the same trap run-hydra-gates.sh documents at its gate-30 loop. Verified with a
three-app simulation: exit 0 when all enable, exit 1 when the first fails, exit 1 when
the last fails.

⚠️ Grep for this failure with the app name (Failed to enable openregister). The bare
Failed to enable also matches the workflow's own echoed script line, which contains
$name unexpanded — it returns 1 on a clean run and 2 on a failing one, so an off-by-one
reads as a correlation that is not there. That mis-measurement already happened once today.

…l the job

`php occ app:enable "$name" || echo "::warning::Failed to enable $name,
continuing..."` let a run proceed WITHOUT the dependency the repository under
test is built on. The absence then resurfaced as APPLICATION failures, which is
indistinguishable from real debt in the log.

Measured on ConductionNL/hermiq, full-scope run 31490144919, six PHPUnit cells,
perfect 6/6 correlation between `grep -c 'Failed to enable openregister'` and
the error count:

  93774658402 8.4/stable33 enable ok     -> Tests: 1500, Errors: 5, Failures: 1
  93774658407 8.3/stable33 enable ok     -> Tests: 1500, Errors: 5, Failures: 1
  93774658415 8.4/stable32 enable ok     -> Tests: 1500, Errors: 5, Failures: 1
  93774658413 8.3/stable31 enable FAILED -> Tests: 1500, Errors: 12
  93774658418 8.3/stable32 enable FAILED -> Tests: 1500, Errors: 12
  93774658420 8.4/stable31 enable FAILED -> Tests: 1500, Errors: 12

The seven extra "errors" were `Interface "OCA\OpenRegister\Service\Flow\
IFlowNodeLogActions" not found` and `Call to undefined method
MockObject_ToolRegistryFacade::describeTools()` — an absent app wearing the
costume of app debt. Two causes, both deserving a red job: one deterministic
(OpenRegister declares min-version="32"; the cell tested stable31) and one
transient (`curl error 60 ... self-signed certificate` during composer install
left Twig missing, so enabling crashed).

Applied to all four install sites (phpunit, newman, and both playwright legs).

⚠️ The failure flag goes through a FILE, not a variable: the loop body runs in
a subshell because it is fed by a pipeline, so a variable set inside it is
invisible after `done` — the same trap run-hydra-gates.sh documents at its
gate-30 loop. Verified with a three-app simulation: exit 0 when all enable,
exit 1 when the FIRST fails and exit 1 when the LAST fails.

⚠️ Grep for the failure with the APP NAME (`Failed to enable openregister`).
The bare `Failed to enable` also matches the workflow's own echoed script line,
which contains `$name` unexpanded — it returns 1 on a clean run and 2 on a
failing one, so an off-by-one reads as a correlation that is not there.
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

This PR was right, and it has been sitting here for five days

Rediscovered the same defect family tonight from the other end, and independently re-verified this PR's measurement before writing anything. It reproduces exactly.

hermiq run 31490144919, six PHPUnit cells, grep -c 'Failed to enable openregister' against the test summary with Actions' echoed step source stripped:

cells app:enable result
3 failed Tests: 1500, Assertions: 4647, Errors: 12
3 succeeded Tests: 1500, Assertions: 4657, Errors: 5, Failures: 1

Perfect 6/6. Same commit, same application code — only the environment differs. That is a controlled experiment, not an anecdote, and it is better evidence than most gate findings ever get.

The sibling defect, four lines away

The composer install for those same additional apps ends in 2>/dev/null || true — stderr discarded and exit status thrown away. It is the only composer install of the 20+ in quality.yml to do either. That is now #484 (draft).

Measured on decidesk#517 (a diff of exactly one .ts file, zero PHP): 6 of 6 PHPUnit cells hit an HTTP 504 installing openregister's dependencies, and 6 of 6 reported the step as successful. Three then went red with 87 errors (Class "Twig\Extension\AbstractExtension" not found), three went green — the difference being only which package the 504 removed.

Clear division of ownership so the two PRs do not collide:

They are two halves of one hole — an app that will not enable, and an app whose dependencies never installed — and this PR's own body already names the second as a cause of the first ("…or its composer install did not complete"). They should land together or not at all. A third, #483, fixes the phpcs gate discarding its own verdict.

Staleness — measured, and better than it looks

mergeable: CONFLICTING, and this branch is now 98 commits behind main, over which quality.yml grew by +944/−39 across 10 commits touching it.

But a dry-run merge of main into this branch (local only, nothing pushed) produces exactly one conflict region. Everything else auto-merges. This is much closer to landing than the metadata suggests — it needs a refresh, not a rewrite.

The finding that outlives all three fixes

This defect was found, measured and fixed on 2026-08-11. The fix never landed, and the fleet has since spent real effort misattributing environment failures to application debt — including tonight, on decidesk#517.

An unmerged fix is indistinguishable from an undiscovered bug, except that the second time you also pay for the rediscovery. Whatever caused this to stall is now itself the finding.

Not touching this branch — no force-push, no rebase from me. Flagging it for a refresh so all three can be sequenced deliberately.

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