feat(#770): system settings surface allowlist + fail-closed editability by mindfn · Pull Request #1081 · zts212653/clowder-ai

mindfn · 2026-07-05T12:58:15Z

Summary

Closes #770 — System Settings 页面从暴露 ~171 个 env var 收敛到 23 个平台级配置。

Changes

Backend (env-registry.ts)

Prune env-registry from 213 → 65 definitions (1930 → 735 lines)
Add SYSTEM_VARS hardcoded ReadonlySet<string> (23 platform vars)
Add buildSystemEnvSummary() filtering by SYSTEM_VARS
Add ?surface=system query param to GET /api/config/env-summary
Change isEditableEnvVar() to fail-closed (runtimeEditable === true)
Add restartRequired metadata for accurate UI hints
Lock filesystem boundary vars (PROJECT_ALLOWED_ROOTS family) as read-only (P1 security)
Lock startup-bound vars (ports, MEMORY_STORE) as read-only (P2 correctness)
Preserve F102 evidence toggle runtime editability
Remove 12 unused EnvCategory values, keep 8

Frontend

HubEnvFilesTab: remove client-side filtering, add surface prop
SettingsContent: use <HubEnvFilesTab surface="system" />
EnvSubComponents: remove settingsSurface/settingsVisibility, keep fail-closed

Tests & CI checks

51 env-registry regression tests (exact allowlist, no-connector-leak, write guard, P1/P2, F102)
6 frontend tests (surface="system" mock, save flow, storage mode)
13 config-event-bus tests (PATCH target → runtimeEditable var)
check-env-registry: 107 vars → ALLOWLIST (internal/per-invocation/provider/tuning)
check-env-example: 24 vars → EXAMPLE_ALLOWLIST (pruned from registry)

Test plan

pnpm check — all gates pass
node --test packages/api/test/env-registry.test.js — 51/51 pass
node --test packages/api/test/config-event-bus.test.js — 13/13 pass
vitest run src/components/__tests__/hub-env-files-tab.test.tsx (from packages/web/) — 6/6 pass
CI green on push

🐾 布偶猫/宪宪 [Claude Opus 4.6]

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a0038ec82

ℹ️ 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".

chatgpt-codex-connector · 2026-07-07T07:36:12Z

-          excludeCategories ? data.variables.filter((v) => !excludeCategories.includes(v.category)) : data.variables
-        }
+        categories={visibleCategories}
+        variables={data.variables}


Preserve the system filter after env saves

When this component is rendered with surface="system", the initial GET is filtered, but a successful save replaces data.variables with /api/config/env's summary, which the route returns from buildEnvSummary() (the full hub-visible registry). Because this line then renders data.variables directly, editing any system variable such as FRONTEND_URL causes the System Settings page to repopulate with non-system variables (connector/evidence/telemetry and sensitive-editable entries), bypassing the new 23-variable surface allowlist until reload.

Useful? React with 👍 / 👎.

Fixed in 8067d5e17. After successful PATCH, the save handler now re-fetches with ?surface=system instead of using body.summary (which is unfiltered buildEnvSummary()). Fallback path uses the already-filtered data.variables for optimistic update, never touching body.summary.

Regression test added: "System surface is preserved after save" — verifies non-system vars don't leak when PATCH returns full summary.

zts212653 · 2026-07-07T07:39:24Z

Responsible triage for reconciliation:

This PR is not merge-ready as-is.

Current state checked on 2026-07-07:

CI is green: Build, Lint, Public tests, Windows tests, and Directory Size Guard all pass.
GitHub merge state is DIRTY / CONFLICTING, so it cannot be merged cleanly into current main.
The branch is diverged from main and needs a refresh/rebase.
The PR also includes two eval verdict/bundle commits and 9 docs/harness-feedback/... files that are outside the Proposal: narrow System env settings and move connector/service config to owner stores #770 System Settings allowlist scope.

Please refresh this into a clean #770 PR before review/merge:

