Skip to content

ci(quality): gate the code this repo actually runs — and it was hiding two real bugs - #23

Closed
rubenvdlinde wants to merge 1 commit into
developmentfrom
ci/quality-enforcement
Closed

ci(quality): gate the code this repo actually runs — and it was hiding two real bugs#23
rubenvdlinde wants to merge 1 commit into
developmentfrom
ci/quality-enforcement

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The gap

code-quality.yml existed here, but it ran only the shared pipeline's PHP legs — which are aimed at phpcs-custom-sniffs/. That is correct for what they are: this is a Python ExApp sidecar and there is no lib/. Forcing PHP tooling at a Python codebase would be the wrong fix.

What it left ungated is the application. ex_app/lib/main.py is 623 lines — the largest of the four sidecars — and nothing in this repo has ever looked at it.

(The file's own comment called this a "PHP-only ExApp". It is a Python ExApp; the only PHP in the tree is a custom sniff. Corrected.)

Two real defects, not style

1. Popen.stdout, and a closure over a global that gets nulled.
start_opentalk() spawned the controller and started a daemon thread whose body was for line in OPENTALK_PROCESS.stdout. It read the global — and stop_opentalk() sets that global to None. A stop, or a stop-then-restart, would have raised AttributeError inside the log thread on its next iteration. Popen.stdout is also Optional and was unguarded. Surfaced by mypy:

ex_app/lib/main.py:164: error: Item "None" of "IO[Any] | None" has no attribute "__iter__" (not iterable)  [union-attr]

The thread now binds the process and the stream locally and returns if the stream is absent.

2. Dead OIDC computation.
_serve_index_html() computed oidc_authority = f"{keycloak_browser_url}/realms/{KEYCLOAK_REALM}" and never used it — residue from an earlier oidc-client-ts/sessionStorage approach the injected bootstrap script no longer takes (it writes plain localStorage keys). The authority reaches the frontend via _build_config_js(), which is what window.config consumes. Removed, and the stale docstring corrected to describe what the function actually does. Surfaced by ruff F841.

Also narrowed selectors' key.fileobj (typed int | HasFileno) with a real isinstance check rather than an assert or a cast — a raw fd reaching .recv()/.close() would raise AttributeError inside the Keycloak proxy loop.

Measured before and after, identically conditioned

check before after
ruff check ex_app/ 5 findings — I001, SIM108, SIM105, RUF010, F841 All checks passed
ruff format --check ex_app/ 1 file would be reformatted 1 file already formatted
mypy ex_app/ exit 1, 5 errors — 4× union-attr on selectors fileobj, 1× union-attr on Popen.stdout 0 errors, 1 source file checked
make check-strict (target did not exist) exit 0
hydra-gates --base origin/development (never ran in CI) exit 0, 5 changed file(s) in scope, 27 of 63 reported, 0 failures

The before column was re-measured against the committed HEAD after the fix, so it is a positive control: both jobs demonstrably fail on the code as it stood.

requirements.txt is installed in CI alongside requirements-dev.txt on purpose — so mypy resolves nc_py_api/httpx for real rather than falling back to ignore_missing_imports and checking less than it appears to. That is what made defect 1 visible at all.

Why Hydra Gates was skipping — and it was not the Playwright dependency

enable-hydra-gates defaults to false in ConductionNL/.github/.github/workflows/quality.yml, and this file never passed it. The job's guard is:

if: ${{ inputs.enable-hydra-gates && !cancelled() }}

It was the first term that deleted the job. !cancelled() — the part with the long comment about needs: implying success() — was already doing its job correctly.

Measured with a --full scan of the whole tree before switching it on: 29 of 63 gates reported, 0 failures. Switched on here.

hydra-gates-ref is deliberately left at main rather than pinned. A pin is a silent expiry date: the fleet pinned v1.0.1 across 22 repos, the pin predated the fixes to 16 gates, and all 16 were dead for as long as the pin stood.

⚠️ One of the 29 gates was passing on nothing

gate-28 (license-triangle) called _pass 28 unconditionally after a comparison that only runs inside if [ -n "${_composer_lic}" ] && [ -d lib ]. This repo has no lib/, so it printed [gate-28] license-triangle: PASS having opened zero files — and the coverage accounting counted it as a gate that reported a result.

Fixed in ConductionNL/.github#(companion PR); with that fix it reports NOT APPLICABLE with a stated reason and the count drops 29 → 27 diff-scoped. Until that companion PR merges, this PR's CI will still show gate-28 as a green PASS. That green is the vacuous one — do not read it as coverage.

No test suite, and no pretending otherwise

This repo has no automated tests: no pytest config, no test_*.py, no phpunit.xml, no tests/. composer.json's check:strict already says so and asks that no test script be re-added until a real suite exists. That is respected — nothing here scaffolds a suite. Both defects above were found by static analysis; neither is covered by a regression test, and that gap is real.

The Makefile's test target is renamed to run, because it never tested anything: an interactive docker run -it that boots the container, asserts nothing, and cannot run in CI at all (-it needs a TTY). There is now no test target, so make test fails loudly instead of exiting 0 having proved nothing.

…g two real bugs

code-quality.yml existed here, but it ran only the shared pipeline's PHP legs
— which are aimed at phpcs-custom-sniffs/. That is correct for what they are:
this is a Python ExApp sidecar and there is no lib/. What it left ungated is
the application. ex_app/lib/main.py is 623 lines — the largest of the four
sidecars — and nothing in this repo has ever looked at it.

The file's own comment called this a "PHP-only ExApp". It is a Python ExApp;
the only PHP in the tree is a custom sniff. Corrected.

TWO REAL DEFECTS, not style

  1. Popen.stdout, and a closure over a global that gets nulled.
     start_opentalk() spawned the controller and started a daemon thread whose
     body was `for line in OPENTALK_PROCESS.stdout`. It read the GLOBAL, and
     stop_opentalk() sets that global to None — so a stop, or a
     stop-then-restart, would have raised AttributeError inside the log thread
     on its next iteration. Popen.stdout is also Optional and was
     unguarded. The thread now binds the process and the stream locally and
     returns if the stream is absent.
     Surfaced by mypy: `Item "None" of "IO[Any] | None" has no attribute
     "__iter__"`.

  2. Dead OIDC computation. _serve_index_html() computed
     `oidc_authority = f"{keycloak_browser_url}/realms/{KEYCLOAK_REALM}"` and
     never used it — residue from an earlier oidc-client-ts/sessionStorage
     approach that the injected bootstrap script no longer takes (it writes
     plain localStorage keys). The authority reaches the frontend via
     _build_config_js(), which is what window.config consumes. Removed, and
     the stale docstring corrected to describe what the function does.
     Surfaced by ruff: `F841 Local variable oidc_authority is assigned to but
     never used`.

  Also narrowed selectors' `key.fileobj` (typed `int | HasFileno`) with a real
  isinstance check rather than an assert or a cast: a raw fd reaching
  .recv()/.close() would raise AttributeError inside the Keycloak proxy loop.

ADDED

  python-checks — ruff (lint + format) and mypy over ex_app/, matching the
  pattern already used by n8n-nextcloud and keycloak-nextcloud.
  pyproject.toml carries the config; requirements-dev.txt pins ruff==0.16.1
  and mypy==2.3.0 exactly, because a floating linter changes a repo's verdict
  with no commit in that repo to explain it. requirements.txt is installed in
  CI too, so mypy resolves nc_py_api/httpx for real rather than falling back
  to ignore_missing_imports and checking less than it appears to — that is
  what made defect 1 visible at all.

MEASURED BEFORE AND AFTER, identically conditioned

  ruff check ex_app/     BEFORE: 5 findings (I001, SIM108, SIM105, RUF010,
                                 F841)
                         AFTER:  All checks passed
  ruff format --check    BEFORE: 1 file would be reformatted
                         AFTER:  1 file already formatted
  mypy ex_app/           BEFORE: 5 errors (4x union-attr on selectors
                                 fileobj, 1x union-attr on Popen.stdout)
                         AFTER:  0 errors, 1 source file checked
  make check-strict      AFTER:  exit 0

The BEFORE numbers were re-measured against the committed HEAD after the fix,
so they are a positive control: both jobs demonstrably fail on the code as it
stood, and mypy exited 1.

WHY HYDRA GATES WAS SKIPPING — and it was not the Playwright dependency

`enable-hydra-gates` defaults to false in the shared workflow and this file
never passed it. The job's guard is
`if: inputs.enable-hydra-gates && !cancelled()`, so it was the FIRST term that
deleted the job. `!cancelled()` was already doing its job correctly.

Measured with a --full scan of the whole tree before switching it on:
29 of 63 gates reported, 0 failures. Switched on here.

One of those 29 was not real. gate-28 (license-triangle) called `_pass`
unconditionally after a comparison that only runs when `[ -d lib ]`, so it
reported PASS having opened zero files. Fixed separately in
ConductionNL/.github; with that fix it reports NOT APPLICABLE here and the
count drops to 28. A gate that inspected nothing now says so.

hydra-gates-ref is deliberately left at `main` rather than pinned. A pin is a
silent expiry date — the fleet pinned v1.0.1 across 22 repos, the pin predated
the fixes to 16 gates, and all 16 were dead for as long as the pin stood.

NO TEST SUITE, AND NO PRETENDING OTHERWISE

This repo has no automated tests: no pytest config, no test_*.py, no
phpunit.xml, no tests/. composer.json's check:strict already says so at length
and asks that no test script be re-added until a real suite exists. That is
respected — nothing here scaffolds a suite. Both defects above were found by
static analysis; neither is covered by a regression test, and that gap is real.

The Makefile's `test` target IS renamed to `run`, because it never tested
anything: it is an interactive `docker run -it` that boots the container,
asserts nothing, and cannot run in CI at all (-it needs a TTY). There is now no
`test` target, so `make test` fails loudly instead of exiting 0 having proved
nothing. check-strict prints what its green does and does not cover.
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Companion gate-28 fix: ConductionNL/.github#172license-triangle reported PASS on repos with no lib/, having opened zero files. Until #172 merges, this PR's Hydra Gates output will still show [gate-28] license-triangle: PASS; that is the vacuous pass, not coverage. With #172 it reads NOT APPLICABLE and the diff-scoped count drops 28 → 27.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Closing: out of scope for now — the ExApp sidecar wrappers are not part of the 16 Nextcloud apps this sweep covers. Findings are recorded in the agent report; the branch ci/quality-enforcement is left in place (not deleted) so this can be reopened as-is.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Quality Report — ConductionNL/opentalk @ 965918d

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint ⏭️
stylelint ⏭️
build ⏭️
composer ✅ 69/69
npm
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️
Hydra gates

Quality workflow — 2026-08-05 21:06 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.

1 participant