feat: audit operational agent settings changes - #27675
Conversation
Wire the seven deployment-wide operational agents settings endpoints (chat retention days, debug retention days, auto-archive days, workspace TTL, computer-use provider, debug-logging allow-users, and personal-model-overrides-enabled) into the enterprise audit log under one grouped agents_operational_settings resource whose diff names exactly the changed setting. Each PUT handler wraps a best-effort raw read of its own site_configs key and its existing upsert in one transaction, so the write path is byte-identical and malformed stored text still gets repaired. A shared helper holds the LockIDChatSettingsWrites advisory lock, captures Old and New only when the stored text actually changed, and suppresses the entry on no-op or capture failure, so concurrent identical PUTs emit one entry. The new key-parameterized GetChatSiteConfigValue query returns the raw stored text (empty when the row is absent) and cannot fail on malformed text, keeping the read-first Old capture from changing behavior. Its dbauthz contract pins to ResourceDeploymentConfig: update, the same permission the PUTs gate on. CODAGT-720
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
Round 1 of adversarial review on PR #27675. The added InTx wrapper changed every database-error response body: the upsert's original error came back as "execute transaction: <err>" instead of "<err>". Each handler now records the write callback's error in a local and reports that error directly, so the write-failure response body is byte-identical to main on all seven endpoints; transaction-level failures (begin, commit, the advisory lock) still surface through the InTx wrapper so they stay distinguishable. WriteFailureDetailMatchesMain pins the Detail for one endpoint, mutation-proven by restoring the wrapper; the seven-endpoint injection table is byte-identical to an unmodified origin/main control. Two claimed tests now pin what they claim. MalformedSettingRepair uses the mock-only NewMockWithDiffFn and asserts the committed diff is exactly the malformed old text to the repaired value; NewCaptureDegradesViaWarn exercises the new-capture failure branch (204, write persists, zero entries, degrade warn). Both are mutation-proven. CODAGT-720
Round 2 prep on PR #27675. Renumber the enum migration to 000562 after main took 000561 (content unchanged, enum-only). Restrict GetChatSiteConfigValue to the agents_ key namespace in SQL so a future caller cannot read unrelated site_configs secrets (derp_mesh_key, webpush keys) through it, bypassing their narrower dedicated-getter gates. NonAgentsKeyReadsEmpty pins the excluded case and that the seven agents_ keys still read exactly. operationalSettingsForKey now panics on an unknown key, matching the audit package's switch convention, instead of silently returning an all-zero struct that would suppress the entry for a setting wired without updating the switch. TestOperationalSettingsForKeyPanicsOnUnknownKey pins the panic and the happy path. CODAGT-720
Round 3 on PR #27675. Part A: the audit transaction and advisory lock changed what a member sees on failure. Each handler now runs the audited path (transaction, lock, Old capture, write, New re-read, comparison) and, on any transaction-level or audited-write failure, falls back to the direct write (what main does today) and derives the response from it, emitting no diff. The full InTx error is logged before the response detail is selected so rollback detail never reaches the body. Measured against an unmodified origin/main control: lock, write, Old-read, and New-read failures all return 204, persist the value, and emit no entry on all seven endpoints, identical to main's plain write. Fallback regression tests are mutation-proven. Part B (plan D5, S2 slice): rename the resource type to chat_operational_settings and give each row a per-setting audit resource target. The target rides on the struct as an ActionIgnore name field, derived from the same chatOperationalSetting descriptor as the storage key so a key and its label cannot drift. The audit filter label is added to the hand-maintained exceptions in AuditFilter.tsx. Stable per-setting IDs, denied-attempt rows, and degraded-capture markers are deliberately out of scope here and land in S1. CODAGT-720
Round 3 follow-up on PR #27675. The post-commit-after-callback position already emits the ordinary entry on this head: the fallback repeats the idempotent upsert, so client status, entry StatusCode, and the real Old-to-New diff agree (204, 204, real diff). PostCommitFallbackEmitsOrdinaryEntry pins the triple via a commit-failing driver, and removing the keep-Old-New-across-the-fallback behavior is mutation-proven. DivergentValueFallbackBaseline covers a client retrying with a corrected value after a failed audited write: the fallback persists the new value with no entry, and the next audited change diffs from the fallback-persisted baseline, not from any pre-failure value. CODAGT-720
Round 4 on PR #27675, implementing the ratified audit-write structure. Delete the fallback structure entirely: no retry, no direct-write path, no failure classification, no degraded rows. Each handler runs one transaction shaped exactly like main's write. The Old and New captures are fatal on error, as in usersecrets.go: the raw read cannot fail independently of the write it shares a connection with, so a failure is a real database failure and the request fails exactly as main's write would. This also settles the cross-unit inconsistency: a genuine write failure now reproduces main's 500 instead of falling back. Replace the global LockIDChatSettingsWrites with seven per-setting advisory locks derived from the exact site_configs key with GenLockID, so writers of different settings never contend and the IDs cannot collide with the sequential LockID block or another subsystem's GenLockID output. The per-key lock covers the absent-row first-write case that FOR UPDATE cannot, and removes cross-setting contention. No-op suppression moves to InitRequestWithCancel, taken inside the transaction and applied after it returns. The rename, per-setting identity and targets, raw storage text, and the agents_ query constraint are unchanged. CODAGT-720
Round 4 follow-up on PR #27675. The fallback deletion took the round-1 write-error detail parity with it, but that fix was orthogonal to the fallback. Restore it: the write error is recorded in a local variable inside the callback and reported directly after InTx returns non-nil, so the write-failure Detail is the bare error main returns on all seven endpoints. The transaction error stays for begin, lock, commit, and rollback failures, which main never reaches. S1's system-prompt endpoint is transactional on main and keeps the wrapped detail; plan-mode and these seven are not transactional on main and return the bare error. Add TestChatOperationalLockIDsDistinct: the seven GenLockID-derived advisory lock IDs are asserted pairwise distinct and distinct from every enumerated sequential LockID* constant, replacing a probability argument with a fact. The sequential list is explicit so a future added constant is caught by review, not a production deadlock. CODAGT-720
Status, 30 July 2026: not ready to mergeHanding this over. CI is green on the current head, but there is a known defect that must be fixed first, shared with #27668. Defect: the handler re-reads the stored value after writing it, to record the new value. If that read fails while the transaction is still healthy, the request returns 500 and rolls back a write that already succeeded. Client cancellation is enough: transactions here are started with a background context, so cancelling a request cancels the individual query and not the transaction. On an unmodified build the write survives and only the response is lost, so this can silently discard an administrator's change. Fix: delete the post-write read and compute the new value from what was written; these upserts store the request value verbatim. Also outstanding:
What is sound: all seven endpoints wired to one resource type, each row naming its own setting through a descriptor that pairs the storage key with its label so the two cannot drift, raw stored text as the audited value with a test proving a malformed value's repair is recorded honestly, the new key-scoped read query restricted in SQL to the Deliberate limits, documented in the description: no per-setting history by resource id, no entries for denied attempts, and no marker when capture degrades. The sibling instruction PR carries those; this unit does not, because the ticket ranks these settings lower priority. Context and the wider plan: see the "Agents audit logging: handover" document on CODAGT-66.
|
Round 4 correction on PR #27675 (the fatal-read premise was falsified). Delete the post-write read and set New from the value written (the upserts store the request value verbatim), removing the data-loss path where a New-read failure, including client cancellation since InTx begins with context.Background(), makes the handler 500 and rolls back a write that already succeeded. Old-capture failure becomes best effort and safe: it runs before anything is written, so on error the transaction rolls back, the plain write runs exactly as main does, no entry is emitted, and the plain write's error (if any) answers exactly as main's would. Stage the no-op decision: set a local inside the callback and call commitAudit(false) only after InTx returns nil, so whether an attempt is logged no longer depends on whether the value happened to match. Bound the lock wait with a service-owned 5s context deadline instead of relying on the client deadline, and log the full InTx error before the response detail is selected so a rollback layered on a write failure is not lost from both response and logs. Keep the per-setting lock, the rename, the per-setting targets, raw stored text, the agents_ query constraint, the unknown-key panic, and the write-error detail parity. Measured against an unmodified origin/main control: every member-visible column matches on all seven endpoints. CODAGT-720
Update, same day: the blocking defect is fixedNew head Also in this head:
Measured against an unmodified main build, all seven endpoints: success 204 with a real diff; identical rewrite 204 with no entry; old-read failure 204, written, no entry; write failure 500 with the bare error detail and nothing persisted; and transaction or lock failures 500 with nothing persisted. Concurrency was checked against real PostgreSQL with a barrier: without the per-setting lock, two concurrent writes to one setting both record the same stale previous value; with it, the second blocks, sees the committed value and suppresses correctly. One residual behaviour a reviewer should weigh, listed among the decisions needing human sign-off in the handover document: a lock wait exceeding the bound returns 500, where an unmodified build would have completed the write. That is the price of recording an exact before-and-after, and dropping the lock is a legitimate alternative that several existing audited endpoints in this codebase already take. A review of this head was commissioned but had not reported when this was written, so treat the PR as unreviewed at
|
1 participant
What
Wires the seven deployment-wide operational chat settings endpoints into the enterprise audit log under one grouped
chat_operational_settingsresource type whose diff names exactly the changed setting (CODAGT-720, deployment-stable slice; the settings-singleton recipe ratified by S1 in #27668). Every row also names its setting as the audit resource target, so "who changed retention?" is answered by the row, not just the diff./api/experimental/chats/config/retention-daysagents_chat_retention_dayschat_retention_days/api/experimental/chats/config/debug-retention-daysagents_chat_debug_retention_dayschat_debug_retention_days/api/experimental/chats/config/auto-archive-daysagents_chat_auto_archive_dayschat_auto_archive_days/api/experimental/chats/config/workspace-ttlagents_workspace_ttlworkspace_ttl/api/experimental/chats/config/computer-use-provideragents_computer_use_providercomputer_use_provider/api/experimental/chats/config/debug-loggingagents_chat_debug_logging_allow_usersdebug_logging_allow_users/api/experimental/chats/config/personal-model-overridesagents_chat_personal_model_overrides_enabledpersonal_model_overrides_enabledHow
Each of the seven PUT handlers runs one transaction shaped exactly like main's write. Inside it, a per-setting advisory lock is taken first (its wait bounded by a service-owned 5s context deadline, so a stuck sibling holder surfaces a service 500 rather than an unbounded wait against the client deadline), the handler's own
site_configskey is read raw (Old), the existing upsert runs byte-identical, andOld/Neware populated only when the stored text actually changed. New is the value just written (the upserts store the request value verbatim), so there is no post-write read to fail after the write has already succeeded. A value-identical PUT stages a cancellation and applies it only after the transaction commits. Only the handler's own field is populated on both sides, so a single-setting change diffs exactly one field and never reads or emits unrelated settings. Each setting is described once by achatOperationalSettingvalue pairing its storage key, its human-readable audit target, and its per-setting lock ID, so a key, its label, and its lock cannot drift apart; the target rides on the struct as anActionIgnorefield, naming the row without entering the diff.The Old capture is best effort: it runs before anything is written, so on failure the transaction rolls back and the plain write runs exactly as main does (its error, if any, answers exactly as main's would, with no entry). A genuine write failure is fatal, as on main. The new key-parameterized
GetChatSiteConfigValuequery is restricted to theagents_namespace in SQL and pins its dbauthz contract toResourceDeploymentConfig: update. The per-setting lock derives from the exact key withGenLockID(FNV-1a 64) instead of the sequentialLockID*block, so writers of different settings never contend and the IDs cannot collide with the sequential block or another subsystem'sGenLockIDoutput;TestChatOperationalLockIDsDistinctproves it.Behavior preservation (invariant 2)
One transaction per request shaped like main's; no post-write read, no fallback to a separate direct-write path after a write. A genuine write failure reproduces main's 500 and bare
Detailexactly (the write error is recorded in a local variable and reported directly). Every failure position was measured against an unmodifiedorigin/maincontrol with separate columns:All member-visible columns (status, body Message and Detail, persisted state) match main on all seven endpoints.
Concurrency, run against real PostgreSQL with a barrier forcing both transactions to read before either commits. Without the per-key lock (mutation), an existing row with two concurrent different-valued writes records both from the same stale baseline (2 entries, last-writer-wins persisted); with the lock, the second blocks on the first's lock, reads the first's committed value, and records the correct transition. Two concurrent identical writes on an existing row produce one entry (the second sees the first's committed value and cancels), which the lock-free baseline emits twice. Removing the lock turns the existing-row suppression into a stale-Old duplicate, mutation-proven.
Review findings closed
InTxbegins withcontext.Background()): closed by deleting the post-write read and setting New from the value written.commitAudit(false)until after the transaction commits.Known limits (deliberate, per plan D5 and the ratified write structure)
The audit filter label for the new type is added to the hand-maintained exceptions in
AuditFilter.tsxso it renders "Chat Operational Settings" instead of the capitalized raw value.Tests
WriteAndSuppressiontable across all seven endpoints: Write entry on change with the per-setting target, no-op suppression on a value-identical PUT (canceled viaInitRequestWithCancel), the storage-level boundary (a row-materializing PUT of the effective fallback default audits as empty-to-value; an identical PUT on an existing row emits nothing), and a validation-rejected PUT emits nothing.SingleSettingDiffNamesExactlyOneField: a single-setting change diffs exactly one field even when other operational settings hold stored values (mock-onlyaudit.NewMockWithDiffFn).MalformedSettingRepair: seed junk text into a key, PUT a valid value, assert the 204 and repaired write are unchanged and the committed diff is exactly the junk-to-valid transition (mutation-proven).NonAgentsKeyReadsEmpty: theagents_SQL constraint.TestChatOperationalSettingPanicsOnUnknownKey: the descriptor'ssettingsmethod panics on an unknown key.TestChatOperationalLockIDsDistinct: the per-key lock IDs are pairwise distinct and distinct from every enumerated sequential lock ID.ConcurrentIdenticalPUTsSingleEntry: the per-key lock serializes concurrent identical PUTs to one entry (race clean); removing it turns suppression into a stale-Old duplicate, mutation-proven.enterprise/audit/diff_internal_test.go: single-field diff, malformed-old tracked as raw text, artificial-ID-ignored.dbauthzMethodTestSuite entry for the raw read query;TestAuditableResources;TestAuditDBEnumsCovered; migration suites.Full local gate green:
make fmt,make lint,make -B genzero-diff,TestChatOperationalSettingsAudit(13, incl.-race),TestChatOperationalSettingPanicsOnUnknownKey,TestChatOperationalLockIDsDistinct,enterprise/auditpackage,dbauthz,coderd/database/migrations, frontend lint/format, full./coderdpackage.Exclusions (named per the plan)
general,explore,title-generation,compaction) and the advisor endpoint (PUT .../config/advisor): deferred to S3 (post-M5, decision D3); advisorEnabledis experiment-derived and read-only on main.agents_desktop_enabled: no HTTP handler exists on main, so there is nothing to audit.agents_template_allowlist: being deleted by CODAGT-74's T2; Ethan's track auditstemplates.agents_allowedinstead./chats/configroute block (user-personal-model-overrides,user-debug-logging,user-prompt,user-compaction-thresholds): per-user actions, not admin config.Plan entry
S2 plan entry (CODAGT-66 PLAN.md)
S2
feat: audit operational agent settings changes(CODAGT-720, deployment-stable slice; base: main; starts after S1 recipe ratified)database.AgentsOperationalSettings{ID uuid.UUID; ChatRetentionDays, ChatDebugRetentionDays, ChatAutoArchiveDays, WorkspaceTTL, ComputerUseProvider, DebugLoggingAllowUsers, PersonalModelOverridesEnabled string}: every field a raw-text mirror of itssite_configs.value(D1 rationale; survey table in agents-surface.md section 4 lists keys/queriers/handlers/routes for all seven). One resource_typeagents_operational_settings,AuditActionMapWrite-only, all fields ActionTrack,idActionIgnore.InTx(behavior-preserving: same single write, same response, malformed stored text still gets repaired exactly as today) and populates ONLY its own field on bothOld(previous raw text) andNew(the exact text just written); all other fields stay zero on both sides, so they never diff and no handler gains reads of, or failure modes from, unrelated settings (the adversarial review killed both the all-seven snapshot and typed-getter Old capture). Artificial ID set onNewonly after the tx succeeds and only when the raw text changed; no-op suppression identical to S1. EXCLUDED and named as exclusions in the PR body: the four model-override contexts and the advisor endpoint (S3, post-M5, decision D3),agents_desktop_enabled(no handler exists),agents_template_allowlist(deleted by 74's T2; Ethan's track auditstemplates.agents_allowedinstead), all member-scope endpoints sharing the route block.enterprise/audit/diff_internal_test.goentry.