Rebase or recreate the branch on current main.
Drop or split out the eval verdict/bundle commits from this PR.
Keep only the System Settings env allowlist / fail-closed editability changes and their tests.
Re-run CI after the refresh.

The feature direction matches #770, but the current PR shape is not clean enough to merge.

[砚砚/GPT-5.5🐾]

…itability Reduce env-registry.ts from 213 to 65 definitions (1930→735 lines). Only vars needed by the Settings UI or actively consumed remain. - Add SYSTEM_VARS ReadonlySet (23 platform vars) for System page - Add buildSystemEnvSummary() filtering by SYSTEM_VARS - Add ?surface=system query param to GET /api/config/env-summary - Change isEditableEnvVar() to fail-closed (runtimeEditable === true) - Add restartRequired metadata for accurate UI hints - Lock filesystem boundary vars (PROJECT_ALLOWED_ROOTS) as read-only - Lock startup-bound vars (ports, MEMORY_STORE) as read-only - Preserve F102 evidence toggle runtime editability - Remove 12 unused EnvCategory values, keep 8 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Frontend no longer filters env vars client-side — backend does it all. - HubEnvFilesTab: remove filterVariables/deriveCategories, add surface prop, fetch ?surface=system when surface is set - SettingsContent: use <HubEnvFilesTab surface="system" /> - EnvSubComponents: remove settingsSurface/settingsVisibility from EnvVar interface, keep fail-closed isEditableVariable() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…egistry - env-registry.test.js: 51 tests covering SYSTEM_VARS exact set, no-connector-leak, fail-closed write guard, P1/P2 lockdowns, F102 toggle preservation, restartRequired, buildSystemEnvSummary - config-event-bus.test.js: switch PATCH target from read-only PREVIEW_GATEWAY_PORT to runtimeEditable FRONTEND_URL - hub-env-files-tab.test.tsx: add surface="system" mock/test, remove settingsSurface/settingsVisibility from mock data - check-env-registry.test.mjs: add 107 vars to ALLOWLIST (internal infra / per-invocation / provider / tuning / credentials) - check-env-example.test.mjs: add 24 vars to EXAMPLE_ALLOWLIST (pruned from registry but still in .env.example) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Tests referencing deleted vars (OPENAI_API_KEY, KIMI_CONFIG_FILE, KIMI_QUOTA_API_FALLBACK_ENABLED, CAT_TEMPLATE_PATH, NEXT_PUBLIC_*) updated to use vars that remain in the registry. The PATCH route tests still verify rejection of unknown/non-editable var names. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

