Skip to content

Fix review findings: console 403, governance safety, consumer supervision, learning-loop plumbing - #74

Merged
CodexManvik merged 7 commits into
masterfrom
fix/review-findings
Sep 22, 2026
Merged

CodexManvik merged 7 commits into
masterfrom
fix/review-findings

Conversation

@CodexManvik

Copy link
Copy Markdown
Owner

Fixes from a whole-repo review of master @ 1ca3d3e. One commit per area. Every behaviour change has a regression test, and each default was picked so that nothing changes unless it was actually broken.

Demo blocker

  • Every Approve / Reject / "Draft with AI" click was a 403. PR fix(demo): operator identity from env + enable embedding selector #72 made the console act as VITE_OPERATOR_NAME, which defaults to demo-operator. That name had no role in rbac_policy.yaml, and the console image never set the variable. This PR grants demo-operator the operator and approver roles and pins the name as a build ARG. A new test reads the default from api.ts and the Dockerfile and checks it against the policy.

Governance safety

  • POST /playbooks had no permission check. Anyone who could reach governance could overwrite a seed playbook with hitl_mode: auto, skipping both the HITL gate and the AI-proposal review. It now requires ?registered_by= with the new register permission (coe-admin). It refuses auto unless the actor may also graduate, and writes an audit record.
  • Human approve/reject decisions are now audited. They were only ever stored in the approvals row.
  • /decide accepts only approved or rejected. Anything else is a 422; previously a typo was stored and later reported as aborted:timeout. The decider needs the matching permission, and deciding a request that isn't pending is a 409. Proposal approve/reject get the same pending check, so a rejected AI runbook can't later be approved into the live registry.
  • Timed-out approvals get expired. Action stopped waiting, but the request stayed pending forever and the console offered an Approve button that no longer did anything. Action now calls POST /approvals/{id}/expire on timeout (only the requester may expire, best effort).
  • Graduation needs real evidence. Dry-run remediation always reports success, so three approved simulations could graduate a playbook to AUTO, and later switching to real k8s remediation meant real changes with no human in the loop. TrainingRecord now carries mode (an optional field in the JSON payload, so no migration). Simulated successes don't count unless GRADUATION_COUNT_SIMULATED is set.
  • Graduation is reversible. Nothing ever moved an auto playbook back to human approval. Feedback now calls the new POST /playbooks/{id}/demote when an auto playbook fails or rolls back.

Reliability

  • Consumer threads died silently on any unhandled exception. Examples: a governance restart surfacing as a ConnectError in the HTTP gate, or a Postgres blip during an audit write. /health is static, so the pod was never restarted. common/supervise.py now restarts a crashed worker with capped backoff. Every consumer runs under it, plus the correlation flusher and all four read-model threads.
  • Helm Deployments now use strategy: Recreate. The default RollingUpdate briefly ran two pods consuming under the same consumer name during every upgrade, which is exactly what ADR-033's replicas:1 assumption rules out.
  • Suppressed situations were lost. The engine kept them in a single slot, so a flush that suppressed two buckets reported one. It is now a queue.
  • The read model is thread-safe and handles recurring incidents. It gets an RLock. A recurrence of a finished incident starts a clean card instead of inheriting the last run's outcome and timeline. A late detection or diagnosis never moves a card's status backwards, and events from an earlier occurrence are ignored.

