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