3 Commits

Author	SHA1	Message	Date
shankar0123	0e29c416b1	refactor(handler,repo): replace strings.Contains error dispatch with typed sentinels (S-2) ... Closes one 2026-04-24 audit finding (P2): - cat-s6-efc7f6f6bd50: 30 strings.Contains(err.Error(), ...) sites in internal/api/handler/ — brittle to repository-layer message changes, untyped against the actual failure mode. Approach (Option B from prompt design notes): - New typed sentinels in internal/repository/errors.go: ErrNotFound, ErrForeignKeyConstraint IsForeignKeyError(err) helper (the only place substring matching at the lib/pq boundary is allowed; isolates the DB-driver string knowledge to one function). - New typed sentinel in internal/domain/errors.go: ErrValidation (reserved for future per-entity validation wrappers; not yet used by all handlers). - 49 sites in internal/repository/postgres/*.go updated to wrap sql.ErrNoRows-derived errors via fmt.Errorf("...: %w", repository.ErrNotFound). - 18 not-found handler sites + 2 FK-constraint handler sites refactored to errors.Is(err, repository.ErrNotFound) / repository.IsForeignKeyError(err). - 23 inline `fmt.Errorf("X not found")` test fixtures across handler tests rewrapped to wrap repository.ErrNotFound. - test_utils.go::ErrMockNotFound rewrapped to wrap repository.ErrNotFound; renewal_policy.go closure docblock updated to reflect the new convention. - integration test mockJobRepository.Get wraps repository.ErrNotFound. CI regression guardrail: - .github/workflows/ci.yml::"Forbidden strings.Contains(err.Error()) regression guard (S-2)" greps for the three patterns ("not found", "violates foreign key", "RESTRICT") under internal/api/handler/ and fails the build on regression. Verification: - go build ./... — clean - go vet ./... — clean - go test ./... -short -count=1 — all packages pass (handler + repository + service + integration) - golangci-lint v2.11.4 run ./... — 0 issues - S-2 guardrail dry-run on post-fix tree → empty (good) - All sibling guardrails (S-1, G-3, D-1+D-2, B-1, L-1, H-1, C-1, F-1, P-1) pass Audit findings closed: - cat-s6-efc7f6f6bd50 (P2) Deferred follow-ups: - 6 domain-specific substring patterns still inline in handlers ("cannot approve", "cannot reject", "cannot be parsed", "no certificates found", "challenge password", "invalid"/ "required" validation chains in profiles + agent_groups). Each needs its own typed sentinel, scoped per service. Documented by the S-2 CI guardrail's allowlist for closure-comments only. - Per-entity not-found sentinels (Option A — ErrCertificateNotFound, ErrAgentNotFound, etc.) deferred. Generic ErrNotFound covers the current dispatch needs; per-entity precision would let handlers return entity-aware error bodies without a domain.Type field, but not blocking.	2026-04-25 17:54:14 +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	3a9fe8ba37	Complete V1 scaffold	2026-03-14 20:01:53 -04:00