Learning loop

  • Stable signatures. The signature hashed the raw list of member fingerprints, so the same fault on the same service got a different signature depending on how many anomalous samples landed in the window (verified: 6 vs 7 samples gave two unrelated IDs). Everything keyed on the signature (suppression, graduation evidence, RCA reliability, the author's memory) only matched by luck. It now hashes the set of series involved.
  • Suppression needs a track record. One success gave reliability 1/1, which cleared any threshold. It now needs RELIABILITY_SUPPRESS_MIN_SAMPLES (default 3). RELIABILITY_SUPPRESS_THRESHOLD was never passed to the engine (dead config); it is now wired.
  • The RCA reliability boost now works. It was one number per signature added equally to every hypothesis, so it could never change the order. It is now per (signature, runbook). It also refreshes every 60s instead of only at boot.
  • Opt-in per-service detection: CORRELATION_ROBUST_KEY_BY=series. The robust detector pooled every service's samples of a metric into one baseline, so a 10→45 CPU jump on a quiet service scored ~0 next to a service idling at 80. The new mode keeps one baseline per metric and service. It's off by default: per-service windows fill about 5× slower, so the live warm-up needs re-tuning against a replay first.
  • No more blackout at the top of every hour. Each new hour's empty seasonal bucket made the detector score 0 until it re-warmed (about 2 minutes). A cold bucket now scores against the fullest warm bucket of the same series.
  • engine.reset() keeps the configuration. It used to silently reset window size and seasonal buckets to their defaults.

Remediation

  • Scale rollback restores the recorded original replica count. The old inverse delta was wrong after a clamp: 9+2 clamps to 10, and 10−2 = 8.
  • Unhealthy results with nothing undone are no longer called ROLLED_BACK. No rollback steps (restart-pod ships with none) now gives unhealthy:no-rollback, and a failed rollback gives unhealthy:rollback-failed.
  • Readiness waits for the rollout. It now also requires observedGeneration ≥ generation and updatedReplicas == replicas, so the old ReplicaSet can't pass the check before the patch has been observed.

Cleanup

  • Dropped the unused geist npm package. It was never imported, but it dragged in next and sharp (about 940 lockfile lines and the Dependabot alerts).
  • Removed the frontend's dead proposePlaybook call.
  • Completed the OutcomeReason type with the reasons it was missing.
  • Pinned ghcr.io/astral-sh/uv:0.11.28 instead of :latest (tag verified on the registry).

⚠️ Needs a decision: what "suppressed" should mean

Correlation's reliability map, which drives suppression, is still loaded only at boot, on purpose. A suppressed situation is dropped before RCA and action, so the fix is never applied. Suppression rarely fired before, because signatures were unstable and the map was boot-only. With stable signatures it will now fire after RELIABILITY_SUPPRESS_MIN_SAMPLES successes following a correlation restart. I'd recommend changing suppression to mean "auto-handle quietly" (skip paging a human but still remediate) rather than "drop". That's a product decision, so it isn't in this PR.

Not changed

  • The review's claim that deploy/playbooks lacks symptoms was wrong: both copies have them and a test pins it. The two copies still differ in their steps.
  • Changes to default posture: dry-run successes no longer graduate playbooks, and suppression needs 3 outcomes instead of 1.

Verification

  • uv run pytest -m "not postgres and not kafka": 831 passed (up from 782).
  • ruff check and ruff format --check are clean.
  • npm run build passes (contrast gate, tsc, vite).
  • helm lint passes with values-live.yaml, and all 7 service Deployments render Recreate.
  • Not run locally: the Postgres/Kafka-marked tests and a live kind cluster.

🤖 Generated with Claude Code

CodexManvik and others added 7 commits September 23, 2026 00:25
PR #72 made the console act as VITE_OPERATOR_NAME, defaulting to
"demo-operator", but that name was in no role in rbac_policy.yaml and the
console image never set the variable. Every Approve, Reject and
"Draft a runbook with AI" click from a stock build was a 403.

Grant demo-operator [operator, approver], pin it as a build ARG in
Dockerfile.frontend so the identity is explicit and overridable, and add a
test that reads the default from api.ts and the Dockerfile and checks it
against the policy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… make graduation reversible

- POST /playbooks took no identity: anyone reaching governance could overwrite
  a seed playbook with hitl_mode=auto and skip both the HITL gate and the
  AI-proposal review. It now requires ?registered_by= with the new `register`
  permission (coe-admin), refuses AUTO unless the actor may also graduate, and
  writes an audit record.
- A human's approve/reject was never written to the audit log. It is now.
- /decide accepted any string (a typo was stored and later reported as a
  timeout) and could re-decide a closed request. It now takes approved|rejected
  only (422), checks the matching approve/reject permission, and returns 409
  unless the request is still pending. Proposal approve/reject get the same
  pending check, so a rejected AI runbook can't be approved into the registry.
- A request action-service stopped waiting for stayed "pending" forever and the
  console kept offering a dead Approve button. Action now expires it on timeout
  (POST /approvals/{id}/expire, requester only, best effort).
- Graduation counted dry-run successes (DryRunRemediator + AlwaysHealthyChecker
  always succeed), so three approved simulations could make a playbook AUTO for
  a later real cluster. TrainingRecord now carries the outcome's mode (additive,
  payload-only) and simulated successes don't count unless
  GRADUATION_COUNT_SIMULATED is set.
- Nothing ever demoted an auto playbook. Feedback now calls the new
  POST /playbooks/{id}/demote when an auto playbook fails or rolls back.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…op losing suppressions

- Consumer threads died for good on any unhandled exception (a governance
  restart surfacing as a ConnectError from the HTTP gate, a Postgres blip in an
  audit write). /health is static, so Kubernetes never restarted the pod and
  the service sat "ready" processing nothing. common/supervise.py restarts a
  crashed worker with capped backoff; every service's consumer (and the
  correlation flusher, and all four read-model topic threads) now runs under it.
  At-least-once re-serves the in-flight entry and the DLQ bounds a poison one.
