From 9481b8e962c125a77cedbefdd0cee8bb39bc5780 Mon Sep 17 00:00:00 2001 From: BaaRai <130491490+BaaRai@users.noreply.github.com> Date: Mon, 9 Nov 2026 12:22:46 +0100 Subject: [PATCH] fix(intercept): judge decisions were stored as unknown (prefix case mismatch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since e233c02 the judge writer prefixes its reason with "[model]" while interceptSource, the approvalDecisionSource SQL fallback and the archive restore checked "[Model]", so model-decided rows were inserted with decision_source='unknown' and missed by the decision_source=model filter. - db: add ModelReasonPrefix ("[model]") and isModelReason, which also accepts the pre-e233c02 "[模型]" prefix; readers compare case-insensitively (lower(reason) LIKE in SQL). - intercept: the writer builds its reason through JudgeReason, using the shared constant. - schema.sql: idempotent backfill of 'unknown' rows without a rule whose reason carries either prefix. - tests: un-skip TestInterceptFilterHTTP (it failed only because of this), make the db filter fixture use the constant, and add a Postgres-free test pinning the writer output to interceptSource. Fixes #156 --- .github/workflows/ci.yml | 2 +- db/export_test.go | 4 ++++ db/intercept.go | 22 +++++++++++++++++--- db/intercept_filter_test.go | 2 +- db/intercept_source_test.go | 37 +++++++++++++++++++++++++++++++++ db/schema.sql | 8 ++++++- db/task_archives_restore.go | 2 +- intercept/intercept.go | 19 ++++++++++++----- server/intercept_filter_test.go | 1 - 9 files changed, 84 insertions(+), 13 deletions(-) create mode 100644 db/export_test.go create mode 100644 db/intercept_source_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7c581c5..b9dac1f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ jobs: - run: go build ./... - run: go vet ./... # Skipped tests, tracked in issue #9: - # - TestInterceptFilterHTTP, TestGraphOverviewExpandsAssociatedCompanyScope, + # - TestGraphOverviewExpandsAssociatedCompanyScope, # TestCaptureApprovalLifecycle: fail on unmodified upstream code with a # fresh database. # - TestSideUsageRecordedOnceOnConsumerCancellation: concurrency/cancellation diff --git a/db/export_test.go b/db/export_test.go new file mode 100644 index 0000000..ac34d04 --- /dev/null +++ b/db/export_test.go @@ -0,0 +1,4 @@ +package db + +// InterceptSource exposes interceptSource to external (db_test) tests. +var InterceptSource = interceptSource diff --git a/db/intercept.go b/db/intercept.go index aae0b36..bfeade7 100644 --- a/db/intercept.go +++ b/db/intercept.go @@ -37,7 +37,7 @@ type InterceptPending struct { ToolInput json.RawMessage `json:"tool_input"` Status string `json:"status"` DecisionSource string `json:"decision_source"` - Reason string `json:"reason"` // rule message or model decision reason (prefixed with [Model]) + Reason string `json:"reason"` // rule message or model decision reason (prefixed with ModelReasonPrefix) DecidedAt *time.Time `json:"decided_at"` CreatedAt time.Time `json:"created_at"` } @@ -225,10 +225,26 @@ func scanInterceptApprovalRow(rows interface{ Scan(...any) error }, r *Intercept ) } +// ModelReasonPrefix starts every reason written by the LLM fallback judge +// (intercept.JudgeReason); readers derive decision_source='model' from it. +const ModelReasonPrefix = "[model]" + +// legacyModelReasonPrefix is what the judge wrote before the UI translation (e233c02). +const legacyModelReasonPrefix = "[模型]" + +// isModelReason reports whether reason was written by the model judge. The +// comparison ignores case: readers used to expect "[Model]" while the writer +// produced "[model]", so stored rows may carry either spelling. +func isModelReason(reason string) bool { + return strings.HasPrefix(strings.ToLower(reason), ModelReasonPrefix) || + strings.HasPrefix(reason, legacyModelReasonPrefix) +} + // Keep legacy rows without decision_source consistent with their displayed source. +// Mirrors isModelReason; PostgreSQL LIKE is case-sensitive and '[' is literal. const approvalDecisionSource = `COALESCE(NULLIF(ip.decision_source,''), CASE WHEN ip.rule_id IS NOT NULL THEN 'rule' - WHEN ip.reason LIKE '[Model]%' THEN 'model' ELSE 'unknown' END)` + WHEN lower(ip.reason) LIKE '` + ModelReasonPrefix + `%' OR ip.reason LIKE '` + legacyModelReasonPrefix + `%' THEN 'model' ELSE 'unknown' END)` const approvalRowColumns = `ip.id, ip.rule_id, ip.conversation_id, ip.task_id, ip.agent_name, ip.tool_name, ip.tool_input, ip.status, ip.reason, ip.decided_at, ip.created_at, ` + approvalDecisionSource + `, @@ -248,7 +264,7 @@ func interceptSource(ruleID int64, reason string) string { if ruleID != 0 { return "rule" } - if strings.HasPrefix(reason, "[Model]") { + if isModelReason(reason) { return "model" } return "unknown" diff --git a/db/intercept_filter_test.go b/db/intercept_filter_test.go index 90b8d0a..986ab63 100644 --- a/db/intercept_filter_test.go +++ b/db/intercept_filter_test.go @@ -38,7 +38,7 @@ func TestInterceptApprovalFilters(t *testing.T) { ruleID = rule.ID } if source == "model" { - reason = "[Model] fixture" + reason = ModelReasonPrefix + " fixture" } id, err := d.CreateDecidedIntercept(ruleID, 0, task, "test", "Bash", []byte(`{"command":"fixture"}`), status, reason) if err != nil { diff --git a/db/intercept_source_test.go b/db/intercept_source_test.go new file mode 100644 index 0000000..e442dfe --- /dev/null +++ b/db/intercept_source_test.go @@ -0,0 +1,37 @@ +package db_test + +import ( + "testing" + + "github.com/Autumn-27/artex/db" + "github.com/Autumn-27/artex/intercept" +) + +// The judge writer (package intercept) and the decision_source readers (package +// db) must agree on the reason prefix; this pins them together without Postgres. +func TestInterceptSourceMatchesJudgeWriter(t *testing.T) { + for _, tc := range []struct{ action, message string }{ + {"allow", ""}, + {"deny", "destructive command outside the workspace"}, + {"ask", db.ModelReasonPrefix + " already prefixed"}, + } { + reason := intercept.JudgeReason(tc.action, tc.message) + if got := db.InterceptSource(0, reason); got != "model" { + t.Fatalf("JudgeReason(%q, %q) = %q classified as %q, want model", tc.action, tc.message, reason, got) + } + } +} + +func TestInterceptSourceLegacyModelPrefix(t *testing.T) { + for _, reason := range []string{"[模型] 允许", "[Model] fixture"} { + if got := db.InterceptSource(0, reason); got != "model" { + t.Fatalf("%q classified as %q, want model", reason, got) + } + } + if got := db.InterceptSource(0, "legacy reason"); got != "unknown" { + t.Fatalf("plain reason classified as %q, want unknown", got) + } + if got := db.InterceptSource(7, "[model] rule wins"); got != "rule" { + t.Fatalf("rule row classified as %q, want rule", got) + } +} diff --git a/db/schema.sql b/db/schema.sql index 4d73a59..f6ce1fc 100644 --- a/db/schema.sql +++ b/db/schema.sql @@ -1088,7 +1088,7 @@ CREATE TABLE IF NOT EXISTS intercept_pending ( tool_input JSONB NOT NULL DEFAULT '{}', status TEXT NOT NULL DEFAULT 'pending' CHECK (status IN ('pending', 'allowed', 'denied', 'timeout')), - -- Decision reason: the rule message when a rule hits; the model's short reason (with the [Model] prefix) for an LLM fallback decision. + -- Decision reason: the rule message when a rule hits; the model's short reason (with the [model] prefix) for an LLM fallback decision. reason TEXT NOT NULL DEFAULT '', decided_at TIMESTAMPTZ, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() @@ -1102,6 +1102,12 @@ ALTER TABLE intercept_pending ADD COLUMN IF NOT EXISTS audit JSONB; ALTER TABLE intercept_pending ADD COLUMN IF NOT EXISTS decision_source TEXT NOT NULL DEFAULT ''; UPDATE intercept_pending SET decision_source=CASE WHEN rule_id IS NOT NULL THEN 'rule' WHEN reason LIKE '[Model]%' THEN 'model' ELSE 'unknown' END WHERE decision_source=''; +-- Judge decisions were stored as 'unknown' because the readers expected a capitalized +-- prefix while the judge wrote '[model]' (and '[模型]' before e233c02). Idempotent: +-- once corrected no row matches the WHERE clause again. +UPDATE intercept_pending SET decision_source='model' + WHERE decision_source='unknown' AND rule_id IS NULL + AND (reason LIKE '[model]%' OR reason LIKE '[模型]%'); -- ===================================================================== -- L. Finding persistence diff --git a/db/task_archives_restore.go b/db/task_archives_restore.go index 2443833..44c4a72 100644 --- a/db/task_archives_restore.go +++ b/db/task_archives_restore.go @@ -677,7 +677,7 @@ func restoreInterceptRows(tx *sql.Tx, raw json.RawMessage) error { source = "unknown" if row["rule_id"] != nil { source = "rule" - } else if reason, _ := row["reason"].(string); strings.HasPrefix(reason, "[Model]") { + } else if reason, _ := row["reason"].(string); isModelReason(reason) { source = "model" } row["decision_source"] = source diff --git a/intercept/intercept.go b/intercept/intercept.go index 7340818..d503322 100644 --- a/intercept/intercept.go +++ b/intercept/intercept.go @@ -476,11 +476,7 @@ func (i *Interceptor) Judge(ctx context.Context, tool string, arguments json.Raw out.ProfileID = cfg.ProfileID configJSON, _ := json.Marshal(cfg) out.ConfigDigest = digestInput(configJSON) - if out.Message == "" { - out.Message = "[model] " + judgeActionLabel(out.Action) - } else if !strings.HasPrefix(out.Message, "[model]") { - out.Message = "[model] " + out.Message - } + out.Message = JudgeReason(out.Action, out.Message) if out.Action == "ask" { out.TimeoutEnabled = true out.TimeoutSeconds = cfg.AskTimeoutSeconds @@ -489,6 +485,19 @@ func (i *Interceptor) Judge(ctx context.Context, tool string, arguments json.Raw return out, true } +// JudgeReason is the reason stored for a judge verdict: the model's message, or +// a label for the action when it gave none, carrying db.ModelReasonPrefix so +// the db readers record decision_source='model'. +func JudgeReason(action, message string) string { + if message == "" { + return db.ModelReasonPrefix + " " + judgeActionLabel(action) + } + if !strings.HasPrefix(message, db.ModelReasonPrefix) { + return db.ModelReasonPrefix + " " + message + } + return message +} + func judgeActionLabel(action string) string { switch action { case "allow": diff --git a/server/intercept_filter_test.go b/server/intercept_filter_test.go index b948335..432e23f 100644 --- a/server/intercept_filter_test.go +++ b/server/intercept_filter_test.go @@ -11,7 +11,6 @@ import ( ) func TestInterceptFilterHTTP(t *testing.T) { - t.Skip("known failure with a fresh database, pre-existing on unmodified upstream; tracked in BaaRai/ARTEX#9") dsn, _, err := db.DSN() if err != nil { t.Skip("no test database configured")