From fa68474e6f5682ef16d8337daebb2fb64d3076a2 Mon Sep 17 00:00:00 2001 From: whoisasx Date: Sat, 4 Jul 2026 00:14:43 +0530 Subject: [PATCH] fix: clean prompt files and reduce nudge noise --- backend/internal/lifecycle/manager_test.go | 54 ++++++++++++++++++- backend/internal/lifecycle/reactions.go | 39 ++++++++++---- backend/internal/session_manager/manager.go | 29 +++++++++- .../internal/session_manager/manager_test.go | 26 ++++++++- 4 files changed, 134 insertions(+), 14 deletions(-) diff --git a/backend/internal/lifecycle/manager_test.go b/backend/internal/lifecycle/manager_test.go index 613c8fb80..15ecec028 100644 --- a/backend/internal/lifecycle/manager_test.go +++ b/backend/internal/lifecycle/manager_test.go @@ -226,13 +226,65 @@ func TestPRObservation_CIFailingNudgesAgentWithLogs(t *testing.T) { "Failure URL: https://ci.example/build", "Log tail (last 1 line):", "boom", - "Failed: lint (cancelled)", "fetch full CI logs only if you need additional context", } { if !strings.Contains(msg.msgs[0], want) { t.Fatalf("CI nudge missing %q:\n%s", want, msg.msgs[0]) } } + if strings.Contains(msg.msgs[0], "lint") || strings.Contains(msg.msgs[0], "cancelled") { + t.Fatalf("cancelled checks must not be included in CI nudge:\n%s", msg.msgs[0]) + } +} + +func TestPRObservation_CancelledChecksDoNotNudge(t *testing.T) { + m, st, msg := newManager() + st.sessions["mer-1"] = working("mer-1") + o := ports.PRObservation{Fetched: true, URL: "pr1", CI: domain.CIFailing, Checks: []ports.PRCheckObservation{ + {Name: "lint", CommitHash: "c1", Status: domain.PRCheckCancelled, URL: "https://ci.example/lint"}, + }} + if err := m.ApplyPRObservation(ctx, "mer-1", o); err != nil { + t.Fatal(err) + } + if len(msg.msgs) != 0 { + t.Fatalf("cancelled-only checks must not nudge, got %v", msg.msgs) + } +} + +func TestReviewCommentsSignatureUsesStableIDs(t *testing.T) { + original := []ports.PRCommentObservation{ + {ID: "c1", ThreadID: "t1", Author: "alice", File: "old.go", Line: 10, Body: "old", URL: "https://old"}, + {ID: "c2", ThreadID: "t2", Author: "bob", File: "old.go", Line: 20, Body: "old", URL: "https://old"}, + } + editedAndReordered := []ports.PRCommentObservation{ + {ID: "c2", ThreadID: "t2", Author: "bob", File: "new.go", Line: 99, Body: "edited", URL: "https://new"}, + {ID: "c1", ThreadID: "t1", Author: "alice", File: "new.go", Line: 42, Body: "edited", URL: "https://new"}, + } + if got, want := reviewCommentsSignature(editedAndReordered), reviewCommentsSignature(original); got != want { + t.Fatalf("signature changed after edit/reorder\n got %q\nwant %q", got, want) + } + + withNewComment := append([]ports.PRCommentObservation(nil), original...) + withNewComment = append(withNewComment, ports.PRCommentObservation{ID: "c3", ThreadID: "t2", Body: "new comment in same thread"}) + if got, old := reviewCommentsSignature(withNewComment), reviewCommentsSignature(original); got == old { + t.Fatalf("new comment id should change signature, got %q", got) + } +} + +func TestFormatCIFailureMessageUsesNonMutatingFence(t *testing.T) { + logTail := "start\n```\ninner\n````\nend" + msg := formatCIFailureMessage([]ports.PRCheckObservation{{ + Name: "build", Status: domain.PRCheckFailed, LogTail: logTail, + }}) + if !strings.Contains(msg, logTail) { + t.Fatalf("message should preserve log text without zero-width mutation:\n%s", msg) + } + if strings.Contains(msg, "\u200b") { + t.Fatalf("message must not insert zero-width characters:\n%s", msg) + } + if !strings.Contains(msg, "`````\n"+logTail+"\n`````") { + t.Fatalf("message should wrap log in a fence longer than embedded runs:\n%s", msg) + } } func TestPRObservation_ReviewCommentsNudgeAgent(t *testing.T) { diff --git a/backend/internal/lifecycle/reactions.go b/backend/internal/lifecycle/reactions.go index 1ff2f45eb..808f67a39 100644 --- a/backend/internal/lifecycle/reactions.go +++ b/backend/internal/lifecycle/reactions.go @@ -511,7 +511,7 @@ func hasUnresolvedComments(comments []ports.PRCommentObservation) bool { func failedPRChecks(checks []ports.PRCheckObservation) []ports.PRCheckObservation { failed := make([]ports.PRCheckObservation, 0, len(checks)) for _, ch := range checks { - if ch.Status == domain.PRCheckFailed || ch.Status == domain.PRCheckCancelled { + if ch.Status == domain.PRCheckFailed { failed = append(failed, ch) } } @@ -545,14 +545,16 @@ func formatCIFailureMessage(checks []ports.PRCheckObservation) string { if ch.LogTail != "" { // LogTail is raw CI job output; sanitize before it reaches the // agent's live pane so embedded escape sequences can't drive the - // terminal (the dedup signature stays on the raw bytes). - tail := escapeMarkdownCodeFenceClosers(domain.SanitizeControlChars(ch.LogTail)) + // terminal (the dedup signature stays on the raw bytes). The fence + // grows to contain embedded backtick fences without mutating logs. + tail := domain.SanitizeControlChars(ch.LogTail) + fence := markdownCodeFence(tail) lineCount := len(strings.Split(tail, "\n")) lineLabel := "lines" if lineCount == 1 { lineLabel = "line" } - fmt.Fprintf(&msg, "\n\nLog tail (last %d %s):\n```\n%s\n```", lineCount, lineLabel, tail) + fmt.Fprintf(&msg, "\n\nLog tail (last %d %s):\n%s\n%s\n%s", lineCount, lineLabel, fence, tail, fence) } msg.WriteString("\n") } @@ -574,8 +576,14 @@ func unresolvedReviewComments(comments []ports.PRCommentObservation) []ports.PRC func reviewCommentsSignature(comments []ports.PRCommentObservation) string { parts := make([]string, 0, len(comments)) for _, c := range comments { - parts = append(parts, strings.Join([]string{c.ID, c.ThreadID, c.Author, c.File, fmt.Sprint(c.Line), c.Body, c.URL}, "\x00")) + id := strings.TrimSpace(c.ID) + threadID := strings.TrimSpace(c.ThreadID) + if id == "" && threadID == "" { + continue + } + parts = append(parts, threadID+"\x00"+id) } + sort.Strings(parts) return strings.Join(parts, "\x01") } @@ -614,14 +622,23 @@ func formatReviewCommentsMessage(comments []ports.PRCommentObservation) string { return msg.String() } -func escapeMarkdownCodeFenceClosers(s string) string { - lines := strings.Split(s, "\n") - for i, line := range lines { - if strings.HasPrefix(line, "```") { - lines[i] = "\u200b" + line +func markdownCodeFence(s string) string { + maxRun := 0 + run := 0 + for _, r := range s { + if r == '`' { + run++ + if run > maxRun { + maxRun = run + } + continue } + run = 0 } - return strings.Join(lines, "\n") + if maxRun < 3 { + return "```" + } + return strings.Repeat("`", maxRun+1) } func (m *Manager) sendOnce(ctx context.Context, id domain.SessionID, prURL, key, sig, msg string, maxAttempts int) error { diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index 6a46ad42b..c3613b0ce 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -386,6 +386,7 @@ func sessionPrefix(project domain.ProjectRecord) string { // rollbackSpawnSeedRow. func (m *Manager) markSpawnFailedTerminated(ctx context.Context, id domain.SessionID) { _ = m.lcm.MarkTerminated(ctx, id) + m.cleanupSystemPromptDir(id) } // rollbackSpawnSeedRow best-effort removes the row of a spawn that failed @@ -395,6 +396,7 @@ func (m *Manager) markSpawnFailedTerminated(ctx context.Context, id domain.Sessi // fails, fall back to parking it terminated so a phantom row never looks live. func (m *Manager) rollbackSpawnSeedRow(ctx context.Context, id domain.SessionID) { if deleted, err := m.store.DeleteSession(ctx, id); err == nil && deleted { + m.cleanupSystemPromptDir(id) return } m.markSpawnFailedTerminated(ctx, id) @@ -417,6 +419,7 @@ func (m *Manager) rollbackSpawn(ctx context.Context, id domain.SessionID) (delet return false, false, fmt.Errorf("rollback %s: %w", id, err) } if deleted { + m.cleanupSystemPromptDir(id) return true, false, nil } killed, err = m.Kill(ctx, id) @@ -456,6 +459,7 @@ func (m *Manager) Kill(ctx context.Context, id domain.SessionID) (bool, error) { if err := m.lcm.MarkTerminated(ctx, id); err != nil { return false, fmt.Errorf("kill %s: %w", id, err) } + defer m.cleanupSystemPromptDir(id) // Clear the restore marker so the next boot's RestoreAll cannot resurrect a // killed session (#2319). Best-effort: teardown below still matters. @@ -540,12 +544,14 @@ func (m *Manager) Restore(ctx context.Context, id domain.SessionID) (domain.Sess } systemPromptFile, err := m.prepareSystemPromptFile(id, rec.Harness, systemPrompt) if err != nil { + m.cleanupSystemPromptDir(id) return domain.SessionRecord{}, fmt.Errorf("restore %s: system prompt file: %w", id, err) } // Restore re-applies the project's resolved agent config so a configured // model/permissions carry across a restore, matching fresh spawn. argv, err := restoreArgv(ctx, agent, id, ws.Path, meta, systemPrompt, systemPromptFile, effectiveAgentConfig(rec.Kind, project.Config), rec.Kind) if err != nil { + m.cleanupSystemPromptDir(id) return domain.SessionRecord{}, fmt.Errorf("restore %s: %w", id, err) } handle, err := m.runtime.Create(ctx, ports.RuntimeConfig{ @@ -555,11 +561,13 @@ func (m *Manager) Restore(ctx context.Context, id domain.SessionID) (domain.Sess Env: m.runtimeEnv(id, rec.ProjectID, rec.IssueID, project.Config.Env), }) if err != nil { + m.cleanupSystemPromptDir(id) return domain.SessionRecord{}, fmt.Errorf("restore %s: runtime: %w", id, err) } metadata := domain.SessionMetadata{Branch: ws.Branch, WorkspacePath: ws.Path, RuntimeHandleID: handle.ID, AgentSessionID: meta.AgentSessionID, Prompt: meta.Prompt} if err := m.lcm.MarkSpawned(ctx, id, metadata); err != nil { _ = m.runtime.Destroy(ctx, handle) + m.cleanupSystemPromptDir(id) return domain.SessionRecord{}, fmt.Errorf("restore %s: completed: %w", id, err) } return m.getRecord(ctx, id) @@ -906,6 +914,7 @@ func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) (Cleanu } ws := workspaceInfo(rec) if ws.Path == "" { + m.cleanupSystemPromptDir(rec.ID) continue } if h := runtimeHandle(rec.Metadata); h.ID != "" { @@ -920,6 +929,7 @@ func (m *Manager) Cleanup(ctx context.Context, project domain.ProjectID) (Cleanu result.Skipped = append(result.Skipped, CleanupSkip{SessionID: rec.ID, Reason: cleanupSkipReason(err)}) continue } + m.cleanupSystemPromptDir(rec.ID) result.Cleaned = append(result.Cleaned, rec.ID) } return result, nil @@ -1089,7 +1099,7 @@ func (m *Manager) writeSystemPromptFile(id domain.SessionID, systemPrompt string if systemPrompt == "" || strings.TrimSpace(m.dataDir) == "" { return "", nil } - path := filepath.Join(m.dataDir, "prompts", string(id), "system.md") + path := filepath.Join(m.systemPromptDir(id), "system.md") if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return "", err } @@ -1115,6 +1125,23 @@ func systemPromptFileRequired(harness domain.AgentHarness) bool { return harness == domain.HarnessAider } +func (m *Manager) systemPromptDir(id domain.SessionID) string { + if strings.TrimSpace(m.dataDir) == "" { + return "" + } + return filepath.Join(m.dataDir, "prompts", string(id)) +} + +func (m *Manager) cleanupSystemPromptDir(id domain.SessionID) { + dir := m.systemPromptDir(id) + if dir == "" { + return + } + if err := os.RemoveAll(dir); err != nil { + m.logger.Warn("system prompt cleanup failed", "session", id, "path", dir, "err", err) + } +} + // spawnEnv builds the runtime environment: the per-project env vars first, then // the AO-internal vars last so they always win (a project cannot override // AO_SESSION_ID and friends). diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index 58097c0c1..34bb3f72d 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -242,6 +242,16 @@ func blockedDataDir(t *testing.T) string { return path } +func requireNoPromptDir(t *testing.T, dataDir string, id domain.SessionID) { + t.Helper() + path := filepath.Join(dataDir, "prompts", string(id)) + if _, err := os.Stat(path); err == nil { + t.Fatalf("prompt dir %s still exists", path) + } else if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("stat prompt dir %s: %v", path, err) + } +} + // alwaysResumeAgent mimics Claude Code: it pins a deterministic session id, so // GetRestoreCommand can resume any session even with no captured agentSessionId // and no prompt. @@ -520,7 +530,12 @@ func TestSpawn_ParksRowTerminatedWhenSeedDeleteFails(t *testing.T) { } func TestKill_TearsDownRuntimeAndWorkspace(t *testing.T) { m, st, rt, ws := newManager() + dataDir := t.TempDir() + m.dataDir = dataDir st.sessions["mer-1"] = mkLive("mer-1") + if _, err := m.writeSystemPromptFile("mer-1", "system prompt"); err != nil { + t.Fatal(err) + } freed, err := m.Kill(ctx, "mer-1") if err != nil || !freed { t.Fatalf("freed=%v err=%v", freed, err) @@ -528,6 +543,7 @@ func TestKill_TearsDownRuntimeAndWorkspace(t *testing.T) { if rt.destroyed != 1 || ws.destroyed != 1 { t.Fatal("kill should destroy runtime and workspace") } + requireNoPromptDir(t, dataDir, "mer-1") } // TestKill_TerminatesIncompleteHandle: a session whose runtime handle or @@ -1356,12 +1372,17 @@ func TestRestore_RefusesIncompleteHandle(t *testing.T) { // outright by RollbackSpawn so the user never sees an orphan terminated row. func TestRollbackSpawn_DeletesSeedRow(t *testing.T) { m, st, _, _ := newManager() + dataDir := t.TempDir() + m.dataDir = dataDir // Seed row matches what CreateSession produces — no Metadata at all. st.sessions["mer-1"] = domain.SessionRecord{ ID: "mer-1", ProjectID: "mer", Activity: domain.Activity{State: domain.ActivityIdle}, } + if _, err := m.writeSystemPromptFile("mer-1", "system prompt"); err != nil { + t.Fatal(err) + } deleted, killed, err := m.RollbackSpawn(ctx, "mer-1") if err != nil { t.Fatalf("rollback err = %v", err) @@ -1372,6 +1393,7 @@ func TestRollbackSpawn_DeletesSeedRow(t *testing.T) { if _, present := st.sessions["mer-1"]; present { t.Fatal("seed row must be removed from the store, not left as terminated") } + requireNoPromptDir(t, dataDir, "mer-1") } // TestRollbackSpawn_FallsBackToKillForLiveRow asserts the no-resurrection @@ -1405,13 +1427,14 @@ func TestSpawn_RejectsMissingAgentBinary(t *testing.T) { st.projects["mer"] = domain.ProjectRecord{ID: "mer", Config: testRoleAgents()} rt := &fakeRuntime{} ws := &fakeWorkspace{} + dataDir := t.TempDir() notFound := func(name string) (string, error) { if name == "tmux" { return "/bin/tmux", nil } return "", fmt.Errorf("exec: %q: not found", name) } - m := New(Deps{Runtime: rt, Agents: fakeAgents{}, Workspace: ws, Store: st, Messenger: &fakeMessenger{}, Lifecycle: &fakeLCM{store: st}, LookPath: notFound}) + m := New(Deps{Runtime: rt, Agents: fakeAgents{}, Workspace: ws, Store: st, Messenger: &fakeMessenger{}, Lifecycle: &fakeLCM{store: st}, DataDir: dataDir, LookPath: notFound}) _, err := m.Spawn(ctx, ports.SpawnConfig{ProjectID: "mer", Kind: domain.KindWorker}) if !errors.Is(err, ports.ErrAgentBinaryNotFound) { @@ -1426,6 +1449,7 @@ func TestSpawn_RejectsMissingAgentBinary(t *testing.T) { if rec, present := st.sessions["mer-1"]; present { t.Fatalf("seed row must be deleted before a runtime handle is live, got %+v", rec) } + requireNoPromptDir(t, dataDir, "mer-1") } func TestSpawn_RejectsMissingTmuxBeforeSessionRow(t *testing.T) {