From dce6ebb8428331a206ae83fdf75174cb7c337db0 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Wed, 5 Aug 2026 22:54:46 +0200 Subject: [PATCH] =?UTF-8?q?ci(quality):=20gate=20the=20code=20this=20repo?= =?UTF-8?q?=20actually=20runs=20=E2=80=94=20and=20it=20was=20hiding=20two?= =?UTF-8?q?=20real=20bugs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/code-quality.yml | 87 ++++++++++++++++++++++++- Makefile | 77 +++++++++++++++++++++- ex_app/lib/main.py | 101 ++++++++++++++++------------- pyproject.toml | 54 +++++++++++++++ requirements-dev.txt | 9 +++ 5 files changed, 279 insertions(+), 49 deletions(-) create mode 100644 pyproject.toml create mode 100644 requirements-dev.txt diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index 1330004..0ad6cb3 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -25,6 +25,62 @@ permissions: pull-requests: write jobs: + # ── The application itself ──────────────────────────────────────────────── + # This job did not exist until 2026-08-05. opentalk is a Python ExApp + # sidecar: the application is ex_app/lib/main.py — 623 lines, the largest of + # the four — and nothing in this repo had ever looked at it. phpcs.xml / + # psalm.xml / phpstan.neon are all pointed at phpcs-custom-sniffs/, which is + # correct for what they are, and leaves the actual app entirely ungated. + # + # Measured on 2026-08-05, first run of these checks against this repo: + # ruff found 5 lint findings and mypy found 5 type errors. Two were real + # defects, not style: + # - F841: _serve_index_html() computed an OIDC authority it never used — + # residue from an earlier sessionStorage approach. + # - union-attr on Popen.stdout: the controller's log thread closed over + # the GLOBAL OPENTALK_PROCESS, which stop_opentalk() sets to None, so a + # stop-then-restart would have raised AttributeError on the log thread. + # All fixed in the commit that added this job, so it starts green on a real + # scan of the file — not on an empty scope. + python-checks: + name: ${{ matrix.check.name }} + runs-on: ubuntu-latest + # Observed locally at well under a minute; bounded so a hung job cannot + # sit until the 6h default and be reported as "still running". + timeout-minutes: 10 + strategy: + fail-fast: false + matrix: + check: + - { name: "Ruff Lint", command: "ruff check ex_app/" } + - { name: "Ruff Format", command: "ruff format --check ex_app/" } + - { name: "Mypy", command: "mypy ex_app/" } + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + # Measured, not a floor: the Dockerfile's final stage is + # python:3.11-slim and it copies site-packages into + # /usr/local/lib/python3.11. Matches pyproject.toml. + python-version: "3.11" + + - name: Install quality tooling + # requirements-dev.txt pins ruff and mypy exactly. requirements.txt is + # installed 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 surfaced the Popen.stdout bug. + run: | + python -m pip install --upgrade pip + pip install -r requirements-dev.txt + pip install -r requirements.txt + + - name: ${{ matrix.check.name }} + run: ${{ matrix.check.command }} + + # ── The shared Conduction quality pipeline ──────────────────────────────── quality: if: github.event_name != 'push' || github.event.created != true uses: ConductionNL/.github/.github/workflows/quality.yml@main @@ -32,9 +88,11 @@ jobs: app-name: opentalk # composer.json pins config.platform.php to 8.3 php-version: "8.3" - # PHP-only ExApp: no package.json, so all npm-side checks are off + # No package.json in this repo, so all npm-side checks are off # (enable-npm gates the npm legs of security/license; enable-frontend - # gates Vue Quality and custom frontend checks). + # gates Vue Quality and custom frontend checks). NB the comment that + # used to sit here called this a "PHP-only ExApp" — it is a PYTHON + # ExApp; the only PHP in the tree is phpcs-custom-sniffs/. enable-npm: false enable-frontend: false # The SBOM job invokes `composer CycloneDX:make-sbom`, which this repo @@ -44,3 +102,28 @@ jobs: # No openspec/specs and no docs/features.json yet — the features check # would fail on every PR comparing "" against "[]". enable-features-extract: false + # Hydra Gates was never evaluated in CI here, for a boring reason: + # `enable-hydra-gates` defaults to false and this file did not pass it. + # The job's `if:` is `inputs.enable-hydra-gates && !cancelled()`, so it + # was the FIRST term that deleted the job, not the Playwright + # dependency. + # + # Measured before switching it on (--full scan of the whole tree, + # 2026-08-05): 29 of 63 gates reported, 0 failures. So this starts green + # honestly. The COVERAGE line is the real output here — most of the + # suite is PHP/Vue/Nextcloud-shaped and has no subject matter in a + # Python sidecar, and the gates say so by name rather than passing + # quietly. + enable-hydra-gates: true + # Deliberately NOT pinned to a tag. 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 every one of those gates was dead for as long as the pin + # stood. This repo's gate surface is small and its diffs are tiny, so + # tracking main costs little and inherits gate fixes the day they land. + hydra-gates-ref: main + # Left at its default (false) on purpose. 34 of the 63 gates have no + # subject matter in a repo with no lib/, no src/ and no manifest; + # demanding full coverage here would fail every PR for a condition no + # PR can fix. The coverage block prints either way, which is the part + # that matters. + # hydra-gates-require-full-coverage: false diff --git a/Makefile b/Makefile index 73eb358..934b85a 100644 --- a/Makefile +++ b/Makefile @@ -1,17 +1,41 @@ -.PHONY: build push test clean +.PHONY: help build push run clean lint format lint-fix format-fix mypy check check-strict APP_ID = opentalk REGISTRY = ghcr.io IMAGE = conductionnl/$(APP_ID)-exapp VERSION ?= latest +help: + @echo "$(APP_ID) ExApp" + @echo "" + @echo " make build Build the Docker image" + @echo " make push Push it to $(REGISTRY)" + @echo " make run Run the container locally (interactive; asserts nothing)" + @echo " make clean Remove the local image" + @echo "" + @echo " make lint ruff check ex_app/" + @echo " make format ruff format --check ex_app/" + @echo " make mypy mypy ex_app/" + @echo " make check lint + mypy" + @echo " make check-strict lint + format + mypy" + @echo "" + @echo "There is NO 'make test' target. This repo has no automated test" + @echo "suite of any kind, and a target that pretends otherwise is worse" + @echo "than its absence. 'make run' is what used to be called 'make test':" + @echo "an interactive 'docker run -it' that boots the container and" + @echo "asserts nothing." + build: docker build -t $(REGISTRY)/$(IMAGE):$(VERSION) . push: build docker push $(REGISTRY)/$(IMAGE):$(VERSION) -test: +# Renamed from `test`. It never tested anything — it starts the container +# interactively and makes no assertion, and cannot run in CI at all (-it needs +# a TTY). Calling that `test` is the same defect as `|| echo skipping`: a +# command whose name claims a verdict it never reaches. +run: docker run --rm -it \ -e APP_ID=$(APP_ID) \ -e APP_VERSION=0.1.0 \ @@ -22,3 +46,52 @@ test: clean: docker rmi $(REGISTRY)/$(IMAGE):$(VERSION) || true + +# ── Python quality ────────────────────────────────────────────────────────── +# The application is ex_app/lib/main.py. Until 2026-08-05 nothing in this repo +# looked at it: the static-analysis stack (phpcs/psalm/phpstan/phpmd) is aimed +# at phpcs-custom-sniffs/, and code-quality.yml ran only the PHP legs. +# Install the tools with: pip install -r requirements-dev.txt + +lint: + ruff check ex_app/ + +format: + ruff format --check ex_app/ + +lint-fix: + ruff check --fix ex_app/ + +format-fix: + ruff format ex_app/ + +mypy: + mypy ex_app/ + +check: + @E=0; \ + for CMD in lint mypy; do \ + echo; echo "=== $$CMD ==="; \ + $(MAKE) --no-print-directory $$CMD || E=1; \ + done; \ + echo; \ + if [ $$E -eq 0 ]; then echo "ALL CHECKS PASSED"; else echo "SOME CHECKS FAILED (see above)"; fi; \ + exit $$E + +check-strict: + @E=0; \ + for CMD in lint format mypy; do \ + echo; echo "=== $$CMD ==="; \ + $(MAKE) --no-print-directory $$CMD || E=1; \ + done; \ + echo; \ + if [ $$E -eq 0 ]; then \ + echo "ALL CHECKS PASSED - STATIC ANALYSIS ONLY."; \ + echo "This green covers ruff (lint + format) and mypy over ex_app/."; \ + echo "It says NOTHING about behaviour: this repo has no automated test"; \ + echo "suite - no pytest config, no test_*.py, no phpunit.xml, no tests/."; \ + echo "Do not add a test target until a real suite exists."; \ + else \ + echo "SOME CHECKS FAILED (see above)"; \ + fi; \ + exit $$E diff --git a/ex_app/lib/main.py b/ex_app/lib/main.py index cf966ca..979e103 100644 --- a/ex_app/lib/main.py +++ b/ex_app/lib/main.py @@ -8,7 +8,7 @@ import subprocess import threading import typing -from contextlib import asynccontextmanager +from contextlib import asynccontextmanager, suppress from pathlib import Path import httpx @@ -22,7 +22,6 @@ ) from nc_py_api.ex_app.integration_fastapi import AppAPIAuthMiddleware - # -- Logging ----------------------------------------------------------------- logging.basicConfig( level=logging.WARNING, @@ -40,10 +39,7 @@ APP_ID = os.environ.get("APP_ID", "opentalk") HARP_ENABLED = bool(os.environ.get("HP_SHARED_KEY")) -if HARP_ENABLED: - PROXY_PREFIX = f"/exapps/{APP_ID}" -else: - PROXY_PREFIX = f"/index.php/apps/app_api/proxy/{APP_ID}" +PROXY_PREFIX = f"/exapps/{APP_ID}" if HARP_ENABLED else f"/index.php/apps/app_api/proxy/{APP_ID}" # Keycloak/OIDC configuration KEYCLOAK_URL = os.environ.get("KEYCLOAK_URL", "") @@ -52,9 +48,7 @@ KEYCLOAK_CLIENT_SECRET = os.environ.get("KEYCLOAK_CLIENT_SECRET", "opentalk-secret") # Keycloak ExApp token API (for server-side auth) -KEYCLOAK_EXAPP_URL = os.environ.get( - "KEYCLOAK_EXAPP_URL", "http://openregister-exapp-keycloak:23002" -) +KEYCLOAK_EXAPP_URL = os.environ.get("KEYCLOAK_EXAPP_URL", "http://openregister-exapp-keycloak:23002") # AppAPI auth for ExApp-to-ExApp communication NEXTCLOUD_URL = os.environ.get("NEXTCLOUD_URL", "http://nextcloud") APP_SECRET = os.environ.get("APP_SECRET", "") @@ -63,9 +57,7 @@ # -- Local Port Proxy -------------------------------------------------------- KEYCLOAK_LOCAL_PORT = int(os.environ.get("KEYCLOAK_LOCAL_PORT", "8180")) -KEYCLOAK_INTERNAL_HOST = os.environ.get( - "KEYCLOAK_INTERNAL_HOST", "openregister-exapp-keycloak" -) +KEYCLOAK_INTERNAL_HOST = os.environ.get("KEYCLOAK_INTERNAL_HOST", "openregister-exapp-keycloak") KEYCLOAK_INTERNAL_PORT = int(os.environ.get("KEYCLOAK_INTERNAL_PORT", "8080")) @@ -83,9 +75,7 @@ def _accept(sock: socket.socket) -> None: client, _ = sock.accept() client.setblocking(False) try: - upstream = socket.create_connection( - (KEYCLOAK_INTERNAL_HOST, KEYCLOAK_INTERNAL_PORT), timeout=5 - ) + upstream = socket.create_connection((KEYCLOAK_INTERNAL_HOST, KEYCLOAK_INTERNAL_PORT), timeout=5) upstream.setblocking(False) except OSError: client.close() @@ -95,23 +85,29 @@ def _accept(sock: socket.socket) -> None: while True: for key, _ in sel.select(timeout=1): - if key.fileobj is srv: - _accept(key.fileobj) + # selectors types fileobj as `int | HasFileno`; everything we + # register here is a real socket. Narrow it for real rather than + # asserting it — a raw fd would otherwise reach .recv()/.close() + # and raise AttributeError inside the proxy loop. + conn = key.fileobj + if not isinstance(conn, socket.socket): + continue + + if conn is srv: + _accept(conn) else: data = None - try: - data = key.fileobj.recv(65536) - except OSError: - pass + with suppress(OSError): + data = conn.recv(65536) if data: try: key.data.sendall(data) except OSError: data = None if not data: - sel.unregister(key.fileobj) + sel.unregister(conn) sel.unregister(key.data) - key.fileobj.close() + conn.close() key.data.close() @@ -153,19 +149,29 @@ def start_opentalk() -> None: if KEYCLOAK_URL: LOGGER.info("OIDC configured with Keycloak at %s", KEYCLOAK_URL) - OPENTALK_PROCESS = subprocess.Popen( + proc = subprocess.Popen( ["/usr/local/bin/opentalk-controller"], env=env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, ) - - def log_output(): - for line in OPENTALK_PROCESS.stdout: + OPENTALK_PROCESS = proc + + # Bind the stream locally rather than reading the global. log_output() runs + # on a daemon thread for the life of the process, and stop_opentalk() sets + # OPENTALK_PROCESS back to None — so a thread that read the global would + # raise AttributeError on None the moment the controller was stopped and + # restarted. Popen.stdout is also Optional; mypy surfaced both when type + # checking was first turned on for this repo. + def log_output() -> None: + stream = proc.stdout + if stream is None: + return + for line in stream: LOGGER.info("[opentalk] %s", line.decode().strip()) threading.Thread(target=log_output, daemon=True).start() - LOGGER.info("OpenTalk controller started with PID: %d", OPENTALK_PROCESS.pid) + LOGGER.info("OpenTalk controller started with PID: %d", proc.pid) def stop_opentalk() -> None: @@ -219,8 +225,16 @@ def rewrite_frontend_paths() -> None: index_path = FRONTEND_DIR / "index.html" if index_path.is_file(): content = index_path.read_text() - for prefix in ["/assets/", "/fonts.", "/fonts/", "/favicon", "/config.js", - "/manifest.json", "/tflite/", "/locales/"]: + for prefix in [ + "/assets/", + "/fonts.", + "/fonts/", + "/favicon", + "/config.js", + "/manifest.json", + "/tflite/", + "/locales/", + ]: content = content.replace(f'"{prefix}', f'"{PROXY_PREFIX}{prefix}') content = content.replace(f"'{prefix}", f"'{PROXY_PREFIX}{prefix}") content = content.replace(f"({prefix}", f"({PROXY_PREFIX}{prefix}") @@ -416,9 +430,7 @@ async def get_auth_token(request: Request): # Call the Keycloak ExApp directly (container-to-container). # The /api/ routes are excluded from AppAPIAuthMiddleware and use # a shared secret for authentication. - keycloak_api_secret = os.environ.get( - "KEYCLOAK_API_SECRET", "keycloak-exapp-internal-secret" - ) + keycloak_api_secret = os.environ.get("KEYCLOAK_API_SECRET", "keycloak-exapp-internal-secret") async with httpx.AsyncClient() as client: resp = await client.post( f"{KEYCLOAK_EXAPP_URL}/api/token", @@ -453,9 +465,7 @@ def get_frontend_config() -> str: When server-side auth is available (Keycloak ExApp), the frontend is configured to skip OIDC login and use the injected token instead. """ - keycloak_browser_url = os.environ.get( - "KEYCLOAK_BROWSER_URL", "http://localhost:8180" - ) + keycloak_browser_url = os.environ.get("KEYCLOAK_BROWSER_URL", "http://localhost:8180") oidc_authority = f"{keycloak_browser_url}/realms/{KEYCLOAK_REALM}" return f"""window.config = {{ @@ -542,7 +552,7 @@ async def proxy(request: Request, path: str): except httpx.RequestError as e: LOGGER.error("Proxy error: %s", str(e)) return JSONResponse( - {"error": f"Proxy error: {str(e)}"}, + {"error": f"Proxy error: {e!s}"}, status_code=502, ) @@ -566,18 +576,19 @@ def _serve_index_html() -> Response: """Serve index.html with token bootstrap script injected. Injects a script that fetches a pre-authenticated Keycloak token from - the /api/auth/token endpoint and stores it in sessionStorage in the - format oidc-client-ts expects. This allows the OpenTalk frontend to - skip the OIDC login redirect (which CSP blocks in iframes). + the /api/auth/token endpoint and writes it into localStorage under the + keys the OpenTalk frontend reads (access_token / refresh_token / + id_token / server_time_offset). This allows the frontend to skip the + OIDC login redirect, which CSP blocks in iframes. + + The OIDC authority is NOT needed here — it is supplied to the frontend by + _build_config_js(), which is what window.config consumes. This function + used to compute it too and never use it; ruff F841 surfaced that when + linting was first turned on for this repo. """ index_path = FRONTEND_DIR / "index.html" html = index_path.read_text() - keycloak_browser_url = os.environ.get( - "KEYCLOAK_BROWSER_URL", "http://localhost:8180" - ) - oidc_authority = f"{keycloak_browser_url}/realms/{KEYCLOAK_REALM}" - # Inject bootstrap script before config.js. Nextcloud's AppAPI proxy # auto-adds CSP nonce to all