fix(audit): record what actually happened, not just what was submitted by mashkovd · Pull Request #182 · mctlhq/mctl-api

mashkovd · 2026-08-16T15:12:13Z

audit_events had one INSERT and two SELECTs. No UPDATE anywhere. Every row was written with status submitted and stayed that way forever, so the audit log recorded intent and never outcome — the actual result of an operation could only be learned by going and asking Argo yourself.

This is the residual left open by F20.

Two paths, because one is not enough

The webhook. HandleArgoWorkflowComplete already existed, already verified an HMAC (gitops#828), and already returned 200 — it just logged and dropped the event. It now maps phase through PhaseToOpStatus and closes the row.

That covers what WorkflowNamespace routes to argo-workflows: create-tenant, delete-tenant*, openclaw-, platform-skill-, mctl-agents-*.

It cannot cover the rest, and not for want of retries. The exit hook reads ARGO_WEBHOOK_SECRET from an ExternalSecret that exists only in the argo-workflows namespace and exits silently when absent:

if not secret: raise SystemExit(0)

So deploy-service, provision-database, retire-service, preview-deploy, scale-service, rollback-service — the bulk of operations by volume — never send an event at all.

Copying the secret into tenant namespaces would fix the coverage and destroy the authentication: any tenant workload could then read it and forge completion events for other tenants' workflows. So the second path is a reconciler that reads Argo directly using mctl-api's own RBAC — every 3 minutes, over rows still submitted within a 7-day lookback.

A workflow that is simply gone gets expired rather than a guess, and only after 72h (Argo's ttlStrategy.secondsAfterFailure), so a row is never closed while its workflow merely has not been created yet.

UPDATE in place, not a second row

GetByWorkflow reads ORDER BY timestamp DESC LIMIT 1. A "completion" row carries no params, so it would become the newest match and break auditEntryTenant — which is what authorizes GET /workflows/{name}/logs for non-admins. Updating in place also preserves the submission timestamp, so ListWorkflows and ListAudit stay ordered by time submitted.

Idempotency needs no coordination

The UPDATE carries AND status NOT IN ('succeeded','failed','error','expired'). Whichever path arrives second changes zero rows. Redelivery and a webhook racing the reconciler are both no-ops — no locking, no dedupe table.

phaseToOpStatus is now exported rather than duplicated, so exactly one mapping from Argo phases to audit statuses exists in the codebase.

Index

Added on audit_events(workflow). GetByWorkflow was already doing a sequential scan on every workflow read, and UpdateStatus would have added another per completion.

Created CONCURRENTLY in its own Exec — it cannot run inside a transaction and pgx wraps multi-statement Execs in one. Partial (WHERE workflow <> ''), since most denied rows have no workflow. A failure logs a warning instead of failing startup: the index is a performance property, not a correctness one.

Tests

Three terminal phases close the row; Running leaves it alone; redelivery cannot reopen a closed row; an unknown workflow is still acknowledged with 200 (cron workflows have no audit row); plus the in-memory logger's matching, maxAge/limit handling, and message-preservation behaviour.

go build, go vet, go test ./... (15 packages) all pass.

audit_events had one INSERT and two SELECTs. No UPDATE anywhere. Every row was written with status "submitted" and stayed that way forever, so the audit log recorded intent and never outcome — the actual result of an operation could only be learned by going and asking Argo yourself. This is the residual left open by F20. Two paths close a row, because one is not enough. The Argo completion webhook already existed, already verified an HMAC (gitops#828), and already returned 200 — it just logged and dropped the event on the floor. It now maps phase through PhaseToOpStatus and closes the row. That covers create-tenant, delete-tenant*, openclaw-*, platform-skill-* and mctl-agents-* — everything WorkflowNamespace routes to argo-workflows. It cannot cover the rest, and not for want of retries. The exit hook reads ARGO_WEBHOOK_SECRET from an ExternalSecret that exists only in the argo-workflows namespace and exits silently when it is absent, so deploy-service, provision-database, retire-service, preview-deploy, scale-service and rollback-service — the bulk of operations by volume — never send an event at all. Copying the secret into tenant namespaces would fix the coverage and destroy the authentication: any tenant workload could then read it and forge completions for other tenants' workflows. So the second path is a reconciler that reads Argo directly with mctl-api's own RBAC, every 3 minutes, over rows still "submitted" within a 7-day lookback. A workflow that is simply gone gets status "expired" rather than a guess, and only after 72h — Argo's ttlStrategy.secondsAfterFailure — so a row is not closed while its workflow merely has not been created yet. UPDATE in place, not a second "completion" row: GetByWorkflow reads ORDER BY timestamp DESC LIMIT 1, so a completion row (which carries no params) would become the newest match and break auditEntryTenant, which is what authorizes GET /workflows/{name}/logs for non-admins. Updating in place also keeps the submission timestamp, so ListWorkflows and ListAudit stay ordered by time submitted. Idempotency needs no coordination between the two paths: the UPDATE carries `AND status NOT IN (terminal)`, so whichever arrives second changes zero rows. Redelivery, and a webhook racing the reconciler, are both no-ops. phaseToOpStatus is now exported rather than duplicated, so there stays exactly one mapping from Argo phases to audit statuses. Index on audit_events(workflow) added — GetByWorkflow was already doing a sequential scan on every workflow read, and UpdateStatus would have added another per completion. Created CONCURRENTLY in its own Exec because that cannot run inside a transaction and pgx wraps multi-statement Execs in one; partial, since most denied rows have no workflow. A failure there logs a warning instead of failing startup — the index is a performance property, not a correctness one. Tests cover the three terminal phases, that Running leaves the row alone, that redelivery cannot reopen a closed row, that an unknown workflow is still acknowledged (cron workflows have no audit row), and the in-memory logger's matching behaviour including message preservation.

cursor · 2026-08-16T15:12:18Z

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

claude · 2026-08-16T15:12:40Z

Claude finished @mashkovd's task in 6m 5s —— View job

Review complete — changes requested

Gather context (diff, changed files, CLAUDE.md)
Review internal/audit/logger.go and postgres.go changes
Review internal/api/audit_reconcile.go (new reconciler)
Review internal/api/handlers_write.go (webhook handling)
Review cmd/api/main.go and internal/operations/executor.go
Review test coverage
Post inline findings
Submit final verdict

Posted a REQUEST_CHANGES review with 3 P2 and 1 P3 inline findings, all on the new auditReconciler:

P2 — ListByStatus orders timestamp DESC and this call caps results at batch (200); under a sustained backlog of >200 stuck submitted rows, older rows are never examined and can silently age out of the 7-day lookback without ever closing.
P2 — GetWorkflowStatus is called with no per-item timeout; ctx is the long-lived process context (cancelled only at shutdown) and the k8s REST config sets no client-side timeout, so one hung call can stall the whole pass.
P2 — no test coverage for reconcileOnce's branching (registry-miss, empty-namespace, NotFound-within-grace vs. past-gcGrace-expiry, non-terminal-phase skip, terminal-phase close). internal/api/smoke_test.go's fakeExecutor already implements the relevant interface method, but the reconciler takes the concrete *operations.Executor/*operations.Registry instead, so it can't be reused as-is.
P3 — the terminal-status list is hardcoded in the UpdateStatus SQL and duplicates audit.TerminalStatuses in Go; a drift risk if a status is ever added to one but not the other.

The webhook-closure path itself (HandleArgoWorkflowComplete), the idempotent UpdateStatus guard, and the in-memory logger changes all look correct and are well tested (redelivery no-op, terminal-status guard, running-phase no-op, unknown-workflow ack, message preservation). I wasn't able to run go build/go vet/go test in this environment (tool permission restrictions on Bash), so please confirm CI is green.

github-actions · 2026-08-16T15:14:46Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Here is the review of the provided diff.

Finding 1: Missing Context Timeout in Reconciler I/O

Severity: P2
File: internal/api/audit_reconcile.go (approx. line 106)
Scenario:
In reconcileOnce, the call to r.exec.GetWorkflowStatus(ctx, ns, e.WorkflowName) uses the server's root ctx, which is only cancelled upon graceful server shutdown. If the Kubernetes API server hangs, becomes unresponsive, or silently blackholes the TCP connection, GetWorkflowStatus will block indefinitely. Because the reconciler is running in a single loop on a single goroutine, this will permanently wedge the reconciler, preventing any subsequent workflows from being marked as completed until the mctl-api process is restarted.
Proposed Fix:
Wrap the network call in a timeout-scoped context for each loop iteration:

reqCtx, reqCancel := context.WithTimeout(ctx, 10*time.Second)
defer reqCancel()
status, err := r.exec.GetWorkflowStatus(reqCtx, ns, e.WorkflowName)

Finding 2: Workflow Starvation Due to Queue Query Limit

Severity: P2
File: internal/api/audit_reconcile.go (approx. line 79) and internal/audit/postgres.go (approx. line 233)
Scenario:
The reconciler queries pending operations using ListByStatus("submitted", ... batch), which limits the result to the newest batch (200) workflows via ORDER BY timestamp DESC LIMIT $3. If there are ever more than 200 concurrently active or stalled workflows (e.g., workflows that failed to create and are waiting out the 72-hour gcGrace period), the oldest workflows will be pushed past the 200th position and entirely excluded from the query results. Since the reconciler is the primary/only mechanism for closing out tenant-namespace workflows, users with older workflows will see their operations starved and stuck in "submitted" state indefinitely until the backlog drops below 200.
Proposed Fix:
Either remove the batch limit entirely for this read query (scanning all pending rows within the 7-day lookback in a background worker is inexpensive) or implement cursor-based pagination so the reconciler iterates through all pending workflows rather than just repeatedly querying the top 200.

Finding 3: Missing Test Coverage for Reconciler Logic

Severity: P2
File: internal/api/audit_reconcile.go
Scenario:
The diff introduces audit_reconcile.go with 150 lines of complex control flow (ticker loops, deriving namespaces from templates, translating Argo phases, evaluating GC grace periods) but completely omits unit tests for this behavior. If future changes break how terminal states are evaluated or grace periods are enforced, they will fail silently in production.
Proposed Fix:
Add an audit_reconcile_test.go suite to cover reconcileOnce behavior. Specifically, add table-driven tests that verify: successful status closures, skipping workflows missing from K8s during the grace period, correctly expiring them after the grace period, and bypassing operations removed from the registry.

Finding 4: Swallowed Kubernetes API Errors

Severity: P3
File: internal/api/audit_reconcile.go (approx. line 114)
Scenario:
If GetWorkflowStatus returns an error that is not an IsNotFound during the GC grace period (such as an RBAC denial, 500 Internal Server Error, or network failure), the error is completely swallowed by the continue statement. Operators will have zero logging visibility to diagnose why the reconciler is failing to process pending workflows.
Proposed Fix:
Log the error explicitly before continuing when it's an unexpected failure:

if err != nil {
    if apierrors.IsNotFound(err) && time.Since(e.Timestamp) > r.gcGrace {
        // ... UpdateStatus("expired") logic ...
    } else if !apierrors.IsNotFound(err) {
        slog.Error("audit reconciler: failed to get workflow status", "workflow", e.WorkflowName, "error", err)
    }
    continue
}

claude · 2026-08-16T15:18:09Z

+}
+
+func (r *auditReconciler) reconcileOnce(ctx context.Context) {
+	pending := r.log.ListByStatus("submitted", r.lookback, r.batch)


P2: ListByStatus (both the Postgres and in-memory implementations) orders by timestamp DESC and this call caps the result at r.batch (200). Under a sustained backlog of more than 200 concurrently-submitted rows, this always returns the newest 200 — older rows are never examined by any pass as long as >= batch newer rows remain unresolved. Given the 7-day lookback, a stuck older row can silently age out of the window and never get closed, which is exactly the failure mode this reconciler exists to fix. Consider ordering oldest-first (or otherwise prioritizing age) so a backlog drains deterministically instead of potentially starving the tail.

claude · 2026-08-16T15:18:19Z

+			continue
+		}
+
+		status, err := r.exec.GetWorkflowStatus(ctx, ns, e.WorkflowName)


P2: this call has no per-request timeout/deadline. ctx here is the long-lived process context passed from cmd/api/main.go (only cancelled on shutdown), and rest.Config in operations.NewExecutor sets no client-side Timeout either. If the Kubernetes/Argo API is slow or unreachable for one workflow lookup, this single call can block indefinitely, stalling the whole reconcileOnce pass (and therefore every other pending row in the batch) until it returns or the process shuts down. Consider wrapping this call in a short context.WithTimeout per item, similar to how PostgresLogger.UpdateStatus/ListByStatus bound their own calls.

claude · 2026-08-16T15:18:29Z

+	}
+}
+
+func (r *auditReconciler) reconcileOnce(ctx context.Context) {


P2: no test coverage for reconcileOnce's branching (operation removed from registry → skip, empty namespace → skip, NotFound within grace → skip, NotFound past gcGrace → expired, other error → retry later, non-terminal phase → skip, terminal phase → UpdateStatus). This is genuinely intricate logic and the diff adds tests only for the webhook path and the in-memory logger, not this reconciler. The repo already has fakeExecutor (internal/api/smoke_test.go:161) implementing WorkflowExecutor.GetWorkflowStatus, which is close to what's needed here — but auditReconciler depends on the concrete *operations.Executor/*operations.Registry rather than an interface, so that fake can't be reused as-is. Worth either accepting the small interfaces (there's precedent in internal/api/interfaces.go's WorkflowExecutor) or adding a k8s fake-dynamic-client-backed test, so the gc-grace/expiry logic in particular gets exercised before merge.

claude · 2026-08-16T15:18:36Z

+		    SET status = $2,
+		        message = CASE WHEN $3 = '' THEN message ELSE $3 END
+		  WHERE workflow = $1
+		    AND status NOT IN ('succeeded', 'failed', 'error', 'expired')`,


P3: this hardcoded status list duplicates audit.TerminalStatuses (internal/audit/logger.go:60). If a status is ever added there, this WHERE clause silently drifts out of sync (redelivery could then reopen a row the Go layer already considers terminal). Not urgent, but worth building this list from TerminalStatuses or adding a test that asserts parity.

claude

Has P1/P2 findings, changes requested: 3 P2, 1 P3. The webhook-closure path and in-memory logger are well covered by tests and look correct (redelivery no-op, terminal-status guard, message-preservation all verified). The new auditReconciler is where the concerns are: (1) ListByStatus's ORDER BY timestamp DESC LIMIT batch can starve older backlog rows out of the 7-day lookback under sustained load; (2) GetWorkflowStatus is called with no per-item timeout, so one hung k8s call can stall the whole pass; (3) none of reconcileOnce's branching has test coverage. Plus one P3 nit on a duplicated terminal-status literal in SQL.

Addresses the four findings on #182. P2 — starvation. ListByStatus returned newest-first and reconcileOnce caps at batch (200). With a sustained backlog above that cap the oldest rows would never be examined by any pass, which is precisely backwards: an old submitted row is the one most likely to be genuinely stuck. Both implementations now return oldest-first, and the interface documents it as a work queue rather than a feed. TestReconcileDrainsOldestFirst fails against the old ordering. P2 — unbounded lookups. GetWorkflowStatus ran on the process-lifetime context, and neither rest.Config nor the caller sets a client timeout, so one slow or unreachable read could stall an entire pass. Each lookup now gets its own 10s deadline; the fake asserts the context it receives actually carries one. P2 — the reconciler's branching had no tests. It does now, one per branch: terminal phases close the row (all three), a tenant-namespaced operation is looked up in the tenant's namespace, Running is left alone, NotFound inside the grace window is left alone while NotFound past it expires with a reason, a transport error leaves the row for the next pass, an unknown operation and a missing tenant are both skipped without a lookup, rows outside the lookback are ignored, and an already-terminal row is never touched. Testing it needed a seam: the reconciler now holds a one-method workflowStatusGetter rather than *operations.Executor, which the executor satisfies unchanged. P3 — the terminal status list was written out in the UPDATE's WHERE clause and duplicated audit.TerminalStatuses. Drift there would let redelivery reopen a row the Go layer considers closed, so the clause is now `NOT (status = ANY($4))` fed from TerminalStatuses directly. Also: Logger.Log now honours a caller-supplied Timestamp instead of always stamping now. Production paths leave it zero and are unaffected; the age-dependent tests set it rather than reaching into the logger's internals.

claude · 2026-08-16T15:24:32Z

Claude finished @mashkovd's task in 1m 47s —— View job

Review complete — approved

Gather context (diff, changed files, CLAUDE.md)
Review full diff 6784cce...d514d43
Verify prior P2 findings (drain order, per-item timeout, reconciler tests) are addressed
Check remaining files (webhook, postgres, logger)
Post inline findings
Submit final verdict

Posted an APPROVE review bound to commit d514d43.

This commit (d514d43, "drain oldest-first, bound each lookup, test the reconciler") directly resolves everything flagged in the prior review of f940638:

Drain order (was P2) — ListByStatus now orders oldest-first: ORDER BY timestamp ASC in internal/audit/postgres.go, and the in-memory internal/audit/logger.go iterates entries in insertion order. A backlog larger than batch (200) now drains deterministically from the tail instead of starving old rows out of the 7-day lookback.
Per-item timeout (was P2) — audit_reconcile.go now wraps each GetWorkflowStatus call in context.WithTimeout(ctx, r.lookupTimeout) (10s default), so one hung Argo/k8s read can no longer stall the whole pass on the process-lifetime context.
Test coverage (was P2) — new workflowStatusGetter interface + fakeStatusGetter in audit_reconcile_test.go (253 lines) exercise every branch: terminal phases close the row, Running is left alone, NotFound within/past gcGrace (leave vs. expire), transport errors leave the row open, unknown-registry operation and empty-namespace both skip the lookup entirely, oldest-first draining under batch=1, lookback exclusion, and already-terminal rows are never re-queried.
Terminal-status duplication (was P3) — postgres.go's UpdateStatus now passes audit.TerminalStatuses as a query parameter (NOT (status = ANY($4))) instead of a hardcoded SQL literal, so the two lists can no longer drift.

One new P3 (non-blocking) on audit_reconcile.go:113: non-NotFound errors from GetWorkflowStatus (RBAC denial, 500, timeout) are silently swallowed with no log line, so an operator can't distinguish "stuck, Argo unreachable" from "not resolved yet." Worth an slog.Warn at some point, doesn't block merge.

The webhook-closure path, in-memory logger, and status-mapping export (phaseToOpStatus → PhaseToOpStatus) were unchanged since the prior review and remain correct. Wasn't able to run go build/go vet/go test in this environment (tool permission restrictions on Bash) — please confirm CI is green.

claude · 2026-08-16T15:26:09Z

+		}
+		ns := operations.WorkflowNamespace(op.WorkflowTemplate, auditEntryTenant(&e))
+		if ns == "" {
+			continue


P3: non-NotFound errors from GetWorkflowStatus (RBAC denial, 500, timeout from lookupTimeout firing) are silently swallowed here — the row is left for the next pass, which is correct, but there's no log line, so an operator has no way to tell "this row is stuck because Argo is unreachable" from "this row just hasn't been resolved yet." Consider an slog.Warn on the non-NotFound branch. Not blocking — the earlier P2s (drain order, per-item timeout, test coverage) are what mattered and are all fixed in this commit.

claude

No P1/P2 findings (1 P3). All three P2s from the prior review are fixed in this commit: ListByStatus now drains oldest-first (ORDER BY timestamp ASC in postgres, insertion-order iteration in-memory), GetWorkflowStatus is called under a per-item context.WithTimeout (lookupTimeout, 10s default), and audit_reconcile_test.go adds thorough table-driven coverage of reconcileOnce's branching via a new workflowStatusGetter interface + fakeStatusGetter (terminal phases, running, NotFound within/past gcGrace, transport errors, unknown operation, missing namespace, oldest-first draining under a small batch, lookback exclusion, already-terminal rows). The P3 (terminal-status duplication) is also fixed — postgres.go's UPDATE now takes TerminalStatuses as a query parameter instead of a hardcoded literal. Good to merge.

github-actions · 2026-08-16T15:26:35Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Finding 1

Severity: P1
File: internal/api/audit_reconcile.go
Line: ~90-150 (inside reconcileOnce)
Concrete failure scenario: The reconciler implements a work queue by fetching the oldest 200 pending entries (r.log.ListByStatus("submitted", r.lookback, r.batch)). If a workflow cannot be immediately resolved to a terminal state—because it is still running in Argo, is missing but younger than the 72h gcGrace, experiences a transient network error, or has an unresolvable namespace—the loop correctly executes continue without updating the database. However, because the row's state is untouched, it remains one of the oldest "submitted" entries and will be fetched again on the next pass. If 200 or more such workflows accumulate, they will permanently fill the entire batch limit. The reconciler will repeatedly poll these same 200 items every 3 minutes and completely starve out newer workflows (head-of-line blocking). No new completed workflows will be closed out until the stuck rows age past the 72-hour grace period or the 7-day lookback window.
Proposed fix: To prevent starvation, the reconciler must be able to iterate past unresolved rows. Modify ListByStatus to support pagination (e.g., via a timestamp cursor or offset) so reconcileOnce can page completely through the pending backlog in chunks. Alternatively, add a last_reconciled_at timestamp column to the audit_events schema, update this timestamp on each check, and order the queue query by last_reconciled_at ASC NULLS FIRST so that checked but unresolved rows are naturally sent to the back of the line.

Finding 2

Severity: P3
File: internal/audit/postgres.go
Line: ~210-218 (inside UpdateStatus)
Concrete failure scenario: The PostgreSQL UPDATE query matches strictly on workflow = $1 and NOT (status = ANY($4)), which updates all non-terminal rows sharing that workflow name. In contrast, the in-memory UpdateStatus iterates backwards from the end of the slice and updates exactly one row (the newest match). If a workflow name is reused (e.g., a run is deleted and recreated identically) and multiple runs are left in the "submitted" state, the PostgreSQL logger will overwrite all historical runs with the newest outcome. This diverges from the in-memory behavior and corrupts the outcome records of the older runs.
Proposed fix: Restrict the Postgres update to the most recent matching row to align with the in-memory behavior. You can achieve this using a subquery:
WHERE id IN (SELECT id FROM audit_events WHERE workflow = $1 AND NOT (status = ANY($4)) ORDER BY timestamp DESC LIMIT 1)

Finding 3

Severity: P3
File: internal/audit/postgres.go
Line: ~232-238 (inside ListByStatus)
Concrete failure scenario: The queue query in ListByStatus filters by status, workflow, and timestamp, ordering by timestamp ASC. Currently, the database only has an index on (timestamp DESC). To find the oldest "submitted" rows, Postgres will scan the timestamp index and filter out the vast majority of rows that are already terminal (e.g., "succeeded"). As the audit_events table grows into the millions of rows, filtering out historical data to find the oldest pending entries will become highly inefficient, unnecessarily increasing database load on every 3-minute reconciler tick.
Proposed fix: Create a composite index to efficiently support the reconciler's queue query:
CREATE INDEX CONCURRENTLY IF NOT EXISTS audit_events_queue ON audit_events (status, timestamp ASC) WHERE workflow <> ''

Addresses the P3 on #182. A row left for the next pass is correct, but an RBAC denial, a 500 or a lookupTimeout firing looked identical to "not resolved yet" from outside. NotFound stays quiet: inside the grace window it is the expected case, not a fault.

claude · 2026-08-16T15:29:26Z

Claude finished @mashkovd's task in 1m 9s —— View job

Review complete — approved

Gather context (diff, changed files, CLAUDE.md)
Review full diff 6784cce...17e0677
Verify prior findings addressed by latest commit (17e0677, non-NotFound logging)
Post inline findings (none — commit is a clean, correctly-scoped fix)
Submit final verdict

Posted an APPROVE review bound to commit 17e0677.

Between d514d43 (previously approved) and 17e0677, only internal/api/audit_reconcile.go changed — a single, precisely-scoped 9-line addition:

if !apierrors.IsNotFound(err) {
    slog.Warn("audit reconcile: workflow lookup failed",
        "workflow", e.WorkflowName, "namespace", ns, "error", err)
}

This resolves the P3 raised in the prior review round (audit_reconcile.go:113): non-NotFound errors from GetWorkflowStatus (RBAC denial, 500, lookupTimeout firing) were previously swallowed with no log line. Now they surface via slog.Warn while still leaving the row for the next pass, so an operator can tell "stuck because Argo is unreachable/denying us" apart from "not resolved yet." The NotFound-within-grace path is correctly left silent, since that's the expected steady state while a workflow hasn't been created yet. No test was added for this specific log line, but it's a non-branching side effect with no correctness impact, so that's not blocking.

No other files changed in this commit. All prior P2s (oldest-first draining, per-item lookup timeout, reconcileOnce test coverage) and the earlier P3 (terminal-status duplication) remain fixed as verified in the previous review round.

Wasn't able to run go build/go vet/go test in this environment (tool permission restrictions on Bash) — please confirm CI is green.

claude

No P1/P2 findings (0 new P3). This commit is a single, precisely-scoped fix: it adds an slog.Warn on the non-NotFound branch of reconcileOnce in internal/api/audit_reconcile.go, resolving the P3 raised on the prior commit (d514d43) — operators can now distinguish a row stuck because Argo/k8s is unreachable or denying the request from one that simply hasn't resolved yet. The NotFound-within-grace path remains silent as intended. No other files changed since d514d43, which was already approved. Good to merge.

github-actions · 2026-08-16T15:32:53Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Here are the findings from the review:

1. Missing error wrapping in I/O operations

Severity: P3
File: internal/operations/executor.go (approx. line 245)
Scenario: GetWorkflowStatus returns the raw error directly from the Kubernetes dynamic client's Get call. If the lookup fails due to a network timeout, RBAC denial, or other transport issue, upstream callers (like the audit reconciler) will log a generic error (e.g., "connection refused" or "forbidden") without the crucial context of which workflow name and namespace failed the lookup.
Proposed Fix: Wrap the error with context before returning:
return nil, fmt.Errorf("get workflow status %s/%s: %w", namespace, name, err)

2. Missing test coverage for new database operations

Severity: P2
File: internal/audit/postgres.go (approx. lines 165, 190)
Scenario: The PR introduces critical and somewhat complex SQL queries in PostgresLogger (UpdateStatus and ListByStatus) involving conditional updates (CASE WHEN $3 = '' ...) and array checks (NOT (status = ANY($4))). However, unlike the in-memory Logger which received thorough coverage in status_test.go, the PostgreSQL implementations lack test coverage. Regressions, type-mapping bugs with pgx, or SQL syntax errors here will go uncaught in CI.
Proposed Fix: Add integration tests for PostgresLogger.UpdateStatus and PostgresLogger.ListByStatus that run against a real PostgreSQL database (e.g., using testcontainers) to ensure parity with the in-memory logger behavior.

… index it Three findings from the agy reviewer, all correct. P1 — head-of-line blocking, which my own previous fix created. Draining oldest-first stopped newer rows from starving the old ones, and introduced the mirror image: a row that cannot be resolved yet is left untouched, so it stays among the oldest and is re-read every pass. A few hundred still-Running workflows would occupy the whole batch indefinitely and no newer completed workflow would ever be closed. ListByStatus now takes a timestamp cursor and reconcileOnce pages through the backlog with it, bounded by maxPerPass (2000) so a large backlog cannot turn one tick into an unbounded walk. TestReconcilePagesPastUnresolvableRows fails against single-page behaviour with "the pass never got past the stuck rows". TestReconcileDrainsOldestFirst was asserting the old single-page semantics, so it now checks the order of lookups rather than that only one happened — oldest-first is the property that matters, and paging is what makes it safe. P3 — the Postgres UPDATE matched every non-terminal row sharing a workflow name while the in-memory implementation updated exactly one. Argo names carry a generated suffix so collisions are unlikely, but "close every historical run with this name" is not a behaviour to leave to chance in an audit table. Scoped to the newest match via a subquery. P3 — added a composite index on (status, timestamp) for the queue query. The existing timestamp-only index would have Postgres walk it discarding every already-terminal row, which is nearly all of them once the table grows.

claude · 2026-08-16T15:35:27Z

Claude finished @mashkovd's task in 3m 43s —— View job

Review complete — changes requested

Gather context (diff, changed files, CLAUDE.md)
Review full diff 6784cce...fa0a152, focused on 17e0677...fa0a152 (this commit's changes)
Verify prior findings addressed (queue starvation, missing composite index)
Post inline findings
Submit final verdict

Posted a REQUEST_CHANGES review bound to commit fa0a152, with 1 P2 and 1 P3 inline finding.

This commit (fa0a152, "page the reconciler queue, scope UpdateStatus to one row, index it") correctly fixes the two Antigravity findings from the prior round:

Queue starvation (fixed) — ListByStatus now takes a cursor (after time.Time) and reconcileOnce pages through the backlog (bounded by maxPerPass) instead of re-reading the same top-200 page every pass. Well tested (TestReconcilePagesPastUnresolvableRows, TestReconcileRespectsMaxPerPass, TestLoggerListByStatusCursor).
Missing composite index (fixed) — audit_events_queue ON audit_events (status, timestamp ASC) WHERE workflow <> '' now backs the reconciler's queue query.

But the fix for the other Antigravity finding (cross-workflow UPDATE scope) introduces a new concurrency bug:

P2 — internal/audit/postgres.go:241: UpdateStatus now scopes to the newest row via WHERE id IN (SELECT id FROM audit_events WHERE workflow = $1 AND NOT (status = ANY($4)) ORDER BY timestamp DESC LIMIT 1). That subquery is uncorrelated, so Postgres evaluates it once as a cached InitPlan — it is not re-run when EvalPlanQual re-checks the top-level predicate after a lock wait. Concretely: if the webhook and the reconciler race to close the same workflow (a race this PR explicitly designs to be safe), both can select the same id while it's still submitted. The first to commit wins normally; the second, unblocked afterward, re-evaluates only id IN (<cached id>) — trivially still true since id is unchanged — and proceeds to overwrite the now-terminal row. This breaks the "whichever path arrives second changes zero rows" guarantee described in the PR body. Fix: keep AND NOT (status = ANY($4)) on the outer, directly-evaluated WHERE in addition to the subquery, so it's actually re-checked post-lock.
P3 — internal/audit/logger.go:182: the cursor's strict after comparison (mirrored in Postgres as timestamp > $4) can skip rows that share the exact same timestamp as the page boundary, for the rest of that pass. Self-heals on the next reconcileOnce (cursor resets to zero), so it's a 3-minute delay at worst — not blocking, but worth a comment since it reads like a bug on first pass.

Wasn't able to run go build/go vet/go test in this environment (tool permission restrictions on Bash) — please confirm CI is green.

github-actions · 2026-08-16T15:38:23Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Here are the findings from reviewing the diff:

Finding 1
Severity: P2
File: internal/audit/postgres.go
Approximate Line: 231 (in UpdateStatus)
Concrete failure scenario: The UPDATE query uses a subquery to lock and update the newest non-terminal row. However, the outer WHERE clause only filters by id IN (...). In PostgreSQL's default Read Committed isolation level, if two status updates run concurrently for the same workflow (e.g. from the webhook and the reconciler racing each other), both transactions can evaluate the subquery before either commits, selecting the same row id. The second transaction will wait for the first to commit, but will then successfully overwrite the row with its own status because the outer WHERE id IN (...) remains true and does not re-check the status. This violates the guarantee that terminal statuses must not be reopened.
Proposed fix: Add the terminal status check to the outer query so it is re-evaluated after the row is locked: WHERE id IN (...) AND NOT (status = ANY($4)), or use SELECT id ... FOR UPDATE in the subquery.

Finding 2
Severity: P2
File: internal/api/audit_reconcile.go
Approximate Line: 104 (in reconcileOnce)
Concrete failure scenario: The reconciler initializes cursor to zero at the start of each pass and processes rows until it hits maxPerPass (2000). If the number of unresolvable rows (e.g. workflows genuinely stuck in "Running" for days or continuously returning transient API errors) exceeds maxPerPass within the 7-day lookback window, the reconciler will repeatedly scan the exact same 2000 oldest rows on every tick and abort. Newer workflows will suffer from head-of-line blocking and be completely starved; their status will never be reconciled until the oldest 2000 rows age out of the lookback window.
Proposed fix: Maintain the pagination cursor state across passes if the queue doesn't fully drain, or implement a last_reconciled_at column in the database to ensure fair and exhaustive scheduling.

Finding 3
Severity: P2
File: internal/audit/postgres.go (Line 245) and internal/api/audit_reconcile.go (Line 104)
Concrete failure scenario: The reconciler uses strict inequality on the timestamp for pagination: timestamp > $4. If multiple audit rows share the exact same timestamp (which is entirely possible with concurrent submissions) and span across a pagination batch boundary, the rows placed into the subsequent batch will be skipped. If the rows in the first batch remain unresolvable, the cursor is continuously set to their timestamp, causing the skipped rows to be permanently starved out in every pass.
Proposed fix: Use a deterministic, unique cursor for pagination, such as (timestamp, id) > (cursor_ts, cursor_id), and update both ListByStatus and the reconciler loop to page by both fields.

Finding 4
Severity: P2
File: internal/audit/postgres.go
Approximate Line: 73 (in NewPostgresLogger)
Concrete failure scenario: NewPostgresLogger is called in the application startup path and executes CREATE INDEX CONCURRENTLY. While CONCURRENTLY prevents taking an exclusive write lock on the table in the database, the pool.Exec() call itself is synchronous and will block the Go application until the index is fully built. For a large audit table, scanning and building the index can take minutes. This will cause the API server to hang on startup, fail Kubernetes liveness/readiness probes, and enter a CrashLoopBackOff.
Proposed fix: Execute index creation asynchronously in a separate goroutine, or remove it from the application startup path entirely and manage the database schema via a dedicated schema migration pipeline.

Finding 5
Severity: P3
File: internal/audit/postgres.go
Approximate Line: 165
Concrete failure scenario: The PR implements complex SQL queries, subqueries, and cursor-based pagination logic in the PostgresLogger (specifically UpdateStatus and ListByStatus). However, testing (in internal/audit/status_test.go) is only provided for the in-memory Logger. The PostgresLogger implementation and its edge cases—such as the cursor exact-match behavior and concurrent update races—are completely untested, violating the repository guidance on test coverage for changed behavior.
Proposed fix: Add integration tests for PostgresLogger in a postgres_test.go file to verify UpdateStatus and ListByStatus, heavily focusing on concurrent updates, pagination boundaries, and terminal status immutability.

claude · 2026-08-16T15:38:45Z

+		        SELECT id FROM audit_events
+		         WHERE workflow = $1 AND NOT (status = ANY($4))
+		         ORDER BY timestamp DESC
+		         LIMIT 1


P2: scoping the update to the newest row via id IN (SELECT ... LIMIT 1) moves the NOT (status = ANY($4)) terminal guard out of the top-level WHERE and into an uncorrelated subquery. That subquery has no outer-row dependency, so Postgres evaluates it once as an InitPlan and caches the resulting id — it is not re-run when EvalPlanQual re-checks the top-level predicate after a lock wait.

Concretely: if the webhook and the reconciler race to close the same workflow, both can select the same id (row still submitted when each subquery runs). The first to acquire the row lock updates and commits. The second, unblocked afterward, re-evaluates only id IN (<cached id>) — which still trivially matches, since id didn't change — and proceeds to overwrite the now-terminal row (e.g. clobbering succeeded with a stale failed, or just the message). That's exactly the "whichever path arrives second changes zero rows" guarantee this PR's description calls out as the point of the guard, and it no longer holds under genuine concurrency (which the reconciler racing the webhook is designed to allow).

Suggested fix: keep the guard on the outer, directly-evaluated predicate too, so EvalPlanQual re-checks it against the post-lock row version:

UPDATE audit_events SET status = $2, message = CASE WHEN $3 = '' THEN message ELSE $3 END WHERE workflow = $1 AND NOT (status = ANY($4)) AND id IN ( SELECT id FROM audit_events WHERE workflow = $1 AND NOT (status = ANY($4)) ORDER BY timestamp DESC LIMIT 1 )

claude · 2026-08-16T15:38:54Z

+		if e.Status != status || e.Timestamp.Before(cutoff) {
+			continue
+		}
+		if !after.IsZero() && !e.Timestamp.After(after) {


P3: the cursor advances with a strict after comparison (mirrored in postgres.go's timestamp > $4), keyed off pending[len(pending)-1].Timestamp. If two or more rows in the same status share that exact timestamp and the page boundary falls between them (batch-sized page ends mid-tie), the remaining tied rows are skipped for the rest of this pass — the next page's timestamp > cursor excludes them too since they're not strictly greater. Not a permanent loss (the next reconcileOnce call restarts the cursor at the zero time and re-lists from the beginning), just a one-pass (3-minute) delay. Worth a code comment if this is accepted as intentional, since it's easy to mistake for a correctness bug on first read.

claude

Has P1/P2 findings, changes requested: 1 P2, 1 P3. This commit correctly addresses the prior Antigravity findings on queue starvation (cursor-based paging in ListByStatus, bounded by maxPerPass) and the missing composite index (audit_events_queue), both well tested. However, the fix for cross-workflow UPDATE scoping introduces a new issue: in internal/audit/postgres.go's UpdateStatus, the terminal-status guard now lives only inside an uncorrelated subquery (id IN (SELECT ... LIMIT 1)), which Postgres evaluates once as a cached InitPlan rather than re-checking via EvalPlanQual when a concurrent UPDATE forces a lock wait — so two genuinely concurrent close attempts on the same workflow (webhook racing the reconciler, which this PR explicitly designs for) can both succeed, breaking the 'whichever path arrives second changes zero rows' guarantee the PR describes. See inline comment for a one-line fix (duplicate the guard onto the outer WHERE). Also a P3 nit on the cursor's strict timestamp comparison under timestamp ties.

Addresses the P2 on #182, which is a defect I introduced in the previous commit while fixing a different one. Scoping the update to the newest row moved `NOT (status = ANY($4))` into an `id IN (SELECT ...)` subquery. That subquery is uncorrelated, so Postgres evaluates it once as an InitPlan and caches the id — and EvalPlanQual, which re-checks the predicate after a lock wait, only re-evaluates the top level. So when the webhook and the reconciler race the same workflow, both subqueries run while the row is still submitted and select the same id. The winner closes it. The loser wakes, re-checks `id IN (<cached id>)`, still matches, and overwrites a row that is already terminal — a stale `failed` landing on top of `succeeded`. Exactly the idempotency the guard was there to provide, undone by moving it. The guard is now in both places. The subquery copy picks the right row; the top-level copy is the one EvalPlanQual re-evaluates, and is what actually makes the update idempotent. The comment says so, because a reader will otherwise delete one of them as redundant. Also documents the P3: the cursor uses a strict `>` comparison, so a page boundary landing mid-tie skips the tied remainder for the rest of that pass. Nothing is lost — the next pass restarts the cursor at zero — so the cost is one 3-minute cycle, which is not worth threading a compound (timestamp, id) cursor through the interface to avoid.

claude · 2026-08-16T15:43:14Z

Claude finished @mashkovd's task in 1m 12s —— View job

Review complete — approved

Gather context (diff, changed files, CLAUDE.md)
Review full diff 6784cce...ea96cf2, focused on fa0a152...ea96cf2 (this commit's changes)
Verify prior P2 finding (uncorrelated-subquery race) is fixed
Post inline findings (none — commit is a clean, correctly-scoped fix)
Submit final verdict

Posted an APPROVE review bound to commit ea96cf2.

This commit (ea96cf2, "keep the terminal guard at the top level of the UPDATE") is a precise, two-line fix for the P2 flagged on the prior commit (fa0a152):

WHERE id IN (
      SELECT id FROM audit_events
       WHERE workflow = $1 AND NOT (status = ANY($4))
       ORDER BY timestamp DESC
       LIMIT 1
)
  AND NOT (status = ANY($4))   -- new: duplicated on the outer WHERE

The subquery that picks "the newest matching row" is uncorrelated (no reference to the outer query), so Postgres evaluates it once as a cached InitPlan. Under READ COMMITTED, when a concurrent transaction updates the same row first, the waiting transaction's EvalPlanQual re-check only re-evaluates predicates that reference the target row's current columns — the outer, directly-evaluated WHERE clause — not the cached subquery result. With the guard living solely inside the subquery (the previous state), a webhook/reconciler race could both select the same id while it was still submitted, and the loser would overwrite an already-terminal row after waking from the lock wait.

Duplicating AND NOT (status = ANY($4)) on the outer predicate means it is re-evaluated post-lock, so the loser's UPDATE now correctly matches zero rows — restoring the "whichever path arrives second changes zero rows" idempotency guarantee this PR is built around, even under genuine webhook/reconciler concurrency.

The only other change in this commit is a clarifying comment added to internal/audit/logger.go documenting the strict-timestamp-cursor tradeoff (no logic change).

All prior findings across earlier commits (drain order, per-item lookup timeout, reconciler test coverage, terminal-status duplication, queue starvation/pagination, composite index, non-NotFound logging) remain fixed as verified in previous review rounds.

Wasn't able to run go build/go vet/go test in this environment (tool permission restrictions on Bash) — please confirm CI is green.

claude

No P1/P2 findings (0 P3). This commit fixes exactly the P2 raised on fa0a152: the terminal-status guard AND NOT (status = ANY($4)) is now duplicated on the outer, directly-evaluated WHERE clause of UpdateStatus in internal/audit/postgres.go, in addition to the uncorrelated subquery. Since Postgres caches an uncorrelated subquery as an InitPlan and does not re-run it during EvalPlanQual's post-lock-wait re-check, the guard now living on the outer predicate is what actually gets re-evaluated against the row's current state — restoring the 'whichever path arrives second changes zero rows' idempotency guarantee under a genuine webhook/reconciler race. The only other change is a clarifying comment in internal/audit/logger.go (no logic change). Good to merge.

github-actions · 2026-08-16T15:45:35Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

No significant issues found.

golangci-lint gocritic rangeValCopy: audit.Entry is 192 bytes and the loop runs over every pending row on each tick.

claude · 2026-08-16T15:49:05Z

Claude finished @mashkovd's task in 39s —— View job

Review complete — approved

Gather context (diff ea96cf2...8219607)
Review changes in this commit
Post inline findings (none — commit is style-only)
Submit final verdict

Posted an APPROVE review bound to commit 8219607.

This commit (8219607, "index the reconcile loop instead of copying entries") is a trivial, non-behavioral style fix: reconcileBatch now ranges by index (e := &pending[i]) instead of by value, avoiding a per-iteration copy of audit.Entry (192 bytes) — the kind of fix golangci-lint's gocritic rangeValCopy flags. e is used strictly for reads inside the loop body, so there's no aliasing hazard from taking a pointer into the slice.

All prior P1/P2 findings from earlier review rounds on this PR (drain order, per-item lookup timeout, reconciler test coverage, terminal-status duplication, queue pagination/starvation, composite index, non-NotFound logging, the uncorrelated-subquery UPDATE race) remain fixed and unaffected by this commit.

Wasn't able to run go build/go vet/go test in this environment (tool permission restrictions on Bash) — please confirm CI is green.

claude

Trivial style-only change: switches the reconcileBatch loop from range-by-value to an indexed pointer (avoids copying audit.Entry, a 192-byte struct, per gocritic rangeValCopy). No logic change — e is used read-only, no aliasing hazard. All prior P1/P2 findings across earlier commits remain fixed.

github-actions · 2026-08-16T16:02:40Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Severity: P2
File: internal/audit/postgres.go
Line: ~253-264 (ListByStatus function)

Concrete failure scenario:
In ListByStatus, the code iterates over database results using for rows.Next() { ... } but fails to check rows.Err() after the loop terminates. If a transport, database, or connection error occurs mid-stream while paging through the result set, rows.Next() will abort and return false. Without checking rows.Err(), the method silently swallows the error and returns the truncated, partial list of rows as if it were a complete and successful page.

When this happens, the auditReconciler processes the partial page. Because len(pending) < r.batch, the reconciler assumes it has cleanly reached the end of the backlog, breaking its loop prematurely. The underlying database error goes completely unlogged, masking potential connectivity or stability issues between the API server and PostgreSQL.

Proposed fix:
Check rows.Err() after the loop finishes to ensure no errors occurred during iteration:

	for rows.Next() {
		e, err := scanAuditEntry(rows.Scan)
		if err != nil {
			slog.Error("audit postgres: scan failed", "error", err)
			continue
		}
		entries = append(entries, e)
	}
	
	if err := rows.Err(); err != nil {
		slog.Error("audit postgres: rows error", "status", status, "error", err)
	}
	
	return entries

claude Bot reviewed Aug 16, 2026

View reviewed changes

claude Bot requested changes Aug 16, 2026

View reviewed changes

claude Bot reviewed Aug 16, 2026

View reviewed changes

claude Bot previously approved these changes Aug 16, 2026

View reviewed changes

mashkovd dismissed claude[bot]’s stale review via 17e0677 August 16, 2026 15:29

claude Bot previously approved these changes Aug 16, 2026

View reviewed changes

mashkovd dismissed claude[bot]’s stale review via fa0a152 August 16, 2026 15:35

claude Bot reviewed Aug 16, 2026

View reviewed changes

claude Bot requested changes Aug 16, 2026

View reviewed changes

claude Bot previously approved these changes Aug 16, 2026

View reviewed changes

style(audit): index the reconcile loop instead of copying entries

8219607

golangci-lint gocritic rangeValCopy: audit.Entry is 192 bytes and the loop runs over every pending row on each tick.

mashkovd dismissed claude[bot]’s stale review via 8219607 August 16, 2026 15:48

claude Bot approved these changes Aug 16, 2026

View reviewed changes

mashkovd merged commit 525b528 into main Aug 16, 2026
7 checks passed

mashkovd deleted the fix/audit-status-closure branch September 2, 2026 21:23

mashkovd mentioned this pull request Oct 18, 2026

governance: define authority and decision-context contract with execution-time revalidation #700

Closed

5 tasks

Conversation

mashkovd commented Aug 16, 2026

Two paths, because one is not enough

UPDATE in place, not a second row

Idempotency needs no coordination

Index

Tests

Uh oh!

cursor Bot commented Aug 16, 2026

Uh oh!

claude Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete — changes requested

Uh oh!

github-actions Bot commented Aug 16, 2026

Antigravity (agy) review — informational

Finding 1: Missing Context Timeout in Reconciler I/O

Finding 2: Workflow Starvation Due to Queue Query Limit

Finding 3: Missing Test Coverage for Reconciler Logic

Finding 4: Swallowed Kubernetes API Errors

Uh oh!

claude Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete — approved

Uh oh!

claude Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 16, 2026

Antigravity (agy) review — informational

Finding 1

Finding 2

Finding 3

Uh oh!

claude Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete — approved

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 16, 2026

Antigravity (agy) review — informational

Uh oh!

claude Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete — changes requested

Uh oh!

github-actions Bot commented Aug 16, 2026

Antigravity (agy) review — informational

Uh oh!

claude Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

claude Bot commented Aug 16, 2026 •

edited

Loading

claude Bot commented Aug 16, 2026 •

edited

Loading

claude Bot commented Aug 16, 2026 •

edited

Loading

claude Bot commented Aug 16, 2026 •

edited

Loading

claude Bot commented Aug 16, 2026 •

edited

Loading

claude Bot commented Aug 16, 2026 •

edited

Loading