From 8552c79942c0ce8879a16eb4bd75881e61506359 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 5 May 2026 20:57:42 +0000 Subject: [PATCH 1/8] chore: update generated API types from latest spec --- docs/openapi/monitoring-api.json | 22 +++++++++++++++------- src/lib/api.generated.ts | 14 +++++++------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/docs/openapi/monitoring-api.json b/docs/openapi/monitoring-api.json index 41903c1..02f8524 100644 --- a/docs/openapi/monitoring-api.json +++ b/docs/openapi/monitoring-api.json @@ -6412,7 +6412,9 @@ "enum": [ "DASHBOARD", "CLI", - "TERRAFORM" + "TERRAFORM", + "MCP", + "API" ] } }, @@ -23307,11 +23309,13 @@ }, "managedBy": { "type": "string", - "description": "Who manages this monitor: DASHBOARD or CLI", + "description": "Source that created/owns this monitor: DASHBOARD, CLI, TERRAFORM, MCP, or API. Use the value matching your surface so audit logs, drift detection, and analytics attribute correctly.", "enum": [ "DASHBOARD", "CLI", - "TERRAFORM" + "TERRAFORM", + "MCP", + "API" ] }, "environmentId": { @@ -27194,11 +27198,13 @@ }, "managedBy": { "type": "string", - "description": "Management source: DASHBOARD or CLI", + "description": "Source that created/owns this monitor: DASHBOARD, CLI, TERRAFORM, MCP, or API", "enum": [ "DASHBOARD", "CLI", - "TERRAFORM" + "TERRAFORM", + "MCP", + "API" ] }, "createdAt": { @@ -33345,12 +33351,14 @@ }, "managedBy": { "type": "string", - "description": "New management source; null preserves current", + "description": "New ownership source: DASHBOARD, CLI, TERRAFORM, MCP, or API; null preserves current value", "nullable": true, "enum": [ "DASHBOARD", "CLI", - "TERRAFORM" + "TERRAFORM", + "MCP", + "API" ] }, "environmentId": { diff --git a/src/lib/api.generated.ts b/src/lib/api.generated.ts index e3b9275..2f2f685 100644 --- a/src/lib/api.generated.ts +++ b/src/lib/api.generated.ts @@ -3188,10 +3188,10 @@ export interface components { /** @description Probe regions to run checks from, e.g. us-east, eu-west */ regions?: string[] | null; /** - * @description Who manages this monitor: DASHBOARD or CLI + * @description Source that created/owns this monitor: DASHBOARD, CLI, TERRAFORM, MCP, or API. Use the value matching your surface so audit logs, drift detection, and analytics attribute correctly. * @enum {string} */ - managedBy: "DASHBOARD" | "CLI" | "TERRAFORM"; + managedBy: "DASHBOARD" | "CLI" | "TERRAFORM" | "MCP" | "API"; /** * Format: uuid * @description Environment to associate with this monitor @@ -4834,10 +4834,10 @@ export interface components { /** @description Probe regions where checks are executed */ regions: string[]; /** - * @description Management source: DASHBOARD or CLI + * @description Source that created/owns this monitor: DASHBOARD, CLI, TERRAFORM, MCP, or API * @enum {string} */ - managedBy: "DASHBOARD" | "CLI" | "TERRAFORM"; + managedBy: "DASHBOARD" | "CLI" | "TERRAFORM" | "MCP" | "API"; /** * Format: date-time * @description Timestamp when the monitor was created @@ -7181,10 +7181,10 @@ export interface components { /** @description New probe regions; null preserves current */ regions?: string[] | null; /** - * @description New management source; null preserves current + * @description New ownership source: DASHBOARD, CLI, TERRAFORM, MCP, or API; null preserves current value * @enum {string|null} */ - managedBy?: "DASHBOARD" | "CLI" | "TERRAFORM" | null; + managedBy?: "DASHBOARD" | "CLI" | "TERRAFORM" | "MCP" | "API" | null; /** * Format: uuid * @description New environment ID; null preserves current (use clearEnvironmentId to unset) @@ -12790,7 +12790,7 @@ export interface operations { /** @description Filter by monitor type */ type?: "HTTP" | "DNS" | "MCP_SERVER" | "TCP" | "ICMP" | "HEARTBEAT"; /** @description Filter by managed-by source */ - managedBy?: "DASHBOARD" | "CLI" | "TERRAFORM"; + managedBy?: "DASHBOARD" | "CLI" | "TERRAFORM" | "MCP" | "API"; /** @description Filter by tag names, comma-separated (e.g. prod,critical) */ tags?: string; /** @description Case-insensitive name search */ From f26d020f48e7b306157b0f278a769b4a23d34275 Mon Sep 17 00:00:00 2001 From: caballeto Date: Wed, 6 May 2026 00:04:41 +0200 Subject: [PATCH 2/8] chore: complete API regen (zod schemas + spec-facts + skill refs) The previous auto-regen commit refreshed `api.generated.ts` and the checked-in OpenAPI spec, but did not propagate the new `MCP` / `API` `managedBy` enum values into the downstream generated artifacts. This runs `npm run zodgen` and `npm run skillgen` so all generated files line up with the spec and the openapi-drift skill test passes. Co-authored-by: Cursor --- .../references/_generated/monitors.fields.md | 6 +++--- src/lib/api-zod.generated.ts | 8 +++++--- src/lib/spec-facts.generated.ts | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/skills/devhelm-configure/references/_generated/monitors.fields.md b/skills/devhelm-configure/references/_generated/monitors.fields.md index 41daeb5..926e687 100644 --- a/skills/devhelm-configure/references/_generated/monitors.fields.md +++ b/skills/devhelm-configure/references/_generated/monitors.fields.md @@ -13,7 +13,7 @@ | `frequencySeconds` | integer (int32) | | ✓ | Check frequency in seconds (30–86400); null defaults to plan minimum (60s on most paid plans) | | `enabled` | boolean | | ✓ | Whether the monitor is active (default: true) | | `regions` | string[] | | ✓ | Probe regions to run checks from, e.g. us-east, eu-west | -| `managedBy` | "DASHBOARD" \| "CLI" \| "TERRAFORM" | ✓ | | Who manages this monitor: DASHBOARD or CLI | +| `managedBy` | "DASHBOARD" \| "CLI" \| "TERRAFORM" \| "MCP" \| "API" | ✓ | | Source that created/owns this monitor: DASHBOARD, CLI, TERRAFORM, MCP, or API. Use the value matching your surface so audit logs, drift detection, and analytics attribute correctly. | | `environmentId` | string (uuid) | | ✓ | Environment to associate with this monitor | | `assertions` | CreateAssertionRequest[] | | ✓ | Assertions to evaluate against each check result | | `auth` | any | | ✓ | | @@ -30,7 +30,7 @@ | `frequencySeconds` | integer (int32) | | ✓ | New check frequency in seconds (30–86400); null preserves current | | `enabled` | boolean | | ✓ | Enable or disable the monitor; null preserves current | | `regions` | string[] | | ✓ | New probe regions; null preserves current | -| `managedBy` | "DASHBOARD" \| "CLI" \| "TERRAFORM" | | ✓ | New management source; null preserves current | +| `managedBy` | "DASHBOARD" \| "CLI" \| "TERRAFORM" \| "MCP" \| "API" | | ✓ | New ownership source: DASHBOARD, CLI, TERRAFORM, MCP, or API; null preserves current value | | `environmentId` | string (uuid) | | ✓ | New environment ID; null preserves current (use clearEnvironmentId to unset) | | `clearEnvironmentId` | boolean | | ✓ | Set to true to remove the environment association | | `assertions` | CreateAssertionRequest[] | | ✓ | Replace all assertions; null preserves current | @@ -52,7 +52,7 @@ | `frequencySeconds` | integer (int32) | ✓ | | Check frequency in seconds (30–86400) | | `enabled` | boolean | ✓ | | Whether the monitor is active | | `regions` | string[] | ✓ | | Probe regions where checks are executed | -| `managedBy` | "DASHBOARD" \| "CLI" \| "TERRAFORM" | ✓ | | Management source: DASHBOARD or CLI | +| `managedBy` | "DASHBOARD" \| "CLI" \| "TERRAFORM" \| "MCP" \| "API" | ✓ | | Source that created/owns this monitor: DASHBOARD, CLI, TERRAFORM, MCP, or API | | `createdAt` | string (date-time) | ✓ | | Timestamp when the monitor was created | | `updatedAt` | string (date-time) | ✓ | | Timestamp when the monitor was last updated | | `assertions` | MonitorAssertionDto[] | | ✓ | Assertions evaluated against each check result; null on list responses | diff --git a/src/lib/api-zod.generated.ts b/src/lib/api-zod.generated.ts index c26bc1f..f427af5 100644 --- a/src/lib/api-zod.generated.ts +++ b/src/lib/api-zod.generated.ts @@ -672,7 +672,7 @@ const CreateMonitorRequest = z frequencySeconds: z.number().int().nullish(), enabled: z.boolean().nullish(), regions: z.array(z.string()).nullish(), - managedBy: z.enum(["DASHBOARD", "CLI", "TERRAFORM"]), + managedBy: z.enum(["DASHBOARD", "CLI", "TERRAFORM", "MCP", "API"]), environmentId: z.string().uuid().nullish(), assertions: z.array(CreateAssertionRequest).nullish(), auth: MonitorAuthConfig.nullish(), @@ -697,7 +697,9 @@ const UpdateMonitorRequest = z frequencySeconds: z.number().int().nullable(), enabled: z.boolean().nullable(), regions: z.array(z.string()).nullable(), - managedBy: z.enum(["DASHBOARD", "CLI", "TERRAFORM"]).nullable(), + managedBy: z + .enum(["DASHBOARD", "CLI", "TERRAFORM", "MCP", "API"]) + .nullable(), environmentId: z.string().uuid().nullable(), clearEnvironmentId: z.boolean().nullable(), assertions: z.array(CreateAssertionRequest).nullable(), @@ -2219,7 +2221,7 @@ const MonitorDto = z frequencySeconds: z.number().int(), enabled: z.boolean(), regions: z.array(z.string()), - managedBy: z.enum(["DASHBOARD", "CLI", "TERRAFORM"]), + managedBy: z.enum(["DASHBOARD", "CLI", "TERRAFORM", "MCP", "API"]), createdAt: z.string().datetime({ offset: true }), updatedAt: z.string().datetime({ offset: true }), assertions: z.array(MonitorAssertionDto).nullish(), diff --git a/src/lib/spec-facts.generated.ts b/src/lib/spec-facts.generated.ts index 9dd7cd3..33740cc 100644 --- a/src/lib/spec-facts.generated.ts +++ b/src/lib/spec-facts.generated.ts @@ -55,7 +55,7 @@ export type SpIncidentStatuses = (typeof SP_INCIDENT_STATUSES)[number] export const AUTH_TYPES = ['bearer', 'basic', 'header', 'api_key'] as const export type AuthTypes = (typeof AUTH_TYPES)[number] -export const MANAGED_BY = ['DASHBOARD', 'CLI', 'TERRAFORM'] as const +export const MANAGED_BY = ['DASHBOARD', 'CLI', 'TERRAFORM', 'MCP', 'API'] as const export type ManagedBy = (typeof MANAGED_BY)[number] export const MATCH_RULE_TYPES = ['severity_gte', 'monitor_id_in', 'region_in', 'incident_status', 'monitor_type_in', 'service_id_in', 'resource_group_id_in', 'component_name_in', 'monitor_tag_in'] as const From 51bd723d3fd8380f69440e36beac492675c536ee Mon Sep 17 00:00:00 2001 From: caballeto Date: Wed, 6 May 2026 00:06:58 +0200 Subject: [PATCH 3/8] fix(monitors): default --regions to us-east for probe-driven types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `devhelm monitors create --type HTTP …` without `--regions` previously hit the API and surfaced an unhelpful `400: At least one region is required for HTTP monitors` (DevEx P1.Bug9). Most users hit this on their first `monitors create` invocation, since region selection is a plan-tier concern they shouldn't have to think about for a sanity check. When `--type` is one of HTTP, TCP, DNS, ICMP, HTTP_HEADLESS, or HTTP_BROWSER and `--regions` is omitted, default to `["us-east"]` and print a one-line stderr notice so the choice isn't invisible. HEARTBEAT (push-based) and MCP_SERVER (region semantics still in flux) are intentionally excluded — both have legitimate "no regions" semantics. Co-authored-by: Cursor --- src/lib/resources.ts | 23 ++++++++++++ test/lib/resources-validation.test.ts | 52 +++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/src/lib/resources.ts b/src/lib/resources.ts index 2bd848d..b293089 100644 --- a/src/lib/resources.ts +++ b/src/lib/resources.ts @@ -113,6 +113,17 @@ export const MONITORS: FullResource = { body.config = buildMonitorConfig(monitorType, raw) if (raw.regions) { body.regions = String(raw.regions).split(',').map((s) => s.trim()).filter(Boolean) + } else if (REGIONS_REQUIRED_TYPES.has(monitorType)) { + // The API rejects probe-driven monitors without at least one + // region (P1.Bug9 — the unhelpful `400: At least one region is + // required for HTTP monitors`). Default to a single region and + // let users know via stderr so the create still succeeds while + // it's obvious where the choice came from. Heartbeat / push + // monitors are excluded because they don't run from probes. + body.regions = [...DEFAULT_MONITOR_REGIONS] + process.stderr.write( + 'note: defaulting --regions us-east; use --regions to override\n', + ) } } else { if (raw.frequency) body.frequencySeconds = Number(raw.frequency) @@ -124,6 +135,18 @@ export const MONITORS: FullResource = { }, } +// Monitor types that the API requires to declare at least one probe +// region. Includes the four shipping types plus the experimental +// browser variants (HTTP_HEADLESS, HTTP_BROWSER) that are wired in the +// CLI ahead of the spec — once they land in MONITOR_TYPES this set +// stays accurate without code changes. MCP_SERVER and HEARTBEAT are +// intentionally excluded: HEARTBEAT is push-driven, and MCP_SERVER's +// region semantics are still in flux. +const REGIONS_REQUIRED_TYPES: ReadonlySet = new Set([ + 'HTTP', 'TCP', 'DNS', 'ICMP', 'HTTP_HEADLESS', 'HTTP_BROWSER', +]) +const DEFAULT_MONITOR_REGIONS: readonly string[] = ['us-east'] + // Returns the monitor's `config` payload as a plain object — the discriminated // union (HttpMonitorConfig | TcpMonitorConfig | …) is enforced by the outer // `apiSchemas.Create/UpdateMonitorRequest` Zod parse, so an unknown `type` diff --git a/test/lib/resources-validation.test.ts b/test/lib/resources-validation.test.ts index c20aa83..15bac4b 100644 --- a/test/lib/resources-validation.test.ts +++ b/test/lib/resources-validation.test.ts @@ -1,8 +1,8 @@ -import {describe, it, expect, beforeEach, afterEach} from 'vitest' +import {describe, it, expect, beforeEach, afterEach, vi} from 'vitest' import {mkdtempSync, writeFileSync, rmSync} from 'node:fs' import {join} from 'node:path' import {tmpdir} from 'node:os' -import {ALERT_CHANNELS, STATUS_PAGES} from '../../src/lib/resources.js' +import {ALERT_CHANNELS, MONITORS, STATUS_PAGES} from '../../src/lib/resources.js' describe('ALERT_CHANNELS bodyBuilder --config validation', () => { it('accepts valid slack config JSON', () => { @@ -98,3 +98,51 @@ describe('STATUS_PAGES bodyBuilder --branding-file validation', () => { expect(() => STATUS_PAGES.bodyBuilder!({'branding-file': path})).toThrow(/Invalid branding file/) }) }) + +describe('MONITORS bodyBuilder --regions defaulting (P1.Bug9)', () => { + let stderrSpy: ReturnType + + beforeEach(() => { + stderrSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + }) + + afterEach(() => { + stderrSpy.mockRestore() + }) + + it('defaults regions to [us-east] for HTTP without --regions', () => { + const body = MONITORS.bodyBuilder({name: 'X', type: 'HTTP', url: 'https://x.com'}) as Record + expect(body.regions).toEqual(['us-east']) + expect(stderrSpy).toHaveBeenCalledWith( + expect.stringContaining('defaulting --regions us-east'), + ) + }) + + it.each(['TCP', 'DNS', 'ICMP', 'HTTP_HEADLESS', 'HTTP_BROWSER'])( + 'defaults regions for %s without --regions', + (type) => { + const body = MONITORS.bodyBuilder({name: 'X', type, url: 'x.com'}) as Record + expect(body.regions).toEqual(['us-east']) + }, + ) + + it('respects an explicit --regions value (no default applied)', () => { + const body = MONITORS.bodyBuilder({ + name: 'X', type: 'HTTP', url: 'https://x.com', regions: 'eu-west,ap-south', + }) as Record + expect(body.regions).toEqual(['eu-west', 'ap-south']) + expect(stderrSpy).not.toHaveBeenCalled() + }) + + it('does NOT default regions for HEARTBEAT (push monitor)', () => { + const body = MONITORS.bodyBuilder({name: 'X', type: 'HEARTBEAT'}) as Record + expect(body.regions).toBeUndefined() + expect(stderrSpy).not.toHaveBeenCalled() + }) + + it('does NOT default regions for MCP_SERVER (region semantics in flux)', () => { + const body = MONITORS.bodyBuilder({name: 'X', type: 'MCP_SERVER', url: 'mcp-server'}) as Record + expect(body.regions).toBeUndefined() + expect(stderrSpy).not.toHaveBeenCalled() + }) +}) From 8ab3bf330efb797d60f998efd426cf22459411d6 Mon Sep 17 00:00:00 2001 From: caballeto Date: Wed, 6 May 2026 00:12:37 +0200 Subject: [PATCH 4/8] chore: regenerate field descriptions for MCP/API enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous regen commits — descriptions.generated.ts is produced by `node scripts/extract-descriptions.mjs` from the same OpenAPI spec and was missed in the auto-regen run. The diff is purely the updated `managedBy` description (DASHBOARD, CLI, TERRAFORM, MCP, or API). Co-authored-by: Cursor --- src/lib/descriptions.generated.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/descriptions.generated.ts b/src/lib/descriptions.generated.ts index 890378a..bee76e0 100644 --- a/src/lib/descriptions.generated.ts +++ b/src/lib/descriptions.generated.ts @@ -9,7 +9,7 @@ export const fieldDescriptions: Record> = "frequencySeconds": "Check frequency in seconds (30–86400); null defaults to plan minimum (60s on most paid plans)", "enabled": "Whether the monitor is active (default: true)", "regions": "Probe regions to run checks from, e.g. us-east, eu-west", - "managedBy": "Who manages this monitor: DASHBOARD or CLI", + "managedBy": "Source that created/owns this monitor: DASHBOARD, CLI, TERRAFORM, MCP, or API. Use the value matching your surface so audit logs, drift detection, and analytics attribute correctly.", "environmentId": "Environment to associate with this monitor", "assertions": "Assertions to evaluate against each check result", "alertChannelIds": "Alert channels to notify when this monitor triggers" @@ -19,7 +19,7 @@ export const fieldDescriptions: Record> = "frequencySeconds": "New check frequency in seconds (30–86400); null preserves current", "enabled": "Enable or disable the monitor; null preserves current", "regions": "New probe regions; null preserves current", - "managedBy": "New management source; null preserves current", + "managedBy": "New ownership source: DASHBOARD, CLI, TERRAFORM, MCP, or API; null preserves current value", "environmentId": "New environment ID; null preserves current (use clearEnvironmentId to unset)", "clearEnvironmentId": "Set to true to remove the environment association", "assertions": "Replace all assertions; null preserves current", From 3d328998062d997096fc605cc5438c819407f44d Mon Sep 17 00:00:00 2001 From: caballeto Date: Wed, 6 May 2026 00:14:00 +0200 Subject: [PATCH 5/8] feat(monitors): add --assertion flag for inline assertions on create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DevEx P1.Bug2: there was no way to create a monitor with assertions in a single CLI call. Users had to `monitors create`, then call the assertions API one assertion at a time — which the CLI didn't expose at all, so most people gave up and used the dashboard. This adds a repeatable `--assertion` flag to `monitors create` that accepts either: - JSON form (any of the ~40 assertion types in the spec): --assertion '{"severity":"fail","config":{"type":"status_code", "expected":"200","operator":"equals"}}' - Shorthand DSL for the three most common assertions: --assertion 'status_code=200' → fail / status_code equals 200 --assertion 'response_time<5000' → warn / thresholdMs 5000 --assertion 'ssl_expiry>=14' → warn / minDaysRemaining 14 Each parsed assertion is POSTed to `/api/v1/monitors/{id}/assertions` after the monitor is created. If any POST fails, the monitor is deleted so users aren't left with a half-configured resource. Best-effort rollback messaging mentions the orphaned id when DELETE itself fails. Also adds `afterCreate` / `afterUpdate` hooks to `crud-commands.ts` so post-create side effects can be wired without forking the CRUD factory (used here for assertions; reused in the next commit for alert channels). Co-authored-by: Cursor --- src/lib/crud-commands.ts | 37 ++++- src/lib/monitor-assertions.ts | 173 ++++++++++++++++++++++++ src/lib/resources.ts | 39 ++++++ test/lib/monitor-assertions.test.ts | 200 ++++++++++++++++++++++++++++ 4 files changed, 448 insertions(+), 1 deletion(-) create mode 100644 src/lib/monitor-assertions.ts create mode 100644 test/lib/monitor-assertions.test.ts diff --git a/src/lib/crud-commands.ts b/src/lib/crud-commands.ts index 2f5cb27..5843539 100644 --- a/src/lib/crud-commands.ts +++ b/src/lib/crud-commands.ts @@ -80,6 +80,18 @@ export interface CreatableResource extends ResourceConfig { * `responseSchema` for the common case where create echoes the entity. */ createResponseSchema?: ZodType + /** + * Optional post-create hook for side-effecting follow-ups that need + * the freshly-created entity's id (alert-channel attachments, + * assertions, etc.). Throw to abort — the hook is responsible for + * any rollback (e.g. DELETE'ing the half-created entity) since the + * factory has no idea which calls are safe to undo. + */ + afterCreate?: ( + created: C, + rawFlags: Record, + ctx: AfterHookContext, + ) => Promise } /** @@ -94,6 +106,23 @@ export interface UpdatableResource extends ResourceConfig { bodyBuilder: (flags: Record) => object updateBodyBuilder?: (flags: Record) => object updateRequestSchema: ZodType + /** Optional post-update hook — see {@link CreatableResource.afterCreate}. */ + afterUpdate?: ( + updated: T, + rawFlags: Record, + ctx: AfterHookContext & {id: string}, + ) => Promise +} + +/** + * Context passed to `afterCreate` / `afterUpdate` hooks. The `command` + * is the live oclif command instance, so hooks can use `command.log` / + * `command.warn` for user-visible output without re-importing oclif. + */ +export interface AfterHookContext { + client: ApiClient + command: Command + apiPath: string } export function createListCommand(config: ResourceConfig) { @@ -165,6 +194,9 @@ export function createCreateCommand(config: CreatableResource) { const built = config.bodyBuilder(raw) const body = parseSchema(config.createRequestSchema, built, `${config.name}.create body invalid`) const value = await apiPostSingle(client, config.apiPath, createResponseSchema, body) + if (config.afterCreate) { + await config.afterCreate(value, raw, {client, command: this, apiPath: config.apiPath}) + } display(this, value, flags.output) } } @@ -185,12 +217,15 @@ export function createUpdateCommand(config: UpdatableResource) { async run() { const {args, flags} = await this.parse(UpdateCmd) const client = buildClient(flags) - const id = args[idLabel] + const id = args[idLabel] as string const raw = extractResourceFlags(flags, Object.keys(resourceFlags)) const built = builder(raw) const body = parseSchema(config.updateRequestSchema, built, `${config.name}.update body invalid`) const path = `${config.apiPath}/${id}` const value = await apiPutSingle(client, path, config.responseSchema, body) + if (config.afterUpdate) { + await config.afterUpdate(value, raw, {client, command: this, apiPath: config.apiPath, id}) + } display(this, value, flags.output) } } diff --git a/src/lib/monitor-assertions.ts b/src/lib/monitor-assertions.ts new file mode 100644 index 0000000..d8ff948 --- /dev/null +++ b/src/lib/monitor-assertions.ts @@ -0,0 +1,173 @@ +/** + * Helpers for the `--assertion` flag on `devhelm monitors create` + * (DevEx P1.Bug2). Two input forms are accepted: + * + * 1. JSON form — full passthrough so any of the ~40 assertion types in + * the spec can be expressed: + * --assertion '{"severity":"fail","config":{"type":"status_code", + * "expected":"200","operator":"equals"}}' + * + * 2. Shorthand DSL — the three most common assertions, written + * operator-style. Implemented as a tiny ad-hoc parser (no + * tokenizer, no schema) because the surface is intentionally + * narrow. Anything more elaborate should use the JSON form. + * status_code=200 → fail / status_code equals 200 + * response_time<5000 → warn / response_time thresholdMs 5000 + * ssl_expiry>=14 → warn / ssl_expiry minDaysRemaining 14 + * + * After the monitor is created the CLI POSTs each parsed assertion to + * `/api/v1/monitors/{id}/assertions`. If any POST fails the monitor is + * deleted to avoid a half-configured state — see `applyAssertions`. + */ +import type {ApiClient} from './api-client.js' +import {apiPost, apiDelete} from './api-client.js' + +export interface ParsedAssertion { + severity: 'fail' | 'warn' + config: Record +} + +const DSL_RE = /^([a-z_]+)\s*(>=|<=|<|>|=)\s*(.+?)\s*$/i + +export function parseAssertionFlag(raw: string): ParsedAssertion { + const trimmed = raw.trim() + if (trimmed.length === 0) { + throw new Error('--assertion: empty value') + } + // `{` and `[` both look like JSON to the user — route them through the + // JSON parser so the error message is structural ("must be an object") + // instead of the generic "cannot parse" from the DSL fallback. + if (trimmed.startsWith('{') || trimmed.startsWith('[')) { + return parseJsonForm(trimmed) + } + return parseDslForm(trimmed) +} + +function parseJsonForm(raw: string): ParsedAssertion { + let parsed: unknown + try { + parsed = JSON.parse(raw) + } catch (err) { + const msg = err instanceof Error ? err.message : String(err) + throw new Error(`--assertion: invalid JSON — ${msg}`) + } + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + throw new Error('--assertion JSON must be an object with {severity, config}') + } + const obj = parsed as Record + const severity = obj.severity ?? 'fail' + if (severity !== 'fail' && severity !== 'warn') { + throw new Error(`--assertion JSON: severity must be "fail" or "warn", got ${JSON.stringify(severity)}`) + } + if (!obj.config || typeof obj.config !== 'object' || Array.isArray(obj.config)) { + throw new Error('--assertion JSON: missing or invalid `config` object') + } + const config = obj.config as Record + if (typeof config.type !== 'string' || config.type.length === 0) { + throw new Error('--assertion JSON: config.type is required (e.g. "status_code", "response_time")') + } + return {severity, config} +} + +function parseDslForm(raw: string): ParsedAssertion { + const match = raw.match(DSL_RE) + if (!match) { + throw new Error( + `--assertion: cannot parse "${raw}" — expected JSON object or DSL like ` + + `"status_code=200", "response_time<5000", "ssl_expiry>=14"`, + ) + } + const [, type, op, value] = match + switch (type) { + case 'status_code': + if (op !== '=') { + throw new Error(`--assertion status_code only supports "=" (got "${op}")`) + } + return { + severity: 'fail', + config: {type: 'status_code', expected: value, operator: 'equals'}, + } + case 'response_time': { + if (op !== '<') { + throw new Error(`--assertion response_time only supports "<" (got "${op}")`) + } + const ms = Number(value) + if (!Number.isFinite(ms) || ms <= 0 || !Number.isInteger(ms)) { + throw new Error( + `--assertion response_time threshold must be a positive integer (got "${value}")`, + ) + } + return {severity: 'warn', config: {type: 'response_time', thresholdMs: ms}} + } + case 'ssl_expiry': { + if (op !== '>=') { + throw new Error(`--assertion ssl_expiry only supports ">=" (got "${op}")`) + } + const days = Number(value) + if (!Number.isFinite(days) || days <= 0 || !Number.isInteger(days)) { + throw new Error( + `--assertion ssl_expiry days must be a positive integer (got "${value}")`, + ) + } + return { + severity: 'warn', + config: {type: 'ssl_expiry', minDaysRemaining: days}, + } + } + default: + throw new Error( + `--assertion DSL only supports status_code, response_time, ssl_expiry. ` + + `Use the JSON form for "${type}", e.g. ` + + `--assertion '{"severity":"fail","config":{"type":"${type}",...}}'`, + ) + } +} + +/** + * POST each assertion to `/api/v1/monitors/{id}/assertions`. If any + * call fails, delete the monitor so the user isn't left with a partial + * setup — better to fail loudly and re-run than to ship a monitor with + * a different alert profile than what the YAML/CLI invocation requested. + * + * The cleanup is best-effort: if the rollback DELETE itself fails (rare, + * usually a transient API hiccup) we surface the original assertion + * error and append a hint about the orphaned monitor so the operator can + * clean it up by hand. + */ +export async function applyAssertions( + monitorId: string, + assertions: readonly ParsedAssertion[], + client: ApiClient, + monitorPath: string = '/api/v1/monitors', +): Promise { + const path = `${monitorPath}/${monitorId}/assertions` + for (const [idx, a] of assertions.entries()) { + try { + await apiPost(client, path, a) + } catch (err) { + const cleanup = await rollbackMonitor(monitorId, monitorPath, client) + const original = err instanceof Error ? err.message : String(err) + const tail = cleanup + ? 'Monitor was rolled back.' + : `Monitor ${monitorId} could NOT be rolled back automatically — delete it manually if it still exists.` + const assertionType = + typeof a.config.type === 'string' ? a.config.type : 'unknown' + throw new Error( + `Failed to apply --assertion #${idx + 1} (${assertionType}): ${original}. ${tail}`, + ) + } + } +} + +async function rollbackMonitor( + monitorId: string, + monitorPath: string, + client: ApiClient, +): Promise { + try { + await apiDelete(client, `${monitorPath}/${monitorId}`) + return true + } catch { + return false + } +} diff --git a/src/lib/resources.ts b/src/lib/resources.ts index b293089..e3de770 100644 --- a/src/lib/resources.ts +++ b/src/lib/resources.ts @@ -6,6 +6,7 @@ import type {components} from './api.generated.js' import {schemas as apiSchemas} from './api-zod.generated.js' import {fieldDescriptions} from './descriptions.generated.js' import {urlFlag} from './validators.js' +import {applyAssertions, parseAssertionFlag, type ParsedAssertion} from './monitor-assertions.js' import { MONITOR_TYPES, HTTP_METHODS, @@ -87,6 +88,13 @@ export const MONITORS: FullResource = { }), port: Flags.string({description: desc('TcpMonitorConfig', 'port', 'TCP port to connect to')}), regions: Flags.string({description: desc('CreateMonitorRequest', 'regions')}), + assertion: Flags.string({ + description: + 'Assertion to attach (repeatable). DSL: status_code=200, response_time<5000, ' + + 'ssl_expiry>=14. JSON: \'{"severity":"fail","config":{"type":"...","..."}}\'. ' + + 'Failures roll back the monitor.', + multiple: true, + }), }, updateFlags: { name: Flags.string({description: desc('UpdateMonitorRequest', 'name')}), @@ -133,6 +141,37 @@ export const MONITORS: FullResource = { } return body }, + // Post-create: attach assertions. The create endpoint only accepts + // the core monitor shape, so assertions go through a separate POST. + // If any assertion POST fails, roll back the monitor so the operator + // isn't left with a partial setup — see `applyAssertions`. + afterCreate: async (created, raw, ctx) => { + const monitorId = extractMonitorId(created) + if (!monitorId) return + const assertions = collectAssertions(raw.assertion) + if (assertions.length === 0) return + await applyAssertions(monitorId, assertions, ctx.client, ctx.apiPath) + }, +} + +function extractMonitorId(value: unknown): string | undefined { + if (!value || typeof value !== 'object') return undefined + const id = (value as {id?: unknown}).id + return typeof id === 'string' && id.length > 0 ? id : undefined +} + +function collectAssertions(raw: unknown): ParsedAssertion[] { + if (raw === undefined) return [] + // oclif passes `multiple: true` flags as a string array, but be + // defensive against single-value invocations and unknown shapes — + // the body builder + hook are reused across tests and YAML paths. + const items = Array.isArray(raw) ? raw : [raw] + const out: ParsedAssertion[] = [] + for (const item of items) { + if (typeof item !== 'string') continue + out.push(parseAssertionFlag(item)) + } + return out } // Monitor types that the API requires to declare at least one probe diff --git a/test/lib/monitor-assertions.test.ts b/test/lib/monitor-assertions.test.ts new file mode 100644 index 0000000..c0ff3b1 --- /dev/null +++ b/test/lib/monitor-assertions.test.ts @@ -0,0 +1,200 @@ +import {describe, expect, it, vi, beforeEach} from 'vitest' +import {applyAssertions, parseAssertionFlag} from '../../src/lib/monitor-assertions.js' + +describe('parseAssertionFlag — DSL form (P1.Bug2)', () => { + it('parses status_code=200 → fail / equals', () => { + expect(parseAssertionFlag('status_code=200')).toEqual({ + severity: 'fail', + config: {type: 'status_code', expected: '200', operator: 'equals'}, + }) + }) + + it('parses status_code=2xx (range pattern)', () => { + const result = parseAssertionFlag('status_code=2xx') + expect(result.config).toMatchObject({type: 'status_code', expected: '2xx', operator: 'equals'}) + }) + + it('parses response_time<5000 → warn / thresholdMs', () => { + expect(parseAssertionFlag('response_time<5000')).toEqual({ + severity: 'warn', + config: {type: 'response_time', thresholdMs: 5000}, + }) + }) + + it('parses ssl_expiry>=14 → warn / minDaysRemaining', () => { + expect(parseAssertionFlag('ssl_expiry>=14')).toEqual({ + severity: 'warn', + config: {type: 'ssl_expiry', minDaysRemaining: 14}, + }) + }) + + it('tolerates surrounding whitespace and inline padding', () => { + expect(parseAssertionFlag(' status_code = 200 ')).toEqual({ + severity: 'fail', + config: {type: 'status_code', expected: '200', operator: 'equals'}, + }) + }) + + it('rejects empty input', () => { + expect(() => parseAssertionFlag(' ')).toThrow(/empty value/) + }) + + it('rejects unknown DSL types with a JSON-form hint', () => { + expect(() => parseAssertionFlag('redirect_count=2')).toThrow(/JSON form for "redirect_count"/) + }) + + it('rejects status_code with non-equals operator', () => { + expect(() => parseAssertionFlag('status_code<200')).toThrow(/status_code only supports "="/) + }) + + it('rejects response_time with non-< operator', () => { + expect(() => parseAssertionFlag('response_time>=5000')).toThrow(/response_time only supports "<"/) + }) + + it('rejects ssl_expiry with non->= operator', () => { + expect(() => parseAssertionFlag('ssl_expiry<14')).toThrow(/ssl_expiry only supports ">="/) + }) + + it('rejects non-integer response_time threshold', () => { + expect(() => parseAssertionFlag('response_time<5.5')).toThrow(/positive integer/) + expect(() => parseAssertionFlag('response_time<-1')).toThrow(/positive integer/) + expect(() => parseAssertionFlag('response_time { + expect(() => parseAssertionFlag('status_code 200')).toThrow(/cannot parse/) + }) +}) + +describe('parseAssertionFlag — JSON form', () => { + it('parses a full {severity, config} object', () => { + const json = JSON.stringify({ + severity: 'fail', + config: {type: 'status_code', expected: '200', operator: 'equals'}, + }) + expect(parseAssertionFlag(json)).toEqual({ + severity: 'fail', + config: {type: 'status_code', expected: '200', operator: 'equals'}, + }) + }) + + it('defaults severity to "fail" when omitted', () => { + const json = JSON.stringify({config: {type: 'response_size', maxBytes: 100}}) + expect(parseAssertionFlag(json)).toMatchObject({severity: 'fail'}) + }) + + it('passes through any assertion type (no DSL restriction)', () => { + const json = JSON.stringify({ + severity: 'warn', + config: {type: 'json_path', path: '$.status', expected: 'ok', operator: 'equals'}, + }) + expect(parseAssertionFlag(json).config).toMatchObject({type: 'json_path', path: '$.status'}) + }) + + it('rejects malformed JSON with a parse error', () => { + expect(() => parseAssertionFlag('{not-json')).toThrow(/invalid JSON/) + }) + + it('rejects JSON arrays', () => { + expect(() => parseAssertionFlag('[]')).toThrow(/object with \{severity, config\}/) + }) + + it('rejects unknown severity', () => { + const json = JSON.stringify({severity: 'critical', config: {type: 'status_code'}}) + expect(() => parseAssertionFlag(json)).toThrow(/severity must be "fail" or "warn"/) + }) + + it('rejects missing config', () => { + expect(() => parseAssertionFlag('{"severity":"fail"}')).toThrow(/missing or invalid `config`/) + }) + + it('rejects config without type', () => { + expect(() => parseAssertionFlag('{"severity":"fail","config":{}}')).toThrow(/config\.type is required/) + }) +}) + +describe('applyAssertions', () => { + type FakeRequest = {method: string; path: string; body?: unknown} + type Outcome = {ok: true; data?: unknown} | {ok: false; status: number; message: string} + + /** + * Minimal fake of the openapi-fetch ApiClient surface that matches + * what `apiPost` / `apiDelete` actually call. Each handler returns an + * Outcome that the fake materialises into the `{data, error, response}` + * shape `checkedFetch` expects. + */ + function fakeClient(outcomes: Record Outcome)>) { + const calls: FakeRequest[] = [] + function resolve(key: string): Outcome { + const v = outcomes[key] ?? outcomes[key.split(' ')[0]] ?? {ok: true} + return typeof v === 'function' ? v() : v + } + function makeResponse(method: string, path: string, body?: unknown): Promise<{data?: unknown; error?: unknown; response: Response}> { + calls.push({method, path, body}) + const o = resolve(`${method} ${path}`) + const response = { + ok: o.ok, status: o.ok ? 200 : o.status, + statusText: o.ok ? 'OK' : 'ERR', + headers: new Headers(), + } as Response + if (o.ok) return Promise.resolve({data: o.data ?? {}, response}) + return Promise.resolve({error: {message: o.message}, response}) + } + const client = { + POST: vi.fn((path: string, opts?: {body?: unknown}) => makeResponse('POST', path, opts?.body)), + DELETE: vi.fn((path: string) => makeResponse('DELETE', path)), + } + return {client, calls} + } + + let stderr: ReturnType + + beforeEach(() => { + stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + }) + + it('POSTs each assertion to the monitor assertions endpoint', async () => { + const {client, calls} = fakeClient({}) + const assertions = [ + {severity: 'fail' as const, config: {type: 'status_code', expected: '200', operator: 'equals'}}, + {severity: 'warn' as const, config: {type: 'response_time', thresholdMs: 5000}}, + ] + await applyAssertions('mon-1', assertions, client as never) + const posts = calls.filter((c) => c.method === 'POST') + expect(posts).toHaveLength(2) + expect(posts[0].path).toBe('/api/v1/monitors/mon-1/assertions') + expect(posts[0].body).toMatchObject({severity: 'fail', config: {type: 'status_code'}}) + }) + + it('rolls back the monitor when an assertion POST fails', async () => { + const {client, calls} = fakeClient({ + POST: {ok: false, status: 400, message: 'bad request'}, + }) + const assertions = [{severity: 'fail' as const, config: {type: 'status_code', expected: '200', operator: 'equals'}}] + await expect(applyAssertions('mon-1', assertions, client as never)).rejects.toThrow( + /Failed to apply --assertion #1 \(status_code\).*Monitor was rolled back/s, + ) + expect(calls.find((c) => c.method === 'DELETE')?.path).toBe('/api/v1/monitors/mon-1') + expect(stderr).not.toHaveBeenCalled() + }) + + it('reports both the original failure and a rollback failure', async () => { + const {client} = fakeClient({ + POST: {ok: false, status: 400, message: '400'}, + DELETE: {ok: false, status: 500, message: 'boom'}, + }) + await expect( + applyAssertions( + 'mon-1', + [{severity: 'fail' as const, config: {type: 'status_code'}}], + client as never, + ), + ).rejects.toThrow(/could NOT be rolled back automatically/) + }) + + it('is a no-op for an empty assertion list', async () => { + const {client, calls} = fakeClient({}) + await applyAssertions('mon-1', [], client as never) + expect(calls).toHaveLength(0) + }) +}) From 198914806340dc4b369e2c9117dbb341e4a9200c Mon Sep 17 00:00:00 2001 From: caballeto Date: Wed, 6 May 2026 00:15:23 +0200 Subject: [PATCH 6/8] feat(monitors): wire --alert-channels and add monitors set-channels DevEx P1.Bug3: there was no way to attach an alert channel to a monitor from the CLI. Users had to switch to the dashboard, which broke the "deploy from CLI / test from dashboard" loop most teams use. Two surfaces ship together so each common workflow has a one-liner: - `monitors create --alert-channels ch-1,ch-2` Comma-separated channel IDs. After the create succeeds, the CLI calls `PUT /api/v1/monitors/{id}/alert-channels`. On failure the monitor is deleted (mirrors the assertion rollback added in the previous commit). - `monitors update --alert-channels ch-1,ch-2` Same flag on update. An empty string explicitly clears all channels; omitting the flag preserves the current set (no PUT issued). - `monitors set-channels --channel-ids ch-1,ch-2` Standalone subcommand for the "I just want to attach channels to this existing monitor" workflow. Required flag, empty string clears. The two `--alert-channels` flag descriptions differ deliberately: on create it warns about rollback semantics, on update it documents the clear-by-empty-string behaviour. Co-authored-by: Cursor --- src/commands/monitors/set-channels.ts | 49 ++++++++++++++ src/lib/monitor-alert-channels.ts | 58 +++++++++++++++++ src/lib/resources.ts | 43 ++++++++++-- test/lib/monitor-alert-channels.test.ts | 87 +++++++++++++++++++++++++ 4 files changed, 231 insertions(+), 6 deletions(-) create mode 100644 src/commands/monitors/set-channels.ts create mode 100644 src/lib/monitor-alert-channels.ts create mode 100644 test/lib/monitor-alert-channels.test.ts diff --git a/src/commands/monitors/set-channels.ts b/src/commands/monitors/set-channels.ts new file mode 100644 index 0000000..6ee5c80 --- /dev/null +++ b/src/commands/monitors/set-channels.ts @@ -0,0 +1,49 @@ +import {Command, Flags} from '@oclif/core' +import {globalFlags, buildClient} from '../../lib/base-command.js' +import {parseAlertChannelsFlag, setAlertChannels} from '../../lib/monitor-alert-channels.js' +import {uuidArg} from '../../lib/validators.js' + +/** + * Replace the alert channel set linked to a monitor. + * + * Standalone subcommand for the common workflow of attaching channels + * to an existing monitor without re-running `monitors update`. Pass + * `--channel-ids ""` (empty) to clear all channels — matching the + * semantics of the underlying `PUT /api/v1/monitors/{id}/alert-channels` + * which always replaces the full list. + */ +export default class MonitorsSetChannels extends Command { + static description = 'Replace the alert channel set linked to a monitor' + + static examples = [ + '<%= config.bin %> monitors set-channels --channel-ids ch-1,ch-2', + '<%= config.bin %> monitors set-channels --channel-ids ""', + ] + + static args = { + id: uuidArg({description: 'monitor id', required: true}), + } + + static flags = { + ...globalFlags, + 'channel-ids': Flags.string({ + description: + 'Comma-separated alert channel IDs (replaces current list; pass empty string to clear all channels)', + required: true, + }), + } + + async run() { + const {args, flags} = await this.parse(MonitorsSetChannels) + const client = buildClient(flags) + const ids = parseAlertChannelsFlag(flags['channel-ids']) ?? [] + await setAlertChannels(args.id as string, ids, client) + if (ids.length === 0) { + this.log(`Cleared all alert channels from monitor '${args.id}'.`) + } else { + this.log( + `Linked ${ids.length} alert channel(s) to monitor '${args.id}': ${ids.join(', ')}`, + ) + } + } +} diff --git a/src/lib/monitor-alert-channels.ts b/src/lib/monitor-alert-channels.ts new file mode 100644 index 0000000..d410b5b --- /dev/null +++ b/src/lib/monitor-alert-channels.ts @@ -0,0 +1,58 @@ +/** + * Helpers for the `--alert-channels` flag on `devhelm monitors + * create / update` and the `monitors set-channels` subcommand + * (DevEx P1.Bug3). Wraps `PUT /api/v1/monitors/{id}/alert-channels`, + * which replaces the linked channel set. + * + * Parsing is intentionally lenient: leading/trailing whitespace and + * commas are stripped, and the empty string clears all channels (for + * `update` and `set-channels`). On `create`, an empty list is treated + * as "no channels requested" and the call is skipped. + */ +import {apiPut, apiDelete, type ApiClient} from './api-client.js' + +export function parseAlertChannelsFlag(raw: string | undefined): string[] | undefined { + if (raw === undefined) return undefined + return raw.split(',').map((s) => s.trim()).filter(Boolean) +} + +export async function setAlertChannels( + monitorId: string, + channelIds: readonly string[], + client: ApiClient, + monitorPath: string = '/api/v1/monitors', +): Promise { + await apiPut(client, `${monitorPath}/${monitorId}/alert-channels`, { + channelIds: [...channelIds], + }) +} + +/** + * Attach channels right after `create` succeeded. On failure, delete the + * monitor so the operator isn't left with a half-configured resource. + * Mirrors the rollback story in `applyAssertions` so the two + * post-create hooks behave the same way under failure. + */ +export async function attachAlertChannelsOrRollback( + monitorId: string, + channelIds: readonly string[], + client: ApiClient, + monitorPath: string = '/api/v1/monitors', +): Promise { + if (channelIds.length === 0) return + try { + await setAlertChannels(monitorId, channelIds, client, monitorPath) + } catch (err) { + let cleanup = true + try { + await apiDelete(client, `${monitorPath}/${monitorId}`) + } catch { + cleanup = false + } + const original = err instanceof Error ? err.message : String(err) + const tail = cleanup + ? 'Monitor was rolled back.' + : `Monitor ${monitorId} could NOT be rolled back automatically — delete it manually if it still exists.` + throw new Error(`Failed to attach --alert-channels: ${original}. ${tail}`) + } +} diff --git a/src/lib/resources.ts b/src/lib/resources.ts index e3de770..fff4100 100644 --- a/src/lib/resources.ts +++ b/src/lib/resources.ts @@ -7,6 +7,7 @@ import {schemas as apiSchemas} from './api-zod.generated.js' import {fieldDescriptions} from './descriptions.generated.js' import {urlFlag} from './validators.js' import {applyAssertions, parseAssertionFlag, type ParsedAssertion} from './monitor-assertions.js' +import {attachAlertChannelsOrRollback, parseAlertChannelsFlag, setAlertChannels} from './monitor-alert-channels.js' import { MONITOR_TYPES, HTTP_METHODS, @@ -95,6 +96,11 @@ export const MONITORS: FullResource = { 'Failures roll back the monitor.', multiple: true, }), + 'alert-channels': Flags.string({ + description: + 'Comma-separated alert channel IDs to attach after create. ' + + 'Failures roll back the monitor.', + }), }, updateFlags: { name: Flags.string({description: desc('UpdateMonitorRequest', 'name')}), @@ -102,6 +108,10 @@ export const MONITORS: FullResource = { frequency: Flags.string({description: desc('UpdateMonitorRequest', 'frequencySeconds')}), method: Flags.string({description: desc('HttpMonitorConfig', 'method'), options: [...HTTP_METHODS]}), port: Flags.string({description: desc('TcpMonitorConfig', 'port', 'TCP port to connect to')}), + 'alert-channels': Flags.string({ + description: + 'Comma-separated alert channel IDs (replaces current list; pass empty string to clear)', + }), }, // bodyBuilder returns a plain `Record`; the outer // `parseSchema(MONITORS.createRequestSchema, ...)` / `updateRequestSchema` @@ -141,16 +151,33 @@ export const MONITORS: FullResource = { } return body }, - // Post-create: attach assertions. The create endpoint only accepts - // the core monitor shape, so assertions go through a separate POST. - // If any assertion POST fails, roll back the monitor so the operator - // isn't left with a partial setup — see `applyAssertions`. + // Post-create: attach assertions and alert channels. Both APIs are + // separate calls — the create endpoint only accepts the core monitor + // shape. If either step fails, roll back the monitor so the operator + // isn't left with a partial setup. Assertions run first because + // they're typically the reason someone is creating the monitor; a + // successful assertion attach + failed channel attach still rolls + // back so behaviour is uniform. afterCreate: async (created, raw, ctx) => { const monitorId = extractMonitorId(created) if (!monitorId) return const assertions = collectAssertions(raw.assertion) - if (assertions.length === 0) return - await applyAssertions(monitorId, assertions, ctx.client, ctx.apiPath) + if (assertions.length > 0) { + await applyAssertions(monitorId, assertions, ctx.client, ctx.apiPath) + } + const channelIds = parseAlertChannelsFlag(rawString(raw['alert-channels'])) + if (channelIds && channelIds.length > 0) { + await attachAlertChannelsOrRollback(monitorId, channelIds, ctx.client, ctx.apiPath) + } + }, + // Post-update: only handles --alert-channels (assertions on update + // would race with the API's own assertion list, so we keep that to + // the dedicated `monitors assertions` topic). An empty string + // explicitly clears all channels — undefined leaves them alone. + afterUpdate: async (_updated, raw, ctx) => { + const channelIds = parseAlertChannelsFlag(rawString(raw['alert-channels'])) + if (channelIds === undefined) return + await setAlertChannels(ctx.id, channelIds, ctx.client, ctx.apiPath) }, } @@ -160,6 +187,10 @@ function extractMonitorId(value: unknown): string | undefined { return typeof id === 'string' && id.length > 0 ? id : undefined } +function rawString(v: unknown): string | undefined { + return typeof v === 'string' ? v : undefined +} + function collectAssertions(raw: unknown): ParsedAssertion[] { if (raw === undefined) return [] // oclif passes `multiple: true` flags as a string array, but be diff --git a/test/lib/monitor-alert-channels.test.ts b/test/lib/monitor-alert-channels.test.ts new file mode 100644 index 0000000..27330f1 --- /dev/null +++ b/test/lib/monitor-alert-channels.test.ts @@ -0,0 +1,87 @@ +import {describe, expect, it, vi, beforeEach} from 'vitest' +import { + attachAlertChannelsOrRollback, + parseAlertChannelsFlag, + setAlertChannels, +} from '../../src/lib/monitor-alert-channels.js' + +describe('parseAlertChannelsFlag (P1.Bug3)', () => { + it('returns undefined when the flag was not provided', () => { + expect(parseAlertChannelsFlag(undefined)).toBeUndefined() + }) + + it('returns an empty array for the empty string (clears all channels)', () => { + expect(parseAlertChannelsFlag('')).toEqual([]) + }) + + it('splits a comma-separated list and trims each entry', () => { + expect(parseAlertChannelsFlag('a, b , c')).toEqual(['a', 'b', 'c']) + }) + + it('drops empty entries from trailing commas', () => { + expect(parseAlertChannelsFlag('a,,b,')).toEqual(['a', 'b']) + }) +}) + +describe('setAlertChannels / attachAlertChannelsOrRollback', () => { + type Outcome = {ok: true; data?: unknown} | {ok: false; status: number; message: string} + + function fakeClient(outcomes: Record) { + const calls: Array<{method: string; path: string; body?: unknown}> = [] + function makeResponse(method: string, path: string, body?: unknown) { + calls.push({method, path, body}) + const o = outcomes[`${method} ${path}`] ?? outcomes[method] ?? {ok: true} + const response = { + ok: o.ok, status: o.ok ? 200 : o.status, + statusText: o.ok ? 'OK' : 'ERR', + headers: new Headers(), + } as Response + return Promise.resolve(o.ok ? {data: o.data ?? {}, response} : {error: {message: o.message}, response}) + } + return { + client: { + PUT: vi.fn((path: string, opts?: {body?: unknown}) => makeResponse('PUT', path, opts?.body)), + DELETE: vi.fn((path: string) => makeResponse('DELETE', path)), + }, + calls, + } + } + + let stderr: ReturnType + beforeEach(() => { + stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) + }) + + it('PUTs the channel list to the alert-channels endpoint', async () => { + const {client, calls} = fakeClient({}) + await setAlertChannels('mon-1', ['ch-1', 'ch-2'], client as never) + expect(calls).toHaveLength(1) + expect(calls[0].path).toBe('/api/v1/monitors/mon-1/alert-channels') + expect(calls[0].body).toEqual({channelIds: ['ch-1', 'ch-2']}) + }) + + it('attachAlertChannelsOrRollback skips the PUT when the list is empty', async () => { + const {client, calls} = fakeClient({}) + await attachAlertChannelsOrRollback('mon-1', [], client as never) + expect(calls).toHaveLength(0) + }) + + it('rolls back the monitor on a failed PUT', async () => { + const {client, calls} = fakeClient({PUT: {ok: false, status: 400, message: 'bad'}}) + await expect( + attachAlertChannelsOrRollback('mon-1', ['ch-1'], client as never), + ).rejects.toThrow(/Failed to attach --alert-channels.*Monitor was rolled back/s) + expect(calls.find((c) => c.method === 'DELETE')?.path).toBe('/api/v1/monitors/mon-1') + expect(stderr).not.toHaveBeenCalled() + }) + + it('reports both errors when the rollback DELETE itself fails', async () => { + const {client} = fakeClient({ + PUT: {ok: false, status: 500, message: 'boom'}, + DELETE: {ok: false, status: 500, message: 'oh no'}, + }) + await expect( + attachAlertChannelsOrRollback('mon-1', ['ch-1'], client as never), + ).rejects.toThrow(/could NOT be rolled back automatically/) + }) +}) From 056a90fbbfbbc6969a55146451be00198856efa4 Mon Sep 17 00:00:00 2001 From: caballeto Date: Wed, 6 May 2026 00:21:05 +0200 Subject: [PATCH 7/8] fix(deploy): scope --prune to local state file; add --prune-org-cli MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DevEx P0.Bug1 (multi-team safety): the previous `--prune` deleted any CLI-managed resource org-wide that wasn't in the local devhelm.yml. Two teams sharing one workspace ran into this immediately — running `deploy --prune --yes` in their own config silently destroyed the other team's monitors because both were tagged `managedBy: CLI`. New scope semantics: - `--prune` (NEW DEFAULT): only deletes resources tracked in this config's `.devhelm/state.json`. Other CLI-managed resources are left alone — they belong to a different config. - `--prune-org-cli`: legacy `--prune` behaviour — also deletes CLI-managed resources elsewhere in the org. Required for cleanup after `state.json` was rotated, or when migrating between configs. - `--prune-all`: unchanged. Deletes everything not in YAML, including dashboard- and Terraform-managed resources. Plan output groups destroys by scope so multi-team users can tell at a glance which deletes are theirs vs widened by `--prune-org-cli`: Tracked by this config: - monitor "mine" Other CLI-managed resources: - monitor "theirs" When every delete shares one scope (the common case) the headers are omitted and the plan looks identical to before. Each `Change` of action `delete` now carries a `pruneScope` field ('state' | 'org-cli' | 'org-all') for downstream JSON consumers (CI, dashboards, automation). Existing tests that exercised the legacy broad behaviour were retargeted at `--prune-org-cli`; new tests cover the state-scoped default and the grouped plan output. Co-authored-by: Cursor --- src/commands/deploy/index.ts | 26 ++++++- src/lib/yaml/differ.ts | 113 +++++++++++++++++++++------- src/lib/yaml/types.ts | 28 +++++++ test/yaml/differ.test.ts | 137 ++++++++++++++++++++++++++++++++-- test/yaml/idempotency.test.ts | 4 +- 5 files changed, 271 insertions(+), 37 deletions(-) diff --git a/src/commands/deploy/index.ts b/src/commands/deploy/index.ts index a04ce2b..8f88e74 100644 --- a/src/commands/deploy/index.ts +++ b/src/commands/deploy/index.ts @@ -17,6 +17,7 @@ export default class Deploy extends Command { '<%= config.bin %> deploy --yes', '<%= config.bin %> deploy -f monitors.yml', '<%= config.bin %> deploy --prune --yes', + '<%= config.bin %> deploy --prune-org-cli --yes', '<%= config.bin %> deploy --prune-all --yes', '<%= config.bin %> deploy --dry-run', '<%= config.bin %> deploy --dry-run --detailed-exitcode', @@ -36,11 +37,21 @@ export default class Deploy extends Command { default: false, }), prune: Flags.boolean({ - description: 'Delete CLI-managed resources not present in config', + description: + 'Delete resources tracked in this config\'s .devhelm/state.json that are absent from YAML. ' + + 'Safe in multi-config orgs — does NOT touch CLI-managed resources from other configs.', + default: false, + }), + 'prune-org-cli': Flags.boolean({ + description: + 'Also delete CLI-managed resources elsewhere in the org that are absent from YAML. ' + + 'Use with caution in shared workspaces; widens --prune to the legacy org-wide CLI scope.', default: false, }), 'prune-all': Flags.boolean({ - description: 'Delete ALL resources not in config, including those not managed by the CLI (use with caution)', + description: + 'Delete ALL resources not in YAML, including dashboard- and Terraform-managed ones. ' + + 'The most destructive option; intended for single-tenant workspaces or scripted teardowns.', default: false, }), 'dry-run': Flags.boolean({ @@ -155,7 +166,16 @@ export default class Deploy extends Command { const currentChildren = await prefetchChildSnapshots(config, refs, client) const changeset = await diff( config, refs, - {prune: flags.prune || flags['prune-all'], pruneAll: flags['prune-all']}, + { + // `--prune` is the safe default (state-scoped). The wider scopes + // imply `--prune` so users don't have to combine flags to enable + // the broader behaviour — `--prune-org-cli` deletes state-tracked + // resources AND legacy CLI-managed orphans, and `--prune-all` + // also picks up dashboard- and Terraform-managed resources. + prune: flags.prune || flags['prune-org-cli'] || flags['prune-all'], + pruneOrgCli: flags['prune-org-cli'] || flags['prune-all'], + pruneAll: flags['prune-all'], + }, currentState, currentChildren, ) diff --git a/src/lib/yaml/differ.ts b/src/lib/yaml/differ.ts index 6af0ebd..1554448 100644 --- a/src/lib/yaml/differ.ts +++ b/src/lib/yaml/differ.ts @@ -15,7 +15,7 @@ import type {components} from '../api.generated.js' import type {DevhelmConfig} from './schema.js' import type {ResolvedRefs} from './resolver.js' import {allHandlers, type ResourceHandler, type CurrentChildEntry} from './handlers.js' -import type {Change, Changeset, DiffOptions} from './types.js' +import type {Change, Changeset, DiffOptions, PruneScope} from './types.js' import {RESOURCE_ORDER} from './types.js' import type {DeployState} from './state.js' import {resourceAddress} from './state.js' @@ -68,8 +68,18 @@ export async function diff( return currentChildren.get(addr) ?? {} } + // Set of `
.` addresses present in this config's + // `.devhelm/state.json`. Drives the `--prune` (state-scoped) branch + // in `diffSection` — non-state addresses are only candidates under + // `--prune-org-cli` or `--prune-all`. Lazily an empty set when no + // state was passed so callers / tests can omit `priorState` without + // accidentally widening the prune scope. + const stateAddresses = new Set( + priorState ? Object.keys(priorState.resources) : [], + ) + for (const handler of allHandlers()) { - diffSection(handler, config[handler.configKey], refs, creates, updates, deletes, options, lookupCurrentChildren) + diffSection(handler, config[handler.configKey], refs, creates, updates, deletes, options, lookupCurrentChildren, stateAddresses) } diffMemberships(config, refs, memberships, options) @@ -78,12 +88,6 @@ export async function diff( updates.sort(byResourceOrder) deletes.sort((a, b) => byResourceOrderIndex(b.resourceType) - byResourceOrderIndex(a.resourceType)) - // Acknowledge unused param: kept in the signature for future drift hooks - // (e.g. detecting parents present in state but absent from both YAML and - // the live API) without forcing every caller to refactor. The differ - // itself reads parent attrs only from `refs` (live API) per RFC 0001. - void priorState - return {creates, updates, deletes, memberships} } @@ -160,6 +164,7 @@ function diffSection( deletes: Change[], options: DiffOptions, lookupCurrentChildren: (resourceType: ResourceType, refKey: string) => Record, + stateAddresses: ReadonlySet, ): void { const desired = new Set() @@ -204,23 +209,57 @@ function diffSection( } } - if ((options.prune || options.pruneAll) && items !== undefined) { + const anyPrune = options.prune || options.pruneOrgCli || options.pruneAll + if (anyPrune && items !== undefined) { for (const entry of refs.allEntries(handler.refType)) { if (entry.isPending) continue - if (!desired.has(entry.refKey)) { - if (handler.resourceType === 'monitor' && !options.pruneAll && entry.managedBy !== 'CLI') continue - deletes.push({ - action: 'delete', - resourceType: handler.resourceType, - refKey: entry.refKey, - existingId: entry.id, - current: entry.raw, - }) - } + if (desired.has(entry.refKey)) continue + + const address = resourceAddress(handler.resourceType as ResourceType, entry.refKey) + const scope = classifyDeleteScope(handler, entry, stateAddresses, address) + + // Decide whether to include this delete based on the requested + // prune flag(s). State-scoped deletes are the safe default and + // are always included when *any* prune flag is on; the wider + // scopes require their explicit opt-in. + const include = + (scope === 'state') || + (scope === 'org-cli' && (options.pruneOrgCli || options.pruneAll)) || + (scope === 'org-all' && options.pruneAll) + if (!include) continue + + deletes.push({ + action: 'delete', + resourceType: handler.resourceType, + refKey: entry.refKey, + existingId: entry.id, + current: entry.raw, + pruneScope: scope, + }) } } } +function classifyDeleteScope( + handler: ResourceHandler, + entry: {refKey: string; managedBy?: string | null}, + stateAddresses: ReadonlySet, + address: string, +): PruneScope { + if (stateAddresses.has(address)) return 'state' + // Monitors carry an explicit `managedBy`; non-CLI monitors are + // foreign (dashboard / Terraform / MCP / API) and only `--prune-all` + // is allowed to touch them. + if (handler.resourceType === 'monitor') { + return entry.managedBy === 'CLI' ? 'org-cli' : 'org-all' + } + // Other resource types lack a managedBy concept on the API, so the + // legacy `--prune` semantic was "delete anything not in YAML" — keep + // that under `--prune-org-cli` for parity, and reserve `--prune-all` + // for an unambiguous "everything". + return 'org-cli' +} + // ── Membership diff ──────────────────────────────────────────────────── // // Why memberships do **not** use the generic `ChildCollectionDef` machinery @@ -251,12 +290,13 @@ function memberKey(memberType: string, nameOrSlug: string): string { * Diff resource group memberships. * * Prune semantics (documented for M8): - * - `options.prune` or `options.pruneAll`: + * - Any prune flag (`--prune` / `--prune-org-cli` / `--prune-all`): * - For groups **present in YAML**: remove any current members not listed. * - For groups **absent from YAML** (orphan groups): *only* under - * `pruneAll` — because `prune` is scoped to CLI-managed resources and - * we don't know membership provenance. `pruneAll` takes the Terraform - * stance: anything not in config is fair game. + * `pruneAll` — membership provenance isn't tracked anywhere outside + * the YAML, so the safe default is to leave dashboard-curated + * memberships alone. `pruneAll` takes the Terraform stance: + * anything not in config is fair game. * - Without prune flags, memberships are additive: we only create missing * ones, never remove. */ @@ -311,7 +351,8 @@ function diffMemberships( } } - if ((options.prune || options.pruneAll) && groupEntry) { + const anyPrune = options.prune || options.pruneOrgCli || options.pruneAll + if (anyPrune && groupEntry) { for (const [key, member] of currentMembers) { if (!desired.has(key)) { const label = member.name ?? member.slug ?? member.id ?? 'unknown' @@ -415,8 +456,28 @@ export function formatPlan(changeset: Changeset): string { } } } - for (const c of changeset.deletes) { - lines.push(` - ${c.resourceType} "${c.refKey}"`) + // Group destroys by prune scope so multi-team users can tell at a + // glance which ones came from this config's state and which came from + // the wider `--prune-org-cli` / `--prune-all` widening. When every + // delete shares a single scope (the common case), only that group's + // header is printed. + if (changeset.deletes.length > 0) { + const groups: Array<{label: string; scope: PruneScope | undefined}> = [ + {label: 'Tracked by this config:', scope: 'state'}, + {label: 'Other CLI-managed resources:', scope: 'org-cli'}, + {label: 'Foreign resources (--prune-all):', scope: 'org-all'}, + ] + const present = new Set(changeset.deletes.map((d) => d.pruneScope ?? 'state')) + const showHeaders = present.size > 1 + for (const {label, scope} of groups) { + const items = changeset.deletes.filter((d) => (d.pruneScope ?? 'state') === scope) + if (items.length === 0) continue + if (showHeaders) lines.push(` ${label}`) + for (const c of items) { + const indent = showHeaders ? ' ' : ' ' + lines.push(`${indent}- ${c.resourceType} "${c.refKey}"`) + } + } } for (const c of changeset.memberships) { const icon = c.action === 'delete' ? '- membership' : '→' diff --git a/src/lib/yaml/types.ts b/src/lib/yaml/types.ts index 3091183..747f293 100644 --- a/src/lib/yaml/types.ts +++ b/src/lib/yaml/types.ts @@ -53,10 +53,38 @@ export interface Change { current?: unknown /** Attribute-level diffs for update changes */ attributeDiffs?: AttributeDiff[] + /** + * For `delete` actions only — which prune scope produced the change. + * `state` means the resource was tracked in this config's + * `.devhelm/state.json`; `org-cli` means it's CLI-managed elsewhere + * in the org; `org-all` is anything else surfaced under `--prune-all`. + * Drives the grouped output in `formatPlan` so multi-team users can + * tell at a glance which destroys are theirs. + */ + pruneScope?: PruneScope } +export type PruneScope = 'state' | 'org-cli' | 'org-all' + export interface DiffOptions { + /** + * Delete resources tracked in `.devhelm/state.json` that are absent + * from YAML. Safe in multi-config orgs because it only touches + * resources THIS config previously created (DevEx P0.Bug1). + */ prune?: boolean + /** + * Delete CLI-managed resources org-wide that are absent from YAML — + * the legacy `--prune` behaviour. Required to clean up resources + * created from a different YAML file or by an earlier reset of the + * local state. Use with caution in shared workspaces. + */ + pruneOrgCli?: boolean + /** + * Delete ALL resources absent from YAML, including dashboard- and + * Terraform-managed ones. The most destructive option; reserve for + * single-tenant workspaces or scripted teardowns. + */ pruneAll?: boolean } diff --git a/test/yaml/differ.test.ts b/test/yaml/differ.test.ts index 35397d4..0eac952 100644 --- a/test/yaml/differ.test.ts +++ b/test/yaml/differ.test.ts @@ -201,18 +201,29 @@ describe('differ', () => { expect(changeset.deletes).toHaveLength(0) }) - it('detects deletes with prune=true', async () => { + it('--prune-org-cli detects deletes for CLI-managed monitors org-wide', async () => { const refs = new ResolvedRefs() refs.set('monitors', 'old-monitor', { id: 'mon-1', refKey: 'old-monitor', managedBy: 'CLI', raw: {managedBy: 'CLI'}, }) const config: DevhelmConfig = {monitors: []} - const changeset = await diff(config, refs, {prune: true}) + const changeset = await diff(config, refs, {pruneOrgCli: true}) expect(changeset.deletes).toHaveLength(1) expect(changeset.deletes[0].refKey).toBe('old-monitor') + expect(changeset.deletes[0].pruneScope).toBe('org-cli') + }) + + it('skips non-CLI monitors during --prune-org-cli', async () => { + const refs = new ResolvedRefs() + refs.set('monitors', 'dashboard-monitor', { + id: 'mon-1', refKey: 'dashboard-monitor', managedBy: 'DASHBOARD', raw: {managedBy: 'DASHBOARD'}, + }) + const config: DevhelmConfig = {monitors: []} + const changeset = await diff(config, refs, {pruneOrgCli: true}) + expect(changeset.deletes).toHaveLength(0) }) - it('skips non-CLI monitors during prune', async () => { + it('skips non-CLI monitors during plain --prune even when state is empty', async () => { const refs = new ResolvedRefs() refs.set('monitors', 'dashboard-monitor', { id: 'mon-1', refKey: 'dashboard-monitor', managedBy: 'DASHBOARD', raw: {managedBy: 'DASHBOARD'}, @@ -251,12 +262,13 @@ describe('differ', () => { expect(changeset.deletes).toHaveLength(0) }) - it('prune: empty array deletes all existing', async () => { + it('--prune-org-cli: empty array deletes all existing tags', async () => { const refs = new ResolvedRefs() refs.set('tags', 'T', {id: '1', refKey: 'T', raw: {name: 'T'}}) const config: DevhelmConfig = {tags: []} - const changeset = await diff(config, refs, {prune: true}) + const changeset = await diff(config, refs, {pruneOrgCli: true}) expect(changeset.deletes).toHaveLength(1) + expect(changeset.deletes[0].pruneScope).toBe('org-cli') }) it('does not delete without prune flag', async () => { @@ -266,6 +278,92 @@ describe('differ', () => { const changeset = await diff(config, refs, {prune: false}) expect(changeset.deletes).toHaveLength(0) }) + }) + + // ── --prune scope to state file (P0.Bug1, multi-team safety) ────────── + describe('diff: --prune state-scoped (P0.Bug1)', () => { + function stateWith(addresses: string[]): import('../../src/lib/yaml/state.js').DeployState { + return { + version: '3' as const, serial: 1, lastDeployedAt: '2026-05-05T00:00:00Z', + resources: Object.fromEntries( + addresses.map((addr) => [addr, { + apiId: addr.split('.').slice(1).join('.') + '-id', + resourceType: addr.split('.')[0] as never, + children: {}, + }]), + ), + } + } + + it('deletes a monitor that IS in this config\'s state file', async () => { + const refs = new ResolvedRefs() + refs.set('monitors', 'mine', { + id: 'mon-1', refKey: 'mine', managedBy: 'CLI', raw: {managedBy: 'CLI'}, + }) + const state = stateWith(['monitors.mine']) + const cs = await diff({monitors: []}, refs, {prune: true}, state) + expect(cs.deletes).toHaveLength(1) + expect(cs.deletes[0].refKey).toBe('mine') + expect(cs.deletes[0].pruneScope).toBe('state') + }) + + it('does NOT delete a CLI monitor that is NOT in this state file (multi-team safety)', async () => { + const refs = new ResolvedRefs() + refs.set('monitors', 'theirs', { + id: 'mon-2', refKey: 'theirs', managedBy: 'CLI', raw: {managedBy: 'CLI'}, + }) + const state = stateWith([]) // empty state — this config never created `theirs` + const cs = await diff({monitors: []}, refs, {prune: true}, state) + expect(cs.deletes).toHaveLength(0) + }) + + it('--prune-org-cli widens scope and deletes the other team\'s CLI monitor', async () => { + const refs = new ResolvedRefs() + refs.set('monitors', 'theirs', { + id: 'mon-2', refKey: 'theirs', managedBy: 'CLI', raw: {managedBy: 'CLI'}, + }) + const cs = await diff({monitors: []}, refs, {pruneOrgCli: true}, stateWith([])) + expect(cs.deletes).toHaveLength(1) + expect(cs.deletes[0].pruneScope).toBe('org-cli') + }) + + it('reports state-tracked and org-cli deletes with their scope', async () => { + const refs = new ResolvedRefs() + refs.set('monitors', 'mine', { + id: 'mon-1', refKey: 'mine', managedBy: 'CLI', raw: {managedBy: 'CLI'}, + }) + refs.set('monitors', 'theirs', { + id: 'mon-2', refKey: 'theirs', managedBy: 'CLI', raw: {managedBy: 'CLI'}, + }) + const cs = await diff({monitors: []}, refs, {pruneOrgCli: true}, stateWith(['monitors.mine'])) + const byKey = Object.fromEntries(cs.deletes.map((d) => [d.refKey, d.pruneScope])) + expect(byKey).toEqual({mine: 'state', theirs: 'org-cli'}) + }) + + it('--prune-all picks up dashboard-managed monitors as scope=org-all', async () => { + const refs = new ResolvedRefs() + refs.set('monitors', 'dash', { + id: 'mon-3', refKey: 'dash', managedBy: 'DASHBOARD', raw: {managedBy: 'DASHBOARD'}, + }) + const cs = await diff({monitors: []}, refs, {pruneAll: true}, stateWith([])) + expect(cs.deletes).toHaveLength(1) + expect(cs.deletes[0].pruneScope).toBe('org-all') + }) + + it('non-monitor resources outside state are NOT touched under plain --prune', async () => { + const refs = new ResolvedRefs() + refs.set('tags', 'shared', {id: 't-1', refKey: 'shared', raw: {name: 'shared'}}) + const cs = await diff({tags: []}, refs, {prune: true}, stateWith([])) + expect(cs.deletes).toHaveLength(0) + }) + + it('non-monitor resources outside state ARE touched under --prune-org-cli', async () => { + const refs = new ResolvedRefs() + refs.set('tags', 'shared', {id: 't-1', refKey: 'shared', raw: {name: 'shared'}}) + const cs = await diff({tags: []}, refs, {pruneOrgCli: true}, stateWith([])) + expect(cs.deletes).toHaveLength(1) + expect(cs.deletes[0].pruneScope).toBe('org-cli') + }) it('creates in dependency order', async () => { const config: DevhelmConfig = { @@ -284,7 +382,7 @@ describe('differ', () => { refs.set('tags', 'T', {id: '1', refKey: 'T', raw: {}}) refs.set('monitors', 'M', {id: '2', refKey: 'M', managedBy: 'CLI', raw: {}}) const config: DevhelmConfig = {tags: [], monitors: []} - const changeset = await diff(config, refs, {prune: true}) + const changeset = await diff(config, refs, {pruneOrgCli: true}) const types = changeset.deletes.map((c) => c.resourceType) expect(types.indexOf('monitor')).toBeLessThan(types.indexOf('tag')) }) @@ -1244,5 +1342,32 @@ describe('differ', () => { expect(result).toContain('name: "API" → "Core API"') expect(result).toContain('frequencySeconds: 60 → 30') }) + + it('groups destroys by prune scope when scopes differ (P0.Bug1)', async () => { + const changeset = { + creates: [], updates: [], memberships: [], + deletes: [ + {action: 'delete' as const, resourceType: 'monitor' as const, refKey: 'mine', existingId: 'm-1', pruneScope: 'state' as const}, + {action: 'delete' as const, resourceType: 'monitor' as const, refKey: 'theirs', existingId: 'm-2', pruneScope: 'org-cli' as const}, + ], + } + const result = formatPlan(changeset) + expect(result).toContain('Tracked by this config:') + expect(result).toContain('Other CLI-managed resources:') + expect(result).toContain('- monitor "mine"') + expect(result).toContain('- monitor "theirs"') + }) + + it('omits destroy headers when every delete shares one scope', async () => { + const changeset = { + creates: [], updates: [], memberships: [], + deletes: [ + {action: 'delete' as const, resourceType: 'monitor' as const, refKey: 'mine', existingId: 'm-1', pruneScope: 'state' as const}, + ], + } + const result = formatPlan(changeset) + expect(result).not.toContain('Tracked by this config:') + expect(result).toContain('- monitor "mine"') + }) }) }) diff --git a/test/yaml/idempotency.test.ts b/test/yaml/idempotency.test.ts index 891f792..4c4bbf4 100644 --- a/test/yaml/idempotency.test.ts +++ b/test/yaml/idempotency.test.ts @@ -151,7 +151,7 @@ describe('idempotency', () => { expect(cs.updates).toHaveLength(0) }) - it('removing one monitor → that monitor in deletes (with prune)', async () => { + it('removing one monitor → that monitor in deletes (with prune-org-cli)', async () => { const config: DevhelmConfig = { monitors: [ {name: 'API', type: 'HTTP', config: {url: 'https://api.example.com', method: 'GET'}}, @@ -169,7 +169,7 @@ describe('idempotency', () => { managedBy: 'CLI', }, managedBy: 'CLI'}, ]) - const cs = await diff(config, refs, {prune: true}) + const cs = await diff(config, refs, {pruneOrgCli: true}) expect(cs.deletes).toHaveLength(1) expect(cs.deletes[0].refKey).toBe('Web') expect(cs.creates).toHaveLength(0) From 94b2b8fd2fb67ecc5bb276d6b055b1827af9bde9 Mon Sep 17 00:00:00 2001 From: caballeto Date: Wed, 6 May 2026 00:24:27 +0200 Subject: [PATCH 8/8] chore(cli): polish topic descriptions, fix a/an typos, clarify --webhook-url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DevEx P1.Bug7 + P1.Bug8 (CLI polish): 1. Topic descriptions in `--help` previously inherited the first subcommand's description ("monitors → Create a new monitor", "alert-channels → Create a new alert channel"). Add explicit per-topic descriptions in `package.json`'s `oclif.topics` for every top-level topic and the nested subtopics under `auth`, `data`, `monitors`, and `status-pages`. 2. CRUD command descriptions hardcoded `Get a ${name}` / `Update a ${name}` / `Delete a ${name}`, which produced ungrammatical "a alert channel", "a API key", "a environment", "a incident". Add an `aOrAn(word)` helper to `crud-commands.ts` that picks the article based on the first letter and use it from get/update/delete factories. Tested directly so future resource names with vowel starts stay covered. 3. The `--webhook-url` flag on `alert-channels create` / `update` inherited the OpenAPI description "Slack incoming webhook URL", but the same flag works for webhook, slack, discord, and teams channels. Override it locally with "Webhook URL (used by webhook, slack, discord, teams channel types)" so users don't think it's slack-only. Co-authored-by: Cursor --- package.json | 79 +++++++++++++++++++++++++++++++++- src/lib/crud-commands.ts | 18 ++++++-- src/lib/resources.ts | 8 +++- test/lib/crud-commands.test.ts | 31 +++++++++++++ 4 files changed, 130 insertions(+), 6 deletions(-) create mode 100644 test/lib/crud-commands.test.ts diff --git a/package.json b/package.json index 854bae6..294fbac 100644 --- a/package.json +++ b/package.json @@ -47,7 +47,84 @@ "@oclif/plugin-help", "@oclif/plugin-not-found" ], - "topicSeparator": " " + "topicSeparator": " ", + "topics": { + "alert-channels": { + "description": "Manage alert channels (slack, webhook, email, pagerduty, opsgenie, teams, discord)" + }, + "api-keys": { + "description": "Manage API keys for the DevHelm public API" + }, + "auth": { + "description": "Manage CLI authentication, contexts, and current identity" + }, + "auth:context": { + "description": "Switch between saved CLI contexts (workspace + token pairs)" + }, + "data": { + "description": "Inspect runtime data — service catalog status, monitor results, and uptime windows" + }, + "data:services": { + "description": "Look up the current status and metadata of upstream services" + }, + "dependencies": { + "description": "Track upstream service dependencies (Stripe, GitHub, etc.) for incident correlation" + }, + "environments": { + "description": "Manage logical environments (production, staging, …) used to scope monitors and policies" + }, + "forensics": { + "description": "Inspect detection forensics: rule evaluations, state transitions, policy snapshots, check traces" + }, + "incidents": { + "description": "Create, inspect, and resolve incidents" + }, + "monitors": { + "description": "Manage HTTP, TCP, DNS, ICMP, MCP, and heartbeat monitors" + }, + "monitors:versions": { + "description": "Inspect historical monitor configuration versions" + }, + "notification-policies": { + "description": "Manage escalation policies that route incidents to alert channels" + }, + "resource-groups": { + "description": "Manage resource groups bundling monitors and services for shared health rollups" + }, + "secrets": { + "description": "Manage workspace secrets used in monitor configurations and headers" + }, + "skills": { + "description": "Generate and install Cursor / Claude agent skill packages for DevHelm" + }, + "state": { + "description": "Manage the local deploy state file (.devhelm/state.json)" + }, + "status-pages": { + "description": "Manage public status pages with components, branding, custom domains, and incidents" + }, + "status-pages:components": { + "description": "Manage components on a status page (monitor, group, or static rows)" + }, + "status-pages:domains": { + "description": "Manage custom domains attached to a status page" + }, + "status-pages:groups": { + "description": "Manage component groups that organise rows on a status page" + }, + "status-pages:incidents": { + "description": "Manage status-page-only incidents (separate from monitor-driven incidents)" + }, + "status-pages:subscribers": { + "description": "Manage email and webhook subscribers for a status page" + }, + "tags": { + "description": "Manage tags applied to monitors for filtering, grouping, and routing" + }, + "webhooks": { + "description": "Manage outbound webhook subscriptions for monitor and incident lifecycle events" + } + } }, "engines": { "node": ">=18.0.0" diff --git a/src/lib/crud-commands.ts b/src/lib/crud-commands.ts index 5843539..00e2b07 100644 --- a/src/lib/crud-commands.ts +++ b/src/lib/crud-commands.ts @@ -125,6 +125,18 @@ export interface AfterHookContext { apiPath: string } +/** + * Pick "a" or "an" based on the next word's first sound. Used for the + * autogenerated CRUD command descriptions (`Get a monitor by id`, + * `Delete an alert channel`) so resource names that start with a vowel + * read naturally. Heuristic on the first letter only — covers every + * resource name we ship today without a full pronunciation table. + */ +export function aOrAn(word: string): 'a' | 'an' { + const first = word.trim().charAt(0).toLowerCase() + return 'aeiou'.includes(first) ? 'an' : 'a' +} + export function createListCommand(config: ResourceConfig) { class ListCmd extends Command { static description = `List all ${config.plural}` @@ -161,7 +173,7 @@ export function createGetCommand(config: ResourceConfig) { const idLabel = config.idField ?? 'id' const getResponseSchema = (config.getResponseSchema ?? config.responseSchema) as ZodType class GetCmd extends Command { - static description = `Get a ${config.name} by ${idLabel}` + static description = `Get ${aOrAn(config.name)} ${config.name} by ${idLabel}` static examples = [`<%= config.bin %> ${config.plural} get <${idLabel}>`] static args = {[idLabel]: idArg(config)} static flags = {...globalFlags} @@ -209,7 +221,7 @@ export function createUpdateCommand(config: UpdatableResource) { const resourceFlags = config.updateFlags ?? config.createFlags ?? {} const builder = config.updateBodyBuilder ?? config.bodyBuilder class UpdateCmd extends Command { - static description = `Update a ${config.name}` + static description = `Update ${aOrAn(config.name)} ${config.name}` static examples = [`<%= config.bin %> ${config.plural} update <${idLabel}>`] static args = {[idLabel]: idArg(config)} static flags = {...globalFlags, ...resourceFlags} @@ -236,7 +248,7 @@ export function createUpdateCommand(config: UpdatableResource) { export function createDeleteCommand(config: ResourceConfig) { const idLabel = config.idField ?? 'id' class DeleteCmd extends Command { - static description = `Delete a ${config.name}` + static description = `Delete ${aOrAn(config.name)} ${config.name}` static examples = [ `<%= config.bin %> ${config.plural} delete <${idLabel}>`, `<%= config.bin %> ${config.plural} delete <${idLabel}> --yes`, diff --git a/src/lib/resources.ts b/src/lib/resources.ts index fff4100..786e1df 100644 --- a/src/lib/resources.ts +++ b/src/lib/resources.ts @@ -311,13 +311,17 @@ export const ALERT_CHANNELS: FullResource = { options: [...CHANNEL_TYPES], }), config: Flags.string({description: 'Channel-specific configuration as JSON'}), - 'webhook-url': urlFlag({description: desc('SlackChannelConfig', 'webhookUrl', 'Slack/Discord/Teams webhook URL')}), + 'webhook-url': urlFlag({ + description: 'Webhook URL (used by webhook, slack, discord, teams channel types)', + }), }, updateFlags: { name: Flags.string({description: desc('UpdateAlertChannelRequest', 'name')}), type: Flags.string({description: 'Alert channel type', options: [...CHANNEL_TYPES]}), config: Flags.string({description: 'Channel-specific configuration as JSON'}), - 'webhook-url': urlFlag({description: desc('SlackChannelConfig', 'webhookUrl', 'Slack/Discord/Teams webhook URL')}), + 'webhook-url': urlFlag({ + description: 'Webhook URL (used by webhook, slack, discord, teams channel types)', + }), }, // bodyBuilder produces a plain object; the outer // `apiSchemas.Create/UpdateAlertChannelRequest` Zod parse validates the diff --git a/test/lib/crud-commands.test.ts b/test/lib/crud-commands.test.ts new file mode 100644 index 0000000..a04e952 --- /dev/null +++ b/test/lib/crud-commands.test.ts @@ -0,0 +1,31 @@ +import {describe, expect, it} from 'vitest' +import {aOrAn} from '../../src/lib/crud-commands.js' + +describe('aOrAn (P1.Bug8)', () => { + it.each([ + ['monitor', 'a'], + ['secret', 'a'], + ['tag', 'a'], + ['notification policy', 'a'], + ['resource group', 'a'], + ['webhook', 'a'], + ['dependency', 'a'], + ['status page', 'a'], + ])('"%s" → "%s"', (word, expected) => { + expect(aOrAn(word)).toBe(expected) + }) + + it.each([ + ['alert channel', 'an'], + ['API key', 'an'], + ['environment', 'an'], + ['incident', 'an'], + ['organization', 'an'], + ])('"%s" → "%s"', (word, expected) => { + expect(aOrAn(word)).toBe(expected) + }) + + it('strips leading whitespace before checking', () => { + expect(aOrAn(' alert channel')).toBe('an') + }) +})