fix(web): resolve PR #130 blockers in /config and /api/config#131
Merged
Conversation
Three blockers found in code review of the RFC-015 web GUI:
1. config_page (/config HTML route) called _to_dict, which was defined as
a nested function inside get_config_endpoint. Every render hit a
NameError that was silently swallowed by a bare except, leaving
config_yaml = "" and forcing the SPA to fall back to /api/config.
Promoted _to_dict to a module-level _config_to_dict helper used by
both routes.
2. update_config compared top-level payload keys against a set of
dotted-path restart-required fields, so {"embedding": {"provider": "x"}}
was reported as applied: ["embedding"], pending_restart: [], silently
telling operators a restart-required change had taken effect.
Added _flatten_keys to walk the nested payload to dotted leaf paths
and lifted the restart set to module scope as _RESTART_REQUIRED_FIELDS.
3. /config HTML route had no auth guard. /api/config was protected, but
the page shell (and once #1 was fixed, the server-rendered YAML body)
was reachable without an API key. Added Depends(require_api_guard)
to the route and made the YAML body redact secrets before serializing.
Tests:
Added four regression tests in tests/test_web_api.py covering nested
restart-required flags, nested non-restart application, /config auth
gate (via monkeypatch of API_KEY because it is captured at module import
time), and /config server-side YAML render. 24 passed, 2 skipped.
Note on the original review's blocker #2 (multi-tenancy regression on
graph and entity endpoints): not a regression. There is no per-tenant
MemoryManager._knowledge_graph anywhere in community or enterprise.
get_knowledge_graph() is a process-wide singleton; get_mm_for_request
is a stub returning the default MemoryManager. The old code's
getattr(tenant_mm, "_knowledge_graph", None) always returned None,
so the new singleton call is a functional improvement, not a regression.
No code change needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dad863d1c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes post-merge blockers in the web config UI/API by addressing config serialization errors, correct restart-required detection for nested updates, and ensuring the /config HTML editor is auth-gated with secret redaction.
Changes:
- Promote config serialization helper to module scope and use it for
/api/configand/configrendering. - Correct restart-required detection by flattening nested payload keys into dotted leaf paths.
- Gate
/configbehindrequire_api_guardand add regression tests for restart detection + page auth/rendering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
web/app.py |
Adds _config_to_dict, _flatten_keys, _RESTART_REQUIRED_FIELDS, fixes /api/config serialization, corrects restart detection in PUT /api/config, and auth-gates /config while redacting before YAML dump. |
tests/test_web_api.py |
Adds regression tests for nested restart-required detection and for /config auth gating + non-empty server-rendered YAML body. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three blockers found while reviewing PR #130 after it had already been merged. Fixes follow up on
8730499(v2.6.0).config_pageNameError:_to_dictwas a closure insideget_config_endpoint, so every render of/confighit aNameErrorthat was silently swallowed by a bareexcept, leavingconfig_yamlempty. Promoted to module-level_config_to_dict.update_configrestart-detection: compared top-level payload keys against a set of dotted-path restart-required fields, so{"embedding": {"provider": "x"}}was reported asapplied: ["embedding"],pending_restart: []— telling operators a restart-required change had taken effect when it had not. Added_flatten_keyswalker;appliedandpending_restartnow contain dotted leaf paths./configHTML route was unauthenticated:/api/configwas protected, but the page shell (and its server-rendered YAML body once the NameError was fixed) was reachable without an API key. AddedDepends(require_api_guard)and made the YAML body redact secrets before serialization.Note on the original review's blocker #2
I originally flagged a "multi-tenancy regression" on the graph/entity/storage endpoints. That was wrong: there is no per-tenant
MemoryManager._knowledge_graphanywhere in community or enterprise.get_knowledge_graph()is a process-wide singleton, andget_mm_for_requestis a stub returning the defaultMemoryManager. The old code'sgetattr(tenant_mm, "_knowledge_graph", None)always returnedNone, so the old graph endpoints were effectively dead. The new singleton call is a functional improvement, not a regression. No code change needed.Test plan
tests/test_web_api.py:test_put_config_nested_restart_required_is_flaggedtest_put_config_nested_non_restart_is_appliedTestConfigPage::test_config_page_requires_auth_when_key_setTestConfigPage::test_config_page_renders_yaml_body_when_authenticatedpytest tests/test_web_api.py: 24 passed, 2 skipped (was 20 + 2).ruff check web/app.py: 22 pre-existing issues; my edits cleared theF821 Undefined name '_to_dict'blocker (down from 24).🤖 Generated with Claude Code