mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-10-19 02:28:59 +00:00
Fix operator-state null capabilityNames crash and stopped-container gating
Issue #1621: saving only "intentionally offline" left the SQLite auto_remediation_policy_json column NULL, and the read path only normalized the policy inside the non-NULL branch, so the API served "capabilityNames": null. The section's dirty-state memo spread that value during render and crashed the expanded row into the route error boundary. Normalize the policy unconditionally after the scan so the wire always carries [], type capabilityNames as nullable in the TS API surface, and guard the spread in the component. Issue #1622: the inline table-row presentation gated the entire operator-overrides section on the resource exposing an auto-authorizable capability, so stopped Docker containers (which only expose start, never auto-authorized) lost intentionally-offline and never-auto-remediate exactly when they matter most. Render the section whenever the resource has an id; the automatic-actions block already self-gates on eligible capabilities. Also add min-w-0 to the section's flex text columns and overflow-x-clip to the inline row content box so the long never-auto-remediate copy wraps instead of escaping the row border below the lg breakpoint. Regression coverage: issue-named Go test for the NULL-column read path and a render test feeding the section the real pre-fix wire payload with capabilityNames: null. Fixes #1621 Fixes #1622 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
0b9385f2f2
commit
7f4db466be
14 changed files with 215 additions and 26 deletions
|
|
@ -4458,6 +4458,12 @@ normalizes the server's `404 operator_state_not_set` response into
|
|||
than a thrown error; non-404 errors propagate. The PUT path
|
||||
percent-encodes the canonical resource id segment so colon-bearing
|
||||
ids round-trip safely through URL routing.
|
||||
Operator-state reads must keep `autoRemediationPolicy.capabilityNames`
|
||||
iterable. SQLite rows whose policy column is `NULL` normalize to an empty
|
||||
capability list before JSON serialization, while the TS compatibility type and
|
||||
rendering path continue to tolerate `null` from pre-fix or cached payloads.
|
||||
Saving only `intentionallyOffline`, `neverAutoRemediate`, priority, or note
|
||||
must therefore never produce a drawer-breaking operator-state response.
|
||||
|
||||
The router wires the operator-state adapter into the findings runtime
|
||||
at startup: `internal/api/router.go` calls
|
||||
|
|
|
|||
|
|
@ -3688,7 +3688,10 @@ Inline detail table rows are also registry-backed. `InlineDetailTableRow`
|
|||
owns the row/cell/content shell and row-click containment for platform,
|
||||
workload, and infrastructure inline drawers; callers may pass row-specific
|
||||
`data-*` attributes, colspan, and content classes, but they must not recreate
|
||||
the surface-alt detail row shell locally.
|
||||
the surface-alt detail row shell locally. The content shell must clip
|
||||
horizontal paint below the large breakpoint without becoming a scroll
|
||||
container, then restore visible overflow for the static desktop layout, so
|
||||
long operator-state copy cannot escape the shared row border.
|
||||
Inline detail section content is registry-backed separately from the row shell.
|
||||
`DetailSectionTable`, `InlineDetailPanel`, and `detailSectionModel.ts` own
|
||||
detail row compaction, section-table rendering, value-tone classes, and the
|
||||
|
|
|
|||
|
|
@ -1790,7 +1790,11 @@ capability is never eligible, low-risk eligible, or elevated eligible; it does
|
|||
not lower `MinimumApprovalLevel`. The first eligible vertical is Docker/Podman
|
||||
container `restart` at `low_risk`; unspecified capabilities normalize to never.
|
||||
`NormalizeResourceOperatorState` trims whitespace, de-duplicates capability
|
||||
names, and lower-cases the criticality value before persistence. The
|
||||
names, guarantees a non-nil empty capability list, and lower-cases the
|
||||
criticality value before persistence. SQLite reads must apply that
|
||||
normalization even when `auto_remediation_policy_json` is `NULL`, so an
|
||||
operator state containing only intentionally-offline or another independent
|
||||
override serializes `capabilityNames: []` rather than `null`. The
|
||||
`ResourceStore` interface now exposes
|
||||
`GetResourceOperatorState`, `SetResourceOperatorState`, and
|
||||
`ClearResourceOperatorState`; both the SQLite (table
|
||||
|
|
@ -1821,7 +1825,10 @@ overview tab, which routes through the canonical TS client at
|
|||
`frontend-modern/src/api/resourceOperatorState.ts` to
|
||||
`/api/resources/{id}/operator-state`. Alongside the existing operator-state
|
||||
controls, the drawer offers automatic actions only for backend-advertised
|
||||
eligible capabilities. Enabling it requires selecting exact capabilities and
|
||||
eligible capabilities. The operator-state section itself remains available for
|
||||
every canonical resource id, including stopped containers and resources with
|
||||
no auto-authorizable capability; only the automatic-actions subsection is
|
||||
capability-gated. Enabling it requires selecting exact capabilities and
|
||||
may add a recurring daily time/timezone window; copy must state that this is a
|
||||
resource allowlist and that the tenant Patrol mode remains the upper bound.
|
||||
The UI must never infer eligibility from capability names, severity, or the
|
||||
|
|
|
|||
|
|
@ -44,6 +44,23 @@ describe('resourceOperatorState api', () => {
|
|||
await expect(getResourceOperatorState('vm:101')).resolves.toBeNull();
|
||||
});
|
||||
|
||||
it('preserves a legacy null capability list for compatibility consumers to normalize', async () => {
|
||||
apiFetchJSONMock.mockResolvedValueOnce({
|
||||
canonicalId: 'docker:host-1/ct-nginx',
|
||||
intentionallyOffline: true,
|
||||
neverAutoRemediate: false,
|
||||
autoRemediationPolicy: {
|
||||
enabled: false,
|
||||
capabilityNames: null,
|
||||
},
|
||||
setAt: '2026-07-20T10:00:00Z',
|
||||
} satisfies ResourceOperatorState);
|
||||
|
||||
const result = await getResourceOperatorState('docker:host-1/ct-nginx');
|
||||
|
||||
expect(result?.autoRemediationPolicy?.capabilityNames).toBeNull();
|
||||
});
|
||||
|
||||
it('rethrows non-404 errors so the caller can surface them to the operator', async () => {
|
||||
apiFetchJSONMock.mockRejectedValueOnce(
|
||||
Object.assign(new Error('Internal server error'), { status: 500 }),
|
||||
|
|
|
|||
|
|
@ -17,7 +17,12 @@ export interface AutoRemediationWindow {
|
|||
|
||||
export interface AutoRemediationPolicy {
|
||||
enabled: boolean;
|
||||
capabilityNames: string[];
|
||||
/**
|
||||
* Nullable on the wire: rows persisted before the store normalized the
|
||||
* policy on read carry `"capabilityNames": null` (issue #1621). Treat
|
||||
* null the same as [].
|
||||
*/
|
||||
capabilityNames: string[] | null;
|
||||
window?: AutoRemediationWindow;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -355,11 +355,6 @@ export const ResourceDetailDrawerOverviewTab: Component<ResourceDetailDrawerOver
|
|||
drawer.resourceTimelineCount() > 0 ||
|
||||
Boolean(drawer.facetBundleError());
|
||||
const shouldRenderOperationalGovernanceSections = () => !compactTableRow();
|
||||
const shouldRenderOperatorStateSection = () =>
|
||||
!compactTableRow() ||
|
||||
resource.capabilities?.some(
|
||||
(capability) => capability.autoAuthorization && capability.autoAuthorization !== 'never',
|
||||
);
|
||||
const shouldRenderActionHistorySection = () =>
|
||||
drawer.actionAuditAvailable() &&
|
||||
(!compactTableRow() ||
|
||||
|
|
@ -697,8 +692,13 @@ export const ResourceDetailDrawerOverviewTab: Component<ResourceDetailDrawerOver
|
|||
never auto-remediate) sits next to the action history so
|
||||
the "what overrides has the operator set" and "what actions
|
||||
has Pulse taken" stories read together. The section is
|
||||
self-fetching keyed on the canonical resource id. */}
|
||||
<Show when={shouldRenderOperatorStateSection() && resource.id}>
|
||||
self-fetching keyed on the canonical resource id. Rendered
|
||||
for every resource with an id — the operator overrides
|
||||
(intentionally offline, never auto-remediate, note, priority)
|
||||
don't depend on capabilities; only the automatic-actions
|
||||
block does, and it gates itself inside the section
|
||||
(issue #1622). */}
|
||||
<Show when={resource.id}>
|
||||
<ResourceOperatorStateSection
|
||||
resourceId={resource.id}
|
||||
capabilities={resource.capabilities}
|
||||
|
|
|
|||
|
|
@ -139,7 +139,9 @@ export const ResourceOperatorStateSection: Component<ResourceOperatorStateSectio
|
|||
capabilityNames: [],
|
||||
};
|
||||
const selected = [...autoCapabilities()].sort();
|
||||
const persistedSelected = [...persistedAuto.capabilityNames].sort();
|
||||
// capabilityNames can arrive as null from rows persisted before the
|
||||
// policy column was always written (issue #1621) — never assume [].
|
||||
const persistedSelected = [...(persistedAuto.capabilityNames ?? [])].sort();
|
||||
const windowChanged = autoWindowEnabled()
|
||||
? !persistedAuto.window ||
|
||||
persistedAuto.window.timezone !== autoWindowTimezone() ||
|
||||
|
|
@ -536,7 +538,7 @@ export const ResourceOperatorStateSection: Component<ResourceOperatorStateSectio
|
|||
= +1h) or with the persisted window when one exists. */}
|
||||
<Show when={!schedulerOpen()}>
|
||||
<div class="flex items-center justify-between gap-3 pt-2 border-t border-border-subtle">
|
||||
<div class="flex-1">
|
||||
<div class="min-w-0 flex-1">
|
||||
<label class="text-sm font-medium text-base-content">Maintenance window</label>
|
||||
<p class="text-[11px] text-muted mt-0.5 leading-tight">
|
||||
Suspend findings on this resource for a defined window. Useful for scheduled upgrades,
|
||||
|
|
@ -673,7 +675,7 @@ export const ResourceOperatorStateSection: Component<ResourceOperatorStateSectio
|
|||
<Show when={eligibleAutoCapabilities().length > 0}>
|
||||
<div class="space-y-3 border-t border-border-subtle pt-3" aria-label="Automatic actions">
|
||||
<div class="flex items-start justify-between gap-3">
|
||||
<div class="flex-1">
|
||||
<div class="min-w-0 flex-1">
|
||||
<label class="text-sm font-medium text-base-content">Automatic actions</label>
|
||||
<p class="mt-0.5 text-[11px] leading-tight text-muted">
|
||||
Choose exactly which actions Patrol may run on this resource. Your Patrol mode, live
|
||||
|
|
@ -780,7 +782,7 @@ export const ResourceOperatorStateSection: Component<ResourceOperatorStateSectio
|
|||
</Show>
|
||||
|
||||
<div class="flex items-start justify-between gap-3">
|
||||
<div class="flex-1">
|
||||
<div class="min-w-0 flex-1">
|
||||
<label class="text-sm font-medium text-base-content">Intentionally offline</label>
|
||||
<p class="text-[11px] text-muted mt-0.5 leading-tight">
|
||||
Suppress findings on this resource. Use when a workload is deprecated, a dev environment
|
||||
|
|
@ -795,7 +797,7 @@ export const ResourceOperatorStateSection: Component<ResourceOperatorStateSectio
|
|||
</div>
|
||||
|
||||
<div class="flex items-start justify-between gap-3 pt-2 border-t border-border-subtle">
|
||||
<div class="flex-1">
|
||||
<div class="min-w-0 flex-1">
|
||||
<label class="text-sm font-medium text-red-700 dark:text-red-400">
|
||||
Never auto-remediate
|
||||
</label>
|
||||
|
|
|
|||
|
|
@ -366,7 +366,10 @@ describe('ResourceDetailDrawer change history section', () => {
|
|||
expect(screen.getByText('Current state')).toBeInTheDocument();
|
||||
expect(screen.getByText('Identity')).toBeInTheDocument();
|
||||
expect(screen.queryByText('Change history')).not.toBeInTheDocument();
|
||||
expect(screen.queryByText('Operator overrides')).not.toBeInTheDocument();
|
||||
// Operator overrides render in every presentation (issue #1622) —
|
||||
// intentionally-offline and never-auto-remediate must stay reachable
|
||||
// even for resources with no auto-authorizable capabilities.
|
||||
expect(screen.getByText('Operator overrides')).toBeInTheDocument();
|
||||
expect(screen.queryByText('Maintenance verification')).not.toBeInTheDocument();
|
||||
expect(screen.queryByText('Action history')).not.toBeInTheDocument();
|
||||
expect(screen.queryByText('Refreshing changes...')).not.toBeInTheDocument();
|
||||
|
|
|
|||
|
|
@ -0,0 +1,94 @@
|
|||
import { describe, expect, it, vi } from 'vitest';
|
||||
import { render, screen, waitFor } from '@solidjs/testing-library';
|
||||
|
||||
import type { ResourceCapability } from '@/types/resource';
|
||||
|
||||
const apiClientMock = vi.hoisted(() => ({
|
||||
apiFetchJSON: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock('@/utils/apiClient', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('@/utils/apiClient')>();
|
||||
return {
|
||||
...actual,
|
||||
apiFetchJSON: apiClientMock.apiFetchJSON,
|
||||
};
|
||||
});
|
||||
|
||||
import { ResourceOperatorStateSection } from '@/components/Infrastructure/ResourceOperatorStateSection';
|
||||
|
||||
// Regression for issue #1621: rows saved with only "intentionally offline"
|
||||
// left the auto_remediation_policy_json column NULL, and the pre-fix read
|
||||
// path skipped normalization for NULL columns, so the wire carried
|
||||
// "capabilityNames": null. The section's dirty-state memo spread that value
|
||||
// during render, throwing "null is not iterable" straight into the route
|
||||
// error boundary. These tests feed the section the real pre-fix wire
|
||||
// payload through the canonical API client to pin the null tolerance.
|
||||
const OPERATOR_STATE_WITH_NULL_CAPABILITIES = JSON.parse(
|
||||
`{
|
||||
"canonicalId": "docker:host-1/ct-nginx",
|
||||
"intentionallyOffline": true,
|
||||
"neverAutoRemediate": false,
|
||||
"autoRemediationPolicy": {"enabled": false, "capabilityNames": null},
|
||||
"setAt": "2026-07-20T10:00:00Z",
|
||||
"setBy": "admin"
|
||||
}`,
|
||||
) as unknown;
|
||||
|
||||
const RESTART_CAPABILITY: ResourceCapability[] = [
|
||||
{
|
||||
name: 'restart',
|
||||
type: 'docker',
|
||||
description: 'Restart the container',
|
||||
autoAuthorization: 'low_risk',
|
||||
},
|
||||
];
|
||||
|
||||
describe('ResourceOperatorStateSection render with capabilityNames: null', () => {
|
||||
it('renders the persisted record without crashing and reports a clean (not dirty) state', async () => {
|
||||
apiClientMock.apiFetchJSON.mockResolvedValue(OPERATOR_STATE_WITH_NULL_CAPABILITIES);
|
||||
|
||||
render(() => (
|
||||
<ResourceOperatorStateSection
|
||||
resourceId="docker:host-1/ct-nginx"
|
||||
capabilities={RESTART_CAPABILITY}
|
||||
/>
|
||||
));
|
||||
|
||||
// The section itself must be up (pre-fix, the throw escaped render
|
||||
// and nothing below the header survived).
|
||||
expect(screen.getByText('Operator overrides')).toBeTruthy();
|
||||
expect(screen.getByText('Intentionally offline')).toBeTruthy();
|
||||
expect(screen.getByText('Never auto-remediate')).toBeTruthy();
|
||||
|
||||
// "Clear all overrides" only renders for persisted() && !isDirty() —
|
||||
// reaching it proves the dirty-state memo evaluated the null
|
||||
// capabilityNames without throwing and treated it as [].
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText('Clear all overrides')).toBeTruthy();
|
||||
});
|
||||
expect(screen.getByText(/Set by admin/)).toBeTruthy();
|
||||
expect(screen.queryByText('Save overrides')).toBeNull();
|
||||
});
|
||||
|
||||
it('hydrates a null capability allowlist as an empty selection in the automatic-actions block', async () => {
|
||||
apiClientMock.apiFetchJSON.mockResolvedValue(OPERATOR_STATE_WITH_NULL_CAPABILITIES);
|
||||
|
||||
render(() => (
|
||||
<ResourceOperatorStateSection
|
||||
resourceId="docker:host-1/ct-nginx-2"
|
||||
capabilities={RESTART_CAPABILITY}
|
||||
/>
|
||||
));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText('Clear all overrides')).toBeTruthy();
|
||||
});
|
||||
|
||||
// The automatic-actions block renders (the resource has an eligible
|
||||
// capability) with its toggle off and no phantom selection derived
|
||||
// from the null allowlist.
|
||||
expect(screen.getByText('Automatic actions')).toBeTruthy();
|
||||
expect(screen.queryByText('Allowed actions')).toBeNull();
|
||||
});
|
||||
});
|
||||
|
|
@ -197,12 +197,16 @@ describe('ResourceDetailDrawerOverviewTab integration', () => {
|
|||
expect(operatorIndex).toBeLessThan(historyIndex);
|
||||
});
|
||||
|
||||
it('keeps eligible automatic actions reachable from compact platform rows', () => {
|
||||
expect(overviewTabSource).toContain('shouldRenderOperatorStateSection');
|
||||
expect(overviewTabSource).toContain('capability.autoAuthorization');
|
||||
expect(overviewTabSource).toContain("capability.autoAuthorization !== 'never'");
|
||||
expect(overviewTabSource).toContain(
|
||||
'<Show when={shouldRenderOperatorStateSection() && resource.id}>',
|
||||
);
|
||||
it('renders operator overrides for every resource regardless of capability eligibility', () => {
|
||||
// Issue #1622: the section used to be gated on the resource exposing
|
||||
// an auto-authorizable capability, which hid intentionally-offline /
|
||||
// never-auto-remediate exactly when they matter most (a stopped
|
||||
// container only exposes `start`, which is never auto-authorized).
|
||||
// The overrides don't depend on capabilities — only the
|
||||
// automatic-actions block does, and it self-gates inside the section
|
||||
// via eligibleAutoCapabilities.
|
||||
expect(overviewTabSource).toContain('<Show when={resource.id}>');
|
||||
expect(overviewTabSource).not.toContain('shouldRenderOperatorStateSection');
|
||||
expect(sectionSource).toContain('<Show when={eligibleAutoCapabilities().length > 0}>');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -3,8 +3,11 @@ import { splitProps, type JSX } from 'solid-js';
|
|||
import { TableCell, TableRow, type TableRowProps } from './Table';
|
||||
|
||||
export const INLINE_DETAIL_TABLE_CELL_CLASS = 'p-0 border-b border-border bg-surface-alt';
|
||||
// overflow-x-clip keeps over-wide content from painting outside the row's
|
||||
// border below the lg breakpoint (issue #1622) without creating a scroll
|
||||
// container the way overflow-hidden would.
|
||||
export const INLINE_DETAIL_TABLE_CONTENT_CLASS =
|
||||
'sticky left-0 max-w-[calc(100vw-3.5rem)] px-2 py-3 sm:px-4 sm:py-4 lg:static lg:max-w-none';
|
||||
'sticky left-0 max-w-[calc(100vw-3.5rem)] overflow-x-clip px-2 py-3 sm:px-4 sm:py-4 lg:static lg:max-w-none lg:overflow-x-visible';
|
||||
|
||||
export interface InlineDetailTableRowProps extends TableRowProps {
|
||||
cellId?: string;
|
||||
|
|
|
|||
|
|
@ -7408,6 +7408,8 @@ describe('shared primitive guardrails', () => {
|
|||
expect(registeredRule?.canonical?.export).toBe('InlineDetailTableRow');
|
||||
expect(inlineDetailTableRowSource).toContain('INLINE_DETAIL_TABLE_CELL_CLASS');
|
||||
expect(inlineDetailTableRowSource).toContain('INLINE_DETAIL_TABLE_CONTENT_CLASS');
|
||||
expect(inlineDetailTableRowSource).toContain('overflow-x-clip');
|
||||
expect(inlineDetailTableRowSource).toContain('lg:overflow-x-visible');
|
||||
expect(inlineDetailTableRowSource).toContain('event.stopPropagation()');
|
||||
expect(registeredRule?.requiredConsumers?.map((consumer) => consumer.path)).toEqual(
|
||||
expect.arrayContaining([
|
||||
|
|
|
|||
|
|
@ -1,6 +1,8 @@
|
|||
package unifiedresources
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
|
@ -84,3 +86,41 @@ func TestAutoRemediationPolicySQLiteRoundTrip(t *testing.T) {
|
|||
t.Fatalf("round-trip policy = %#v", got.AutoRemediationPolicy)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSQLiteOperatorStateNullPolicyColumnNormalizesOnRead(t *testing.T) {
|
||||
store, err := NewSQLiteResourceStore(t.TempDir(), defaultOrgID)
|
||||
if err != nil {
|
||||
t.Fatalf("open store: %v", err)
|
||||
}
|
||||
defer store.Close()
|
||||
|
||||
saved := ResourceOperatorState{
|
||||
CanonicalID: "docker:host-1/ct-nginx",
|
||||
IntentionallyOffline: true,
|
||||
SetAt: time.Date(2026, 7, 20, 10, 0, 0, 0, time.UTC),
|
||||
SetBy: "operator:richard",
|
||||
}
|
||||
if err := store.SetResourceOperatorState(saved); err != nil {
|
||||
t.Fatalf("set: %v", err)
|
||||
}
|
||||
|
||||
got, found, err := store.GetResourceOperatorState(saved.CanonicalID)
|
||||
if err != nil {
|
||||
t.Fatalf("get: %v", err)
|
||||
}
|
||||
if !found {
|
||||
t.Fatal("expected entry after set")
|
||||
}
|
||||
if got.AutoRemediationPolicy.CapabilityNames == nil {
|
||||
t.Fatal("CapabilityNames must be normalized when the policy column is NULL")
|
||||
}
|
||||
|
||||
encoded, err := json.Marshal(got)
|
||||
if err != nil {
|
||||
t.Fatalf("marshal: %v", err)
|
||||
}
|
||||
if strings.Contains(string(encoded), `"capabilityNames":null`) ||
|
||||
!strings.Contains(string(encoded), `"capabilityNames":[]`) {
|
||||
t.Fatalf("wire payload must contain an empty capabilityNames list, got: %s", encoded)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2931,8 +2931,11 @@ func scanResourceOperatorState(scanner resourceOperatorStateScanner) (ResourceOp
|
|||
if err := json.Unmarshal([]byte(autoPolicyJSON.String), &state.AutoRemediationPolicy); err != nil {
|
||||
return ResourceOperatorState{}, fmt.Errorf("unmarshal auto remediation policy: %w", err)
|
||||
}
|
||||
state.AutoRemediationPolicy = NormalizeAutoRemediationPolicy(state.AutoRemediationPolicy)
|
||||
}
|
||||
// Normalize even when the column was NULL (row saved with an empty
|
||||
// policy) so CapabilityNames is never nil and the wire carries [],
|
||||
// not null (issue #1621).
|
||||
state.AutoRemediationPolicy = NormalizeAutoRemediationPolicy(state.AutoRemediationPolicy)
|
||||
if startAt.Valid {
|
||||
t := startAt.Time
|
||||
state.MaintenanceStartAt = &t
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue