← The appendix · generated 2026-10-21
Infisical: every bot comment, every Heal defect
An open-source secrets-management platform.
46 PRs of its real history (2026-10-14 → 2026-10-16) replayed on a private copy, with CodeRabbit, cubic and Greptile on every PR at their default settings, Greptile's TREX on; the repo's own .greptile/config.json travels with the code and applies to Greptile. On the Heal side, a full run before the span (t1), one midway (t2) and one after it (t3).
100 defect-claim comments on 46 PRs, 94 distinct claims once the same point from two bots is merged; the 190 walkthroughs, summaries and "no issues" posts the bots also wrote are listed but not counted. 538 Heal defects shown, 21 of them on lines a replayed PR wrote, the rest on code from before the PRs tested. 16 security defects left out.
What ran, and when
The bots ran on their vendors' hosted services, on the settings above. The vendors publish no version number, so the posting dates below are the only identity of what reviewed these PRs.
- coderabbit: 96 comments, posted between 2026-10-20 13:19 and 2026-10-21 00:24 UTC
- cubic: 133 comments, posted between 2026-10-20 13:26 and 2026-10-21 00:24 UTC
- greptile: 61 comments, posted between 2026-10-20 13:30 and 2026-10-21 00:31 UTC
Security defectsSecurity defects are left out of this appendix: the full run found 16 on this product, and its maintainers have not been told about them yet. Publishing an unfixed security defect before the people who can fix it have heard of it would put their users at risk, so those rows wait for the disclosure. They are counted in every figure of the benchmark page.
The comments are shown as the bots posted them, with no verdict on whether they hold; the same-issue judge only links a comment to the Heal defect it names. A Heal defect is a failing test on the running product. Download the comments and the defects as CSV.
This product's caveats
- Judge truth: no human graded a sample on this product, so every judge-based number is the model's alone.
- Blame names the last PR that touched a line, not the one that created the defect, so 'several PRs' and 'pre-span' are upper bounds on what a reviewer could have caught.
- The defect ledger is cumulative over the runs: a defect filed by a later run may sit on lines an earlier PR of the span wrote.
- The run after the span saw the code 14 PRs past the last replayed PR (the replay was stopped early by hand); a defect whose lines those later PRs wrote falls outside the window.
- The bots' account settings are the vendors' defaults with Greptile's TREX on; the repo's own .greptile/config.json travels with the code and applies to Greptile.
- Cost sums every metrics row of the workspace's runs, stopped attempts included; a run's summary card shows one segment only.
| PR | Bot | Where | Claims | The comment |
|---|---|---|---|---|
| #8635 2026-10-20 13:19 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The announcement modal now places navigation controls in a footer below the card stack. The footer includes Previous and Next/Done buttons, clickable announcement indicators, and a current-position display. The Storybook description reflects the updated controls.
**Changes: Announcement modal**
|Layer / File(s)|Summary|
|:---|:---|
|**Card stack and footer navigation** `frontend/src/components/announcements/AnnouncementModal.tsx`, `frontend/src/components/v3/platform/AnnouncementModal.stories.tsx`|The modal uses a flexible card-stack container and a bottom pager. The pager provides Previous and Next/Done buttons, clickable indicators, and a current-position display. Next cl … the rest (2459 characters)oses the modal when the current announcement is the last one. The Storybook description documents the footer controls.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Feature **Suggested reviewers:** `andrewhuhh` Merge Risk: 🔵 Low · up to 043be On very narrow screens the Next or Done button may be partly pushed out of view in the announcement modal. This is a small layout issue that can be fixed after merge or just before it. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main UI change: making announcement modal navigation easier to discover. It follows the conventional commit format. | | Description check | | The description includes the required Context, Screenshots, Steps to verify the change, Type, and Checklist sections. It explains the previous and new behavior, documents verification steps, identifie… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes the required Context, Screenshots, Steps to verify the change, Type, and Checklist sections. It explains the previous and new behavior, documents verification steps, identifies the change as an improvement, and records testing and contribution-guide checks. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8635 2026-10-20 13:23 | coderabbit inline · P2 | frontend/src/components/announcements/AnnouncementModal.tsx:292 | defect-functional | **Keep the dot row within the footer.**
The backend returns at most five announcements, but five fixed-width dots can still overflow a narrow footer. The pager does not wrap, and the navigation buttons cannot shrink. Constrain or horizontally scroll the dot row so `Next` or `Done` remains visible. |
| #8635 2026-10-20 13:23 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8635 2026-10-20 13:26 | cubic review-body | — | summary | **No issues found** across 2 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8635 2026-10-20 13:30 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** The change is safe to merge within the reviewed scope: navigation and keyboard-focus checks passed, and Done retains the existing close callback.
Summary
Replaces the side arrows with a navigation bar below the announcement card.
- The announcement modal puts clear navigation controls below each multi-card announcement.
Browser checks passed for button navigation, dot jumps, arrow keys, peeking previews, count synchronization, keyboard focus, Done closing, and the single-announcement layout.
No actionable issues were found. The change is safe to merge based on the reviewed scope.
Reviews (1) · Last reviewed commit: "improvement(ui): make announcem … the rest (40 characters)ent modal..." · Reviewed by the reviewer |
| #8632 2026-10-20 13:32 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Select now supports controlled and uncontrolled open state. Overlay layers register Escape handlers and dispatch Escape events to the most recently registered popup. Select processes each event once and closes only when the event is not prevented.
**Changes: Select Escape handling**
|Layer / File(s)|Summary|
|:---|:---|
|**Overlay popup Escape dispatch** `frontend/src/components/v3/utils/overlay-layer.ts`, `frontend/src/components/v3/utils/overlay-layer.test.ts`|Overlay layers register optional Escape handlers, propagate popup registrations and dismissal events to parent layers, and invoke the most recently reg … the rest (4438 characters)istered handler. Tests cover event cancellation, handler ordering, and cleanup.| |**Select open state and Escape integration** `frontend/src/components/v3/generic/Select/Select.tsx`|Select supports controlled and default open state, exposes state and Escape-event tracking through context, and registers popup dismissal handling. SelectContent invokes the optional Escape callback and closes only when the event is not prevented.| **Priority:** ⬇️ Low **Estimated code review effort:** 3 (Moderate) | ~20 minutes **Change:** Bug fix ### Sequence Diagram(s) ```mermaid sequenceDiagram participant guardDismiss participant SelectContent participant SelectOpenState guardDismiss->>SelectContent: Invoke the registered Escape handler SelectContent->>SelectContent: Process the keyboard event once SelectContent->>SelectOpenState: Close if the event is not prevented ``` **Suggested reviewers:** `andrewhuhh` Merge Risk: ⚪ Minimal · up to 185e9 No actionable Select dismissal issue remains identified. The failed Northflank build still needs diagnosis before the deployment is ready. Security architecture risk: 🔵 Low · up to 185e9 The change is confined to popup dismissal and preserves cancellation and parent-dismissal guards. No introduced security vulnerability was demonstrated. Remaining uncertainty concerns browser event ordering and mounted-component cleanup across nested overlays. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The demonstrated exposure is browser-side dismissal behavior for a Select popup and its enclosing overlay ancestry. The inspected event path ends in UI callbacks and open-state updates; it does not demonstrate increased tenant, credential, service, or data-store authority. **Trust Boundaries and Controls** - _observed_ — Cancellation gates the registered Select close request, while event marking prevents the same Escape from also reaching an ancestor dismissal callback, including after synchronous unregistering. These are UI containment controls, not authentication or authorization boundaries. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the primary change: preventing Select Escape events from dismissing parent overlays. It follows the conventional commit format. | | Description check | | The description includes all required sections, explains the problem and solution, provides screenshots and verification steps, identifies the change type, and completes the checklist. It also clearly… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes all required sections, explains the problem and solution, provides screenshots and verification steps, identifies the change type, and completes the checklist. It also clearly reports the remaining failed Northflank check and merge restriction. ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8632 2026-10-20 13:43 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No blocking code defects were identified in the reviewed changes. Overall merge readiness remains blocked on the acknowledged Northflank preview-build failure, and PierreOucif has withheld merge authorization.
Summary
This PR keeps Select Escape events from closing enclosing overlays. Select shares its open state and cancelable Escape handler with the overlay layer, and popup registrations and consumed events propagate through enclosing layers.
No actionable issues were found. On the current head, 40 browser cases passed across nested Dialog/Sheet compositions, cancellation, controlled refusal, callback delivery, and focus restoration. Twenty dis … the rest (1137 characters)patched Escape synchronously when an option received focus; twenty used native keyboard input. The equivalent immediate-focus cases reproduced the enclosing-overlay dismissal on the base revision. All four focused bookkeeping tests passed, including original-event consumption through registration cleanup. These checks cover synthetic component compositions, not authenticated product/provider acceptance or every possible event interleaving. PierreOucif acknowledges the failed Northflank build and explicitly withholds merge authorization; its cause is not established by these changes. Overall merge readiness remains blocked on that acknowledged preview-build failure. No merge authorization is granted here. ### the check validation blocked Evidence upload could not be completed because this environment exposes no artifact-upload mechanism. Execution completed and evidence remains available locally, but no uploaded artifact references are claimed. - Escape closes an open Select before reaching its parent overlay. Reviews (1) · Last reviewed commit: "fix(ui): keep Select Escape from dismiss..." · Reviewed by the reviewer |
| #8632 2026-10-20 13:43 | cubic review-body | — | summary | **No issues found** across 3 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8462 2026-10-20 13:45 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The change adds PAM folder alerts for access-request events, emits those events within approval transactions, and adds resource-aware alert configuration to the frontend. It renames the PAM alerting entitlement while preserving legacy license-key support and existing Slack notification configurations.
**Changes: PAM folder alerting**
|Layer / File(s)|Summary|
|:---|:---|
|**Enterprise alerting entitlement compatibility** `backend/src/ee/services/license/*`, `backend/src/services/license-client/*`, `frontend/src/hooks/api/subscriptions/types.ts`, `frontend/src/components/license/UpgradeGate/*`, `tests/fakes/license/features.go`|The PAM feature name changes from Slack notific … the rest (7338 characters)ations to Enterprise alerting. Legacy Slack entitlement keys still enable the renamed feature.| |**Transactional approval event emission** `backend/src/services/approval-policy/*`, `backend/src/ee/services/pam-access-request/pam-access-approval-resource.ts`, `backend/src/ee/services/pam-access-request/pam-access-request-events.ts`, `backend/src/server/routes/index.ts`, `backend/CLAUDE.md`|The approval policy service emits resource-built events within request, approval, rejection, and bypass transactions. PAM access-request events identify their folder and request targets.| |**PAM folder alert provider and lifecycle wiring** `backend/src/services/alert/providers/pam-folder-alert-*`, `backend/src/server/routes/index.ts`, `backend/src/ee/services/pam-folder/pam-folder-service.ts`, `backend/src/ee/services/pam/CLAUDE.md`|The provider resolves folder alert data, checks scope and permissions, builds alert payloads, and gates non-email channels by plan. Folder deletion removes associated alerts.| |**Resource-aware alert configuration** `frontend/src/hooks/api/alerts/types.ts`, `frontend/src/views/Alerts/*`, `frontend/src/pages/organization/IdentityDetailsByIDPage/components/IdentityAlertAction.tsx`, `frontend/src/pages/project/IdentityDetailsByIDPage/components/ProjectIdentityAlertAction.tsx`|The shared alert UI accepts resource-specific event types, email recipients for PAM folders, and channel paywall settings. Alert actions use a generic resource identifier.| |**PAM folder alert and legacy notification settings** `frontend/src/pages/pam/PamAccountsPage/components/FolderApprovalsTab.tsx`, `frontend/src/pages/pam/PamAccountsPage/components/FolderNotificationsSection.tsx`, `docs/documentation/platform/pam/access-requests/overview.mdx`|The PAM folder page adds alert controls and marks existing Slack notification configurations as read-only legacy settings. The documentation describes alert channels and events.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~45 minutes **Change:** Feature ### Sequence Diagram(s) ```mermaid sequenceDiagram participant ApprovalPolicyService participant PAMAccessApprovalResource participant EventOutboxService ApprovalPolicyService->>PAMAccessApprovalResource: Build event for approval lifecycle transition PAMAccessApprovalResource-->>ApprovalPolicyService: Return PAM folder event or null ApprovalPolicyService->>EventOutboxService: Emit event in the transaction ``` **Suggested reviewers:** `lb-vn`, `scott-ray-wilson` Merge Risk: 🔵 Low · up to 80283 Some users may be directed to connect a Slack integration even though legacy notification settings can no longer be added or edited. This is a bounded issue that does not block merging. Security architecture risk: 🔵 Low · up to 80283 The change adds notification destinations for privileged-access activity while retaining folder-level administration and organization isolation. No introduced security defect was confirmed, but recipient-delegation policy and end-to-end recovery guarantees remain incompletely established. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The demonstrated new disclosure scope is metadata for access requests in a configured PAM folder, delivered to its selected notification destinations. Cross-organization and cross-project target selection is constrained by the request query and folder checks. Email recipients may include verified-domain addresses without folder membership, so notification delegation is broader than direct folder access. **Trust Boundaries and Controls** - _observed_ — Alert authorization first awaits PAM project membership and validates folder ownership. Reading requires folder audit-log authority or policy-management authority; writes require policy-management authority. Missing project membership throws before the resource-permission fallback can run. Built-in Folder Admin grants both actions, Auditor grants audit-log access, and Operator and Connector grant neither. - _observed_ — Directed delivery rechecks active, accepted organization membership and effective project membership for user recipients, including expanded group recipients. Explicit email addresses are checked against currently verified organization domains. These checks do not establish folder membership or reauthorize the alert creator at each send; the inspected design delegates destination selection through folder policy-management authority. **Resilience and Maintainability Implications** - _observed_ — Approval decisions use locked request state and emit lifecycle events inside the transaction that changes approval or grant state. Event insertion failures propagate through that transaction. The delivery engine tracks successful channels by event identifier and skips them on retry; partial delivery requests another attempt. This supports recovery but does not establish exactly-once external notification delivery. **Hardening Proposals** - _proposed_ — Make the PAM notification-delegation policy explicit: clarify whether folder administrators may intentionally send request metadata to project members and verified-domain addresses without folder access, and what revocation should mean for existing destinations. This is policy clarification, not a verified authorization finding. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: moving PAM approval notifications to the shared alerting module. It follows the conventional commit format. | | Description check | | The description includes the context, screenshots, change summary, type selection, and checklist. The Steps to verify section is empty, but the description is otherwise complete and directly related t… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes the context, screenshots, change summary, type selection, and checklist. The Steps to verify section is empty, but the description is otherwise complete and directly related to the pull request. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 32 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8462 2026-10-20 13:53 | coderabbit review-body | — | summary | > [!CAUTION]
> Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
>
> **⚠️ Outside diff range comments (1)**
>
>
> 🟡 Minor · Remove the hint that tells users to connect a Slack… · FolderNotificationsSection.tsx:285-290
>
> `frontend/src/pages/pam/PamAccountsPage/components/FolderNotificationsSection.tsx:285-290`
> **🎯 Functional Correctness** | **🟡 Minor** | **⚡ Quick win**
>
> **Remove the hint that tells users to connect a Slack workflow integration.**
>
> This section is now read-only legacy UI. Users cannot add or edit Slack notification configs here, so connecting a new Slack workflow integration does not help. The hint still tells users to add o … the rest (810 characters)ne under Organization Settings, Workflow Integrations, which is a dead end. > > The hint can also appear above working legacy configs. `integrationOptions` is empty when the viewer cannot list workflow integrations, and the code at Line 127-128 handles that case with the saved `integrationSlug` fallback. Remove the hint, or replace it with a pointer to the Alerts card. > > > Proposed fix > > ```diff > - {integrationOptions.length === 0 && ( > - > - No Slack workflow integrations are connected to this organization yet. Add one under > - Organization Settings, Workflow Integrations. > - > - )} > ``` > > > Based on learnings: "The older per-folder Slack configs (`pam_folder_notification_configs`) are legacy: they still post and can be removed, never added or edited." > > > > > > _Source: Learnings_ > > |
| #8462 2026-10-20 13:57 | cubic review-body | — | summary | **No issues found** across 35 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8462 2026-10-20 13:57 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Medium impact]** The changes affect PAM approval alerts and the channels that receive them\.
No blocking issue was established; the remaining concern is a temporary, non-blocking interruption to paid-channel configuration during rollout.
Findings
1. **Paid channels appear locked**
Summary
Moves PAM approval alerts into the shared alert module while keeping existing Slack notifications active.
- PAM folder alerts send updates when access requests change.
- Folder approval settings now use the shared alert editor.
- PAM enterprise alert channels use a broader plan entitlement.
No blocking issue was established. Normalize or invalidate legacy plan caches to avoi … the rest (112 characters)d this temporary interruption. Reviews (1) · Last reviewed commit: "address reviews" · Reviewed by the reviewer |
| #8462 2026-10-20 13:57 | greptile inline · P2 | backend/src/services/alert/providers/pam-folder-alert-provider.ts:306 | defect-functional | **Paid channels appear locked**
If pre-deployment cloud plan caches survive deployment, paid customers temporarily receive an upgrade error when adding Slack, webhook, or PagerDuty PAM alert channels. Those cached plans contain `pamSlackNotifications`, but this check reads only `pamEnterpriseAlerting`, and `getPlan` returns cached JSON without migrating the field. Access returns after the plan is refreshed; the cache lifetime is nominally 15 minutes, with jitter. This is a non-blocking rollout compatibility concern. Normalize the legacy field on cache reads, or invalidate/version the affected caches when deploying the rename.
Artifacts
**Executed PAM cache compatibility harness**
- Loads … the rest (1310 characters)repository TypeScript and exercises historical and PR-head entitlement behavior with stubbed infrastructure, making the reproduction inspectable and repeatable. **Executed command capture script** - Runs the before or after harness and records its command, working directory, exit code, stdout, and stderr, preserving observed execution evidence. **Legacy paid plan generated by historical projection** - The before run generated this fixture using the actual pre-rename projection, demonstrating the legacy cache contains the paid Slack grant without the renamed field. **Before rename: paid Slack entitlement accepted** - Executed historical cache reading, projection, reconciliation, and the original Slack entitlement guard through an HTTP adapter, showing the legacy paid cache is accepted. **PR head: legacy paid cache rejects non-email channels** - Executed the actual PR-head provider and error handler against legacy and refreshed plans, showing three erroneous 400 responses become 200 responses after fresh projection. **Source lines and unchanged application worktree** - Captured numbered source, historical cache-key comparison, cache-reference search, and Git status, identifying the compatibility gap and confirming no tracked application changes. Ran code and verified through TREX |
| #8636 2026-10-20 14:01 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The pull request adds typed analytics events and telemetry tracking across the Secrets overview page, its import and management workflows, and member activation surfaces. Tracking includes interactions, flow outcomes, resource counts, and invocation ownership.
**Changes: Secrets Analytics**
|Layer / File(s)|Summary|
|:---|:---|
|**Analytics contracts and activation tracking** `frontend/src/lib/analytics/events.ts`, `frontend/src/pages/secret-manager/OverviewPage/components/InviteMembersModal/InviteMembersModal.tsx`, `frontend/src/pages/secret-manager/OverviewPage/components/InviteMembersNudge/InviteMembersNudge.tsx`|Adds typed overview and activation events. The invite moda … the rest (7062 characters)l and nudge capture activation events through organization-scoped analytics.| |**Telemetry lifecycle and overview context** `frontend/src/pages/secret-manager/OverviewPage/OverviewTelemetry.tsx`, `frontend/src/pages/secret-manager/OverviewPage/OverviewPage.tsx`|Adds count and outcome classification, flow lifecycle tracking, request observation, and React context. The overview records scope, viewed and loaded events, and popup flow ownership.| |**Overview operations and interactions** `frontend/src/pages/secret-manager/OverviewPage/OverviewPage.tsx`|Adds telemetry for folder and secret mutations, batch commits, filters, selection, search, navigation, sorting, pagination, and environment interactions.| |**Component actions and flow outcomes** `frontend/src/pages/secret-manager/OverviewPage/components/*`|Adds telemetry to create, copy, export, breadcrumb, row-edit, reveal, and bulk-selection actions. Tracked async flows use invocation ownership checks for completion and sheet closure.| |**Import flow tracking and ownership** `frontend/src/pages/secret-manager/OverviewPage/components/SecretDropzone/ImportSecretsModal.tsx`|Tracks import source, outcomes, counts, and cancellation. Centralized close handling checks invocation ownership before changing lock state or completing callbacks.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~45 minutes **Change:** Feature **Suggested reviewers:** `andrewhuhh` Merge Risk: 🟡 Moderate · up to 6078c Successful secret operations can leave forms or dialogs open under the identified scope and render transitions, and protected-branch commit analytics can report the wrong outcome. Resolve these issues before merging. Security architecture risk: 🟡 Moderate · up to 6078c The explicit event properties are carefully bounded, and the inspected changes preserve secret-operation permissions and approval handling. However, the analytics transport may attach browser URLs containing secret-folder paths despite those property restrictions. New visit and interaction events broaden that potential exposure. **Retained concerns** - **Medium · security · inferred:** The new events constrain custom properties but not SDK-generated browser metadata. Overview retains secretPath in its URL, and the existing transport forwards events to PostHog without a location-property scrubber. SDK URL enrichment can therefore carry secret-folder paths outside the intended allowlist. The transport condition predates this PR, but new visit and interaction captures broaden its exposure. Actual outgoing payloads were not available, so this is an inferred privacy concern rather than a verified disclosure. Security review details **Security Blast Radius** - _inferred_ — The privacy concern affects telemetry-enabled production browsers visiting V3 Overview pages or using the new interactions. Events are associated with organization and project identity. The supported exposure concerns browser-location metadata; inspected custom payloads do not carry secret values, clipboard text or file contents. **Security Findings and Attack Paths** - _inferred_ — A user-controlled folder path persists in the Overview query string. An ordinary visit or subsequent instrumented interaction reaches the SDK through the organization capture adapter. Default URL enrichment can then transmit that path despite the custom-property allowlist. Existing explicit captures shared this condition, but the new capture points increase its reach. Final SDK-enriched payloads remain unverified. **Trust Boundaries and Controls** - _observed_ — The existing transport requires production mode, enabled telemetry and an API key. Automatic page, click, exception and session-recording collection is explicitly disabled. These gates reduce collection scope but do not sanitize the new explicit captures. - _observed_ — The inspected Overview mutations preserve concrete project, environment and path arguments, folder permission filters, approval-response handling and context-keyed pending changes. The telemetry state machine does not grant mutation authority or replace those controls. **Resilience and Maintainability Implications** - _observed_ — The transport catches SDK capture exceptions, and the request observer returns original mutation results or rethrows original request errors. This keeps ordinary capture failures from replacing backend outcomes in the inspected execution path. **Hardening Proposals** - _proposed_ — Extend the privacy boundary to the final SDK-enriched payload: sanitize location and referrer properties or replace them with a fixed route, then validate serialized events for nested-folder visits and deep links. This would close the current proof gap without relying on custom-property selection alone. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 14 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the primary change: adding semantic telemetry for the Secrets Overview. It also follows the conventional commit format. | | Description check | | The description is complete and relevant. It includes context, screenshots, verification steps, change type, checklist status, testing details, scope limitations, and implementation constraints. | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 14 - Waived: $3.50 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8636 2026-10-20 14:10 | cubic inline · P2 | frontend/src/pages/secret-manager/OverviewPage/components/SecretTableRow/SecretEditTableRow.tsx:1052 | defect-functional | A failed `refetchSharedValue()` returns before this flow starts, so failures while loading a secret for editing are absent from telemetry. Start the attempt before fetching and emit `failed` on this error path. |
| #8636 2026-10-20 14:10 | cubic inline · P2 | frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/SelectionPanel.tsx:196 | defect-functional | Bulk-move telemetry reports a zero or undercounted selection when rotations are selected because this bucket omits `selectedRotationCount`. Include the rotation count so the interaction reflects all selected items offered to the move flow.
```suggestion
countBucket: overviewCountBucket(
selectedFolderCount + selectedKeysCount + selectedRotationCount
)
``` |
| #8636 2026-10-20 14:10 | cubic inline · P2 | frontend/src/pages/secret-manager/OverviewPage/components/CopySecretsSheet/CopySecretsSheet.tsx:471 | defect-functional | Copy telemetry merges folder-creation responses into `responseCountBucket` and omits `folderResponseCountBucket`, so folder-only and secret-copy outcomes cannot be distinguished. Record the folder count separately in both success and partial-failure events. |
| #8636 2026-10-20 14:10 | cubic review-body | — | summary | **3 issues found** across 14 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8636 2026-10-20 14:12 | coderabbit inline · P2 | frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx:184 | defect-functional | **Do not reassign `formOwner.current` during render.**
Line 178 replaces `formOwner.current` during render when the telemetry instance changes. React can replay or discard a render. A discarded render can then reset `invocation` and make an in-flight submission look stale, so the form does not close. Do this reset in an effect keyed on `overviewTelemetry`.
_Source: Linters/SAST tools_ |
| #8636 2026-10-20 14:12 | coderabbit inline · P1 | frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx:487 | defect-functional | **Do not require `attempt.isCurrent()` to close the form.**
When the overview scope changes, `cancelOpenFlows()` deletes submitted flow states. A successful create that completes afterward fails `attempt.isCurrent()`, even when the invocation is still active. The form remains open with the submitted data.
The default context is not a cause because its `isCurrent()` returns `true`.
🐛 Suggested fix
```diff
--- "a/frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx"
+++ "b/frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx"
@@ -480,11 +480,10 @@
console.warn("failed to create secrets");
}
… the rest (580 characters)if ( allRequestsSucceeded && - attempt.isCurrent() && formOwner.current.invocation === invocation ) { onClose(); reset(); } ``` 📝 Committable suggestion > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion if ( allRequestsSucceeded && formOwner.current.invocation === invocation ) { ``` |
| #8636 2026-10-20 14:12 | coderabbit inline · P2 | frontend/src/pages/secret-manager/OverviewPage/components/SecretTableRow/SecretEditTableRow.tsx:236 | defect-functional | **Do not write `editConfirmation.current` during render.**
Line 236 writes the ref during render. `handleEditSecret` compares this ref after an `await`. A discarded render can therefore cause a wrong stale-confirmation decision, and the confirmation dialog stays open after a successful save. Sync the ref in an effect, or compare against popup state captured through a token set in `handlePopUpOpen`.
_Source: Linters/SAST tools_ |
| #8636 2026-10-20 14:12 | coderabbit inline · P2 | frontend/src/pages/secret-manager/OverviewPage/OverviewPage.tsx:1361 | defect-functional | **Preserve popup ownership across discarded renders.**
When `telemetryFlows` changes in a render that React discards, the render-time mutation of `popupOwners.current` can persist. A later render with the previous scope resets the owner map, while `popupOwnership` still contains the previous owner token. `isPopupCurrent` then rejects the close request.
`usePopUp` stores open state separately and does not restore the owner. The `ImportSecretsSheet` close callback returns before calling `handlePopUpToggle`, so the popup can remain open.
Moving the reset to `useEffect` is not sufficient. That effect can clear ownership after a committed scope change while the popup remains open. Store the ow … the rest (154 characters)ner token and scope in committed popup state, and validate that record for each popup invocation. Do not use a `useMemo` snapshot as the ownership source. |
| #8636 2026-10-20 14:12 | coderabbit inline · P2 | frontend/src/pages/secret-manager/OverviewPage/OverviewPage.tsx:2536 | defect-functional | **Use the protected-branch predicate for the commit flow phase.**
`POST /api/v1/pit/batch/commit` returns only `{ message: "success" }`. It does not return the service result's `approvalId`. Therefore, protected-branch changes can be recorded as `response-received`.
Proposed fix
```diff
- await observeOverviewRequest(overviewTelemetry, flow, () =>
- createCommit({
- projectId,
- environment: singleVisibleEnv.slug,
- secretPath,
- pendingChanges: changes,
- message
- })
- );
-
const hasOnlyFolderChanges = changes.folders.length > 0 && changes.secrets.length === 0;
const requiresApproval = isProtectedBranch & … the rest (484 characters)& !hasOnlyFolderChanges; + const attempt = overviewTelemetry.startFlow(flow); + try { + await createCommit({ + projectId, + environment: singleVisibleEnv.slug, + secretPath, + pendingChanges: changes, + message + }); + } catch (error) { + attempt.flow({ phase: "failed" }); + throw error; + } + attempt.flow({ phase: requiresApproval ? "approval-requested" : "response-received" }); ``` |
| #8636 2026-10-20 14:12 | coderabbit review-body | — | summary | **Actionable comments posted: 5** |
| #8636 2026-10-20 14:19 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Medium impact]** The telemetry changes affect the Secrets Manager Overview and its activation prompts\.
No blocking issue was found; the abandonment telemetry gap and missing regression coverage are non-blocking follow-ups.
Findings
1. **Page exits lose cancellation events**
2. **Editor safeguards lack regression tests**
Summary
Adds explicit organization-scoped telemetry for Secrets Overview.
- Overview visits and usable loads now produce scoped telemetry events.
- Overview controls now report navigation, filters, and batch choices.
- Create and row-edit flows report progress without closing newer editors.
- Imports report their source and observed results wit … the rest (665 characters)hout taking over a replacement sheet. - Copy, export, and bulk actions report their observed outcomes. - Activation modal and card events now use organization-scoped capture. Two non-blocking follow-ups remain: leaving Overview with an unsubmitted create or import flow does not record cancellation, and the new editor ownership safeguards lack committed regression tests. Neither finding prevents merging. PierreOucif explicitly described retaining action-origin scope for anchored copy as intentional because the source and destination stay unchanged. Reviews (1) · Last reviewed commit: "feat: add semantic Secrets Overview tele..." · Reviewed by the reviewer |
| #8636 2026-10-20 14:19 | greptile inline · P2 | frontend/src/pages/secret-manager/OverviewPage/OverviewPage.tsx:1194 | defect-functional | **Page exits lose cancellation events**
Leaving Overview with an unsubmitted create or import sheet open does not record `canceled`. This effect calls `cancelOpenFlows()` when another scope renders, but has no unmount cleanup; neither sheet records cancellation when its parent unmounts. This is a non-blocking telemetry gap: abandonment counts depend on how users leave the page. Add lifecycle-safe unmount cleanup for open flows while preserving submitted requests and their eventual results.
Artifacts
**Evidence from the check**
- The executed script extracts the relevant source and drives Chromium assertions, making the unmount evidence reproducible.
**Command output from the check**
- … the rest (993 characters)The successful command captured exact extracted source, event payloads, lifecycle probes, and 18 passing checks, confirming missing unmount cancellation. **Recording of the check** - Chromium opens create and import flows and changes scope, showing that the existing effect emits canceled. **Scope-change control poster** - A browser screenshot captures the import scope-change control and observed cancellation, providing the recording's preview. **Recording of the check** - Chromium opens the same create and import flows and unmounts Overview, showing opened events without canceled despite confirmed cleanup. **Page-unmount reproduction poster** - A browser screenshot captures the replacement page and unmount lifecycle output with only opened telemetry, providing the recording's preview. **Command output from the check** - Executed unstaged and staged git diff checks returned exit code zero, confirming tracked source remained unchanged. Ran code and verified through TREX |
| #8636 2026-10-20 14:19 | greptile inline · P2 | frontend/src/pages/secret-manager/OverviewPage/OverviewTelemetry.tsx:120 | tests | **Editor safeguards lack regression tests**
The new flow state and request-ownership guards decide whether successful requests may close or reset editors, but no committed tests cover those safeguards. The existing Overview suite passes the same 59 tests in six unchanged files before and after this change. This is a non-blocking maintenance concern: later changes can silently break event counts or dismiss a replacement editor. Commit repeatable tests for retries, scope changes, replacement editors, and out-of-order responses; the temporary checks described in the PR cannot protect later changes.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. … the rest (1591 characters)I'll remember it for next time! Artifacts **Evidence from the check** - Executed discovery and base/head comparison source. **Command output from the check** - Committed-test inventory, full Overview test inspection, unchanged-test comparisons, and source excerpts. **Command output from the check** - Actual frontend Vitest discovery output. **Command output from the check** - Base: six test files and 59 tests passed. **Command output from the check** - Head: six test files and 59 tests passed. **Command output from the check** - Captured execution of the discovery/comparison script. **Evidence from the check** - Executed temporary direct-helper test source; not committed regression coverage. **Evidence from the check** - Executed temporary probe configuration. **Evidence from the check** - Executed wrapper recording the probe command and output. **Command output from the check** - Initial probe harness import failure before tests executed. **Command output from the check** - Second probe harness mock-resolution failure before tests executed. **Command output from the check** - Corrected probe: three ownership tests passed. **Command output from the check** - Committed-test inventory, full Overview test inspection, unchanged-test comparisons, and source excerpts. **Command output from the check** - Base: six test files and 59 tests passed. **Command output from the check** - Head: six test files and 59 tests passed. **Command output from the check** - Corrected probe: three ownership tests passed. Ran code and verified through TREX |
| #8634 2026-10-20 14:20 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The cancellation feedback form now accepts an optional comment of up to 500 characters. It trims and validates the comment, displays privacy guidance, and includes non-empty comments with the cancellation reason in analytics.
**Changes: Billing cancellation comments**
|Layer / File(s)|Summary|
|:---|:---|
|**Analytics event contract** `frontend/src/lib/analytics/events.ts`, `ANALYTICS.md`|The billing cancellation event properties now allow an optional comment. The analytics guidance documents the 500-character limit, trimming, omission when empty, and restrictions on use and content.|
|**Comment collection and submission** `frontend/src/pages/organization/BillingV2Page/comp … the rest (5158 characters)onents/CancellationFeedbackDialog.tsx`|The dialog initializes and validates the optional comment, displays a character count, privacy guidance, and validation errors, and disables the textarea during submission. Analytics includes the comment only when it is non-empty.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Feature **Suggested reviewers:** `andrewhuhh` Merge Risk: 🔵 Low · up to f482a Screen-reader users do not receive the comment count as they type. The cancellation flow remains usable, but the count should be made accessible. Security architecture risk: 🟡 Moderate · up to f482a The optional comment creates a new path for potentially sensitive customer text to enter analytics. Length limits, privacy guidance, and submission only after successful cancellation constrain exposure, but they do not prevent sensitive content from being transmitted. The change does not introduce a new privileged operation or automatically collect secrets. **Retained concerns** - **Medium · security · inferred:** The new comment property permits customer-entered sensitive text to cross into organization-linked analytics. Trimming, a 500-character cap, and a warning constrain volume and guide users, but do not enforce the documented content restrictions. Sensitive disclosure is possible if a user includes such content; no actual disclosure is established. Security review details **Security Blast Radius** - _inferred_ — The directly exposed data is the cancelling user’s optional comment, associated with an organization, product, action, and reason in the configured analytics destination. The new code does not automatically read secret stores or confer additional billing authority. **Security Findings and Attack Paths** - _inferred_ — If a user pastes a credential or personal information into the comment and cancellation succeeds, the inspected path forwards that content to PostHog. This is a newly enabled disclosure path compared with the reason-only base, not evidence that sensitive content has actually been collected or exposed to unauthorized readers. **Trust Boundaries and Controls** - _observed_ — The UI warns against secrets, credentials, and personal data, applies a native length limit, and validates trimmed length. The documented exception is limited to this event property and prohibits sensitive values from code. Neither the inspected form nor its transport enforces content-level exclusion. **Resilience and Maintainability Implications** - _observed_ — The existing transition ordering is preserved: cancellation rejection leaves the dialog open without capture; success precedes capture and completion. Inputs and dismissal are disabled during submission, and the Telemetry capturer catches synchronous PostHog capture errors. - _inferred_ — Comment state is initialized on mount rather than reset when organization, product, or action props change. Ordinary dismissal and success close the flow, but whether a live organization change can preserve the mounted dialog remains unresolved; no cross-organization transfer is established. **Hardening Proposals** - _proposed_ — Treat this property as potentially sensitive feedback: establish restricted readership, retention, and deletion handling at the destination, and consider a dedicated feedback channel if those guarantees cannot be provided by analytics. Content screening could reduce accidental credential submission, but should not be represented as a complete safeguard. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: adding an optional cancellation comment to the Billing V2 cancellation event. | | Description check | | The description includes the required context, screenshots, verification steps, feature type, and checklist. It explains the UI, analytics behavior, privacy guidance, and testing scope. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8634 2026-10-20 14:21 | cubic review-body | — | summary | **No issues found** across 3 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8634 2026-10-20 14:24 | coderabbit inline · P2 | frontend/src/pages/organization/BillingV2Page/components/CancellationFeedbackDialog.tsx:193 | defect-functional | **Make the changing character count accessible.**
The count changes as the user types, but the textarea describes only the privacy hint. Screen-reader users do not receive the remaining-limit update from this markup. Give the count an ID, include it in `aria-describedby`, and use `aria-live="polite"` for its updates. (w3.org)
Based on learnings: dynamic non-urgent status updates should use `aria-live="polite"` so screen readers can announce them.
_Source: Learnings_ |
| #8634 2026-10-20 14:24 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8634 2026-10-20 14:29 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** Safe to merge within the reviewed scope; no actionable issues were found in the changed dialog or analytics property.
Summary
Adds optional cancellation comments to all three Billing V2 cancellation flows, with a 500-character limit, a live counter, and a sensitive-data warning. Trimmed, nonempty comments accompany the existing feedback event only after cancellation succeeds.
Focused browser checks of the shared dialog passed: typing updated the counter, input stopped at 500 characters, blank and whitespace-only comments remained optional and omitted the comment property, and padded comments were trimmed. Pending cancellation disabled the textare … the rest (762 characters)a and withheld feedback; failed cancellation preserved the input and sent no feedback. These checks used controlled cancellation callbacks and captured analytics calls locally, rather than a live billing account or PostHog destination. PierreOucif explicitly describes sending free-text feedback to PostHog for churn analysis as intentional. The PR adds the matching, narrowly scoped exception in ANALYTICS.md. PierreOucif also identifies a possible Slack destination as deferred and outside this PR. No actionable issues found. Safe to merge within the reviewed scope. - Billing V2 cancellation dialogs can send an optional comment with the reason. Reviews (1) · Last reviewed commit: "feat(billing): add optional cancellation..." · Reviewed by the reviewer |
| #8650 2026-10-20 14:32 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The Secret Scanning documentation is reorganized into guides for local and repository scans, provider setup, configuration, and findings. Navigation, redirects, and frontend links now point to the corresponding guides.
**Changes: Secret Scanning documentation**
|Layer / File(s)|Summary|
|:---|:---|
|**Navigation and scanning concepts** `docs/docs.json`, `docs/documentation/platform/secret-scanning/concepts/secret-scanning.mdx`, `docs/documentation/platform/secret-scanning/overview.mdx`, `docs/documentation/getting-started/choose-a-product/secret-scanning.mdx`, `docs/documentation/platform/secret-scanning/usage.mdx`|Navigation separates local and repository scanning and adds … the rest (4179 characters)links to dedicated guides. The overview and concepts pages describe scanning approaches. The former usage page is removed, and its URL redirects to the repository scanning guide.| |**Local scanning workflows and rules** `docs/documentation/platform/secret-scanning/scan-locally.mdx`, `docs/documentation/platform/secret-scanning/pre-commit-hook.mdx`, `docs/documentation/platform/secret-scanning/ignore-findings.mdx`, `docs/documentation/platform/secret-scanning/local-configuration.mdx`, `docs/snippets/documentation/platform/secret-scanning/*`|The local guides cover CLI scans, pre-commit hooks, ignored findings, and local configuration. Snippets document rule fields, allowlists, and inline-ignore syntax.| |**Repository scans and provider setup** `docs/documentation/platform/secret-scanning/scan-repositories.mdx`, `docs/documentation/platform/secret-scanning/github.mdx`, `docs/documentation/platform/secret-scanning/gitlab.mdx`, `docs/documentation/platform/secret-scanning/bitbucket.mdx`|The repository guide describes data sources, resources, historical and realtime scans, and Auto-Scan. Provider guides describe connection requirements, setup, repository selection, and scan history.| |**Repository configuration and findings** `docs/documentation/platform/secret-scanning/configuration.mdx`, `docs/documentation/platform/secret-scanning/findings.mdx`, `frontend/src/pages/secret-scanning/SecretScanningFindingsPage/components/SecretScanningFindingsSection.tsx`, `frontend/src/pages/secret-scanning/SettingsPage/components/ProjectScanningConfigTab/SecretScanningConfigForm.tsx`|The configuration guide documents repository scan rules and allowlists. The findings guide covers finding details, statuses, updates, deduplication, and notifications. Frontend documentation links now point to the findings and configuration guides.| **Priority:** ⬇️ Low **Estimated code review effort:** 3 (Moderate) | ~25 minutes **Change:** Other **Suggested reviewers:** `adkah`, `mathnogueira` Merge Risk: 🔵 Low · up to 7c333 Readers could misunderstand licensing, remediation, or how to configure local scans. These are localized documentation corrections; the reorganization is otherwise mergeable with those corrections tracked. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly describes the Secret Scanning documentation overhaul and follows the conventional commit format. | | Description check | | The description explains the documentation overhaul, identifies the change type, and includes the relevant checklist confirmations. The required steps-to-verify section is omitted, but the description… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description explains the documentation overhaul, identifies the change type, and includes the relevant checklist confirmations. The required steps-to-verify section is omitted, but the description is otherwise sufficiently complete for this documentation change. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. (18 skipped: 18 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8650 2026-10-20 14:38 | coderabbit inline · P2 | docs/documentation/platform/secret-scanning/concepts/secret-scanning.mdx:11 | docs | **Do not promise automatic detection and revocation.**
A repository scans each new push only when Auto-Scan is enabled. A scan records a finding; the user must revoke an exposed credential. The current sentence can lead readers to assume that detection and remediation happen without those steps. State both conditions explicitly. (infisical.com) |
| #8650 2026-10-20 14:38 | coderabbit inline · P2 | docs/documentation/platform/secret-scanning/concepts/secret-scanning.mdx:17 | docs | **Link to both configuration workflows.**
This paragraph covers local and repository scanning, but its link opens only the repository configuration guide. A local CLI user who follows it will reach dashboard instructions instead of the `.infisical-scan.toml` instructions. Add a separate link to the local configuration guide. (infisical.com) |
| #8650 2026-10-20 14:38 | coderabbit inline · P2 | docs/documentation/platform/secret-scanning/overview.mdx:3 | docs | **Include uncommitted code in the overview.**
The description limits secret scanning to committed code. The CLI also scans changes before a commit, which is a principal workflow on this page. Describe scanning as checking code for exposed secrets before or after commit. The concepts page has the same narrow definition at Line 9. (infisical.com) |
| #8650 2026-10-20 14:38 | coderabbit inline · P2 | docs/documentation/platform/secret-scanning/scan-repositories.mdx:12 | docs | **State that repository scanning requires the paid add-on.**
The pricing page says secret scanning is a paid add-on to Enterprise, not part of the base plan. Line 12 can lead a reader to purchase Enterprise and expect repository scanning to be available. State that the Enterprise plan also requires the secret-scanning add-on. (infisical.com) |
| #8650 2026-10-20 14:38 | coderabbit review-body | — | summary | **Actionable comments posted: 4** |
| #8650 2026-10-20 14:42 | cubic inline · P2 | docs/documentation/platform/secret-scanning/pre-commit-hook.mdx:19 | docs | Both examples print detected secret values, which can persist in terminal scrollback or commit/CI logs. Add `--redact` to both commands. |
| #8650 2026-10-20 14:42 | cubic inline · P2 | docs/documentation/platform/secret-scanning/overview.mdx:8 | docs | This limits Secret Scanning to already-committed credentials, but the CLI also scans directories and uncommitted changes. Revise both this sentence and the page description to include pre-commit scans. |
| #8650 2026-10-20 14:42 | cubic inline · P2 | docs/documentation/platform/secret-scanning/concepts/secret-scanning.mdx:11 | docs | This overstates what scanning does: scans run only when configured or invoked, and Infisical records findings rather than revoking credentials. Describe detection as conditional and revocation as a response teams must take.
```suggestion
As teams grow, it gets easy for a secret to end up in version control, a CI/CD pipeline, or a shared file. Anyone who can read that code can then use the secret to reach production systems, third-party services, or internal APIs. A secret scanner can detect leaks when you run it or configure automatic scans, giving your team a chance to revoke them before someone else finds them.
``` |
| #8650 2026-10-20 14:42 | cubic review-body | — | summary | **6 issues found** across 24 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8650 2026-10-20 14:42 | cubic inline · P2 | docs/documentation/platform/secret-scanning/configuration.mdx:15 | docs | This claim is false for `.infisical-scan.toml`: without a saved configuration, the repository scan omits `-c`, and the CLI automatically loads that file from the checkout. Correct the list or disable config auto-discovery before saying repository scans ignore it. |
| #8650 2026-10-20 14:42 | cubic inline · P2 | docs/documentation/platform/secret-scanning/scan-repositories.mdx:12 | docs | State that repository scanning requires the Secret Scanning add-on in addition to Enterprise; the current wording implies the base Enterprise plan includes it.
```suggestion
Scanning repositories requires Infisical's Enterprise plan and the Secret Scanning add-on. If you're self-hosting Infisical, contact sales@infisical.com to purchase an enterprise license.
``` |
| #8650 2026-10-20 14:42 | cubic inline · P3 | docs/documentation/platform/secret-scanning/concepts/secret-scanning.mdx:17 | docs | Link to both configuration guides here; local CLI users currently land on repository settings instead of the `.infisical-scan.toml` instructions.
```suggestion
Infisical's scanner includes rules for hundreds of services. You can add your own rules and skip values you've decided are safe in the [repository scanning configuration](/documentation/platform/secret-scanning/configuration) or the [local scanning configuration](/documentation/platform/secret-scanning/local-configuration).
``` |
| #8650 2026-10-20 14:45 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Low impact]** The changes stay within Secret Scanning documentation and its help links\.
No blocking issue was established; the documentation can merge with a non-blocking compatibility concern affecting previously shared section links.
Findings
1. **Preserve existing section links**
Summary
Reorganizes Secret Scanning documentation into local scanning and connected repository workflows.
- The Secret Scanning docs guide readers to separate local and repository scanning paths.
- Local scanning guides explain how to scan code and handle findings before a push.
- Repository scanning guides explain how data sources scan connected code.
- A dedicated findings guide … the rest (1149 characters)explains what secret findings contain and how teams review them. - Repository configuration guides show how to set detection rules and skip safe values. The new navigation destinations, updated product documentation links, and shared MDX imports passed focused checks. One non-blocking compatibility issue remains in `docs/docs.json`: existing findings and configuration section links on the old usage page no longer reach their corresponding guidance. No blocking issue was established. The email-service prerequisite remains consistent with the implementation: the server checks SMTP configuration before creating in-app notifications or sending email. ### the check validation blocked Native Mintlify visual verification was blocked because the pinned mint@4.2.13 CLI rejects pre-existing logo.href and contextual assistant/mcp configuration fields on both the base and head revisions. Representative pages and bookmark behavior were exercised in a fallback MDX renderer instead; deployed Mintlify redirect behavior was not observed. Reviews (1) · Last reviewed commit: "docs(secret-scanning): overhaul section ..." · Reviewed by the reviewer |
| #8650 2026-10-20 14:45 | greptile inline · P2 | docs/docs.json:4876 | defect-functional | **Preserve existing section links**
Existing `/documentation/platform/secret-scanning/usage#findings` and `/documentation/platform/secret-scanning/usage#configuration` links now redirect to `scan-repositories`, which contains neither section anchor. Readers following previously shared links or bookmarks therefore lose their section destination instead of reaching the new findings or configuration guide. This is non-blocking, but retaining compatibility sections with links to the moved guides would avoid that extra search.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
**Evidence from the check**
- … the rest (2048 characters)The authored harness reads exact Git objects and executes MDX with explicit snippet resolution and substitute components; its scope and rendering limitations are documented in the source. **Evidence from the check** - The authored Playwright script starts the fallback servers and asserts representative page interactions and bookmark behavior; the browser evidence is reproducible. **Evidence from the check** - The authored runner executes integrity checks, native preview attempts, and Chromium capture while saving command metadata and actual output; logs are generated from executed commands. **Command output from the check** - The focused checker validates original navigation, links, assets, frontend hrefs, and MDX execution; all base checks pass. **Command output from the check** - The focused checker validates new routes, both frontend hrefs, nine imports, and all scoped MDX execution; direct destinations pass while legacy fragment loss is recorded. **Command output from the check** - The browser capture script records successful page loads, nested snippet visibility, imported-anchor navigation, and legacy bookmark outcomes; both runs finish without page errors. **Recording of the check** - Chromium visits the original configuration, findings, local guide, and legacy bookmarks using base-commit MDX; both old usage anchors exist. **Recording of the check** - Chromium exercises the new configuration and findings pages, expanded nested snippets, and local configuration; new content renders but legacy usage anchors are absent. **Original repository configuration in the fallback MDX renderer** - A Chromium screenshot captures the original configuration section with its example expanded; this is fallback rendering rather than the Mintlify UI. **Migrated configuration with nested snippet properties expanded** - A Chromium screenshot captures the new configuration page after expanding shared rule properties; nested allowlist content is present in fallback rendering. Ran code and verified through TREX |
| #8582 2026-10-20 14:47 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Webhook alert channels now support bearer, basic, and custom-header authentication, plus optional custom headers. Saved credentials are redacted and can be reused only while the webhook URL remains unchanged. Frontend forms, backend delivery, tests, and documentation cover these settings.
**Changes: Webhook Channel Configuration and Delivery**
|Layer / File(s)|Summary|
|:---|:---|
|**Validate and persist webhook configuration** `backend/src/services/alert/alert-channel-types.ts`, `backend/src/services/alert/alert-channel-config-fns.ts`, `backend/src/services/alert/channels/alert-channel-registry.ts`, `backend/s … the rest (6644 characters)rc/services/alert/alert-channel-service.ts`, `backend/src/services/alert/alert-channel-service.test.ts`, `backend/src/services/alert/channels/alert-channels.test.ts`, `backend/e2e-test/routes/v1/alerts.spec.ts`, `backend/e2e-test/testUtils/alerts.ts`|Webhook validation now covers authentication modes, custom-header constraints, and saved-credential reuse. Channel creation and updates store parsed configuration. Tests cover validation, credential redaction and updates, header trimming, and URL-change rejection.| |**Build authenticated webhook requests** `backend/src/services/alert/channels/alert-channel-webhook-fns.ts`, `backend/src/services/alert/alert-channel-test-service.ts`, `backend/src/services/alert/alert-channel-test-service.test.ts`, `backend/src/services/alert/channels/alert-channels.test.ts`|Webhook requests combine configured headers, authentication headers, and JSON content type. Channel testing parses incoming and stored configurations before delivery.| |**Model webhook form data** `frontend/src/hooks/api/alerts/types.ts`, `frontend/src/hooks/api/alerts/utils.ts`, `frontend/src/pages/cert-manager/components/CertificateAlerts/types.ts`, `frontend/src/pages/secret-manager/OverviewPage/components/SecretTableRow/SecretReminderForm.tsx`, `frontend/src/views/Alerts/ChannelsField.tsx`|Form schemas and conversion helpers handle authentication credentials, saved-secret state, signing-secret state, and custom headers. Alert form integrations use shared channel conversion and initialize header collections.| |**Render webhook controls and guidance** `frontend/src/views/Alerts/ChannelCard.tsx`, `frontend/src/views/Alerts/ChannelSecretInput.tsx`, `frontend/src/views/Alerts/WebhookAuthenticationField.tsx`, `frontend/src/views/Alerts/WebhookHeadersField.tsx`, `frontend/src/views/Alerts/WebhookSigningSecretField.tsx`, `docs/documentation/platform/identities/machine-identities.mdx`, `docs/documentation/platform/pki/applications/alerting/webhook-alerts.mdx`|The alert form renders authentication, signing-secret, and custom-header controls. Documentation describes authentication options, credential reuse, and header constraints.| **Priority:** ⬇️ Low **Estimated code review effort:** 4 (Complex) | ~45 minutes **Change:** Feature ### Sequence Diagram(s) ```mermaid sequenceDiagram participant sendWebhookNotification participant triggerWebhook participant safeRequestPost sendWebhookNotification->>triggerWebhook: Pass parsed config and payload triggerWebhook->>safeRequestPost: POST with configured and authentication headers ``` **Suggested reviewers:** `thiago-as`, `carlosmonastyrski`, `lobocv` Merge Risk: ⚪ Minimal · up to 8ea36 No actionable merge-blocking issue remains; webhook credentials are not forwarded through redirects. Security architecture risk: 🔵 Low · up to 8ea36 Encrypted storage, destination binding, permission checks, and outbound request controls provide meaningful protection. No new vulnerability was established, but credential protection in failed-request logs and deployment-specific network exceptions were not fully confirmed. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — Outbound requests gain the authority conferred by the configured endpoint credentials; their downstream privileges cannot be determined from this repository. Saved-channel test reuse remains subject to organization, project, channel-type, and owning-alert edit checks. **Security Findings and Attack Paths** - _observed_ — The supplied redirect-exfiltration candidate is countered by the actual POST dispatch, which forces maxRedirects to zero. A redirect response does not automatically forward the authentication headers to another destination. No verified Security finding remains in the supplied assessment. **Trust Boundaries and Controls** - _observed_ — The shared outbound boundary rejects private addresses by default and pins validated DNS results. Existing development and internal-network exceptions remain configuration-dependent. Header validation protects transport and signing headers, while delivery constructs generated authentication and content-type headers after custom headers. **Resilience and Maintainability Implications** - _observed_ — Webhook requests retain bounded timeout and cancellation signals. Production dispatch catches delivery failures and records failure status, but also passes the full exception to logging. Whether the existing logger redacts every new credential-bearing header remains unresolved; this is a coverage limitation, not a verified disclosure. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description check | | The description covers the problem, implemented behavior, UI changes, verification steps, feature type, and documentation impact. The checklist items remain unchecked, and the verification steps conta… | | Title check | | The title clearly and concisely describes the main change: adding authentication and custom headers to webhook channels. It follows the conventional commit format. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description covers the problem, implemented behavior, UI changes, verification steps, feature type, and documentation impact. The checklist items remain unchecked, and the verification steps contain a duplicate number, but the required content is otherwise complete. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 21 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8582 2026-10-20 14:52 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Medium impact]** Webhook alert delivery changes reach several alert setup flows and their configured endpoints\.
Do not merge until the authentication tabs satisfy the repository’s shared-component styling requirement.
Findings
1. **Authentication tabs override shared spacing**
Summary
Adds authentication and custom headers to webhook alert channels.
- Webhook alerts can use authentication and custom headers.
Reviews (1) · Last reviewed commit: "improve naming" · Reviewed by the reviewer |
| #8582 2026-10-20 14:52 | greptile inline · P1 | frontend/src/views/Alerts/WebhookAuthenticationField.tsx:66 | quality | **Authentication tabs override shared spacing**
`WebhookAuthenticationField` replaces the shared `TabsContent` spacing with `mt-0` here and on line 77. This removes the component’s default `mt-3` spacing from the authentication panels and violates the repository directive that new UI must not override v3 design-system component defaults. Remove both margin overrides, or introduce a shared component variant if compact tabs are required, before merging.
**Rule Used:** New UI must not override or deviate from the default styling of v3 design-system components (frontend/src/components/v3). Follow DESIGN.md. Flag: legacy v2 color tokens (e.g. mineshaft-*, bunker-*) instead of semantic tokens li … the rest (164 characters)ke bg-org or text-dange... (source) Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
| #8582 2026-10-20 14:59 | cubic inline · P2 | frontend/src/hooks/api/alerts/types.ts:258 | defect-functional | This schema accepts more than 20 headers, but the backend rejects arrays above 20, so 21+ entries pass client validation and fail on submit. Add a `.max(20, ...)` constraint here.
```suggestion
headers: z
.array(
z.object({
key: z.string().trim().min(1, "Header name is required"),
value: z.string().min(1, "Header value is required")
})
)
.max(20, "A webhook channel can have at most 20 headers")
.default([]),
``` |
| #8582 2026-10-20 14:59 | cubic review-body | — | summary | **4 issues found** across 23 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8582 2026-10-20 14:59 | cubic inline · P2 | frontend/src/views/Alerts/WebhookHeadersField.tsx:65 | defect-functional | The Add Header button remains enabled after 20 entries, but the server rejects more than 20 headers, leaving the channel unsavable. Disable or hide the button at the limit.
```suggestion
<Button
type="button"
variant="outline"
size="xs"
className="self-start"
isDisabled={fields.length >= 20}
onClick={() => append({ key: "", value: "" })}
``` |
| #8582 2026-10-20 14:59 | cubic inline · P2 | backend/src/services/alert/channels/alert-channel-registry.ts:33 | defect-security | Custom headers can carry API keys, but channel details return their values verbatim because `headers` is not redacted. Redact custom header values and expose only names and has-value flags. |
| #8582 2026-10-20 14:59 | cubic inline · P2 | docs/documentation/platform/pki/applications/alerting/webhook-alerts.mdx:63 | docs | This says all credentials are hidden and blank fields preserve them, but Basic Auth usernames and API Key header names remain visible and must stay populated on updates. Clarify that only secret values are hidden and reused when left blank.
```suggestion
Infisical hides saved secret values; leave those fields blank to keep them. Basic Auth usernames and API Key header names remain visible when you edit the channel. If you change the URL, re-enter the secret value, because Infisical only sends it to the URL it was saved with.
``` |
| #8526 2026-10-20 15:01 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The change adds persisted audit-log stream failure state, transactional delivery-failure events, and organization-wide alert support. The frontend adds alert configuration for audit-log streams and displays failure status and error details.
**Changes: Audit-log stream delivery alerts**
|Layer / File(s)|Summary|
|:---|:---|
|**Persist stream health** `backend/src/db/migrations/*`, `backend/src/db/schemas/audit-log-streams.ts`, `backend/src/ee/services/audit-log-stream/audit-log-stream-service.ts`|The stream schema and migration add nullable failure fields. Saving credentials clears the failure fields.|
|**Update … the rest (7410 characters)delivery and failure state** `backend/src/ee/services/audit-log-stream-outbox/*`, `backend/e2e-test/audit-log-stream-outbox.integration.spec.ts`, `backend/e2e-test/fakes/*`, `backend/e2e-test/setup/reset-fakes.ts`, `backend/vitest.e2e.config.mts`|The outbox processes delivery results and stale claims in transactions, tracks stream failures, and emits failure events. Tests and e2e fakes cover retries, drops, recovery, and failure transitions.| |**Route failure events to organization alerts** `backend/src/services/alert/providers/audit-log-stream-alert-*`, `backend/src/server/routes/index.ts`, `backend/e2e-test/routes/v1/audit-log-stream-failure-alert.spec.ts`, `backend/e2e-test/testUtils/license.ts`, `backend/CLAUDE.md`|The alert provider handles delivery-failure events and resolves affected streams. Route registration and an end-to-end test cover alert delivery to an organization recipient.| |**Configure alerts and display stream health** `frontend/src/hooks/api/alerts/types.ts`, `frontend/src/hooks/api/auditLogStreams/types/providers/root-provider.ts`, `frontend/src/pages/organization/AuditLogsPage/components/AuditLogStreamTab/*`, `frontend/src/pages/organization/IdentityDetailsByIDPage/components/IdentityAlertAction.tsx`, `frontend/src/pages/project/IdentityDetailsByIDPage/components/ProjectIdentityAlertAction.tsx`, `frontend/src/views/Alerts/*`|The frontend adds audit-log stream alert types and setup, supports resource-less alerts, and displays failure details in stream rows. Identity alert actions now provide their resource type.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~45 minutes **Change:** Feature ### Sequence Diagram(s) ```mermaid sequenceDiagram participant OutboxService participant OutboxDAL participant EventOutbox participant AlertProvider participant AlertDAL OutboxService->>OutboxDAL: Commit delivery result and stream health OutboxService->>EventOutbox: Emit delivery-failed event in transaction EventOutbox->>AlertProvider: Dispatch delivery-failed event AlertProvider->>AlertDAL: Resolve stream IDs and providers ``` **Suggested reviewers:** `thiago-as`, `scott-ray-wilson`, `calvin486` Merge Risk: ⚪ Minimal · up to a4bd4 This change persists audit-log stream health, raises one organization-wide alert when a stream begins dropping logs, and shows a failing badge in the UI. No defects were confirmed in failure tracking, recovery, alert routing, or the shared alert form changes. Existing identity and PAM alert screens still pass their resource types. The change appears ready to merge after normal checks. Security architecture risk: 🟡 Moderate · up to a4bd4 The change improves visibility into audit-log delivery failures and preserves organization-level access controls. However, a failure while recording the notification can still allow delivery work to be permanently dropped while the stream appears healthy. The new failure-alert guarantee therefore remains incomplete. **Retained concerns** - **Medium · reliability · observed:** The new notification contract permits an exhausted delivery to be dropped without persisted failure health or a durable alert event. Both delivery and stale recovery delete exhausted rows before calling flagStreamFailing. If health mutation or event emission fails, that helper rolls back only its savepoint, logs the error, and returns successfully, allowing the deletion to commit. A healthy stream can consequently remain displayed as healthy after losing delivery work; if no further drops occur, the documented next-drop retry cannot repair the missing notification. This compromises audit-delivery monitoring rather than introducing the preexisting drop policy. Security review details **Security Blast Radius** - _inferred_ — A failure of one stream can trigger the organization's configured stream alerts, rather than only a stream-specific notification. The inspected target lookup constrains stream resolution to the alert organization; the new provider does not establish cross-organization authority. **Trust Boundaries and Controls** - _observed_ — Creation invokes server-side provider permission and scope assertions using the actor's organization. Listing checks provider Read permission and filters by that organization. The provider maps these operations to Settings permissions; the frontend Settings gate is not the sole authorization control. **Resilience and Maintainability Implications** - _observed_ — The rollback test proves that explicitly aborting a transaction can restore dropped rows, health, and events together. It calls the DAL and emitter directly, however, so it does not establish that the production helper propagates an emission failure. That helper deliberately catches the failure and preserves the outer deletion. **Hardening Proposals** - _proposed_ — Define an explicit externally safe error-content contract, using classified messages or redaction before persistence and notification. Length limiting alone does not establish confidentiality, although the inspected evidence did not prove a credential-disclosure path. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 29 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title is concise, uses conventional commit format, and clearly identifies the primary change: alerts for audit-log streams that start dropping logs. | | Description check | | The description covers the context, behavior changes, verification steps, screenshots, feature type, and most checklist items. It is relevant and sufficiently complete. | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 29 files. (1 skipped: 1 unsupported.) **Resolution** Write docstrings for the functions missing them to satisfy the coverage threshold. ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8526 2026-10-20 15:13 | cubic inline · P2 | backend/src/ee/services/audit-log-stream-outbox/audit-log-stream-outbox-service.test.ts:609 | tests | This only checks that the sweep runs more than once, so a regression that raises the batch cap still passes. Assert the expected cap to protect the bounded-work guarantee.
```suggestion
expect(recoverStaleClaims.mock.calls).toHaveLength(10);
``` |
| #8526 2026-10-20 15:13 | cubic inline · P2 | backend/src/ee/services/audit-log-stream/audit-log-stream-service.ts:204 | defect-functional | This clears the failure state on filter- or mode-only updates that resubmit unchanged credentials; the validation ping can succeed while actual batch delivery still fails. Clear health only after credentials actually change or a real delivery succeeds, or the failing indicator disappears and the next drop emits another alert. |
| #8526 2026-10-20 15:13 | cubic review-body | — | summary | **3 issues found** across 30 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8526 2026-10-20 15:13 | cubic inline · P2 | backend/src/ee/services/audit-log-stream-outbox/audit-log-stream-outbox-service.ts:421 | defect-concurrency | A clean batch can erase a newer failure when stream drains overlap: its send may succeed before another drain drops rows, but its transaction can clear the flag afterward. Gate the clear on a success newer than `failingSince` or serialize result commits per stream. |
| #8526 2026-10-20 15:14 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Medium impact]** The changes affect audit log stream delivery and alerts within an organization\.
Changes are required before merging because an alert-insert failure can permanently remove audit logs without saving the configured notification or failing status.
Findings
1. **Dropped logs go unreported**
Summary
Adds organization-wide alerts when an external audit log stream starts dropping logs.
- Audit log streams record delivery failures and alert on dropped rows.
- Organizations can get alerts when an audit log stream starts failing.
- Audit Logs settings now let people manage stream alerts and see stream health.
Do not merge until dropping the logs also gu … the rest (170 characters)arantees a saved notification or durable notification retry. Reviews (1) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." · Reviewed by the reviewer |
| #8526 2026-10-20 15:14 | greptile inline · P1 | backend/src/ee/services/audit-log-stream-outbox/audit-log-stream-outbox-service.ts:247 | defect-data | **Dropped logs go unreported**
If the alert-event insert fails, `flagStreamFailing` rolls back its savepoint and catches the error, but the outer transaction still commits deletion of the exhausted audit-log rows. The stream retains no failing status and no alert is queued. With no further logs, subsequent drains and stale-claim sweeps cannot retry the notification, so an organization can permanently lose logs without receiving its configured alert.
Before merging, let this error roll back the drop too, or save a durable notification retry before committing the deletion.
Artifacts
**Evidence from the check**
- Authored harness loads the actual historical and head services, injects a dat … the rest (1644 characters)abase-side failure, and asserts persisted state—the executed test source is included. **Evidence from the check** - Runner initializes an isolated PostgreSQL cluster, captures both checks with commands and exit codes, and stops the server—the verification is reproducible. **Command output from the check** - Checked installed database commands and provisioning options—PostgreSQL was initially absent, but Debian package tooling was available. **Command output from the check** - Downloaded and extracted Debian PostgreSQL packages into the workspace and checked linked libraries—a real server could be run without installing a system service. **Command output from the check** - Initialized and started a fresh user-owned cluster with TCP disabled—the test used an isolated real database. **Command output from the check** - Executed the historical service with a failing event insert, then recovered the database and swept the retained stale claim—the row survived rollback and an alert was eventually persisted. **Command output from the check** - Executed the head service under the same insertion failure and checked recovery—the deletion committed while health and alert records were absent, with no retry after recovery. **Command output from the check** - Captured the exact head, candidate source lines, comparison revision, runtime versions, and clean tracked diff—the check targeted the requested candidate without production edits. **Command output from the check** - Stopped the isolated PostgreSQL server after both checks—the verification left no test database server running. Ran code and verified through TREX |
| #8638 2026-10-20 15:16 | coderabbit issue-comment | — | summary | 📝 Walkthrough
This pull request adds OpenAPI operation identifiers and ACS documentation. It also extends test clients and fake DNS, adds an in-process SAML identity provider, and introduces integration tests for SAML configuration, login, redirects, assertion validation, and enforcement.
**Changes: SAML integration test support and coverage**
|Layer / File(s)|Summary|
|:---|:---|
|**Document SAML and email-domain API operations** `backend/src/ee/routes/v1/email-domain-router.ts`, `backend/src/ee/routes/v1/saml-router.ts`, `backend/src/lib/api-docs/constants.ts`, `tests/clients/api/oapi-codegen.yaml`|Adds OpenAPI operation identifiers for email-domain and SAML routes, documents ACS param … the rest (7069 characters)eters, and updates the generated-client operation allowlist.| |**Configure test clients and tenant access** `tests/harness/infisical/client.go`, `tests/harness/infisical/client_test.go`, `tests/harness/infisical/infisical.go`, `tests/harness/tenant.go`, `tests/docs/client.md`|Adds configurable client options, redirect control, anonymous tenant clients, and a method that returns the test server URL. Documents redirect and cookie handling.| |**Support TXT records in fake DNS** `tests/infra/fakenet/*`, `tests/cmd/fakenet/main.go`, `tests/docs/fakes.md`|Adds TXT record storage, DNS responses, admin endpoints, and a publishing helper with cleanup. Updates the DNS server call, adds DNS tests, and documents TXT record setup.| |**Build and verify the SAML identity-provider fake** `tests/fakes/samlidp/*`, `tests/go.mod`|Adds a fake SAML IdP that parses redirect requests and generates configurable signed responses. Adds tests for request parsing, signing, tampering, and non-success responses, plus XML-signing dependencies.| |**Set up SAML integration fixtures and configuration tests** `tests/suites/sso/saml/helpers_test.go`, `tests/suites/sso/saml/main_test.go`, `tests/suites/sso/saml/config_test.go`|Adds shared SAML test setup and helpers, suite initialization, and tests for creating, reading, and updating SAML configurations.| |**Cover SAML login and redirect flows** `tests/suites/sso/saml/login_test.go`, `tests/suites/sso/saml/redirect_test.go`|Adds tests for SAML signup and login, account linking, identity attributes, redirect routing, callback ports, and inactive or unknown configurations.| |**Cover SAML validation and enforcement** `tests/suites/sso/saml/assertion_test.go`, `tests/suites/sso/saml/provider_test.go`, `tests/suites/sso/saml/enforcement_test.go`|Adds tests for invalid assertions and email domains, provider-specific signing and audience behavior, and organization SAML enforcement.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~60 minutes **Change:** Other **Suggested reviewers:** `akhilmhdh` Merge Risk: 🔵 Low · up to 920d4 Production behavior changes only through API documentation metadata and a new, documentation-oriented body schema on the SAML login callback. The rest adds SAML end-to-end tests and test infrastructure. One small follow-up remains: add generous field limits to the new callback schema to meet the project's input-validation standard. Request size is already capped, so this is safe to merge with that follow-up. Security architecture risk: 🔵 Low · up to 920d4 The inspected SAML changes retain the existing authentication flow. Most additions support integration testing, and no introduced security weakness was established. Some interface behavior, network exposure, and test-state cleanup details remain unverified, preventing a conclusive minimal-risk assessment. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The demonstrated downstream scope is SAML integration testing and fake-network DNS state. The response configuration has multiple test dependents, but high fanout does not itself establish production exposure. External reachability of the TXT admin interface and isolation among concurrent test domains remain unresolved. **Trust Boundaries and Controls** - _observed_ — The inspected base-to-head SAML comparison retains passport authentication in preValidation, before schema validation, and retains configuration-route authentication hooks. Operation identifiers and the ACS body declaration do not replace those controls or create a new unauthenticated route. Exact unknown-field behavior was not independently verified. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 21.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 46 functions across 23 files. (4 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly identifies the primary change: adding SAML API end-to-end test coverage. It follows the conventional commit format, although the wording could be more precise about the broader test … | | Description check | | The description includes the required Context, Screenshots, Steps to verify, Type, and Checklist sections. It provides a basic summary and verification command, and the generated summary supplies addi… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Title check **Explanation** The title clearly identifies the primary change: adding SAML API end-to-end test coverage. It follows the conventional commit format, although the wording could be more precise about the broader test coverage. Full details: Description check **Explanation** The description includes the required Context, Screenshots, Steps to verify, Type, and Checklist sections. It provides a basic summary and verification command, and the generated summary supplies additional implementation context. Several checklist items remain unchecked, but the description is mostly complete. Full details: Docstring Coverage **Explanation** Docstring coverage is 21.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 46 functions across 23 files. (4 skipped: 4 unsupported.) **Resolution** Write docstrings for the functions missing them to satisfy the coverage threshold. ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 27 - Waived: $6.75 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8638 2026-10-20 15:23 | coderabbit inline · P2 | backend/src/ee/routes/v1/saml-router.ts:362 | hardening | **Add explicit ACS field bounds, but do not add a second pre-passport validator.**
The new `SAMLResponse` and `RelayState` properties lack the `.max()` bounds required by the backend validation guide. Add limits based on the supported SAML payload contract. Do not choose an arbitrary smaller limit: Zod validation runs after `passport.authenticate`, so it may reject a response that passport already accepted.
Fastify already applies its default 1 MiB `bodyLimit` while parsing the form body, before `preValidation`. The requested pre-passport total-size protection therefore already exists. |
| #8638 2026-10-20 15:23 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8638 2026-10-20 15:26 | cubic inline · P1 | tests/suites/sso/saml/config_test.go:191 | defect-security | This makes a certificate-only update silently disable organization-wide SSO enforcement. Preserve enforcement when rotating the IdP certificate and assert it remains enabled here, or password logins regain access. |
| #8638 2026-10-20 15:26 | cubic inline · P2 | tests/docs/fakes.md:113 | docs | The literal `value` will not verify an Infisical email domain; the verifier requires `infisical-domain-verification=<VerificationCode>` at the record name returned by the API. Show the required value and record name in this example. |
| #8638 2026-10-20 15:26 | cubic inline · P2 | tests/suites/sso/saml/helpers_test.go:144 | defect-functional | Azure SP-initiated responses expect `spn:<issuer>`, but this default uses `SITE_URL`, so `spLogin` fails unless each Azure caller overrides `Audience`. Choose the default based on the configured provider and login flow. |
| #8638 2026-10-20 15:26 | cubic inline · P2 | backend/src/ee/routes/v1/saml-router.ts:361 | design | Require `SAMLResponse` in this schema; the ACS can only authenticate with an IdP assertion, but OpenAPI currently lets generated clients omit it and send requests Passport rejects.
```suggestion
SAMLResponse: z.string().describe(SamlSso.ACS.SAMLResponse),
``` |
| #8638 2026-10-20 15:26 | cubic review-body | — | summary | **4 issues found** across 29 files
**Tip**: the reviewer can generate docs of your entire codebase and keep them up to date. Try it here.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8638 2026-10-20 15:30 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** No blocking findings were identified; the checked support behavior passed, while complete application-backed SAML behavior remains unverified.
Summary
Adds API end-to-end tests for SAML login, signup, redirects, provider signatures, configuration, and enforcement.
- SAML tests now cover login, setup, redirects, and enforcement.
- Tests can use a fake SAML identity provider to drive login responses.
- fakenet can publish and answer test DNS TXT records.
- The API client and test harness can call and inspect SAML endpoints.
The container-free identity-provider, DNS, and HTTP-client support tests passed. Local checks also verified TXT publication and … the rest (850 characters)resolution, long-value chunking, isolated record cleanup, composed request headers, ACS form encoding, and redirect inspection. No blocking defect was identified in the checked paths. PierreOucif explicitly preserves the existing enforcement reset on configuration updates and intentionally leaves the ACS strings unbounded. Neither is reported as a new defect. ### the check validation blocked The selected application-backed SAML signup, returning-user login, and invalid-signature tests could not run because their 180-second timeout expired while the backend Docker image was still building. No selected test began and no product endpoint was reached. Complete SAML signup, session creation, and invalid-signature rejection remain unverified. Reviews (1) · Last reviewed commit: "feat: addressed the reviewer review" · Reviewed by the reviewer |
| #8592 2026-10-20 15:33 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The certificate authority installation flow now uses v3 dialog and form components. App connection forms support both sheet and dialog layouts, with the sheet layout remaining the default.
**Changes: Certificate authority installation dialogs**
|Layer / File(s)|Summary|
|:---|:---|
|**Add dialog layout to app connection forms** `frontend/src/pages/organization/AppConnections/AppConnectionsPage/components/AppConnectionForm/AppConnectionForm.tsx`, `.../AppConnectionFormContext.tsx`, `.../AppConnectionFormFooter.tsx`|`AppConnectionForm` accepts an optional `"sheet"` or `"dialog"` layout and defaults to `"sheet"`. … the rest (5085 characters)Create and update forms adjust their layout classes based on the setting. The footer renders `DialogFooter` for dialog layout and retains the sheet footer for sheet layout.| |**Migrate certificate authority installation dialogs** `frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/CaInstallCertModal/*`, `AGENTS.md`|The installation modal and ADCS, external, internal, and Venafi forms now use v3 dialog and form components. Connection creation passes `layout="dialog"` to `AppConnectionForm`. The development guidance now directs v2 `Modal` parents hosting v3 overlays to migrate to v3 `Dialog`.| **Priority:** ⬇️ Low **Estimated code review effort:** 3 (Moderate) | ~25 minutes **Change:** Refactor **Suggested reviewers:** `carlosmonastyrski`, `claude`, `andrewhuhh` Merge Risk: ⚪ Minimal · up to a0e48 The migration of the CA installation flow to v3 dialog components, plus the dialog layout option for app connection forms, shows no concrete merge-blocking issue. The sheet layout remains the default for existing callers. Security architecture risk: 🔵 Low · up to a0e48 The layout change preserves the inspected permission checks, project identity, and installation requests. No introduced security weakness was identified, but clipboard failure behavior and interruption during installation remain only partially validated. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The inspected new layout caller does not expand connection-creation authority or request scope: it retains the current project identity, and connection creation forwards projectId through the existing mutation. This establishes frontend continuity, not independent proof of backend authorization. **Trust Boundaries and Controls** - _observed_ — The CA connection-creation affordance is unavailable unless the project permission permits creating app connections. Changing the layout does not replace that check or move completion and cancellation authority away from the surrounding caller. **Resilience and Maintainability Implications** - _observed_ — The existing connection-creation discard guard increments an attempt token on dismissal and ignores a completion from an invalidated attempt. The PR adds only the layout argument at this caller. The guard protects subsequent UI selection; it does not demonstrate cancellation or rollback of a server-side creation request. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 10 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the primary change: migrating CaInstallCertModal to the v3 Dialog. It uses the conventional commit format. | | Description check | | The description includes the required Context, Screenshots, Steps to verify the change, Type, and Checklist sections. It explains the migration, testing coverage, and compatibility with the existing s… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes the required Context, Screenshots, Steps to verify the change, Type, and Checklist sections. It explains the migration, testing coverage, and compatibility with the existing sheet layout. Full details: Docstring Coverage **Explanation** Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 10 files. (1 skipped: 1 unsupported.) **Resolution** Write docstrings for the functions missing them to satisfy the coverage threshold. ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8592 2026-10-20 15:41 | cubic review-body | — | summary | **3 issues found** across 11 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8592 2026-10-20 15:41 | cubic inline · P2 | frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/CaInstallCertModal/InternalCaInstallForm.tsx:158 | defect-functional | These three labels are not programmatically connected to their sibling controls, leaving the parent, validity, and path-length controls without accessible names. Add a matching `htmlFor` and control `id` for each field. |
| #8592 2026-10-20 15:41 | cubic inline · P2 | frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/CaInstallCertModal/ExternalCaInstallForm.tsx:88 | defect-functional | This renders the CSR section title as an unassociated `<label>` instead of a heading, so screen-reader heading navigation skips it. Use a heading element for this section title.
```suggestion
<h3>Certificate Signing Request</h3>
``` |
| #8592 2026-10-20 15:41 | cubic inline · P2 | frontend/src/pages/organization/AppConnections/AppConnectionsPage/components/AppConnectionForm/AppConnectionFormFooter.tsx:37 | defect-functional | This fallback is the default for existing AppConnectionForm sheets, so it also changes their footer to put Cancel first and render it as ghost. Preserve the sheet's original submit-first, outline-Cancel layout; keep the new treatment in the dialog branch. |
| #8592 2026-10-20 15:47 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No merge-blocking issues were identified in this frontend dialog migration.
Summary
Migrates the CA certificate install flow from the v2 modal to the v3 dialog.
- CA certificate installation now runs in a v3 dialog.
- CA install forms use v3 fields and scrolling sections.
- Connection setup opens as a dialog inside CA installation.
No actionable issues were found. No merge-blocking issues were identified.
Reviews (1) · Last reviewed commit: "improvement(cert-manager): migrate CaIns..." · Reviewed by the reviewer |
| #8568 2026-10-20 15:49 | coderabbit issue-comment | — | summary | 📝 Walkthrough
Secret Scanning now uses organization-scoped URLs without project IDs. Backend project creation and project resolution establish the active project, while frontend cookies and navigation preserve project selection. Secret Scanning screens and shared access-management interfaces now use product-specific routing and labels.
**Changes: Secret Scanning organization scope**
|Layer / File(s)|Summary|
|:---|:---|
|**Project bootstrap and request resolution** `backend/src/ee/services/secret-scanning-v2/*`, `backend/src/ee/services/sub-org/sub-org-service.ts`, `backend/src/services/org/org-service.ts`, `backend/src/server/plugins/inject-secret-scanning-project-id*`, `backend/src/ser … the rest (7275 characters)vices/project/*`, `backend/src/services/smtp/emails/SecretScanningScanFailedTemplate.tsx`, `backend/CLAUDE.md`, `CLAUDE.md`|Organization and sub-organization creation bootstrap Secret Scanning projects for creators. Request resolution honors explicit project IDs and valid organization-scoped cookies before falling back to the active project. Project billing counts and failure-email project details were updated.| |**Implicit project resolution and organization routes** `frontend/src/helpers/project.ts`, `frontend/src/helpers/secretScanningActiveProject.ts`, `frontend/src/hooks/*`, `frontend/src/pages/secret-scanning/layout.tsx`, `frontend/src/routes.ts`, `frontend/src/const/routes.ts`, `frontend/src/pages/redirects/*`, `frontend/src/pages/secret-scanning/*/route.tsx`, `frontend/src/pages/project/*/route-secret-scanning.tsx`|Secret Scanning routes no longer require `$projectId`. The layout resolves an implicit project from active-project and membership data. Index and legacy project-URL redirects preserve organization context, and legacy project URLs can pin the project cookie.| |**Project selection and navigation** `frontend/src/components/v3/platform/RootCommandMenu/RootCommandMenu.tsx`, `frontend/src/helpers/organizationSwitch.ts`, `frontend/src/layouts/ProjectLayout/components/TypeSelect/TypeSelect.tsx`, `frontend/src/layouts/SecretScanningLayout/components/SecretScanningInstanceBanner.tsx`, `frontend/src/pages/organization/ProjectsPage/*`, `frontend/src/pages/organization/SettingsPage/components/OrgSecretScanningTab/OrgSecretScanningTab.tsx`, `frontend/src/pages/organization/*/*ProjectRow.tsx`, `frontend/src/pages/public/ErrorPage/components/ProjectAccessError.tsx`|Project selection is stored in an organization-scoped cookie before navigation. Organization project views resolve the Secret Scanning project and handle access restrictions; project links and access-error resolution account for Secret Scanning.| |**Secret Scanning and access-management interface** `frontend/src/pages/secret-scanning/*`, `frontend/src/pages/project/AccessControlPage/*`, `frontend/src/pages/project/GroupDetailsByIDPage/*`, `frontend/src/pages/project/IdentityDetailsByIDPage/*`, `frontend/src/pages/project/MemberDetailsByIDPage/*`, `frontend/src/pages/project/RoleDetailsBySlugPage/*`, `frontend/src/pages/project/AppConnectionsPage/*`, `frontend/src/pages/project/AuditLogsPage/*`, `frontend/src/pages/organization/AppConnections/*`, `frontend/src/components/auth/signupProducts.ts`|Secret Scanning screens use organization-level navigation and revised product wording. Shared access-management labels now use product visibility and label helpers instead of Certificate Manager-only conditions.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~60 minutes **Change:** Bug fix ### Sequence Diagram(s) ```mermaid sequenceDiagram participant OrganizationService participant bootstrapSecretScanningProject participant Transaction OrganizationService->>bootstrapSecretScanningProject: bootstrap project for organization creator bootstrapSecretScanningProject->>Transaction: create project and administrator access ``` **Suggested reviewers:** `scott-ray-wilson`, `sheensantoscapadngan`, `akhilmhd` Merge Risk: 🔵 Low · up to e6432 Custom roles cannot be opened from a machine identity’s role card, and two messages still call Secret Scanning a project. These are bounded issues that can be fixed before merge or accepted for follow-up. Security architecture risk: 🔵 Low · up to e6432 The change affects project selection and administrator setup. Existing access checks and all-or-nothing provisioning limit the identified risk. No new exploitable path was established, although compatibility and operational coverage remain incomplete. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _observed_ — The new persistent authority consists of project-administrator membership in the newly created organization's Secret Scanning project. Backend cookie selection is constrained to a Secret Scanning project belonging to the authenticated organization. **Security Findings and Attack Paths** - _observed_ — A modified selection cookie can influence project selection but does not itself grant data-source creation authority. The inspected creation operation obtains project permission and checks the Create action before using connections, encrypting credentials, persisting the data source, or queuing a scan. These service controls are unchanged by the PR. **Trust Boundaries and Controls** - _observed_ — The sub-organization endpoint requires JWT or identity access-token authentication and passes server-derived permission context to the service. Parent-organization permission enforcement precedes the new bootstrap call; actor and organization binding remain unchanged from the comparison base. **Resilience and Maintainability Implications** - _observed_ — Existing database definitions assign cleanup ownership through cascading organization-to-project, organization/project-to-membership, and membership-to-role foreign keys. Organization deletion delegates the database removal within a transaction, providing an existing cleanup path for the newly provisioned records. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly summarizes the primary change: removing project terminology from Secret Scanning. It is concise and follows the conventional commit format. | | Description check | | The description explains the problem, affected areas, intended pattern, and migration behavior. The verification steps are empty, and checklist items remain unchecked, but the description is otherwise… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description explains the problem, affected areas, intended pattern, and migration behavior. The verification steps are empty, and checklist items remain unchecked, but the description is otherwise sufficiently complete. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 50 files. (35 skipped: 2 unsupported, 33 over the file limit.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8568 2026-10-20 16:03 | coderabbit inline · P2 | frontend/src/pages/project/IdentityDetailsByIDPage/components/IdentityRoleDetailsSection/IdentityRoleDetailsSection.tsx:148 | defect-functional | **Do not treat Secret Scanning as a standalone product for role behavior.**
`isStandaloneProduct` controls more than the card title. It also does these things:
- Role rows become non-clickable, with no `role`, no `tabIndex`, and no `onClick`.
- The sheet description says "pre-defined roles" only.
Secret Scanning supports custom project roles. `ProjectRoleList` shows "Add Role" for every type except Certificate Manager, and this PR adds `RoleDetailsBySlugPage/route-secret-scanning.tsx`. After this change, a machine identity's custom Secret Scanning role cannot be opened from this card. `MemberRoleDetailsSection` still allows that navigation for users, so the two pages now behave differently … the rest (611 characters). Use `hidesProjects` for the title only. Keep navigation and the sheet text tied to `isCertManager || isPam`. Proposed fix ```diff - const isSecretScanning = currentProject?.type === ProjectType.SecretScanning; - const isStandaloneProduct = isCertManager || isPam || isSecretScanning; + const isStandaloneProduct = isCertManager || isPam; + const projectsHidden = hidesProjects(currentProject?.type); ``` ```diff - <CardTitle>{isStandaloneProduct ? "Roles" : "Project Roles"}</CardTitle> + <CardTitle>{isStandaloneProduct || projectsHidden ? "Roles" : "Project Roles"}</CardTitle> ``` |
| #8568 2026-10-20 16:03 | coderabbit review-body | — | summary | **Actionable comments posted: 1**
> [!CAUTION]
> Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
>
> **⚠️ Outside diff range comments (2)**
>
>
> 🟡 Minor · Use getProjectLabel in the remove-user success toast. · MemberDetailsByIDPage.tsx:107-112
>
> `frontend/src/pages/project/MemberDetailsByIDPage/MemberDetailsByIDPage.tsx:107-112`
> **🎯 Functional Correctness** | **🟡 Minor** | **⚡ Quick win**
>
> **Use `getProjectLabel` in the remove-user success toast.**
>
> This toast still checks only `ProjectType.CertificateManager`. When a user is removed on Secret Scanning, the toast says "Successfully removed user from project". The other strings in this … the rest (1730 characters)file now use `projectLabel`. Use the same helper here. You can compute the label once above `handleRemoveUser`. > > > Proposed fix > > ```diff > - text: `Successfully removed user from ${ > - currentProject?.type === ProjectType.CertificateManager ? "Certificate Manager" : "project" > - }`, > + text: `Successfully removed user from ${ > + hidesProjects(currentProject?.type) ? getProjectLabel(currentProject?.type) : "project" > + }`, > ``` > > > > > > > > > 🟡 Minor · Use the product label in the delete-identity error and… · IdentityDetailsByIDPage.tsx:210-215 > > `frontend/src/pages/project/IdentityDetailsByIDPage/IdentityDetailsByIDPage.tsx:210-215` > **🎯 Functional Correctness** | **🟡 Minor** | **⚡ Quick win** > > **Use the product label in the delete-identity error and dialog.** > > The rest of this page now uses `accessControlLabel` and `namesProduct`. The delete-identity error toast and the delete dialog description still check only `isCertManager`. On Secret Scanning, users see "Failed to delete project machine identity" and "permanently deletes the project machine identity". This goes against the PR goal of removing "project" wording from Secret Scanning. Use `getProjectLabel(currentProject?.type)` when `projectsHidden` is true. > > > Proposed fix > > ```diff > - text: isCertManager > - ? "Failed to delete Certificate Manager machine identity" > - : "Failed to delete project machine identity" > + text: `Failed to delete ${projectsHidden ? getProjectLabel(currentProject?.type) : "project"} machine identity` > ... > - isCertManager ? "Certificate Manager" : "project" > + projectsHidden ? getProjectLabel(currentProject?.type) : "project" > ``` > > > > > > > > Also applies to: 457-459 > > > > > > |
| #8568 2026-10-20 16:04 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Medium impact]** The changes affect Secret Scanning entry\, access\, and project setup across organizations\.
Not safe to merge until navigation can no longer silently change a tab's selected project and redirect subsequent edits.
Findings
1. **Tabs switch projects unexpectedly**
Summary
Removes project wording from Secret Scanning and moves its pages to organization-scoped URLs.
- Secret Scanning opens at an organization URL and keeps its selected project in scope.
- Secret Scanning access, connection, and scan screens use product-focused wording.
- New organizations get a Secret Scanning project with creator admin access.
Do not merge until project selection … the rest (201 characters)remains stable within each tab; the current behavior can apply edits to the wrong project. Reviews (1) · Last reviewed commit: "Merge branch 'main' into matheus/remove-..." · Reviewed by the reviewer |
| #8568 2026-10-20 16:04 | greptile inline · P1 | frontend/src/pages/secret-scanning/layout.tsx:39 | defect-functional | **Tabs switch projects unexpectedly**
`beforeLoad` reads the shared cookie on each navigation. In an organization with several legacy projects, opening project A in one tab and then project B from organization settings in another tab overwrites that cookie. Navigating to Findings or Settings in the first tab then switches its project to B without an explicit project-selection action. Saving configuration in that tab sends the update to B instead of A.
Explicit `projectId` values do not prevent this: navigation has already replaced the project context and loaded B’s configuration. Keep the viewed project stable per tab, changing it only when the user explicitly opens another project. This n … the rest (1735 characters)eeds to be fixed before merging because ordinary navigation can redirect edits to the wrong project. Artifacts **Evidence from the check** - Contains the complete executed harness, narrow fixtures, imported product code, synchronization, mutation assertions, and capture commands; both comparison runs now complete successfully. **Command output from the check** - Records the command, working directory, exit code 0, settled route contexts, and request traces for both runs; head changes tab A’s write target from A to B. **Recording of the check** - Records tab A opening A, tab B selecting B, and tab A navigating through Findings and Settings before saving; the historical route preserves project A. **Tab A after saving with the historical project-ID route** - Captures the rendered Settings form after tab A completes the comparison save; the selected project remains A. **Tab B after selecting project B in the historical comparison** - Captures tab B after the actual organization-settings selector opens B; the second-tab selection was exercised before tab A navigated. **Recording of the check** - Records tab A opening A, tab B selecting B, and tab A settling on B through Findings and Settings before saving; the changed route redirects the subsequent edit to B. **Tab A after saving project B configuration at head** - Captures the rendered Settings form after tab A’s originally project-A session saves following the cross-tab selection; the settled selected project is B. **Tab B after selecting project B at head** - Captures tab B after the real organization-settings action opens B and writes the shared cookie; this selection precedes tab A’s context replacement. Ran code and verified through TREX |
| #8568 2026-10-20 16:08 | cubic review-body | — | summary | **11 issues found** across 88 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8568 2026-10-20 16:08 | cubic inline · P1 | frontend/src/pages/secret-scanning/layout.tsx:34 | defect-functional | The layout re-reads the organization-wide cookie on navigation, so another tab selecting project B can switch this tab from A and send configuration edits to B. Keep this tab’s resolved project stable, changing it only after an explicit project selection. |
| #8568 2026-10-20 16:08 | cubic inline · P2 | frontend/src/pages/secret-scanning/SettingsPage/SettingsPage.tsx:38 | defect-functional | This bypasses i18n for the browser title, so users of supported non-English locales now see “Secret Scanning Settings” in English. Add a localized Secret Scanning settings title key and pass its `t(...)` result here. |
| #8568 2026-10-20 16:08 | cubic inline · P2 | CLAUDE.md:116 | docs | This overstates when new organizations are bootstrapped: orgs created from the admin console have no creator, so `org-service.ts` skips this call and they resolve the project lazily. Qualify the creation claim and include admin-console orgs in the lazy-resolution case.
```suggestion
Secret Scanning has one active project per organization (the newest). Like Certificate Manager, its URLs are `/organizations/$orgId/secret-scanning/…` with no `$projectId`; legacy orgs can still hold several projects, so the frontend pins the one being viewed (see `frontend/CLAUDE.md`). New orgs with a creator and sub-orgs get it at creation (`bootstrapSecretScanningProject`); older orgs and orgs created from th … the rest (229 characters)e admin console get it lazily from `GET /api/v2/secret-scanning/project`. `projectId` is optional on its API (`inject-secret-scanning-project-id`). Do not add a project picker or a way to create more Secret Scanning projects. ``` |
| #8568 2026-10-20 16:08 | cubic inline · P2 | frontend/src/pages/project/AccessControlPage/components/ProjectRoleListTab/components/ProjectRoleList/ProjectRoleList.tsx:89 | defect-functional | Secret Scanning still shows project-specific copy in this card and after role deletion. Update these `isCertManager` copy checks to use the projectless-product condition so the Access Control page consistently removes “project” language. |
| #8568 2026-10-20 16:08 | cubic inline · P2 | frontend/src/pages/organization/ProjectsPage/components/ProjectCategoryOverview.tsx:431 | defect-functional | `ProjectCategoryOverview` ignores the per-org pinned project, so members of a legacy Secret Scanning project can have the tile blocked or be sent to the active project. Resolve the cookie against `projects` before computing access and entering. |
| #8568 2026-10-20 16:08 | cubic inline · P2 | frontend/src/helpers/organizationSwitch.ts:65 | defect-functional | This detour replaces a legacy organization’s saved Secret Scanning selection with its active project before opening the page. Route directly to `/secret-scanning/data-sources` so the layout’s cookie-aware resolver preserves the selected project.
```suggestion
section = "secret-scanning/data-sources";
``` |
| #8568 2026-10-20 16:08 | cubic inline · P2 | backend/src/services/org/org-service.ts:811 | defect-functional | The instance-bootstrap identity is created as an org admin only after this call, so it never receives Secret Scanning project membership. Project permission checks require that membership, and the lazy resolver will not backfill it once this project exists; add project-admin membership when the identity is created so instance-admin identity requests can access Secret Scanning. |
| #8568 2026-10-20 16:08 | cubic inline · P2 | frontend/src/helpers/secretScanningActiveProject.ts:35 | defect-functional | This accepts any project in the user's global project list, not just a Secret Scanning project in `orgId`. A legacy URL can pin another organization's or product's UUID, causing the Secret Scanning layout to resolve to the wrong project; validate against org- and product-scoped membership before returning it. |
| #8568 2026-10-20 16:08 | cubic inline · P2 | frontend/src/components/v3/platform/RootCommandMenu/RootCommandMenu.tsx:869 | defect-functional | These links omit both the URL project ID and a cookie update. In a legacy multi-project org, a cookie change from another tab can make a later layout load resolve a different project than this menu's `project`; pin `project.id` before each navigation. |
| #8568 2026-10-20 16:08 | cubic inline · P2 | frontend/src/pages/secret-scanning/layout.tsx:21 | defect-functional | `ensureQueryData` returns a cached active project ID even after it becomes stale, so revisiting after the org’s active Secret Scanning project changes can keep loading the previous project. Fetch this org-scoped ID on route load instead of accepting stale cached data.
```suggestion
context.queryClient.fetchQuery<string>({
queryKey: secretScanningV2Keys.activeProjectId(params.orgId),
queryFn: fetchSecretScanningProjectId,
staleTime: 0
}),
``` |
| #8568 2026-10-20 16:08 | cubic inline · P3 | frontend/src/pages/project/IdentityDetailsByIDPage/components/IdentityRoleDetailsSection/IdentityRoleDetailsSection.tsx:148 | docs | This changes the editor description to say only pre-defined roles are available, but the editor also offers custom roles for Secret Scanning. Keep the custom-role wording for this product. |
| #8614 2026-10-20 16:10 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The PR migrates secret-manager integration authorization and configuration pages from v2 to v3 UI components. Forms use structured cards, labeled fields, inline validation, and updated pending-button props. Several authorization and integration-creation flows now clear loading state in `finally` blocks.
**Changes: Integration authorization and configuration**
|Layer / File(s)|Summary|
|:---|:---|
|**Authorization forms** `frontend/src/pages/secret-manager/integrations/*AuthorizePage/*AuthorizePage.tsx`|Authorization pages use v3 card and field components, documentation badges, accessible validation messages, and pending-aware buttons. Several flows now clear loading state i … the rest (4378 characters)n `finally` blocks.| |**Configuration forms** `frontend/src/pages/secret-manager/integrations/*ConfigurePage/*ConfigurePage.tsx`|Configuration pages use v3 card, field, tab, select, toggle, and combobox components. Existing form values and conditional behavior remain wired through the new components. Several flows now clear loading state in `finally` blocks.| **Priority:** ➖ Normal **Estimated code review effort:** 3 (Moderate) | ~25 minutes **Change:** Refactor **Suggested reviewers:** `andrewhuhh` Merge Risk: 🔵 Low · up to e7218 The integration pages move to the newer UI components and keep their existing submission behavior. Loading spinners now also clear after failed requests. One small issue remains on the Azure DevOps connect page: a missing API token error also appears, and is announced, under the organization-name field. The change is safe to merge once that wiring is fixed or accepted. Security architecture risk: 🔵 Low · up to e7218 The inspected flows preserve project scope, credential selection and submission payloads while improving recovery after failed submissions. Remaining uncertainty concerns affected flows that were not fully traced. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The security-sensitive outcomes remain creation of project integration credentials and configuration of secret synchronization to selected provider targets. The inspected migration does not introduce another service, credential authority or target-selection mechanism; this conclusion is limited to the traced flows. **Trust Boundaries and Controls** - _observed_ — The inspected Checkly route retains authentication and organization/project context middleware. Its access-token request still carries the current project identifier, while the backend authenticates the request and enforces project integration-creation permission. Client pending state is not the authorization boundary. **Resilience and Maintainability Implications** - _observed_ — V3 pending buttons disable ordinary repeated activation, and finally cleanup releases the local lock after failure. This does not establish server-side idempotency or roll back persisted credentials. The inspected persistence operation still creates records, and successful authorization still invokes navigation without awaiting it; both behaviors predate this PR. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 16 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly identifies the migration of native secret-manager integration creation pages to V3 and follows the conventional commit format. | | Description check | | The description includes the required Context, Screenshots, Steps to verify the change, Type, and Checklist sections. It provides detailed scope, verification results, limitations, and testing status. | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8614 2026-10-20 16:15 | coderabbit inline · P2 | frontend/src/pages/secret-manager/integrations/AzureDevopsAuthorizePage/AzureDevopsAuthorizePage.tsx:160 | defect-functional | **The Organization Name field shows the API token error as its own error.**
The Organization Name `Field` reads its state from `apiKeyErrorText`. This affects `data-invalid`, `isError`, `aria-describedby` and the `FieldError` content. If the API token is blank, the page shows "API Key cannot be blank" twice: once under the token field and once under the organization name field. Both `FieldError` elements render with `role="alert"`, so screen readers announce the message twice. The organization-name input's accessible description also says the API key is blank, which is wrong for that input.
`handleButtonClick` does not validate `devopsOrgName`. Remove the error wiring from the organization … the rest (2120 characters)-name field. Add a separate `devopsOrgNameErrorText` state only if this field needs its own validation. 🐛 Proposed fix ```diff - <Field data-invalid={apiKeyErrorText !== ""}> + <Field> <FieldLabel htmlFor="azuredevops-azure-devops-organization-name"> @@ <Input placeholder="" value={devopsOrgName} onChange={(e) => setDevopsOrgName(e.target.value)} id="azuredevops-azure-devops-organization-name" - isError={apiKeyErrorText !== ""} - aria-describedby={ - apiKeyErrorText !== "" - ? "azuredevops-azure-devops-organization-name-error" - : undefined - } /> - {apiKeyErrorText !== "" && ( - <FieldError id="azuredevops-azure-devops-organization-name-error"> - {apiKeyErrorText} - </FieldError> - )} </Field> ``` 📝 Committable suggestion > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion <Field> <FieldLabel htmlFor="azuredevops-azure-devops-organization-name"> Azure DevOps Organization Name <Tooltip> <TooltipTrigger asChild> <button type="button" aria-label="More information"> ? </button> </TooltipTrigger> <TooltipContent> This is the slug of the organization. An example would be 'my-acme-org' </TooltipContent> </Tooltip> </FieldLabel> <Input placeholder="" value={devopsOrgName} onChange={(e) => setDevopsOrgName(e.target.value)} id="azuredevops-azure-devops-organization-name" /> </Field> ``` |
| #8614 2026-10-20 16:15 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8614 2026-10-20 16:20 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/AzureDevopsAuthorizePage/AzureDevopsAuthorizePage.tsx:134 | defect-functional | This tooltip trigger is a `<button>` nested inside the organization input’s `<label>`, which is invalid label content and can pollute the field’s accessible name. Render the label and tooltip trigger as siblings instead. |
| #8614 2026-10-20 16:20 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/AzureDevopsAuthorizePage/AzureDevopsAuthorizePage.tsx:156 | defect-functional | Remove the API-key error wiring from the organization-name field; a blank token currently renders the same alert under both inputs and gives the organization input an incorrect accessible description. |
| #8614 2026-10-20 16:20 | cubic review-body | — | summary | **2 issues found** across 16 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8614 2026-10-20 16:23 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No merge-blocking issue was identified; focused browser checks support preserved form behavior, without establishing authenticated backend or provider acceptance.
Summary
Migrates 16 native-integration creation pages to V3 components.
- Authorization pages show provider fields in V3 cards with connect actions in the footer.
- Setup pages use V3 fields and controls while keeping provider choices in their existing flow.
No actionable issues found. Focused browser checks of the actual AWS Parameter Store and CircleCI components preserved required validation, focus, form submission, dependent selections, and base/head mutation payloads. These checks … the rest (590 characters)used synthetic application boundaries and do not establish authenticated backend or provider acceptance. The prior source-based checks and passing whitespace check remain applicable. PierreOucif explicitly identifies the inherited SecretPathInput ref and blur omissions as outside this migration. PierreOucif also intentionally preserves direct routes, copy, retirement behavior, and the current shared Card API. No merge-blocking issue was identified within the reviewed scope. Reviews (1) · Last reviewed commit: "improvement(secret-manager): migrate nat..." · Reviewed by the reviewer |
| #8617 2026-10-20 16:25 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Authorization and configuration pages for nine secret manager integrations now use v3 UI components. The forms add structured labels and validation messages, and several submission handlers reset loading state in `finally` blocks. Existing integration and selection flows are retained.
**Changes: Secret manager integration pages**
|Layer / File(s)|Summary|
|:---|:---|
|**Authorization forms** `frontend/src/pages/secret-manager/integrations/*AuthorizePage/*.tsx`|The authorization pages use v3 cards, fields, and pending-state buttons. Fields display labels and inline validation errors with accessibility associatio … the rest (3369 characters)ns. Several authorization handlers now clear loading state in `finally` blocks.| |**Standard configuration forms** `frontend/src/pages/secret-manager/integrations/CloudflarePagesConfigurePage/CloudflarePagesConfigurePage.tsx`, `.../CloudflareWorkersConfigurePage/CloudflareWorkersConfigurePage.tsx`, `.../CodefreshConfigurePage/CodefreshConfigurePage.tsx`, `.../DatabricksConfigurePage/DatabricksConfigurePage.tsx`, `.../DigitalOceanAppPlatformConfigurePage/DigitalOceanAppPlatformConfigurePage.tsx`, `.../FlyioConfigurePage/FlyioConfigurePage.tsx`|These forms use v3 card sections, labeled fields, updated select controls, and pending-state actions. Existing selection behavior and conditions that disable actions when no services or apps are available remain in place. Several handlers now clear loading state in `finally` blocks.| |**GitHub and GitLab configuration forms** `frontend/src/pages/secret-manager/integrations/GithubConfigurePage/GithubConfigurePage.tsx`, `frontend/src/pages/secret-manager/integrations/GitlabConfigurePage/GitlabConfigurePage.tsx`|The forms use v3 fields, tabs, selectors, toggles, and dialogs. Submission handlers clear loading state in `finally` blocks. The GitLab page no longer destructures `handlePopUpClose` from the popup hook.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~45 minutes **Change:** Refactor **Suggested reviewers:** `andrewhuhh` Merge Risk: ⚪ Minimal · up to 39b98 The integration authorization and configuration pages move to the newer UI components. Their submit actions, OAuth flows, and selection behavior remain wired to the existing handlers. No concrete issue blocks merging. Provider submit and OAuth paths were not exercised end to end. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the migration of native secret-manager integration creation pages to V3 and follows the conventional commit format. | | Description check | | The description is complete and relevant. It documents the migration scope, preserved behavior, UI changes, screenshots, verification steps, change type, checklist status, and testing limitations. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 17 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8617 2026-10-20 16:35 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/GcpSecretManagerAuthorizePage/GcpSecretManagerAuthorizePage.tsx:160 | defect-functional | The field loses its previous `h-48` and now uses V3's `min-h-16`, leaving the service-account JSON editor only 4rem tall. Restore `h-48` so users can review pasted credentials without constant scrolling.
```suggestion
<TextArea
className="h-48"
``` |
| #8617 2026-10-20 16:35 | cubic review-body | — | summary | **1 issue found** across 17 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8617 2026-10-20 16:42 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No blocking issue was identified; the changes are safe to merge within the reviewed scope.
Summary
Migrates 17 native-integration creation pages from V2 to V3 components, adding composed card headers, content, footers, fields, menus, tabs, and toggles. Loading-state cleanup moves into `finally`, and GitLab’s overwrite prompt is protected while pending.
No actionable regression was found. Browser checks of the actual GitHub page source with synthetic project context and API responses confirmed repository selection and menu dismissal, value preservation across tabs, footer submission, pending duplicate-click prevention, and successful retry after r … the rest (871 characters)ejection. No real provider writes, external OAuth, or authenticated application routing were exercised. The other 16 pages were not rendered during these checks; the established source checks for GCP’s separate OAuth action and GitLab’s pending confirmation remain preserved. Acknowledged by `PierreOucif`: `SecretPathInput` ref/blur omissions and GitLab’s `initialSyncBehavior` ref/blur omissions are inherited limitations. Shared footer-band styling is explicitly deferred. No blocking issue was identified within the reviewed scope. - Provider credential pages use the V3 card layout. - Provider configuration pages use V3 cards and footers. - GitHub setup keeps its tabs and choices in a V3 card. - GitLab setup keeps its tabs and overwrite prompt in V3. Reviews (1) · Last reviewed commit: "improvement(secret-manager): migrate nat..." · Reviewed by the reviewer |
| #8616 2026-10-20 17:16 | coderabbit issue-comment | — | summary | 📝 Walkthrough
Secret Manager integration authorization and configuration pages now use v3 UI components. Several pages derive submit-button loading feedback from mutation pending state. The GCP configuration page also replaces its overwrite-warning modal with a v3 dialog.
**Changes: Authorization forms**
|Layer / File(s)|Summary|
|:---|:---|
|**Authorization forms** `frontend/src/pages/secret-manager/integrations/*AuthorizePage/*.tsx`|Provider authorization forms use v3 cards, fields, validation feedback, and pending-aware buttons. Several pages now use mutation pending state instead of local loading state.|
**Configuration forms**
|Layer / File(s)|Summary|
|:---|:---|
|**Configuration … the rest (4848 characters)forms and pending state** `frontend/src/pages/secret-manager/integrations/HashicorpVaultConfigurePage/*.tsx`, `HasuraCloudConfigurePage/*.tsx`, `HerokuConfigurePage/*.tsx`, `LaravelForgeConfigurePage/*.tsx`, `NetlifyConfigurePage/*.tsx`, `NorthflankConfigurePage/*.tsx`, `RailwayConfigurePage/*.tsx`|These pages use v3 form and card components. Existing configuration fields and selection behavior remain; several pages use mutation pending state for submit feedback.| |**Multi-select configuration forms** `frontend/src/pages/secret-manager/integrations/OctopusDeployConfigurePage/*.tsx`, `QoveryConfigurePage/*.tsx`|These pages use v3 fields, cards, and selection components while retaining their provider configuration controls and pending-aware submit buttons.| |**GCP configuration tabs and overwrite dialog** `frontend/src/pages/secret-manager/integrations/GcpSecretManagerConfigurePage/GcpSecretManagerConfigurePage.tsx`|The connection and options tabs use v3 components. The overwrite-warning prompt is now a v3 dialog with Cancel and Continue actions.| **Priority:** ⬇️ Low **Estimated code review effort:** 4 (Complex) | ~45 minutes **Change:** Refactor **Suggested reviewers:** `andrewhuhh` Merge Risk: 🔵 Low · up to ff5f6 The migration keeps the existing submit behavior. Remaining issues are small wording and accessibility fixes on the GCP and Qovery pages, and they can be fixed before or shortly after merge. Security architecture risk: 🔵 Low · up to ff5f6 The inspected changes preserve credential submission, secret-source selection, provider targets, and success navigation. Pending-aware actions improve request lifecycle handling. No introduced security concern was established, but server-side authorization and duplicate-request recovery were not independently verified. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The inspected frontend delta continues to act through existing project authorization records and selected provider resources. No additional credential authority or broader target scope was identified in the GCP and Octopus comparisons; server-enforced isolation was not independently established. **Trust Boundaries and Controls** - _observed_ — User-entered credentials are submitted with the current project identifier to the existing integration-auth endpoint. Configuration sends an integration authorization identifier and selected synchronization scope to the existing integration endpoint. These client-side selections and pending controls do not establish server-side authorization. **Resilience and Maintainability Implications** - _observed_ — GCP’s absence of an explicit form-level in-flight submission guard predates this PR. Pending buttons reduce repeat interaction but do not prove duplicate-request safety or reconciliation after an interrupted request. No introduced worsening was established. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: migrating native provider creation pages to V3. It uses the conventional commit format. | | Description check | | The description is complete and follows the repository template. It includes context, screenshots, verification steps, type classification, checklist status, scope, preserved behavior, and known verif… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description is complete and follows the repository template. It includes context, screenshots, verification steps, type classification, checklist status, scope, preserved behavior, and known verification limits. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 17 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8616 2026-10-20 17:20 | coderabbit inline · P2 | frontend/src/pages/secret-manager/integrations/GcpSecretManagerConfigurePage/GcpSecretManagerConfigurePage.tsx:454 | defect-functional | **Rename the `labelValue` field label to "Label Value".**
The `labelValue` controller displays the text "Label Name". The `labelName` field on Line 430 uses the same text. When `shouldLabel` is on, the user sees two fields with the same label. The user cannot tell which input sets the GCP label value. Screen readers announce the same name for both inputs.
🐛 Proposed fix
```diff
--- "a/frontend/src/pages/secret-manager/integrations/GcpSecretManagerConfigurePage/GcpSecretManagerConfigurePage.tsx"
+++ "b/frontend/src/pages/secret-manager/integrations/GcpSecretManagerConfigurePage/GcpSecretManagerConfigurePage.tsx"
@@ -449,9 +449,9 @@
name="labelValue"
… the rest (1006 characters)render={({ field, fieldState: { error } }) => ( <Field data-invalid={Boolean(error)}> <FieldLabel htmlFor="GcpSecretManagerConfigurePage-field-7"> - Label Name + Label Value </FieldLabel> <Input id="GcpSecretManagerConfigurePage-field-7" aria-describedby="GcpSecretManagerConfigurePage-field-7-feedback" ``` 📝 Committable suggestion > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion <FieldLabel htmlFor="GcpSecretManagerConfigurePage-field-7"> Label Value </FieldLabel> ``` |
| #8616 2026-10-20 17:20 | coderabbit inline · P2 | frontend/src/pages/secret-manager/integrations/GcpSecretManagerConfigurePage/GcpSecretManagerConfigurePage.tsx:514 | defect-functional | **Add a `DialogDescription` to the overwrite-warning dialog.**
`DialogContent` wraps Radix `DialogPrimitive.Content`. The dialog has a `DialogTitle` but no `DialogDescription`, so Radix does not set `aria-describedby` to the warning text. Assistive technology announces "Heads Up" but not the overwrite warning. Radix also logs a missing-description console warning in development. Use the v3 `DialogDescription` export, or set `aria-describedby` on `DialogContent` to the id of the warning paragraph.
♿ Proposed fix
```diff
--- "a/frontend/src/pages/secret-manager/integrations/GcpSecretManagerConfigurePage/GcpSecretManagerConfigurePage.tsx"
+++ "b/frontend/src/pages/secret-manager/integrations … the rest (1249 characters)/GcpSecretManagerConfigurePage/GcpSecretManagerConfigurePage.tsx" @@ -502,16 +502,18 @@ onOpenChange={(isOpen) => handlePopUpToggle("confirmIntegration", isOpen)} > <DialogContent isPending={isLoading}> <DialogHeader> <DialogTitle>Heads Up</DialogTitle> + <DialogDescription> + You're about to overwrite any existing secrets in GCP Secret Manager. + </DialogDescription> </DialogHeader> <DialogBody> - <p>You're about to overwrite any existing secrets in GCP Secret Manager.</p> - <p className="mt-4"> + <p> To avoid this behavior, you may consider adding a secret prefix/suffix or enabling labeling in the options tab. </p> </DialogBody> <DialogFooter> <DialogClose asChild> <Button onClick={() => handlePopUpClose("confirmIntegration")} variant="ghost"> ``` Also add `DialogDescription` to the `@app/components/v3` import. Based on learnings: for Dialog overlays, "verify that ... proper ARIA roles/attributes (role="dialog", aria-modal, aria-labelledby, aria-describedby) are set." _Source: Learnings_ |
| #8616 2026-10-20 17:20 | coderabbit inline · P2 | frontend/src/pages/secret-manager/integrations/QoveryConfigurePage/QoveryConfigurePage.tsx:210 | defect-functional | **Replace "Checkly" with "Qovery" in the card description.**
The Qovery configuration page tells users that their secrets sync to "Checkly environment variables". This is the wrong provider name, and users see it on every visit. This PR rewrites the line, so this is a good time to correct it.
✏️ Proposed fix
```diff
- Choose which environment in Infisical you want to sync to Checkly environment variables.
+ Choose which environment in Infisical you want to sync to Qovery environment variables.
```
📝 Committable suggestion
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing l … the rest (291 characters)ines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion <CardDescription> Choose which environment in Infisical you want to sync to Qovery environment variables. </CardDescription> ``` |
| #8616 2026-10-20 17:20 | coderabbit inline · P2 | frontend/src/pages/secret-manager/integrations/QoveryConfigurePage/QoveryConfigurePage.tsx:326 | docs | **Fix the typo "organizaztions" in the empty-state option.**
Users see this text when they have no Qovery organizations.
✏️ Proposed fix
```diff
--- "a/frontend/src/pages/secret-manager/integrations/QoveryConfigurePage/QoveryConfigurePage.tsx"
+++ "b/frontend/src/pages/secret-manager/integrations/QoveryConfigurePage/QoveryConfigurePage.tsx"
@@ -323,7 +323,7 @@
))
) : (
<SelectItem value="none" key="target-app-none">
- No organizaztions found
+ No organizations found
</SelectItem>
)}
< … the rest (377 characters)/SelectContent> ``` 📝 Committable suggestion > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion No organizations found ``` |
| #8616 2026-10-20 17:20 | coderabbit review-body | — | summary | **Actionable comments posted: 4** |
| #8616 2026-10-20 17:27 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/NorthflankConfigurePage/NorthflankConfigurePage.tsx:126 | defect-functional | This `aria-describedby` points to an element that this page never renders; the other three controls have the same dangling reference. Remove these attributes or render matching feedback elements. |
| #8616 2026-10-20 17:27 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/HashicorpVaultAuthorizePage/HashicorpVaultAuthorizePage.tsx:109 | defect-functional | Each input references a feedback ID even when its `FieldError` is not mounted, leaving dangling `aria-describedby` references before validation errors occur. Set each prop only when that field has an error. |
| #8616 2026-10-20 17:27 | cubic review-body | — | summary | **9 issues found** across 17 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8616 2026-10-20 17:27 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/LaravelForgeConfigurePage/LaravelForgeConfigurePage.tsx:106 | defect-functional | These `aria-describedby` targets are never rendered, so the controls expose broken ARIA references. Remove the three attributes or render matching feedback elements. |
| #8616 2026-10-20 17:27 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/RailwayConfigurePage/RailwayConfigurePage.tsx:151 | defect-functional | These `aria-describedby` references point to elements this page never renders, so assistive technology receives no described-by text and accessibility checks may fail; remove the references or render matching feedback elements. |
| #8616 2026-10-20 17:27 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/OctopusDeployConfigurePage/OctopusDeployConfigurePage.tsx:194 | defect-functional | Pressing Enter while a selector has focus never submits this form because the V3 `Combobox` prevents the default action even when its popup is closed. Preserve native Enter submission when no option is being committed. |
| #8616 2026-10-20 17:27 | cubic inline · P2 | frontend/src/pages/secret-manager/integrations/GcpSecretManagerConfigurePage/GcpSecretManagerConfigurePage.tsx:430 | defect-functional | Change this field label to “Label Value”; both GCP label inputs are currently announced as “Label Name,” so users and screen readers cannot distinguish them.
```suggestion
Label Value
``` |
| #8616 2026-10-20 17:27 | cubic inline · P3 | frontend/src/pages/secret-manager/integrations/NorthflankAuthorizePage/NorthflankAuthorizePage.tsx:67 | defect-functional | The input references an error element that is absent until validation fails, leaving a dangling `aria-describedby` IDREF in the normal state. Set this attribute only while the matching `FieldError` is rendered.
```suggestion
aria-describedby={
apiKeyErrorText !== "" ? "NorthflankAuthorizePage-field-1-feedback" : undefined
}
``` |
| #8616 2026-10-20 17:27 | cubic inline · P3 | frontend/src/pages/secret-manager/integrations/QoveryConfigurePage/QoveryConfigurePage.tsx:237 | defect-functional | These controls reference feedback IDs that are never rendered, leaving invalid ARIA ID references. Remove the `aria-describedby` attributes or render matching feedback elements. |
| #8616 2026-10-20 17:27 | cubic inline · P3 | frontend/src/pages/secret-manager/integrations/QoveryConfigurePage/QoveryConfigurePage.tsx:209 | defect-functional | Replace “Checkly” with “Qovery”; this description tells users that the Qovery integration syncs to the wrong provider.
```suggestion
Choose which environment in Infisical you want to sync to Qovery environment variables.
``` |
| #8616 2026-10-20 17:34 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No blocking issue was identified; the change is safe to merge within the reviewed scope.
Summary
Migrates 17 native-integration pages from V2 to V3 components while keeping their requests, payloads, and success navigation.
- Provider authorization pages use the shared V3 card layout.
- Provider configuration pages use V3 fields and a shared card layout.
- GCP’s overwrite warning uses the V3 dialog and action order.
Intentional limitations acknowledged by PierreOucif: existing direct-URL pages remain; native `required` attributes are removed under G3; inherited `SecretPathInput` ref/blur limits remain; existing omitted select refs are not repaire … the rest (343 characters)d. PierreOucif described provider-backed rendering and GCP dialog interactions as source-only checks. The additional GCP confirmation checks used an isolated rendered page with controlled dependencies, not a live provider connection. Reviews (1) · Last reviewed commit: "improvement(integrations): port native p..." · Reviewed by the reviewer |
| #8618 2026-10-20 17:36 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Authorization and configuration pages for seven integrations now use v3 UI components and mutation-provided pending states. The authentication-selection page also uses v3 card and button components. Existing integration fields, validation, and request flows remain represented.
**Changes: Integration page UI**
|Layer / File(s)|Summary|
|:---|:---|
|**Authentication selection layout** `frontend/src/pages/secret-manager/integrations/SelectIntegrationAuthPage/SelectIntegrationAuthPage.tsx`|The connection-selection card uses v3 card sections, updated connection buttons, and a v3 field separator.|
|**Authorization fo … the rest (2765 characters)rms and pending states** `frontend/src/pages/secret-manager/integrations/*AuthorizePage/*AuthorizePage.tsx`|Seven authorization forms use v3 fields, card sections, validation feedback, and documentation badges. Their buttons use pending state from the save mutation hook.| |**Configuration forms and pending states** `frontend/src/pages/secret-manager/integrations/*ConfigurePage/*ConfigurePage.tsx`|Eight configuration forms use v3 fields, selects, and card sections. Their buttons use pending state from the create mutation hook, while existing fields and disabled conditions remain. | **Priority:** ➖ Normal **Estimated code review effort:** 3 (Moderate) | ~25 minutes **Change:** Refactor **Suggested reviewers:** `andrewhuhh` Merge Risk: ⚪ Minimal · up to b08de No actionable PR-introduced issue remains; the change is mergeable after normal checks. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: migrating native provider creation pages to V3. It follows the conventional commit format. | | Description check | | The description is complete and follows the repository template. It explains the context, UI changes, verification steps, change type, checklist status, screenshots, preserved behavior, and known veri… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description is complete and follows the repository template. It explains the context, UI changes, verification steps, change type, checklist status, screenshots, preserved behavior, and known verification limitation. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 16 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8618 2026-10-20 17:44 | cubic inline · P3 | frontend/src/pages/secret-manager/integrations/WindmillConfigurePage/WindmillConfigurePage.tsx:4 | defect-functional | All three controls reference `*-feedback` IDs that this component never renders, leaving dangling ARIA descriptions. Remove these references or add matching feedback elements so assistive technology receives valid descriptions. |
| #8618 2026-10-20 17:44 | cubic review-body | — | summary | **1 issue found** across 16 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8618 2026-10-20 17:51 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No blocking findings remain; the reviewed changes are suitable to merge within the stated provider-page scope.
Summary
Migrates 16 native-integration pages from V2 to V3 components.
- Provider connection pages now use V3 cards and fields.
- Provider setup pages now use V3 cards and fields.
- Saved and new connections appear as separate choices in a V3 card.
No actionable issues were found. Source review and focused browser checks support merging within the stated provider-page scope. Browser coverage used actual page components with dummy local data and controlled responses; it did not exercise authenticated backend flows or real providers.
Ack … the rest (519 characters)nowledged scope from `PierreOucif`: direct-URL pages remain intentionally available until native integrations retire on August 19, 2027; existing copy and the Select Connection empty-list OAuth redirect are intentionally unchanged. Shared path-input ref forwarding and blur limitations are acknowledged as existing behavior and intentionally unchanged; no shared path-input fix is required for this migration. Reviews (1) · Last reviewed commit: "improvement(integrations): port native p..." · Reviewed by the reviewer |
| #8620 2026-10-20 17:53 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
MFA confirmation now manages loading state around login completion. The MFA success callback returns the organization-selection promise when stored organization context exists, and returns `undefined` when it does not.
**Changes: MFA completion**
|Layer / File(s)|Summary|
|:---|:---|
|**MFA completion callbacks** `frontend/src/components/auth/Mfa.tsx`, `frontend/src/pages/auth/SelectOrgPage/SelectOrgPage.tsx`|The confirmation handler sets loading state before `completeLogin` and clears it in a `finally` block. The MFA success callback returns the organization-selection result when stored MFA organization contex … the rest (3889 characters)t exists; otherwise, it returns `undefined`.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Bug fix **Suggested reviewers:** `andrewhuhh` Merge Risk: ⚪ Minimal · up to 7a9aa The MFA confirmation retry flow has no established merge-blocking risk. Security architecture risk: ⚪ Minimal · up to 7a9aa The change restores retry after organization-selection failure without repeating MFA verification or replacing recovery codes. The inspected flow retains server-side organization and MFA checks; no material security risk was found to be introduced or worsened. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The changed behavior concerns the current user's completion flow for a selected root or sub-organization. Restoring the retry control does not expand independently attackable organization scope because client-supplied organization IDs remain subject to server-side membership checks on each request. **Security Findings and Attack Paths** - _inferred_ — No introduced MFA or organization-authorization bypass was identified in the inspected path. Bypassing the UI pending state still reaches the existing authenticated selection endpoint; returning its promise does not itself grant authority or mint credentials. **Trust Boundaries and Controls** - _observed_ — MFA assurance originates in a server-issued verified session token. Organization selection re-evaluates organization and MFA-method changes when deciding whether another challenge is required; the frontend loading flag is not the authorization control. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title uses the conventional commit format and clearly describes the main change: allowing users to retry after MFA organization selection fails. | | Description check | | The description is complete and matches the repository template. It explains the problem and solution, provides verification steps, identifies the change as a fix, documents testing and limitations, a… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description is complete and matches the repository template. It explains the problem and solution, provides verification steps, identifies the change as a fix, documents testing and limitations, and completes the checklist. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 2 - Waived: $0.50 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8620 2026-10-20 17:53 | cubic review-body | — | summary | **No issues found** across 2 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8620 2026-10-20 18:06 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** Safe to merge within the verified scope; no actionable defects were found in the changed confirmation and selection callbacks.
Summary
This PR lets users retry organization selection after first-time MFA without repeating verification.
- MFA users can retry organization selection after it fails.
No blocking issues were found; the change is safe to merge within the verified scope. These checks used the real recovery components with controlled callbacks. Authenticated first-time MFA against a licensed backend remains unverified.
Reviews (1) · Last reviewed commit: "fix(auth): release recovery-code confirm..." · Reviewed by the reviewer |
| #8621 2026-10-20 18:08 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Combobox creation now supports dialog, sheet, and action modes. The single- and multi-select flows share form overlay handling. New Storybook examples demonstrate the creation workflows, and the migration guide documents their contracts.
**Changes: Combobox creation modes**
|Layer / File(s)|Summary|
|:---|:---|
|**Creation contracts and shared overlays** `frontend/src/components/v3/generic/Combobox/Combobox.tsx`, `frontend/src/components/v3/generic/Combobox/index.ts`, `frontend/src/components/V3_MIGRATION.md`|Added shared form render props and sheet and action configuration types. Dialog and sheet forms use sha … the rest (5520 characters)red overlay state and dismissal handling. Updated the public exports and migration guidance.| |**Selection flows and creation examples** `frontend/src/components/v3/generic/Combobox/Combobox.tsx`, `frontend/src/components/v3/generic/Combobox/Combobox.creation.stories.tsx`|Single- and multi-select flows route action creation through `onAction` and open shared form overlays for dialog and sheet modes. Storybook examples demonstrate sheet, dialog, and action workflows.| **Priority:** ⬇️ Low **Estimated code review effort:** 3 (Moderate) | ~25 minutes **Change:** Feature ### Sequence Diagram(s) ```mermaid sequenceDiagram participant User participant Combobox participant CreationOverlay participant Form participant Caller User->>Combobox: Select a form creation option Combobox->>CreationOverlay: Open dialog or sheet CreationOverlay->>Form: Render with initialInput, complete, and cancel Form->>Caller: Submit form data Caller->>Combobox: Update options and selection, then complete ``` **Suggested reviewers:** `andrewhuhh` Merge Risk: ⚪ Minimal · up to 2d012 This change adds Sheet-based and caller-owned action creation modes to the Combobox, along with Storybook examples and migration guidance. Existing inline and dialog behavior is reported unchanged. No concrete defects were found, so the change is ready to merge. The authenticated PKI creation and policy sub-wizard flows are reported as untested and are worth checking when the PKI callers adopt these modes. Security architecture risk: 🔵 Low · up to 2d012 The change is limited to shared UI creation workflows. Persistence and selection remain caller-owned, and the inspected dispatch paths do not establish a new security vulnerability. Action workflows must supply their own validation and pending-work controls; production integrations were not verified. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The demonstrated exposure is a shared frontend callback contract carrying user-entered query text. Tenant, asset, service, or datastore consequences depend on downstream integrations; no such privileged integration was verified in the inspected scope. **Trust Boundaries and Controls** - _observed_ — Action dispatch checks isDisabled in both selection variants. This is a UI invocation control, not a demonstrated authorization boundary. Action intentionally delegates validation, pending work, and eventual persistence to its caller. - _observed_ — Form opening rejects disabled or pending configurations, cancellation rejects pending work, and dismissal can pass through caller-owned discard interception. Sheet receives the same pending state and connects existing dismissal guards, disabled close behavior, and focus-return callbacks. **Resilience and Maintainability Implications** - _observed_ — Completion callbacks lack session identity and at-most-once guards, so retained callbacks can close an overlay after cancellation or reopening. This behavior predates the PR in dialog creation and is reused for Sheet. The inspected completion sinks affect search, popup state, and focus rather than persisting or selecting domain objects; a worsened security outcome was not established. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description check | | The description is complete and aligned with the template. It explains the context, documents UI changes with screenshots, lists verification steps, identifies the change type, and includes the checkl… | | Title check | | The title clearly and concisely describes the main change: adding Sheet and in-place Combobox creation support. It follows the conventional commit format. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description is complete and aligned with the template. It explains the context, documents UI changes with screenshots, lists verification steps, identifies the change type, and includes the checklist. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 4 - Waived: $1.00 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8621 2026-10-20 18:15 | cubic review-body | — | summary | **No issues found** across 4 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8621 2026-10-20 18:21 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** Safe to merge within the shared Combobox scope; no blocking issues were established.
Summary
Adds Sheet-backed creation forms and caller-owned, in-place creation actions to `Combobox`, along with composition stories, migration guidance, and public type exports.
Both Sheet compositions passed rendered lifecycle checks: dismissing a nested popup left the creation Sheet open, Keep Editing preserved the draft, Discard restored the parent query and focus, and successful creation did not submit the parent attachment form. Action creation passed all 32 combinations of single/multiple selection, query type, pointer/keyboard activation, and enabled/disabl … the rest (848 characters)ed state. Enabled callbacks received the expected trimmed query, and disabled actions did not invoke them. The established protections remain intact: pending saves block Sheet dismissal, and starting an action preserves selected options. No blocking issues were found. This change is safe to merge within its shared-component scope. Scope acknowledgment: PierreOucif explicitly notes that no new test suite was committed and that authenticated PKI creation and the policy sub-wizard remain outside this change. The extracted real form and domain workflows still require validation before that migration can be considered ready. - Combobox creation forms can open in a Sheet. - The Create row can start a caller-owned workflow in place. Reviews (1) · Last reviewed commit: "improvement(ui): support Sheet and in-pl..." · Reviewed by the reviewer |
| #8623 2026-10-20 18:23 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Azure ADCS and Venafi signing-configuration forms now use independently scrollable content containers with separately positioned footers. Their fields and submission behavior remain unchanged.
**Changes: Signing form layout**
|Layer / File(s)|Summary|
|:---|:---|
|**Scrollable signing forms** `frontend/src/pages/cert-manager/CertAuthDetailsByIDPage/components/CaSigningConfigSection.tsx`|Azure ADCS and Venafi fields are in padded, independently scrollable containers. Each form’s footer sits outside its scrollable area. Field and submission behavior remain unchanged.|
**Priority:** ⬇️ Low
**Estimated code revie … the rest (2102 characters)w effort:** 2 (Simple) | ~10 minutes **Change:** Bug fix **Suggested reviewers:** `andrewhuhh` Merge Risk: ⚪ Minimal · up to 9a6db The layout change has no verified merge-blocking risk in the supplied evidence. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the primary change: anchoring signing configuration sheet footers. It follows the conventional commit format. | | Description check | | The description is complete and relevant. It explains the prior and new behavior, includes UI screenshots, verification steps, change type, checklist status, test results, and known verification limit… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description is complete and relevant. It explains the prior and new behavior, includes UI screenshots, verification steps, change type, checklist status, test results, and known verification limits. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8623 2026-10-20 18:28 | cubic review-body | — | summary | **No issues found** across 1 file
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8623 2026-10-20 18:32 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** Safe to merge. Both signing forms retain visible action footers, reachable fields, and working native form submission in the tested short viewports.
Summary
Moves scrolling into a separate body inside the Azure ADCS and Venafi signing forms, keeping Cancel and Save outside the scrolling body and inside the same form.
Browser checks of both providers at 1000×320 and 600×240 confirmed that the footer remains visible while the body scrolls, all fields remain reachable, and Save submits the correct form. These checks mounted the real changed component and shared UI primitives with synthetic domain data and persistence callbacks; connected-provider behav … the rest (640 characters)ior and authenticated authorization acceptance were not tested. Focused lint and frontend typechecking passed. PierreOucif explicitly identified the footer alignment change as intentional: removing the negative-margin/alignment compensation restores default SheetFooter styling. Submit handlers, field values, reset logic, pending guards, and Native ADCS remain unchanged. No actionable issues were found; the scoped layout change is safe to merge. - Azure ADCS and Venafi sheets keep their actions visible while fields scroll. Reviews (1) · Last reviewed commit: "fix(cert-manager): anchor signing config..." · Reviewed by the reviewer |
| #8624 2026-10-20 18:34 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The identity modal now uses `DialogBody` and `DialogFooter` to structure its form. The name input no longer uses autofocus. Existing role, delete-protection, and metadata controls retain their behavior.
**Changes: Identity modal layout**
|Layer / File(s)|Summary|
|:---|:---|
|**Structure the identity modal form** `frontend/src/pages/organization/AccessManagementPage/components/OrgIdentityTab/components/IdentitySection/OrgIdentityModal.tsx`|The form fields are placed in `DialogBody`, and the action buttons are placed in `DialogFooter`. The name input no longer uses autofocus. Existing control behavior remains un … the rest (2015 characters)changed.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Bug fix **Suggested reviewers:** `andrewhuhh` Merge Risk: ⚪ Minimal · up to e672e The identity modal now keeps its action buttons in a footer and scrolls its fields in the body, and the name field no longer takes focus automatically. No concrete merge-blocking risk was found. Normal runtime verification of the update flow is still advisable. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly identifies the UI change and follows the conventional commit format. | | Description check | | The description includes context, screenshots, verification steps, change type, checklist status, test results, and known limitations. It clearly identifies the in-scope and out-of-scope behavior. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8624 2026-10-20 18:40 | cubic inline · P3 | frontend/src/pages/organization/AccessManagementPage/components/OrgIdentityTab/components/IdentitySection/OrgIdentityModal.tsx:152 | defect-functional | The `DialogBody` now has `overflow-y-auto` and no padding, so the 3px focus ring on the full-width inputs is clipped at the top, left, and right edges. Add a small inset padding to the body so the ring stays visible. |
| #8624 2026-10-20 18:40 | cubic review-body | — | summary | **1 issue found** across 1 file
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8624 2026-10-20 18:44 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** The change is safe to merge within its stated composition-only scope; the tested rendering preserves contained scrolling and native form behavior.
Summary
The PR puts the identity edit fields inside DialogBody and the action buttons inside DialogFooter. Cancel now uses ghost and still precedes Update. Removing input autoFocus lets the dialog choose initial focus; submission, validation, reset, and license-check code remain unchanged.
Browser checks using the actual child form, dialog primitives, application styles, and React Hook Form found no actionable regression in the tested composition. With 24 metadata rows at short viewport heights, the body … the rest (1055 characters)scrolled while the header and footer remained visible. Cancel did not submit; Update and Enter submitted expected values; invalid fields received focus; and Update remained disabled during submission. These checks used deterministic organization, subscription, and role fixtures, an asynchronous update sink, and a closed UpgradeGate stub. They do not establish authenticated persistence or normal-runtime permission and license behavior. Acknowledged scope limits: PierreOucif explicitly identified the inherited role-picker Escape behavior as outside this change; it remains neither fixed nor accepted. PierreOucif also identified missing parent pending-dismiss locks as an inherited limitation, with pending-state propagation outside this bounded change. Neither is reported as a new defect or treated as fixed. No blocking issues were found in the reviewed change. - Machine identity fields scroll while the dialog actions stay together. Reviews (1) · Last reviewed commit: "fix(ui): compose machine identity dialog..." · Reviewed by the reviewer |
| #8625 2026-10-20 18:47 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
PamDetailSheet now has independently scrollable sidebar and content regions with separate footer slots. PAM approval, access-request, and discovery sheets place their existing action controls in those slots.
**Changes: PAM detail sheet layout**
|Layer / File(s)|Summary|
|:---|:---|
|**Detail sheet structure and footer slots** `frontend/src/pages/pam/components/PamDetailSheet.tsx`|PamDetailSheet adds accessible sheet headers, separate scrolling regions, and dedicated sidebar and content footer props.|
**PAM action placement**
|Layer / File(s)|Summary|
|:---|:---|
|**Approval, access-request, and discovery acti … the rest (3865 characters)ons** `frontend/src/pages/pam/PamApprovalRequestsPage/components/ApprovalRequestDetailSheet.tsx`, `frontend/src/pages/pam/components/RequestAccessSheet.tsx`, `frontend/src/pages/pam/PamDiscoveryPage/components/DiscoverySourceDetailSheet.tsx`|Approval and access-request controls move to the content footer. The discovery scan button moves to the sidebar footer. Existing action behavior is preserved.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~12 minutes **Change:** Bug fix **Suggested reviewers:** `lb-vn` Merge Risk: ⚪ Minimal · up to ad73f This change anchors PAM detail sheet headers and action footers while keeping the existing action behavior. No concrete merge-blocking risk was identified. Security architecture risk: ⚪ Minimal · up to ad73f The change reorganizes existing PAM controls without introducing new privileges or changing their conditions, callbacks, or submitted data. The relocated submit button remains associated with its validated form. No material security risk was found to be introduced or worsened. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The changed exposure is the placement and accessibility of controls within existing PAM sheets. The comparison does not add callers, credentials, resource targets, or mutation authority. Shared-shell layout affects its consumers, but privileged actions remain supplied by those consumers. **Trust Boundaries and Controls** - _observed_ — Existing frontend controls survive relocation: review actions remain status-gated, Approve remains disabled for the requester's own request, revoke retains confirmation, Scan Now retains missing-source and mutation-pending controls, and request submission remains absent in the business-pending view. These observations do not independently verify server-side authorization. **Resilience and Maintainability Implications** - _observed_ — PamDetailSheet still does not pass mutation-pending state into SheetContent, so sheet-level pending-dismiss locking remains absent. This condition predates the PR; the comparison preserves existing dismissal and mutation paths rather than establishing cancellation or stronger concurrency guarantees. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title uses the conventional commit format and clearly summarizes the main change: anchoring PAM detail-sheet headers and action footers. | | Description check | | The description includes the required context, screenshots, verification steps, type selection, and checklist. It also documents testing results and known limitations. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8625 2026-10-20 18:54 | cubic inline · P3 | frontend/src/pages/pam/PamApprovalRequestsPage/components/ApprovalRequestDetailSheet.tsx:147 | defect-functional | These actions now align to the left because `SheetFooter` does not apply `justify-end`, while the removed row did. Preserve the existing right alignment by adding a full-width, right-aligned wrapper or applying `justify-end` to the footer. |
| #8625 2026-10-20 18:54 | cubic inline · P3 | frontend/src/pages/pam/components/PamDetailSheet.tsx:156 | defect-functional | The sidebar title loses its previous `text-lg` styling because `SheetTitle` supplies no font size. Add `text-lg` to preserve the existing title hierarchy.
```suggestion
<SheetTitle className="text-lg">{title || "PAM Details"}</SheetTitle>
``` |
| #8625 2026-10-20 18:54 | cubic review-body | — | summary | **2 issues found** across 4 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8625 2026-10-20 18:58 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No blocking issue was identified in the changed composition; authenticated product behavior remains unverified.
Summary
Updates PAM detail sheets to keep headers and action footers separate from scrolling content.
- PAM detail sheets separate headings and actions from scrolling details.
- Access request buttons stay available outside the scrolling form.
Browser checks using source-attributed components and real UI primitives confirmed reachable scrolling content and visible footer actions at normal and short viewport heights, including long wrapped titles. Accessible dialog titles and optional descriptions behaved correctly. Access-request checks … the rest (1057 characters)confirmed required-field validation and focus, textarea newline behavior, exactly-once submission, distinct form IDs, and Cancel behavior. These checks used bounded synthetic renders, not an authenticated product session. API and metadata boundaries were stubbed for access-request submission; authenticated mutations and permissions were not verified. Acknowledged limits from PierreOucif: nested Select Escape behavior remains unresolved and is not fixed or accepted here; pending-dismiss locks remain absent because mutation pending state is not propagated into SheetContent; authenticated mutation and permission checks remain outside the supplied verification. PierreOucif described these as inherited limits outside this composition change, with picker/primitives, save-bar tab editors, and pending-state propagation outside its bounded scope. No actionable issues were found. No blocking issue was identified in the changed composition. Reviews (1) · Last reviewed commit: "fix(pam): anchor detail sheet headers an..." · Reviewed by the reviewer |
| #8626 2026-10-20 19:02 | coderabbit issue-comment | — | summary | 📝 Walkthrough
Several forms now use `SheetFooter` for their action areas. The dynamic-secret form header sits outside the scrollable content, and the selected-provider header uses default styling.
**Changes: Sheet Form Layout**
|Layer / File(s)|Summary|
|:---|:---|
|**SheetFooter adoption** `frontend/src/components/dynamic-secrets/DynamicSecretProviderForm/DynamicSecretProviderForm.tsx`, `frontend/src/components/pki-syncs/forms/*`, `frontend/src/components/proxied-services/forms/*`, `frontend/src/components/secret-rotations-v2/forms/SecretRotationV2Form.tsx`, `frontend/src/components/secret-syncs/forms/*`|These forms use `SheetFooter` in place of custom footer wrappers. The PKI sync and … the rest (2451 characters)secret sync edit forms place Save with the right-side navigation controls.| |**Dynamic-secret header layout** `frontend/src/components/dynamic-secrets/DynamicSecretProviderForm/DynamicSecretProviderForm.tsx`, `frontend/src/pages/secret-manager/OverviewPage/components/CreateDynamicSecretForm/CreateDynamicSecretForm.tsx`|The dynamic-secret form header is outside the scrollable content. The selected-provider `SheetHeader` no longer uses negative margins.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Bug fix **Suggested reviewers:** `andrewhuhh` Merge Risk: 🔵 Low · up to 7b604 On sufficiently narrow screens, dynamic-secret users may be unable to reach the rightmost footer action; restore responsive stacking before or immediately after merge. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title uses conventional commit format and clearly identifies the primary UI change: replacing custom footer markup with canonical Sheet footers in reusable wizard forms. | | Description check | | The description includes the required Context, Screenshots, Steps to verify the change, Type, and Checklist sections. It explains the scope, verification performed, and known verification limits. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 9… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 9 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8626 2026-10-20 19:04 | cubic inline · P2 | frontend/src/components/dynamic-secrets/DynamicSecretProviderForm/DynamicSecretProviderForm.tsx:460 | defect-functional | This footer now keeps Back, Cancel, and the provider submit button in one non-wrapping row on narrow sheets. Restore the responsive column layout so long labels do not overflow or compress the primary action.
```suggestion
<SheetFooter className="shrink-0 flex-col-reverse items-stretch border-t @sm:flex-row @sm:items-center @sm:justify-end">
``` |
| #8626 2026-10-20 19:04 | cubic review-body | — | summary | **1 issue found** across 9 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8626 2026-10-20 19:10 | coderabbit inline · P2 | frontend/src/components/dynamic-secrets/DynamicSecretProviderForm/DynamicSecretProviderForm.tsx:477 | defect-functional | **Restore a stacked footer at narrow widths.**
`CreateDynamicSecretForm` reaches this footer with Back, Cancel, and Submit. The form Sheet is full width at narrow viewports, but the footer is a non-wrapping row. Each button is `shrink-0` and `whitespace-nowrap`, so the row can exceed the available width. The rightmost action can then extend outside the Sheet viewport.
Suggested fix
```diff
--- "a/frontend/src/components/dynamic-secrets/DynamicSecretProviderForm/DynamicSecretProviderForm.tsx"
+++ "b/frontend/src/components/dynamic-secrets/DynamicSecretProviderForm/DynamicSecretProviderForm.tsx"
@@ -457,11 +457,17 @@
</section>
</div>
- <SheetFooter className="s … the rest (1747 characters)hrink-0 border-t"> + <SheetFooter className="shrink-0 border-t flex-col-reverse @sm:flex-row @sm:justify-end"> {onBack && ( - <Button type="button" variant="ghost" onClick={onBack} isDisabled={isPending}> + <Button + type="button" + variant="ghost" + onClick={onBack} + isDisabled={isPending} + className="@sm:mr-auto" + > Back </Button> )} <Button type="button" variant="ghost" onClick={onCancel} isDisabled={isPending}> Cancel ``` 📝 Committable suggestion > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion <SheetFooter className="shrink-0 border-t flex-col-reverse @sm:flex-row @sm:justify-end"> {onBack && ( <Button type="button" variant="ghost" onClick={onBack} isDisabled={isPending} className="@sm:mr-auto" > Back </Button> )} <Button type="button" variant="ghost" onClick={onCancel} isDisabled={isPending}> Cancel </Button> <Button type="submit" variant="project" isPending={isPending} isDisabled={providerSubmitState.isDisabled || isPending} > {mode === "create" ? definition.create.submitLabel : definition.edit.submitLabel} </Button> </SheetFooter> ``` |
| #8626 2026-10-20 19:10 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8626 2026-10-20 19:12 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** No identified defect blocks merging; source inspection supports preserved form ownership and button behavior, but rendered acceptance remains incomplete.
Summary
Replaces custom footer containers with `SheetFooter` across eight reusable forms.
- Reusable wizard forms keep their actions in the shared sheet footer.
- Edit sync forms put Save after Cancel and step navigation.
- Dynamic-secret setup keeps its provider header above the scrolling fields.
No actionable issues were established. Source inspection supports the composition-only change, and no identified defect blocks merging. Rendered behavior and provider submissions remain unverified.
Ackno … the rest (877 characters)wledged limitation: PierreOucif explicitly preserves dismissal while a request is pending because the outer hosts do not pass `SheetContent.isPending`. This existing limitation is not reported as a new issue. ### the check validation blocked The dynamic-secret browser check encountered a React-refresh initialization error and exhausted its execution budget before the correction could be retested. The edit-sync browser check could not render because its synthetic harness omitted the `GcpCertificateManagerScope` export required by the PKI schema. No keyboard, submission, or short-height layout assertions completed. Evidence uploads were unavailable because no artifact-upload operation, authenticated upload endpoint, or documented upload command was supplied. Reviews (1) · Last reviewed commit: "fix(ui): use canonical Sheet footers in ..." · Reviewed by the reviewer |
| #8627 2026-10-20 19:14 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Nine certificate manager views now use the shared `SheetFooter` component instead of manually styled footer wrappers. Existing controls and actions remain in place.
**Changes: Certificate manager footer adoption**
|Layer / File(s)|Summary|
|:---|:---|
|**Replace manual footer wrappers** `frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/CreateCaWizard/CreateCaWizard.tsx`, `frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/ExternalCaModal.tsx`, `frontend/src/pages/cert-manager/CodeSigningPage/components/CreateSignerWizard/CreateSignerWizard.tsx`, `frontend/src/pages/cert-m … the rest (2980 characters)anager/PoliciesPage/components/CertificatePoliciesTab/CreatePolicyModal.tsx`, `frontend/src/pages/cert-manager/SettingsPage/components/HsmConnectorsTab/CreateHsmConnectorWizard/CreateHsmConnectorWizard.tsx`, `frontend/src/pages/cert-manager/SettingsPage/components/HsmConnectorsTab/EditHsmConnectorSheet.tsx`, `frontend/src/pages/cert-manager/SignerDetailPage/components/EditSignerModal/EditSignerModal.tsx`, `frontend/src/pages/cert-manager/SignerDetailPage/components/EditSignerPolicyModal/EditSignerPolicyModal.tsx`, `frontend/src/pages/cert-manager/components/CertificateAlerts/CertificateAlertSheet.tsx`|These views replace manual footer wrappers with `SheetFooter`. Their existing navigation, form actions, indicators, and related footer content remain in place.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Refactor **Suggested reviewers:** `carlosmonastyrski` Merge Risk: ⚪ Minimal · up to c80d1 This is a localized footer-wrapper adoption with no established behavioral regression, so it is mergeable. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: replacing manual footers with canonical Sheet footers in Certificate Manager. | | Description check | | The description includes all required sections, explains the scope and behavior, provides screenshots and verification steps, identifies the change type, and completes the checklist. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 9… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 9 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 9 - Waived: $2.25 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8627 2026-10-20 19:15 | cubic inline · P2 | frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/ExternalCaModal.tsx:1138 | defect-functional | This footer loses its original `px-6` and `gap-3`, so the buttons sit 16px from the edge instead of 24px and the gap shrinks to 8px. Add `px-6 gap-3` to the className to keep the existing layout, as CertificateWizardSheet does.
```suggestion
<SheetFooter className="shrink-0 items-center justify-end gap-3 border-t px-6">
``` |
| #8627 2026-10-20 19:15 | cubic inline · P2 | frontend/src/pages/cert-manager/CodeSigningPage/components/CreateSignerWizard/CreateSignerWizard.tsx:474 | defect-functional | The footer's original `px-6 py-4` and `gap-3` were dropped in this swap, and `SheetFooter` defaults to `p-4 gap-2`. Footer padding shrinks from 24px to 16px horizontally, and the gap between the status text and the buttons shrinks too. Restore the original spacing classes on the `SheetFooter` so the footer keeps its layout.
```suggestion
<SheetFooter className="shrink-0 items-center justify-between gap-3 border-t px-6 py-4">
``` |
| #8627 2026-10-20 19:15 | cubic inline · P2 | frontend/src/pages/cert-manager/SignerDetailPage/components/EditSignerPolicyModal/EditSignerPolicyModal.tsx:310 | defect-functional | `SheetFooter` brings its own `gap-2 p-4`, so the action row loses its original `gap-3` and `px-6` spacing. Restore those values in the `className` to keep the footer layout unchanged.
```suggestion
<SheetFooter className="shrink-0 items-center justify-between gap-3 border-t px-6">
``` |
| #8627 2026-10-20 19:15 | cubic review-body | — | summary | **4 issues found** across 9 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8627 2026-10-20 19:15 | cubic inline · P3 | frontend/src/pages/cert-manager/components/CertificateAlerts/CertificateAlertSheet.tsx:248 | defect-functional | The footer's horizontal padding drops from `px-6` to `p-4` and the button gap drops from `gap-3` to `gap-2`, because the `SheetFooter` defaults now apply. The PR description says the change preserves existing layout, so either note this spacing change or keep the original values by adding them to `className`.
```suggestion
<SheetFooter className="shrink-0 items-center justify-between gap-3 border-t px-6 py-4">
``` |
| #8627 2026-10-20 19:24 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** No blocking findings identified; the footer substitutions preserve the existing form placement and action handlers, although independent browser checks remain incomplete.
Summary
Replaces manual footer wrappers with `SheetFooter` in nine Certificate Manager forms and wizards.
- Certificate Manager forms now use the sheet’s shared footer.
Acknowledged limits:
- PierreOucif explicitly identifies CreateSigner’s compatibility CA `FilterableSelect` Escape behavior and the existing pending-dismiss/Back/Stepper gaps as unchanged and out of scope.
- PierreOucif states that HSM Credentials/final-create cannot be reached without a PKCS#11 Gateway and does no … the rest (589 characters)t claim HSM Verify, external issuance, signer save/reissue, alert delivery, OAuth, or simulated mutation-response acceptance. These capabilities are unchanged by the footer substitutions. ### the check validation blocked Independent browser checks could not complete because Vite could not resolve React from the isolated rendering harness. Short-viewport layout, native submission, Cancel, and read-only Close behavior were therefore not independently verified by these checks. Reviews (1) · Last reviewed commit: "fix(cert-manager): use canonical Sheet f..." · Reviewed by the reviewer |
| #8630 2026-10-20 19:26 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The changes remove the `NoticeBannerV2` component and its export, and remove the `AdminTable` and `AdminV3Adapters` modules. The lifecycle and migration records are updated to document the `NoticeBannerV2` removal.
**Changes: NoticeBannerV2 removal**
|Layer / File(s)|Summary|
|:---|:---|
|**Remove NoticeBannerV2 and update records** `frontend/src/components/v2/NoticeBannerV2/NoticeBannerV2.tsx`, `frontend/src/components/v2/index.tsx`, `frontend/src/components/COMPONENT_LIFECYCLE.md`, `frontend/src/components/V3_MIGRATION.md`|The component implementation and barrel export are removed. The lifecycle and migration records now document the removal.|
**Admin component module re … the rest (3794 characters)movals** |Layer / File(s)|Summary| |:---|:---| |**Remove admin table and adapter modules** `frontend/src/pages/admin/components/AdminTable.tsx`, `frontend/src/pages/admin/components/AdminV3Adapters.tsx`|The `AdminTable` module and the `AdminV3Adapters` module, including their exported components and aliases, are removed.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Other **Suggested reviewers:** `andrewhuhh` Merge Risk: 🔵 Low · up to a1fb4 The code removals have no identified consumers, but the migration ledger should be corrected before merge. Architecture risk: 🔵 Low · up to a1fb4 The change affects 1 system. **Changed systems:** `frontend` **Architecture concerns** No architecture-level concerns identified. Review details **Systems and components** - _observed_ — frontend (ui) was modified; 6 changed files map to changed impact. **Before / after behavior** - _observed_ — Modified behavior in frontend/src/components/COMPONENT_LIFECYCLE.md: `NoticeBannerV2` changed from blocked pending V3 `Alert` verification to removed, with its implementation and export surfaces documented as deleted after no consumers were found. - _observed_ — Modified behavior in frontend/src/components/V3_MIGRATION.md: The `NoticeBannerV2` coverage row now documents removal of the unused implementation, family index, and root export instead of planned migration to V3 Alert. - _observed_ — Modified behavior in frontend/src/components/v2/NoticeBannerV2/NoticeBannerV2.tsx: The `NoticeBannerV2` component and its imports, props type, styling, info icon, title rendering, and child-content rendering were removed. - _observed_ — Modified behavior in frontend/src/components/v2/index.tsx: The `NoticeBannerV2` re-export was removed from the component barrel file. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main changes: deleting the orphan NoticeBanner family and unused admin adapters. It follows the conventional commit format. | | Description check | | The description is complete and relevant. It explains the context, lists the removed sources, states that screenshots are not applicable, provides verification steps, identifies the change as a chore,… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description is complete and relevant. It explains the context, lists the removed sources, states that screenshots are not applicable, provides verification steps, identifies the change as a chore, and includes the required checklist items. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8630 2026-10-20 19:31 | coderabbit inline · P2 | frontend/src/components/V3_MIGRATION.md:62 | docs | **Reconcile the `NoticeBannerV2` consumer count.**
This row reports `2 / 0` consumers, but the deletion proof says no incoming implementation edge or external symbol reference remained. `COMPONENT_LIFECYCLE.md` also records no consumers at the same commit. Set the count to `0 / 0`, or identify and explain the two historical files. Otherwise, the ledger provides contradictory deletion evidence. |
| #8630 2026-10-20 19:31 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8630 2026-10-20 19:32 | cubic review-body | — | summary | **No issues found** across 7 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8630 2026-10-20 19:37 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** No blocking issues were identified. Executed checks support that the deleted modules have no surviving consumers and that removing the empty family export leaves the public V2 exports unchanged.
Summary
Removes unused UI code without changing consumers:
- The V2 catalog no longer exposes the unused NoticeBannerV2 banner.
- The admin area drops its unused V3 adapter files.
No actionable issues were found. Before-and-after reference and module-resolution checks found no surviving consumers of the deleted modules and confirmed that the public V2 barrel exports remain unchanged. Vite transformation and import-resolution checks passed for the barrel and … the rest (699 characters)production dynamic-import callers and targets. The full frontend typecheck passed. The prior whitespace check also passed; a production build and browser tests were not run. Acknowledged by PierreOucif: the global `--assert-zero` gate still exits 1 because live legacy families remain. This is an intentional partial migration, not completion of the migration or establishment of a new workflow acceptance gate. No blocking issues were identified. The checked removals are safe to merge from a code-review perspective; merge authorization remains pending as stated in the PR description. Reviews (1) · Last reviewed commit: "chore(ui): delete orphan notice family a..." · Reviewed by the reviewer |
| #8646 2026-10-20 19:38 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The signing and verification modals now use v3 Dialog components. Each modal owns the operation state and passes it to its form. Dirty unsent input triggers a discard confirmation when a close is requested. The forms use DialogBody and DialogFooter.
**Changes: KMS signing and verification dialogs**
|Layer / File(s)|Summary|
|:---|:---|
|**Signing form and discard flow** `frontend/src/pages/kms/OverviewPage/components/CmekSignModal.tsx`|`CmekSignModal` owns the signing state and passes it to `SignForm`. The form resets signing results and reports dirty unsent input. Dirty close requests open a discard confirmati … the rest (4654 characters)on; clean or completed forms close directly. The form and footer submit action use the v3 dialog layout.| |**Verification form and discard flow** `frontend/src/pages/kms/OverviewPage/components/CmekVerifyModal.tsx`|`CmekVerifyModal` owns the verification state and passes it to `VerifyForm`. The form resets verification results and reports dirty unsent input. Dirty close requests open a discard confirmation. Verification results and input fields remain conditionally displayed in the v3 dialog layout.| **Priority:** ⬇️ Low **Estimated code review effort:** 3 (Moderate) | ~20 minutes **Change:** Feature ### Sequence Diagram(s) ```mermaid sequenceDiagram participant SignForm participant CmekSignModal participant useDiscardChangesGuard participant DiscardChangesAlertDialog SignForm->>CmekSignModal: Report dirty unsent input CmekSignModal->>useDiscardChangesGuard: Check close request useDiscardChangesGuard->>DiscardChangesAlertDialog: Open discard confirmation DiscardChangesAlertDialog->>CmekSignModal: Confirm discard CmekSignModal->>CmekSignModal: Close dialog ``` ```mermaid sequenceDiagram participant VerifyForm participant CmekVerifyModal participant useDiscardChangesGuard participant DiscardChangesAlertDialog VerifyForm->>CmekVerifyModal: Report dirty unsent input CmekVerifyModal->>useDiscardChangesGuard: Check close request useDiscardChangesGuard->>DiscardChangesAlertDialog: Open discard confirmation DiscardChangesAlertDialog->>CmekVerifyModal: Confirm discard CmekVerifyModal->>CmekVerifyModal: Close dialog ``` **Suggested reviewers:** `bernie-g` Merge Risk: ⚪ Minimal · up to 04358 The dialog migration has no established merge-blocking issue after normal checks. The identified text and display issues can be addressed separately. Security architecture risk: 🔵 Low · up to 04358 The change is confined to signing and verification dialogs. Request formats and server-side permission checks remain unchanged, and the new controls protect pending operations and unsent input. No introduced security issue was established, but interruption and rapid-reopen behavior were not fully verified. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — Each request targets one selected key, while effective authority remains the caller's existing project-level Sign or Verify permission over customer-managed keys. Moving browser mutation ownership does not itself grant additional tenant, project, or key authority. **Trust Boundaries and Controls** - _observed_ — Browser-controlled key IDs, data, signatures, and algorithms cross an authenticated API boundary. The examined routes require authentication, UUID key IDs, enumerated algorithms, Base64 validation, payload limits, and rate limiting. Before KMS invocation, the service rejects missing, reserved, non-customer-managed, or disabled keys and requires the corresponding permission in the key's project. These controls predate this PR. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the primary change: migrating KMS signing dialogs to V3 dialog shells. It also follows the conventional commit format. | | Description check | | The description includes all required sections, explains the context and scope, provides screenshots, verification steps, change type, checklist status, and testing details. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8646 2026-10-20 19:46 | cubic review-body | — | summary | **No issues found** across 2 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8646 2026-10-20 19:52 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No blocking defect was identified; the available evidence supports merging, with the incomplete algorithm-popup interaction check noted above.
Summary
Moves the KMS Sign and Verify dialogs to the V3 header, body, and footer composition.
- The Sign Data dialog uses the V3 shell and guards unsent input.
- The Verify Signature dialog uses the V3 shell and guards unsent input.
No actionable issues or merge-blocking defects were identified in the reviewed changes.
Acknowledged by PierreOucif: inherited Controller ref/onBlur omissions and the `decodeBase64(data).toString()` preview remain unchanged. Temporary toast overlap with a short footer is also … the rest (552 characters)documented by PierreOucif as known; normal pointer departure restores its timeout. ### the check validation blocked Independent algorithm-popup interaction checks did not complete. The disposable browser harness encountered TSX transformation and source-import interception problems, and completed results were not collected after correction. Popup selection, Escape isolation, and trigger-focus restoration remain independently unverified. Reviews (1) · Last reviewed commit: "improvement(ui): compose KMS signing dia..." · Reviewed by the reviewer |
| #8647 2026-10-20 19:55 | coderabbit issue-comment | — | summary | 📝 Walkthrough
Certificate Manager enrollment and collection dialogs now use v3 platform components. The changes update form validation, accessibility, and pending-state handling, and replace deletion modals with confirmation dialogs.
**Changes: Certificate Manager UI**
|Layer / File(s)|Summary|
|:---|:---|
|**Enrollment form and sheet** `frontend/src/pages/cert-manager/CertificatesPage/components/CertificateTemplateEnrollmentModal.tsx`|The enrollment form now uses v3 sheet components, generated control IDs, inline validation, and pending-aware actions. Reset behavior responds to popup state. The CA-chain controls are hidden and cleared when bootstrap certificate validation is disabled.|
… the rest (4940 characters)|**Add collection item** `frontend/src/pages/cert-manager/PkiCollectionDetailsByIDPage/components/AddPkiCollectionItemModal.tsx`|The add-item dialog now uses v3 form components and requires an item selection. Certificate loading uses the project ID, and the form displays validation and pending states.| |**Deletion dialogs and collection actions** `frontend/src/pages/cert-manager/DiscoveryPage/components/DeleteDiscoveryModal.tsx`, `frontend/src/pages/cert-manager/DiscoveryPage/components/DeleteInstallationModal.tsx`, `frontend/src/pages/cert-manager/PkiCollectionDetailsByIDPage/components/PkiCollectionItemsSection.tsx`|The deletion flows now use `DeleteConfirmDialog`. The collection section also uses the v3 icon button and adds an irreversible-action description.| **Priority:** ⬇️ Low **Estimated code review effort:** 3 (Moderate) | ~20 minutes **Change:** Refactor **Suggested reviewers:** `carlosmonastyrski`, `andrewhuhh` Merge Risk: 🔵 Low · up to 034d6 Assistive-technology users may not be able to identify which selection error belongs to the focused control. This is a bounded accessibility issue that should be fixed, but it does not block the collection workflow. Security architecture risk: 🔵 Low · up to 034d6 The changes remain confined to existing Certificate Manager workflows and preserve the inspected permission checks, mutation targets, secret masking, and pending-action protections. No introduced security weakness was demonstrated. Navigation during an active request and independent server-side enforcement remain unverified. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The inspected frontend reach remains existing certificate-template configuration, project-scoped item discovery, collection membership, and selected discovery or installation records. Resource identities still come from parent selection or current-project context; no new credential, cross-service authority, or broader target selector was identified in these callers. This does not establish backend authorization enforcement. **Trust Boundaries and Controls** - _observed_ — The enrollment entry action retains its parent-owned template Edit permission and EST entitlement checks. Discovery deletion, installation deletion, and collection-item removal retain their respective permission-gated entry actions. Typed confirmation and required selection are interaction safeguards, not substitutes for server authorization. **Resilience and Maintainability Implications** - _observed_ — The new deletion contract requires an exact nonempty confirmation key, tracks the returned promise, disables confirmation and Cancel while pending, and refuses pending dismissal. Form callers pass submitting state into shared overlays and buttons, which disable close controls and submit buttons and guard Escape and outside dismissal. Delete pending state clears on both success and failure, while parent callbacks retain ownership of successful closure. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly identifies the main change: migrating cert-manager PKI overlays to V3. It is concise and follows the conventional commit format. | | Description check | | The description includes all required sections, explains the scope and behavior changes, provides screenshots, verification steps, change type, and a completed checklist. It also documents known verif… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes all required sections, explains the scope and behavior changes, provides screenshots, verification steps, change type, and a completed checklist. It also documents known verification limits. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8647 2026-10-20 20:01 | coderabbit inline · P2 | frontend/src/pages/cert-manager/PkiCollectionDetailsByIDPage/components/AddPkiCollectionItemModal.tsx:135 | defect-functional | **🎯 Functional Correctness** | **🟡 Minor** | **⚡ Quick win**
**Associate each validation error with its select trigger.**
`SelectTrigger` already exposes `aria-invalid` through `isError`. However, `Field` and `FieldError` do not create an error ID or apply `aria-describedby`. A user of assistive technology can receive the alert, but the focused select has no programmatic association with its specific error. Add an error ID and reference it from both triggers.
Suggested fix
```diff
<SelectTrigger
id={itemInputId}
ref={field.ref}
onBlur={field.onBlur}
className="w-full"
… the rest (359 characters)isError={Boolean(error)} + aria-describedby={error ? `${itemInputId}-error` : undefined} > ... - <FieldError>{error?.message}</FieldError> + <FieldError id={`${itemInputId}-error`}>{error?.message}</FieldError> ``` Apply the same change to the certificate selector. |
| #8647 2026-10-20 20:01 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8647 2026-10-20 20:03 | cubic review-body | — | summary | **No issues found** across 5 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8647 2026-10-20 20:10 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No merge-blocking issues were identified within the reviewed scope; focused browser checks support the enrollment and collection flows, subject to the stated limits.
Summary
Migrates five PKI consumers to supported V3 controls and overlays.
- EST enrollment uses a scrollable Sheet with Save and Cancel in its footer.
- Collection users add certificate authorities and certificates through a V3 dialog.
- Collection item removal uses a typed confirmation dialog.
- Discovery and installation deletion use the shared typed-confirmation dialog.
No actionable issues were found. Isolated browser checks using the changed components, real form handling and s … the rest (1046 characters)hared controls passed 19 EST checks covering create/update payloads, passphrase validation and masking, failed-save retention, pending dismissal protection, reset behavior and short-height scrolling. Collection checks passed for both CA and certificate attachment, including required selection, selected-value retention on the first Escape, expected payloads, pending dismissal protection and close/reopen reset. The subsequent parent-closing Escape was checked after popup teardown; back-to-back Escape behavior during teardown remains unverified. These browser checks used fake domain hooks and persistence, not authenticated backend requests. The three removal prompts were not exercised in these checks. Acknowledgments from `PierreOucif`: the backend’s empty-ID error remains outside this change, and authenticated provider/permission checks remain unverified. No merge-blocking issues were identified within the reviewed scope. Reviews (1) · Last reviewed commit: "improvement(cert-manager): migrate clear..." · Reviewed by the reviewer |
| #8633 2026-10-20 20:12 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The share form now uses SecretInput for secret values, limits optional secret names to 50 characters, and displays a count of active sharing constraints. SecretInput supports multiline sizing and alignment.
**Changes: Share secret form**
|Layer / File(s)|Summary|
|:---|:---|
|**Multiline secret input** `frontend/src/components/v3/platform/SecretInput/SecretInput.tsx`|SecretInput accepts `isMultiline`. When enabled for the default variant, it applies a taller minimum height, top alignment, and vertical padding.|
|**Share form behavior** `frontend/src/pages/public/ShareSecretPage/components/ShareSecretForm.tsx`|The form uses SecretInput for the secret value. Supplied values a … the rest (2726 characters)re read-only and show a locked-value description. The optional name is limited to 50 characters. The options trigger displays “No constraints” or the count of active constraints.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Feature **Suggested reviewers:** `adilsitos` Merge Risk: 🔵 Low · up to e3e9c The share form remains usable through other focus paths, but clicking the lower part of the multiline field does not focus it. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title uses conventional commit format and clearly summarizes the main Share a Secret improvements: masked values, name prefilling, and constraint counts. | | Description check | | The description includes context, UI screenshots reference, verification steps, change type, and checklist. It explains the masked value field, read-only stored values, constraint count, and editable … | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes context, UI screenshots reference, verification steps, change type, and checklist. It explains the masked value field, read-only stored values, constraint count, and editable organization-level sharing behavior. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 2 - Waived: $0.50 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8633 2026-10-20 20:14 | cubic inline · P2 | frontend/src/pages/public/ShareSecretPage/components/ShareSecretForm.tsx:228 | defect-functional | The header stops counting specific recipients when external email delivery is enabled, even though those recipients still receive separate links. Count any non-empty recipient list so the constraint total reflects the selected recipients.
```suggestion
!isPublic && parseRecipients(watch("emails")).length > 0
``` |
| #8633 2026-10-20 20:14 | cubic review-body | — | summary | **2 issues found** across 2 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8633 2026-10-20 20:14 | cubic inline · P3 | frontend/src/components/v3/platform/SecretInput/SecretInput.tsx:249 | defect-functional | The multiline shell is taller than its content, but `items-start` leaves the absolutely positioned textarea covering only the one-line child. Make the child stretch so clicks anywhere in the visible field focus it.
```suggestion
variant === "default" && isMultiline && "min-h-16 items-stretch py-1",
``` |
| #8633 2026-10-20 20:15 | coderabbit inline · P2 | frontend/src/components/v3/platform/SecretInput/SecretInput.tsx:249 | defect-functional | **🎯 Functional Correctness** | **🟡 Minor** | **⚡ Quick win**
**Make the textarea fill the multiline field.**
When the value or placeholder occupies one line, `min-h-16` enlarges the outer container but not the inner wrapper. The absolutely positioned textarea covers only the wrapper. Clicking the lower part of the visible field therefore does not focus it. Give the inner wrapper a matching minimum height while preserving its ability to grow with multiline content. |
| #8633 2026-10-20 20:15 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8633 2026-10-20 20:31 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No merge-blocking issues were found in the reviewed changes.
Summary
The share form now masks secret values and lets users focus stored values without editing them.
- The share form masks secret values and explains when a stored value is locked.
- More Options shows how many sharing constraints are active.
- Share names have a 50-character limit.
Browser checks confirmed that multiline values reveal on focus, conceal on blur, and retain their exact content through submission. Stored values reveal through label or keyboard focus and conceal again on blur while remaining read-only.
No actionable issues were found. The reviewed changes are safe to … the rest (116 characters)merge. Reviews (1) · Last reviewed commit: "improvement(secret-sharing): masked shar..." · Reviewed by the reviewer |
| #8656 2026-10-20 20:32 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The unused V2 `SecretPathInput` implementation and barrel export were removed. The lifecycle and migration records now mark the V2 component as candidate removal. The independent V3 implementation and consumers remain unchanged; parity validation and merged-main removal are not certified.
**Changes: V2 SecretPathInput removal**
|Layer / File(s)|Summary|
|:---|:---|
|**Remove V2 component and record candidate status** `frontend/src/components/v2/SecretPathInput/SecretPathInput.tsx`, `frontend/src/components/v2/SecretPathInput/index.tsx`, `frontend/src/components/V3_MIGRATION.md`, `frontend/src/components/COMPONENT_LIFECYCLE.md`|The V2 implementation and barrel export are del … the rest (3589 characters)eted. The migration record documents the reference audit and candidate removal. The lifecycle record distinguishes `SecretPathInput` removal from `SecretInput`, which remains blocked.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~10 minutes **Change:** Other **Suggested reviewers:** `sheensantoscapadngan`, `andrewhuhh` Merge Risk: 🔵 Low · up to c1629 The change is otherwise low risk, but the candidate inventory should be corrected before merge so audit records remain reliable. Architecture risk: 🔵 Low · up to c1629 The change affects 1 system. **Changed systems:** `frontend` **Architecture concerns** No architecture-level concerns identified. Review details **Systems and components** - _observed_ — frontend (ui) was modified; 4 changed files map to changed impact. **Before / after behavior** - _observed_ — Modified behavior in frontend/src/components/COMPONENT_LIFECYCLE.md: `SecretInput` remains blocked, while `SecretPathInput` replaces the previous blocked status with a candidate-removal record describing its deleted unused V2 implementation and family index, reference audit, and unchanged independent V3 implementation. - _observed_ — Modified behavior in frontend/src/components/V3_MIGRATION.md: Updated the `SecretPathInput` migration status from an active V3 replacement requiring parity checks to deletion of the unused V2 implementation and family index, with no incoming references or root, dynamic, glob, CSS, or initializer ingress identified. - _observed_ — Modified behavior in frontend/src/components/v2/SecretPathInput/SecretPathInput.tsx: The `SecretPathInput` implementation was deleted, including its public wrapper, project-context integration, folder-query and suggestion state management, keyboard navigation, and popover-based folder suggestions. - _observed_ — Modified behavior in frontend/src/components/v2/SecretPathInput/index.tsx: The file’s `SecretPathInput` re-export was removed. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the removal of the unused V2 SecretPathInput source and follows the Conventional Commits format. | | Description check | | The description is complete and relevant. It includes context, verification steps, change type, checklist status, documentation impact, and the scope of unchanged V3 consumers. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8656 2026-10-20 20:36 | coderabbit inline · P2 | frontend/src/components/V3_MIGRATION.md:70 | docs | **🗄️ Data Integrity & Integration** | **🟡 Minor** | **⚡ Quick win**
**Synchronize the candidate inventory totals.**
Line 83 presents the totals as current candidate-worktree numbers. The candidate deletes one family and two V2 files, so update the totals to 34 families and 70 files.
Suggested fix
```diff
-First wave deletes **17 families / 35 files** plus their root exports. The candidate retains **35 families / 72 V2 files including the barrel**, and the same 312 external consumer files. These are candidate-worktree numbers until the PR is explicitly merged; the baseline does not change because a PR was opened.
+First wave deletes **17 families / 35 files** plus their root exports. Th … the rest (243 characters)e candidate retains **34 families / 70 V2 files including the barrel**, and the same 312 external consumer files. These are candidate-worktree numbers until the PR is explicitly merged; the baseline does not change because a PR was opened. ``` |
| #8656 2026-10-20 20:36 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8656 2026-10-20 20:38 | cubic inline · P3 | frontend/src/components/V3_MIGRATION.md:70 | docs | Update the candidate inventory totals to 34 families and 70 files; this retirement removes one family and two V2 files, making the current totals stale. |
| #8656 2026-10-20 20:38 | cubic review-body | — | summary | **1 issue found** across 4 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8656 2026-10-20 20:41 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** Safe to merge within the stated source-retirement scope; current consumers continue to resolve to the unchanged V3 implementation.
Summary
Deletes the unused V2 `SecretPathInput` implementation and family index.
- The unused V2 secret-path input is removed from the candidate.
No actionable issues found. Dependency audits and compiler-resolved reference checks passed on both base and head. Frontend type checks passed on both revisions with a 6 GiB Node heap limit. Lint was not run. The change is safe to merge within this source-retirement scope.
Reviews (1) · Last reviewed commit: "chore(ui): remove unused V2 SecretPathIn..." · Reviewed by the revie … the rest (3 characters)wer |
| #8655 2026-10-20 20:43 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Mfa.tsx now displays WebAuthn failed-attempt messages in a danger Alert with a close icon. The basic Error component has been removed.
**Changes: WebAuthn failure alert**
|Layer / File(s)|Summary|
|:---|:---|
|**Replace the failed-attempt display** `frontend/src/components/auth/Mfa.tsx`, `frontend/src/components/basic/Error.tsx`|Mfa.tsx replaces the Error display with a danger Alert and the existing remaining-attempt message. The basic Error component is removed.|
**Priority:** ⬇️ Low
**Estimated code review effort:** 2 (Simple) | ~5 minutes
**Change:** Refactor
**Suggested reviewers:** `andrewhuhh`
Merge … the rest (2303 characters)Risk: ⚪ Minimal · up to 7805b The new alert presentation has no identified merge-blocking risk. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description check | | The description includes the required Context, Screenshots, Steps to verify the change, Type, and Checklist sections. It documents the scope, verification results, limitations, and checklist selection… | | Title check | | The title uses the conventional commit format and clearly describes the main change: replacing the passkey failure presentation with the V3 alert. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes the required Context, Screenshots, Steps to verify the change, Type, and Checklist sections. It documents the scope, verification results, limitations, and checklist selections clearly. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 1 - Waived: $0.25 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8655 2026-10-20 20:44 | cubic review-body | — | summary | **No issues found** across 2 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8655 2026-10-20 20:56 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Low impact]** The change reaches only the passkey MFA failure display\.
Safe to merge with one non-blocking readability concern in the passkey failure warning.
Findings
1. **Restore error text contrast**
Summary
Replaces the passkey failure message in `Mfa.tsx` with the V3 danger Alert and deletes the unused Error helper.
- Passkey failures now appear in the V3 danger alert.
### the check validation blocked
Evidence upload was unavailable because the environment exposed no artifact-upload tool or upload endpoint. Browser execution completed successfully, and scripts, output, and screenshots were preserved locally. Authenticated MFA and passkey-provider accept … the rest (131 characters)ance were not tested. Reviews (1) · Last reviewed commit: "improvement(auth): use V3 alert for pass..." · Reviewed by the reviewer |
| #8655 2026-10-20 20:56 | greptile inline · P2 | frontend/src/components/auth/Mfa.tsx:433 | defect-functional | **Restore error text contrast**
The new `AlertDescription` inherits the danger variant's 80%-opacity text color. Rendering the exact replacement with the application styles reduces the remaining-attempts message's contrast from 5.09:1 to 3.54:1 in dark mode, below the 4.5:1 minimum for normal-sized text. The additional light-theme check also falls from 5.93:1 to 4.12:1. This is a non-blocking readability regression for users reading the passkey failure warning, particularly those with low vision. Use a sufficiently contrasting description color while retaining the V3 alert.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for … the rest (46 characters)next time! Ran code and verified through TREX |
| #8661 2026-10-20 20:58 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The changes make the action bar’s clear control icon-only by default, retain the non-icon-only control in the secret dashboard commit form, and add conditional right padding to toasts with a close button.
**Changes: Selected Action Bar Clear Control**
|Layer / File(s)|Summary|
|:---|:---|
|**Clear control default and override** `frontend/src/components/v3/generic/SelectedActionBar/SelectedActionBar.tsx`, `frontend/src/pages/secret-manager/SecretDashboardPage/components/CommitForm/CommitForm.tsx`|`SelectedActionBar` defaults `iconOnlyClear` to `true`. The secret dashboard commit form sets it to `false`.|
**Toas … the rest (2654 characters)t Close Button Padding** |Layer / File(s)|Summary| |:---|:---| |**Conditional toast padding** `frontend/src/components/v3/generic/Toast/Toast.tsx`|Toast styling adds right padding when a close button descendant is present.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~8 minutes **Change:** Bug fix **Suggested reviewers:** `andrewhuhh` Merge Risk: ⚪ Minimal · up to 33811 The shared UI changes are ready to merge with no established blocking risk. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly summarizes both main UI fixes and follows the conventional commit format. | | Description check | | The description is complete and matches the required template. It explains the problems and solutions, lists affected and unchanged surfaces, includes screenshots, verification steps, selected change … | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description is complete and matches the required template. It explains the problems and solutions, lists affected and unchanged surfaces, includes screenshots, verification steps, selected change types, and checklist results. ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8661 2026-10-20 20:59 | cubic review-body | — | summary | **No issues found** across 3 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8661 2026-10-20 21:10 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** Safe to merge. Browser checks confirmed the intended toast spacing and selection-control behavior without identifying a regression.
Summary
Updates two shared UI defaults:
- Toasts leave room for the close button when it appears.
- Bulk-selection bars show an X clear button by default.
Browser checks confirmed that long toast titles remain clear of the close button at desktop and mobile widths. Loading toasts and toasts with closeButton: false retain their original padding and layout. The selection-clear button has the “Unselect All” accessible name and tooltip, and clearing works by click, Escape, and keyboard activation. The explicit “Discard” … the rest (208 characters)text override remains functional. No actionable issues were found. The changes are safe to merge. Reviews (1) · Last reviewed commit: "fix(ui): reserve toast close-button spac..." · Reviewed by the reviewer |
| #8659 2026-10-20 21:11 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
SurfaceSelect now uses Combobox instead of FilterableSelect. It retains multi-select behavior, maps changes from the selected options, and sets a search accessibility label.
**Changes: Surface selector**
|Layer / File(s)|Summary|
|:---|:---|
|**Combobox integration** `frontend/src/components/proxied-services/forms/SurfaceSelect.tsx`|SurfaceSelect replaces FilterableSelect with Combobox. It maps option changes from the selected option array and sets the search accessibility label to "Select surfaces".|
**Priority:** ⬇️ Low
**Estimated code review effort:** 2 (Simple) | ~5 minutes
**Change:** Refactor
**Sugge … the rest (2408 characters)sted reviewers:** `saifsmailbox98` Merge Risk: ⚪ Minimal · up to 7d8de The selector migration preserves the existing multi-select behavior and has no identified merge-blocking risk. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: migrating the proxied-services surface picker to Combobox. It follows the conventional commit format. | | Description check | | The description includes context, screenshots, verification steps, change type, checklist status, test results, and explicit test limitations. It aligns with the repository template and the pull reque… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes context, screenshots, verification steps, change type, checklist status, test results, and explicit test limitations. It aligns with the repository template and the pull request objectives. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 1 - Waived: $0.25 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8659 2026-10-20 21:12 | cubic review-body | — | summary | **No issues found** across 1 file
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8659 2026-10-20 21:25 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** Safe to merge within the reviewed scope; the actual picker preserved controlled form values and passed the exercised browser interactions.
Summary
Replaces `FilterableSelect` with `Combobox` in `SurfaceSelect`, preserving the four surface options, their order, controlled enum-array mapping, clear behavior, and disabled/error props. Adds the accessible search name “Select surfaces.”
No actionable issues found. Browser checks using the actual components, production styles, and a React Hook Form controlled field passed pointer and keyboard selection, external edit values, chip removal, Backspace, clearing to an empty array, disabled/error states, focus … the rest (601 characters)retention, and Escape behavior. The migrated picker passed all 20 main interaction checks; the previous picker passed all 18 applicable comparison checks. Supplemental edit-transition and computed error-style checks also passed. Nine shared Combobox tests and scoped lint passed. Safe to merge within this picker-migration scope. Authenticated create/edit wizard submission and provider acceptance were not exercised. - The surface picker now uses the V3 Combobox for multiple selections. Reviews (1) · Last reviewed commit: "improvement(proxied-services): migrate s..." · Reviewed by the reviewer |
| #8587 2026-10-20 21:27 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Cron enqueueing now tracks each job’s last scheduled fire in Redis. The enqueue logic handles existing run hashes and applies a threshold to first fires. New tests cover restart deduplication, missing timestamps, and first-fire boundaries.
**Changes: Cron fire deduplication**
|Layer / File(s)|Summary|
|:---|:---|
|**Persistent fire tracking and enqueue logic** `backend/src/lib/cron/cron-job.ts`|The enqueue script uses a persistent per-job last-fire key, handles existing run hashes, and applies a two-interval threshold to first fires. The caller passes three Redis keys and a migration eligibility flag.|
|**Enque … the rest (5475 characters)ue behavior validation** `backend/e2e-test/cron-job.spec.ts`, `backend/src/lib/cron/cron-job.test.ts`|End-to-end tests cover restart deduplication and first-fire cases. Unit tests identify three-key EVAL calls and parse the updated argument positions.| **Priority:** ➖ Normal **Estimated code review effort:** 3 (Moderate) | ~25 minutes **Change:** Bug fix **Suggested reviewers:** `victorvhs017` Merge Risk: ⚪ Minimal · up to 04362 The change makes cron fire deduplication persistent across restarts, with tests covering restart, backfill, and first-fire behavior. No merge-blocking risk was identified. Security architecture risk: 🟡 Moderate · up to 04362 Persistent deduplication prevents repeated scheduling after restarts, but it also prevents recovery of an unexecuted fire after its temporary run state expires. For weekly encryption-key cleanup, this can postpone security maintenance until the following week. Existing worker controls remain in place, and the inspected production wiring does not introduce a new externally accessible entrypoint. **Retained concerns** - **Medium · reliability · inferred:** The durable marker can suppress recovery of work that never executed. If a fire is enqueued, all relevant workers are interrupted until its run hash expires, and a worker restarts before the next scheduled fire, LAST_KEY causes enqueueing to return AlreadyEnqueued while processing removes the expired pending entry. The base implementation could recreate that latest fire after restart. KMS root-key cleanup runs weekly with only one hour of run-state retention, so this failure sequence can postpone retirement of eligible obsolete keys until the following week. Lease reclaim and retries do not recover a run whose hash has disappeared. Security review details **Security Blast Radius** - _inferred_ — The recovery concern follows a job's shared Redis name and prefix across workers, not an individual request or tenant. For KMS cleanup, its downstream effect is delayed instance-level obsolete-key retirement. The evidence does not establish credential disclosure, privilege escalation, or attacker control over this transition. **Security Findings and Attack Paths** - _inferred_ — The identified concern is an interruption-and-restart recovery regression affecting security maintenance, not a verified attacker-driven exploit. It requires run-state expiry before successful execution and restart before the next scheduled fire. **Trust Boundaries and Controls** - _observed_ — The inspected production path supplies Redis and locking authority during initialization. Run processing selects a handler from the in-process registration map and retains lease-based execution. The changed Lua script does not introduce request-supplied handler selection or new execution credentials. **Resilience and Maintainability Implications** - _observed_ — The added integration scenarios cover suppression after a completed run's hash disappears, legacy backfill, overdue first-fire skipping, and the just-due migration exception. The expired-hash restart scenario explicitly completes the first handler before deleting its run state, so it does not establish recovery of an unexecuted fire. **Hardening Proposals** - _proposed_ — Define an explicit recovery contract that distinguishes enqueue acknowledgement from terminal execution outcome, then retain sufficient durable state to reconcile an expired unclaimed run without blindly replaying completed work. Validate the interruption-and-restart sequence using the weekly cleanup retention window. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title uses conventional commit format and clearly summarizes the cron deduplication and first-fire behavior changes. | | Description check | | The description provides detailed context, verification steps, change type, rollout behavior, known rollback risk, and test results. The required structure is substantially complete. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 3 - Waived: $0.75 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8587 2026-10-20 21:34 | cubic inline · P2 | backend/src/lib/cron/cron-job.ts:117 | defect-functional | This timestamp watermark assumes a job’s cron pattern never changes. Changing a job to a less-frequent pattern can leave its next occurrence earlier than the stored `last`, so this comparison suppresses it; include a pattern/version fingerprint or reset the watermark when the pattern changes. |
| #8587 2026-10-20 21:34 | cubic review-body | — | summary | **2 issues found** across 3 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8587 2026-10-20 21:34 | cubic inline · P2 | backend/src/lib/cron/cron-job.ts:137 | defect-concurrency | This skip is not visible to old pods: their enqueue script only checks `runKey`. During a mixed-version rollout, an old slot holder can enqueue this fire after this branch, defeating first-fire suppression; write a tombstone run hash with a TTL or defer this behavior until old pods are gone. |
| #8587 2026-10-20 21:35 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Medium impact]** The change reaches scheduled jobs across the backend’s general\-worker workflow\.
The reported test coverage gap is non-blocking; no blocking production defect is established.
Findings
1. **Next fire is not tested**
Summary
Adds a permanent per-job timestamp so expired run hashes no longer allow duplicate fires.
- Cron jobs keep a lasting record of each fire they enqueue.
- New cron jobs wait for their next scheduled fire.
PierreOucif explicitly documents skipping old fires on first registration as intentional, so newly registered jobs do not immediately run an old fire on deploy. PierreOucif also documents allowing just-due fires during migra … the rest (613 characters)tion as intentional, to avoid dropping a boundary fire during the initial rollout. PierreOucif acknowledges the rollback/redeploy duplicate-fire limitation as known: if a rollback lasts beyond run-hash expiry, redeployment can enqueue that fire again; the PR describes this as existing main behavior rather than a regression. The remaining concern is a non-blocking test coverage gap in backend/e2e-test/cron-job.spec.ts. It does not establish a production scheduling defect and does not block merging. Reviews (1) · Last reviewed commit: "chore(cron): label the first-deploy migr..." · Reviewed by the reviewer |
| #8587 2026-10-20 21:35 | greptile inline · P2 | backend/e2e-test/cron-job.spec.ts:586 | tests | **Next fire is not tested**
This test checks that the old fire is skipped, but never moves the clock forward to check that the next fire runs. The unchanged test also passes against a scheduler double that continually skips fires, so it does not protect the promise that a newly registered job eventually runs. This is a non-blocking coverage gap: extend the test past the next minute and assert exactly one handler call.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
**Executed source for the next-fire coverage check**
- Uploaded the authored script that executes the repository callback against two i … the rest (653 characters)solated scheduler doubles; HTTP 200 confirms the source was uploaded. **Existing test accepts a continually-skipping scheduler** - Uploaded the captured original execution showing both scheduler doubles pass, demonstrating the missing next-fire coverage. **Extended test rejects a continually-skipping scheduler** - Uploaded the captured extended execution showing the control passes and the continually-skipping double fails, supporting the proposed assertion. **Tracked files remain unchanged** - Uploaded the captured Git diff checks and proof-file hashes, confirming validation left tracked files unchanged. Ran code and verified through TREX |
| #8644 2026-10-20 21:37 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The universal auth login path now converts the client-secret usage count and configured limit to numbers before checking whether the limit has been reached. The existing revocation and unauthorized-error behavior remains unchanged.
**Changes: Universal auth usage limit**
|Layer / File(s)|Summary|
|:---|:---|
|**Normalize usage limit comparison** `backend/src/services/identity-ua/identity-ua-service.ts`|The login path compares numeric conversions of the client-secret usage count and limit before enforcing the limit.|
**Priority:** ➖ Normal
**Estimated code review effort:** 2 (Simple) | ~10 minutes
**Change:** … the rest (4701 characters)Bug fix · **Severity of issue fixed:** Medium **Suggested reviewers:** `thiago-as` Merge Risk: ⚪ Minimal · up to 4e4ff Universal Auth login now compares client-secret usage counts and limits as numbers. Secrets with multi-digit usage limits will no longer be revoked early. Enforcement at the configured limit still revokes the secret and rejects the login. No merge-blocking risk was identified in this change. Security architecture risk: ⚪ Minimal · up to 4e4ff The fix restores numeric usage-limit enforcement without changing credential validation, token authority, or public interfaces. No material security risk introduced or materially worsened by this change was identified. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The affected security outcome is token issuance using an existing machine-identity credential. Reaching the changed gate requires a matching secret and passing the existing IP and lockout checks. The conversion introduces no new identity, organization scope, or token privilege. **Security Findings and Attack Paths** - _inferred_ — Concurrent requests can pass the application-level snapshot check before their increments complete. The same opportunity exists at the base for a valid single-use secret, so correcting multi-digit comparisons does not introduce this transition gap or increase its maximum credential authority. Actual database-level prevention was not verified. **Trust Boundaries and Controls** - _observed_ — The public route applies its existing write rate limit and input schema. The service retains secret-hash verification, non-revoked lookup filtering, configured IP and lockout checks, expiration enforcement, and organization-membership checks before token issuance. **Resilience and Maintainability Implications** - _observed_ — For finite limits, login awaits the usage-update transaction before calling the token issuer. Login contains no compensation that refunds a committed use after later token-issuance or response failure. The exhaustion branch writes revocation before rejecting the request. These failure-ordering behaviors are unchanged. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change and follows the conventional commit format. | | Description check | | The description explains the issue, technical cause, solution, verification steps, fix type, and checklist status. The omitted Screenshots section is not required because this change has no UI impact. | | Linked Issues check | | Issue `#8378` is closed and supplies historical context only, so it creates no active coding requirements. The reviewed diff nevertheless matches the issue context: the Universal Auth login check conver… | | Out of Scope Changes check | | The pull request changes only the usage-limit comparison in backend/src/services/identity-ua/identity-ua-service.ts. This change directly addresses the stated Universal Auth login bug and introduces n… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | Full details: Linked Issues check **Explanation** Issue `#8378` is closed and supplies historical context only, so it creates no active coding requirements. The reviewed diff nevertheless matches the issue context: the Universal Auth login check converts both usage values with Number() before it compares them, while it preserves the existing revocation behavior. Full details: Out of Scope Changes check **Explanation** The pull request changes only the usage-limit comparison in backend/src/services/identity-ua/identity-ua-service.ts. This change directly addresses the stated Universal Auth login bug and introduces no unrelated behavior or files. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8644 2026-10-20 21:37 | cubic review-body | — | summary | **No issues found** across 1 file
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8644 2026-10-20 21:42 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** Safe to merge within the reviewed scope: the changed comparison corrects premature revocation while preserving finite-limit enforcement and unlimited-secret behavior.
Summary
The PR converts client-secret usage counts and limits to numbers before comparing them.
- Universal Auth login checks client-secret limits as numbers.
Reviews (1) · Last reviewed commit: "fix(identity-ua): compare client secret ..." · Reviewed by the reviewer |
| #8667 2026-10-20 21:49 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The blackbox workflow now runs `make test-suites`. The Infisical readiness probe now has a two-minute startup timeout and retains its HTTP 200 matcher.
**Changes: Blackbox test execution**
|Layer / File(s)|Summary|
|:---|:---|
|**Select the test suites target** `.github/workflows/run-blackbox-tests.yml`|The `Run suites` step now invokes `make test-suites` instead of `make test-all`.|
|**Set the readiness startup timeout** `tests/harness/infisical/ready.go`|`readyStrategy` now applies a two-minute startup timeout to the `/api/status` probe. The HTTP 200 requirement remains unchanged.|
**Priority:** ⬇️ Low
**Es … the rest (2239 characters)timated code review effort:** 2 (Simple) | ~5 minutes **Change:** Bug fix **Suggested reviewers:** `akhilmhdh` Merge Risk: ⚪ Minimal · up to fa45e The black-box workflow now runs the intended suites, with a longer allowance for Infisical startup. No actionable merge risk remains after normal checks. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title identifies the CI test failure fix and matches the main change. It contains spelling errors in "resovled" and "contraint," but remains clear and related. | | Description check | | The description explains the CI test failure, the test-suite change, and the timeout increase. It marks the change as a fix. The verification steps and several checklist items are incomplete, but the … | | Docstring Coverage | | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 … | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description explains the CI test failure, the test-suite change, and the timeout increase. It marks the change as a fix. The verification steps and several checklist items are incomplete, but the description is mostly complete. Full details: Docstring Coverage **Explanation** Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8667 2026-10-20 21:50 | cubic inline · P2 | tests/harness/infisical/ready.go:32 | defect-functional | This strategy now fails after two minutes even though the harness allows five minutes for the application’s 540 cold-start migrations. Keep the readiness timeout aligned with the five-minute startup allowance.
```suggestion
WithStartupTimeout(5 * time.Minute)
``` |
| #8667 2026-10-20 21:50 | cubic inline · P2 | .github/workflows/run-blackbox-tests.yml:86 | tests | Switching to `make test-suites` (which runs only `./suites/...`) permanently removes the entire `harnesstest` package from CI. No other workflow runs `make test-harness`, `test-all`, or the harnesstest package (verified: the only match across `.github/workflows/` is this file), and `tests/Makefile` defines `test-suites: PKGS = ./suites/...` while `test-all` includes `./harnesstest/...`. This fixes the red build by deleting coverage rather than by the actual root-cause fix in this PR (the startup-timeout change in `tests/harness/infisical/ready.go`). The harness tests prove the test tooling itself and are the only CI check that catches harness regressions, so when the API is rewritten the har … the rest (401 characters)ness can silently break without anything failing. Keep the harness covered by running `make test-suites && make test-harness` (each target starts/stops the stack with its own trap), or run the harnesstest packages as a separate step; at minimum document explicitly why `./harnesstest/...` is excluded so it doesn't look accidental. ```suggestion run: make test-suites && make test-harness ``` |
| #8667 2026-10-20 21:50 | cubic review-body | — | summary | **2 issues found** across 2 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8667 2026-10-20 21:56 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Low impact]** The changes stay within the blackbox test workflow and container startup wait\.
Safe to merge with a non-blocking gap in harness regression coverage.
Findings
1. **Harness tests stop running**
Summary
This PR separates the black-box suites from the harness tests and gives the application longer to start.
- Blackbox CI runs the product suites without the harness tests.
- The harness gives Infisical two minutes to become ready.
The package-selection check confirmed that both harness regression packages stop running in CI, and the unit job does not replace that coverage. This is a non-blocking coverage gap; no blocking issue was established.
Review … the rest (102 characters)s (1) · Last reviewed commit: "fix: resovled test failing due to ci con..." · Reviewed by the reviewer |
| #8667 2026-10-20 21:56 | greptile inline · P2 | .github/workflows/run-blackbox-tests.yml:86 | tests | **Harness tests stop running**
Switching to `test-suites` removes the only CI run of `./harnesstest/...`. `test-unit` also excludes those packages, and no other workflow runs them. This leaves the isolated-instance and shared-harness regression checks without CI coverage, so regressions they catch can pass unnoticed. This is a non-blocking coverage concern: keep this job limited to suites, but run `test-harness` in a separate job so it does not compete for the same runner's resources.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
**Evidence from the check**
- Executed against both revisions to in … the rest (593 characters)spect workflow invocations and probe actual Make package selection, demonstrating the harness coverage loss without launching Docker. **Command output from the check** - Captured the base workflow-selected Make recipe, complete workflow invocation inventory, and passing unit run; test-all selects both harness regression packages. **Command output from the check** - Captured the head workflow-selected Make recipe, complete workflow invocation inventory, and passing unit run; test-suites omits both harness packages with no replacement CI invocation. Ran code and verified through TREX |
| #8576 2026-10-20 21:58 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Secret folder queries now share search and tag predicates. The test harness adds project-role creation support, and integration tests cover tag-filtered secret listings, dashboard searches, and tag-based permissions.
**Changes: Secret tag queries and permissions**
|Layer / File(s)|Summary|
|:---|:---|
|**Shared search and tag predicates** `backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts`|Added shared search and correlated tag-selection predicates. `countByFolderIds` and `findByFolderIds` use them for search and tag-slug filtering.|
|**Role and secret test harness** `tests/clients/api/oapi-codegen. … the rest (4976 characters)yaml`, `tests/clients/api/overlay.yaml`, `tests/docs/client.md`, `tests/docs/writing-tests.md`, `tests/fixture/role.go`, `tests/suites/secretmanager/secrets/helpers_test.go`|Added API generation configuration and an overlay, project-role fixture support, secret listing helpers, and related harness documentation.| |**Tag filtering and permission integration tests** `tests/suites/secretmanager/secrets/dashboard_test.go`, `tests/suites/secretmanager/secrets/permission_test.go`|Added coverage for dashboard search and listing with tag filters, allowed and denied tags, and conflicting allow and deny roles.| **Priority:** ➖ Normal **Estimated code review effort:** 3 (Moderate) | ~25 minutes **Change:** Bug fix **Suggested reviewers:** `akhilmhdh` Merge Risk: ⚪ Minimal · up to fdd91 No actionable issue was established in the changed secret-listing or test paths. The PR is mergeable after normal checks. Security architecture risk: 🔵 Low · up to fdd91 The change strengthens tag-based access checks while preserving the reviewed project, folder, and actor restrictions. No introduced security regression was established. Uncertainty remains about whether aggregate counts should reflect per-secret access restrictions. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The affected exposure is authenticated secret listing within resolved project and folder scope, including keys, tags, metadata, and permission-governed values. The reviewed change preserves folder and shared-or-owner restrictions rather than granting access to another tenant or actor's personal secrets. **Security Findings and Attack Paths** - _inferred_ — The relevant attack path is choosing a benign tag or search term that removes a deny tag from the authorization input. Separating existence-based selection from tag hydration addresses that path in the reviewed queries. No introduced or materially worsened attack path was established. **Trust Boundaries and Controls** - _observed_ — Search and tag parameters refine candidate selection rather than confer authority. Service checks evaluate complete secret subjects through the existing permission helper, which supports both separate describe/read-value actions and the legacy combined action. The helper and service enforcement are unchanged between base and head. **Resilience and Maintainability Implications** - _observed_ — Aggregate counting remains separate from per-secret tag authorization. This predates the PR and is not retained as an introduced concern. Whether counts must conceal secrets denied by tag-specific policy remains an unresolved disclosure-policy question. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files. (4 skipped: 4 … | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :---------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title uses the conventional commit format and clearly describes the main query change: returning all tags for access evaluation. | | Description check | | The description includes context, verification steps, change type, and checklist status. It also explains the tag-based permission fix and documents the test coverage. | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files. (4 skipped: 4 unsupported.) **Resolution** Write docstrings for the functions missing them to satisfy the coverage threshold. ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8576 2026-10-20 22:06 | cubic inline · P2 | tests/fixture/role.go:26 | defect-functional | Calling `NewRole` without permissions encodes `permissions` as `null`, which the create-role API rejects because it expects an array. Encode an empty slice as `[]` for this case.
```suggestion
body, err := json.Marshal(map[string]any{"slug": slug, "name": slug, "permissions": append([]Permission{}, permissions...)})
``` |
| #8576 2026-10-20 22:06 | cubic review-body | — | summary | **1 issue found** across 10 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8576 2026-10-20 22:06 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** No blocking findings remain; the checked database behavior supports merging this change.
Summary
This PR keeps every tag available for permission checks when users filter or search secrets.
- Secret listings check every tag before showing matching secrets.
- Go integration tests can create roles and call the dashboard listing.
Verified the real secret-list database code against disposable PostgreSQL using the same fixtures on the base and head revisions. On the base revision, filtering by allowed drops the restrict tag. On this branch, both normal and rank-paginated filtering preserve both tags, exclude unrelated secrets, and return no results fo … the rest (313 characters)r a nonexistent tag. All four checks passed on this branch. No blocking issues were found. The change is safe to merge within the checked scope. HTTP permission enforcement and the full application test suite were not executed. Reviews (1) · Last reviewed commit: "regenerate api.gen" · Reviewed by the reviewer |
| #8670 2026-10-20 22:12 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The quick-add secret name input no longer uses explicit medium font styling or normal-weight placeholder styling.
**Changes: Quick-add input styling**
|Layer / File(s)|Summary|
|:---|:---|
|**Update secret name input classes** `frontend/src/pages/secret-manager/OverviewPage/components/QuickAddSecretRow/QuickAddSecretRow.tsx`|Removed the `font-medium` and `placeholder:font-normal` classes from the secret name input.|
**Priority:** ⬇️ Low
**Estimated code review effort:** 1 (Trivial) | ~2 minutes
**Change:** Bug fix
**Suggested reviewers:** `andrewhuhh`
Merge Risk: ⚪ Minimal · up to efaf5
The quick-add inpu … the rest (2312 characters)t now inherits the intended shared styling, with no identified merge-blocking risk. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the UI fix and follows the conventional commit format. | | Description check | | The description explains the prior and current behavior, provides verification steps, identifies the change as a fix, and includes the relevant checklist items. The UI screenshot section and local-tes… | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description explains the prior and current behavior, provides verification steps, identifies the change as a fix, and includes the relevant checklist items. The UI screenshot section and local-testing checkbox are not completed, but the description is otherwise mostly complete. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 1 - Waived: $0.25 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8670 2026-10-20 22:13 | cubic review-body | — | summary | **No issues found** across 1 file
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8670 2026-10-20 22:23 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** Safe to merge. Browser checks confirm the intended font-weight alignment without changing measured placeholder typography.
Summary
Removes `font-medium` and `placeholder:font-normal` from the quick-add secret name input so typed keys match existing secret keys.
Browser checks using the actual input components, application CSS, and fonts confirmed that typed quick-add names change from font weight 500 to 400, matching existing-key presentations in both single- and multi-environment layouts. Placeholder weight remains 400, with the other measured placeholder styles unchanged.
Verification used a focused rendering fixture with mocked project context, … the rest (287 characters)not an authenticated full-app session. No actionable issues were found. This typography change is safe to merge. - Quick-add secret names now match the weight of existing rows. Reviews (1) · Last reviewed commit: "fix(ui): match quick-add secret key inpu..." · Reviewed by the reviewer |
| #8662 2026-10-20 22:26 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The secret input now tracks cursor and suggestion state while editing references. The reference wizard emits navigation strings and searches same-project secrets from the environment step, displaying results alongside filtered environments.
**Changes: Secret reference editing and search**
|Layer / File(s)|Summary|
|:---|:---|
|**Reference editing state** `frontend/src/components/v3/platform/SecretInput/InfisicalSecretInput.tsx`|The input tracks cursor position and suggestion activation, centralizes reference replacement, and controls popover visibility.|
|**Wizard reference navigation** `frontend/src/components/v3/platform/SecretInput/SecretReferenceWizard.tsx`|The wizard a … the rest (4593 characters)ccepts an `onNavigate` callback, synchronizes state with reference input, and emits reference strings for navigation.| |**Environment-step secret search** `frontend/src/components/v3/platform/SecretInput/SecretReferenceWizard.tsx`|The wizard searches same-project secrets, combines results with environment options, and supports selecting results and navigating breadcrumbs.| **Priority:** ➖ Normal **Estimated code review effort:** 3 (Moderate) | ~20 minutes **Change:** Bug fix ### Sequence Diagram(s) ```mermaid sequenceDiagram participant InfisicalSecretInput participant SecretReferenceWizard participant ProjectSecretQuickSearch InfisicalSecretInput->>SecretReferenceWizard: Pass reference input SecretReferenceWizard->>ProjectSecretQuickSearch: Search after debounce ProjectSecretQuickSearch-->>SecretReferenceWizard: Return matching secrets SecretReferenceWizard->>InfisicalSecretInput: Emit selected secret reference InfisicalSecretInput->>InfisicalSecretInput: Replace reference content ``` **Suggested reviewers:** `mathnogueira` Merge Risk: 🟡 Moderate · up to 731ac Adding a reference before another can silently remove part of a secret value. Fix the replacement boundary before merging. Security architecture risk: 🔵 Low · up to 731ac The new search remains restricted to the current project and uses existing server-side secret-description permissions. No introduced authorization bypass or secret-value disclosure was established. Some supporting response paths and downstream draft-saving behavior remain outside the verified scope. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _observed_ — The new wizard caller searches across environments and folders in the current project, not received cross-project grants. Its maximum discovery scope is therefore broader than the currently browsed folder, but ordinary-secret results remain subject to server-side permissions. **Trust Boundaries and Controls** - _observed_ — Client-supplied project and search parameters cross an authenticated server boundary. The server resolves permissions using the requested project together with actor identity, authentication method, and organization, then evaluates each ordinary secret's environment, path, key, and tags before returning it. **Hardening Proposals** - _proposed_ — Preserve cross-project identity directly from the selected grant, or defer final selection until source-project identity is available, rather than allowing an unqualified reference fallback. This addresses a dependency already present at the PR base, not an established regression from this change. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main UI fixes for selector scrolling, search, and dismissal. It follows the conventional commit format. | | Description check | | The description is complete. It explains the context, affected behavior, screenshots, verification steps, change type, checklist status, and frontend-only scope. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 2 - Waived: $0.50 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #8662 2026-10-20 22:30 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Medium impact]** The changes affect the secret\-reference selector across value\-editing workflows\.
The PR should not merge until the new heading satisfies the repository’s explicit UI styling requirement.
Findings
1. **New heading breaks casing rules**
Summary
Updates the secret reference selector:
- The reference selector follows the text and stays out of the way when it is not needed.
- Typing a secret name finds matches across project environments and folders.
One blocking repository requirement remains: remove the uppercase styling from the new “Secrets in this project” heading before merging.
Reviews (1) · Last reviewed commit: "fix(ui): secret referen … the rest (48 characters)ce selector scrol..." · Reviewed by the reviewer |
| #8662 2026-10-20 22:30 | greptile inline · P1 | frontend/src/components/v3/platform/SecretInput/SecretReferenceWizard.tsx:584 | quality | **New heading breaks casing rules**
The new “Secrets in this project” heading adds `uppercase`. The repository’s UI rule explicitly forbids casing overrides such as uppercase text in new UI, so this heading violates the required styling. Remove `uppercase` rather than copying the older headings’ styling. This requirement must be satisfied before merging.
```suggestion
<div className="px-2 py-1.5 text-[10px] font-semibold tracking-wider text-muted">
Secrets in this project
</div>
```
**Rule Used:** New UI must not override or deviate from the default styling of v3 design-system components (frontend/src/components/v3). Follow DESIGN.md. Flag: legacy v2 … the rest (235 characters)color tokens (e.g. mineshaft-*, bunker-*) instead of semantic tokens like bg-org or text-dange... (source) Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
| #8662 2026-10-20 22:31 | coderabbit inline · P1 | frontend/src/components/v3/platform/SecretInput/InfisicalSecretInput.tsx:92 | defect-data | **🗄️ Data Integrity & Integration** | **🟠 Major** | **⚡ Quick win**
**Stop the right-boundary scan at the next reference, or navigation deletes user text.**
`replaceReferenceContent` uses `getIndexOfUnclosedRefToTheRight` (Lines 24-29) to decide whether the current reference is already closed. The helper returns the first `}` anywhere to the right of the caret. It does not stop at whitespace or at a following `${`.
Trigger: the value is `${dev. and ${prod.KEY}` and the caret is after `${dev.`. The helper finds the `}` of `${prod.KEY}`, so `isEnclosed` is true. `rhsValue` then starts at that `}`. Selecting an environment, folder, or secret replaces ` and ${prod.KEY` with the new content. … the rest (883 characters)This change makes the problem more likely. `onNavigate` now calls `replaceReferenceContent` on every environment, folder, breadcrumb, and tab click, not only on the final selection. A common way to hit it is adding a new reference in front of an existing one, for example in `postgres://${user}:${pass}@...`. The text is removed without any warning and can be saved with the secret. Fix the boundary in the helper so that a reference ends only at its own `}`. 🐛 Proposed fix for the boundary helper ```diff const getIndexOfUnclosedRefToTheRight = (value: string, pos: number) => { for (let i = pos; i < value.length; i += 1) { if (value[i] === "}") return i - 1; + // A new reference or whitespace ends the current (unclosed) reference + if (value[i] === "$" && value[i + 1] === "{") return -1; + if (/\s/.test(value[i])) return -1; } return -1; }; ``` |
| #8662 2026-10-20 22:31 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8662 2026-10-20 22:34 | cubic inline · P1 | frontend/src/components/v3/platform/SecretInput/InfisicalSecretInput.tsx:282 | defect-data | Stop the right-side scan at the next `${` or whitespace before applying navigation updates; otherwise a later `}` can make this call delete intervening user text. |
| #8662 2026-10-20 22:34 | cubic inline · P2 | frontend/src/components/v3/platform/SecretInput/SecretReferenceWizard.tsx:621 | defect-functional | A failed deep-search request is shown as “No matching environments or secrets,” falsely implying the search succeeded. Read the query error state and show an error message instead. |
| #8662 2026-10-20 22:34 | cubic review-body | — | summary | **2 issues found** across 2 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #7670 2026-10-20 22:36 | coderabbit issue-comment | — | summary | 📝 Walkthrough
This change adds Portainer as an app connection and secret-sync destination. It adds backend connection and synchronization operations, frontend connection and destination forms, and Portainer integration and API documentation.
**Changes: Portainer app connections**
|Layer / File(s)|Summary|
|:---|:---|
|**Connection contracts and API** `backend/src/services/app-connection/portainer/*`, `backend/src/services/app-connection/app-connection-*`, `backend/src/server/routes/v1/app-connection-routers/*`|Adds API-token connection schemas and validation, Portainer request handling through direct or gateway routes, environment and stack listing, and app-connection routes and service … the rest (9664 characters)registration.| |**Connection setup and discovery** `frontend/src/pages/organization/AppConnections/...`, `frontend/src/hooks/api/appConnections/portainer/*`, `frontend/src/helpers/appConnections.ts`|Adds the Portainer connection form and query hooks for environments and stacks. The form supports API-token credentials and optional gateway selection.| **Portainer secret sync** |Layer / File(s)|Summary| |:---|:---| |**Synchronization behavior and API** `backend/src/services/secret-sync/portainer/*`, `backend/src/services/secret-sync/secret-sync-*`, `backend/src/server/routes/v1/secret-sync-routers/*`|Adds Portainer destination schemas and routes, duplicate detection, and secret retrieval, synchronization, import, and removal. Compose values are encoded and decoded; Kubernetes stacks are excluded. Tests cover supported stack types and synchronization cases.| |**Destination setup and display** `frontend/src/components/secret-syncs/forms/*`, `frontend/src/pages/secret-manager/...`, `frontend/src/hooks/api/secretSyncs/*`, `frontend/src/helpers/secretSyncs.ts`|Adds Portainer destination validation, environment and stack selection, review and detail displays, and integration-table destination values.| |**Portainer documentation and API reference** `backend/src/lib/api-docs/constants.ts`, `docs/api-reference/endpoints/{app-connections,secret-syncs}/portainer/*`, `docs/integrations/*/portainer.mdx`, `docs/snippets/*`, `docs/docs.json`|Adds Portainer connection and secret-sync guides, endpoint metadata, navigation and catalog entries, form and token-generation snippets, and API documentation fields.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~60 minutes **Change:** Feature ### Sequence Diagram(s) ```mermaid sequenceDiagram participant User participant PortainerSyncFields participant AppConnectionRoute participant PortainerConnectionService participant PortainerAPI User->>PortainerSyncFields: Select connection PortainerSyncFields->>AppConnectionRoute: Request environments and stacks AppConnectionRoute->>PortainerConnectionService: List environments and stacks PortainerConnectionService->>PortainerAPI: Request environments and stacks PortainerAPI-->>PortainerConnectionService: Return environment and stack data PortainerConnectionService-->>PortainerSyncFields: Return mapped results PortainerSyncFields-->>User: Show environment and stack options ``` ```mermaid sequenceDiagram participant SecretSyncRoute participant SecretSyncFns participant PortainerSyncFns participant PortainerAPI SecretSyncRoute->>SecretSyncFns: Dispatch Portainer sync SecretSyncFns->>PortainerSyncFns: Sync secrets for selected stack PortainerSyncFns->>PortainerAPI: Read stack and update environment PortainerAPI-->>PortainerSyncFns: Return stack and update response PortainerSyncFns-->>SecretSyncFns: Return sync result SecretSyncFns-->>SecretSyncRoute: Return operation result ``` Merge Risk: 🔵 Low · up to b66d0 This change adds Portainer connections and secret syncing to stack environment variables. Two minor issues remain. If a connection uses a plain http:// Portainer URL without a Gateway, the API token is sent unencrypted. An empty environment or stack selection also shows a generic validation message. Neither issue blocks normal use, so the change is mergeable once the owner accepts or addresses them. Security architecture risk: 🟡 Moderate · up to b66d0 HTTP configurations can expose Portainer tokens and synchronized secrets. Concurrent writers to the same stack can also undo secret removal or rotation. Authentication and destination checks limit exposure, but recovery after an interrupted deployment remains uncertain. **Retained concerns** - **Low · security · observed:** The new Portainer connection accepts HTTP URLs and forwards its API token and synchronization payloads without requiring TLS. A network observer or active intermediary on a plaintext segment can obtain credentials and secrets. HTTPS configurations verify certificates, and gateway tunneling protects its tunnel, but neither requires encryption on an HTTP destination hop. - **Medium · security · inferred:** Portainer synchronization and removal replace the entire environment-variable list without destination-scoped serialization or a conditional-write token. When separate syncs share a stack, one can retain another's old values and write them back after successful removal or rotation. Per-sync locks do not prevent this interleaving. Duplicate-target detection is useful counterevidence, but enforcement is optional. Successful cleanup can delete its sync record, leaving no subsequent reconciliation from that owner. Security review details **Security Blast Radius** - _inferred_ — Transport exposure includes secrets carried by the configured sync and the connection's API token. A stolen token may authorize resources beyond the selected stack; its actual Portainer privileges are not established. Concurrent-write effects are bounded to writers sharing the same external stack. No cross-tenant escalation was established. **Security Findings and Attack Paths** - _observed_ — The retained transport finding is reachable when an authorized configurator selects an HTTP instance URL. Subsequent requests send X-API-Key and applicable secret-bearing bodies over that protocol. Exploitation requires access to the plaintext network segment; anonymous callers cannot simply bypass the connection authorization boundary. - _inferred_ — A writer can read an old list, retain another owner's secret, and submit that stale value after the other owner removes or rotates it. Both operations can complete without an overlapping deployment conflict. Per-sync locking and fresh reads on retries do not reconcile a successful stale replacement; optional duplicate-target blocking reduces, but does not universally prevent, this exposure. **Trust Boundaries and Controls** - _observed_ — Discovery routes require JWT or OAuth authentication and obtain the connection through the shared Connect authorization path. That path rejects another organization's connection and checks connection-specific project or organization permission. Sync usage additionally rejects a project-scoped connection belonging to a different project. - _observed_ — Direct requests inherit validated-address pinning, private-address restrictions subject to existing configuration, and redirect blocking. Gateway attachment is organization-scoped during connection creation, and gateway routing uses target host/port credentials. These controls constrain destination reachability but do not enforce HTTPS for Portainer. **Resilience and Maintainability Implications** - _inferred_ — If Portainer persists Env before a deployment fails or is interrupted, a retry's equality shortcut cannot distinguish stored configuration from successful workload rollout. The adapter awaits updates and propagates errors, but has no deployment-status reconciliation or rollback. Whether supported Portainer versions exhibit this persistence ordering remains an external-contract gap, not a verified failure. **Hardening Proposals** - _proposed_ — Require HTTPS for credential-bearing Portainer destinations, including the gateway-to-Portainer hop. - _proposed_ — Enforce exclusive ownership or serialize the complete read-modify-write operation by canonical Portainer destination identity. Where external writers remain possible, use a supported conditional-update or conflict-reconciliation contract. Establish deployment recovery semantics before treating stored-variable equality as proof of completed rollout. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly identifies the feature and follows the conventional commit format. It accurately summarizes the Portainer app-connection and secret-sync support. | | Description check | | The description is complete and relevant. It includes context, a screenshot, verification steps, feature classification, checklist status, implementation details, and testing information. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 50 files. (29 skipped: 22 unsupported, 7 over the file limit.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 79 - Waived: $19.75 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. Comment `@the reviewer help` to get the list of available commands. |
| #7670 2026-10-20 22:44 | coderabbit inline · P2 | backend/src/services/app-connection/portainer/portainer-connection-schemas.ts:21 | defect-security | **🔒 Security & Privacy** | **🛡️ Detected with Advanced Tier** | **🟡 Minor** | **⚡ Quick win**
**Sensitive Data Exposure**
**Reachability:** External
**Exploitability:** Difficult
**CWE:** CWE-319 — Cleartext Transmission of Sensitive Information
**Require HTTPS for direct Portainer requests.** The schema accepts `http://` URLs, and `safeRequest` sends the `X-API-Key` header through an `http.Agent` on the direct path. Reject non-HTTPS URLs when no Gateway is configured.
Enforce HTTPS for direct requests
```diff
+ if (!gatewayId && url.protocol !== "https:") {
+ throw new BadRequestError({
+ message: "Portainer instance URL must use HTTPS unless the connection is routed throug … the rest (75 characters)h a Gateway." + }); + } + if (!gatewayId || !gatewayV2Service) { ``` |
| #7670 2026-10-20 22:44 | coderabbit inline · P2 | frontend/src/components/secret-syncs/forms/schemas/portainer-sync-destination-schema.ts:17 | defect-functional | **🎯 Functional Correctness** | **🟡 Minor** | **⚡ Quick win**
**The "Environment is required" and "Stack is required" messages never show for an empty selection.**
`PortainerSyncFields` sets both fields to `undefined` when the connection or environment changes. In Zod 3, an `undefined` input uses `required_error`, not `invalid_type_error`. If `required_error` is not set, Zod uses the default message "Required". So an empty Environment or Stack field shows "Required" and not the intended text. Set `required_error` on both fields.
🐛 Proposed fix
```diff
--- "a/frontend/src/components/secret-syncs/forms/schemas/portainer-sync-destination-schema.ts"
+++ "b/frontend/src/components/secret-sy … the rest (1484 characters)ncs/forms/schemas/portainer-sync-destination-schema.ts" @@ -7,14 +7,20 @@ z.object({ destination: z.literal(SecretSync.Portainer), destinationConfig: z.object({ environmentId: z - .number({ invalid_type_error: "Environment is required" }) + .number({ + required_error: "Environment is required", + invalid_type_error: "Environment is required" + }) .int() .positive("Environment is required"), stackId: z - .number({ invalid_type_error: "Stack is required" }) + .number({ + required_error: "Stack is required", + invalid_type_error: "Stack is required" + }) .int() .positive("Stack is required") }) }) ); ``` 📝 Committable suggestion > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion environmentId: z .number({ required_error: "Environment is required", invalid_type_error: "Environment is required" }) .int() .positive("Environment is required"), stackId: z .number({ required_error: "Stack is required", invalid_type_error: "Stack is required" }) .int() .positive("Stack is required") ``` |
| #7670 2026-10-20 22:44 | coderabbit review-body | — | summary | **Actionable comments posted: 2** |
| #7670 2026-10-20 22:46 | greptile issue-comment | — | summary | Confidence Score: 3/5
**[High impact]** The feature sends selected Infisical secrets into Portainer stacks and can trigger stack redeploys\.
Not safe to merge until both Gateway reliability issues are fixed, because valid Portainer secret syncs can fail during deployment or stop retrying temporary conflicts.
Findings
1. **Long deployments lose their connection**
2. **Temporary conflicts stop retries**
Summary
Adds Portainer app connections and secret syncs for Compose and Swarm stacks, including API tokens, Gateways, Gateway pools, environment and stack pickers, import and removal support, tests, and setup guides.
Two Gateway reliability issues need fixing before merging:
- Portainer … the rest (563 characters)connections can reach instances and list supported stacks. - Portainer syncs write and import stack environment variables. - The sync wizard lets teams choose Portainer environments and stacks. Intentional behavior acknowledged by PierreOucif: destinations are distinguished by Gateway route to allow overlapping instance-local addresses. PierreOucif's guide also states that the sync does not check deployment results after the update request returns. Reviews (1) · Last reviewed commit: "docs(secret-sync): drop the duplicated p..." · Reviewed by the reviewer |
| #7670 2026-10-20 22:46 | greptile inline · P1 | backend/src/services/app-connection/portainer/portainer-connection-fns.ts:175 | defect-functional | **Long deployments lose their connection**
If Portainer takes more than two minutes to redeploy a file-based stack before replying, this Gateway request loses its connection even though the update allows four minutes. The call leaves the Gateway's two-minute idle timeout enabled. The connection closes while deployment is still running, and the sync can ultimately fail despite Portainer completing the deployment. Request retries can delay the final failure, but do not prevent the premature disconnect.
Enable `longLived` for these longer Gateway requests so the bounded request timeout controls the wait. This needs fixing before merging.
Artifacts
**Executed Portainer Gateway timeout test** … the rest (820 characters)- The authored script executes the real sync and Gateway helpers against a local TLS fixture, explicitly showing the stubs, time scaling, and in-memory fix. **Fixture startup log before ALPN correction** - The initial command failed TLS protocol negotiation before reaching an endpoint, documenting a fixture setup failure rather than candidate evidence. **Current code disconnects during a quiet update** - The before command captures real requests, Gateway timeout teardown, deployment-time disconnect, retry, and sync failure, confirming the premature connection loss. **In-memory fix receives the completed update** - The after command repeats the same fixture with longLived enabled and receives HTTP 200 OK, confirming the idle-timeout bypass resolves this reproduction. Ran code and verified through TREX |
| #7670 2026-10-20 22:46 | greptile inline · P1 | backend/src/services/secret-sync/portainer/portainer-sync-fns.ts:117 | defect-functional | **Temporary conflicts stop retries**
When Portainer returns `409` because a deployment is already running, a Gateway-backed sync stops retrying instead of allowing the temporary conflict to clear. `withGatewayV2Proxy` wraps the upstream HTTP error in `BadRequestError`, losing its HTTP status. It then misses the retryable branch in `toStackUpdateError`, and this blanket classification sets `shouldRetry: false`. The queue ends the attempt sequence, leaving the secrets unsynced until another sync runs. The identical response through a direct connection remains retryable.
Preserve the upstream error status or distinguish temporary upstream failures from configuration errors so Gateway syncs re … the rest (89 characters)tain their retries. This needs fixing before merging. Ran code and verified through TREX |
| #7670 2026-10-20 22:47 | cubic inline · P1 | frontend/src/components/secret-syncs/forms/SecretSyncDestinationFields/PortainerSyncFields.tsx:55 | defect-functional | Creating a Portainer connection saves the form before this callback clears the destination IDs, so OAuth restoration pairs the new connection with the old IDs. Those positive IDs pass validation and can target the wrong stack; clear them before storing the form snapshot. |
| #7670 2026-10-20 22:47 | cubic inline · P1 | backend/src/services/secret-sync/portainer/portainer-sync-fns.ts:174 | defect-functional | Portainer 2.45+ Git stacks may expose `GitSourceId` without `GitConfig`, so this guard fails the sync before the existing `sourceID` update path can run. Allow source-backed stacks to build their payload without legacy `GitConfig`. |
| #7670 2026-10-20 22:47 | cubic inline · P1 | backend/src/services/secret-sync/secret-sync-fns.ts:373 | defect-functional | Git-backed Portainer syncs cannot apply changed secrets because `writeGitStackEnv` calls Portainer's `/stacks/{id}/git` update endpoint with `POST`; use `PUT` for that endpoint. |
| #7670 2026-10-20 22:47 | cubic inline · P1 | backend/src/services/secret-sync/portainer/portainer-sync-fns.ts:98 | defect-functional | Legacy Portainer Git updates omit `repositoryURL`, so the POST replaces the stack’s repository URL with an empty value. Preserve `gitConfig.URL` in the non-`sourceID` payload; otherwise later deployments cannot clone the stack. |
| #7670 2026-10-20 22:47 | cubic inline · P1 | backend/src/services/app-connection/portainer/portainer-connection-fns.ts:130 | defect-security | Reject non-HTTPS URLs before making this direct request; it sends the `X-API-Key` header over cleartext HTTP. |
| #7670 2026-10-20 22:47 | cubic inline · P2 | backend/src/services/secret-sync/portainer/portainer-sync-fns.ts:99 | defect-functional | For legacy Git stacks using inline credentials, this payload never sends `auth.Password`, so the update can discard credentials and break the next deployment. Include `repositoryPassword` when the returned password is present.
```suggestion
...(auth?.Password !== undefined ? { repositoryPassword: auth.Password } : {}),
repositoryUsername: auth?.Username ?? "",
``` |
| #7670 2026-10-20 22:47 | cubic inline · P2 | backend/src/services/secret-sync/portainer/portainer-sync-fns.ts:92 | defect-functional | Git updates omit `configFilePath`, so stacks using a non-default compose file lose their compose path when this replacement payload is saved. Include `gitConfig.ConfigFilePath` in the payload.
```suggestion
repositoryReferenceName: gitConfig.ReferenceName ?? "",
configFilePath: gitConfig.ConfigFilePath ?? "",
``` |
| #7670 2026-10-20 22:47 | cubic inline · P3 | docs/integrations/secret-syncs/portainer.mdx:80 | docs | The initial-sync section omits that enabling **Include secrets from all subfolders** makes both import options invalid. Add that users must choose **Overwrite Portainer** in this case; otherwise API users can follow the documented combination and receive validation errors.
```suggestion
Under **Initial Sync Behavior**, choose how Infisical handles variables that already exist on the stack on the first sync. If **Include secrets from all subfolders** is enabled, choose **Overwrite Portainer** because the import options aren't available:
``` |
| #7670 2026-10-20 22:47 | cubic review-body | — | summary | **9 issues found** across 91 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #7670 2026-10-20 22:47 | cubic inline · P3 | frontend/src/components/secret-syncs/forms/schemas/portainer-sync-destination-schema.ts:11 | defect-functional | Set `required_error` on both number schemas; clearing either selection currently shows Zod’s generic “Required” instead of the field-specific message. |
| #8669 2026-10-20 22:51 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The changes add private-key guidance for Palo Alto Networks certificate syncs. The create and edit forms display the guidance for Palo Alto Networks destinations. The documentation describes private-key prerequisites and certificate handling.
**Changes: Palo Alto Networks guidance**
|Layer / File(s)|Summary|
|:---|:---|
|**Define and surface private-key guidance** `frontend/src/helpers/pkiSyncs.ts`, `frontend/src/components/pki-syncs/forms/CreatePkiSyncForm.tsx`, `frontend/src/components/pki-syncs/forms/EditPkiSyncForm.tsx`, `docs/documentation/platform/pki/applications/certificate-syncs/palo-alto-networks.mdx`, `docs/documentation/platform/pki/applications/certificate-sync … the rest (2741 characters)s/palo-alto-networks-ssl-tls-profile.mdx`|Adds shared guidance that certificates without private keys are skipped. The create and edit forms append the guidance for Palo Alto Networks destinations. The documentation describes private-key prerequisites, including the exception for imported issuing CA certificates.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~8 minutes **Change:** Other **Suggested reviewers:** `carlosmonastyrski` Merge Risk: 🔵 Low · up to ad3ac General Palo Alto Networks users may incorrectly treat valid issuing CA certificates as unusable because the form gives them an overly broad private-key requirement. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description check | | The description clearly explains the private-key requirement, the skip behavior, and the Palo Alto Networks context. It includes the required Context and Type sections, and the checklist is mostly com… | | Title check | | The title clearly and concisely describes the main change: documenting that Palo Alto Networks syncs require certificate private keys. It uses a valid conventional commit format. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description clearly explains the private-key requirement, the skip behavior, and the Palo Alto Networks context. It includes the required Context and Type sections, and the checklist is mostly complete. The Screenshots and Steps to verify sections are empty, but these omissions are non-critical for this documentation-focused change. Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8669 2026-10-20 22:52 | cubic inline · P2 | docs/documentation/platform/pki/applications/certificate-syncs/palo-alto-networks-ssl-tls-profile.mdx:21 | docs | This makes the CSR's origin sound like the blocker, so users who import the matching private key are told their certificate will be skipped. State that the sync skips certificates only when their private key is unavailable in Infisical.
```suggestion
An SSL/TLS service profile can only use a certificate that was imported with its private key. If a certificate was issued from a CSR generated elsewhere and its private key isn't available in Infisical, the sync can't deploy it and skips it. See [Imported Certificate Not Showing Under SSL/TLS Service Profile](https://knowledgebase.paloaltonetworks.com/KCSArticleDetail?id=kA10g000000PLmJCAW).
``` |
| #8669 2026-10-20 22:52 | cubic inline · P2 | frontend/src/components/pki-syncs/forms/CreatePkiSyncForm.tsx:384 | defect-functional | This adds the warning only to the `lg:flex` sidebar, which is hidden on smaller viewports, so mobile users never see it. Render the note in the certificate step's main content or otherwise make it visible responsively. |
| #8669 2026-10-20 22:52 | cubic review-body | — | summary | **2 issues found** across 5 files
Reply with feedback, questions, or to request a fix.View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8669 2026-10-20 22:55 | coderabbit inline · P2 | frontend/src/helpers/pkiSyncs.ts:205 | defect-functional | **🎯 Functional Correctness** | **🟡 Minor** | **⚡ Quick win**
**Scope the private-key requirement to deployed certificates.**
The general Palo Alto Networks sync can import issuing CA certificates without private keys. The shared note currently says every certificate needs a private key, so both create and edit forms show incorrect guidance. Update the shared constant; both forms consume it.
Suggested fix
```diff
--- "a/frontend/src/helpers/pkiSyncs.ts"
+++ "b/frontend/src/helpers/pkiSyncs.ts"
@@ -201,8 +201,8 @@
PkiSync.PaloAltoNetworksSslTlsProfile
];
export const PALO_ALTO_NETWORKS_PRIVATE_KEY_NOTE =
- "Palo Alto Networks needs each certificate's private key. Certificates wit … the rest (1095 characters)hout one, such as those issued from a CSR generated outside Infisical, are skipped."; + "Palo Alto Networks needs each deployed certificate's private key. Issuing CA certificates imported alongside do not need one. Other certificates without one, such as those issued from a CSR generated outside Infisical, are skipped."; export const isPaloAltoNetworksPkiSync = (destination?: PkiSync) => destination === PkiSync.PaloAltoNetworks || destination === PkiSync.PaloAltoNetworksSslTlsProfile; ``` 📝 Committable suggestion > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion export const PALO_ALTO_NETWORKS_PRIVATE_KEY_NOTE = "Palo Alto Networks needs each deployed certificate's private key. Issuing CA certificates imported alongside do not need one. Other certificates without one, such as those issued from a CSR generated outside Infisical, are skipped."; ``` |
| #8669 2026-10-20 22:55 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8669 2026-10-20 23:02 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** Safe to merge. The changes add documentation and destination-specific guidance without changing certificate syncing behavior.
Summary
Documents that Palo Alto Networks certificate syncs need the certificate's private key.
- Palo Alto sync forms explain when certificates are skipped.
No actionable issues were found. The change is safe to merge.
Reviews (1) · Last reviewed commit: "chore: Add PAN private key warning on UI..." · Reviewed by the reviewer |
| #8676 2026-10-20 23:03 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The organization/project switcher now uses a responsive maximum width in project scope. It hides organization badges in project scope and uses shorter badge labels elsewhere.
**Changes: Switcher presentation**
|Layer / File(s)|Summary|
|:---|:---|
|**Scope-based switcher presentation** `frontend/src/layouts/OrganizationLayout/components/NavBar/Navbar.tsx`|In project scope, the switcher uses a maximum width of `max-w-52` and omits organization badges. In other scopes, badges remain visible with the labels “Org” and “Sub.”|
**Priority:** ⬇️ Low
**Estimated code review effort:** 1 (Trivial) | ~5 minutes
**Chang … the rest (1735 characters)e:** Bug fix **Suggested reviewers:** `scott-ray-wilson` Merge Risk: ⚪ Minimal · up to 49443 The scoped UI presentation changes match the stated objectives and are ready to merge. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly describes the main UI change and follows the conventional commit format. | | Description check | | The description is complete and matches the repository template. It explains the prior and new behavior, includes screenshots, verification steps, change type, and checklist status. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8676 2026-10-20 23:04 | cubic review-body | — | summary | **No issues found** across 1 file
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8676 2026-10-20 23:14 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** Safe to merge. No blocking issues or new layout regressions were established.
Summary
Keeps the org or sub-org name visible in project and product scope by replacing the fixed 72px org cell with `max-w-52`. Badges are hidden in those scopes, and organization-scope labels are shortened to “Org” and “Sub”.
Browser checks covered 98 before-and-after cases across organization, product, and project scope, including root organizations, sub-organizations, long names, and three viewport widths. Badge assertions passed, and no new layout regression was established. These checks used the actual Navbar and frontend styles with mocked service dependencies, rath … the rest (253 characters)er than an authenticated deployment. No actionable issues found. Safe to merge. - The header shows the org name in project and product scope. Reviews (1) · Last reviewed commit: "fix(ui): keep the org name visible in pr..." · Reviewed by the reviewer |
| #8675 2026-10-20 23:16 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Sheet and Dialog footers were updated across the frontend to use right alignment and consistent action ordering. The changes also add footer guidance, update Sheet stories, and adjust organization identity creation button variants.
**Changes: Footer alignment and action ordering**
|Layer / File(s)|Summary|
|:---|:---|
|**Apply footer layout conventions** `DESIGN.md`, `frontend/src/components/v3/generic/Sheet/Sheet.stories.tsx`, `frontend/src/pages/cert-manager/.../CertificateTemplateEnrollmentModal.tsx`, `frontend/src/pages/project/RoleDetailsBySlugPage/components/*`, `frontend/src/pages/project/components/Fold … the rest (3756 characters)erAccessSection/EditFolderAccessSheet.tsx`, `frontend/src/pages/secret-manager/OverviewPage/components/SecretDropzone/*`, `frontend/src/pages/secret-manager/SecretDashboardPage/.../VaultSecretImportModal.tsx`|Added footer layout guidance and applied right alignment in Sheet stories and selected certificate, role, folder, and secret-management views. The CSV mapping dialog keeps Back on the left.| |**Update identity and role form actions** `frontend/src/pages/organization/AccessManagementPage/.../IdentitySection/*`, `frontend/src/pages/project/AccessControlPage/.../IdentityTab/...`, `frontend/src/pages/project/IdentityDetailsByIDPage/.../IdentityRoleModify.tsx`, `frontend/src/pages/project/MemberDetailsByIDPage/.../MemberMultiRoleModify.tsx`|Right-aligned form footers and moved Cancel before the primary action. Organization identity creation selects its button variant using `isSubOrganization`.| |**Update approval and settings actions** `frontend/src/pages/organization/ProductSettingsPage/.../HoneyTokenModal.tsx`, `frontend/src/pages/secret-manager/SecretApprovalsPage/components/*`, `frontend/src/pages/secret-manager/SettingsPage/.../SecretValidationRulesSection.tsx`, `frontend/src/views/Alerts/AlertForm.tsx`|Right-aligned approval, policy, validation, alert, and honey-token actions. Several forms now place Cancel or Reject before the primary action.| **Priority:** ⬇️ Low **Estimated code review effort:** 2 (Simple) | ~12 minutes **Change:** Bug fix **Suggested reviewers:** `andrewhuhh`, `scott-ray-wilson` Merge Risk: ⚪ Minimal · up to 19de7 The footer changes have no identified material regression and are mergeable after normal checks. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly summarizes the primary UI change and follows the conventional commit format. | | Description check | | The description includes the required context, screenshots, verification steps, change type, and checklist. It clearly documents the scope and behavior of the footer standardization. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 23 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) ### Usage-based review receipt - Mode: Continue automatically - Reviewed files: 24 - Waived: $6.00 (charged $0.00) - View usage details > [!NOTE] > This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. **Manage usage-based reviews**. > [!WARNING] > ⚠️ This pull request shows signs of AI-generated slop (description_diff_mismatch). It has been flagged by the reviewer slop detection and should be reviewed carefully. Comment `@the reviewer help` to get the list of available commands. |
| #8675 2026-10-20 23:23 | cubic review-body | — | summary | **No issues found** across 24 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8675 2026-10-20 23:24 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Medium impact]** Safe to merge. The changes preserve button handlers, form associations, and submit types, while the tested Sheet story retains accessible actions and working dismissal at desktop and mobile widths.
Summary
Standardizes sheet footers without changing their submit or cancel behavior.
- Sheet footers put their actions on the right, with Cancel before the main action.
- Organization identity creation matches its button style to the organization scope.
- Access review choices sit at the right, with rejection before approval.
Browser checks of the updated Sheet story confirmed right-aligned actions without overlap or overflow at 1440px, 375px, and 320p … the rest (271 characters)x viewport widths. Close continued to dismiss the sheet, and the footer remained anchored while its content scrolled. No actionable issues found. Safe to merge. Reviews (1) · Last reviewed commit: "fix(ui): right-align sheet footers with ..." · Reviewed by the reviewer |
| #8660 2026-10-20 23:26 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The agent reference adds the `file-output.combine-certificate-chain` option and documents how combined certificate-and-chain output interacts with chain paths, root omission, reloads, and restarts.
**Changes: Full-chain output documentation**
|Layer / File(s)|Summary|
|:---|:---|
|**Document combined certificate-and-chain output** `docs/documentation/platform/pki/reference/infisical-agent.mdx`|The reference describes the new option, which writes the certificate and chain to `certificate.path`. It documents that `chain.path` is ignored when the option is enabled, while `chain.omit-root` remains effective. It als … the rest (2878 characters)o covers reload and restart behavior.| **Priority:** ⬇️ Low **Estimated code review effort:** 1 (Trivial) | ~4 minutes **Change:** Other **Suggested reviewers:** `adkah` Merge Risk: ⚪ Minimal · up to bd6a1 The reference explains how to reload NGINX after certificate renewal. No actionable issue remains before merge. Architecture risk: 🔵 Low · up to bd6a1 The change affects 1 system. **Changed systems:** `docs` **Architecture concerns** No architecture-level concerns identified. Review details **Systems and components** - _observed_ — docs (service) was modified; 1 changed file maps to changed impact. **Before / after behavior** - _observed_ — Modified behavior in docs/documentation/platform/pki/reference/infisical-agent.mdx: Added the `file-output.combine-certificate-chain` configuration option, which writes the certificate followed by its chain to `certificate.path`; when enabled, `chain.path` is ignored while `chain.omit-root` remains effective. - _observed_ — Modified behavior in docs/documentation/platform/pki/reference/infisical-agent.mdx: Added a full-chain certificate file section documenting combined certificate-and-chain output, including configuration, server integration, post-event reloads, root CA inclusion control, and restart-time rewrite and issuance-hook behavior. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly describes the documentation change and follows the conventional commit format. | | Description check | | The description includes context, verification steps, change type, and checklist information. It accurately identifies the documentation-only scope and dependency on the CLI release. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8660 2026-10-20 23:27 | cubic review-body | — | summary | **No issues found** across 1 file
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8660 2026-10-20 23:31 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** No blocking defects identified; merge remains subject to PierreOucif's stated requirement to wait for the supporting CLI release.
Summary
Documents `file-output.combine-certificate-chain` and adds a full-chain example for web servers.
- The agent can write a full certificate chain to one file.
No blocking defects identified. Merge remains subject to the stated CLI-release prerequisite.
### the check validation blocked
The documentation preview could not start because the existing `docs.json` fails schema validation: `logo.href` must be a valid URL, and contextual options `assistant` and `mcp` are unsupported by the cached Mint version. The same fai … the rest (279 characters)lure occurred with the merge-base page and the updated page. The new table row, full-chain example, and anchor navigation therefore remain unverified in a rendered page. Reviews (1) · Last reviewed commit: "docs(cert-agent): keep server examples t..." · Reviewed by the reviewer |
| #8485 2026-10-20 23:33 | coderabbit issue-comment | — | summary | 📝 Walkthrough
Adds S3-compatible app connections for supported and configured storage endpoints. Adds admin-managed hostname allowlists and extends PAM session recordings to use AWS or S3-compatible connections.
**Changes: S3-Compatible Storage and PAM Recordings**
|Layer / File(s)|Summary|
|:---|:---|
|**Storage hostname configuration** `backend/src/db/migrations/*`, `backend/src/db/schemas/super-admin.ts`, `backend/src/server/routes/v1/admin-router.ts`, `backend/src/server/plugins/serve-ui.ts`, `backend/src/lib/config/env.ts`, `frontend/src/pages/admin/GeneralPage/components/GeneralPageForm.tsx`, `frontend/src/hooks/api/admin/types.ts`, `frontend/index.html`, `docs/documentation/platfo … the rest (7902 characters)rm/admin-panel/server-admin.mdx`, `docs/self-hosting/configuration/envars.mdx`|Adds persisted and API-managed allowed storage hostnames, an admin form, and HTTPS origins for configured storage hosts in the UI CSP. Adds the internal-IP environment setting.| |**S3-compatible connections** `backend/src/services/app-connection/*`, `backend/src/server/routes/v1/app-connection-routers/*`, `backend/src/lib/aws/s3.ts`, `backend/src/lib/api-docs/constants.ts`, `frontend/src/hooks/api/appConnections/*`, `frontend/src/helpers/appConnections.ts`, `backend/e2e-test/fakes/s3.ts`|Adds S3-compatible connection schemas, endpoint and credential validation, API registration, and frontend connection types and provider resolution. Adds a shared S3 client factory.| |**PAM recording storage** `backend/src/ee/services/pam-session-recording/*`, `backend/src/ee/services/pam-account-template/*`, `backend/src/ee/services/pam-account/*`, `backend/src/ee/services/pam/pam-validators.ts`, `frontend/src/hooks/api/pam/*`, `docs/documentation/platform/pam/sessions/external-storage.mdx`, `backend/src/ee/services/pam/CLAUDE.md`|Resolves AWS or S3-compatible recording access and accepts inherited recording configuration. Recording deletion uses batches of individual object deletion requests.| |**Connection and recording forms** `frontend/src/pages/organization/AppConnections/AppConnectionsPage/components/AppConnectionForm/*`, `frontend/src/pages/pam/PamAccountsPage/components/*`, `frontend/src/pages/pam/PamTemplatesPage/components/TemplateDetailSheet.tsx`|Adds S3-compatible connection creation and editing forms. PAM account and template forms can select AWS or S3-compatible recording connections and omit region settings for S3-compatible connections.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~60 minutes **Change:** Feature ### Sequence Diagram(s) ```mermaid sequenceDiagram participant AdminForm as GeneralPageForm participant AdminAPI as admin-router participant ConnectionForm as S3CompatibleConnectionForm participant ConnectionAPI as app-connection-router participant CredentialValidation as validateS3CompatibleConnectionCredentials participant PAM as PAM recording services participant Storage as S3 storage AdminForm->>AdminAPI: save allowedStorageHostnames AdminAPI->>AdminAPI: validate and store hostname configuration ConnectionForm->>ConnectionAPI: create or update S3-compatible connection ConnectionAPI->>CredentialValidation: validate endpoint and credentials CredentialValidation->>Storage: call ListBuckets PAM->>PAM: resolve recording connection and configuration PAM->>Storage: send recording storage requests ``` Merge Risk: 🔵 Low · up to b528b Switching an account from AWS to S3-compatible recording storage can make a save fail when the bucket is cleared to inherit the template setting. The issue is narrow and has a straightforward fix. Security architecture risk: 🟡 Moderate · up to b528b Custom storage destinations expand where recording data and storage requests can travel. Endpoint restrictions, access checks, and playback integrity controls provide meaningful protection, but upload-client behavior and failure recovery remain incompletely established. No newly introduced exploitable bypass was established. **Retained concerns** No architecture-level concerns identified. Security review details **Security Blast Radius** - _inferred_ — The new destination policy is instance-wide, while recording connection selection remains organization-bound and requires Connect permission on an organization-scoped connection. An approved custom destination can serve multiple authorized organizations; enabling private-IP access expands reachable backend network scope without itself granting cross-organization recording access. **Trust Boundaries and Controls** - _observed_ — Backend DNS pinning protects backend storage requests, not clients consuming presigned URLs. Presigning does not invoke the configured agent, and browser playback makes its own fetch. The external uploader’s network enforcement remains a material coverage gap, not a verified SSRF finding. **Resilience and Maintainability Implications** - _observed_ — External upload and metadata persistence remain separate operations. Duplicate metadata inserts are ignored, and insertion does not verify the stored object. These behaviors predate this PR; playback integrity checks detect corruption but do not establish upload completion or recover orphaned objects. The inspected deletion implementation supports paginated, repeatable object deletes, but its production lifecycle owner remains unestablished. **Hardening Proposals** - _proposed_ — Establish the actual upload consumer’s DNS, redirect, TLS, retry, and completion guarantees before treating backend endpoint validation as an end-to-end network control. Validate SDK redirect behavior separately. - _proposed_ — For the inherited recording lifecycle, consider durable storage provenance, explicit upload-completion reconciliation, and an identified cleanup owner so destination changes and interrupted writes have defined recovery behavior. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :---------------- | :----------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 42 files. (5 skipped: 5… | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: adding S3-compatible storage for PAM session recordings. It follows the conventional commit format. | | Description check | | The description includes relevant context, a UI screenshot, the feature classification, checklist items, and an autogenerated implementation summary. The Steps to verify section is empty, and some con… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description includes relevant context, a UI screenshot, the feature classification, checklist items, and an autogenerated implementation summary. The Steps to verify section is empty, and some conditional checklist items remain unchecked, but the description is mostly complete and directly related to the pull request. Full details: Docstring Coverage **Explanation** Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 42 files. (5 skipped: 5 unsupported.) **Resolution** Write docstrings for the functions missing them to satisfy the coverage threshold. ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8485 2026-10-20 23:42 | cubic inline · P2 | backend/src/ee/services/pam-account/pam-account-service.ts:945 | defect-functional | Key this on changes to the effective recording configuration, not only the patched connection ID: moving an account with an existing connection override to a template with an inherited bucket takes the false branch, so the bucket is not validated and `corsProbeUrl` is null. The UI then skips the bucket CORS check.
```suggestion
recordingConnectionId !== undefined || templateId !== undefined || settingsOverrides !== undefined
``` |
| #8485 2026-10-20 23:42 | cubic inline · P2 | frontend/src/pages/organization/AppConnections/AppConnectionsPage/components/AppConnectionForm/S3CompatibleConnectionForm.tsx:141 | defect-functional | The `Method` label targets `id="method"`, but this trigger has no matching ID, so assistive technology cannot associate the label with the selector. Add `id="method"` to the trigger.
```suggestion
<SelectTrigger id="method" className="w-full" isError={Boolean(error)}>
``` |
| #8485 2026-10-20 23:42 | cubic inline · P2 | frontend/src/helpers/appConnections.ts:710 | defect-functional | This misses password-only URL userinfo because `username` is empty for `https://:pass@host`; the form accepts it, but backend endpoint validation rejects it. Also reject `url.password` here.
```suggestion
url.username || url.password ||
``` |
| #8485 2026-10-20 23:42 | cubic inline · P2 | frontend/src/helpers/appConnections.ts:681 | defect-functional | This accepts any `s3.<label>.amazonaws.com` as AWS, so a typo such as `s3.fake-region.amazonaws.com` passes the form and is rejected by backend region validation. Restrict the match to supported AWS regions. |
| #8485 2026-10-20 23:42 | cubic review-body | — | summary | **4 issues found** across 48 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8485 2026-10-20 23:42 | coderabbit inline · P2 | frontend/src/pages/pam/PamAccountsPage/components/AccountDetailSheet.tsx:638 | defect-functional | **🗄️ Data Integrity & Integration** | **🟡 Minor** | **⚡ Quick win**
**Ignore the hidden region for S3-compatible overrides.**
When the selected connection is S3-compatible and the bucket is empty, clear `region` before calculating `hasS3Override`. Otherwise, a stale AWS region can submit `recordingS3Config: { bucket: "" }`. The PATCH schema rejects the empty bucket, so the save fails.
🐛 Proposed fix
```diff
--- "a/frontend/src/pages/pam/PamAccountsPage/components/AccountDetailSheet.tsx"
+++ "b/frontend/src/pages/pam/PamAccountsPage/components/AccountDetailSheet.tsx"
@@ -628,14 +628,14 @@
const onSubmit = (values: OverridesForm) => {
const bucket = values.recordingBucket.trim … the rest (558 characters)(); - const region = values.recordingRegion.trim(); + const region = isS3CompatibleRecording ? "" : values.recordingRegion.trim(); const keyPrefix = values.recordingKeyPrefix.trim(); const hasS3Override = Boolean(bucket) || Boolean(region); const settingsOverrides = hasS3Override ? { recordingS3Config: { bucket, - ...(region && !isS3CompatibleRecording ? { region } : {}), + ...(region ? { region } : {}), ...(keyPrefix ? { keyPrefix } : {}) } } ``` |
| #8485 2026-10-20 23:42 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8485 2026-10-20 23:43 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[High impact]** The PR extends PAM session recording to configured external storage providers\.
Not safe to merge until clearing a recording connection validates the effective saved configuration.
Findings
1. **Clearing connections breaks recordings**
Summary
Adds S3-compatible storage connections for PAM recordings, supporting AWS S3, Cloudflare R2, Google Cloud Storage, OCI, and admin-allowed custom hosts. Updates connection forms, recording settings, browser access rules, and setup documentation.
One blocking issue remains: clearing an account's recording connection can retain a regionless bucket override while inheriting an AWS connection. The update succee … the rest (446 characters)ds, but subsequent recording uploads and playback fail. Validate the effective connection and retained settings before merging. - S3-compatible connections support AWS S3 and other object-storage services. - PAM recordings can use compatible storage connections. - Server admins can allow custom storage hosts for session playback. Reviews (1) · Last reviewed commit: "add S3\_COMPATIBLE\_STORAGE\_ALLOW\_INTERNAL..." · Reviewed by the reviewer |
| #8485 2026-10-20 23:43 | greptile inline · P1 | backend/src/ee/services/pam-account/pam-account-service.ts:950 | defect-functional | **Clearing connections breaks recordings**
An account can use an S3-compatible connection with a bucket override that has no region. When its template uses AWS, a PATCH containing only `recordingConnectionId: null` skips this check because `null` is falsy. The update succeeds and preserves the regionless bucket override, but recording access now inherits the AWS connection. Upload and playback requests then fail with “Select the AWS region of the recording bucket.”
Validate the effective saved connection and bucket settings whenever the connection changes, including when it is cleared. This recording failure needs to be fixed before merging.
Ran code and verified through TREX |
| #8648 2026-10-20 23:49 | cubic review-body | — | summary | **No issues found** across 2 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8648 2026-10-20 23:54 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The CLI FAQ and command reference now document that login or logout deletes saved offline secrets. They state that offline access resumes after a later secret fetch.
**Changes: Offline secrets documentation**
|Layer / File(s)|Summary|
|:---|:---|
|**Document offline secret lifecycle** `docs/cli/faq.mdx`, `docs/cli/reference.mdx`|The FAQ states that login or logout removes cached secrets and requires a new fetch before offline access resumes. The logout reference clarifies that deletion applies to shared secrets across profiles, and that the next fetch saves them again.|
**Priority:** ⬇️ Low
**Estimated code review effort:** 1 (Trivial) | ~4 minutes
**Change:** Other
**S … the rest (2616 characters)uggested reviewers:** `victorvhs017` Merge Risk: 🔵 Low · up to 7cd62 The FAQ may mislead machine-identity users about offline-secret availability; clarify the conditions before merging. Architecture risk: 🔵 Low · up to 7cd62 The change affects 1 system. **Changed systems:** `docs` **Architecture concerns** No architecture-level concerns identified. Review details **Systems and components** - _observed_ — docs (service) was modified; 2 changed files map to changed impact. **Before / after behavior** - _observed_ — Modified behavior in docs/cli/faq.mdx: Added documentation that login or logout removes cached secrets and requires a new fetch before offline access is available again. - _observed_ — Modified behavior in docs/cli/reference.mdx: The `logout` documentation now states that it deletes shared offline secrets for all profiles, regardless of which profiles remain logged in, and that they are saved again on the next secret fetch. Pre-merge checks: 5 | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the primary documentation change: documenting that logout deletes the offline secrets backup. It follows the conventional commit format. | | Description check | | The description includes the required context, verification steps, change type, and checklist. It accurately explains the logout behavior and its effect on offline secrets shared across profiles. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8648 2026-10-20 23:55 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** No blocking defects were identified; merge remains subject to PierreOucif's acknowledged requirement that Infisical/cli#433 be released first.
Summary
Documents that `infisical logout` deletes secrets saved for offline use.
- The CLI docs explain when saved secrets disappear and return.
PierreOucif explicitly states that these docs should merge after Infisical/cli#433 is released, because the reference describes the new logout behavior. This remains an acknowledged release dependency, not a reported defect.
No blocking issues were found in the documentation changes. Both complete MDX files compiled successfully, and focused browser checks confirmed … the rest (523 characters)that the new paragraphs are visible. The changes are suitable to merge once the acknowledged CLI release dependency is satisfied. ### the check validation blocked Full Mintlify preview verification was blocked because the pinned Mintlify version rejects existing docs.json settings for logo.href and the contextual options assistant and mcp. Focused rendering succeeded, but full-site behavior was not verified. Reviews (1) · Last reviewed commit: "docs(cli): note that logout deletes the ..." · Reviewed by the reviewer |
| #8648 2026-10-20 23:58 | coderabbit inline · P2 | docs/cli/faq.mdx:66 | docs | **🎯 Functional Correctness** | **🟡 Minor** | **⚡ Quick win**
**Qualify the offline-backup lifecycle.**
Machine-identity `infisical login` does not delete the existing backup. Also, only a successful logged-in fetch that requests secret values recreates it. The current sentence can mislead users about offline availability.
Suggested fix
```diff
--- "a/docs/cli/faq.mdx"
+++ "b/docs/cli/faq.mdx"
@@ -62,7 +62,7 @@
<Accordion title="Can I fetch secrets with Infisical while I'm offline?">
Yes. If you have previously retrieved secrets for a specific project and environment (such as dev, staging, or prod), the `run`/`secret` command will utilize the saved secrets, even when offline, on subs … the rest (908 characters)equent fetch attempts. -Running `infisical login` or `infisical logout` deletes the saved secrets, so they're only available offline again after your next fetch. +Running `infisical login` for a user profile or `infisical logout` deletes the saved secrets. The saved secrets become available offline again after a successful logged-in fetch that requests secret values. </Accordion> ``` 📝 Committable suggestion > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion Running `infisical login` for a user profile or `infisical logout` deletes the saved secrets. The saved secrets become available offline again after a successful logged-in fetch that requests secret values. ``` |
| #8648 2026-10-20 23:58 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8699 2026-10-21 00:00 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
Release workflows now run a reusable freeze check before changelog generation and release jobs. The check validates the deployment-freeze JSON setting, permits releases when `frozen` is false, and blocks them when the setting is invalid or `frozen` is true.
**Changes: Release workflow gating**
|Layer / File(s)|Summary|
|:---|:---|
|**Reusable freeze validation** `.github/workflows/release-freeze-check.yml`|The reusable workflow validates that the input is valid JSON with one object and a boolean `frozen` field. It succeeds when `frozen` is false. It fails for missing or malformed input, or when a freeze is acti … the rest (4481 characters)ve.| |**Freeze checks in release workflows** `.github/workflows/generate-release-changelog.yml`, `.github/workflows/helm-release-infisical-core.yml`, `.github/workflows/helm-release-infisical-nkp.yml`, `.github/workflows/release-standalone-docker-img-postgres-offical.yml`, `.github/workflows/release_helm_gateway.yaml`|The workflows pass `vars.DEPLOY_FREEZE` to the reusable check. Changelog generation and release jobs now depend on the check. Release jobs retain their test jobs as prerequisites to the check.| **Priority:** ⬇️ Low **Estimated code review effort:** 3 (Moderate) | ~20 minutes **Change:** Other Merge Risk: ⚪ Minimal · up to 81b16 Release workflows now pause for a deployment-freeze check after their existing tests. Valid unfrozen settings allow releases to proceed. Active or invalid settings block releases, as intended. No merge-blocking issue remains. Architecture risk: 🔵 Low · up to 81b16 The changed surface does not map to a changed system, dependency edge, entrypoint, or external dependency. **Changed systems:** None identified. **Architecture concerns** No architecture-level concerns identified. Review details **Before / after behavior** - _observed_ — Modified behavior in .github/workflows/generate-release-changelog.yml: Adds a release freeze-check job using `vars.DEPLOY_FREEZE` and makes `generate_changelog` wait for its completion via `needs`. - _observed_ — Modified behavior in .github/workflows/helm-release-infisical-core.yml: Added a `freeze-check` job that runs after `test-helm`, invokes the release-freeze check workflow, and supplies `vars.DEPLOY_FREEZE` as its `freeze` input. - _observed_ — Modified behavior in .github/workflows/helm-release-infisical-core.yml: Changed the `release` job dependency from `test-helm` to `freeze-check`. - _observed_ — Modified behavior in .github/workflows/helm-release-infisical-nkp.yml: Added the `freeze-check` job, requiring `test-helm` and passing `DEPLOY_FREEZE` to the reusable release-freeze workflow. Changed `release` to depend on `freeze-check` instead of directly on `test-helm`. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :--------------- | :----------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Description check | | The description explains the change and classifies it as a chore, but the Steps to verify section is empty and the checklist is entirely unchecked. No test results are provided. | Add concrete verification steps and test results. Complete the checklist by confirming the conventional title, local testing status, documentation status, CLAUDE.md status, and contributing guide review. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: validating the freeze flag before release workflows. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (6 skipped: 6 unsupported.) ✨ Finishing Touches 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8699 2026-10-21 00:02 | cubic review-body | — | summary | **No issues found** across 6 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8699 2026-10-21 00:06 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[High impact]** Safe to merge. The release-freeze check blocks frozen or invalid inputs while preserving existing test prerequisites.
Summary
Adds a shared release-freeze check to Docker, Helm, and changelog workflows.
- Release workflows check the freeze setting before running release work.
No actionable issues were found. The change is safe to merge based on the executed shell checks and local dependency checks. A hosted release was not executed.
Reviews (1) · Last reviewed commit: "move to before the actual release step" · Reviewed by the reviewer |
| #8710 2026-10-21 00:08 | cubic review-body | — | summary | **No issues found** across 2 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8710 2026-10-21 00:08 | coderabbit issue-comment | — | summary | No actionable comments were generated in the recent review. 🎉
📝 Walkthrough
The project-template eligibility list and organization settings links no longer include PAM. The settings list retains KMS.
**Changes: Project-template scope**
|Layer / File(s)|Summary|
|:---|:---|
|**Limit project-template products** `frontend/src/helpers/project.ts`, `frontend/src/pages/organization/SettingsPage/components/OrgProductSettingsTab/OrgProductSettingsTab.tsx`|The project helper and organization settings list no longer include PAM for project templates. The settings list retains KMS.|
**Priority:** ⬇️ Low
**Estimated code review effort:** 1 (Trivial) | ~5 minutes
**Change:** Bug fix
**Suggested … the rest (3297 characters)reviewers:** `mathnogueira` Merge Risk: ⚪ Minimal · up to 7550e The change correctly removes PAM project-template support without affecting KMS. Pre-merge checks: 4 · 1 Failed checks: 1 warning | Check name | Status | Explanation | Resolution | | :--------------- | :----------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | Description check | | The description identifies the change through the generated summary and marks the PR as a fix, but the required Context and Steps to verify the change sections are empty. The screenshots are also plac… | Add a concise Context section that explains the previous and new behavior. Add reproducible verification steps. Move the images under the Screenshots section or remove that section if not needed. Complete the applicable checklist items, inc… | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: removing PAM from the project templates list. | | Docstring Coverage | | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Docstring Coverage **Explanation** No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. Full details: Description check **Explanation** The description identifies the change through the generated summary and marks the PR as a fix, but the required Context and Steps to verify the change sections are empty. The screenshots are also placed before the Screenshots heading, and the checklist remains largely unchecked. **Resolution** Add a concise Context section that explains the previous and new behavior. Add reproducible verification steps. Move the images under the Screenshots section or remove that section if not needed. Complete the applicable checklist items, including local testing and the title-format confirmation. ✨ Finishing Touches 📝 Generate docstrings 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8710 2026-10-21 00:14 | greptile issue-comment | — | summary | Confidence Score: 5/5
**[Low impact]** Safe to merge within the reviewed scope; the intended PAM removal preserves supported template settings.
Summary
Removes PAM from project-template settings.
- PAM no longer appears in shared project-template settings.
No actionable issues found. Safe to merge within the reviewed scope.
Reviews (1) · Last reviewed commit: "remove PAM from secret template in org s..." · Reviewed by the reviewer |
| #8637 2026-10-21 00:16 | coderabbit issue-comment | — | summary | 📝 Walkthrough
The change adds AWS KMS-backed signing keys for internal certificate authorities. It introduces a shared key-source service for AWS KMS and HSM, updates CA storage and lifecycle operations, and adds licensing, API, interface, and documentation support.
**Changes: External CA key sources**
|Layer / File(s)|Summary|
|:---|:---|
|**Key-source contracts and persistence** `backend/src/db/migrations/*`, `backend/src/db/schemas/certificate-authority-secret.ts`, `backend/src/services/certificate-authority/internal/*schemas.ts`, `backend/src/services/certificate-authority/ca-key-source/*types.ts`, `backend/src/services/signer/signer-enums.ts`, `backend/src/ee/services/license/*`, `b … the rest (10516 characters)ackend/src/services/license-client/feature-mapping.ts`, `backend/src/lib/api-docs/constants.ts`, `frontend/src/hooks/api/*`|The CA secret storage and configuration contracts now include external key identifiers, public-key data, AWS connection IDs, and regions. The key-source enum and license feature mapping include AWS KMS.| |**External provider signing** `backend/src/services/certificate-authority/ca-key-source/*`, `backend/src/services/certificate-authority/ca-signer.ts`, `backend/src/services/certificate-authority/ca-key-source/aws-kms-ca-key-source.test.ts`, `backend/src/services/certificate/certificate-fns.ts`|A shared service dispatches key operations to HSM or AWS KMS providers. The AWS KMS provider creates and describes keys, signs CA artifacts, and prepares key release. External signer helpers reuse ephemeral keys; tests cover AWS KMS signing, algorithm handling, permissions, and release behavior.| |**CA creation, signing, and key release** `backend/src/services/certificate-authority/internal/internal-certificate-authority-service.ts`, `backend/src/services/certificate-authority/certificate-authority-{fns,service,secret-dal}.ts`, `backend/src/services/project/project-cleanup-queue.ts`, `backend/src/services/app-connection/app-connection-service.ts`, `backend/src/services/cert-manager-export/cert-manager-export-service.ts`, `backend/CLAUDE.md`|CA workflows now create and use external keys through the key-source service. CA and project deletion prepare and execute key releases. Project lookup exposes externally keyed CA secrets; export rejects CAs whose external connections cannot be recreated.| |**CA service and route integration** `backend/src/server/routes/*`, `backend/src/services/certificate-authority/certificate-authority-queue.ts`, `backend/src/services/certificate-profile/*`, `backend/src/services/certificate-v3/*`, `backend/src/services/certificate/certificate-service.ts`, `backend/src/services/pki-application/*`, `backend/src/ee/services/certificate-authority-ocsp/*`, `backend/src/ee/services/pki-scep/pki-scep-fns.ts`|Certificate, SCEP, OCSP, queue, and route dependencies now use the key-source service. Internal CA CSR signing resolves provider-required signature algorithms. CA audit events include key-source metadata.| |**CA setup and key-source display** `frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/CreateCaWizard/*`, `frontend/src/pages/cert-manager/CertAuthDetailsByIDPage/components/CaDetailsSection.tsx`, `frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/CaSection.tsx`, `docs/documentation/platform/pki/ca/private-ca.mdx`, `.infisicalignore`|The CA wizard supports AWS connection and region selection, license gating, and source-specific algorithms. CA details and deletion confirmation display AWS KMS key information. The documentation describes AWS KMS setup and deletion timing.| **Priority:** ➖ Normal **Estimated code review effort:** 4 (Complex) | ~60 minutes **Change:** Feature ### Sequence Diagram(s) ```mermaid sequenceDiagram participant User participant CreateCaWizard participant InternalCertificateAuthorityService participant CaKeySourceService participant AWSKMS participant CertificateAuthoritySecretDAL User->>CreateCaWizard: Select AWS KMS, connection, and region CreateCaWizard->>InternalCertificateAuthorityService: Submit CA configuration InternalCertificateAuthorityService->>CaKeySourceService: Create external CA key CaKeySourceService->>AWSKMS: Create key and retrieve public key AWSKMS-->>CaKeySourceService: Key reference and public key CaKeySourceService-->>InternalCertificateAuthorityService: Key details and signer InternalCertificateAuthorityService->>CertificateAuthoritySecretDAL: Persist external key reference ``` **Suggested reviewers:** `carlosmonastyrski`, `sheensantoscapadngan`, `saifsmailbox98` Merge Risk: 🔵 Low · up to 407d6 This change adds AWS KMS-backed signing keys for internal CAs and moves HSM handling onto a shared key-source layer. No correctness or security defect was identified. One efficiency issue remains: each AWS KMS signing operation rebuilds its AWS client and may refresh credentials first. That can add latency, STS throttling risk and AWS cost on high-volume issuance or OCSP traffic. The change is mergeable, with that follow-up recommended. Security architecture risk: 🟡 Moderate · up to 407d6 AWS-backed signing preserves important setup authorization and key-selection controls. However, failed or interrupted cleanup can remove a certificate authority or project without retiring its AWS signing key. The change also requires coordinated schema rollback and appropriately restricted AWS permissions. **Retained concerns** - **Medium · security · inferred:** AWS key retirement is not durably coupled to CA ownership. CA deletion and project hard deletion remove owning database records before executing in-memory release callbacks, while release preparation and execution errors are suppressed. An AWS credential or permission failure, provider failure, or interruption can therefore leave a signing key unreleased after deletion succeeds, without the owning record needed for normal retry. Creation has a related orphan window between AWS allocation and database persistence. Any subsequent signing still requires AWS authority; this is a lifecycle-security concern, not an unauthenticated signing bypass. Security review details **Security Blast Radius** - _inferred_ — The retained lifecycle concern affects the AWS key of an individual CA, or the collected external CA keys during project hard deletion. A surviving key still requires AWS signing authority to use. Application operations target stored key ARNs, but maximum authority of the underlying AWS principal cannot be bounded without deployed IAM policies. **Security Findings and Attack Paths** - _inferred_ — An authorized deletion can complete despite failed AWS retirement because both preparation and release errors are suppressed. Interruption after database deletion produces a similar recovery gap. This can retain signing capability outside the application's CA inventory; it does not demonstrate that an unprivileged caller gains signing permission. **Trust Boundaries and Controls** - _observed_ — CA creation checks project Create permission and provider usage authorization before allocating a key. Connection authorization checks organization ownership, AWS type, Connect permission, and project affinity. Generic CA deletion checks Delete permission and profile dependencies. Runtime signing delegates through a persisted server-created key reference rather than accepting a fresh caller-selected ARN. **Resilience and Maintainability Implications** - _observed_ — Normal failure containment includes duplicate-name prechecking before allocation, transactional CA-secret persistence, attempted cleanup when public-key retrieval or later creation fails, and reading the CA secret from the primary database before deletion. These controls reduce ordinary failure windows but do not durably recover lost or failed release callbacks. **Hardening Proposals** - _proposed_ — Persist key-retirement intent and ownership information before deleting the CA or project, retaining the key ARN and credential reference until retirement is confirmed. Use retryable, idempotent recovery with explicit failed and pending-deletion states, and make allocation recoverable across interruption before CA persistence. - _proposed_ — Validate deployed role trust and KMS policies against the intended organization, project, key, and operation scope. Consider durable ownership tags for reconciliation. These are deployment assurances and hardening proposals, not findings that existing AWS permissions are excessive. Pre-merge checks: 4 · 1 Failed checks: 1 inconclusive | Check name | Status | Explanation | Resolution | | :---------------- | :--------------------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- | | Docstring Coverage | | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 50 files. (6 skipped: 3… | Write docstrings for the functions missing them to satisfy the coverage threshold. | Passed checks: 4 passed | Check name | Status | Explanation | | :------------------------ | :--------------------------------------------------------------------------------------------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Title check | | The title clearly and concisely describes the main change: adding AWS KMS-backed signing keys for internal CAs. It follows the conventional commit format. | | Description check | | The description provides clear context, identifies the feature and documentation changes, and completes the checklist. The Steps to verify section is empty, and the Screenshots placeholder remains des… | | Linked Issues check | | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | | Check skipped because no linked issues were found for this pull request. | Full details: Description check **Explanation** The description provides clear context, identifies the feature and documentation changes, and completes the checklist. The Steps to verify section is empty, and the Screenshots placeholder remains despite UI changes, but the description is otherwise mostly complete. Full details: Docstring Coverage **Explanation** Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 50 files. (6 skipped: 3 unsupported, 3 over the file limit.) **Resolution** Write docstrings for the functions missing them to satisfy the coverage threshold. ✨ Finishing Touches: 💡 1 📝 Generate docstrings: 💡 🧪 Generate unit tests (beta) Comment `@the reviewer help` to get the list of available commands. |
| #8637 2026-10-21 00:24 | cubic inline · P2 | backend/CLAUDE.md:1030 | docs | `pkiAwsKmsSigning` is enforced only by the AWS KMS provider; the HSM provider's `createKey` calls `generateKeyPair` without an entitlement check. Describe the check as provider-specific so future providers do not inherit the AWS-only gate.
```suggestion
`createKey` (provider-specific entitlement checks and key creation; returns the columns to store) and `buildSigner` (returns a `TCaSigner`, usually through
``` |
| #8637 2026-10-21 00:24 | cubic inline · P2 | backend/src/services/certificate-authority/ca-key-source/ca-key-source-service.ts:106 | defect-data | AWS credential, network, or API failures are swallowed here, so CA deletion can succeed while its KMS key remains active with no retry path. Persist a cleanup retry or otherwise retain the key reference until deletion is scheduled. |
| #8637 2026-10-21 00:24 | cubic inline · P2 | docs/documentation/platform/pki/ca/private-ca.mdx:288 | docs | The new AWS KMS algorithms contradict the page’s global “What key algorithms are supported?” answer, which still lists only RSA 2048/4096 and ECDSA P-256/P-384. Update the FAQ and distinguish AWS KMS support from the other key sources so readers see the correct options. |
| #8637 2026-10-21 00:24 | cubic inline · P2 | backend/src/db/migrations/20261012152847_ca-secret-external-key.ts:57 | defect-data | Rolling back after an AWS KMS CA has been created fails here because the replacement check rejects every `keySource = 'aws-kms'` row. Handle or explicitly guard these rows before attempting the legacy constraint so rollback does not fail at constraint creation. |
| #8637 2026-10-21 00:24 | cubic inline · P2 | backend/src/services/certificate-authority/certificate-authority-secret-dal.ts:14 | defect-data | This query reads from a replica, so replication lag can hide an external CA secret from the one-time cleanup; the project cascade then removes its reference without scheduling KMS deletion. Default to the primary.
```suggestion
(tx || db.primaryNode())(TableName.CertificateAuthoritySecret)
``` |
| #8637 2026-10-21 00:24 | cubic review-body | — | summary | **7 issues found** across 56 files
View guided diff | Turn on auto-fix | Re-trigger the reviewer |
| #8637 2026-10-21 00:24 | cubic inline · P2 | backend/src/services/certificate-authority/internal/internal-certificate-authority-schemas.ts:94 | defect-security | The deprecated create endpoint still accepts caller-supplied read-only key metadata through this shared schema. Apply the read-only refinement to its create schema too. |
| #8637 2026-10-21 00:24 | cubic inline · P2 | backend/src/services/certificate-authority/ca-key-source/aws-kms-ca-key-source.test.ts:240 | tests | This skips the ECDSA KMS regression test whenever OpenSSL lacks ML-DSA support. Use Node crypto for the ECDSA fake signer or gate this test on ECDSA-specific availability so CI still exercises the path without PQC. |
| #8637 2026-10-21 00:24 | coderabbit inline · P2 | backend/src/services/certificate-authority/ca-key-source/aws-kms-ca-key-source.ts:188 | defect-performance | **🚀 Performance & Scalability** | **🟡 Minor** | **⚡ Quick win**
**Cache the KMS client across signers; do not rebuild it on every `buildSigner` call.**
`client` lives inside `buildSigner`. Each `getCaSigner` call therefore runs `$getKmsClient` again for an AWS KMS CA. That function reads the app connection, decrypts it, and calls `getAwsConnectionConfig`. For assume-role connections, that is an STS `AssumeRole` round trip before every KMS `Sign`.
Every issuance, CRL rebuild and OCSP miss pays this cost. The OCSP responder is unauthenticated, and nonced requests are never cached. An anonymous caller can therefore drive one decrypt, one STS call and one KMS `Sign` per request. On self-hos … the rest (1590 characters)ted instances, only the signing concurrency cap limits this. The cost is latency, STS throttling risk and KMS charges. Keep a provider-level cache keyed by `appConnectionId` and region. Give it a TTL shorter than the 15-minute assumed-role session. Drop the entry when a send fails with a credential error. ♻️ Proposed change ```diff --- "a/backend/src/services/certificate-authority/ca-key-source/aws-kms-ca-key-source.ts" +++ "b/backend/src/services/certificate-authority/ca-key-source/aws-kms-ca-key-source.ts" @@ -183,9 +183,8 @@ }); } - let client: KMSClient | undefined; const sign = async (message: Buffer, messageType: MessageType, signingAlgorithm: SigningAlgorithmSpec) => { - client ??= await $getKmsClient(appConnectionId, parseAwsKmsKeyArnRegion(keyArn)); + const client = await $getCachedKmsClient(appConnectionId, parseAwsKmsKeyArnRegion(keyArn)); try { const { Signature } = await client.send( new SignCommand({ ``` ```ts // at factory scope const KMS_CLIENT_TTL_MS = 10 * 60 * 1000; const kmsClients = new Map<string, { client: Promise<KMSClient>; expiresAt: number }>(); const $getCachedKmsClient = (appConnectionId: string, region: string) => { const key = `${appConnectionId}:${region}`; const hit = kmsClients.get(key); if (hit && hit.expiresAt > Date.now()) return hit.client; const client = $getKmsClient(appConnectionId, region); kmsClients.set(key, { client, expiresAt: Date.now() + KMS_CLIENT_TTL_MS }); client.catch(() => kmsClients.delete(key)); return client; }; ``` |
| #8637 2026-10-21 00:24 | coderabbit review-body | — | summary | **Actionable comments posted: 1** |
| #8637 2026-10-21 00:31 | greptile issue-comment | — | summary | Confidence Score: 4/5
**[Medium impact]** The changes reach internal CA setup\, signing\, and key cleanup\.
Do not merge until approval and fresh-CSR renewal consistently resolve the certificate signature algorithm for AWS KMS-backed CAs.
Findings
1. **CSR approval and renewal fail**
Summary
This PR adds AWS KMS-backed signing keys for internal CAs.
- Internal CAs can create signing keys in AWS KMS and use them through an AWS connection.
- CA signing workflows send signature requests through the selected key source.
- External CA keys use shared storage and are released when their CA is deleted.
- The CA wizard lets administrators choose AWS KMS and see the key details.
**Fix before … the rest (431 characters)merging:** CSR approval and fresh-CSR renewal still need the signature correction used by initial signing. An AWS KMS P-384 CA accepts a request under a SHA384-only policy, but approval rejects the same request under the unchanged policy. Fresh-CSR renewal also fails, either during policy validation or at the KMS provider's digest check. Reviews (1) · Last reviewed commit: "Add alias to AWS KMS keys" · Reviewed by the reviewer |
| #8637 2026-10-21 00:31 | greptile inline · P1 | backend/src/services/certificate-v3/certificate-v3-service.ts:1435 | defect-functional | **CSR approval and renewal fail**
This correction runs only during initial CSR signing. For an AWS KMS P-384 CA, submission resolves a SHA-256 CSR to `ECDSA-SHA384`, so a policy permitting only `ECDSA-SHA384` accepts the request. Approval then reconstructs `ECDSA-SHA256` from the original CSR and rejects the same request under the unchanged policy.
Fresh-CSR renewal also skips this correction: the SHA384-only policy rejects it, and even when the policy permits SHA256, signing fails because the KMS P-384 provider requires SHA384. Apply the same CA-aware resolution before approval and renewal validation, and pass the resolved algorithm through to signing. This must be fixed before merging be … the rest (1443 characters)cause accepted requests cannot be approved and existing certificates cannot be renewed through this path. Artifacts **Before control responses with the base submission service** - Ran the exact base submission service against the same downstream contracts and captured HTTP responses, commands, exit codes, source excerpts, and the complete executed harness; submission rejects the CSR before approval. **Head responses showing accepted submission and failing approval and renewal** - Ran unmodified head services and captured HTTP responses, call arguments, commands, exit codes, source excerpts, and the complete executed harness; approval and renewal fail without downstream algorithm normalization. **Executed CSR approval and renewal regression checks** - Generated real CSR and CA fixtures and exercised real service functions through local HTTP adapters with mocked infrastructure; both final runs passed assertions reproducing the supplied failure. **Executed configuration for the base-submission and head controls** - Loaded the exact base submission source only for the before control while retaining identical downstream services; the comparison isolates submission normalization. **Executed command and evidence capture script** - Executed both controls and saved their actual combined output with reproducible metadata and source; both captured Vitest processes exited successfully. Ran code and verified through TREX |