feat: console: gate Settings ▸ Secrets behind secrets-panel dev flag#2121
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe Secrets settings section is now controlled by the ChangesSecrets panel gating
Test environment isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant FlagRegistry
participant FlagResolution
participant SettingsSurface
participant PersistedSelection
FlagRegistry->>FlagResolution: Register secrets-panel at dev tier
FlagResolution->>SettingsSurface: Provide flag state
PersistedSelection->>SettingsSurface: Provide selected section id
SettingsSurface->>SettingsSurface: Filter navigation and active-id resolution
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/settings/secretsGate.test.ts`:
- Around line 17-21: Strengthen the test named “flag-off sections are filtered
from nav AND from id resolution” by verifying that persisted “secrets” active-id
resolution uses the filtered sections collection. Prefer a behavioral assertion;
otherwise assert that active is derived from sections rather than the unfiltered
registry, while retaining the existing navigation-filtering checks.
In `@apps/web/src/settings/SettingsSurface.tsx`:
- Around line 93-96: Align the `secrets-panel` ADR documentation with the
canonical registration: update the comment in
`apps/web/src/settings/SettingsSurface.tsx` lines 93-96 and the test
documentation in `apps/web/src/settings/secretsGate.test.ts` lines 5-11 from ADR
0068 to ADR 0080; no behavior changes are needed.
In `@tests/conftest.py`:
- Around line 57-65: Update the environment cleanup loop in the test setup to
also remove PROTOAGENT_HOST_CONFIG before test defaults are applied, ensuring
setdefault uses the deterministic /nonexistent/... value rather than an
inherited desktop-app configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 963f4952-942e-4c2c-aa7e-a5f23168bd93
📒 Files selected for processing (4)
apps/web/src/settings/SettingsSurface.tsxapps/web/src/settings/secretsGate.test.tsruntime/flags.pytests/conftest.py
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head fcd9fc86b80b · formal
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
Overall risk: low. All three findings land in test files (one in test infrastructure, two in a new test file), none in production code. No correctness defects in the actual feature.
Fix first: Finding #2 — the secretsGate.test.ts assertions only check that certain patterns exist in source text, never verifying that the gating function shown() is actually called or filters by flag. This test would pass even if the gating logic were entirely deleted, giving false confidence. Strengthen it to exercise the runtime behavior.
Panel agreement: All three findings confirmed by the verifier; no disagreements. Verdicts unchanged from the initial review pass.
Verification note: The verifier could not fetch individual PR files (404s at the head SHA for github_read_pr_file/github_read_file), but the full diff was sufficient to confirm every claim. Finding #3's line number was corrected from 20 to ~13 based on diff inspection.
Coverage gaps: None of the finder reports surfaced explicit Gap: lines. The test-only nature of the PR means production logic wasn't directly reviewed in this pass, but that's inherent to the diff — no gap to flag.
[
{
"file": "apps/web/src/settings/secretsGate.test.ts",
"line": 19,
"severity": "minor",
"category": "tests",
"claim": "Test 'flag-off sections are filtered from nav AND from id resolution' only asserts structural existence of `shown` and `sections` in source, not that `shown()` is called on `sections` or filters based on `flag` — giving false confidence that the gating is enforced.",
"evidence": "expect(src).toContain(\"const shown = (list: Section[])\");\nexpect(src).toMatch(/const sections = \\[\\s*\\.\\.\\.agentSections/);",
"verdict": "confirmed",
"note": "Both assertions are pure string-pattern checks on raw source text. Neither verifies that `shown()` is called, that it receives `sections`, or that it filters by `flag`. The test would pass if both patterns exist anywhere in the file, even if the gating logic were deleted."
},
{
"file": "apps/web/src/settings/secretsGate.test.ts",
"line": 13,
"severity": "minor",
"category": "correctness",
"claim": "The regex `/id: \"secrets\"[^}]*flag: \"secrets-panel\"/` couples to object-key order — reordering keys (a valid refactor) breaks the test despite the flag still being present, risking the guard being weakened or removed during a benign change.",
"evidence": "expect(src).toMatch(/id: \"secrets\"[^}]*flag: \"secrets-panel\"/); — requires `id` to lexically precede `flag` in the object literal.",
"verdict": "confirmed",
"note": "The regex requires `id` before `flag` because `[^}]*` cannot cross a `}` boundary, so `{flag: \"secrets-panel\", id: \"secrets\"}` (keys reordered) would not match. A benign refactor swapping object keys would produce a false-negative test failure. (Line-number corrected from 20 to ~13 per verifier inspection of the diff.)"
},
{
"file": "tests/conftest.py",
"line": 56,
"severity": "minor",
"category": "conventions",
"claim": "Loop variable `_leaked` uses underscore prefix (conventionally 'intentionally unused') but is consumed inside the loop body, misleading readers and linters configured to suppress `_`-prefixed names.",
"evidence": "for _leaked in (\n \"PROTOAGENT_HOME\",\n ...\n):\n os.environ.pop(_leaked, None)",
"verdict": "confirmed",
"note": "Diff shows `os.environ.pop(_leaked, None)` — the variable IS consumed. Python's `_`-prefix convention (PEP-8/ruff F841) does signal 'intentionally unused'; using it for a consumed loop variable is misleading to both humans and lint suppressors."
}
]
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 77b0ebd7b6f7 · formal
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
Overall risk: Low. The PR adds a single flag property to an existing shown() filtering pipeline already proven in production (settings.devices). The runtime change is a one-line decoration; no new control flow or state management.
Fix-first: Strengthen secretsGate.test.ts to actually verify that a flag-off section is excluded from the rendered output — the current test greps for symbol names without exercising the filter, making flag-off regressions invisible to CI (finding #1).
Panel disagreement: None — correctness, cross-file, and conventions reviews converged on the same two test weaknesses, and the structural (protopatch) pass independently surfaced all of them.
What verification changed: The only-prop-unused finding was dropped: it's factually correct but targets pre-existing code (line 110 of SettingsSurface.tsx), not code touched by this PR (lines 90–97). The remaining three findings were confirmed with detailed notes.
Gaps: The structural pass completed. One coverage gap: SettingsSurface.tsx lines 90–97 — the actual shown(AGENT_SECTIONS) wiring — was not examined by any finder for correctness of the filter callback itself. The test gap means the implementation is unverified at both unit and integration levels.
[
{
"file": "apps/web/src/settings/secretsGate.test.ts",
"line": 19,
"severity": "minor",
"category": "tests",
"claim": "Test 'flag-off sections are filtered from nav AND from id resolution' only asserts structural existence of `shown` and `sections` in source text, not that `shown()` is called on `AGENT_SECTIONS` or filters based on `flag` — giving false confidence that the gating is enforced. Flagged by correctness, cross-file, and conventions reviews.",
"evidence": "expect(src).toContain(\"const shown = (list: Section[])\");\nexpect(src).toMatch(/const sections = \\[\\s*\\.\\.\\.agentSections/); — two independent pattern checks with no connecting assertion.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Test only greps for symbol names in raw source; never calls shown() with a flag-off scenario or asserts filtered output. The shown() wiring (agentSections = shown(AGENT_SECTIONS)) exists in the component body but is not verified by this test."
},
{
"file": "apps/web/src/settings/secretsGate.test.ts",
"line": 13,
"severity": "minor",
"category": "correctness",
"claim": "The regex `/id: \"secrets\"[^}]*flag: \"secrets-panel\"/` couples to object-key order — reordering keys (a valid refactor) breaks the test despite the flag still being present, risking the guard being weakened or removed during a benign change. Flagged by correctness, cross-file, and conventions reviews.",
"evidence": "expect(src).toMatch(/id: \"secrets\"[^}]*flag: \"secrets-panel\"/); — `[^}]` cannot cross a `}` boundary, so `{flag: \"secrets-panel\", id: \"secrets\"}` would not match.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Verified from diff: the regex requires id before flag and [^}] can't cross closing brace. Reversing keys breaks the match while the object is semantically identical."
},
{
"file": "apps/web/e2e/mock-server.mjs",
"line": 361,
"severity": "minor",
"category": "cross-file",
"claim": "Mock-server comment claims the 'flag-off (prod) path is covered by the source-level secretsGate.test unit test,' but that test never verifies flag-off behavior — it only checks that source patterns exist, so the cross-file coverage claim is false.",
"evidence": "mock-server.mjs: \"its flag-off (prod) path is covered by the source-level secretsGate.test unit test.\" vs secretsGate.test.ts: only asserts `src.toContain('const shown')`, `src.toMatch(/const sections = [...]/)`, and `flags.toMatch(/tier=\"dev\"/)` — none verifies a flag-off section is actually removed.",
"verdict": "confirmed",
"note": "Diff confirms the comment (line ~360 in new file) explicitly claims flag-off coverage by secretsGate.test. That test has zero assertions about flag-off filtering — it only greps for symbol existence. The claim in the comment is false."
}
]
…ns, test behavior not source text The QA panel's WARN on the first head was right: the gate test greped source for symbol names, so a regression in the actual filtering would stay green. Extract the pure filter to settings/sectionGate.ts (the visibility.ts/fieldVisible pattern), wire SettingsSurface through it, and rewrite secretsGate.test.ts to exercise flag-off exclusion + flag-on inclusion by id, with the remaining source guards made order-insensitive (object-scoped match, not cross-key regex). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hh8gD6749kPXoBK9Wsd1FA
|
WARN adjudication (protoEngineer's brain, per the review-processing flow): fix-before-merge — this ticket exists to keep a pre-release panel off prod, and tests that grep for symbol names can't prove the gate works; dropping the |
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head dc254004bae0 · formal
[review-synthesizer completed: workflow code-review-structural:report]
Prose Brief
Overall risk: negligible. The PR extracts an inline .filter() from SettingsSurface.tsx into a new visibleSections utility (sectionGate.ts), adds a secrets-panel feature flag in runtime/flags.py and mock-server.mjs, and ships unit tests. The extraction is byte-for-byte identical — no logic changed. The verifier confirmed the empty findings array from four independent finders is genuine; there is nothing here that could regress behavior.
Fix-first item: none — the PR is clean.
Panel disagreements: none. All finders returned nothing, and the verifier agreed.
What verification changed: the verifier confirmed that the pre-existing only prop dead-code issue in SettingsSurface.tsx was correctly excluded as out-of-scope (the diff doesn't touch that line) and that the extraction preserves semantics exactly.
Gaps: no structural pass was skipped. One cosmetic gap worth noting (not a code defect): the verifier observed an ADR 0068/0080 terminology inconsistency in comments, but no finder flagged it and it does not affect correctness.
[]
…rtifacts, fleet inputs/secrets (#2130) Bump 0.106.0 → 0.107.0 and roll the changelog for the release. Highlights: - Versioned file artifacts + save_file_artifact (ADR 0092 D2, #2126) - Document-generation stack baked into the desktop bundle (ADR 0092 D1, #2123) - Fleet agents accept operator inputs + secrets at create time (#2121/#2122/#2125/#2127) - Deterministic persona-drift detection (#2116) - Chat tab context menu Close Others/Left/Right (#2112) - Fixes: archetype-catalog bundling (#2115), PyPI publishes on tag push (#2113) Backfilled [Unreleased] entries for the feature PRs that lacked them, rolled to [0.107.0], scaffolded + polished the marketing changelog. uv.lock: project version line only (surgical — avoids uv 0.11.13-vs-pinned-0.11.29 marker drift). Claude-Session: https://claude.ai/code/session_01EVmWoy2TdXdMshNn5WBR2Z Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
console: gate Settings ▸ Secrets behind
secrets-paneldev flag (bd-1oq)See the diff for details.
Summary by CodeRabbit
New Features
Bug Fixes
Tests