diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_remediation.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_remediation.go new file mode 100644 index 0000000000..315d29fd6e --- /dev/null +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_remediation.go @@ -0,0 +1,93 @@ +package acp + +import ( + "net/url" + "strings" +) + +const ( + // maxOpenCodeActionURLBytes bounds the complete validated remediation URL. + // The allowlisted route plus a bounded workspace identifier leaves ample + // headroom while keeping the field a bounded diagnostic. + maxOpenCodeActionURLBytes = 256 + openCodeActionURLPrefix = "/workspace/" + openCodeActionURLSuffix = "/go" + openCodeActionURLHost = "opencode.ai" +) + +// NormalizeOpenCodeActionURL is the single allowlist validator shared by the +// OpenCode stderr parser and the structured ACP error projection. It accepts +// only `https://opencode.ai/workspace//go` with an exact +// host, HTTPS scheme, no userinfo, query, fragment (including empty +// delimiters), percent-encoding, or port, a bounded workspace identifier, and +// a bounded complete URL. Any other input — including arbitrary provider +// URLs — returns "". +func NormalizeOpenCodeActionURL(raw string) string { + if raw == "" || len(raw) > maxOpenCodeActionURLBytes { + return "" + } + u, err := url.Parse(raw) + if err != nil { + return "" + } + if !hasAllowlistedOpenCodeAuthority(raw, u) || !hasAllowlistedOpenCodePath(u) { + return "" + } + workspaceID := strings.TrimSuffix(strings.TrimPrefix(u.Path, openCodeActionURLPrefix), openCodeActionURLSuffix) + if !safeOpenCodeWorkspaceID(workspaceID) { + return "" + } + return "https://" + openCodeActionURLHost + openCodeActionURLPrefix + workspaceID + openCodeActionURLSuffix +} + +// hasAllowlistedOpenCodeAuthority accepts only the exact HTTPS route with no +// userinfo, opaque data, query (including a trailing "?" via ForceQuery), +// fragment, percent-encoding, or port. A trailing "#" is not distinguishable +// in the parsed struct, so the raw input is checked too. +func hasAllowlistedOpenCodeAuthority(raw string, u *url.URL) bool { + if u.Scheme != "https" || u.Host != openCodeActionURLHost { + return false + } + return u.User == nil && u.Opaque == "" && !u.ForceQuery && + u.RawQuery == "" && u.Fragment == "" && u.RawPath == "" && + !strings.ContainsRune(raw, '#') +} + +func hasAllowlistedOpenCodePath(u *url.URL) bool { + return strings.HasPrefix(u.Path, openCodeActionURLPrefix) && + strings.HasSuffix(u.Path, openCodeActionURLSuffix) +} + +// safeOpenCodeWorkspaceID bounds the workspace identifier to the same +// alphanumeric/underscore/hyphen charset OpenCode uses for its identifiers. +// The dot-colon-slash tolerance of safeOpenCodeField is intentionally not +// reused here so a `..`-style segment cannot pass as a workspace ID. +func safeOpenCodeWorkspaceID(id string) bool { + if id == "" || len(id) > 128 { + return false + } + for _, r := range id { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || + (r >= '0' && r <= '9') || r == '_' || r == '-' { + continue + } + return false + } + return true +} + +// extractOpenCodeActionURL finds the first allowlisted OpenCode action URL in +// the raw provider message, before sanitization strips URLs and identifiers. +// Strictness is intentional: a URL followed by punctuation or prose does not +// validate, and the sanitizer still removes the token from the message. +func extractOpenCodeActionURL(raw string) string { + for _, token := range strings.Fields(raw) { + if !strings.HasPrefix(token, "https://") { + continue + } + if normalized := NormalizeOpenCodeActionURL(token); normalized != "" { + return normalized + } + } + return "" +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_stderr.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_stderr.go index 0cbe061916..7e722c2fb2 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_stderr.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_stderr.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/coder/acp-go-sdk" "github.com/kandev/kandev/internal/agentctl/types/streams" "go.uber.org/zap" ) @@ -40,13 +41,52 @@ func (e *providerPromptError) Error() string { // ProviderErrorFromError extracts the safe provider diagnostic from a prompt // error without exposing the provider-specific wrapper to lifecycle callers. +// It first unwraps the correlated stderr diagnostic; for a structured ACP +// service-failure it reads only the explicit `action_url` field and the safe +// message — never the raw error string. func ProviderErrorFromError(err error) *streams.ProviderError { var providerErr *providerPromptError - if !errors.As(err, &providerErr) || providerErr == nil || !providerErr.ProviderError.Valid() { + if errors.As(err, &providerErr) && providerErr != nil && providerErr.ProviderError.Valid() { + copy := providerErr.ProviderError + return © + } + return providerErrorFromACPActionURL(err) +} + +// providerErrorFromACPActionURL projects a future ACP service-failure +// response that carries a structured `action_url` into a provider diagnostic. +// The URL must pass the shared allowlist; the message runs through the same +// sanitizer as stderr so it stays URL-free and identifier-redacted. +// +// The contract is intentionally URL-gated: a message-only ACP error (no +// valid `action_url`) falls through to the generic error path rather than +// surfacing an unsanitized ACP message, matching the stderr diagnostic's +// bounded-message rule. OpenCode's current ACP failures therefore keep the +// existing short-error fallback. +func providerErrorFromACPActionURL(err error) *streams.ProviderError { + var reqErr *acp.RequestError + if !errors.As(err, &reqErr) || reqErr == nil { return nil } - copy := providerErr.ProviderError - return © + data, ok := reqErr.Data.(map[string]any) + if !ok { + return nil + } + rawURL, _ := data["action_url"].(string) + remediationURL := NormalizeOpenCodeActionURL(rawURL) + if remediationURL == "" { + return nil + } + message := sanitizeOpenCodeMessage(reqErr.Message) + if message == "" { + return nil + } + return &streams.ProviderError{ + Source: streams.ProviderErrorSourceOpenCodeACP, + Message: message, + RemediationURL: remediationURL, + OccurredAt: time.Now(), + } } // ConsumeStderrLine implements adapter.StderrLineConsumer. OpenCode's @@ -98,6 +138,14 @@ func parseOpenCodeStderrLine(line string) (openCodeStderrDiagnostic, bool) { if !safeOpenCodeIdentifier(sessionID, "ses_") { return openCodeStderrDiagnostic{}, false } + // Capture the allowlisted remediation URL before sanitization. OpenCode may + // carry it in a dedicated `action_url` field or inline in the provider + // error text; either way only the shared validator accepts it, and the + // sanitized message never contains the URL or workspace identifier. + remediationURL := NormalizeOpenCodeActionURL(fields["action_url"]) + if remediationURL == "" { + remediationURL = extractOpenCodeActionURL(fields["error.error"]) + } message := sanitizeOpenCodeMessage(fields["error.error"]) if message == "" { return openCodeStderrDiagnostic{}, false @@ -108,11 +156,12 @@ func parseOpenCodeStderrLine(line string) (openCodeStderrDiagnostic, bool) { } providerError := streams.ProviderError{ - Source: streams.ProviderErrorSourceOpenCodeStderr, - ProviderID: safeOpenCodeField(fields["providerID"]), - ModelID: safeOpenCodeField(fields["modelID"]), - Message: message, - OccurredAt: occurredAt, + Source: streams.ProviderErrorSourceOpenCodeStderr, + ProviderID: safeOpenCodeField(fields["providerID"]), + ModelID: safeOpenCodeField(fields["modelID"]), + Message: message, + RemediationURL: remediationURL, + OccurredAt: occurredAt, } if resetAt := openCodeResetAt(message, occurredAt); resetAt != nil { providerError.ResetAt = resetAt diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_stderr_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_stderr_test.go index 1158435811..42a6fd25db 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_stderr_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/opencode_stderr_test.go @@ -13,6 +13,132 @@ import ( "github.com/kandev/kandev/internal/agentctl/types/streams" ) +func TestNormalizeOpenCodeActionURLAcceptsOnlyAllowlistedRoute(t *testing.T) { + const want = "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" + accepted := []string{ + want, + "https://opencode.ai/workspace/a/go", + "https://opencode.ai/workspace/WRK-build_run-42/go", + "https://opencode.ai/workspace/" + strings.Repeat("x", 128) + "/go", + } + for _, raw := range accepted { + if got := NormalizeOpenCodeActionURL(raw); got != raw { + t.Errorf("NormalizeOpenCodeActionURL(%q) = %q, want the input itself", raw, got) + } + } + + rejected := []string{ + "", + "http://opencode.ai/workspace/wrk_123/go", + "https://opencode.example.com/workspace/wrk_123/go", + "https://opencode.ai/workspace/wrk_123", + "https://opencode.ai/workspace/wrk_123/go/", + "https://opencode.ai/workspace/wrk_123/go/extra", + "https://opencode.ai/workspace/wrk_123/other", + "https://opencode.ai/workspace/wrk_123/go?source=email", + "https://opencode.ai/workspace/wrk_123/go?", + "https://opencode.ai/workspace/wrk_123/go#fragment", + "https://opencode.ai/workspace/wrk_123/go#", + "https://opencode.ai/workspace/wrk_123/go?#", + "https://user:pass@opencode.ai/workspace/wrk_123/go", + "https://opencode.ai:443/workspace/wrk_123/go", + "https://opencode.ai/workspace/wrk_123%2F..%2Fgo", + "https://opencode.ai/workspace/%77rk_123/go", + "https://opencode.ai/workspace/../go", + "https://opencode.ai/workspace/../workspace/wrk_123/go", + "https://opencode.ai/workspace/wrk_123/go extra", + "https://opencode.ai/workspace/wrk_123/go.", + "https://opencode.ai/workspace/" + strings.Repeat("w", 200) + "/go", + "https://example.test/workspace/wrk_123/go", + "not a url", + } + for _, raw := range rejected { + if got := NormalizeOpenCodeActionURL(raw); got != "" { + t.Errorf("NormalizeOpenCodeActionURL(%q) = %q, want empty", raw, got) + } + } +} + +func TestExtractOpenCodeActionURLFindsValidTokenInMessage(t *testing.T) { + const want = "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" + line := "AI_APICallError: 5-hour usage limit reached. To continue using this model now, enable usage from your available balance: " + want + if got := extractOpenCodeActionURL(line); got != want { + t.Fatalf("extractOpenCodeActionURL() = %q, want %q", got, want) + } + for _, raw := range []string{ + "provider failed", + "visit https://example.test/workspace/wrk_123/go instead", + "balance: https://opencode.ai/workspace/wrk_123/go.", + "https://opencode.ai/workspace/wrk_123", + } { + if got := extractOpenCodeActionURL(raw); got != "" { + t.Errorf("extractOpenCodeActionURL(%q) = %q, want empty", raw, got) + } + } +} + +func TestProviderErrorFromACPRequestErrorReadsOnlyStructuredActionURL(t *testing.T) { + const wantURL = "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" + + t.Run("structured action_url projects a safe provider error", func(t *testing.T) { + err := &sdk.RequestError{ + Code: -32603, + Message: "AI_APICallError: 5-hour usage limit reached: " + wantURL, + Data: map[string]any{"action_url": wantURL}, + } + got := ProviderErrorFromError(err) + if got == nil { + t.Fatal("ProviderErrorFromError() = nil, want provider error") + } + if got.Source != streams.ProviderErrorSourceOpenCodeACP { + t.Fatalf("source = %q", got.Source) + } + if got.RemediationURL != wantURL { + t.Fatalf("remediation URL = %q, want %q", got.RemediationURL, wantURL) + } + if strings.Contains(got.Message, "https://") || + strings.Contains(got.Message, "wrk_01KQM7K5CYT715264YKKFB17ZY") { + t.Fatalf("message contains private URL or identifier: %q", got.Message) + } + if got.Message == "" { + t.Fatal("message is empty") + } + }) + + t.Run("missing or malformed action_url keeps the generic error path", func(t *testing.T) { + for _, tt := range []struct { + name string + err error + }{ + {name: "no data", err: &sdk.RequestError{Code: -32603, Message: "provider failed"}}, + {name: "wrong shape", err: &sdk.RequestError{Code: -32603, Message: "provider failed", Data: "action_url"}}, + {name: "wrong host", err: &sdk.RequestError{ + Code: -32603, Message: "provider failed", + Data: map[string]any{"action_url": "https://example.test/workspace/wrk_123/go"}, + }}, + {name: "query", err: &sdk.RequestError{ + Code: -32603, Message: "provider failed", + Data: map[string]any{"action_url": wantURL + "?source=email"}, + }}, + {name: "malformed id", err: &sdk.RequestError{ + Code: -32603, Message: "provider failed", + Data: map[string]any{"action_url": "https://opencode.ai/workspace/../go"}, + }}, + {name: "oversized", err: &sdk.RequestError{ + Code: -32603, Message: "provider failed", + Data: map[string]any{"action_url": "https://opencode.ai/workspace/" + strings.Repeat("w", 300) + "/go"}, + }}, + {name: "wrapped generic error", err: fmt.Errorf("provider failed")}, + } { + t.Run(tt.name, func(t *testing.T) { + if got := ProviderErrorFromError(tt.err); got != nil { + t.Fatalf("ProviderErrorFromError() = %+v, want nil", got) + } + }) + } + }) +} + func TestParseOpenCodeStderrLineAcceptsForegroundStreamError(t *testing.T) { line := `timestamp=2026-08-02T15:15:44.504Z level=ERROR run=a93db686 message="stream error" providerID=opencode-go modelID=kimi-k3 session.id=ses_03d1ac324ffeA2eAGeBfuq80dq small=false agent=build mode=primary error.error="AI_APICallError: 5-hour usage limit reached. Resets in 4hr 19min. To continue using this model now, enable usage from your available balance: https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go"` @@ -47,6 +173,58 @@ func TestParseOpenCodeStderrLineAcceptsForegroundStreamError(t *testing.T) { if diagnostic.ProviderError.Message == "" { t.Fatal("message is empty") } + wantURL := "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" + if diagnostic.ProviderError.RemediationURL != wantURL { + t.Fatalf("remediation URL = %q, want %q", diagnostic.ProviderError.RemediationURL, wantURL) + } +} + +func TestParseOpenCodeStderrLineCapturesExplicitActionURLField(t *testing.T) { + line := `timestamp=2026-08-02T15:15:44Z level=ERROR message="stream error" providerID=opencode-go modelID=kimi-k3 session.id=ses_123 small=false agent=build action_url="https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" error.error="AI_APICallError: usage limit reached"` + + diagnostic, ok := parseOpenCodeStderrLine(line) + if !ok { + t.Fatal("parseOpenCodeStderrLine() rejected an action_url record") + } + wantURL := "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" + if diagnostic.ProviderError.RemediationURL != wantURL { + t.Fatalf("remediation URL = %q, want %q", diagnostic.ProviderError.RemediationURL, wantURL) + } + if strings.Contains(diagnostic.ProviderError.Message, "https://") || + strings.Contains(diagnostic.ProviderError.Message, "wrk_") { + t.Fatalf("message contains private URL or identifier: %q", diagnostic.ProviderError.Message) + } +} + +func TestParseOpenCodeStderrLineRejectsUntrustedActionURLs(t *testing.T) { + base := `timestamp=2026-08-02T15:15:44Z level=ERROR message="stream error" providerID=opencode-go modelID=kimi-k3 session.id=ses_123 small=false agent=build error.error="usage limit reached"` + for _, tt := range []struct { + name string + line string + }{ + { + name: "wrong host", + line: strings.Replace(base, `error.error=`, `action_url="https://example.test/workspace/wrk_123/go" error.error=`, 1), + }, + { + name: "query", + line: strings.Replace(base, `error.error=`, `action_url="https://opencode.ai/workspace/wrk_123/go?x=1" error.error=`, 1), + }, + { + name: "malformed id", + line: strings.Replace(base, `error.error=`, `action_url="https://opencode.ai/workspace/../go" error.error=`, 1), + }, + } { + t.Run(tt.name, func(t *testing.T) { + diagnostic, ok := parseOpenCodeStderrLine(tt.line) + if !ok { + t.Fatal("parseOpenCodeStderrLine() rejected the record") + } + if diagnostic.ProviderError.RemediationURL != "" { + t.Fatalf("remediation URL = %q, want empty", diagnostic.ProviderError.RemediationURL) + } + }) + } } func TestPromptDiagnosticSettlesOnlyMatchingSession(t *testing.T) { diff --git a/apps/backend/internal/agentctl/server/api/agent.go b/apps/backend/internal/agentctl/server/api/agent.go index 840893743c..738b64cb4c 100644 --- a/apps/backend/internal/agentctl/server/api/agent.go +++ b/apps/backend/internal/agentctl/server/api/agent.go @@ -562,11 +562,22 @@ func (s *Server) handleWSPrompt(ctx context.Context, msg *ws.Message) *ws.Messag zap.Error(err)) return } - s.logger.Error("async prompt failed", zap.Error(err)) + providerError := acptransport.ProviderErrorFromError(err) + // The raw error string is only safe for the correlated stderr + // diagnostic (its Error() is the sanitized message). A structured + // ACP RequestError serializes its Data — including action_url and + // any other raw provider fields — into Error(), so when a safe + // provider diagnostic exists it must win for both the event and + // the log; never let the raw error cross the boundary. + message := err.Error() + if providerError != nil { + message = providerError.Message + } + s.logger.Error("async prompt failed", zap.String("error_message", message)) s.procMgr.SendErrorEventWithProviderError( - err.Error(), + message, req.PromptGeneration, - acptransport.ProviderErrorFromError(err), + providerError, ) } }() diff --git a/apps/backend/internal/agentctl/server/api/agent_test.go b/apps/backend/internal/agentctl/server/api/agent_test.go index a07b84c0c1..64da673dde 100644 --- a/apps/backend/internal/agentctl/server/api/agent_test.go +++ b/apps/backend/internal/agentctl/server/api/agent_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/coder/acp-go-sdk" "github.com/gorilla/websocket" "github.com/kandev/kandev/internal/agentctl/server/adapter" "github.com/kandev/kandev/internal/agentctl/server/config" @@ -419,6 +420,62 @@ func TestHandleWSPrompt_NoAdapter(t *testing.T) { } } +// TestHandleWSPrompt_SanitizesACPActionURLError is a regression test for the +// structured ACP service-failure path: RequestError.Error() serializes Data +// (action_url and any other raw provider fields), so the sanitized provider +// message must be the only text that reaches the error event and downstream +// persisted/UI error surfaces. +func TestHandleWSPrompt_SanitizesACPActionURLError(t *testing.T) { + s := newTestServer(t) + prompted := make(chan uint64, 1) + const wantURL = "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" + s.procMgr.SetAdapterForTest(&promptErrorAdapter{ + sessionID: "session-123", + err: &acp.RequestError{ + Code: -32603, + Message: "AI_APICallError: 5-hour usage limit reached: " + wantURL, + Data: map[string]any{ + "action_url": wantURL, + "raw_secret": "provider-internal-value", + }, + }, + prompted: prompted, + }) + + msg, _ := ws.NewRequest("req-1", "agent.prompt", PromptRequest{ + Text: "hello", + PromptGeneration: 42, + }) + resp := s.handleWSPrompt(context.Background(), msg) + if resp.Type != ws.MessageTypeResponse { + t.Fatalf("expected response type, got %q", resp.Type) + } + + select { + case event := <-s.procMgr.GetUpdates(): + if event.Type != adapter.EventTypeError { + t.Fatalf("event type = %q, want error", event.Type) + } + if strings.Contains(event.Error, "https://") || + strings.Contains(event.Error, "wrk_") || + strings.Contains(event.Error, "raw_secret") || + strings.Contains(event.Error, "provider-internal-value") { + t.Fatalf("error message leaks raw ACP data: %q", event.Error) + } + if !strings.Contains(event.Error, "usage limit reached") { + t.Fatalf("error message lost the sanitized copy: %q", event.Error) + } + if event.ProviderError == nil { + t.Fatal("expected a provider error diagnostic") + } + if event.ProviderError.RemediationURL != wantURL { + t.Fatalf("remediation URL = %q, want %q", event.ProviderError.RemediationURL, wantURL) + } + case <-time.After(2 * time.Second): + t.Fatal("expected an error event") + } +} + func TestHandleWSPrompt_SuppressesPromptAbandonedAfterCancel(t *testing.T) { s := newTestServer(t) prompted := make(chan uint64, 1) diff --git a/apps/backend/internal/agentctl/types/streams/provider_error.go b/apps/backend/internal/agentctl/types/streams/provider_error.go index f2a11c25a4..59907c3233 100644 --- a/apps/backend/internal/agentctl/types/streams/provider_error.go +++ b/apps/backend/internal/agentctl/types/streams/provider_error.go @@ -2,18 +2,25 @@ package streams import "time" -const ProviderErrorSourceOpenCodeStderr = "opencode_stderr" +const ( + ProviderErrorSourceOpenCodeStderr = "opencode_stderr" + // ProviderErrorSourceOpenCodeACP marks a provider diagnostic projected + // from a structured ACP service-failure response. + ProviderErrorSourceOpenCodeACP = "opencode_acp" +) // ProviderError is the bounded, sanitized provider diagnostic that may cross // the agentctl boundary. It intentionally contains no raw stderr or provider -// account/workspace identifiers. +// account/workspace identifiers. RemediationURL is only ever populated by the +// adapter-specific allowlist validator; it is never reconstructed from prose. type ProviderError struct { - Source string `json:"source,omitempty"` - ProviderID string `json:"provider_id,omitempty"` - ModelID string `json:"model_id,omitempty"` - Message string `json:"message,omitempty"` - OccurredAt time.Time `json:"occurred_at,omitempty"` - ResetAt *time.Time `json:"reset_at,omitempty"` + Source string `json:"source,omitempty"` + ProviderID string `json:"provider_id,omitempty"` + ModelID string `json:"model_id,omitempty"` + Message string `json:"message,omitempty"` + RemediationURL string `json:"remediation_url,omitempty"` + OccurredAt time.Time `json:"occurred_at,omitempty"` + ResetAt *time.Time `json:"reset_at,omitempty"` } func (e *ProviderError) Valid() bool { diff --git a/apps/backend/internal/gateway/websocket/task_notifications.go b/apps/backend/internal/gateway/websocket/task_notifications.go index caa1e136d7..96c412d86d 100644 --- a/apps/backend/internal/gateway/websocket/task_notifications.go +++ b/apps/backend/internal/gateway/websocket/task_notifications.go @@ -196,7 +196,35 @@ func (b *TaskEventBroadcaster) logSessionStateMetadata(action, sessionID string, b.logger.Debug("received session.state_changed with metadata", zap.String("action", action), zap.String("session_id", sessionID), - zap.Any("metadata", metadata)) + // RemediationURL is deliberately excluded from the wholesale metadata + // echo (ADR-2026-08-07-allowlisted-provider-action-links): the link is + // a user-visible destination, not a log field. + zap.Any("metadata", redactRemediationURL(metadata))) +} + +// redactRemediationURL returns a copy of the metadata with the +// last_agent_error.remediation_url key removed, so the debug echo never +// mutates the event payload that is broadcast to clients. +func redactRemediationURL(metadata interface{}) interface{} { + m, ok := metadata.(map[string]interface{}) + if !ok { + return metadata + } + lastErr, ok := m["last_agent_error"].(map[string]interface{}) + if !ok { + return metadata + } + copyLastErr := make(map[string]interface{}, len(lastErr)) + for key, value := range lastErr { + copyLastErr[key] = value + } + delete(copyLastErr, "remediation_url") + copyMetadata := make(map[string]interface{}, len(m)) + for key, value := range m { + copyMetadata[key] = value + } + copyMetadata["last_agent_error"] = copyLastErr + return copyMetadata } func (b *TaskEventBroadcaster) routeBroadcast( diff --git a/apps/backend/internal/gateway/websocket/task_notifications_redact_test.go b/apps/backend/internal/gateway/websocket/task_notifications_redact_test.go new file mode 100644 index 0000000000..81c43f0c8a --- /dev/null +++ b/apps/backend/internal/gateway/websocket/task_notifications_redact_test.go @@ -0,0 +1,45 @@ +package websocket + +import ( + "reflect" + "testing" +) + +func TestRedactRemediationURLRemovesOnlyTheLinkKey(t *testing.T) { + metadata := map[string]interface{}{ + "last_agent_error": map[string]interface{}{ + "message": "5-hour usage limit reached", + "remediation_url": "https://opencode.ai/workspace/wrk_123/go", + }, + "other_key": "kept", + } + redacted := redactRemediationURL(metadata).(map[string]interface{}) + lastErr := redacted["last_agent_error"].(map[string]interface{}) + if _, ok := lastErr["remediation_url"]; ok { + t.Fatal("remediation_url still present in the redacted echo") + } + if lastErr["message"] != "5-hour usage limit reached" { + t.Fatalf("message was dropped: %#v", lastErr) + } + if redacted["other_key"] != "kept" { + t.Fatalf("unrelated metadata was dropped: %#v", redacted) + } + // The original event payload must be untouched. + original := metadata["last_agent_error"].(map[string]interface{}) + if _, ok := original["remediation_url"]; !ok { + t.Fatal("redaction mutated the source payload") + } +} + +func TestRedactRemediationURLPassesThroughUnexpectedShapes(t *testing.T) { + for _, metadata := range []interface{}{ + "not a map", + nil, + map[string]interface{}{"last_agent_error": "not a map"}, + map[string]interface{}{}, + } { + if got := redactRemediationURL(metadata); !reflect.DeepEqual(got, metadata) { + t.Fatalf("redactRemediationURL(%#v) changed the value: %#v", metadata, got) + } + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_agent.go b/apps/backend/internal/orchestrator/event_handlers_agent.go index b00b568fe6..c830edadac 100644 --- a/apps/backend/internal/orchestrator/event_handlers_agent.go +++ b/apps/backend/internal/orchestrator/event_handlers_agent.go @@ -1476,6 +1476,7 @@ func (s *Service) persistLastAgentError(ctx context.Context, data watcher.AgentE Message: errMsg, OccurredAt: time.Now().UTC(), AgentExecutionID: data.AgentExecutionID, + RemediationURL: providerRemediationURL(data), } if err := s.repo.SetSessionMetadataKey(ctx, data.SessionID, models.SessionMetaKeyLastAgentError, lastErr); err != nil { s.logger.Warn("failed to persist last agent error", @@ -1494,6 +1495,9 @@ func (s *Service) persistLastAgentError(ctx context.Context, data watcher.AgentE "stamp": lastErr.Stamp(), "agent_execution_id": lastErr.AgentExecutionID, } + if lastErr.RemediationURL != "" { + eventData["remediation_url"] = lastErr.RemediationURL + } if err := s.eventBus.Publish(ctx, events.TaskSessionErrorChanged, bus.NewEvent( events.TaskSessionErrorChanged, "orchestrator", @@ -1559,6 +1563,17 @@ func (s *Service) clearRecoveredAgentError(ctx context.Context, taskID string, s } } +// providerRemediationURL returns the adapter-validated remediation URL from the +// normalized provider diagnostic, or "" when the failure carried none. The URL +// is only ever set by the adapter's allowlist validator; the orchestrator does +// not validate or reconstruct URLs from prose. +func providerRemediationURL(data watcher.AgentEventData) string { + if data.ProviderError == nil || !data.ProviderError.Valid() { + return "" + } + return data.ProviderError.RemediationURL +} + // createRecoveryStatusMessage builds and persists the ActionMessage shown // in the kanban chat surface after a recoverable agent failure. Must only // be called for non-office sessions (office sessions render their own error UI). @@ -1593,6 +1608,11 @@ func (s *Service) createRecoveryStatusMessage(ctx context.Context, data watcher. "is_auth_error": authErr, "resume_corrupted": resumeCorrupted, } + // The validated remediation URL is carried independently of quota + // classification so the generic recoverable card can still show the link. + if remediationURL := providerRemediationURL(data); remediationURL != "" { + meta["remediation_url"] = remediationURL + } applyProviderQuotaMetadata(meta, data) // Include cached auth methods so the frontend can show login options. diff --git a/apps/backend/internal/orchestrator/event_handlers_test.go b/apps/backend/internal/orchestrator/event_handlers_test.go index 6e45b8a564..08e896ddd8 100644 --- a/apps/backend/internal/orchestrator/event_handlers_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "path/filepath" + "strings" "sync" "sync/atomic" "testing" @@ -2387,6 +2388,74 @@ func TestClearResumeToken(t *testing.T) { func TestHandleRecoverableFailure(t *testing.T) { ctx := context.Background() + t.Run("persists validated remediation URL in last agent error metadata", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + taskRepo := newMockTaskRepo() + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithScheduler(repo, newMockStepGetter(), taskRepo, agentMgr) + const wantURL = "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" + + svc.handleRecoverableFailure(ctx, watcher.AgentEventData{ + TaskID: "t1", + SessionID: "s1", + AgentExecutionID: "exec-1", + ErrorMessage: "usage limit reached", + ProviderError: &streams.ProviderError{ + Source: streams.ProviderErrorSourceOpenCodeStderr, + Message: "usage limit reached", + RemediationURL: wantURL, + OccurredAt: time.Date(2026, 8, 2, 15, 15, 44, 0, time.UTC), + }, + }) + + session, err := repo.GetTaskSession(ctx, "s1") + if err != nil { + t.Fatalf("failed to get session: %v", err) + } + lastErr, ok := models.LoadLastAgentError(session.Metadata) + if !ok { + t.Fatalf("expected last agent error metadata, got %#v", session.Metadata) + } + if lastErr.RemediationURL != wantURL { + t.Fatalf("remediation URL = %q, want %q", lastErr.RemediationURL, wantURL) + } + // The plain error_message contract stays URL-free. + if strings.Contains(session.ErrorMessage, "https://") || + strings.Contains(session.ErrorMessage, "wrk_") { + t.Fatalf("error_message contains private URL or identifier: %q", session.ErrorMessage) + } + }) + + t.Run("omits remediation URL when the diagnostic carries none", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + taskRepo := newMockTaskRepo() + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithScheduler(repo, newMockStepGetter(), taskRepo, agentMgr) + + svc.handleRecoverableFailure(ctx, watcher.AgentEventData{ + TaskID: "t1", + SessionID: "s1", + AgentExecutionID: "exec-1", + ErrorMessage: "provider failed", + }) + + session, err := repo.GetTaskSession(ctx, "s1") + if err != nil { + t.Fatalf("failed to get session: %v", err) + } + lastErr, ok := models.LoadLastAgentError(session.Metadata) + if !ok { + t.Fatalf("expected last agent error metadata, got %#v", session.Metadata) + } + if lastErr.RemediationURL != "" { + t.Fatalf("remediation URL = %q, want empty", lastErr.RemediationURL) + } + }) + t.Run("sets session to WAITING_FOR_INPUT with error message", func(t *testing.T) { repo := setupTestRepo(t) seedSession(t, repo, "t1", "s1", "step1") diff --git a/apps/backend/internal/orchestrator/recovery_actions_test.go b/apps/backend/internal/orchestrator/recovery_actions_test.go index 8f8947a988..5f98ad11df 100644 --- a/apps/backend/internal/orchestrator/recovery_actions_test.go +++ b/apps/backend/internal/orchestrator/recovery_actions_test.go @@ -105,6 +105,7 @@ func TestCreateRecoveryStatusMessage_OpenCodeQuotaCarriesSafeMetadata(t *testing mc := &mockMessageCreator{} svc.messageCreator = mc resetAt := time.Date(2026, 8, 2, 19, 34, 44, 0, time.UTC) + const wantURL = "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" svc.createRecoveryStatusMessage(ctx, watcher.AgentEventData{ TaskID: "t-quota", @@ -112,12 +113,13 @@ func TestCreateRecoveryStatusMessage_OpenCodeQuotaCarriesSafeMetadata(t *testing AgentID: "opencode-acp", ErrorMessage: "5-hour usage limit reached", ProviderError: &streams.ProviderError{ - Source: streams.ProviderErrorSourceOpenCodeStderr, - ProviderID: "opencode-go", - ModelID: "kimi-k3", - Message: "5-hour usage limit reached", - OccurredAt: time.Date(2026, 8, 2, 15, 15, 44, 0, time.UTC), - ResetAt: &resetAt, + Source: streams.ProviderErrorSourceOpenCodeStderr, + ProviderID: "opencode-go", + ModelID: "kimi-k3", + Message: "5-hour usage limit reached", + RemediationURL: wantURL, + OccurredAt: time.Date(2026, 8, 2, 15, 15, 44, 0, time.UTC), + ResetAt: &resetAt, }, }) @@ -137,4 +139,89 @@ func TestCreateRecoveryStatusMessage_OpenCodeQuotaCarriesSafeMetadata(t *testing if meta["error_output"] != "5-hour usage limit reached" { t.Fatalf("error_output = %#v", meta["error_output"]) } + if meta["remediation_url"] != wantURL { + t.Fatalf("remediation_url = %#v, want %q", meta["remediation_url"], wantURL) + } +} + +func TestCreateRecoveryStatusMessage_GenericFailureCarriesRemediationURL(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t-generic", "s-generic", "step1") + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithScheduler(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + mc := &mockMessageCreator{} + svc.messageCreator = mc + const wantURL = "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go" + + // A structured ACP-sourced diagnostic is not quota-classified, but the + // validated link still reaches the generic recoverable card. + svc.createRecoveryStatusMessage(ctx, watcher.AgentEventData{ + TaskID: "t-generic", + SessionID: "s-generic", + AgentID: "opencode-acp", + ErrorMessage: "AI_APICallError: usage limit reached", + ProviderError: &streams.ProviderError{ + Source: streams.ProviderErrorSourceOpenCodeACP, + Message: "usage limit reached", + RemediationURL: wantURL, + OccurredAt: time.Date(2026, 8, 2, 15, 15, 44, 0, time.UTC), + }, + }) + + if len(mc.sessionMessages) != 1 { + t.Fatalf("expected 1 session message, got %d", len(mc.sessionMessages)) + } + meta := mc.sessionMessages[0].metadata + if meta["failure_kind"] != nil { + t.Fatalf("failure_kind = %#v, want unset for generic failure", meta["failure_kind"]) + } + if meta["remediation_url"] != wantURL { + t.Fatalf("remediation_url = %#v, want %q", meta["remediation_url"], wantURL) + } + if _, ok := meta["error_output"]; ok { + t.Fatalf("error_output unexpectedly set for non-quota diagnostic: %#v", meta["error_output"]) + } +} + +func TestProviderRemediationURLRejectsInvalidDiagnostics(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t-none", "s-none", "step1") + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithScheduler(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + mc := &mockMessageCreator{} + svc.messageCreator = mc + + for _, tt := range []struct { + name string + providerErr *streams.ProviderError + }{ + {name: "no provider error"}, + {name: "no remediation url", providerErr: &streams.ProviderError{ + Source: streams.ProviderErrorSourceOpenCodeStderr, + Message: "usage limit reached", + OccurredAt: time.Date(2026, 8, 2, 15, 15, 44, 0, time.UTC), + }}, + {name: "invalid provider error", providerErr: &streams.ProviderError{ + Source: streams.ProviderErrorSourceOpenCodeStderr, + RemediationURL: "https://opencode.ai/workspace/wrk_123/go", + }}, + } { + t.Run(tt.name, func(t *testing.T) { + svc.createRecoveryStatusMessage(ctx, watcher.AgentEventData{ + TaskID: "t-none", + SessionID: "s-none", + ErrorMessage: "provider failed", + ProviderError: tt.providerErr, + }) + if len(mc.sessionMessages) == 0 { + t.Fatal("expected a session message") + } + if url, ok := mc.sessionMessages[0].metadata["remediation_url"]; ok && url != "" { + t.Fatalf("remediation_url = %#v, want absent", url) + } + mc.sessionMessages = nil + }) + } } diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 9d1044ebe1..cfd0adaff3 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -315,10 +315,13 @@ const SessionMetaKeyPendingStepCompletion = "pending_step_completion_signal" const SessionMetaKeyLastAgentError = "last_agent_error" // LastAgentError is persisted under TaskSession.Metadata[SessionMetaKeyLastAgentError]. +// RemediationURL is only ever set from an adapter-validated provider +// diagnostic; it is never reconstructed from the error message. type LastAgentError struct { Message string `json:"message"` OccurredAt time.Time `json:"occurred_at"` AgentExecutionID string `json:"agent_execution_id,omitempty"` + RemediationURL string `json:"remediation_url,omitempty"` DismissedAt *time.Time `json:"dismissed_at,omitempty"` } diff --git a/apps/web/app/office/tasks/[id]/page.tsx b/apps/web/app/office/tasks/[id]/page.tsx index d209bc5377..e95acad65b 100644 --- a/apps/web/app/office/tasks/[id]/page.tsx +++ b/apps/web/app/office/tasks/[id]/page.tsx @@ -13,6 +13,10 @@ import { type TaskDecisionDTO, } from "@/lib/api/domains/office-api"; import { listTaskSessions } from "@/lib/api/domains/session-api"; +import { + liveSessionMetadataFromStore, + mergeLiveSessionMetadata, +} from "@/components/task/simple/chat-entries"; import { OfficeSimplePane } from "@/components/task/simple/OfficeSimplePane"; import { TaskAdvancedMode } from "./task-advanced-mode"; import { IssueDetailSkeleton } from "./task-detail-skeleton"; @@ -35,6 +39,11 @@ type IssueDetailPageProps = { params: Promise<{ id: string }>; }; +// Sentinel for the live-session metadata key: distinguishes "no store entry" +// (keep the initial fetch) from an explicit server-side null (metadata was +// cleared and must not be resurrected). +const SESSION_METADATA_ABSENT = "\u0000absent\u0000"; + function mapDecisionDTO(d: TaskDecisionDTO): TaskDecision { return { id: d.id, @@ -166,6 +175,7 @@ function mapTaskSession(session: ApiTaskSession): TaskSession { completedAt: session.completed_at ?? undefined, updatedAt: session.updated_at, errorMessage: session.error_message ?? undefined, + metadata: session.metadata ?? undefined, commandCount: session.command_count, }; } @@ -236,6 +246,34 @@ function useSessionLiveSync({ [sessionStatesKey], ); + // Same stable-key trick for the live metadata (last_agent_error etc.) + // carried by session.state_changed, so the office chat can render the + // remediation link without a refetch. Tri-state per session: the sentinel + // marks "no metadata update" (no store row, or a partial row without a + // metadata field), explicit null means the server cleared metadata, and an + // object is the live metadata. + const sessionMetadataKey = useAppStore((s) => { + const items = s.taskSessions?.items ?? {}; + return baseSessions + .map((sess) => + JSON.stringify(liveSessionMetadataFromStore(items, sess.id) ?? SESSION_METADATA_ABSENT), + ) + .join("\u0001"); + }); + const sessionStoreMetadata = useMemo( + () => + sessionMetadataKey.split("\u0001").map((chunk) => { + try { + const parsed = JSON.parse(chunk); + if (parsed === SESSION_METADATA_ABSENT) return undefined; + return parsed && typeof parsed === "object" ? (parsed as Record) : null; + } catch { + return null; + } + }), + [sessionMetadataKey], + ); + const connectionStatus = useAppStore((s) => s.connection.status); useSessionLiveSyncSubscriptions({ connectionStatus, @@ -261,7 +299,7 @@ function useSessionLiveSync({ void onCommentsRefetch(); }, [sessionStatesKey, taskId, onTaskRefetch, onCommentsRefetch]); - return sessionStoreStates; + return { sessionStoreStates, sessionStoreMetadata }; } // --------------------------------------------------------------------------- @@ -391,7 +429,7 @@ function useIssueData(id: string) { setTimeline(updatedTimeline); }, []); - const sessionStoreStates = useSessionLiveSync({ + const { sessionStoreStates, sessionStoreMetadata } = useSessionLiveSync({ task, baseSessions, onTaskRefetch, @@ -402,8 +440,11 @@ function useIssueData(id: string) { baseSessions.map((s, i) => ({ ...s, state: (sessionStoreStates[i] ?? s.state) as TaskSession["state"], + // Live session.state_changed metadata wins over the initial fetch; + // explicit null (server cleared metadata) is preserved. + metadata: mergeLiveSessionMetadata(s.metadata, sessionStoreMetadata[i]), })), - [baseSessions, sessionStoreStates], + [baseSessions, sessionStoreStates, sessionStoreMetadata], ); // Refetch comments when a new comment is created via office WS event diff --git a/apps/web/app/office/tasks/[id]/types.ts b/apps/web/app/office/tasks/[id]/types.ts index b4f888215a..1b6805797b 100644 --- a/apps/web/app/office/tasks/[id]/types.ts +++ b/apps/web/app/office/tasks/[id]/types.ts @@ -83,6 +83,8 @@ export type TaskSession = { updatedAt?: string; /** Verbatim error payload populated when state === "FAILED". */ errorMessage?: string; + /** Server metadata (incl. last_agent_error); refreshed by session.state_changed. */ + metadata?: Record | null; /** Server-resolved tool_call count; powers the "ran N commands" segment * of the timeline entry header without a per-session message fetch. */ commandCount?: number; @@ -99,6 +101,8 @@ export type RunError = { agentProfileId?: string; rawPayload: string; failedAt: string; + /** Adapter-validated provider remediation URL from last_agent_error metadata. */ + remediationUrl?: string; }; export type TaskLabelLocal = { diff --git a/apps/web/components/task/chat/message-list-shared.test.tsx b/apps/web/components/task/chat/message-list-shared.test.tsx index 8be76fbba6..e36c1c653b 100644 --- a/apps/web/components/task/chat/message-list-shared.test.tsx +++ b/apps/web/components/task/chat/message-list-shared.test.tsx @@ -1,6 +1,6 @@ import { useState } from "react"; import type { ReactNode } from "react"; -import { fireEvent, render, screen } from "@testing-library/react"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import { afterEach, describe, expect, it, vi } from "vitest"; import { buildGroupedRenderItems, type RenderItem } from "@/hooks/use-processed-messages"; import type { Message } from "@/lib/types/http"; @@ -13,6 +13,10 @@ function elementWithRect(top: number, bottom: number): HTMLElement { el.getBoundingClientRect = () => ({ top, bottom }) as DOMRect; return el; } +const AGENT_ERROR_MESSAGE = "agent process exited"; + +// NOTE: mockStoreState is vi.hoisted, so it cannot reference the const above — +// the factory runs before module initialization. const mockStoreState = vi.hoisted(() => ({ taskSessions: { items: { @@ -233,17 +237,20 @@ describe("MessageItem memo boundary", () => { }); describe("MessageItem agent error notice", () => { - it("shows retained agent errors even when there are no messages", () => { + afterEach(() => { + cleanup(); + }); + + const REMEDIATION_URL = "https://opencode.ai/workspace/wrk_01KQM7K5CYT715264YKKFB17ZY/go"; + + function renderNotice(error: { message: string; occurredAt?: string; remediationUrl?: string }) { render( { onScrollToMessage={noop} />, ); + } + + it("shows retained agent errors even when there are no messages", () => { + renderNotice({ message: AGENT_ERROR_MESSAGE }); expect(screen.getByTestId("last-agent-error-notice").getAttribute("role")).toBe("alert"); - expect(screen.queryByText("agent process exited")).not.toBeNull(); + expect(screen.getByTestId("last-agent-error-notice").textContent).toContain( + AGENT_ERROR_MESSAGE, + ); + }); + + it("renders a validated remediation link, and nothing for an invalid URL", () => { + renderNotice({ + message: "usage limit reached", + remediationUrl: REMEDIATION_URL, + }); + + const link = screen.getByTestId("remediation-link") as HTMLAnchorElement; + expect(link.href).toBe(REMEDIATION_URL); + expect(link.rel).toBe("noopener noreferrer"); + expect(screen.getByTestId("last-agent-error-notice").textContent).toContain( + "usage limit reached", + ); + + cleanup(); + renderNotice({ message: "usage limit reached", remediationUrl: "https://evil.example.com/x" }); + expect(screen.queryByTestId("remediation-link")).toBeNull(); }); }); diff --git a/apps/web/components/task/chat/message-list-shared.tsx b/apps/web/components/task/chat/message-list-shared.tsx index 21a9d5297c..80bd601bfe 100644 --- a/apps/web/components/task/chat/message-list-shared.tsx +++ b/apps/web/components/task/chat/message-list-shared.tsx @@ -11,6 +11,7 @@ import { TurnGroupMessage } from "@/components/task/chat/messages/turn-group-mes import { PrepareProgress } from "@/components/session/prepare-progress"; import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { dismissLastAgentError } from "@/lib/api/domains/session-api"; +import { RemediationLink } from "@/components/task/remediation-link"; import { type LastAgentError, lastAgentErrorStamp, @@ -329,6 +330,11 @@ export function LastAgentErrorNotice({ 
             {error.message}
+ {error.remediationUrl && ( +
+ +
+ )}