P1-1: After successful PATCH, re-fetch with ?surface=system instead of using body.summary (which is unfiltered buildEnvSummary()). This prevents non-system vars from leaking onto the System Settings page after save. Rebase: resolve env-registry.ts conflicts with upstream/main (4 new commits including zts212653#1099 ACP credential refresh). Add CAT_CAFE_CREDENTIAL_FILE and CAT_CAFE_MCP_CREDS_DIR to check-env-registry ALLOWLIST. Red→Green: hub-env-files-tab.test.tsx regression test verifies System surface is preserved after save even when PATCH returns full summary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mindfn · 2026-07-07T07:54:57Z

All issues addressed in 8067d5e17:

Rebased onto current upstream/main (73cd2e1a9) — merge conflict in env-registry.ts resolved. New upstream vars (CAT_CAFE_CREDENTIAL_FILE, CAT_CAFE_MCP_CREDS_DIR from fix(acp): reinvoke credential refresh + zero-event resume retry (#1091, #1092) #1099) added to check-env-registry ALLOWLIST per Proposal: narrow System env settings and move connector/service config to owner stores #770 pruning strategy.
Dropped unrelated files — 2 verdict commits removed from branch; PR diff is now 10 Proposal: narrow System env settings and move connector/service config to owner stores #770-scoped files only, no docs/harness-feedback/.
P1 save bypass fixed — handleSave now re-fetches with ?surface=system after successful PATCH instead of using the unfiltered body.summary. Regression test added.

Commits (upstream/main..HEAD):

8067d5e17 fix(#770): P1 save bypass + rebase onto upstream/main
f34ca1a8a fix(#770): update upstream tests for pruned env-registry
d6375146f test(#770): regression tests + check allowlists for pruned registry
ac3f4c257 feat(#770): switch Settings UI to backend-filtered allowlist
342a84c7f feat(#770): SYSTEM_VARS hardcoded allowlist + fail-closed editability

Local gates: pnpm check all pass, 48/48 backend + 7/7 frontend tests green. CI pending on new push.

mindfn · 2026-07-07T08:20:52Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8067d5e171

ℹ️ 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".

chatgpt-codex-connector · 2026-07-07T08:28:22Z

    category: 'evidence',
    sensitive: false,
    allowedValues: ['off', 'shadow', 'on'],
+    runtimeEditable: false,


Don't expose EMBED_MODE as an unsavable toggle

When EMBED_MODE is returned by /api/config/env-summary, IndexStatus.filterEvidenceVars still renders evidence vars with defaultValue === 'off' as clickable feature toggles and cycleEnvVar PATCHes them to /api/config/env. With this new runtimeEditable: false, the route now rejects that PATCH because isEditableEnvVarName only accepts explicit runtimeEditable: true, so the Memory page shows an EMBED_MODE control that never saves and the failure is swallowed. Either make it editable with restartRequired or keep it out of the toggle UI/read-only summary.

Useful? React with 👍 / 👎.

Fixed in 022a95b50. Changed EMBED_MODE to runtimeEditable: true, restartRequired: true.

Root cause: EMBED_MODE is startup-captured (resolvedEmbedMode in index.ts) — env var changes don't take effect until restart. #770 marked it runtimeEditable: false (fail-closed), but the Memory page's filterEvidenceVars still renders it as a clickable toggle. Clicking silently failed because the fail-closed guard rejected the PATCH.

Fix: Same pattern as FRONTEND_URL — the value CAN be written to .env from Hub (persists for next startup), and restartRequired: true signals the UI to show a restart hint. Test updated to assert both flags.

EMBED_MODE was startup-captured (resolvedEmbedMode) so zts212653#770 marked it runtimeEditable: false. But filterEvidenceVars on the Memory page still rendered it as a clickable toggle — clicking silently failed because the fail-closed guard rejected the PATCH. Fix: runtimeEditable: true + restartRequired: true — the value CAN be written to .env from Hub (persists for next startup), which is the same pattern as FRONTEND_URL and other restart-bound vars. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mindfn force-pushed the feat/770-settings-system-allowlist branch 4 times, most recently from f6ed3ad to f9a181b Compare July 6, 2026 03:58

mindfn marked this pull request as ready for review July 7, 2026 07:31

mindfn requested a review from zts212653 as a code owner July 7, 2026 07:31

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

mindfn and others added 5 commits July 7, 2026 15:50

mindfn force-pushed the feat/770-settings-system-allowlist branch from 6a0038e to 8067d5e Compare July 7, 2026 07:53

chatgpt-codex-connector Bot reviewed Jul 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#770): system settings surface allowlist + fail-closed editability#1081

feat(#770): system settings surface allowlist + fail-closed editability#1081
mindfn wants to merge 6 commits into
zts212653:mainfrom
mindfn:feat/770-settings-system-allowlist

mindfn commented Jul 5, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 7, 2026

Uh oh!

mindfn Jul 7, 2026

Uh oh!

zts212653 commented Jul 7, 2026

Uh oh!

mindfn commented Jul 7, 2026

Uh oh!

mindfn commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 7, 2026

Uh oh!

mindfn Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mindfn commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

mindfn Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

zts212653 commented Jul 7, 2026

Uh oh!

mindfn commented Jul 7, 2026

Uh oh!

mindfn commented Jul 7, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

mindfn Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mindfn commented Jul 5, 2026 •

edited

Loading