Skip to content

fix: gate hosted admin panel routes behind deployment mode (Azure/GPT-RAG#592) - #271

Merged
Paulo Lacerda (placerda) merged 6 commits into
developfrom
placerda-feature-hosted-panel-contract
Sep 3, 2026
Merged

fix: gate hosted admin panel routes behind deployment mode (Azure/GPT-RAG#592)#271
Paulo Lacerda (placerda) merged 6 commits into
developfrom
placerda-feature-hosted-panel-contract

Conversation

@placerda

Copy link
Copy Markdown
Contributor

Summary

Fixes the concrete hosted/panel contract defect from Azure/GPT-RAG#592. Released v2.5.0 mounted the ingestion /dashboard admin SPA and admin API routes unconditionally, in every deployment mode, and never consumed DEPLOY_ADMINISTRATIVE_PANEL. A hosted/no-panel deployment therefore exposed the exact same administrative surface as classic and hosted/panel deployments — a fail-open violation of the frozen ADR-0001 hosted-panel contract.

What changed

  • utils/deployment_mode.py (new) — resolves CLASSIC / HOSTED_NO_PANEL / HOSTED_PANEL once at startup from App Configuration (DEPLOY_HOSTED_AGENT_ORCHESTRATION, DEPLOY_ADMINISTRATIVE_PANEL). Hosted/panel mode fails closed at startup (os._exit(1)) if the panel-only Cosmos account/database aren't configured.
  • main.py — moved all mode-dependent mounting out of module import time and into the ASGI lifespan, gated on the resolved mode. Classic behavior is byte-for-byte unchanged. Hosted/no-panel mounts none of the admin/panel surface (true 404, no panel Cosmos required). Hosted/panel mounts the admin surface and the new panel API. POST /retrieve is untouched — it already self-gates on its own hosted-retrieval flags, independent of admin/panel mode. A structural mode change requires a restart (config is read once at startup, matching the requirement).
  • api/panel.py (new) — the repository-local pieces of the ADR-0001 hosted-panel contract:
    • GET /api/panel/status — live readiness check.
    • GET/POST /api/panel/feedback — Cosmos-backed curation/feedback metadata, reusing the orchestrator's existing dashboard/Cosmos contract (no new storage contract invented).
    • GET /api/panel/overview — aggregates jobs/files/feedback into one dashboard payload; degrades gracefully on transient Cosmos errors instead of failing the whole request.
    • All routes require the Entra Admin app role. A missing/misconfigured tenant is a hard 500, never a silent bypass — there is no development-mode auth bypass when the panel is enabled. No token or conversation/protected content is ever logged.
  • Teststests/test_deployment_mode.py (24), tests/test_panel_api.py (20), tests/test_main_deployment_gating.py (5) covering flag gating, no-panel route absence, panel readiness/failure modes, authorization, and the history/feedback/overview contracts. Also fixed a latent sys.modules leak in tests/test_blob_metadata.py that only surfaced once the new tests were collected as part of the full suite (verified pre-existing via git stash on unmodified develop).
  • Docs — new README section documenting both flags and the panel endpoints/failure contracts; CHANGELOG.md entry under [Unreleased].

Explicit, precise blocker (why this is a draft PR)

GET /api/panel/conversations/{id}/history intentionally returns 501 Not Implemented. Retrieving managed Foundry Conversation history — the actual chat history shown by the panel per ADR-0001 — requires a cross-repo API contract between gpt-rag-ingestion, gpt-rag-orchestrator, and Azure AI Foundry that does not exist yet. This repository cannot safely define that contract unilaterally, and I am not fabricating a fake/success-shaped history response. This is tracked as remaining coordination under Azure/GPT-RAG#592.

For the same reason, no frontend Panel UI (history/feedback/overview tabs) is added in this PR. The existing admin SPA (frontend/src/App.tsx) needs no changes for the no-panel fail-closed requirement (the backend static-mount fix alone prevents /dashboard from serving anything in hosted/no-panel mode). Building new frontend tabs now would either require faking Foundry history data or shipping a UI with a hole where history should be, so it's deferred pending the cross-repo decision above.

Everything else in scope (mode gating, fail-closed validation, feedback/curation, dashboard overview, authorization, tests, docs) is complete and repo-local.

Test results

  • Backend: python -m pytest -q159 passed, 0 failed.
  • Frontend: npm run test (vitest) → 1 passed, 0 failed.

Remaining coordination

Related: Azure/GPT-RAG#592

…-RAG#592)

Previously the admin SPA (/dashboard) and admin API routes were mounted
unconditionally at import time regardless of hosted deployment mode, and
DEPLOY_ADMINISTRATIVE_PANEL was never consumed. A hosted/no-panel
deployment therefore exposed the same administrative surface as classic
and hosted/panel deployments, violating the frozen ADR-0001 hosted-panel
contract.

- Add utils/deployment_mode.py: resolves CLASSIC / HOSTED_NO_PANEL /
  HOSTED_PANEL from App Configuration (DEPLOY_HOSTED_AGENT_ORCHESTRATION,
  DEPLOY_ADMINISTRATIVE_PANEL) once at startup, with fail-closed Cosmos
  resource validation for hosted/panel.
- Rework main.py lifespan() to resolve the mode, validate required
  resources (os._exit(1) on failure), and mount the admin surface and new
  panel API from inside lifespan instead of at module import time.
  Classic behavior is unchanged; hosted/no-panel mounts nothing
  admin/panel-related; hosted/panel mounts both. POST /retrieve is
  unaffected (self-gates on its own hosted-retrieval flags).
- Add api/panel.py implementing the repository-local pieces of the
  ADR-0001 panel contract: GET /api/panel/status, GET/POST
  /api/panel/feedback (Cosmos-backed curation metadata reusing the
  orchestrator's dashboard/Cosmos contract), GET /api/panel/overview.
  All routes require the Entra Admin app role with no dev-mode bypass.
  GET /api/panel/conversations/{id}/history returns 501: retrieving
  managed Foundry Conversation history needs a still-undefined
  cross-repo API and is out of scope for this repository alone.
- Fix tests/test_blob_metadata.py: its module-level sys.modules stubbing
  for utils/utils.file_utils leaked permanently and broke collection of
  the new tests when the full suite ran; now cleaned up in a
  try/finally.
- Add tests/test_deployment_mode.py, tests/test_panel_api.py,
  tests/test_main_deployment_gating.py covering flag gating, no-panel
  absence, panel readiness/failure modes, authorization, and the
  history/feedback/overview contracts.
- Document the deployment-mode flags and panel API in README.md and add
  a CHANGELOG.md entry.

Full backend suite: 159 passed. Frontend suite: 1 passed. No frontend
Panel UI is added in this change (deferred; would require either faking
Foundry history data or shipping an incomplete UI given the cross-repo
history-retrieval gap above).

Related: Azure/GPT-RAG#592

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…/feedback

FeedbackRecord(**doc) construction previously happened outside the Cosmos
read's try/except, so a single stored document with an invalid rating
value or a missing required field raised an unhandled pydantic
ValidationError -> unhandled 500 for every caller, indefinitely, until the
bad document was manually purged from Cosmos.

Each document is now validated individually. Malformed documents are
never silently dropped from the response; if any are found, the endpoint
raises a sanitized 502 naming the count of unreadable documents (reusing
the route's existing 502 Cosmos-failure contract) instead of returning a
silently-filtered partial list. Document content (comments, tags,
conversation IDs) is never logged or included in the error detail.

Adds three focused tests: invalid rating literal, missing required field,
and confirms document content never leaks into the response/logs. Updates
CHANGELOG under Unreleased/Fixed.

Full backend suite: 161 passed. Frontend suite: 1 passed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@placerda

Copy link
Copy Markdown
Contributor Author

Fix: unhandled data-integrity crash in GET /api/panel/feedback

Independent review found that FeedbackRecord(**doc) list construction sat outside the Cosmos read's ry/except. A single stored feedback document with an invalid
ating value (or missing a required field like id/conversationId/createdAt) raised an unhandled pydantic.ValidationError, which FastAPI turned into an unhandled 500 — permanently, for every caller, until the bad document was manually purged from Cosmos.

Fix (commit �f4658c):

  • Each stored document is now validated individually inside list_feedback().
  • Malformed documents are never silently dropped from the response (that would hide data corruption from the caller).
  • If any malformed documents are found, the endpoint raises a sanitized 502 naming only the count of unreadable documents — reusing the route's existing 502 Cosmos-failure response contract rather than inventing a new response shape.
  • Document content (comments, tags, conversation IDs) is never logged or included in the error detail, consistent with the module's existing "never log conversation/protected content" discipline.

Tests added ( ests/test_panel_api.py):

  • est_feedback_list_returns_502_on_malformed_document — invalid
    ating literal value; also asserts document content never leaks into the response or logs.
  • est_feedback_list_returns_502_on_malformed_document_missing_required_field — missing required createdAt.
  • Existing happy-path/regression tests ( est_feedback_list_success_and_filters_by_conversation_id, est_feedback_list_returns_502_on_cosmos_failure) still pass unchanged.

Full suite results:

  • Backend: 161 passed (python -m pytest -q), up from 158 (net +3 new panel tests).
  • Frontend: 1 passed (npm run test), no regressions, no frontend changes required.

CHANGELOG updated under Unreleased/Fixed.

…l feedback API

Second remediation round for PR #271 review feedback:

- list_feedback(): remove the "rating" not in doc prefilter that silently
  skipped documents missing the rating key without incrementing
  invalid_count. All dict documents now flow through the same
  FeedbackRecord(**doc) validation/except path, so a missing rating is
  counted and surfaced via the existing sanitized 502 data-integrity
  response, exactly like an invalid rating value.
- create_feedback(): wrap client.create_document(...) in try/except so a
  raised exception (not just a None return) is caught and converted into
  the same documented sanitized 502, instead of propagating as an
  unhandled 500. No document/body content is logged.

Tests added/updated in tests/test_panel_api.py:
- test_feedback_list_success_and_filters_by_conversation_id: removed the
  stale "junk" doc that codified the old silent-skip behavior.
- test_feedback_list_returns_502_on_document_missing_rating_entirely: new,
  asserts a document missing rating entirely now triggers 502 with an
  accurate invalid count.
- test_feedback_create_returns_502_on_cosmos_exception: new, simulates
  create_document raising and asserts 502 with no leaked content in the
  response or logs.
- _FakeCosmosDBClient: added create_error simulation, reset in
  _install_stubs for test isolation.

Backend: 163 passed (up from 161).
Frontend: 1 passed, no regressions.

Related: Azure/GPT-RAG#592

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@placerda

Copy link
Copy Markdown
Contributor Author

Second remediation round: two remaining explicit error-handling gaps closed

Addressed both gaps identified in re-review:

  1. Silent drop of rating-less feedback documents. The prior fix added an invalid_count/502 data-integrity contract for malformed documents, but a leftover prefilter (if not isinstance(doc, dict) or "rating" not in doc: continue) special-cased documents missing the
    ating key entirely and skipped them before that counter ran — silently excluding them from the response instead of counting them. Removed the special case; every dict document now flows through the same FeedbackRecord(**doc) validation, so a missing
    ating is treated identically to an invalid one (counted, 502 raised).
  2. Unhandled 500 on create_document exception. create_feedback() only checked for a None return from client.create_document(...); a raised exception propagated as an unhandled 500 instead of the documented 502 write-failure contract. Wrapped the call in ry/except Exception, logging only a fixed message (no body/content) and raising the same sanitized HTTPException(502, ...) used for the None case.

Tests ( ests/test_panel_api.py)

  • Removed the stale {"id": "junk", "noRatingField": True} doc from the success test — it previously codified the silent-skip bug.
  • Added est_feedback_list_returns_502_on_document_missing_rating_entirely — a document missing
    ating now triggers 502 with an accurate invalid count.
  • Added est_feedback_create_returns_502_on_cosmos_exception — simulates create_document raising, asserts 502 and that no request content (comment/tags) leaks into the response or logs.
  • _FakeCosmosDBClient gained create_error simulation support, reset per-test in _install_stubs().

Verification

  • ests/test_panel_api.py: 24 passed (was 22).
  • Full backend suite: 163 passed (was 161).
  • Full frontend suite: 1 passed, no regressions.

Commit: 11f08ea

Related: Azure/GPT-RAG#592

validate_panel_resources previously validated only the panel's Cosmos
account/database, so a hosted/panel deployment with Cosmos configured
but OAUTH_AZURE_AD_TENANT_ID/OAUTH_AZURE_AD_CLIENT_ID unset would pass
startup, mount every /api/panel/* route, then 500 on the first request
inside require_panel_admin/validate_bearer_jwt instead of failing
closed at startup as ADR-0001 requires.

Startup validation now also requires an Entra tenant ID and client ID
(honoring the same legacy CLIENT_ID fallback dependencies.py's JWT
validation already accepts, so startup and runtime agree on what
"Entra configured" means) and exits with the same actionable
PanelResourceError when either is missing/blank. HOSTED_NO_PANEL is
unaffected.

Adds 7 focused tests to tests/test_deployment_mode.py covering the
CLIENT_ID fallback pass case, missing tenant, missing client (with and
without fallback), and the HOSTED_NO_PANEL no-op. Full backend suite:
170 passed (was 163). Frontend suite: 1 passed.

Related: Azure/GPT-RAG#592

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@placerda

Copy link
Copy Markdown
Contributor Author

Third re-review fix: fail closed on missing Entra config at hosted-panel startup

Gap: validate_panel_resources (utils/deployment_mode.py) validated only the panel's Cosmos account/database. A hosted/panel deployment with Cosmos configured but OAUTH_AZURE_AD_TENANT_ID/OAUTH_AZURE_AD_CLIENT_ID unset would pass startup validation, mount every /api/panel/* route, and only then 500 on the first real request inside require_panel_admin/validate_bearer_jwt -- a "starts up healthy, then 500s everything" failure mode that violates the ADR-0001 "no development-mode auth bypass" invariant, which is a startup-time contract, not a runtime one.

Fix: Startup validation now also requires an Entra tenant ID and client ID, honoring the same legacy CLIENT_ID fallback that dependencies.py's JWT validation already accepts (so startup and runtime agree on what "Entra configured" means -- no stricter, no looser). Missing/blank keys raise the same PanelResourceError -> os._exit(1) fail-closed path already used for Cosmos, with an updated message naming exactly which keys are missing. HOSTED_NO_PANEL is unaffected -- it never requires Entra any more than it requires panel Cosmos.

Tests added (tests/test_deployment_mode.py, 7 new):

  • Passes with legacy CLIENT_ID fallback (no OAUTH_AZURE_AD_CLIENT_ID set)
  • Fails closed: missing/blank OAUTH_AZURE_AD_TENANT_ID (3 cases)
  • Fails closed: missing OAUTH_AZURE_AD_CLIENT_ID with tenant set (2 cases, including with CLIENT_ID also blank)
  • HOSTED_NO_PANEL no-op even with nothing configured

Verification:

  • Backend: python -m pytest -q -> 170 passed (was 163)
  • Frontend: npm run test -> 1 passed

Commit: cf9f7c0 (pushed to placerda-feature-hosted-panel-contract)

Related: Azure/GPT-RAG#592

@placerda

Copy link
Copy Markdown
Contributor Author

Final independent review

Follow-ups ef4658c, 11f08ea, and cf9f7c0 make malformed feedback reads/writes explicit, remove silent corruption drops, and fail hosted-panel startup when Cosmos or Entra tenant/client configuration is missing. Final local verification: 170/170 backend tests and 1/1 frontend test passed; final focused review found no remaining high-confidence security or correctness issues.

This PR correctly remains draft: managed Foundry Conversation history retrieval and the corresponding panel frontend are still a cross-repo contract blocker; /api/panel/conversations/{id}/history returns explicit 501 rather than fake data. A new ingestion release must not be published/promoted until that contract is completed and live hosted/no-panel/panel behavior is repeated.

@placerda

Copy link
Copy Markdown
Contributor Author

The hosted/panel scope is deferred to Azure/GPT-RAG#611. Keep this PR in draft. Its fail-closed mode gating and tests remain useful input, but the intentional managed-history 501 and missing panel frontend will not be promoted in the first hosted preview. That preview requires DEPLOY_ADMINISTRATIVE_PANEL=false.

Paulo Lacerda and others added 2 commits September 3, 2026 13:58
Resolves the two conflicts introduced by 50 commits of develop drift.

main.py: keeps develop's fail-closed `panel_operator_router` (issue #611 /
ADR-0004) alongside this PR's `_mount_admin_and_panel_surface()` gate. The
module-level `admin_router` include and the `/dashboard` static mount move
inside `admin_surface_enabled(mode)`, which is the purpose of this PR --
a hosted/no-panel deployment must not expose the admin surface. Classic
mode is unchanged: `admin_surface_enabled` returns True for CLASSIC.

CHANGELOG.md: rebases this PR's `[Unreleased]` entries onto develop's
published history (v2.6.0, v2.7.0, v2.7.1, v2.7.2). Drops the duplicated
`POST /retrieve` entry, which was already published in v2.6.0.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5fb4d435-911e-4149-aef6-5f393697c46e
`_mounted_paths()` imported `fastapi.routing._IncludedRouter` unconditionally.
That private wrapper only exists on FastAPI >=0.139, while `requirements.txt`
pins `fastapi==0.115.12`, so all four tests that used the helper failed with
`ImportError` on the version this service actually ships.

On the pinned version `include_router()` copies `APIRoute` objects into
`app.routes` eagerly, so the shallow scan is already complete. Make the import
optional and keep the recursive path for newer FastAPI.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5fb4d435-911e-4149-aef6-5f393697c46e
@placerda

Copy link
Copy Markdown
Contributor Author

Conflito com develop resolvido e branch atualizado.

O que estava conflitando (2 arquivos):

  • main.py — o develop tinha ganho o panel_operator_router (issue #611) na mesma região que este PR reescreve. As duas mudancas sao complementares, nao concorrentes: a resolucao mantem o panel_operator_router do develop e move o admin_router + o SPA estatico para dentro de _mount_admin_and_panel_surface(mode), sob if admin_surface_enabled(mode). O admin_router deixa de ser incluido incondicionalmente — que era exatamente a falha descrita em #592.
  • CHANGELOG.md — a entrada de POST /retrieve deste branch (GPT-RAG#596) ja tinha sido publicada no develop na v2.6.0, entao foi descartada como duplicata. O ## [Unreleased] resultante contem apenas as entradas de #592.

README.md fez auto-merge sem intervencao.

Correcao adicional em tests/test_main_deployment_gating.py: o helper _mounted_paths() importava fastapi.routing._IncludedRouter incondicionalmente. Esse wrapper privado so existe em FastAPI >=0.139, enquanto o requirements.txt pina fastapi==0.115.12 — os quatro testes que usavam o helper falhavam com ImportError na versao que o servico realmente entrega. O import passou a ser opcional; na versao pinada o include_router() e eager e a varredura rasa de app.routes ja e suficiente.

Validacao local: 228 passed, 0 failed com o requirements.txt do repositorio. O workflow Tests tambem passou neste branch.

@placerda
Paulo Lacerda (placerda) marked this pull request as ready for review September 3, 2026 17:22
@placerda
Paulo Lacerda (placerda) merged commit 22d257d into develop Sep 3, 2026
2 checks passed
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