3 Commits

Author	SHA1	Message	Date
shankar0123	b3cc7cbdb2	fix(policies): close the D-006 loop — TitleCase seed canonicals + severity-aware, config-consuming rule engine (D-008) ... D-008 was a three-part drift in the policy engine that made the D-005/D-006 remediation cosmetic below the DB layer: (a) migrations/seed.sql INSERTed rules with pre-D-005 lowercase types ('ownership', 'environment', 'lifetime', 'renewal_window') that the handler validator rejects on Create/Update but that raw SQL INSERTs bypassed entirely. At runtime evaluateRule's switch fell through to the default "unknown policy rule type" error branch on every demo rule × every cert × every cycle, flooding logs while emitting zero violations. (b) migrations/seed_demo.sql persisted lowercase severity values ('critical', 'error', 'warning') on policy_violations rows. INSERT succeeded because that column had no CHECK, but any frontend comparing against the canonical PolicySeverity enum mis-categorized every seeded violation. (c) evaluateRule hardcoded Severity: PolicySeverityWarning on every emitted violation and ignored rule.Config entirely — so the D-006 per-rule severity column (000013) and every per-arm Config JSON ({allowed_issuer_ids, allowed_domains, required_keys, allowed, lead_time_days, max_days}) was dead data below the evaluation layer. This commit lands (a)+(b)+(c) atomically. Shipping any subset leaves the feature half-working. ## Changes Domain (internal/domain/policy.go): * Add PolicyTypeCertificateLifetime as the 6th TitleCase canonical. Pre-D-008 the seeded "max-certificate-lifetime" rule had no engine arm — routing it through RenewalLeadTime would conflate "how close to expiry before we renew" with "how long can the cert possibly be", two distinct semantics. The new type accepts config {"max_days": int} and flags certs whose NotAfter - NotBefore exceeds the cap. Handler validator (internal/api/handler/validation.go): * ValidatePolicyType allowlist grown to 6 canonicals (AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime, CertificateLifetime). OpenAPI (api/openapi.yaml): * PolicyType enum grown to match domain. Frontend (web/src/api/types.ts, types.test.ts): * POLICY_TYPES tuple gains CertificateLifetime; pin test asserts all 6 canonicals and rejects casing drift. Migration 000014 (policy_violations severity CHECK): * Named CHECK constraint (policy_violations_severity_check) mirroring 000013's allowlist, defense-in-depth at the DB layer against future drift from bypassed writes (migrations, psql sessions, future callers). Symmetric down migration drops by name. Seed data: * migrations/seed.sql rewritten to emit TitleCase canonicals with per-arm config JSON that actually exercises the config-consuming paths (not the missing-field backstops): - pr-require-owner → RequiredMetadata {"required_keys":["owner"]} Warning - pr-allowed-environments → AllowedEnvironments {"allowed":["production","staging","development"]} Error - pr-max-certificate-lifetime → CertificateLifetime {"max_days":90} Critical - pr-min-renewal-window → RenewalLeadTime {"lead_time_days":14} Warning Severities are now differentiated per rule (D-006 intent). * migrations/seed_demo.sql violation rows flipped to TitleCase severity ('Critical', 'Error', 'Warning') so migration 000014 applies cleanly on upgrade paths. Engine rewrite (internal/service/policy.go): * evaluateRule rewritten. All six arms now: 1. Parse rule.Config into the per-arm typed struct. 2. Bad JSON → log at ValidateCertificate boundary and skip this rule (no co-located poisoning of other rules in the same batch). 3. Empty/null Config → emit the pre-D-008 missing-field violation (backwards compat invariant — operators who haven't reconfigured still see the same output). 4. Violations emitted carry rule.Severity (no more hardcoded Warning); D-006 column is now load-bearing. * CertificateLifetime arm reads NotBefore/NotAfter from the certificate's latest version via CertRepo. Injected via PolicyService.SetCertRepo() setter — avoids churning ~36 NewPolicyService call sites while keeping the lifetime arm optional (degrades to a log+skip if the setter is not wired). Server wiring (cmd/server/main.go): * policyService.SetCertRepo(certRepo) wired after construction. Tests (internal/service/policy_test.go): * 25 new subtests across 5 groups: - TestEvaluateRule_SeverityPassThrough (6): every rule type emits violations carrying rule.Severity, not hardcoded. - TestEvaluateRule_ConfigConsumed (12): every per-arm Config path exercised positive + negative. - TestEvaluateRule_EmptyConfig_BackCompat (3): empty/null Config still emits pre-D-008 missing-field violations. - TestEvaluateRule_BadConfig_SkipsRule: malformed JSON logs and skips cleanly without poisoning neighbors. - TestEvaluateRule_CertificateLifetime_RepoScenarios (3): ok when repo wired, log+skip when not, handles missing NotBefore/NotAfter edges. Provenance: D-008 surfaced during D-005/D-006 remediation review in eef1db0. That commit added persistence and CI pins for the severity field but did not re-verify the evaluation layer consumed it; this finding and fix close the audit-process gap.	2026-04-18 14:55:56 +00:00
shankar0123	eef1db0f0a	fix(policies): stop 400ing the "+ New Policy" button + add per-rule severity (D-005, D-006) ... Coverage Gap Audit findings D-005 (P0) + D-006 (P1) fixed together in a single commit because they share the same root cause — policy CRUD sending values the backend silently rejects — and splitting them would leave a half-working UI between commits. ## D-005 (P0): PoliciesPage dropdown 400s every Create Policy Root cause ---------- `web/src/pages/PoliciesPage.tsx` populated the Type `<select>` from a hardcoded `['key_algorithm', 'ownership', 'allowed_issuers', ...]` array. The backend's `internal/api/handler/validators.go::ValidatePolicyType` enforces the TitleCase allowlist `AllowedIssuers`, `AllowedDomains`, `RequiredMetadata`, `AllowedEnvironments`, `RenewalLeadTime` — defined in `internal/domain/policy.go`. Every Create Policy request was rejected with `400 invalid policy type`. The error surfaced only as a transient toast; the modal closed anyway. Silent user-visible failure. Fix --- - `web/src/api/types.ts`: added `POLICY_TYPES` and `POLICY_SEVERITIES` tuples with `as const` and narrowed `PolicyRule.type`, `.severity`, and `PolicyViolation.severity` to the literal-union types. Dropdown is now sourced from the tuple; casing drift becomes a compile error. - `web/src/pages/PoliciesPage.tsx`: rekeyed `severityStyles` / `severityDots` to the TitleCase values, added `humanize()` for display (AllowedIssuers → "Allowed Issuers"), removed the `badge-neutral` fallback that was papering over the mismatch. - `web/src/api/types.test.ts` (new): pins both tuples exactly. If anyone edits one side of the frontend/backend contract without the other, CI fails with a clear assertion. Pure-TS vitest, no RTL dependency. ## D-006 (P1): `severity` field silently dropped on create/update Root cause ---------- `PolicyRule` had no `Severity` field in `internal/domain/policy.go`. The frontend has always sent `severity` on create/update, but Go's `json.Decoder` (default settings, no `DisallowUnknownFields`) silently dropped it. The value never reached PostgreSQL. Every rule rendered with the same severity because there was no severity — just a display computation downstream. Fix: option (b), full-stack schema add (not delete-the-field) ------------------------------------------------------------- - Migration `000013_policy_rule_severity` (up + down): adds `severity VARCHAR(50) NOT NULL DEFAULT 'Warning'` to `policy_rules` with CHECK constraint `severity IN ('Warning', 'Error', 'Critical')`. No index — three-value column on a low-thousands-rows table, planner will seq-scan regardless. PG 11+ metadata-only ADD COLUMN, safe on live data. - `internal/domain/policy.go`: added `Severity PolicySeverity` field. - `internal/repository/postgres/policy.go`: plumbed `severity` through ListRules SELECT + Scan, GetRule SELECT + Scan, CreateRule INSERT, UpdateRule UPDATE (4 queries). - `internal/service/policy.go::UpdatePolicy`: if the client omits severity on a PUT (zero-value empty string), fetch the existing rule and preserve its severity. Without this, partial updates would trip the NOT NULL CHECK and 500. Preserves pre-existing behavior for Name/Type (out of scope). - `internal/api/handler/policies.go::CreatePolicy`: default empty severity to `'Warning'`, then validate via `ValidatePolicySeverity`. 400 with clear message instead of 500 on CHECK violation. `UpdatePolicy`: validates severity only when provided. - `internal/mcp/types.go` + `internal/mcp/tools.go`: added optional `severity` on the MCP `create_policy` / `update_policy` tool inputs so LLM callers stay in sync with the wire contract. - `api/openapi.yaml`: added `severity` to the `PolicyRule` schema with the enum and default. Acceptance criterion (user-defined) ----------------------------------- "Create a rule with severity=Critical, reload the page, and still see Critical — no silent drops." Verified end-to-end: frontend sends `severity: "Critical"`, handler validates, service persists, DB stores, GET returns, React renders the correct badge. Seed data --------- `migrations/seed.sql`: four demo rules now have differentiated severities — `pr-require-owner` → Warning, `pr-allowed-environments` → Error, `pr-max-certificate-lifetime` → Critical, `pr-min-renewal-window` → Warning. The user called out that seeding all four at the same severity makes the feature look decorative; differentiation demonstrates the column carries real signal. ## Integration test fix (side effect of D-006) `internal/integration/e2e_test.go::TestCrossResourceWorkflow/CreatePolicy` was sending `"severity": "High"` — a value from the pre-audit severity vocabulary that the new `ValidatePolicySeverity` correctly rejects with 400. Changed to `"Error"` (closest semantic match in the new TitleCase allowlist). Only severity reference in the integration/ directory; verified via grep. ## Out of scope, logged for follow-up (d/D-008) Three policy-engine drift issues orthogonal to D-005 + D-006, explicitly deferred per direction: 1. `migrations/seed.sql` policy_rules INSERTs use lowercase TYPE values (`'ownership'`, `'environment'`, `'lifetime'`, `'renewal_window'`). These are load-bearing on `internal/service/policy.go::evaluateRule`'s `switch rule.Type` (which also uses the lowercase strings). Migrating requires coordinated changes across seed + evaluation engine. 2. `migrations/seed_demo.sql:482-483` contains lowercase `'critical'` severity — will now fail the new CHECK constraint. Separate fix. 3. `evaluateRule` hardcodes `Severity: domain.PolicySeverityWarning` on emitted violations and ignores the configured `rule.Config`. The new severity column is read correctly on the CRUD path but not yet consulted during evaluation. ## Verification Backend: - `go build ./...` — clean - `go vet ./...` — clean - `go test -short ./...` — all packages green, including `internal/service` (policy service), `internal/api/handler` (policy + MCP handler tests), `internal/integration` (e2e_test.go after fix), `internal/domain`, `internal/repository/postgres`. Frontend: - `tsc --noEmit` — clean - `vitest run` — 223/223 passing (4 new assertions in types.test.ts) - `vite build` — clean (only the pre-existing chunk-size warning)	2026-04-18 13:02:04 +00:00
shankar0123	1d1b89c9b5	Implement M3: expiration threshold alerting with dedup and status transitions ... - Add alert_thresholds_days JSONB column to renewal_policies (default [30,14,7,0]) - Add RenewalPolicy.AlertThresholdsDays field + EffectiveAlertThresholds() helper - Add RenewalPolicyRepository interface + postgres implementation - Rewrite CheckExpiringCertificates with per-policy threshold alerting - Add SendThresholdAlert + HasThresholdNotification for deduplication via [threshold:N] tags - Add Type and MessageLike filters to NotificationFilter + postgres query support - Auto-transition certs to Expiring (>0 days) or Expired (<=0 days) status - Record expiration_alert_sent audit events per threshold crossing - Fix .gitignore: allow SQL migration files, scope server/agent build artifact rules - Track previously untracked cmd/ and migrations/ directories - Update docs (README, architecture, demo-advanced) for threshold alerting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>	2026-03-15 00:03:43 -04:00