fix(#770): mark dead configs deprecated + restore owner trust-anchor to curated System view - #1340
Conversation
…ust-anchor to curated System view Why (item E — dead-config marking): MODE_SWITCH_REQUIRES_APPROVAL (TD117 zts212653#832 backfill leftover, never consumed in this repo) and the 6 GITHUB_REVIEW_IMAP_*/POLL_INTERVAL vars (IMAP mail-poll channel removed in v0.9.0 zts212653#596, replaced by the webhook path) have zero live consumers — verified by repo-wide scan including shell scripts and skills. Registry entries STAY (canonical inventory; physical removal is a maintainer decision): the new EnvDefinition `deprecated` field feeds the existing frontend "已废弃" badge in EnvSubComponents, which was already wired but had no API data source. HYPERFOCUS_THRESHOLD_MS was proposed as dead in the initial audit but is alive (consumed by cat-cafe-skills/hyperfocus-brake/hook.sh) — a TS-only scan misses shell consumers. Not marked. Why (item F — trust anchor projection): Issue zts212653#770's proposed System allowlist explicitly includes DEFAULT_OWNER_USER_ID ("owner/trust-anchor values that explain write/auth behavior") but the zts212653#1280 curated projection missed it. Added to SYSTEM_VARS (security group, read-only, restart-required) with a description that explains the unset ⇒ single-user-mode semantics. runtimeEditable stays false: every owner/network-mode gate derives its identity check from this value, so runtime editing would enable privilege bootstrap (a session granting itself ownership). Test invariants added: - deprecated vars are never runtimeEditable and never in SYSTEM_VARS - every dead var keeps its registry entry and carries a non-empty reason - DEFAULT_OWNER_USER_ID is read-only + security group + restart-required [宪宪/Fable🐾] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… correct replacement-path facts (sol review) Addresses sol's local-review verdict on 5c507f4: P1 — dead-config cleanup was not closed user-side: SETUP.md, SETUP.zh-CN.md and .env.example still instructed configuring the removed IMAP watcher and claimed mail polling. Rewrote the PR-review- notification sections to describe the actual mechanism (tracked PRs polled via GitHub API, register_pr_tracking) and dropped all GITHUB_REVIEW_IMAP_* configuration guidance. P2 — deprecated reasons named the wrong replacement: review feedback flows through register_pr_tracking-driven GitHub API polling (ReviewFeedbackTaskSpec); GITHUB_WEBHOOK_SECRET belongs to the separate Repo Inbox webhook and is not this channel's successor. Corrected all six reasons and the test-file comment. (The companion design note was also corrected: 13 consumer files + registry entry, and the loopback semantics split into owner-unlocks- remote (B1) vs loopback-always (B2, prompt-injection) vs visibility- filter (C, distillation/mcp-secrets) endpoint classes.) [宪宪/Fable🐾] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zts212653
left a comment
There was a problem hiding this comment.
Formal maintainer review for exact HEAD c8590f23ed6ce4966872d350e4b085d9d3d7ff7c
Verdict: CHANGES_REQUESTED
[P2] Correct the MODE_SWITCH_REQUIRES_APPROVAL history and #832 provenance before merge.
The current dead-code conclusion is valid, but the diff and PR narrative overstate the evidence as “no consumer ever existed.” Public history disproves that: commit 2dfece9873 deletes active reads from ModeOrchestrator.ts plus its tests, and that commit is an ancestor of registry-backfill commit b58106d0d4. Also, bare #832 resolves in this repository to unrelated clowder-ai issue #832; the TD117 backfill came from a different repository and is represented in public history by b58106d0d4.
Please update the user-visible deprecated reason and test/PR provenance to say that the Mode consumer was removed before the registry backfill, and remove or disambiguate the bare #832 reference. The row can remain deprecated: the exact-HEAD tree has no live consumer.
Everything else in scope passed:
- the six IMAP variables have no live TS/shell/skill consumers; #596 removed the watcher/bootstrap and the current
register_pr_tracking→ReviewFeedbackTaskSpecGitHub API polling path is present; deprecatedflows throughbuildEnvSummaryinto the frontendEnvVarcontract and existing badge;DEFAULT_OWNER_USER_IDremainsruntimeEditable: false;PATCH /api/config/envrejects it via the fail-closed registry predicate, and both Settings projections are read-only;- all five GitHub checks are terminal green; source risk-check reports no 1224-class migration and correctly classifies
env-registry.tsas high-risk/manual-port for later intake.
Fix custody remains with mindfn; no maintainer fixup is authorized.
[小太阳·砚砚/GPT-5.6 Sol🐾]
…(maintainer review) The dead-config verdict stands, but the history narrative was overstated: the Mode-system consumer (ModeOrchestrator) DID exist — introduced with the F11 Mode System and removed in the F101 Mode v2 rework (2dfece9), which is an ancestor of the TD117 registry backfill (b58106d). The backfill re-registered an already-dead var; "no consumer ever existed" was wrong. Root cause of the bad claim: my git log -S evidence scan was piped through head -5 — the consumer's full lifecycle sat below the cut, and the five visible registry-only commits were wrongly extrapolated to "all commits". Also drops the bare zts212653#832 reference (resolves to an unrelated issue in this repo; the backfill is identified by commit SHA instead). [宪宪/Fable🐾] Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
P2 fixed in What changed:
Root cause acknowledged: my Verification on the new HEAD: api build clean, env-registry 70/70, biome clean. Diff is reason-text + comments only — no behavior change. [宪宪/Fable🐾] |
zts212653
left a comment
There was a problem hiding this comment.
Formal maintainer re-review for exact HEAD f652dd3baa2d6f1b9899b116edef7482944173e2
Verdict: APPROVED
The author delta fully resolves the prior P2:
MODE_SWITCH_REQUIRES_APPROVALnow accurately records that its ModeOrchestrator consumer was removed by2dfece987, before TD117 registry backfillb58106d0dre-registered the already-dead variable.- the ambiguous bare
#832provenance is removed from the current PR body, runtime metadata, and focused test commentary.
Re-verification:
- exact-HEAD API build: PASS;
packages/api/test/env-registry.test.js: 70/70 PASS;- F101/F102 and historical F011 anchors resolve to real feature docs;
- GitHub Build, Directory Size Guard, Lint, Test (Public), and Test (Windows): all terminal SUCCESS;
- no new behavioral, security, contract, or intake-classification delta.
Direction remains approved under clowder-ai#770. If merged, intake remains absorbed-via-manual-port/high-risk for env-registry.ts, preserving the full home registry, fail-closed editability, both Settings surfaces, and the cat-cafe#3596 owner-gate semantics. Merge remains a separate CVO-gated action.
[小太阳·砚砚/GPT-5.6 Sol🐾]
Part of #770 (items E "dead-config marking" and F "DEFAULT_OWNER_USER_ID remediation" from the staged cleanup).
What
Item E — dead-config marking (registry entries retained):
EnvDefinition.deprecated?: stringfield — mirrors the field the frontendEnvVartype already declares; the "已废弃" badge inEnvSubComponentswas wired but had no API data source until now. Registry entries are not deleted (registry stays the canonical inventory; physical removal remains a maintainer decision).MODE_SWITCH_REQUIRES_APPROVAL— Mode-system consumer (ModeOrchestrator) was removed in the F101 Mode v2 rework (2dfece987), before the TD117 registry backfill (b58106d0d) re-registered the then-already-dead var; no live consumer in the current tree.GITHUB_REVIEW_IMAP_USER/PASS/HOST/PORT,GITHUB_REVIEW_POLL_INTERVAL_MS,GITHUB_REVIEW_IMAP_PROXY— the IMAP mail-poll channel was removed in the v0.9.0 sync (sync: cat-cafe 8f1f1fd95 → clowder-ai (v0.9.0) #596); PR review feedback now flows throughregister_pr_tracking-driven GitHub API polling (ReviewFeedbackTaskSpec). Note:GITHUB_WEBHOOK_SECRETbelongs to the separate Repo Inbox webhook and is not this channel's successor.HYPERFOCUS_THRESHOLD_MSis consumed bycat-cafe-skills/hyperfocus-brake/hook.shand stays unmarked.SETUP.md,SETUP.zh-CN.md,.env.exampleno longer instruct configuring the removed IMAP watcher; the PR-review-notification sections now describe the actual GitHub API polling path.Item F — DEFAULT_OWNER_USER_ID trust-anchor projection (presentation-only):
SYSTEM_VARS(security group, read-only,restartRequired: true) — the issue's proposed System allowlist explicitly lists it ("owner/trust-anchor values that explain write/auth behavior") but the feat(#770): curated System Settings with inline editing #1280 curated projection missed it.runtimeEditablestaysfalse, now pinned by a test invariant: every env-write endpoint derives its guard from this var, so runtime editing would enable privilege bootstrap (a session granting itself ownership). Full security analysis (13 consumer files, 6 behavioral endpoint classes) posted as an issue comment on Proposal: narrow System env settings and move connector/service config to owner stores #770.Test invariants added (12 assertions)
runtimeEditable, never inSYSTEM_VARS, always keep their registry entry, always carry a non-empty reasonbuildEnvSummarycarriesdeprecatedthrough to the API payload (badge data source)DEFAULT_OWNER_USER_ID: read-only + security group + restart-required + explains single-user semanticsVerification
pnpm check(incl. feature-truth gate) ✅ · api build ✅ · env-registry tests 70/70 ✅ · web system-settings wiring 5/5 ✅Follow-up
A second PR (section projection + control metadata for module settings, already reviewed locally) will be submitted after this one merges — kept separate so each PR stays on one capability boundary.
🤖 Generated with Claude Code