- The Helm Deployments used the default RollingUpdate, so each upgrade briefly
  ran two pods consuming under the same consumer name - the exact overlap
  ADR-033's replicas:1 assumption rules out. They are now strategy: Recreate.
- CorrelationEngine kept suppressed situations in a single slot, so a
  flush_all() that suppressed two buckets reported one. It is a queue now and
  the drain loops until empty.
- The read model had no lock while four consumer threads and the request
  threads mutated and pruned it. It now takes an RLock around every mutation
  and read. It is also occurrence-aware: a recurrence of a finished incident
  (same id, newer first_seen) starts a clean card instead of inheriting the last
  run's outcome and timeline; a late detection/diagnosis never moves a card's
  status backwards; and an event from an earlier occurrence is ignored.
- Fix the stale values.yaml comment claiming values-live turns the sandbox on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ries detection switch

- Signatures hashed the raw list of member fingerprints, so the same fault on
  the same service got a different signature depending on how many anomalous
  samples landed in the window (verified: 6 vs 7 samples -> two unrelated ids).
  Everything keyed on the signature - suppression, graduation evidence, RCA
  reliability, the author's past decisions - only matched by luck. It is now
  the hash of the SET of series involved.
- Suppression cleared on a single success (reliability 1/1 = 1.0). It now needs
  RELIABILITY_SUPPRESS_MIN_SAMPLES (default 3) labelled outcomes. The service
  also never passed RELIABILITY_SUPPRESS_THRESHOLD to the engine - dead config,
  now wired.
- RCA's reliability boost was one number per signature added equally to every
  runbook-bearing hypothesis: a uniform offset that could never change their
  order. It is now per (signature, runbook), so a proven runbook can overtake,
  with confidence as the tiebreak at the 1.0 clamp. It was also built once at
  boot; ReliabilityProvider re-reads the training store every
  RELIABILITY_REFRESH_SECONDS (default 60).
- RobustCorrelator pooled every service's samples of a metric into one
  baseline, so a 10 -> 45 CPU jump on a quiet service scored ~0 beside one
  idling at 80. New CORRELATION_ROBUST_KEY_BY=series keeps one baseline per
  (metric, service). Opt-in, default unchanged: per-series windows fill ~5x
  slower, so the live warm-up needs re-tuning against a replay first.
- Every top-of-hour opened an empty seasonal bucket and the robust detector
  scored 0 until it re-warmed. A cold bucket now scores against the fullest
  warm bucket of the same series.
- CorrelationEngine.reset() rebuilt the correlator from three kwargs, silently
  resetting window size / seasonal buckets to defaults. Correlators now expose
  clone_empty() with their full configuration.

Correlation's own reliability map (what drives suppression) is still loaded
only at boot, deliberately - see the PR description: suppressing a situation
drops it before RCA/action, so a live-refreshing map would stop remediating
faults the system has learned to fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… happened

- Scale was undone with the inverse delta, which is wrong whenever the forward
  step was clamped: 9 replicas + 2 clamps to 10, and 10 - 2 = 8, one below the
  start. KubernetesRemediator now records the pre-scale count and rollback
  restores exactly that.
- An unhealthy result was reported ROLLED_BACK even when the playbook had no
  rollback steps (restart-pod ships with none) or the rollback itself failed.
  Those are now FAILURE with "unhealthy:no-rollback" /
  "unhealthy:rollback-failed".
- The post-remediation readiness check compared ready == replicas only, which
  the OLD ReplicaSet satisfies before the controller has observed the patch. It
  now also requires observedGeneration >= generation and updatedReplicas ==
  replicas (both read defensively, so fakes/older clients are unaffected).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uv:latest

- geist was never imported (the fonts are vendored in frontend/src/fonts) but
  pulled in next + sharp - ~940 lockfile lines and the Dependabot "critical"
  alerts that never applied to this Vite app.
- proposePlaybook (the sync draft call) had no caller left in the console since
  the draft-async + Agent Activity flow replaced it. The backend endpoint stays.
- OutcomeReason claimed to be the exact health_after vocabulary but was missing
  preflight-failed, interrupted:unknown and the denied:unsafe-* reasons; add
  those and the two new unhealthy:* reasons.
- Pin ghcr.io/astral-sh/uv to 0.11.28 in both Dockerfiles instead of :latest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 22, 2026 19:37
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 1faf940c-2967-40cb-a3b0-4e6538491808


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@CodexManvik
CodexManvik merged commit 6417b00 into master Sep 22, 2026
6 checks passed
@CodexManvik
CodexManvik deleted the fix/review-findings branch September 22, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants