Files
multica/server/internal/util/text_postgres_test.go
T
d6301091e0 MUL-6310 fix(agent tasks): stop NUL bytes wedging tasks on complete/messages/cancel (#7124)
* fix(agent tasks): stop NUL bytes wedging tasks on complete/messages/cancel

GH #7098 reported that a NUL in a failure diagnostic 500s POST
/api/daemon/tasks/{id}/fail and leaves the task stuck in 'running'.
PR #7115 fixes that one endpoint. Verifying the report turned up the
same defect on three more persistence paths, including the one users
are most likely to hit.

/complete is the important one. The whole request is marshalled into
agent_task_queue.result, a JSONB column, and encoding/json renders an
embedded NUL as a unicode escape that PostgreSQL rejects with SQLSTATE
22P05 - a different error from the TEXT case, same user-visible wedge.
Agent stdout carries a stray NUL far more readily than a curated error
string does: a tool that cats a binary, a Windows host emitting UTF-16,
a child process dumping memory as it crashes.

Nothing reaps the wedged row. Both sweepers (FailStaleTasks,
FailTasksForOfflineRuntimes) require the runtime to be offline, so a
healthy heartbeating daemon leaves the task in 'running' indefinitely,
holding one of the agent's max_concurrent_tasks slots forever.

Changes:

- util.SanitizeTextForPostgres - one shared definition of "safe to
  persist": remove NUL, replace other invalid UTF-8 with U+FFFD.
  handler.sanitizeNullBytes now delegates to it, so the task endpoints
  and the comment/skill/property endpoints cannot drift apart. This
  does change behaviour at those existing call sites: invalid UTF-8
  becomes U+FFFD instead of being dropped silently.
- util.SanitizeJSONForPostgres - depth-bounded walk over a decoded JSON
  tree, keys included. task_messages.input is JSONB, so a NUL nested in
  a tool's arguments breaks the insert even when every top-level string
  is clean.
- /complete: scrub right after decode, before the context-exhaustion
  re-route, so the failure classifier sees the text we will persist.
- /messages: scrub the strings and walk input. This endpoint has no
  daemon-side retry, so an unguarded batch is dropped silently.
- /cancel-ack and CancelTaskWithResult: same TEXT columns, same hazard.

Note for reviewers: strings.ToValidUTF8 alone does NOT solve this. NUL
is a valid UTF-8 encoding of U+0000 and passes straight through, which
is why the explicit ReplaceAll is load-bearing and has a test pinning
it.

Tests: unit coverage for both helpers, plus PostgreSQL-backed
regressions for the /complete JSONB payload and a NUL nested two levels
into task message input. Both include a control leg asserting the
database really does reject the unsanitized payload, so deleting the
sanitizer fails the test rather than silently passing.

Refs GH #7098, MUL-6310. Follow-up to #7115.

Co-authored-by: multica-agent <[email protected]>

* fix(agent tasks): sanitize messages.type; drive NUL tests through the handlers

Addresses both must-fix items from review.

1. TaskMessageRequest.Type was still written to a TEXT column unsanitized,
   so a NUL in `type` alone kept the whole batch 500ing and being dropped.
   Sanitized alongside tool/content/output, and covered by the entry test.

2. The two regression tests called the sanitizers by hand and then hit the
   query directly. That proved the helpers and the database behaviour but
   not the wiring: deleting the sanitize calls from CompleteTask /
   ReportTaskMessages left both tests green, which is the one regression
   they existed to catch.

   The positive branches now POST through testHandler.CompleteTask and
   testHandler.ReportTaskMessages via newDaemonTokenRequest, asserting HTTP
   200 plus the persisted row. The NUL travels the way the report describes
   -- escaped on the wire by encoding/json, rebuilt by the decoder -- so the
   handler sees exactly what a daemon would send.

   Verified by sabotage: commenting out the sanitize calls fails both tests
   with the reported errors (22P05 on /complete, "failed to persist task
   message" on /messages). Restored before commit.

   The direct-to-database rejection legs are kept and consolidated into
   TestPostgresRejectsUnsanitizedTaskPayloads, now also covering the TEXT
   `type` column, so the suite still proves PostgreSQL genuinely refuses
   these payloads rather than the fix being a no-op.

Seeding note: the fixture task now gets an issue, because
requireDaemonTaskAccess resolves the workspace through it and a task with no
issue / chat / autopilot link 404s before the handler reads the body.

Refs GH #7098, MUL-6310.

Co-authored-by: multica-agent <[email protected]>

* refactor(agent tasks): collapse normalizeTaskFailureDiagnostic into the shared helper

#7115 landed its own sanitizer at the FailTask service boundary while this
branch added util.SanitizeTextForPostgres for the remaining paths. The two
had identical semantics, which is exactly the drift this PR set out to
prevent: one definition of "safe to persist", not one per entry point.

FailTask now calls util.SanitizeTextForPostgres directly. The service-boundary
guard itself stays — failClaimedTaskBeforeLaunch reaches FailTask without
going through request decoding, so the handler-level scrub does not cover it.

Drops TestNormalizeTaskFailureDiagnostic, whose cases are a subset of
TestSanitizeTextForPostgres in internal/util. TestFailTaskSanitizesNULDiagnostic
is kept untouched: it drives the real FailTask against PostgreSQL and asserts
the task reaches 'failed' with the diagnostic intact, so it still guards the
wiring — and it passing against the shared helper is what demonstrates this
collapse preserved behaviour.

Refs GH #7098, MUL-6310.

Co-authored-by: multica-agent <[email protected]>

---------

Co-authored-by: J <[email protected]>
Co-authored-by: multica-agent <[email protected]>
2026-08-18 14:17:14 +08:00

156 lines
4.4 KiB
Go

package util
import (
"encoding/json"
"strings"
"testing"
)
func TestSanitizeTextForPostgres(t *testing.T) {
tests := []struct {
name string
in string
want string
}{
{
name: "ordinary text is returned unchanged",
in: "worker failed: connection refused",
want: "worker failed: connection refused",
},
{
name: "multi-byte text is returned unchanged",
in: "任务失败:诊断详情",
want: "任务失败:诊断详情",
},
{
name: "embedded NUL is removed and surrounding text kept",
in: "worker failed\x00 diagnostic details",
want: "worker failed diagnostic details",
},
{
name: "UTF-16-shaped run of NULs is removed",
in: "w\x00o\x00r\x00k\x00e\x00r\x00",
want: "worker",
},
{
name: "a string of only NULs collapses to empty",
in: "\x00\x00\x00",
want: "",
},
{
name: "invalid UTF-8 becomes U+FFFD rather than vanishing",
in: string([]byte{'b', 'a', 'd', 0xff, 'b', 'y', 't', 'e'}),
want: "bad\uFFFDbyte",
},
{
name: "NUL removal and UTF-8 repair compose",
in: string([]byte{'a', 0x00, 'b', 0xff, 'c'}),
want: "ab\uFFFDc",
},
{
name: "empty stays empty",
in: "",
want: "",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := SanitizeTextForPostgres(tt.in); got != tt.want {
t.Fatalf("SanitizeTextForPostgres(%q) = %q, want %q", tt.in, got, tt.want)
}
})
}
}
// TestSanitizeTextForPostgresToValidUTF8IsNotEnough pins the single fact this
// whole guard rests on: NUL is VALID UTF-8, so the strings.ToValidUTF8 call on
// its own lets it straight through to the database. If someone ever
// "simplifies" SanitizeTextForPostgres down to just ToValidUTF8, this test is
// what tells them they have reopened GH #7098.
func TestSanitizeTextForPostgresToValidUTF8IsNotEnough(t *testing.T) {
poisoned := "worker failed\x00 diagnostic"
if !strings.ContainsRune(strings.ToValidUTF8(poisoned, "\uFFFD"), 0) {
t.Fatal("premise broken: strings.ToValidUTF8 now strips NUL on its own")
}
if strings.ContainsRune(SanitizeTextForPostgres(poisoned), 0) {
t.Fatal("SanitizeTextForPostgres left a NUL behind")
}
}
func TestSanitizeJSONForPostgres(t *testing.T) {
t.Run("cleans strings at every depth, keys included", func(t *testing.T) {
in := map[string]any{
"cmd\x00": "cat\x00 binary",
"args": []any{"-n\x00", "1"},
"nested": map[string]any{
"deep": []any{map[string]any{"k": "v\x00"}},
},
"count": float64(3),
"ok": true,
"null": nil,
}
out, ok := SanitizeJSONForPostgres(in).(map[string]any)
if !ok {
t.Fatalf("SanitizeJSONForPostgres returned %T, want map[string]any", SanitizeJSONForPostgres(in))
}
encoded, err := json.Marshal(out)
if err != nil {
t.Fatalf("marshal sanitized value: %v", err)
}
// The marshalled form is what actually reaches the JSONB column, so
// assert on that: encoding/json renders a surviving NUL as an escape,
// which is precisely what PostgreSQL rejects with SQLSTATE 22P05.
if strings.Contains(string(encoded), "\\u0000") {
t.Fatalf("sanitized JSON still carries a NUL escape: %s", encoded)
}
if _, exists := out["cmd"]; !exists {
t.Fatalf("poisoned key was not cleaned: %v", out)
}
if got := out["cmd"]; got != "cat binary" {
t.Fatalf("out[cmd] = %v, want %q", got, "cat binary")
}
if got := out["count"]; got != float64(3) {
t.Fatalf("non-string value was mutated: %v", got)
}
if got := out["ok"]; got != true {
t.Fatalf("bool value was mutated: %v", got)
}
if got, exists := out["null"]; !exists || got != nil {
t.Fatalf("null value was mutated: %v", got)
}
})
t.Run("stops at the depth limit instead of recursing forever", func(t *testing.T) {
// Build a chain deeper than the cap. The point is that this returns at
// all rather than blowing the stack on a hostile payload.
var deep any = "leaf\x00"
for i := 0; i < sanitizeJSONMaxDepth+10; i++ {
deep = map[string]any{"next": deep}
}
out := SanitizeJSONForPostgres(deep)
encoded, err := json.Marshal(out)
if err != nil {
t.Fatalf("marshal deep value: %v", err)
}
if strings.Contains(string(encoded), "\\u0000") {
t.Fatalf("deep payload still carries a NUL escape: %s", encoded)
}
})
t.Run("scalars pass through", func(t *testing.T) {
if got := SanitizeJSONForPostgres("plain"); got != "plain" {
t.Fatalf("got %v, want plain", got)
}
if got := SanitizeJSONForPostgres(nil); got != nil {
t.Fatalf("got %v, want nil", got)
}
})
}