Files
coder/enterprise/coderd/chatinstructions_audit_test.go
T
Mathias FredrikssonandMichael Suchacz d3f08b1983 feat: audit chat system instructions changes (#27668)
Adds an audit record for administrative events on the deployment-wide
chat instruction settings (system prompt, the include-default toggle,
and the plan-mode instructions), per CODAGT-719 and operator decision
D5. Each endpoint records under a stable identity: resource type
`chat_instruction_settings`, a fixed resource ID and a human-readable
target ("System prompt", "Plan mode instructions"), so two changes to
one setting share an ID and history-by-setting works. A real change
exports a Write entry with the old-to-new text visible; a
value-identical PUT still upserts and still returns 204 but records
nothing.

Attempts are recorded, not only transitions. Identity is assigned before
the authorization check, so a denied PUT exports a 403 row with an empty
diff (no request content reaches it), a validation failure exports a 400
row, and a write failure exports a 500 row, each with an empty diff; an
operator can tell "nothing changed" from "something changed and capture
degraded" by the status code.

The write path stays authoritative. The advisory lock and, on plan-mode,
the transaction exist only to serve change-detection; if any of that
machinery fails (lock, begin, commit, rollback), the handler runs main's
idempotent write path directly and derives the response from it, so a
member-visible failure of audit-only infrastructure can never replace
main's successful response. Accepted consequence: when the lock cannot
be taken, two concurrent identical writes can produce two rows instead
of one. That is audit degradation, which is allowed; changing a member's
response is not. Write failures keep the exact response the endpoint
produced before this wiring (transaction error for the system prompt,
which was always transactional; the raw write error for plan mode, which
was not), and the full transaction error is logged so rollback failures
cannot vanish.

<details>
<summary>CODAGT-66 plan entry: S1 (verbatim)</summary>

**S1 `feat: audit chat system instructions changes`** (CODAGT-719; base:
main)

- Struct: `database.ChatSystemPromptSettings{ID uuid.UUID; SystemPrompt
string; IncludeDefaultSystemPrompt bool; PlanModeInstructions string}`
in `coderd/database/types.go` (ticket-sketched shape; one struct, both
endpoints).
- Registration: union entry (diff.go), table.go entry (`id`
ActionIgnore, other three ActionTrack), `AuditActionMap` Write-only;
four request.go cases (`ResourceTarget` "", `ResourceID` from struct,
`ResourceType` new enum value `chat_system_prompt_settings`,
`ResourceRequiresOrgID` false with the "Artificial ID / deployment
singleton" comment convention).
- Migration: `ALTER TYPE resource_type ADD VALUE IF NOT EXISTS
'chat_system_prompt_settings';` comment-only no-op down (000558 shape);
number picked at push per the numbering constraint.
- codersdk: constant + prose `FriendlyString` ("chat system prompt
settings"); `TestAuditDBEnumsCovered` forces both. `coderd/audit.go`
presentation switches: rely on safe defaults (no link, generic
description); no FE changes (filter label falls back to capitalized
value; acceptable per precedent).
- Wiring `putChatSystemPrompt` and `putChatPlanModeInstructions`:
InitRequest with Action Write; artificial `ID: uuid.New()` on `New` only
when a change is detected; no-op suppression by leaving both aReq sides
unset (nil resource IDs skip the log, request.go skip rule); the write
path itself stays byte-identical (upserts still run unconditionally).
- `putChatSystemPrompt` (writes two keys conditionally in one existing
tx): inside that tx, read the pair via `GetChatSystemPromptConfig` for
`Old`, perform the conditional writes exactly as today, then RE-READ the
pair for `New`. The re-read is load-bearing:
`include_default_system_prompt` is computed from the toggle row AND the
prompt, so a prompt-only write can flip the effective value without the
request carrying the pointer. `PlanModeInstructions` stays zero on both
sides.
- `putChatPlanModeInstructions` (no tx exists today): wrap its
read-upsert in `InTx` (behavior-preserving: same single write);
`Old`/`New` populate only `PlanModeInstructions`; the two system-prompt
fields stay zero on both sides; no cross-key reads.
- Change detection compares the populated payload fields only (never the
artificial ID).
- Tests: handler-level coderdtest with `audit.NewMock()` asserting Write
entry on change and NO entry on a value-identical PUT, for both
endpoints (this also exercises `ResourceRequiresOrgID` end to end); the
fallback-flip case (no explicit include-default row, nonempty prompt set
to empty, effective boolean flips: entry emitted with the boolean diff);
diff assertions (old->new prompt text tracked, not secret) in
`enterprise/audit/diff_internal_test.go`; `TestAuditableResources`
passes by construction.
- Bookkeeping at PR open: correct CODAGT-719's no-op premise ("matches
the existing 204-on-unchanged behavior" does not exist on main;
suppression is new, write path unchanged).
- Review focus: Old capture and the New re-read inside the tx (three of
four existing singletons never set Old; do not copy them; and the
computed include-default value makes a naive New construction wrong);
the skip-on-no-op mechanism; prompt text deliberately visible in diffs.

</details>

Note: the plan excerpt above predates operator decision D5 (2026-07-30),
which this PR implements: the resource type is
`chat_instruction_settings` (not `chat_system_prompt_settings`), each
setting carries a stable ID and a display-name target (not a per-write
artificial ID and an empty target), no-op suppression runs through
`InitRequestWithCancel` (not the nil-ID skip), and attempts (denied,
failed, capture-degraded) record rows with real statuses and empty
diffs. Ticket bookkeeping for CODAGT-719 was corrected on Linear at
kickoff: the ticket's "matches the existing 204-on-unchanged behavior"
premise does not exist on main; suppression is new, and the write path
is unchanged.

> 🤖 This PR was created with the help of Coder Agents, and _will be_
reviewed by a human. 🏂🏻

---------

Co-authored-by: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com>
2026-08-18 20:03:06 +02:00

194 lines
6.7 KiB
Go

package coderd_test
import (
"context"
"encoding/json"
stderrors "errors"
"sync"
"sync/atomic"
"testing"
"github.com/stretchr/testify/require"
"cdr.dev/slog/v3/sloggers/slogtest"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/util/ptr"
"github.com/coder/coder/v2/codersdk"
entaudit "github.com/coder/coder/v2/enterprise/audit"
"github.com/coder/coder/v2/testutil"
)
// captureBackend is an enterprise audit backend that records exported logs
// so tests can assert on the production differ's output.
type captureBackend struct {
mu sync.Mutex
logs []database.AuditLog
}
func (*captureBackend) Decision() entaudit.FilterDecision {
return entaudit.FilterDecisionStore | entaudit.FilterDecisionExport
}
func (b *captureBackend) Export(_ context.Context, alog database.AuditLog, _ entaudit.BackendDetails) error {
b.mu.Lock()
defer b.mu.Unlock()
b.logs = append(b.logs, alog)
return nil
}
func (b *captureBackend) reset() {
b.mu.Lock()
defer b.mu.Unlock()
b.logs = nil
}
func (b *captureBackend) entries() []database.AuditLog {
b.mu.Lock()
defer b.mu.Unlock()
out := make([]database.AuditLog, len(b.logs))
copy(out, b.logs)
return out
}
// failNextGetChatSystemPromptConfigStore fails the Old capture read of the
// chat system prompt configuration once. Failure state is shared across InTx
// wrappers.
type failNextGetChatSystemPromptConfigStore struct {
database.Store
fail *atomic.Bool
}
func newFailNextGetChatSystemPromptConfigStore(store database.Store) *failNextGetChatSystemPromptConfigStore {
return &failNextGetChatSystemPromptConfigStore{
Store: store,
fail: &atomic.Bool{},
}
}
func (s *failNextGetChatSystemPromptConfigStore) InTx(function func(database.Store) error, txOpts *database.TxOptions) error {
return s.Store.InTx(func(tx database.Store) error {
return function(&failNextGetChatSystemPromptConfigStore{
Store: tx,
fail: s.fail,
})
}, txOpts)
}
func (s *failNextGetChatSystemPromptConfigStore) GetChatSystemPromptConfig(ctx context.Context) (database.GetChatSystemPromptConfigRow, error) {
if s.fail.CompareAndSwap(true, false) {
return database.GetChatSystemPromptConfigRow{}, stderrors.New("forced old capture failure")
}
return s.Store.GetChatSystemPromptConfig(ctx)
}
// TestChatInstructionSettingsOldCaptureBestEffort proves the Old capture is
// best effort: a failed Old read costs nothing (nothing was written yet), so
// the handler runs the direct write path and emits no audit entry. There is
// no post-write read to fail, so no diff can ever be fabricated from a zero
// value.
func TestChatInstructionSettingsOldCaptureBestEffort(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
rawDB, pubsub := dbtestutil.NewDB(t)
store := newFailNextGetChatSystemPromptConfigStore(rawDB)
backend := &captureBackend{}
auditor := entaudit.NewAuditor(rawDB, entaudit.DefaultFilter, backend)
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
rawClient, _, _ := coderdtest.NewWithAPI(t, &coderdtest.Options{
Database: store,
Pubsub: pubsub,
DeploymentValues: coderdtest.DeploymentValues(t),
Auditor: auditor,
Logger: &logger,
})
client := codersdk.NewExperimentalClient(rawClient)
_ = coderdtest.CreateFirstUser(t, client.Client)
// Discard the login entry emitted by user creation.
backend.reset()
// The Old capture fails before anything is written: the transaction
// is abandoned at no cost, the handler runs the direct write path,
// and no audit entry is emitted (there is no baseline to diff
// against, so any row would be a guess).
store.fail.Store(true)
err := client.UpdateChatSystemPrompt(ctx, codersdk.UpdateChatSystemPromptRequest{
SystemPrompt: "Changed prompt.",
IncludeDefaultSystemPrompt: ptr.Ref(true),
})
require.NoError(t, err)
require.Empty(t, backend.entries())
resp, err := client.GetChatSystemPrompt(ctx)
require.NoError(t, err)
require.Equal(t, "Changed prompt.", resp.SystemPrompt)
require.True(t, resp.IncludeDefaultSystemPrompt)
}
// TestChatInstructionSettingsIncludeDefaultPresence exercises the production
// differ against the include-default presence transition: writing an explicit
// false over a legacy absent override row does not move the effective value
// but inserts the persistent row, which changes future behavior, so the
// presence transition must be audited.
func TestChatInstructionSettingsIncludeDefaultPresence(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
rawDB, pubsub := dbtestutil.NewDB(t)
backend := &captureBackend{}
auditor := entaudit.NewAuditor(rawDB, entaudit.DefaultFilter, backend)
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
rawClient, _, _ := coderdtest.NewWithAPI(t, &coderdtest.Options{
Database: rawDB,
Pubsub: pubsub,
DeploymentValues: coderdtest.DeploymentValues(t),
Auditor: auditor,
Logger: &logger,
})
client := codersdk.NewExperimentalClient(rawClient)
_ = coderdtest.CreateFirstUser(t, client.Client)
// Discard the login entry emitted by user creation.
backend.reset()
// Legacy shape: a non-empty prompt with no override row computes
// effective false.
err := client.UpdateChatSystemPrompt(ctx, codersdk.UpdateChatSystemPromptRequest{
SystemPrompt: "Legacy custom instructions.",
})
require.NoError(t, err)
require.Len(t, backend.entries(), 1)
// Explicit false over the absent row: same effective value, but the
// override row now exists, so the entry must record the presence
// transition.
backend.reset()
err = client.UpdateChatSystemPrompt(ctx, codersdk.UpdateChatSystemPromptRequest{
SystemPrompt: "Legacy custom instructions.",
IncludeDefaultSystemPrompt: ptr.Ref(false),
})
require.NoError(t, err)
require.Len(t, backend.entries(), 1)
var diff map[string]codersdk.AuditDiffField
require.NoError(t, json.Unmarshal(backend.entries()[0].Diff, &diff))
require.Equal(t, codersdk.AuditDiffField{Old: false, New: true},
diff["include_default_system_prompt_set"])
// The effective value did not move, and the prompt is unchanged.
require.NotContains(t, diff, "include_default_system_prompt")
require.NotContains(t, diff, "system_prompt")
// Repeating the same explicit-false PUT is a true no-op now: the row
// exists with the same value, so nothing is audited.
backend.reset()
err = client.UpdateChatSystemPrompt(ctx, codersdk.UpdateChatSystemPromptRequest{
SystemPrompt: "Legacy custom instructions.",
IncludeDefaultSystemPrompt: ptr.Ref(false),
})
require.NoError(t, err)
require.Empty(t, backend.entries())
}