Skip to content

Design-debt roll-up: lower-severity findings from Fable review of PR #232 -> HEAD #282

Description

@richardmhope

Summary

Roll-up of lower-severity design debt found across the PR #232 → HEAD review. Individually minor; grouped here so they don't get lost. Each is small and local.

Config / admin surface

  • _validation hand-duplicates config._guard_production (services/effective_config.py:207-225 vs config.py:420-448) — secret-key length, SQLite, wildcard FORWARDED_ALLOW_IPS conditions and message text copied by hand. A fourth prod guard will silently not appear on /admin/config, and the hub's "Effective config" pill will report "0 issues" on a config that refuses to boot. test_validation_reflects_prod_guards pins only the current guards. Extract a single source both call.
  • Readiness predicates live outside their owning modules — MISP/webhook/audit/SSO readiness is inlined in admin_hub_tiles rather than in each settings module (only AI follows the validate_selection pattern), so each will drift from its subsystem's real send-path requirements.
  • RSS tile shows "N ACTIVE" green with the poller off (effective_config.py:362-368) — rss_poll_enabled defaults false, so enabled feeds are fetched only on a manual POST; the tile gives no hint automatic collection is off.
  • Zero tests hit GET/POST /admin/oidc (web/admin_oidc.py:24,81) — gates are correct in code but a regression opening the OIDC console to ANALYST would pass the suite (unlike /admin and /admin/config, which have role-gate tests).

AI settings

  • Dead admin controlai_embeddings_enabled/ai_embedding_model are surfaced as a working toggle (admin_ai.html:65-73) but nothing reads them; related.py:48 always uses local_embedding. A no-op control on a governance console is worse than none. Wire it or drop it with a "not yet implemented" label.
  • Settings-change audit omits base_url and model (web/admin_ai.py:86-95) — the one field that can redirect the API key (see the openai-compatible issue) isn't captured, so repointing it looks like a no-op save in the trail. Both are non-secret; log old→new.
  • timeout is unbounded server-side (web/admin_ai.py:65,81) — min="1" is client-side only; 0/negative/1e9 are accepted. Clamp or validate.
  • Three hand-maintained backend vocabularies_BACKEND_CHOICES, _AI_BACKENDS, _BACKENDS; an out-of-vocab value silently coerces to "none"/"AMBER" with no feedback. Derive choices from _AI_BACKENDS.
  • probe() calls the private backend._complete (services/ai.py:371-391), duplicating exception-mapping run() owns and bypassing the OUTBOX metadata trail every other egress gets. A check() method on AIBackend keeps fail-soft mapping in one place.

OIDC

  • Entra-specific preferred_username email fallback promoted into the generic base (auth/oidc/base.py:101-102) — wrong for Okta, where it's commonly a login name; a provider omitting email then provisions User.email = "jdoe", which flows into dissemination + JWT claims. Move the fallback into EntraAdapter.
  • Downgrade of the multi-provider migration fails on real multi-provider data (migrations/versions/c3d4e5f6a7b8…:83-92) — it recreates ix_user_email unique, but duplicate emails across providers are the designed norm; on SQLite the partial batch failure leaves the schema mid-flight. (Upgrade itself is correct.)
  • Unused logger (auth/routes.py:34) — its only consumer, the default-role fallback warning, was deleted; that warning was useful operator signal (a silent STAKEHOLDER default is now invisible in logs). Consider restoring the warning rather than deleting the logger.
  • Provider a half-enabled with only a client_id (services/oidc_settings.py:56-73) — missing locator or env secret surfaces as a runtime 500 at first login instead of a config-time error; no save-time validation, and a role-map typo is silently dropped by parse_role_map.

UX / portal

  • noscript save fallback is non-functional and would be destructive (report_edit.html:101-105,164-175) — the hidden version has no server-rendered value (posts "" → 422), and textareas render empty (content lives only in the JSON island), so a "successful" submit would blank the report. A presence-only test (test_portal.py:631) pins the broken fallback. Render real fallback values or drop the noscript button + comment.
  • Feed context is O(N) lazy loads, computed twice per GET (services/feed.py:47-107, web/feed.py:48-51) — per-event lazy loads of tags/audience_groups/requirements; mark_visible_read re-runs visible_items, recomputing every context again. Use selectinload like dissemination.matched_stakeholders.
  • _needs_you_now loads every draft in the deployment to keep the caller's 5 (web/notebooks.py:127-136) — push the author filter into SQL + limit.
  • Delivery-reason honesty is asymmetric (services/feed.py:47-75) — a changed level preference gets the honest "outside your current preference" treatment, but a changed tag filter still shows a positive "Matches your … preference" chip on a report the current filters would never deliver. Also _match's docstring misstates the routing model as tag→audience→level ordering when it's a conjunction.
  • A11y: notebook phase tabs expose no state to AT (notebook_detail.html:49-57) — active tab conveyed only by is-active class; no aria-current/aria-selected. The same PR's dock_tab macro shows the repo's full ARIA pattern.
  • Dashboard "+ Start a notebook" tile scrolls to a closed <details> instead of opening it (dashboard.html:109→122).

CI / release (minor)

  • docker build + Trivy gate never runs pre-merge (ci.yml:129-131,161-168) — if: github.event_name != 'pull_request', so a Dependabot base-digest bump that breaks the build/trips Trivy is discovered only on the post-merge main push — main red at exactly the commit a v* tag would release. Run the docker job on PRs that touch Dockerfile/uv.lock (non-required, so no deadlock).
  • release.yml has no concurrency group (release.yml:11-14) — two v* tags close together can land :latest in the wrong order. Add a group without cancel-in-progress.
  • Over-broad cosign verify identity in docs (docs/RELEASING.md:76-78) — the regexp accepts a cert from any workflow on any ref in the repo; tighten to release.yml@refs/tags/v.*.

Found in Fable review of PR #232 → HEAD.

Metadata

Metadata

Assignees

No one assigned

    Labels

    fable-reviewCorrectness findings from Claude Fable code reviewpriority: lowNice to have / later

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions