From 9e5f71f558685a203e5665f552d9280731331ddd Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 29 Sep 2026 21:23:25 +0200 Subject: [PATCH 1/3] Follow-ups: git-backed basis, previous answer as context, marker-gated memory A follow-up asking for a diagram of the previous answer failed as "basis no longer indexed" a minute after the answer, and a retry stored it as a standing rule. - message_sources records repo, path, commit and lines; a rework and a re-explain re-read their basis from git through the source viewer, so a poll re-inserting a touched file's chunks no longer loses it (0037 backfills rows whose chunks still exist). - The previous answer reaches the answer prompt of a follow-up as context that is never cited. Changes and release lanes keep the question only. - A diagram of the previous answer is a rework. - A standing rule is kept only when the gate quotes a permanence word the question actually holds; the same rule twice is one rule. - The writing step reports which commits a re-read basis came from. --- AGENTS.md | 6 +- backend/cmd/rongo/main.go | 6 +- backend/internal/ask/answer.go | 71 +++-- backend/internal/ask/answer_retry_test.go | 6 +- backend/internal/ask/answer_stages_test.go | 8 +- backend/internal/ask/answer_test.go | 44 ++-- backend/internal/ask/answer_testlabel_test.go | 2 +- backend/internal/ask/changes.go | 2 +- backend/internal/ask/docsonly_test.go | 4 +- .../internal/ask/language_everywhere_test.go | 6 +- backend/internal/ask/language_test.go | 4 +- backend/internal/ask/memory_test.go | 104 +++++++- backend/internal/ask/pipeline.go | 77 ++++-- backend/internal/ask/pipeline_test.go | 58 +++- backend/internal/ask/process_test.go | 4 +- backend/internal/ask/release.go | 2 +- backend/internal/ask/rework.go | 21 +- backend/internal/ask/rework_test.go | 31 +++ backend/internal/ask/scope_test.go | 16 +- backend/internal/ask/understand.go | 45 +++- backend/internal/httpapi/rework_test.go | 51 ++++ backend/internal/memory/context.go | 5 +- backend/internal/memory/memory.go | 24 +- backend/internal/memory/memory_test.go | 36 +++ .../internal/retrieve/eval/flow-rubrics.json | 26 ++ backend/internal/retrieve/eval/intent_test.go | 73 ++++- backend/internal/sourceview/commit.go | 28 ++ backend/internal/sourceview/commit_test.go | 24 ++ .../store/migrations/0037_source_identity.sql | 32 +++ .../store/source_identity_backfill_test.go | 70 +++++ .../internal/threads/source_identity_test.go | 184 +++++++++++++ backend/internal/threads/sources.go | 249 ++++++++++++++++++ backend/internal/threads/store.go | 120 +-------- ui/src/Trace.tsx | 10 + ui/src/TraceDetailMore.test.tsx | 12 + 35 files changed, 1210 insertions(+), 251 deletions(-) create mode 100644 backend/internal/store/migrations/0037_source_identity.sql create mode 100644 backend/internal/store/source_identity_backfill_test.go create mode 100644 backend/internal/threads/source_identity_test.go create mode 100644 backend/internal/threads/sources.go diff --git a/AGENTS.md b/AGENTS.md index f8273911..94298d43 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -60,8 +60,9 @@ Rules, not description. Code is truth — implementation is discoverable, so it ### Threads - **Thread is a record.** Follow-up adds an answer, never rewrites one. Correction is a new question. - **One language per thread** — its first question's. Everything a person reads follows it, replays included. The record decides, not the request. -- **Answer prompt gets the previous QUESTION only**, never the previous answer — prose beside sources it was NOT written from is how a claim acquires a citation it was never read from. One turn back, never the thread. -- **Rework ("summarize", "as a table") answers from the previous answer and ITS OWN sources, no search.** First turn is never a rework. A basis missing even ONE chunk is refused — never summarised from survivors, never searched afresh; a fresh answer to "summarize" is a different answer dressed as a summary. Not "translate". +- **Rework ("summarize", "as a table", "zeichne ein Diagramm davon") answers from the previous answer and ITS OWN sources, no search.** First turn is never a rework. A basis missing even ONE source is refused — never summarised from survivors, never searched afresh; a fresh answer to "summarize" is a different answer dressed as a summary. Not "translate". +- **A turn's basis is recorded by repo, path, commit, lines — re-read from git, never by chunk row.** A poll re-inserts every chunk of a touched file under new ids; keyed on them, a minute-old answer's diagram was refused as "no longer indexed". Missing means git cannot produce it: purged repo, replaced snapshot, file now excluded. +- **Previous answer reaches the answer prompt as context, never a source** — markers stripped, fenced off, never cited, its claims never restated as fact. "The flow" is the flow it described. Changes and release lanes get the question only. - **Recall reaches the model as ONE user message**, never a user/assistant pair — a prose assistant turn makes the model continue the conversation instead of returning JSON. - **Thread is a funnel: narrows, never widens.** Pin is a ceiling; "in all repos" under a pin is not honoured. A named repo the pin excludes is reported as outside — silent refusal to widen is the same quiet drop the rule exists to stop. - **Empty repo restriction means the whole corpus** — so a pin or chosen repo that no longer exists must FAIL the turn, never search. Has bitten twice. @@ -69,6 +70,7 @@ Rules, not description. Code is truth — implementation is discoverable, so it ### Memory - **Standing instruction kept the turn it is said, in English, per reader.** About the READER only — never a claim about code, never language, audience or repo pin, never a one-off. +- **Kept only on the reader's word, checked in Go** — the gate quotes it (`memory_marker`), a quote the question lacks is dropped (`standingOnly`). The model's judgement alone filed "zeichne ein diagramm des ablaufs" as "Draw a diagram for the answer." and every later answer drew one. Forgetting is not gated. Same text twice is one rule. - **The block outranks the prompt**: overrides form, length, diagrams, what to mention. Never citing, inventing, "nothing found", language, audience. Empty memory leaves the prompt byte-identical — that is the eval baseline. Contradiction REPLACES, a rule never expires, one past the cap is refused, never fitted in by dropping one. Never on a shared page. ### Routing diff --git a/backend/cmd/rongo/main.go b/backend/cmd/rongo/main.go index 7c01fff9..ec211ee8 100644 --- a/backend/cmd/rongo/main.go +++ b/backend/cmd/rongo/main.go @@ -389,12 +389,14 @@ func main() { // The viewer and the answer pipeline read files through the same service: // the viewer shows a citation, the pipeline reads a process model whose - // nodes were cited, both at the indexed commit under the same rules. + // nodes were cited, both at the indexed commit under the same rules. A + // thread reads its answers' sources back through it too, at the commit + // each was read at, so a re-index since the answer changes nothing. source := sourceview.New(db, gitClient, cfg.IndexMaxFileBytes).WithCommits(gitClient) deps := httpapi.Deps{ Auth: authSvc, Repos: repostatus.New(db, moduleOpts(cfg)), - Threads: threads.NewStore(db), + Threads: threads.NewStore(db).WithEvidence(source), Source: source, Commit: source, OIDCAdminGroup: cfg.OIDCAdminGroup, diff --git a/backend/internal/ask/answer.go b/backend/internal/ask/answer.go index 3469d32b..eb1f5f83 100644 --- a/backend/internal/ask/answer.go +++ b/backend/internal/ask/answer.go @@ -135,8 +135,8 @@ type Answer struct { // PromptParts is the answer prompt by section, in estimated tokens. System is // every rule the audience, language and scope assembled — and the thread's -// previous question with it, because a follow-up is written into the rules -// (answerFollowUp) rather than into the message the reader typed. Sources is +// previous question and answer with it, because a follow-up is written into +// the rules (answerFollowUp) rather than into the message the reader typed. Sources is // the code in front of the model, headers and separators included. Question // is what was asked, and only that. type PromptParts struct { @@ -370,16 +370,21 @@ alone and that a new thread can answer across the whole corpus, then answer for them. Make no claim of any kind about any other repository - not a guess, not a comparison, not "presumably".` +// FollowUp is the turn a follow-up continues: the previous question and the +// answer it got. Zero on a first turn, and then the prompt carries nothing +// about a follow-up at all. +type FollowUp struct { + Question string + Answer string +} + +// followUpOf is what a follow-up of t carries into the answer prompt. +func followUpOf(t Thread) FollowUp { + return FollowUp{Question: t.Question, Answer: t.Answer} +} + // answerFollowUp is added when the turn continues a thread that already // answered something. Its one format argument is the PREVIOUS QUESTION. -// -// The previous answer's text is not here and must not be: the sources are what -// a claim rests on, and a model handed its own earlier prose alongside sources -// it was NOT written from ends up restating it and citing the new sources for -// it. The question is enough for both things this rule is for — telling the -// model what a pronoun points at, and telling it not to write the same answer -// again with a picture on top. A rework is the exception, and it has its own -// block: answerRework. const answerFollowUp = ` This is a follow-up to an earlier question in the same thread: %s. Answer the @@ -387,6 +392,28 @@ NEW question. Where it points at something without naming it ("that", "this", "it"), it points at the subject of that earlier question. Do not restate what was already explained - the reader has it directly above.` +// answerFollowUpAnswer follows answerFollowUp when the previous turn +// answered. Its one format argument is that answer, markers stripped. +// +// "The flow" in a follow-up is the flow the previous answer described, and +// the question alone cannot say which that was. The answer is prose the +// model wrote itself, not something read from code, so it goes in as +// context and never as a source: handed back beside sources it was NOT +// written from, a claim of it would otherwise be restated and cited to a +// source that never said it. +const answerFollowUpAnswer = ` + +Your previous answer, which the reader has directly above, was: + +<<< +%s +>>> + +It is context, never a source: it tells you what the new question refers to. +Never cite it and never restate its claims as fact. Every claim you make rests +on the numbered sources below. Where they say something it did not, or +contradict it, the sources win.` + // answerRework replaces answerFollowUp on a turn that asks for the previous // answer in another form. Its one format argument is the reader's // instruction. The previous text stands in the user message beside the @@ -1205,11 +1232,10 @@ const structureIsConfiguration = "\nThis is configuration, not code. It says whi // the model. A model handed only a question and a system prompt answers it // fluently from its own training, and that answer would be about some other // codebase — the single most expensive failure this product can produce. -// followingUp is the question this thread asked last, empty when there is -// none. Only the question: see answerFollowUp for why the previous answer's -// text stays out of here. +// followingUp is the turn this one continues, zero when there is none; its +// answer goes in as context, never as a source (answerFollowUpAnswer). func (a *Answerer) Answer(ctx context.Context, question string, audience Audience, lang Language, - sources []Source, scope Scope, followingUp string, onToken func(string)) (Answer, error) { + sources []Source, scope Scope, followingUp FollowUp, onToken func(string)) (Answer, error) { if len(sources) == 0 { return Answer{Text: NothingFound(lang, nil)}, nil @@ -1237,8 +1263,8 @@ func (a *Answerer) Answer(ctx context.Context, question string, audience Audienc // Rework writes the previous answer again in the form the instruction asks // for, from that answer's own sources. The previous text goes into the user // message beside them — the one place in the product where prose of the -// model's own reaches the answering prompt, and it is safe here because the -// sources next to it are the ones it was written from. +// model's own is material to rework rather than context, and it is safe here +// because the sources next to it are the ones it was written from. // // No follow-up rule: "do not restate what was already explained" would forbid // the one thing this call exists to do, the same as Reexplain. @@ -1249,7 +1275,7 @@ func (a *Answerer) Rework(ctx context.Context, instruction string, audience Audi return Answer{}, fmt.Errorf("rework: no sources to rework from") } rules := memory.Applying(memory.From(ctx).Rows(), scope.Known) - system := systemPrompt(audience, lang, t.Sources, scope, "", fmt.Sprintf(answerRework, instruction), memory.Block(rules, scope.Known)) + system := systemPrompt(audience, lang, t.Sources, scope, FollowUp{}, fmt.Sprintf(answerRework, instruction), memory.Block(rules, scope.Known)) user := renderRework(instruction, t, scope.Stages) // The sources are measured on their own here: the user message also // carries the previous turn, which is neither the question nor the code @@ -1267,11 +1293,11 @@ func (a *Answerer) Rework(ctx context.Context, instruction string, audience Audi } // systemPrompt assembles the answering rules for one turn. followingUp is -// the previous question of an ordinary follow-up, rework the rendered rework +// the previous turn of an ordinary follow-up, rework the rendered rework // block; at most one of them is set. memories is the reader's standing // instructions as memory.Block rendered them, empty for a reader with none, // and then the prompt is byte for byte what it was before memory existed. -func systemPrompt(audience Audience, lang Language, sources []Source, scope Scope, followingUp, rework, memories string) string { +func systemPrompt(audience Audience, lang Language, sources []Source, scope Scope, followingUp FollowUp, rework, memories string) string { name := languageName(lang) system := fmt.Sprintf(answerCommon, name) if audience == AudienceDev { @@ -1350,8 +1376,11 @@ func systemPrompt(audience Audience, lang Language, sources []Source, scope Scop if scope.AllDenied && len(scope.Known) > 0 { system += fmt.Sprintf(answerAllDenied, strings.Join(scope.Known, ", ")) } - if followingUp != "" { - system += fmt.Sprintf(answerFollowUp, followingUp) + if followingUp.Question != "" { + system += fmt.Sprintf(answerFollowUp, followingUp.Question) + if followingUp.Answer != "" { + system += fmt.Sprintf(answerFollowUpAnswer, strings.TrimSpace(stripMarkersOutsideFences(followingUp.Answer))) + } } system += rework // Computed from the sources rather than read off the scope, so this block diff --git a/backend/internal/ask/answer_retry_test.go b/backend/internal/ask/answer_retry_test.go index 2f8b9d0f..3d51940f 100644 --- a/backend/internal/ask/answer_retry_test.go +++ b/backend/internal/ask/answer_retry_test.go @@ -48,7 +48,7 @@ func breakAfter(t *testing.T, tokens ...string) (*llm.Client, *atomic.Int32) { func TestAnswer_aRetriedAnswerReportsTwoAttemptsInItsDetail(t *testing.T) { c, calls := failThenStream(t, "Stored in ", "store.go [1].") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -71,7 +71,7 @@ func TestAnswer_aRetriedAnswerReportsTwoAttemptsInItsDetail(t *testing.T) { func TestAnswer_aStreamThatBrokeAfterTextFailsTheTurn(t *testing.T) { c, calls := breakAfter(t, "Stored [2] and ", "then") - _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil) + _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err == nil { t.Fatal("err = nil, want the turn to fail rather than store a fragment") @@ -86,7 +86,7 @@ func TestAnswer_aStreamThatBrokeAfterTextFailsTheTurn(t *testing.T) { func TestAnswer_anOrdinaryTurnSaysNothingAboutAttempts(t *testing.T) { c, _, _ := streamUpstream(t, "Stored in store.go [1].") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } diff --git a/backend/internal/ask/answer_stages_test.go b/backend/internal/ask/answer_stages_test.go index 0084bc54..22e1d57e 100644 --- a/backend/internal/ask/answer_stages_test.go +++ b/backend/internal/ask/answer_stages_test.go @@ -25,7 +25,7 @@ func stagedSources() []Source { func TestAnswer_stagedSourcesAreLabelledAndTheRuleSaysReportEveryStage(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") scope := Scope{Known: []string{"acme-service", "acme-infra"}, Stages: declaredStages} - if _, err := NewAnswerer(c).Answer(context.Background(), "How often?", AudienceBA, LanguageEN, stagedSources(), scope, "", nil); err != nil { + if _, err := NewAnswerer(c).Answer(context.Background(), "How often?", AudienceBA, LanguageEN, stagedSources(), scope, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } p := *prompt @@ -51,7 +51,7 @@ func TestAnswer_anAskedStageRulesOutTheOthers(t *testing.T) { scope := Scope{Known: []string{"acme-service", "acme-infra"}, Stages: declaredStages, Stage: "prod"} sources := stagedSources() sources = append(sources[:2], sources[3]) - if _, err := NewAnswerer(c).Answer(context.Background(), "How often in production?", AudienceBA, LanguageEN, sources, scope, "", nil); err != nil { + if _, err := NewAnswerer(c).Answer(context.Background(), "How often in production?", AudienceBA, LanguageEN, sources, scope, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } p := *prompt @@ -66,7 +66,7 @@ func TestAnswer_anAskedStageRulesOutTheOthers(t *testing.T) { func TestAnswer_noStagedSourceMeansNoStageRule(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") scope := Scope{Known: []string{"peeq"}, Stages: declaredStages} - if _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), scope, "", nil); err != nil { + if _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), scope, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if strings.Contains(*prompt, "deployed configuration") || strings.Contains(*prompt, "(stage ") { @@ -76,7 +76,7 @@ func TestAnswer_noStagedSourceMeansNoStageRule(t *testing.T) { func TestAnswer_redactedMarkerIsExplainedToTheModel(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil); err != nil { + if _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if !strings.Contains(*prompt, "") { diff --git a/backend/internal/ask/answer_test.go b/backend/internal/ask/answer_test.go index 384e884f..8d402239 100644 --- a/backend/internal/ask/answer_test.go +++ b/backend/internal/ask/answer_test.go @@ -94,7 +94,7 @@ func TestAnswer_streamsAndResolvesTheMarkersItUsed(t *testing.T) { var seen []string // When - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", collect(&seen)) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, collect(&seen)) if err != nil { t.Fatalf("Answer: %v", err) } @@ -121,7 +121,7 @@ func TestAnswer_aMarkerWithNoSourceIsDroppedNotInvented(t *testing.T) { // under an answer — the failure this product can least afford. c, _, _ := streamUpstream(t, "This happens in delivery [7].") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -138,7 +138,7 @@ func TestAnswer_aGroupedMarkerCountsForEachNumberInIt(t *testing.T) { // leads nowhere. c, _, _ := streamUpstream(t, "Compared on poll [1, 2], and again [2,1].") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -151,7 +151,7 @@ func TestAnswer_aGroupedMarkerCountsForEachNumberInIt(t *testing.T) { func TestAnswer_anInventedNumberInsideAGroupIsDroppedAlone(t *testing.T) { c, _, _ := streamUpstream(t, "Compared on poll [1, 9].") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -168,7 +168,7 @@ func TestAnswer_anIndexExpressionInCodeIsNotACitation(t *testing.T) { c, _, _ := streamUpstream(t, "The call is in store.go [2]:\n\n```go\nname := args[1]\nvalue := parts[1]\n```\n") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -189,7 +189,7 @@ func TestAnswer_markersAreRenumberedInOrderOfFirstAppearance(t *testing.T) { c, _, _ := streamUpstream(t, "Issued in grant.go [", "2", "], stored [1] and again [2].") var seen []string - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", collect(&seen)) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, collect(&seen)) if err != nil { t.Fatalf("Answer: %v", err) } @@ -211,7 +211,7 @@ func TestAnswer_markersAreRenumberedInOrderOfFirstAppearance(t *testing.T) { func TestAnswer_aGroupedMarkerIsRenumberedPerNumber(t *testing.T) { c, _, _ := streamUpstream(t, "Compared on poll [2, 1] and [9, 2].") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -232,7 +232,7 @@ func TestAnswer_aMarkerInsideInlineCodeIsNotRenumbered(t *testing.T) { // closing backtick says it is code. c, _, _ := streamUpstream(t, "Use `args[", "2]` as in grant.go [2].") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -250,7 +250,7 @@ func TestAnswer_anUnclosedBacktickIsProseAtTheEnd(t *testing.T) { // a stray backtick with a marker after it is prose, and the marker counts. c, _, _ := streamUpstream(t, "A stray ` and then [2]") - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -267,7 +267,7 @@ func TestAnswer_aCutAnswerIsStillRenumberedAndFlushed(t *testing.T) { c, _, _ := streamUpstreamEnding(t, "length", []string{"Stored [2] and then [", "1"}) var seen []string - got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", collect(&seen)) + got, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, collect(&seen)) if err != nil { t.Fatalf("Answer: %v", err) } @@ -286,7 +286,7 @@ func TestAnswer_withoutSourcesItSaysSoAndNeverCallsTheModel(t *testing.T) { // built from nothing but the question and the system prompt. c, _, calls := streamUpstream(t, "I suspect that ...") - got, err := NewAnswerer(c).Answer(context.Background(), "How does shipping work?", AudienceBA, LanguageEN, nil, Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "How does shipping work?", AudienceBA, LanguageEN, nil, Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -306,11 +306,11 @@ func TestAnswer_theAudienceReachesThePrompt(t *testing.T) { // The role changes only this step: language level, depth, whether code is // embedded. A prompt that ignored it would make the BA/DEV switch decorative. cBA, promptBA, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(cBA).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil); err != nil { + if _, err := NewAnswerer(cBA).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } cDev, promptDev, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(cDev).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, "", nil); err != nil { + if _, err := NewAnswerer(cDev).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } @@ -434,7 +434,7 @@ func TestAnswer_theIntentSharpensTheOpeningSentence(t *testing.T) { } { c, prompt, _ := streamUpstream(t, "x") if _, err := NewAnswerer(c).Answer(context.Background(), "Wo?", AudienceBA, LanguageEN, twoSources(), - Scope{Intent: intent}, "", nil); err != nil { + Scope{Intent: intent}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if !strings.Contains(*prompt, want) { @@ -457,13 +457,13 @@ func TestAnswer_theIntentSharpensTheOpeningSentence(t *testing.T) { func TestAnswer_anIntentWithNoRuleAddsNothing(t *testing.T) { base, basePrompt, _ := streamUpstream(t, "x") if _, err := NewAnswerer(base).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), - Scope{}, "", nil); err != nil { + Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } for _, intent := range []string{"how", "sideways"} { c, prompt, _ := streamUpstream(t, "x") if _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), - Scope{Intent: intent}, "", nil); err != nil { + Scope{Intent: intent}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if *prompt != *basePrompt { @@ -479,7 +479,7 @@ func TestAnswer_anEmptyCompletionIsAnErrorNotAnAnswer(t *testing.T) { c, _, _ := streamUpstream(t) a := NewAnswerer(c) - _, err := a.Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, "", nil) + _, err := a.Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err == nil { t.Fatal("Answer: nil error on an empty completion") @@ -496,7 +496,7 @@ func TestAnswer_aCutAnswerKeepsWhatTheReaderAlreadySaw(t *testing.T) { c, _, _ := streamUpstreamEnding(t, "length", []string{"The grant ", "is created in store.go [1]."}) a := NewAnswerer(c) - got, err := a.Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, "", nil) + got, err := a.Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v, want the partial text kept", err) @@ -509,7 +509,7 @@ func TestAnswer_aCutAnswerKeepsWhatTheReaderAlreadySaw(t *testing.T) { func TestAnswer_aCutAnswerWithNoTextIsStillAnError(t *testing.T) { c, _, _ := streamUpstreamEnding(t, "length", nil) - _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, "", nil) + _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil) if err == nil || !strings.Contains(err.Error(), "length") { t.Fatalf("err = %v, want the length failure surfaced", err) @@ -526,11 +526,11 @@ func TestAnswer_theCorpusWideMarkerRuleReachesBothPrompts(t *testing.T) { // Its absence half has to be the strict one: three chips under a refusal // open three files that say nothing about what was asked. cBA, promptBA, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(cBA).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil); err != nil { + if _, err := NewAnswerer(cBA).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } cDev, promptDev, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(cDev).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, "", nil); err != nil { + if _, err := NewAnswerer(cDev).Answer(context.Background(), "How?", AudienceDev, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } @@ -570,7 +570,7 @@ func TestAnswer_aRefusalThatEnumeratesEveryMarkerIsStillResolved(t *testing.T) { } c, _, _ := streamUpstream(t, "There is no information about shares here", run.String(), ".") - got, err := NewAnswerer(c).Answer(context.Background(), "Should I buy shares?", AudienceBA, LanguageEN, sources, Scope{}, "", nil) + got, err := NewAnswerer(c).Answer(context.Background(), "Should I buy shares?", AudienceBA, LanguageEN, sources, Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } diff --git a/backend/internal/ask/answer_testlabel_test.go b/backend/internal/ask/answer_testlabel_test.go index f592c76b..f88a6ba6 100644 --- a/backend/internal/ask/answer_testlabel_test.go +++ b/backend/internal/ask/answer_testlabel_test.go @@ -18,7 +18,7 @@ func TestAnswer_testSourcesAreLabelledAndTheRuleSaysCiteTheMechanism(t *testing. {ChunkID: 2, Repo: "llmwire", Branch: "master", Path: "registry_test.go", Symbol: "TestNewRegistry_Rejects", StartLine: 426, EndLine: 450, Text: "func TestNewRegistry_Rejects(t *testing.T) {", Reason: "hit"}, } - if _, err := NewAnswerer(c).Answer(context.Background(), "How are profiles loaded?", AudienceBA, LanguageEN, sources, Scope{Known: []string{"llmwire"}}, "", nil); err != nil { + if _, err := NewAnswerer(c).Answer(context.Background(), "How are profiles loaded?", AudienceBA, LanguageEN, sources, Scope{Known: []string{"llmwire"}}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } p := *prompt diff --git a/backend/internal/ask/changes.go b/backend/internal/ask/changes.go index acb8e9e2..3b77feb8 100644 --- a/backend/internal/ask/changes.go +++ b/backend/internal/ask/changes.go @@ -61,7 +61,7 @@ const IntentChanges = "changes" // changed wants the list, not a question back. Sources are commits, cited // like files and stored like them. func (p *Pipeline) answerChanges(ctx context.Context, question string, audience Audience, lang Language, - u Understanding, scope Scope, followingUp string, ev Events) (Answer, error) { + u Understanding, scope Scope, followingUp FollowUp, ev Events) (Answer, error) { scope.SinceDays = clampSinceDays(int(u.SinceDays)) scope.Topic = strings.TrimSpace(u.Topic) diff --git a/backend/internal/ask/docsonly_test.go b/backend/internal/ask/docsonly_test.go index e1849b52..d2d15637 100644 --- a/backend/internal/ask/docsonly_test.go +++ b/backend/internal/ask/docsonly_test.go @@ -92,7 +92,7 @@ func TestAnswerPromptCarriesTheDocumentationOnlyRule(t *testing.T) { // does say it, and may have said it for a year while the code moved. c, prompt, _ := streamUpstream(t, "x") _, err := NewAnswerer(c).Answer(context.Background(), "How are the models chosen?", AudienceBA, LanguageEN, - docSources(), Scope{}, "", nil) + docSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -107,7 +107,7 @@ func TestAnswerPromptOmitsTheRuleWhenCodeIsPresent(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") mixed := append(docSources(), twoSources()[0]) _, err := NewAnswerer(c).Answer(context.Background(), "How are the models chosen?", AudienceBA, LanguageEN, - mixed, Scope{}, "", nil) + mixed, Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } diff --git a/backend/internal/ask/language_everywhere_test.go b/backend/internal/ask/language_everywhere_test.go index a03eaa1c..0e13a5ca 100644 --- a/backend/internal/ask/language_everywhere_test.go +++ b/backend/internal/ask/language_everywhere_test.go @@ -80,7 +80,7 @@ func TestAnswer_theLanguageIsSaidLastAsWell(t *testing.T) { // that has just read two thousand tokens of English tends to answer in // it. The closing line is what keeps a German answer German. c, prompt, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(c).Answer(context.Background(), "Wie?", AudienceBA, LanguageDE, twoSources(), Scope{}, "", nil); err != nil { + if _, err := NewAnswerer(c).Answer(context.Background(), "Wie?", AudienceBA, LanguageDE, twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if strings.Count(*prompt, "German") < 2 { @@ -96,7 +96,7 @@ func TestAnswer_theLanguageIsSaidLastAsWell(t *testing.T) { func TestAnswer_germanIsSpelledTheSwissWay(t *testing.T) { c, prompt, _ := streamUpstream(t, "Der Gesch", "aeftsprozess ist gr", "ößer [1].") var seen []string - a, err := NewAnswerer(c).Answer(context.Background(), "Wie?", AudienceBA, LanguageDE, twoSources(), Scope{}, "", collect(&seen)) + a, err := NewAnswerer(c).Answer(context.Background(), "Wie?", AudienceBA, LanguageDE, twoSources(), Scope{}, FollowUp{}, collect(&seen)) if err != nil { t.Fatalf("Answer: %v", err) } @@ -115,7 +115,7 @@ func TestAnswer_germanIsSpelledTheSwissWay(t *testing.T) { // An Italian answer is what the model wrote. c, _, _ = streamUpstream(t, "Il Gesch", "aeftsprozess è größer.") - a, err = NewAnswerer(c).Answer(context.Background(), "Come?", AudienceBA, LanguageIT, twoSources(), Scope{}, "", nil) + a, err = NewAnswerer(c).Answer(context.Background(), "Come?", AudienceBA, LanguageIT, twoSources(), Scope{}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } diff --git a/backend/internal/ask/language_test.go b/backend/internal/ask/language_test.go index 27aaa2c8..07ad2089 100644 --- a/backend/internal/ask/language_test.go +++ b/backend/internal/ask/language_test.go @@ -11,11 +11,11 @@ func TestAnswer_theLanguageReachesThePrompt(t *testing.T) { // make the selector decorative. An unknown value falls back to English // rather than failing the turn. cDE, promptDE, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(cDE).Answer(context.Background(), "How?", AudienceBA, LanguageDE, twoSources(), Scope{}, "", nil); err != nil { + if _, err := NewAnswerer(cDE).Answer(context.Background(), "How?", AudienceBA, LanguageDE, twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } cXX, promptXX, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(cXX).Answer(context.Background(), "How?", AudienceBA, Language("xx"), twoSources(), Scope{}, "", nil); err != nil { + if _, err := NewAnswerer(cXX).Answer(context.Background(), "How?", AudienceBA, Language("xx"), twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } diff --git a/backend/internal/ask/memory_test.go b/backend/internal/ask/memory_test.go index aebafd1d..02e15c23 100644 --- a/backend/internal/ask/memory_test.go +++ b/backend/internal/ask/memory_test.go @@ -16,7 +16,7 @@ import ( ) const flowchartDirective = `{"intent":"memory","terms":[],"code_terms":[],"repos":[], - "memory":"Never draw flowchart diagrams.","memory_scope":"","memory_replaces":["3"],"memory_removes":[]}` + "memory":"Never draw flowchart diagrams.","memory_marker":"nie wieder","memory_scope":"","memory_replaces":["3"],"memory_removes":[]}` func withMemory(rows ...memory.Row) context.Context { return memory.With(context.Background(), memory.NewHolder(rows)) @@ -158,7 +158,7 @@ func TestPipeline_aDirectiveAloneIsRememberedAndAnsweredWithoutAModel(t *testing func TestPipeline_aDirectiveBesideAQuestionIsAppliedToThatSameAnswer(t *testing.T) { db := gatherDB(t) hitID := seedChunk(t, db, "a.go", 0, 1, 10, "f", "func f() {}") - reply := `{"intent":"how","terms":["t"],"code_terms":["f"],"repos":[],"memory":"Never draw flowchart diagrams."}` + reply := `{"intent":"how","terms":["t"],"code_terms":["f"],"repos":[],"memory":"Never draw flowchart diagrams.","memory_marker":"never"}` c, streams, system := memoryUpstream(t, reply, "So [1].") p := NewPipeline(c, &fakeSearch{hits: []retrieve.Hit{hitFor(t, db, hitID)}}, NewGatherer(db, GatherOptions{MaxHops: 1, TokenBudget: 5000}), &fakeRouter{}) @@ -205,6 +205,96 @@ func TestPipeline_aMemoryIntentWithNothingToKeepRunsAsAQuestion(t *testing.T) { } } +// TestPipeline_aRuleWithoutTheReadersWordForItIsNotKept is the incident: +// "zeichne ein diagramm des ablaufs" asked for THIS answer drawn, and the +// understanding filed it as a standing rule anyway. A rule is kept only on +// the reader's own word that it lasts, quoted from the question — never on +// the model's judgement alone. +func TestPipeline_aRuleWithoutTheReadersWordForItIsNotKept(t *testing.T) { + for name, reply := range map[string]string{ + "no marker": `{"intent":"rework","terms":[],"code_terms":[],"repos":[],"memory":"Draw a diagram for the answer."}`, + "a marker the question does not hold": `{"intent":"rework","terms":[],"code_terms":[],"repos":[], + "memory":"Draw a diagram for the answer.","memory_marker":"ab jetzt"}`, + "a rule alone, invented": `{"intent":"memory","terms":[],"code_terms":[],"repos":[], + "memory":"Draw a diagram for the answer.","memory_marker":""}`, + } { + t.Run(name, func(t *testing.T) { + c, streams, _ := memoryUpstream(t, reply, "So [1].") + p := reworkPipeline(t, c) + ev := Events{OnMemory: func(d memory.Directive) (memory.Added, error) { + t.Fatalf("kept %+v with no word of the reader's saying it lasts", d) + return memory.Added{}, nil + }} + + answer, _, err := p.Run(withMemory(), "zeichne ein diagramm des ablaufs", AudienceBA, LanguageDE, reworkThread(), ev) + if err != nil { + t.Fatalf("Run: %v", err) + } + // What is left is a request for the previous answer drawn. + if answer.Scope.Intent != IntentRework || *streams != 1 { + t.Errorf("intent = %q, streams = %d; want the previous answer reworked", answer.Scope.Intent, *streams) + } + }) + } +} + +// TestPipeline_theReadersWordKeepsTheRuleWhateverItsCase: the marker is +// checked against the question, case folded; "Ab jetzt" is "ab jetzt". +func TestPipeline_theReadersWordKeepsTheRuleWhateverItsCase(t *testing.T) { + reply := `{"intent":"memory","terms":[],"code_terms":[],"repos":[], + "memory":"Draw a diagram in every answer.","memory_marker":"ab jetzt"}` + c, _, _ := memoryUpstream(t, reply) + p := NewPipeline(c, &fakeSearch{}, NewGatherer(gatherDB(t), GatherOptions{MaxHops: 1, TokenBudget: 5000}), &fakeRouter{}) + var got memory.Directive + ev := Events{OnMemory: func(d memory.Directive) (memory.Added, error) { + got = d + return memory.Added{Row: memory.Row{ID: 1, Text: d.Text, ScopeLive: true}}, nil + }} + + if _, _, err := p.Run(withMemory(), "Ab jetzt immer mit Diagramm, bitte.", AudienceBA, LanguageDE, Thread{}, ev); err != nil { + t.Fatalf("Run: %v", err) + } + if got.Text != "Draw a diagram in every answer." { + t.Errorf("kept %+v", got) + } +} + +// TestKeptRule_isWhatTheProductKeeps: the intent eval grades this, so it +// must be the same check Run applies. +func TestKeptRule_isWhatTheProductKeeps(t *testing.T) { + u := Understanding{Memory: "Draw a diagram in every answer.", MemoryMarker: "Ab Jetzt"} + if got := u.KeptRule("ab jetzt immer mit Diagramm"); got != u.Memory { + t.Errorf("kept %q, want the rule", got) + } + if got := u.KeptRule("zeichne ein diagramm des ablaufs"); got != "" { + t.Errorf("kept %q from a question without the word", got) + } + if u.Memory == "" { + t.Error("KeptRule changed the understanding it was asked about") + } +} + +// TestPipeline_forgettingNeedsNoMarker: "show flowcharts again" lasts by +// nature and carries no "from now on"; the gate is on keeping a rule, never +// on dropping one. +func TestPipeline_forgettingNeedsNoMarker(t *testing.T) { + reply := `{"intent":"memory","terms":[],"code_terms":[],"repos":[],"memory":"","memory_removes":[3]}` + c, _, _ := memoryUpstream(t, reply) + p := NewPipeline(c, &fakeSearch{}, NewGatherer(gatherDB(t), GatherOptions{MaxHops: 1, TokenBudget: 5000}), &fakeRouter{}) + var got memory.Directive + ev := Events{OnMemory: func(d memory.Directive) (memory.Added, error) { + got = d + return memory.Added{Removed: []string{"Never draw flowchart diagrams."}}, nil + }} + + if _, _, err := p.Run(withMemory(memory.Row{ID: 3, Text: "Never draw flowchart diagrams."}), "Zeig wieder Flowcharts.", AudienceBA, LanguageDE, Thread{}, ev); err != nil { + t.Fatalf("Run: %v", err) + } + if len(got.Removes) != 1 || got.Removes[0] != 3 { + t.Errorf("directive = %+v, want the rule forgotten", got) + } +} + func TestPipeline_aFullMemoryRefusesTheRuleAndSaysSo(t *testing.T) { db := gatherDB(t) c, _, _ := memoryUpstream(t, flowchartDirective) @@ -243,7 +333,7 @@ func TestPipeline_aFullMemoryRefusesTheRuleAndSaysSo(t *testing.T) { func TestPipeline_aFailedWriteBesideAQuestionIsATraceLineNotAFailedTurn(t *testing.T) { db := gatherDB(t) hitID := seedChunk(t, db, "a.go", 0, 1, 10, "f", "func f() {}") - reply := `{"intent":"how","terms":["t"],"code_terms":["f"],"repos":[],"memory":"Never draw flowchart diagrams."}` + reply := `{"intent":"how","terms":["t"],"code_terms":["f"],"repos":[],"memory":"Never draw flowchart diagrams.","memory_marker":"never"}` c, streams, _ := memoryUpstream(t, reply, "So [1].") p := NewPipeline(c, &fakeSearch{hits: []retrieve.Hit{hitFor(t, db, hitID)}}, NewGatherer(db, GatherOptions{MaxHops: 1, TokenBudget: 5000}), &fakeRouter{}) @@ -283,7 +373,7 @@ func TestMemoryAnswer_aRuleWithQuotesIsNotEscaped(t *testing.T) { func TestAnswer_theReadersRulesCloseThePromptAheadOfTheLanguage(t *testing.T) { c, prompt, _ := streamUpstream(t, "So [1].") rows := []memory.Row{{ID: 1, Text: "Never draw flowchart diagrams.", ScopeLive: true}} - got, err := NewAnswerer(c).Answer(withMemory(rows...), "How?", AudienceBA, LanguageDE, twoSources(), Scope{}, "", func(string) {}) + got, err := NewAnswerer(c).Answer(withMemory(rows...), "How?", AudienceBA, LanguageDE, twoSources(), Scope{}, FollowUp{}, func(string) {}) if err != nil { t.Fatalf("Answer: %v", err) } @@ -301,11 +391,11 @@ func TestAnswer_theReadersRulesCloseThePromptAheadOfTheLanguage(t *testing.T) { func TestAnswer_withoutRulesThePromptIsWhatItWas(t *testing.T) { c, plain, _ := streamUpstream(t, "So [1].") - if _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", func(string) {}); err != nil { + if _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, func(string) {}); err != nil { t.Fatalf("Answer: %v", err) } c, empty, _ := streamUpstream(t, "So [1].") - if _, err := NewAnswerer(c).Answer(withMemory(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, "", func(string) {}); err != nil { + if _, err := NewAnswerer(c).Answer(withMemory(), "How?", AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, func(string) {}); err != nil { t.Fatalf("Answer: %v", err) } if *plain != *empty { @@ -322,7 +412,7 @@ func TestAnswer_aScopedRuleFollowsTheTurnsRepositories(t *testing.T) { // this turn is not about. c, prompt, _ := streamUpstream(t, "So [1].") rows := []memory.Row{{ID: 1, Text: "Skip the tests.", Scope: "shop", ScopeLive: true}} - if _, err := NewAnswerer(c).Answer(withMemory(rows...), "How?", AudienceBA, LanguageEN, twoSources(), Scope{Known: []string{"peeq"}}, "", func(string) {}); err != nil { + if _, err := NewAnswerer(c).Answer(withMemory(rows...), "How?", AudienceBA, LanguageEN, twoSources(), Scope{Known: []string{"peeq"}}, FollowUp{}, func(string) {}); err != nil { t.Fatalf("Answer: %v", err) } if strings.Contains(*prompt, "Skip the tests") { diff --git a/backend/internal/ask/pipeline.go b/backend/internal/ask/pipeline.go index f6060f27..7fdf260f 100644 --- a/backend/internal/ask/pipeline.go +++ b/backend/internal/ask/pipeline.go @@ -187,20 +187,19 @@ func NewPipeline(c *llm.Client, s Searcher, g *Gatherer, r Routes) *Pipeline { // after the question has been understood, and the reason a follow-up is // never asked which repository was meant. // - Question and Answer are WHAT the turn is about. They reach the -// understanding step and nothing else, so that "show me that as a diagram" -// resolves to the subject the reader is following up on instead of being -// searched for as the word "diagram". +// understanding step, so that "show me that as a diagram" resolves to the +// subject the reader is following up on instead of being searched for as +// the word "diagram", and the answering prompt, so that "the flow" is the +// flow the previous answer described. // -// Answer is the previous answer's TEXT, and it is deliberately kept out of the -// answering prompt of an ordinary follow-up: sources are the truth, and a -// model handed its own earlier prose beside sources it was NOT written from -// ends up citing them for it. Only the previous QUESTION goes there, as the -// thing a pronoun points at. +// Answer is the previous answer's TEXT. In an ordinary follow-up it reaches +// the answering prompt as context and never as a source (answerFollowUpAnswer): +// sources are the truth, and a model handed its own earlier prose as +// something to cite ends up citing new sources for claims they never made. // -// The one turn that does read the answer is a rework ("summarize", "as a -// table"): the answer IS what that turn is about, and it is handed over -// together with Sources, the material it was written from, so every claim in -// the reworked text still has its source in front of the model. +// A rework ("summarize", "as a table") is about the answer itself: it is +// handed over together with Sources, the material it was written from, so +// every claim in the reworked text still has its source in front of the model. type Thread struct { // Pin is the repositories the thread has already narrowed to. Pin []string @@ -208,10 +207,11 @@ type Thread struct { Question string // Answer is the answer that question got. Answer string - // Sources is what Answer was written from, as the record resolves them - // now, and SourcesTotal is how many the record holds. A re-index between - // the turns drops chunks from Sources and not from SourcesTotal, which - // is how a rework tells a whole basis from a partial one. + // Sources is what Answer was written from, re-read from git at the + // commit each was read at, and SourcesTotal is how many the record + // holds. A source git can no longer produce — a purged repository, a + // replaced snapshot — is missing from Sources and not from SourcesTotal, + // which is how a rework tells a whole basis from a partial one. Sources []Source SourcesTotal int } @@ -235,6 +235,14 @@ func (p *Pipeline) Run(ctx context.Context, question string, audience Audience, if err != nil { return Answer{}, nil, err } + // A rule the reader gave no word for is not kept. A turn that was ONLY + // that rule asked for something about the answer and nothing of the + // code: the previous answer in another form, which is a rework — and + // the guard below makes it an ordinary question on a first turn. + if u.standingOnly(question) && u.Intent == IntentMemory && u.Directive().Empty() { + slog.Info("instruction not kept: no word of the reader's says it lasts", "thread", llm.ThreadID(ctx)) + u.Intent = IntentRework + } // A rework the guard refuses is an ordinary question from here on, in // the record too: the intent rides the scope onto the row, and a // re-explain of that row keys on it to rework again. @@ -340,13 +348,13 @@ func (p *Pipeline) Run(ctx context.Context, question string, audience Audience, // the fused search nor the routing ladder has anything to say about a // date window. if p.isChanges(u) { - answer, err := p.answerChanges(ctx, question, audience, lang, u, scope, t.Question, ev) + answer, err := p.answerChanges(ctx, question, audience, lang, u, scope, FollowUp{Question: t.Question}, ev) return answer, nil, err } // A release question leaves here for the same reason: its sources are // the commits between two deployed versions. if p.isRelease(u) { - answer, err := p.answerRelease(ctx, question, audience, lang, u, scope, t.Question, ev) + answer, err := p.answerRelease(ctx, question, audience, lang, u, scope, FollowUp{Question: t.Question}, ev) return answer, nil, err } // A rework leaves here too: the previous answer and its own sources are @@ -399,7 +407,7 @@ func (p *Pipeline) Run(ctx context.Context, question string, audience Audience, // is what the reader asked this turn, and the thread's older question is // search material they did not type here. Naming it would say the turn // went looking for something they asked a turn ago. - answer, err := p.gatherAndAnswer(ctx, question, audience, lang, hits, scope, withoutPrior(texts, u.Prior), t.Question, ev) + answer, err := p.gatherAndAnswer(ctx, question, audience, lang, hits, scope, withoutPrior(texts, u.Prior), followUpOf(t), ev) return answer, nil, err } @@ -851,7 +859,7 @@ func withCensusDetail(d map[string]any, c Census) map[string]any { // terms are the search terms for the "nothing found" answer; a resume has none // to report, having searched nothing. func (p *Pipeline) gatherAndAnswer(ctx context.Context, question string, audience Audience, lang Language, - hits []retrieve.Hit, scope Scope, terms []string, followingUp string, ev Events) (Answer, error) { + hits []retrieve.Hit, scope Scope, terms []string, followingUp FollowUp, ev Events) (Answer, error) { scope = p.describeProjects(ctx, scope) @@ -879,7 +887,7 @@ func (p *Pipeline) gatherAndAnswer(ctx context.Context, question string, audienc // step's detail attached once the stream has closed: what it cost, and how // many of the sources in front of the model it actually cited. func (p *Pipeline) answer(ctx context.Context, question string, audience Audience, lang Language, - sources []Source, scope Scope, followingUp string, ev Events) (Answer, error) { + sources []Source, scope Scope, followingUp FollowUp, ev Events) (Answer, error) { ev.status("answering") answer, err := p.answerer.Answer(ctx, question, audience, lang, sources, scope, followingUp, ev.tokens()) answer.Scope = scope @@ -1191,15 +1199,15 @@ func (p *Pipeline) searchScoped(ctx context.Context, question, prior string, tex // turn that went through a card is still a turn of the thread, and the reader // who typed "und wo wird das entschieden?" gets it answered by a clarification // and then by an answer: without the thread the answer prompt loses the rule -// that says what "das" points at. Only t.Question reaches the prompt — see -// answerFollowUp for why the previous answer's text does not. +// that says what "das" points at, and the previous answer beside it as +// context (answerFollowUpAnswer). func (p *Pipeline) Resume(ctx context.Context, question string, audience Audience, lang Language, hits []retrieve.Hit, scope Scope, t Thread, ev Events) (Answer, error) { // Marked as resumed so the locate loop stays out of it: this path replays // the candidate's stored hits and searches for nothing more. scope.Resumed = true - return p.gatherAndAnswer(ctx, question, audience, lang, hits, scope, nil, t.Question, ev) + return p.gatherAndAnswer(ctx, question, audience, lang, hits, scope, nil, followUpOf(t), ev) } // ResumeRepo continues a turn after the reader chose a REPOSITORY off a @@ -1280,7 +1288,7 @@ func (p *Pipeline) ResumeRepo(ctx context.Context, question string, u Understand // documentation-only footing come out of a resumed turn the way they come // out of any other: a copy of it here once left both off exactly the // turns a repository card sent the reader into. - return p.gatherAndAnswer(ctx, question, audience, lang, hits, scope, withoutPrior(texts, u.Prior), t.Question, ev) + return p.gatherAndAnswer(ctx, question, audience, lang, hits, scope, withoutPrior(texts, u.Prior), followUpOf(t), ev) } // Reexplain answers the same question for the other audience from sources a @@ -1315,5 +1323,22 @@ func (p *Pipeline) Reexplain(ctx context.Context, question string, audience Audi // again for the other audience, and the first answer is right above it in // the thread. Telling the model not to restate what was already explained // would forbid the one thing this path exists to do. - return p.answer(ctx, question, audience, lang, sources, scope, "", ev) + return p.answer(ctx, question, audience, lang, sources, scope, FollowUp{}, ev.readAt(sources)) +} + +// readAt adds to the writing step where a basis re-read from the record came +// from, for the two turns that answer from one: rework and re-explain. +func (e Events) readAt(sources []Source) Events { + at := readAt(sources) + if e.OnDetail == nil || len(at) == 0 { + return e + } + next := e.OnDetail + e.OnDetail = func(step string, d map[string]any) { + if step == "writing" { + d["read_at"] = at + } + next(step, d) + } + return e } diff --git a/backend/internal/ask/pipeline_test.go b/backend/internal/ask/pipeline_test.go index 5984533d..b47e0804 100644 --- a/backend/internal/ask/pipeline_test.go +++ b/backend/internal/ask/pipeline_test.go @@ -738,7 +738,7 @@ func TestTheAnswerPromptForbidsInventingTheRestOfTheCorpus(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") sc := Scope{Known: []string{"rongo"}, AllDenied: true} if _, err := NewAnswerer(c).Answer(context.Background(), "vergleiche das mit allen Repositories", - AudienceBA, LanguageEN, twoSources(), sc, "", nil); err != nil { + AudienceBA, LanguageEN, twoSources(), sc, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if !strings.Contains(*prompt, "Only rongo is in front of you") { @@ -758,7 +758,7 @@ func TestTheAnswerPromptForbidsClaimsAboutARepositoryTheThreadLeftOut(t *testing c, prompt, _ := streamUpstream(t, "x") sc := Scope{Known: []string{"rongo"}, Outside: []string{"loom"}} if _, err := NewAnswerer(c).Answer(context.Background(), "und wie macht das loom?", - AudienceBA, LanguageEN, twoSources(), sc, "", nil); err != nil { + AudienceBA, LanguageEN, twoSources(), sc, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if !strings.Contains(*prompt, "this thread does not cover: loom") { @@ -778,7 +778,7 @@ func TestTheLoopsNotFoundNeverOverridesASourceThatAnswers(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") sc := Scope{Located: "Not found in the index."} if _, err := NewAnswerer(c).Answer(context.Background(), "wo wird das gesetzt?", - AudienceDev, LanguageEN, twoSources(), sc, "", nil); err != nil { + AudienceDev, LanguageEN, twoSources(), sc, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if strings.Contains(*prompt, "say so plainly rather than answering") { @@ -806,7 +806,7 @@ func TestTheLoopsOpeningFollowsTheAudience(t *testing.T) { cl, prompt, _ := streamUpstream(t, "x") sc := Scope{Located: "FOUND: ConverterPetRegistry.java:150 setAnzahlhaustiere"} if _, err := NewAnswerer(cl).Answer(context.Background(), "wo?", - c.audience, LanguageEN, twoSources(), sc, "", nil); err != nil { + c.audience, LanguageEN, twoSources(), sc, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if !strings.Contains(*prompt, c.want) { @@ -818,21 +818,30 @@ func TestTheLoopsOpeningFollowsTheAudience(t *testing.T) { } } -// TestTheAnswerPromptCarriesThePreviousQuestionAndNotItsAnswer holds the line -// this whole feature has to stay behind. The previous QUESTION is what a -// pronoun points at, so it goes in. The previous ANSWER is prose the model -// wrote itself, and handing it back alongside real sources is how a claim ends -// up carrying a citation it was never read from. -func TestTheAnswerPromptCarriesThePreviousQuestionAndNotItsAnswer(t *testing.T) { +// TestTheAnswerPromptCarriesThePreviousQuestionAndItsAnswerAsContext: "the +// flow" in a follow-up is the flow the previous answer described, so the +// answer goes in beside the question. It goes in as context, never as a +// source: without its markers, fenced off from the numbered list, under a +// rule that it is not cited and its claims are not restated as fact. +func TestTheAnswerPromptCarriesThePreviousQuestionAndItsAnswerAsContext(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") - if _, err := NewAnswerer(c).Answer(context.Background(), "Kannst du das in einem Diagramm aufzeigen?", + if _, err := NewAnswerer(c).Answer(context.Background(), "Und wo wird der Bypass geprüft?", AudienceBA, LanguageEN, twoSources(), Scope{}, - "Wie unterscheidet sich rongo von reinem RAG?", nil); err != nil { + FollowUp{Question: "Wie funktioniert der Bypass?", Answer: "Der Hinweis wird bei der Registrierung gelesen [1][2]."}, nil); err != nil { t.Fatalf("Answer: %v", err) } - if !strings.Contains(*prompt, "Wie unterscheidet sich rongo von reinem RAG?") { + if !strings.Contains(*prompt, "Wie funktioniert der Bypass?") { t.Errorf("the previous question never reached the prompt:\n%s", *prompt) } + if !strings.Contains(*prompt, "Der Hinweis wird bei der Registrierung gelesen .") { + t.Errorf("the previous answer, markers stripped, never reached the prompt:\n%s", *prompt) + } + if strings.Contains(*prompt, "gelesen [1]") { + t.Errorf("the previous answer kept its markers, which point at another turn's list:\n%s", *prompt) + } + if !strings.Contains(*prompt, "never a source") { + t.Errorf("nothing says the previous answer is not citable:\n%s", *prompt) + } if !strings.Contains(*prompt, "Do not restate what") { t.Errorf("nothing stops the turn from writing the same answer again:\n%s", *prompt) } @@ -843,7 +852,7 @@ func TestTheAnswerPromptCarriesThePreviousQuestionAndNotItsAnswer(t *testing.T) func TestTheAnswerPromptOfAFirstTurnSaysNothingAboutAFollowUp(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") if _, err := NewAnswerer(c).Answer(context.Background(), "How is pricing resolved?", - AudienceBA, LanguageEN, twoSources(), Scope{}, "", nil); err != nil { + AudienceBA, LanguageEN, twoSources(), Scope{}, FollowUp{}, nil); err != nil { t.Fatalf("Answer: %v", err) } if strings.Contains(*prompt, "This is a follow-up") { @@ -892,6 +901,27 @@ func TestReexplainAnswersFromStoredSourcesWithoutSearchingOrGathering(t *testing } } +// TestReexplainSaysWhichCommitItsBasisWasReadAt: like a rework, the basis +// is the record re-read from git, and the writing step says where from. +func TestReexplainSaysWhichCommitItsBasisWasReadAt(t *testing.T) { + p := newTestPipeline(t) + var writing map[string]any + + if _, err := p.Reexplain(context.Background(), "frage", AudienceDev, LanguageEN, + []Source{{ChunkID: 1, Repo: "peeq", Path: "a.go", SHA: "abcdef0123", Text: "package a", StartLine: 1, EndLine: 1}}, Scope{}, + Events{OnDetail: func(step string, d map[string]any) { + if step == "writing" { + writing = d + } + }}); err != nil { + t.Fatalf("reexplain: %v", err) + } + + if got, _ := writing["read_at"].([]string); len(got) != 1 || got[0] != "peeq abcdef0" { + t.Errorf("read_at = %v", writing["read_at"]) + } +} + // TestReexplainRebuildsTheProjectStructure: Scope.Structure is never // persisted, so a re-explain that did not rebuild it would answer the same // question from the same sources with the structure block gone — present when diff --git a/backend/internal/ask/process_test.go b/backend/internal/ask/process_test.go index 4bc60dc6..c71ab9c9 100644 --- a/backend/internal/ask/process_test.go +++ b/backend/internal/ask/process_test.go @@ -119,7 +119,7 @@ func TestTheProcessListingReachesThePromptAndIsNeverCited(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") listing := "Process \"order-intake\" in shop/workflow/order-intake.bpmn:\n- Validate order (serviceTask) -> Charge payment\n" _, err := NewAnswerer(c).Answer(context.Background(), "Walk me through order intake", AudienceBA, LanguageEN, - bothReposSources(), Scope{Known: []string{"peeq", "rongo"}, Processes: listing}, "", nil) + bothReposSources(), Scope{Known: []string{"peeq", "rongo"}, Processes: listing}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -138,7 +138,7 @@ func TestTheProcessListingReachesThePromptAndIsNeverCited(t *testing.T) { // And nothing of it without a listing. c, prompt, _ = streamUpstream(t, "x") _, err = NewAnswerer(c).Answer(context.Background(), "Walk me through order intake", AudienceBA, LanguageEN, - bothReposSources(), Scope{Known: []string{"peeq", "rongo"}}, "", nil) + bothReposSources(), Scope{Known: []string{"peeq", "rongo"}}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } diff --git a/backend/internal/ask/release.go b/backend/internal/ask/release.go index a9bf6726..8fbb1336 100644 --- a/backend/internal/ask/release.go +++ b/backend/internal/ask/release.go @@ -122,7 +122,7 @@ const ( // answerRelease is the turn from the scope on: no search, no routing, no // walk. The two stages and the project ARE the scope. func (p *Pipeline) answerRelease(ctx context.Context, question string, audience Audience, lang Language, - _ Understanding, scope Scope, followingUp string, ev Events) (Answer, error) { + _ Understanding, scope Scope, followingUp FollowUp, ev Events) (Answer, error) { declared := p.stagesOf(ctx, &scope) pair := releasePair(question, declared) diff --git a/backend/internal/ask/rework.go b/backend/internal/ask/rework.go index 149784c2..a874e473 100644 --- a/backend/internal/ask/rework.go +++ b/backend/internal/ask/rework.go @@ -63,7 +63,26 @@ func (p *Pipeline) answerRework(ctx context.Context, instruction string, audienc answer, err := p.answerer.Rework(ctx, instruction, audience, lang, t, scope, ev.tokens()) answer.Scope = scope if err == nil { - ev.detail("writing", writingDetail(answer, len(t.Sources))) + ev.readAt(t.Sources).detail("writing", writingDetail(answer, len(t.Sources))) } return answer, err } + +// readAt is where a rework's basis was read from: each repository at the +// commit its files were read at, once. The basis is the record re-read from +// git, not the index as it stands now, and the trace says so. +func readAt(sources []Source) []string { + var out []string + seen := map[string]bool{} + for _, s := range sources { + if s.IsCommit() || s.SHA == "" { + continue + } + k := s.Repo + " " + shortSHA(s.SHA) + if !seen[k] { + seen[k] = true + out = append(out, k) + } + } + return out +} diff --git a/backend/internal/ask/rework_test.go b/backend/internal/ask/rework_test.go index c9221db2..319dc797 100644 --- a/backend/internal/ask/rework_test.go +++ b/backend/internal/ask/rework_test.go @@ -236,4 +236,35 @@ func TestUnderstandNamesRework(t *testing.T) { if !strings.Contains(understandSystem, `"rework"`) || !strings.Contains(understandSystem, "It exists only when a previous turn is above") { t.Error("the understanding prompt must define rework and tie it to a previous turn") } + // "zeichne ein diagramm des ablaufs" asks for the previous answer drawn, + // nothing new of the code. Absent from the examples, one reply called it + // a rework and the next a standing rule. + if !strings.Contains(understandSystem, `"zeichne ein Diagramm davon"`) { + t.Error("a diagram of the previous answer must be named as a rework") + } +} + +// TestAReworkSaysWhichCommitsItsBasisWasReadAt: the basis is re-read from +// git, not from the index as it is now, and the trace says where from. +func TestAReworkSaysWhichCommitsItsBasisWasReadAt(t *testing.T) { + c, _ := reworkUpstream(t, reworkReply) + p := reworkPipeline(t, c) + th := reworkThread() + th.Sources[0].SHA = "0123456789abcdef" + th.Sources[1].SHA = "0123456789abcdef" + var writing map[string]any + + if _, _, err := p.Run(context.Background(), "summarize", AudienceBA, LanguageEN, th, + Events{OnDetail: func(step string, d map[string]any) { + if step == "writing" { + writing = d + } + }}); err != nil { + t.Fatalf("Run: %v", err) + } + + got, _ := writing["read_at"].([]string) + if len(got) != 1 || got[0] != "peeq 0123456" { + t.Errorf("read_at = %v, want the one repository at its short commit", writing["read_at"]) + } } diff --git a/backend/internal/ask/scope_test.go b/backend/internal/ask/scope_test.go index ba2d5701..e9c0ea81 100644 --- a/backend/internal/ask/scope_test.go +++ b/backend/internal/ask/scope_test.go @@ -39,7 +39,7 @@ func TestAnswerDoesNotPromiseToCoverARepositoryWithNoSources(t *testing.T) { // built to prevent. c, prompt, _ := streamUpstream(t, "x") _, err := NewAnswerer(c).Answer(context.Background(), "How do peeq and rongo differ?", AudienceBA, LanguageEN, - twoSources(), Scope{Known: []string{"peeq", "rongo"}}, "", nil) + twoSources(), Scope{Known: []string{"peeq", "rongo"}}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -59,7 +59,7 @@ func TestOneProjectIsNotAComparisonOfItsOwnRepositories(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") _, err := NewAnswerer(c).Answer(context.Background(), "How does checkout work?", AudienceBA, LanguageEN, bothReposSources(), - Scope{Known: []string{"peeq", "rongo"}, Projects: []string{"shop"}}, "", nil) + Scope{Known: []string{"peeq", "rongo"}, Projects: []string{"shop"}}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -77,7 +77,7 @@ func TestAProjectBesideALooseRepositoryIsStillAComparison(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") _, err := NewAnswerer(c).Answer(context.Background(), "How does shop differ from rongo?", AudienceBA, LanguageEN, bothReposSources(), - Scope{Known: []string{"peeq", "rongo"}, Projects: []string{"shop"}, Loose: []string{"rongo"}}, "", nil) + Scope{Known: []string{"peeq", "rongo"}, Projects: []string{"shop"}, Loose: []string{"rongo"}}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -99,7 +99,7 @@ func TestTwoProjectsStillCompare_AndByProjectName(t *testing.T) { c, prompt, _ := streamUpstream(t, "x") _, err := NewAnswerer(c).Answer(context.Background(), "How do shop and legacy-crm differ?", AudienceBA, LanguageEN, bothReposSources(), - Scope{Known: []string{"peeq", "rongo"}, Projects: []string{"shop", "legacy-crm"}}, "", nil) + Scope{Known: []string{"peeq", "rongo"}, Projects: []string{"shop", "legacy-crm"}}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -124,7 +124,7 @@ func TestNoProjectsMeansTodaysComparison(t *testing.T) { // it did before projects existed. This is the eval's guarantee in one test. c, prompt, _ := streamUpstream(t, "x") _, err := NewAnswerer(c).Answer(context.Background(), "How do peeq and rongo differ?", AudienceBA, LanguageEN, - bothReposSources(), Scope{Known: []string{"peeq", "rongo"}}, "", nil) + bothReposSources(), Scope{Known: []string{"peeq", "rongo"}}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -148,7 +148,7 @@ func TestTheStructureBlockReachesThePromptAndIsMarkedConfiguration(t *testing.T) }}) _, err := NewAnswerer(c).Answer(context.Background(), "How does checkout work?", AudienceBA, LanguageEN, bothReposSources(), - Scope{Known: []string{"peeq", "rongo"}, Projects: []string{"shop"}, Structure: block}, "", nil) + Scope{Known: []string{"peeq", "rongo"}, Projects: []string{"shop"}, Structure: block}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -448,7 +448,7 @@ func TestAnswerPromptCarriesTheComparisonAndTheMissingRepository(t *testing.T) { // loom's side out of its training. c, prompt, _ := streamUpstream(t, "x") _, err := NewAnswerer(c).Answer(context.Background(), "How do peeq and rongo differ?", AudienceBA, LanguageEN, - bothReposSources(), Scope{Known: []string{"peeq", "rongo"}, Unknown: []string{"loom"}}, "", nil) + bothReposSources(), Scope{Known: []string{"peeq", "rongo"}, Unknown: []string{"loom"}}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } @@ -471,7 +471,7 @@ func TestAnswerPromptStaysUnchangedForAnOrdinaryTurn(t *testing.T) { // prompt must be what it was before any of this existed. c, prompt, _ := streamUpstream(t, "x") _, err := NewAnswerer(c).Answer(context.Background(), "How?", AudienceBA, LanguageEN, - twoSources(), Scope{Known: []string{"peeq"}}, "", nil) + twoSources(), Scope{Known: []string{"peeq"}}, FollowUp{}, nil) if err != nil { t.Fatalf("Answer: %v", err) } diff --git a/backend/internal/ask/understand.go b/backend/internal/ask/understand.go index f86ccfcb..8ec534ab 100644 --- a/backend/internal/ask/understand.go +++ b/backend/internal/ask/understand.go @@ -122,7 +122,11 @@ type Understanding struct { // saved rules it contradicts; MemoryRemoves the saved rules the reader // asks to forget. The intent "memory" is a question that is ONLY a // directive, answered without a search. - Memory string `json:"memory"` + Memory string `json:"memory"` + // MemoryMarker is the reader's own word that the instruction lasts, as + // the model quoted it from the question. It is checked, not believed: + // see standingOnly. + MemoryMarker string `json:"memory_marker"` MemoryScope string `json:"memory_scope"` MemoryReplaces IDs `json:"memory_replaces"` MemoryRemoves IDs `json:"memory_removes"` @@ -157,6 +161,33 @@ func (ids *IDs) UnmarshalJSON(b []byte) error { // Directive is what the understanding read as a standing instruction, or // nothing. +// standingOnly drops a rule the question gives no word of the reader's for. +// A rule lasts because the reader said so — "ab jetzt", "never" — and the +// model has to quote that word; a quote the question does not hold is the +// model's judgement, not the reader's. "zeichne ein diagramm des ablaufs" +// was filed as "Draw a diagram for the answer." with no such word, and every +// later answer of that reader drew one. Forgetting is not gated: "show +// flowcharts again" carries no "from now on". Reports whether a rule went. +func (u *Understanding) standingOnly(question string) bool { + if u.Memory == "" { + return false + } + marker := strings.ToLower(strings.TrimSpace(u.MemoryMarker)) + if marker != "" && strings.Contains(strings.ToLower(question), marker) { + return false + } + u.Memory, u.MemoryScope, u.MemoryReplaces = "", "", nil + return true +} + +// KeptRule is the rule the product keeps from this understanding of +// question: Memory, unless standingOnly drops it. What the intent eval +// grades, so it grades what the reader would get. +func (u Understanding) KeptRule(question string) string { + u.standingOnly(question) + return u.Memory +} + func (u Understanding) Directive() memory.Directive { return memory.Directive{ Text: u.Memory, @@ -330,7 +361,8 @@ code lately, asking for no notes, is "changes", never "release". "rework" is a request to restate the PREVIOUS ANSWER in another form, asking nothing new of the code: "summarize", "tl;dr", "shorter", "in one paragraph", "as a table", "as bullet points", "simpler", "rephrase", "expand the second -point", "fasse zusammen", "kürzer", "résume", "riassumi". +point", "draw that as a diagram", "fasse zusammen", "kürzer", +"zeichne ein Diagramm davon", "résume", "en diagramme", "riassumi". It exists only when a previous turn is above; with none, or when the question asks about anything the previous answer does not already say, it is not "rework". A rework has terms [], code_terms [] and repos [].%s @@ -421,7 +453,14 @@ const understandMemoryFields = ` instruction about the answer language, the audience or which repositories to search is not a memory: those are settings the reader picks. Never a statement about what the - code does, never a credential. + code does, never a credential. No word of the reader's + saying it lasts means it is for THIS answer: memory "". + "zeichne ein Diagramm des Ablaufs", "draw it as a + diagram", "keine Emojis" are for this answer. + memory_marker the word or words of the question that say the instruction + lasts, copied exactly as the reader wrote them ("nie + wieder", "from now on", "always"), else "". A memory with + no marker is not kept. memory_scope the project or repository the instruction is limited to, written exactly as the reader named it, else "". memory_replaces ids of saved instructions (listed below the question when diff --git a/backend/internal/httpapi/rework_test.go b/backend/internal/httpapi/rework_test.go index 125f4fce..7dc8321a 100644 --- a/backend/internal/httpapi/rework_test.go +++ b/backend/internal/httpapi/rework_test.go @@ -8,9 +8,60 @@ import ( "github.com/trick77/rongo/internal/ask" "github.com/trick77/rongo/internal/auth" + "github.com/trick77/rongo/internal/repos" + "github.com/trick77/rongo/internal/sourceview" "github.com/trick77/rongo/internal/threads" ) +// checkout is file bytes by "sha:path", as sourceview reads git. +type checkout map[string]string + +func (c checkout) Object(_ context.Context, _ repos.Spec, sha, path string) (string, int64, error) { + body, ok := c[sha+":"+path] + if !ok { + return "", 0, fmt.Errorf("no %s:%s", sha, path) + } + return "blob", int64(len(body)), nil +} + +func (c checkout) ReadFile(_ context.Context, _ repos.Spec, sha, path string) ([]byte, error) { + body, ok := c[sha+":"+path] + if !ok { + return nil, fmt.Errorf("no %s:%s", sha, path) + } + return []byte(body), nil +} + +// TestAsk_aFollowUpAfterAPollStillHasTheWholeBasis is the incident: a good +// answer, a poll a minute later that re-indexed a file it read, and +// "zeichne ein diagramm des ablaufs" refused because every chunk of that +// file had a new id. The basis is read at the commit it was read at. +func TestAsk_aFollowUpAfterAPollStillHasTheWholeBasis(t *testing.T) { + db := askDB(t) + chunkID := seedChunk(t, db) + src := ask.Source{ChunkID: chunkID, Repo: "peeq", Path: "a.go", SHA: "abc1234", StartLine: 2, EndLine: 3, Reason: "hit"} + a := &fakeAsker{tokens: []string{"x"}, sources: []ask.Source{src}} + viewer := sourceview.New(db, checkout{"abc1234:a.go": "package a\nfunc Bypass() {\n}\n"}, 1<<20) + deps := Deps{Auth: auth.NewService(db, "dev", ""), Ask: a, Threads: threads.NewStore(db).WithEvidence(viewer)} + postAsk(t, deps, `{"question":"wie funktioniert der bypass?","audience":"ba"}`) + if _, err := db.Exec(`DELETE FROM chunks WHERE id = ?`, chunkID); err != nil { + t.Fatalf("re-index: %v", err) + } + + list, err := deps.Threads.List(context.Background(), testSubject) + if err != nil || len(list) != 1 { + t.Fatalf("list threads: %v (%d)", err, len(list)) + } + postAsk(t, deps, fmt.Sprintf(`{"question":"zeichne ein diagramm des ablaufs","audience":"ba","thread_id":%q}`, list[0].PublicID)) + + if a.gotThread.SourcesTotal != 1 || len(a.gotThread.Sources) != 1 { + t.Fatalf("basis %d of %d, want whole", len(a.gotThread.Sources), a.gotThread.SourcesTotal) + } + if got := a.gotThread.Sources[0].Text; got != "func Bypass() {\n}" { + t.Errorf("text = %q, want lines 2-3 at the commit the answer read", got) + } +} + // TestAsk_aFollowUpCarriesThePreviousAnswersSources: a rework answers from // the previous turn's own basis, so the handler reads it with the previous // question and answer, whole or not — the count says which. diff --git a/backend/internal/memory/context.go b/backend/internal/memory/context.go index 629bad1b..43b6a4e1 100644 --- a/backend/internal/memory/context.go +++ b/backend/internal/memory/context.go @@ -36,14 +36,15 @@ func (h *Holder) Rows() []Row { } // Apply folds what a directive did into the rows: the deleted go, the new -// one comes first. +// one comes first. A rule the reader already had comes back as itself and +// moves to the front rather than standing twice. func (h *Holder) Apply(a Added) { if h == nil { return } h.mu.Lock() defer h.mu.Unlock() - gone := map[int64]bool{} + gone := map[int64]bool{a.Row.ID: true} for _, id := range a.Deleted { gone[id] = true } diff --git a/backend/internal/memory/memory.go b/backend/internal/memory/memory.go index 39130aa7..398544cb 100644 --- a/backend/internal/memory/memory.go +++ b/backend/internal/memory/memory.go @@ -225,12 +225,32 @@ func (s *Store) Add(ctx context.Context, subject string, d Directive, sourceMess if sourceMessageID != 0 { source = sourceMessageID } + // A rule the reader already has is not written twice: asking again + // listed it twice on the Memory page and twice in every prompt. One + // statement, so the transaction still opens with a write. res, err := tx.ExecContext(ctx, ` - INSERT INTO memories (user_subject, text, scope, source_message_id) VALUES (?, ?, ?, ?)`, - subject, text, scope, source) + INSERT INTO memories (user_subject, text, scope, source_message_id) + SELECT ?, ?, ?, ? + WHERE NOT EXISTS (SELECT 1 FROM memories WHERE user_subject = ? AND lower(text) = lower(?) AND scope = ?)`, + subject, text, scope, source, subject, text, scope) if err != nil { return out, fmt.Errorf("add memory: %w", err) } + if n, err := res.RowsAffected(); err != nil { + return out, err + } else if n == 0 { + if err := tx.QueryRowContext(ctx, ` + SELECT id, text FROM memories WHERE user_subject = ? AND lower(text) = lower(?) AND scope = ?`, + subject, text, scope).Scan(&out.Row.ID, &out.Row.Text); err != nil { + return out, fmt.Errorf("read the rule already kept: %w", err) + } + out.Row.Scope = scope + out.Row.members, out.Row.ScopeLive = resolveScope(pm, scope) + if err := tx.Commit(); err != nil { + return out, err + } + return out, nil + } // Counted after the write, inside the lock, and rolled back when the // cap is passed: the row never lands, and nothing else changes either. var n int diff --git a/backend/internal/memory/memory_test.go b/backend/internal/memory/memory_test.go index aaf11c67..91589f31 100644 --- a/backend/internal/memory/memory_test.go +++ b/backend/internal/memory/memory_test.go @@ -64,6 +64,42 @@ func TestAdd_writesTheRuleAndListReadsItBack(t *testing.T) { } } +// TestAdd_aRuleTheReaderAlreadyHasIsNotAddedTwice: asking again wrote the +// same rule a second time, and the Memory page listed it twice. The rule +// the reader already has comes back as what was kept. +func TestAdd_aRuleTheReaderAlreadyHasIsNotAddedTwice(t *testing.T) { + db := testDB(t) + s := NewStore(db) + ctx := context.Background() + first, err := s.Add(ctx, "jan", Directive{Text: "Draw a diagram for the answer."}, 0) + if err != nil { + t.Fatalf("add: %v", err) + } + + again, err := s.Add(ctx, "jan", Directive{Text: "draw a diagram for the answer."}, 0) + if err != nil { + t.Fatalf("add again: %v", err) + } + + if again.Row.ID != first.Row.ID || again.Row.Text != first.Row.Text { + t.Errorf("again = %+v, want the rule already kept (%d)", again.Row, first.Row.ID) + } + rows, _ := s.List(ctx, "jan") + if len(rows) != 1 { + t.Errorf("%d rows, want one", len(rows)) + } + // Another reader's copy is their own rule. + if other, _ := s.Add(ctx, "other", Directive{Text: "Draw a diagram for the answer."}, 0); other.Row.ID == first.Row.ID { + t.Error("another reader was handed jan's row") + } + // And the holder does not list it twice either. + h := NewHolder([]Row{first.Row}) + h.Apply(again) + if len(h.Rows()) != 1 { + t.Errorf("holder rows = %+v", h.Rows()) + } +} + func TestAdd_replacesAndRemovesOnlyTheReadersOwnRows(t *testing.T) { db := testDB(t) s := NewStore(db) diff --git a/backend/internal/retrieve/eval/flow-rubrics.json b/backend/internal/retrieve/eval/flow-rubrics.json index 41039139..ac7e0fb2 100644 --- a/backend/internal/retrieve/eval/flow-rubrics.json +++ b/backend/internal/retrieve/eval/flow-rubrics.json @@ -180,5 +180,31 @@ "path": "src/main/java/works/weave/socks/cart/controllers/CartsController.java" } ] + }, + { + "question": "draw the flow as a diagram", + "lang": "en", + "follows": "What happens when a customer places an order?", + "must": [ + "The answer contains a diagram of the flow.", + "The diagram has the orders service fetch the customer's data and charge the payment service before it posts the shipment.", + "The diagram ends with the shipment going onto a message queue that queue-master reads." + ], + "must_not": [ + "The diagram saves or ships the order before payment is authorised.", + "The diagram adds a step the previous answer did not describe, such as an e-mail to the customer." + ] + }, + { + "question": "In that flow, where is the shipping cost added?", + "lang": "en", + "follows": "What happens when a customer places an order?", + "must": [ + "The orders service adds a fixed, hard-coded shipping amount of 4.99 to the order total." + ], + "must_not": [ + "The shipping service computes the shipping cost.", + "The shipping cost depends on the address or the weight." + ] } ] diff --git a/backend/internal/retrieve/eval/intent_test.go b/backend/internal/retrieve/eval/intent_test.go index 3ed19342..ef7dbd98 100644 --- a/backend/internal/retrieve/eval/intent_test.go +++ b/backend/internal/retrieve/eval/intent_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/trick77/rongo/internal/ask" + "github.com/trick77/rongo/internal/memory" ) // intentStages are the declared stage names the classifier is told about, @@ -30,6 +31,13 @@ type intentCase struct { want string notLane bool record bool + // answered is the previous turn's answer, for a follow-up that refers + // to it: a rework exists only with one above. + answered string + // memory runs the case with memory on, and rule says whether the + // product keeps a standing rule from it (ask.Understanding.KeptRule). + memory bool + rule bool } // intentCases is the gate's routing measured where it is decided, which @@ -71,8 +79,57 @@ var intentCases = []intentCase{{ name: "what is between two stages, with no notes asked for", question: "what is between prod and intg?", record: true, +}, { + // The incident: one reply called this a rework AND a standing rule, the + // next a rule alone, and every later answer of that reader drew one. + name: "a diagram of the previous answer is a rework and no rule", + question: "zeichne ein diagramm des ablaufs", + follows: "wie funktioniert der bypass der partnervalidierung?", + answered: bypassAnswer, + want: ask.IntentRework, + memory: true, +}, { + name: "the same in English", + question: "draw that as a diagram", + follows: "how does the partner validation bypass work?", + answered: bypassAnswer, + want: ask.IntentRework, + memory: true, +}, { + name: "the same in French", + question: "fais-en un diagramme", + follows: "comment fonctionne le contournement de la validation?", + answered: bypassAnswer, + want: ask.IntentRework, + memory: true, +}, { + name: "a table of the previous answer is a rework and no rule", + question: "als Tabelle bitte", + follows: "wie funktioniert der bypass der partnervalidierung?", + answered: bypassAnswer, + want: ask.IntentRework, + memory: true, +}, { + name: "from now on is a rule", + question: "ab jetzt immer mit Diagramm", + want: ask.IntentMemory, + memory: true, + rule: true, +}, { + name: "never again is a rule", + question: "never show me flowcharts again", + want: ask.IntentMemory, + memory: true, + rule: true, }} +// bypassAnswer is the opening of the incident's first answer, which is all +// the understanding step is shown of one. +const bypassAnswer = "Der CRM-Bypass der Partnervalidierung funktioniert so, dass eine Schadenmeldung mit dem " + + "Verarbeitungshinweis BYPASS_PARTNERVALIDATION bei der Ereignisregistrierung an Syrius technisch verändert " + + "wird: Die Sozialversicherungsnummer wird entfernt und eine vorhandene Personalnummer durch einen künstlichen " + + "Wert mit Präfix BYPASS- ersetzt; in einer Produktionsumgebung wird diese Änderung nicht angewendet." + // TestEvalUnderstandIntent measures the classifier's intent on the release // gate, one short-gate call per case. Run it twice: a pinned gate call still // re-rolls, and a one-case gap between runs says nothing. @@ -84,14 +141,15 @@ func TestEvalUnderstandIntent(t *testing.T) { for _, tc := range intentCases { t.Run(tc.name, func(t *testing.T) { - thread := ask.Thread{} - if tc.follows != "" { - thread.Question = tc.follows + thread := ask.Thread{Question: tc.follows, Answer: tc.answered} + ctx := context.Background() + if tc.memory { + ctx = memory.With(ctx, memory.NewHolder(nil)) } var got ask.Understanding var last error for attempt := 1; attempt <= expandAttempts; attempt++ { - got, last = u.Understand(context.Background(), tc.question, thread, intentStages) + got, last = u.Understand(ctx, tc.question, thread, intentStages) if last == nil { break } @@ -113,6 +171,13 @@ func TestEvalUnderstandIntent(t *testing.T) { case got.Intent != tc.want: t.Errorf("intent = %q, want %q (stage=%q)", got.Intent, tc.want, got.Stage) } + if tc.memory { + kept := got.KeptRule(tc.question) + t.Logf("memory=%q marker=%q kept=%q", got.Memory, got.MemoryMarker, kept) + if (kept != "") != tc.rule { + t.Errorf("rule kept = %q, want kept: %v", kept, tc.rule) + } + } }) } } diff --git a/backend/internal/sourceview/commit.go b/backend/internal/sourceview/commit.go index fe18e036..6b739c4d 100644 --- a/backend/internal/sourceview/commit.go +++ b/backend/internal/sourceview/commit.go @@ -112,6 +112,34 @@ func (s *Service) Commit(ctx context.Context, repo, sha string) (Commit, error) if n == 0 { return Commit{}, fmt.Errorf("%w: %s/%s is not in the commit lane", ErrNotFound, repo, sha) } + return s.show(ctx, repo, branch, sha) +} + +// RecordedCommit is Commit for a thread's own record: a commit an answer was +// written from, read back without the lane's permission. The lane is a +// window that slides with every push and drops what fell out of it; the +// answer that cited the commit did not stop being written from it. +func (s *Service) RecordedCommit(ctx context.Context, repo, sha string) (Commit, error) { + if s.commits == nil { + return Commit{}, fmt.Errorf("%w: no commit reader", ErrNotFound) + } + if !shaRe.MatchString(sha) { + return Commit{}, fmt.Errorf("%w: commit %q", ErrInvalid, sha) + } + var branch string + err := s.db.QueryRowContext(ctx, `SELECT branch FROM repo_state WHERE name = ?`, repo).Scan(&branch) + if errors.Is(err, sql.ErrNoRows) { + return Commit{}, fmt.Errorf("%w: unknown repository %q", ErrNotFound, repo) + } + if err != nil { + return Commit{}, fmt.Errorf("look up repository %q: %w", repo, err) + } + return s.show(ctx, repo, branch, sha) +} + +// show reads sha of repo from the checkout, once whoever asked has decided +// it may be shown. +func (s *Service) show(ctx context.Context, repo, branch, sha string) (Commit, error) { d, err := s.commits.Show(ctx, repos.Spec{Name: repo}, sha) if err != nil { return Commit{}, fmt.Errorf("%w: %w", ErrNotFound, err) diff --git a/backend/internal/sourceview/commit_test.go b/backend/internal/sourceview/commit_test.go index 5171dd92..b638284f 100644 --- a/backend/internal/sourceview/commit_test.go +++ b/backend/internal/sourceview/commit_test.go @@ -40,3 +40,27 @@ func TestCommit_servesARecordedCommitWithItsFiles(t *testing.T) { t.Errorf("no reader: %v", err) } } + +// TestRecordedCommit_readsACommitTheLaneHasSinceDropped: the lane is a +// window, and an answer written from a commit that slid out of it is still +// written from it. The thread's record reads it without the lane's say. +func TestRecordedCommit_readsACommitTheLaneHasSinceDropped(t *testing.T) { + f := newFixture(t, 1<<20) + + got, err := f.svc.RecordedCommit(context.Background(), "peeq", f.second) + if err != nil { + t.Fatalf("RecordedCommit: %v", err) + } + if got.SHA != f.second || got.Branch != "main" || len(got.Files) != 1 { + t.Errorf("RecordedCommit = %+v", got) + } + if _, err := f.svc.RecordedCommit(context.Background(), "peeq", "-rf"); !errors.Is(err, ErrInvalid) { + t.Errorf("malformed sha: %v", err) + } + if _, err := f.svc.RecordedCommit(context.Background(), "nope", f.second); !errors.Is(err, ErrNotFound) { + t.Errorf("unknown repo: %v", err) + } + if _, err := f.svc.WithCommits(nil).RecordedCommit(context.Background(), "peeq", f.second); !errors.Is(err, ErrNotFound) { + t.Errorf("no reader: %v", err) + } +} diff --git a/backend/internal/store/migrations/0037_source_identity.sql b/backend/internal/store/migrations/0037_source_identity.sql new file mode 100644 index 00000000..e3fd1f95 --- /dev/null +++ b/backend/internal/store/migrations/0037_source_identity.sql @@ -0,0 +1,32 @@ +-- A source is recorded by what it IS — repository, path, the commit it was +-- read at, the lines — not only by the row it came from. chunk ids are not +-- stable: a poll that touches a file re-inserts every chunk of it under new +-- ids, so a record keyed on them lost its basis a minute after the answer +-- and a rework of it was refused. The commit does not move; the record +-- re-reads the lines from git there. +-- +-- chunk_id and commit_id stay: the uniqueness is keyed on them, and a row +-- the backfill below cannot complete still resolves the old way. +-- For a commit source sha is the commit, path and lines stay empty. +ALTER TABLE message_sources ADD COLUMN repo TEXT NOT NULL DEFAULT ''; +ALTER TABLE message_sources ADD COLUMN path TEXT NOT NULL DEFAULT ''; +ALTER TABLE message_sources ADD COLUMN sha TEXT NOT NULL DEFAULT ''; +ALTER TABLE message_sources ADD COLUMN start_line INTEGER NOT NULL DEFAULT 0; +ALTER TABLE message_sources ADD COLUMN end_line INTEGER NOT NULL DEFAULT 0; +ALTER TABLE message_sources ADD COLUMN symbol TEXT NOT NULL DEFAULT ''; + +-- A chunk still in the index has not been re-read since the answer, so the +-- file's sha is the commit it was read at. +UPDATE message_sources +SET (repo, path, sha, start_line, end_line, symbol) = ( + SELECT f.repo, f.path, f.sha, c.start_line, c.end_line, c.symbol + FROM chunks c JOIN files f ON f.id = c.file_id + WHERE c.id = message_sources.chunk_id) +WHERE chunk_id <> 0 + AND EXISTS (SELECT 1 FROM chunks c WHERE c.id = message_sources.chunk_id); + +UPDATE message_sources +SET (repo, sha) = ( + SELECT c.repo, c.sha FROM commits c WHERE c.id = message_sources.commit_id) +WHERE commit_id <> 0 + AND EXISTS (SELECT 1 FROM commits c WHERE c.id = message_sources.commit_id); diff --git a/backend/internal/store/source_identity_backfill_test.go b/backend/internal/store/source_identity_backfill_test.go new file mode 100644 index 00000000..734f07c8 --- /dev/null +++ b/backend/internal/store/source_identity_backfill_test.go @@ -0,0 +1,70 @@ +package store + +import ( + "strings" + "testing" +) + +// sourceIdentityBackfill is 0037's UPDATE statements, taken from the +// migration itself so the test cannot drift from it. +func sourceIdentityBackfill(t *testing.T) string { + t.Helper() + body, err := migrationsFS.ReadFile("migrations/0037_source_identity.sql") + if err != nil { + t.Fatalf("read migration: %v", err) + } + i := strings.Index(string(body), "UPDATE message_sources") + if i < 0 { + t.Fatal("0037 no longer contains the backfill") + } + return string(body)[i:] +} + +func TestSourceIdentityBackfill_completesRowsThatStillJoinAndLeavesTheRest(t *testing.T) { + db := migratedDB(t) + _, headID := seedThreadAndHead(t, db, "q") + for _, q := range []string{ + `INSERT INTO repo_state (name, clone_url, branch) VALUES ('peeq', 'x', 'master')`, + `INSERT INTO files (id, repo, path, sha) VALUES (1, 'peeq', 'a.go', 'deadbeef')`, + `INSERT INTO chunks (id, file_id, ordinal, start_line, end_line, symbol, text, raw_text, content_hash) + VALUES (7, 1, 0, 3, 9, 'A', 't', 't', 'h')`, + `INSERT INTO commits (id, repo, sha, committed_at, author, subject, body, paths) + VALUES (5, 'peeq', 'aaa1111', '2026-09-17T10:00:00Z', 'x', 's', 'b', '')`, + } { + if _, err := db.Exec(q); err != nil { + t.Fatalf("seed: %v", err) + } + } + for _, row := range [][2]int64{{7, 0}, {8, 0}, {0, 5}} { + if _, err := db.Exec(`INSERT INTO message_sources (message_id, chunk_id, commit_id, reason) VALUES (?, ?, ?, 'hit')`, + headID, row[0], row[1]); err != nil { + t.Fatalf("seed source: %v", err) + } + } + + if _, err := db.Exec(sourceIdentityBackfill(t)); err != nil { + t.Fatalf("backfill: %v", err) + } + + type ident struct { + repo, path, sha, symbol string + start, end int + } + read := func(chunk, commit int64) ident { + var i ident + if err := db.QueryRow(`SELECT repo, path, sha, symbol, start_line, end_line FROM message_sources + WHERE chunk_id = ? AND commit_id = ?`, chunk, commit).Scan(&i.repo, &i.path, &i.sha, &i.symbol, &i.start, &i.end); err != nil { + t.Fatalf("read %d/%d: %v", chunk, commit, err) + } + return i + } + if got := read(7, 0); got != (ident{"peeq", "a.go", "deadbeef", "A", 3, 9}) { + t.Errorf("chunk row = %+v", got) + } + if got := read(8, 0); got != (ident{}) { + t.Errorf("a row whose chunk is gone = %+v, want it left alone", got) + } + if got := read(0, 5); got != (ident{repo: "peeq", sha: "aaa1111"}) { + t.Errorf("commit row = %+v", got) + } +} diff --git a/backend/internal/threads/source_identity_test.go b/backend/internal/threads/source_identity_test.go new file mode 100644 index 00000000..80f85a6e --- /dev/null +++ b/backend/internal/threads/source_identity_test.go @@ -0,0 +1,184 @@ +package threads + +import ( + "context" + "database/sql" + "fmt" + "testing" + "time" + + "github.com/trick77/rongo/internal/ask" + "github.com/trick77/rongo/internal/sourceview" +) + +// fakeEvidence is the checkout as Sources reads it: files by (repo, path, +// sha), commits by (repo, sha). What it lacks is not found, the way a purged +// repository or a replaced snapshot is. +type fakeEvidence struct { + files map[string]string + commits map[string]sourceview.Commit + reads int +} + +func (f *fakeEvidence) Read(_ context.Context, repo, path, sha string) (sourceview.File, error) { + f.reads++ + body, ok := f.files[repo+"|"+path+"|"+sha] + if !ok { + return sourceview.File{}, fmt.Errorf("%w: %s/%s@%s", sourceview.ErrNotFound, repo, path, sha) + } + return sourceview.File{Repo: repo, Branch: "master", Path: path, SHA: sha, Content: body}, nil +} + +func (f *fakeEvidence) RecordedCommit(_ context.Context, repo, sha string) (sourceview.Commit, error) { + c, ok := f.commits[repo+"|"+sha] + if !ok { + return sourceview.Commit{}, fmt.Errorf("%w: %s@%s", sourceview.ErrNotFound, repo, sha) + } + return c, nil +} + +func reindexFile(t *testing.T, db *sql.DB, path string) { + t.Helper() + // What ReplaceFile does to a touched file: every chunk goes, new ids come. + if _, err := db.Exec(`DELETE FROM chunks WHERE file_id IN (SELECT id FROM files WHERE path = ?)`, path); err != nil { + t.Fatalf("re-index %s: %v", path, err) + } +} + +func TestSourcesSurviveARe_indexByReadingTheCommitTheyWereReadFrom(t *testing.T) { + // Given an answer written from two windows of one file, then a poll that + // re-indexed the file and so gave every chunk of it a new id + s, ctx, threadID, db := newThreadStore(t) + ev := &fakeEvidence{files: map[string]string{ + "peeq|a.go|deadbeef": "package a\n\nfunc A() {}\nfunc B() {}\n", + }} + s.WithEvidence(ev) + msg, _ := s.AddQuestion(ctx, threadID, "ba", "de", "frage", 0) + insertChunk(t, db, 1, "peeq", "a.go", "package a") + if err := s.SaveSources(ctx, msg.ID, []ask.Source{ + {ChunkID: 1, Repo: "peeq", Path: "a.go", SHA: "deadbeef", StartLine: 1, EndLine: 1, Reason: "hit"}, + {ChunkID: 2, Repo: "peeq", Path: "a.go", SHA: "deadbeef", StartLine: 3, EndLine: 4, Symbol: "A", Reason: "reference:A", Hop: 1}, + }); err != nil { + t.Fatalf("save sources: %v", err) + } + reindexFile(t, db, "a.go") + + // When + got, total, err := s.Sources(ctx, testSubject, msg.ID) + + // Then the basis is whole, in the text it had when the answer was written + if err != nil { + t.Fatalf("sources: %v", err) + } + if total != 2 || len(got) != 2 { + t.Fatalf("total %d, got %d, want 2 and 2", total, len(got)) + } + if got[0].Text != "package a" || got[1].Text != "func A() {}\nfunc B() {}" { + t.Errorf("texts = %q, %q", got[0].Text, got[1].Text) + } + if got[1].Symbol != "A" || got[1].Reason != "reference:A" || got[1].Hop != 1 || got[1].Branch != "master" || got[1].SHA != "deadbeef" { + t.Errorf("second source = %+v", got[1]) + } + if ev.reads != 1 { + t.Errorf("read the file %d times, want once for both windows", ev.reads) + } +} + +func TestASourceGitCannotProduceIsMissing(t *testing.T) { + // A purged repository, a replaced snapshot: the object is gone, and the + // basis is short by it. Rework refuses on that; it must not be hidden. + s, ctx, threadID, _ := newThreadStore(t) + s.WithEvidence(&fakeEvidence{files: map[string]string{"peeq|a.go|deadbeef": "package a\n"}}) + msg, _ := s.AddQuestion(ctx, threadID, "ba", "en", "q", 0) + if err := s.SaveSources(ctx, msg.ID, []ask.Source{ + {ChunkID: 1, Repo: "peeq", Path: "a.go", SHA: "deadbeef", StartLine: 1, EndLine: 1, Reason: "hit"}, + {ChunkID: 2, Repo: "gone", Path: "b.go", SHA: "cafe", StartLine: 1, EndLine: 2, Reason: "hit"}, + }); err != nil { + t.Fatalf("save sources: %v", err) + } + + got, total, err := s.Sources(ctx, testSubject, msg.ID) + if err != nil { + t.Fatalf("sources: %v", err) + } + if total != 2 || len(got) != 1 || got[0].Path != "a.go" { + t.Errorf("total %d, got %+v; want 2 and only a.go", total, got) + } +} + +func TestSplitSiblingsOfOneLineAreOneSource(t *testing.T) { + // An overlong line is stored as sibling chunks sharing one line range. + // Read back from git they are the same text; counting them twice would + // report a basis short by one that is in fact whole. + s, ctx, threadID, _ := newThreadStore(t) + s.WithEvidence(&fakeEvidence{files: map[string]string{"peeq|min.js|deadbeef": "x=1\n"}}) + msg, _ := s.AddQuestion(ctx, threadID, "ba", "en", "q", 0) + if err := s.SaveSources(ctx, msg.ID, []ask.Source{ + {ChunkID: 1, Repo: "peeq", Path: "min.js", SHA: "deadbeef", StartLine: 1, EndLine: 1, Reason: "hit"}, + {ChunkID: 2, Repo: "peeq", Path: "min.js", SHA: "deadbeef", StartLine: 1, EndLine: 1, Reason: "hit"}, + }); err != nil { + t.Fatalf("save sources: %v", err) + } + + got, total, err := s.Sources(ctx, testSubject, msg.ID) + if err != nil { + t.Fatalf("sources: %v", err) + } + if total != 1 || len(got) != 1 || got[0].Text != "x=1" { + t.Errorf("total %d, got %+v; want one source", total, got) + } +} + +func TestACommitSourceOutlivesItsRowInTheCommitLane(t *testing.T) { + // The window slides with every push, and dropAbsent deletes what fell + // out of it. The commit is still in git; the record reads it there. + s, ctx, threadID, db := newThreadStore(t) + at := "2026-09-17T10:00:00Z" + s.WithEvidence(&fakeEvidence{commits: map[string]sourceview.Commit{ + "rongo|aaa1111": {Repo: "rongo", Branch: "master", SHA: "aaa1111", CommittedAt: at, Subject: "Newest", Body: "why", + Files: []sourceview.FileChange{{Path: "a.go"}, {Path: "b.go"}}}, + }}) + msg, _ := s.AddQuestion(ctx, threadID, "ba", "en", "what changed?", 0) + insertCommit(t, db, 11, "rongo", "aaa1111", at, "Newest", "a.go\nb.go") + if err := s.SaveSources(ctx, msg.ID, []ask.Source{ + {Kind: ask.SourceCommit, CommitID: 11, Repo: "rongo", SHA: "aaa1111", Reason: "hit"}, + }); err != nil { + t.Fatalf("save sources: %v", err) + } + if _, err := db.Exec(`DELETE FROM commits WHERE id = 11`); err != nil { + t.Fatal(err) + } + + got, total, err := s.Sources(ctx, testSubject, msg.ID) + if err != nil { + t.Fatalf("sources: %v", err) + } + if total != 1 || len(got) != 1 { + t.Fatalf("total %d, got %d", total, len(got)) + } + c := got[0] + if !c.IsCommit() || c.Subject != "Newest" || c.Text != "why" || len(c.Paths) != 2 || + !c.CommittedAt.Equal(time.Date(2026, 9, 17, 10, 0, 0, 0, time.UTC)) { + t.Errorf("commit source = %+v", c) + } +} + +func TestARowFromBeforeTheIdentityStillResolvesByItsChunk(t *testing.T) { + // A row the backfill could not complete has only its chunk id; it reads + // the way every row did before, so an older thread keeps working. + s, ctx, threadID, db := newThreadStore(t) + s.WithEvidence(&fakeEvidence{}) + msg, _ := s.AddQuestion(ctx, threadID, "ba", "en", "q", 0) + insertChunk(t, db, 1, "peeq", "a.go", "package a") + if err := s.SaveSources(ctx, msg.ID, []ask.Source{{ChunkID: 1, Reason: "hit"}}); err != nil { + t.Fatalf("save sources: %v", err) + } + + got, total, err := s.Sources(ctx, testSubject, msg.ID) + if err != nil { + t.Fatalf("sources: %v", err) + } + if total != 1 || len(got) != 1 || got[0].Text != "package a" { + t.Errorf("total %d, got %+v", total, got) + } +} diff --git a/backend/internal/threads/sources.go b/backend/internal/threads/sources.go new file mode 100644 index 00000000..a32ff4c2 --- /dev/null +++ b/backend/internal/threads/sources.go @@ -0,0 +1,249 @@ +package threads + +import ( + "context" + "database/sql" + "errors" + "fmt" + "log/slog" + "sort" + "strings" + "time" + + "github.com/trick77/rongo/internal/ask" + "github.com/trick77/rongo/internal/sourceview" +) + +// Evidence is the checkout as the record reads it back: a file at the commit +// an answer read it at, and a commit an answer was written from. The source +// viewer satisfies it, so a rework reads exactly what a citation opens — +// same permission, same redaction. +type Evidence interface { + Read(ctx context.Context, repo, path, sha string) (sourceview.File, error) + RecordedCommit(ctx context.Context, repo, sha string) (sourceview.Commit, error) +} + +// WithEvidence gives the store the checkout. Without it a source resolves +// only through the chunk it came from, which is what a test store without a +// git binary wants. +func (s *Store) WithEvidence(e Evidence) *Store { + s.evidence = e + return s +} + +// SaveSources records what an answer was actually written from: every source +// by what it is — repository, path, commit, lines — beside the row it came +// from. The row id is not stable across a re-index; the commit is. +func (s *Store) SaveSources(ctx context.Context, messageID int64, sources []ask.Source) error { + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() + + for _, src := range sources { + // One id per row: a commit source has no chunk, a chunk no commit. + chunkID, commitID := src.ChunkID, int64(0) + path, start, end, symbol := src.Path, src.StartLine, src.EndLine, src.Symbol + if src.IsCommit() { + chunkID, commitID = 0, src.CommitID + path, start, end, symbol = "", 0, 0, "" + } + if _, err := tx.ExecContext(ctx, ` + INSERT INTO message_sources (message_id, chunk_id, commit_id, reason, hop, repo, path, sha, start_line, end_line, symbol) + VALUES (?,?,?,?,?,?,?,?,?,?,?)`, + messageID, chunkID, commitID, src.Reason, src.Hop, src.Repo, path, src.SHA, start, end, symbol); err != nil { + return fmt.Errorf("store source %d/%d: %w", chunkID, commitID, err) + } + } + return tx.Commit() +} + +// recorded is one message_sources row as stored. +type recorded struct { + chunkID, commitID int64 + repo, path, sha string + start, end int + symbol, reason string + hop int +} + +// key is what makes two rows one source. Siblings of one overlong line share +// a line range and read back as the same text, so they are one; a row with +// no identity is its own id. +func (r recorded) key() string { + switch { + case r.commitID != 0 && r.repo != "": + return "commit|" + r.repo + "|" + r.sha + case r.commitID != 0: + return fmt.Sprintf("commit#%d", r.commitID) + case r.repo != "": + return fmt.Sprintf("file|%s|%s|%s|%d|%d", r.repo, r.path, r.sha, r.start, r.end) + } + return fmt.Sprintf("chunk#%d", r.chunkID) +} + +// Sources reads an answer's basis back, chunks ordered by hop, then commits +// newest first. A source is read from git at the commit it was read at, so a +// re-index since the answer changes nothing; one git can no longer produce — +// a purged repository, a replaced snapshot, a file now excluded — is +// silently omitted. A message that does not belong to a thread owned by +// subject yields an empty slice, the same shape as "no sources yet", never +// another user's evidence. +// +// Sources also reports total: how many sources the record holds for this +// message, scoped by the same ownership check. The caller decides what an +// incomplete set means (Sources itself does not know), but it can only +// decide correctly by comparing len(returned) against total — a basis short +// by SOME of its evidence looks identical to a whole one if only the +// resolved slice is visible. +func (s *Store) Sources(ctx context.Context, subject string, messageID int64) (sources []ask.Source, total int, err error) { + rows, err := s.db.QueryContext(ctx, ` + SELECT ms.chunk_id, ms.commit_id, ms.repo, ms.path, ms.sha, ms.start_line, ms.end_line, ms.symbol, ms.reason, ms.hop + FROM message_sources ms + JOIN messages m ON m.id = ms.message_id + JOIN threads t ON t.id = m.thread_id + WHERE ms.message_id = ? AND t.user_subject = ? + ORDER BY ms.hop, ms.chunk_id, ms.commit_id`, messageID, subject) + if err != nil { + return nil, 0, fmt.Errorf("read sources: %w", err) + } + var recs []recorded + seen := map[string]bool{} + for rows.Next() { + var r recorded + if err := rows.Scan(&r.chunkID, &r.commitID, &r.repo, &r.path, &r.sha, &r.start, &r.end, &r.symbol, &r.reason, &r.hop); err != nil { + _ = rows.Close() + return nil, 0, fmt.Errorf("scan source: %w", err) + } + if k := r.key(); !seen[k] { + seen[k] = true + recs = append(recs, r) + } + } + _ = rows.Close() + if err := rows.Err(); err != nil { + return nil, 0, err + } + + // Deliberately NOT filtered on enabled, unlike every retrieval and + // routing query. This is the RECORD: a turn answered before its + // repository was parked cites it, and a thread is never rewritten. + // Parking stops NEW answers, it does not revise old ones. + files := map[string]fileRead{} + out := []ask.Source{} + var commits []ask.Source + for _, r := range recs { + var src ask.Source + var ok bool + if r.commitID != 0 { + src, ok, err = s.commitSource(ctx, r) + if ok { + commits = append(commits, src) + } + } else { + src, ok, err = s.chunkSource(ctx, r, files) + if ok { + out = append(out, src) + } + } + if err != nil { + return nil, 0, err + } + } + sort.SliceStable(commits, func(i, j int) bool { return commits[i].CommittedAt.After(commits[j].CommittedAt) }) + return append(out, commits...), len(recs), nil +} + +// fileRead is one file read from git, kept for the other windows of it. +type fileRead struct { + branch string + lines []string + err error +} + +// chunkSource resolves a file source: from git at its commit, else from the +// chunk it came from while that row still exists — a chunk id is never +// reused, so a row that still joins holds the text it held. +func (s *Store) chunkSource(ctx context.Context, r recorded, files map[string]fileRead) (ask.Source, bool, error) { + if r.repo != "" && s.evidence != nil { + k := r.repo + "|" + r.path + "|" + r.sha + f, ok := files[k] + if !ok { + file, err := s.evidence.Read(ctx, r.repo, r.path, r.sha) + f = fileRead{branch: file.Branch, err: err} + if err == nil { + // Split the way the chunker splits, so a window is the lines + // it was cut from. + f.lines = strings.Split(strings.TrimSuffix(file.Content, "\n"), "\n") + } + files[k] = f + } + if f.err == nil && r.start >= 1 && r.end >= r.start && r.end <= len(f.lines) { + return ask.Source{ + ChunkID: r.chunkID, Repo: r.repo, Branch: f.branch, Path: r.path, SHA: r.sha, Symbol: r.symbol, + StartLine: r.start, EndLine: r.end, Text: strings.Join(f.lines[r.start-1:r.end], "\n"), + Reason: r.reason, Hop: r.hop, + }, true, nil + } + if f.err != nil { + slog.Debug("source not readable from git", "repo", r.repo, "path", r.path, "sha", r.sha, "err", f.err) + } + } + src := ask.Source{ChunkID: r.chunkID, Reason: r.reason, Hop: r.hop} + err := s.db.QueryRowContext(ctx, ` + SELECT f.repo, rs.branch, f.path, f.sha, c.symbol, c.start_line, c.end_line, c.raw_text + FROM chunks c + JOIN files f ON f.id = c.file_id + JOIN repo_state rs ON rs.name = f.repo + WHERE c.id = ?`, r.chunkID).Scan(&src.Repo, &src.Branch, &src.Path, &src.SHA, &src.Symbol, &src.StartLine, &src.EndLine, &src.Text) + if errors.Is(err, sql.ErrNoRows) { + return ask.Source{}, false, nil + } + if err != nil { + return ask.Source{}, false, fmt.Errorf("read source chunk %d: %w", r.chunkID, err) + } + return src, true, nil +} + +// commitSource resolves a commit source: from the commit lane while it still +// holds the commit, else from git — the lane's window slides with every push. +func (s *Store) commitSource(ctx context.Context, r recorded) (ask.Source, bool, error) { + src := ask.Source{Kind: ask.SourceCommit, CommitID: r.commitID, Reason: r.reason, Hop: r.hop} + var at, paths string + q, args := ` + SELECT c.repo, rs.branch, c.sha, c.committed_at, c.subject, c.body, c.paths + FROM commits c JOIN repo_state rs ON rs.name = c.repo + WHERE c.id = ?`, []any{r.commitID} + if r.repo != "" { + q, args = ` + SELECT c.repo, rs.branch, c.sha, c.committed_at, c.subject, c.body, c.paths + FROM commits c JOIN repo_state rs ON rs.name = c.repo + WHERE c.repo = ? AND c.sha = ?`, []any{r.repo, r.sha} + } + err := s.db.QueryRowContext(ctx, q, args...).Scan(&src.Repo, &src.Branch, &src.SHA, &at, &src.Subject, &src.Text, &paths) + switch { + case err == nil: + src.CommittedAt, _ = time.Parse(time.RFC3339, at) + if paths != "" { + src.Paths = strings.Split(paths, "\n") + } + return src, true, nil + case !errors.Is(err, sql.ErrNoRows): + return ask.Source{}, false, fmt.Errorf("read commit source %d: %w", r.commitID, err) + case r.repo == "" || s.evidence == nil: + return ask.Source{}, false, nil + } + c, err := s.evidence.RecordedCommit(ctx, r.repo, r.sha) + if err != nil { + slog.Debug("commit not readable from git", "repo", r.repo, "sha", r.sha, "err", err) + return ask.Source{}, false, nil + } + src.Repo, src.Branch, src.SHA, src.Subject, src.Text = c.Repo, c.Branch, c.SHA, c.Subject, c.Body + src.CommittedAt, _ = time.Parse(time.RFC3339, c.CommittedAt) + src.CommittedAt = src.CommittedAt.UTC() + for _, f := range c.Files { + src.Paths = append(src.Paths, f.Path) + } + return src, true, nil +} diff --git a/backend/internal/threads/store.go b/backend/internal/threads/store.go index 7492ba89..3f5810e1 100644 --- a/backend/internal/threads/store.go +++ b/backend/internal/threads/store.go @@ -223,7 +223,8 @@ const noCeiling = int64(1<<63 - 1) // Store is the thread and message store, backed by the messages tables. type Store struct { - db *sql.DB + db *sql.DB + evidence Evidence } // NewStore builds a Store. @@ -1367,123 +1368,6 @@ func (s *Store) LinkChoice(ctx context.Context, subject string, messageID, clari return nil } -// SaveSources records what an answer was actually written from, as chunk ids. -func (s *Store) SaveSources(ctx context.Context, messageID int64, sources []ask.Source) error { - tx, err := s.db.BeginTx(ctx, nil) - if err != nil { - return err - } - defer func() { _ = tx.Rollback() }() - - for _, src := range sources { - // One id per row: a commit source has no chunk, a chunk no commit. - chunkID, commitID := src.ChunkID, int64(0) - if src.IsCommit() { - chunkID, commitID = 0, src.CommitID - } - if _, err := tx.ExecContext(ctx, ` - INSERT INTO message_sources (message_id, chunk_id, commit_id, reason, hop) VALUES (?,?,?,?,?)`, - messageID, chunkID, commitID, src.Reason, src.Hop); err != nil { - return fmt.Errorf("store source %d/%d: %w", chunkID, commitID, err) - } - } - return tx.Commit() -} - -// Sources resolves an answer's chunk ids back to their text, ordered by hop -// then chunk id. A chunk a re-index removed no longer joins and is silently -// omitted from the returned slice. A message that does not belong to a -// thread owned by subject yields an empty slice, the same shape as "no -// sources yet", never another user's evidence. -// -// Sources also reports total: how many rows message_sources actually holds -// for this message, scoped by the same ownership check. The caller decides -// what an incomplete set means (Sources itself does not know), but it can -// only decide correctly by comparing len(returned) against total — a -// re-index that removed SOME of the evidence looks identical to one that -// removed none of it if only the resolved slice is visible. -func (s *Store) Sources(ctx context.Context, subject string, messageID int64) (sources []ask.Source, total int, err error) { - if err := s.db.QueryRowContext(ctx, ` - SELECT COUNT(*) - FROM message_sources ms - JOIN messages m ON m.id = ms.message_id - JOIN threads t ON t.id = m.thread_id - WHERE ms.message_id = ? AND t.user_subject = ?`, messageID, subject).Scan(&total); err != nil { - return nil, 0, fmt.Errorf("count sources: %w", err) - } - - rows, err := s.db.QueryContext(ctx, ` - SELECT ms.chunk_id, f.repo, r.branch, f.path, f.sha, c.symbol, c.start_line, c.end_line, c.raw_text, ms.reason, ms.hop - FROM message_sources ms - JOIN chunks c ON c.id = ms.chunk_id - JOIN files f ON f.id = c.file_id - -- Deliberately NOT filtered on r.enabled, unlike every retrieval and - -- routing query. This is the RECORD: a turn answered before its - -- repository was parked cites it, and a thread is never rewritten. An - -- enabled clause here would empty the sources of answers that were - -- correct when they were given, which is the opposite of what parking - -- means — it stops NEW answers, it does not revise old ones. - JOIN repo_state r ON r.name = f.repo - JOIN messages m ON m.id = ms.message_id - JOIN threads t ON t.id = m.thread_id - WHERE ms.message_id = ? AND t.user_subject = ? - ORDER BY ms.hop, ms.chunk_id`, messageID, subject) - if err != nil { - return nil, 0, fmt.Errorf("read sources: %w", err) - } - defer func() { _ = rows.Close() }() - out := []ask.Source{} - for rows.Next() { - var src ask.Source - if err := rows.Scan(&src.ChunkID, &src.Repo, &src.Branch, &src.Path, &src.SHA, &src.Symbol, &src.StartLine, &src.EndLine, &src.Text, &src.Reason, &src.Hop); err != nil { - return nil, 0, fmt.Errorf("scan source: %w", err) - } - out = append(out, src) - } - if err := rows.Err(); err != nil { - return nil, 0, err - } - commits, err := s.commitSources(ctx, subject, messageID) - if err != nil { - return nil, 0, err - } - return append(out, commits...), total, nil -} - -// commitSources is the commit half of Sources: the rows of a changes turn, -// read back from the commits table in the order the answer numbered them. -// Same ownership check, same "no enabled filter" rule, and a commit a -// re-index dropped no longer joins, which the caller reads off total. -func (s *Store) commitSources(ctx context.Context, subject string, messageID int64) ([]ask.Source, error) { - rows, err := s.db.QueryContext(ctx, ` - SELECT ms.commit_id, c.repo, r.branch, c.sha, c.committed_at, c.subject, c.body, c.paths, ms.reason, ms.hop - FROM message_sources ms - JOIN commits c ON c.id = ms.commit_id - JOIN repo_state r ON r.name = c.repo - JOIN messages m ON m.id = ms.message_id - JOIN threads t ON t.id = m.thread_id - WHERE ms.message_id = ? AND ms.commit_id <> 0 AND t.user_subject = ? - ORDER BY c.committed_at DESC, ms.commit_id`, messageID, subject) - if err != nil { - return nil, fmt.Errorf("read commit sources: %w", err) - } - defer func() { _ = rows.Close() }() - out := []ask.Source{} - for rows.Next() { - src := ask.Source{Kind: ask.SourceCommit} - var at, paths string - if err := rows.Scan(&src.CommitID, &src.Repo, &src.Branch, &src.SHA, &at, &src.Subject, &src.Text, &paths, &src.Reason, &src.Hop); err != nil { - return nil, fmt.Errorf("scan commit source: %w", err) - } - src.CommittedAt, _ = time.Parse(time.RFC3339, at) - if paths != "" { - src.Paths = strings.Split(paths, "\n") - } - out = append(out, src) - } - return out, rows.Err() -} - // narrowedTo is the repositories a turn resumed from the too-broad panel was // narrowed to, and nothing at all for every other turn. // diff --git a/ui/src/Trace.tsx b/ui/src/Trace.tsx index 2fe9a550..95e00bd4 100644 --- a/ui/src/Trace.tsx +++ b/ui/src/Trace.tsx @@ -422,6 +422,7 @@ function Detail({ step, detail }: { step: string; detail: StepDetail }) { const sourceTok = asNumber(detail.prompt_sources); const attempts = asNumber(detail.attempts); const memories = asNumber(detail.memories); + const readAt = asStrings(detail.read_at); return (
{inTok !== null && <>{tokens(inTok)} tokens in} @@ -436,6 +437,15 @@ function Detail({ step, detail }: { step: string; detail: StepDetail }) { {cited} of {sources} sources cited )} + {/* A rework or re-explain answers from the record, re-read from + git at the commit each repository was read at - not from the + index as it stands now. */} + {readAt.length > 0 && ( + <> + {" · "} + read at + + )} {/* The second call the answer took. A call is retried at most once (`llm.Client`, attempts is 1 or 2) and the pipeline sends the key only past one, so a turn that went through first time says diff --git a/ui/src/TraceDetailMore.test.tsx b/ui/src/TraceDetailMore.test.tsx index 871815b8..c71e339c 100644 --- a/ui/src/TraceDetailMore.test.tsx +++ b/ui/src/TraceDetailMore.test.tsx @@ -32,6 +32,18 @@ describe("Trace, the remaining step details", () => { expect(container.querySelector(".trace-detail")?.textContent).toContain("3 of 12 sources cited · retried once"); }); + it("says which commits a basis re-read from the record came from", () => { + const { container } = strict( + , + ); + expect(container.querySelector(".trace-detail")?.textContent).toContain("2 of 5 sources cited · read at peeq 0123456loom abcdef0"); + }); + it("names a pinned thread's scope, a corpus-wide ask, and the repositories left out", () => { const { rerender } = strict( Date: Tue, 29 Sep 2026 21:32:39 +0200 Subject: [PATCH 2/3] Review fixes: split lines, marker patterns, lazy basis, deleted files - A window re-read from git that is larger than a chunk can be was an overlong line split into siblings; it reads back through its own chunk row or is missing, never as the whole line. - The permanence marker matches the question with case, curly apostrophes and spacing folded, and a "don't ... anymore" pattern matches its parts in order. - A follow-up reads what the basis is from the database; the text is read from git only once the turn is a rework. - A cited file deleted or renamed since is still read at its commit; a file the index now skips stays refused. --- AGENTS.md | 2 +- backend/internal/ask/memory_test.go | 24 +++ backend/internal/ask/pipeline.go | 10 ++ backend/internal/ask/rework_test.go | 41 +++++ backend/internal/ask/understand.go | 30 +++- backend/internal/httpapi/ask.go | 22 ++- backend/internal/httpapi/rework_test.go | 30 +++- backend/internal/httpapi/server.go | 3 + backend/internal/indexer/chunk.go | 7 + backend/internal/sourceview/sourceview.go | 25 ++- .../internal/sourceview/sourceview_test.go | 27 +++ .../internal/threads/source_identity_test.go | 51 +++++- backend/internal/threads/sources.go | 157 +++++++++++------- 13 files changed, 332 insertions(+), 97 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 94298d43..8f107cde 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -61,7 +61,7 @@ Rules, not description. Code is truth — implementation is discoverable, so it - **Thread is a record.** Follow-up adds an answer, never rewrites one. Correction is a new question. - **One language per thread** — its first question's. Everything a person reads follows it, replays included. The record decides, not the request. - **Rework ("summarize", "as a table", "zeichne ein Diagramm davon") answers from the previous answer and ITS OWN sources, no search.** First turn is never a rework. A basis missing even ONE source is refused — never summarised from survivors, never searched afresh; a fresh answer to "summarize" is a different answer dressed as a summary. Not "translate". -- **A turn's basis is recorded by repo, path, commit, lines — re-read from git, never by chunk row.** A poll re-inserts every chunk of a touched file under new ids; keyed on them, a minute-old answer's diagram was refused as "no longer indexed". Missing means git cannot produce it: purged repo, replaced snapshot, file now excluded. +- **A turn's basis is recorded by repo, path, commit, lines — re-read from git, never by chunk row.** A poll re-inserts every chunk of a touched file under new ids; keyed on them, a minute-old answer's diagram was refused as "no longer indexed". Missing means git cannot produce it: purged repo, replaced snapshot, file the index now skips — a file deleted or renamed since is still read at its commit. A window bigger than a chunk (split overlong line) is never read back whole. Text read only once the turn is a rework; every follow-up reads refs, not files. - **Previous answer reaches the answer prompt as context, never a source** — markers stripped, fenced off, never cited, its claims never restated as fact. "The flow" is the flow it described. Changes and release lanes get the question only. - **Recall reaches the model as ONE user message**, never a user/assistant pair — a prose assistant turn makes the model continue the conversation instead of returning JSON. - **Thread is a funnel: narrows, never widens.** Pin is a ceiling; "in all repos" under a pin is not honoured. A named repo the pin excludes is reported as outside — silent refusal to widen is the same quiet drop the rule exists to stop. diff --git a/backend/internal/ask/memory_test.go b/backend/internal/ask/memory_test.go index 02e15c23..e08d8b8d 100644 --- a/backend/internal/ask/memory_test.go +++ b/backend/internal/ask/memory_test.go @@ -274,6 +274,30 @@ func TestKeptRule_isWhatTheProductKeeps(t *testing.T) { } } +// TestKeptRule_readsTheMarkerTheWayTheReaderTypedIt: the prompt's own +// examples are patterns ("don't ... anymore"), a reader types a curly +// apostrophe, and a model quotes back either. The words must be the +// question's, in its order; spelling and spacing are not the test. +func TestKeptRule_readsTheMarkerTheWayTheReaderTypedIt(t *testing.T) { + rule := "Do not mention the library lerb-chooser-ui." + for _, c := range []struct { + question, marker string + kept bool + }{ + {"don’t mention lerb-chooser-ui anymore", "don't ... anymore", true}, + {"don't mention lerb-chooser-ui anymore", "don’t … anymore", true}, + {"don't mention it\nanymore", "don't mention it anymore", true}, + {"ne le mentionne plus jamais", "ne ... plus jamais", true}, + {"anymore, don't mention it", "don't ... anymore", false}, + {"mention lerb-chooser-ui", "...", false}, + } { + got := Understanding{Memory: rule, MemoryMarker: c.marker}.KeptRule(c.question) + if (got != "") != c.kept { + t.Errorf("%q with marker %q: kept %q, want kept: %v", c.question, c.marker, got, c.kept) + } + } +} + // TestPipeline_forgettingNeedsNoMarker: "show flowcharts again" lasts by // nature and carries no "from now on"; the gate is on keeping a rule, never // on dropping one. diff --git a/backend/internal/ask/pipeline.go b/backend/internal/ask/pipeline.go index 7fdf260f..60b929eb 100644 --- a/backend/internal/ask/pipeline.go +++ b/backend/internal/ask/pipeline.go @@ -214,6 +214,11 @@ type Thread struct { // which is how a rework tells a whole basis from a partial one. Sources []Source SourcesTotal int + // ReadBasis reads Sources with their text, for the one turn that needs + // it: a rework. Until then Sources says what the basis is and no more, + // because reading it means reading every file from git. Nil when Sources + // already carries the text. + ReadBasis func(context.Context) ([]Source, int, error) } // Run answers one question, or ends the turn by asking which of several @@ -361,6 +366,11 @@ func (p *Pipeline) Run(ctx context.Context, question string, audience Audience, // the whole of what it reads, and a search on "summarize" has nothing to // add but a second, different answer. if isRework(u, t) { + if t.ReadBasis != nil { + if t.Sources, t.SourcesTotal, err = t.ReadBasis(ctx); err != nil { + return Answer{}, nil, fmt.Errorf("read the previous answer's basis: %w", err) + } + } answer, err := p.answerRework(ctx, question, audience, lang, t, scope, ev) return answer, nil, err } diff --git a/backend/internal/ask/rework_test.go b/backend/internal/ask/rework_test.go index 319dc797..f5ce41b4 100644 --- a/backend/internal/ask/rework_test.go +++ b/backend/internal/ask/rework_test.go @@ -244,6 +244,47 @@ func TestUnderstandNamesRework(t *testing.T) { } } +// TestOnlyAReworkReadsTheBasis: reading the basis reads every file of it +// from git, so a follow-up carries only what the basis is until the turn +// turns out to be a rework. A read that fails fails the turn: answering +// "summarize" afresh would be a different answer dressed as a summary. +func TestOnlyAReworkReadsTheBasis(t *testing.T) { + refs := reworkThread() + full := refs.Sources + for i := range refs.Sources { + refs.Sources[i].Text = "" + } + reads := 0 + refs.ReadBasis = func(context.Context) ([]Source, int, error) { + reads++ + return twoSources(), len(full), nil + } + + c, prompt := reworkUpstream(t, reworkReply) + if _, _, err := reworkPipeline(t, c).Run(context.Background(), "summarize", AudienceBA, LanguageEN, refs, Events{}); err != nil { + t.Fatalf("Run: %v", err) + } + if reads != 1 || !strings.Contains(*prompt, "func issueGrant() {}") { + t.Errorf("reads = %d; the rework must answer from the text it read:\n%s", reads, *prompt) + } + + reads = 0 + p := newTestPipeline(t) + p.understander = NewUnderstander(twoStepUpstream(t, `{"intent":"how","terms":["t"],"code_terms":["c"],"repos":[]}`, "x")) + if _, _, err := p.Run(context.Background(), "and where is it checked?", AudienceBA, LanguageEN, refs, Events{}); err != nil { + t.Fatalf("Run: %v", err) + } + if reads != 0 { + t.Errorf("an ordinary follow-up read the basis %d times", reads) + } + + refs.ReadBasis = func(context.Context) ([]Source, int, error) { return nil, 0, errors.New("git gone") } + c, _ = reworkUpstream(t, reworkReply) + if _, _, err := reworkPipeline(t, c).Run(context.Background(), "summarize", AudienceBA, LanguageEN, refs, Events{}); err == nil || !strings.Contains(err.Error(), "git gone") { + t.Errorf("err = %v, want the failed read", err) + } +} + // TestAReworkSaysWhichCommitsItsBasisWasReadAt: the basis is re-read from // git, not from the index as it is now, and the trace says where from. func TestAReworkSaysWhichCommitsItsBasisWasReadAt(t *testing.T) { diff --git a/backend/internal/ask/understand.go b/backend/internal/ask/understand.go index 8ec534ab..d5bbb7dd 100644 --- a/backend/internal/ask/understand.go +++ b/backend/internal/ask/understand.go @@ -172,14 +172,40 @@ func (u *Understanding) standingOnly(question string) bool { if u.Memory == "" { return false } - marker := strings.ToLower(strings.TrimSpace(u.MemoryMarker)) - if marker != "" && strings.Contains(strings.ToLower(question), marker) { + if saysItLasts(question, u.MemoryMarker) { return false } u.Memory, u.MemoryScope, u.MemoryReplaces = "", "", nil return true } +// markerFold is what the check forgives: case, the apostrophe a keyboard +// curls, an ellipsis typed as one character, and spacing. +var markerFold = strings.NewReplacer("’", "'", "‘", "'", "`", "'", "…", "...") + +// saysItLasts reports whether marker is words of the question, in its +// order. A marker may be a pattern the prompt showed — "don't ... anymore", +// "ne ... plus jamais" — whose parts must each stand in the question, one +// after the other; a marker of nothing but dots is no marker. +func saysItLasts(question, marker string) bool { + norm := func(s string) string { + return strings.Join(strings.Fields(markerFold.Replace(strings.ToLower(s))), " ") + } + q, found := norm(question), false + for _, part := range strings.Split(norm(marker), "...") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + i := strings.Index(q, part) + if i < 0 { + return false + } + q, found = q[i+len(part):], true + } + return found +} + // KeptRule is the rule the product keeps from this understanding of // question: Memory, unless standingOnly drops it. What the intent eval // grades, so it grades what the reader would get. diff --git a/backend/internal/httpapi/ask.go b/backend/internal/httpapi/ask.go index 33a95dcb..139a833e 100644 --- a/backend/internal/httpapi/ask.go +++ b/backend/internal/httpapi/ask.go @@ -548,20 +548,24 @@ func (s *Server) handleAsk(w http.ResponseWriter, r *http.Request) { slog.Error("read last turn failed", "err", err) } else if ok { prior.Question, prior.Answer = last.Question, last.Answer - // And what that answer was written from, for a rework. Read - // here rather than once the understanding has said the turn is - // one: the pipeline has no thread store, and one SELECT per - // follow-up is the cost. A read that fails fails the request, - // unlike the reads above: a turn that goes on without the - // basis answers "summarize" afresh, which is worse than no - // answer. - sources, total, err := s.deps.Threads.Sources(ctx, u.Subject, last.ID) + // And what that answer was written from, for a rework: what + // the sources are, from one SELECT, and their text only once + // the understanding has said the turn IS a rework — reading + // every file from git is the cost of a rework, not of every + // follow-up. A read that fails fails the request, unlike the + // reads above: a turn that goes on without the basis answers + // "summarize" afresh, which is worse than no answer. + refs, err := s.deps.Threads.SourceRefs(ctx, u.Subject, last.ID) if err != nil { slog.Error("read last turn's sources failed", "err", err) http.Error(w, "internal server error", http.StatusInternalServerError) return } - prior.Sources, prior.SourcesTotal = sources, total + subject, id := u.Subject, last.ID + prior.Sources, prior.SourcesTotal = refs, len(refs) + prior.ReadBasis = func(ctx context.Context) ([]ask.Source, int, error) { + return s.deps.Threads.Sources(ctx, subject, id) + } } } diff --git a/backend/internal/httpapi/rework_test.go b/backend/internal/httpapi/rework_test.go index 7dc8321a..e44871ce 100644 --- a/backend/internal/httpapi/rework_test.go +++ b/backend/internal/httpapi/rework_test.go @@ -54,17 +54,22 @@ func TestAsk_aFollowUpAfterAPollStillHasTheWholeBasis(t *testing.T) { } postAsk(t, deps, fmt.Sprintf(`{"question":"zeichne ein diagramm des ablaufs","audience":"ba","thread_id":%q}`, list[0].PublicID)) - if a.gotThread.SourcesTotal != 1 || len(a.gotThread.Sources) != 1 { - t.Fatalf("basis %d of %d, want whole", len(a.gotThread.Sources), a.gotThread.SourcesTotal) + if a.gotThread.ReadBasis == nil { + t.Fatal("a follow-up carries no way to read its basis") } - if got := a.gotThread.Sources[0].Text; got != "func Bypass() {\n}" { + sources, total, err := a.gotThread.ReadBasis(context.Background()) + if err != nil || total != 1 || len(sources) != 1 { + t.Fatalf("basis %d of %d (%v), want whole", len(sources), total, err) + } + if got := sources[0].Text; got != "func Bypass() {\n}" { t.Errorf("text = %q, want lines 2-3 at the commit the answer read", got) } } // TestAsk_aFollowUpCarriesThePreviousAnswersSources: a rework answers from -// the previous turn's own basis, so the handler reads it with the previous -// question and answer, whole or not — the count says which. +// the previous turn's own basis, so the handler hands over what it is and a +// way to read it, whole or not — the count says which. The text is read only +// when asked for: every follow-up needs the refs, only a rework the files. func TestAsk_aFollowUpCarriesThePreviousAnswersSources(t *testing.T) { db := askDB(t) chunkID := seedChunk(t, db) @@ -80,11 +85,18 @@ func TestAsk_aFollowUpCarriesThePreviousAnswersSources(t *testing.T) { } postAsk(t, deps, fmt.Sprintf(`{"question":"summarize","audience":"ba","thread_id":%q}`, list[0].PublicID)) - if len(a.gotThread.Sources) != 1 || a.gotThread.Sources[0].ChunkID != chunkID { - t.Errorf("sources = %+v, want the one chunk the index still holds", a.gotThread.Sources) + if a.gotThread.SourcesTotal != 2 || len(a.gotThread.Sources) != 2 || a.gotThread.Sources[0].Text != "" { + t.Errorf("refs = %+v (%d), want the two the record holds, unread", a.gotThread.Sources, a.gotThread.SourcesTotal) + } + sources, total, err := a.gotThread.ReadBasis(context.Background()) + if err != nil { + t.Fatalf("read basis: %v", err) + } + if len(sources) != 1 || sources[0].ChunkID != chunkID { + t.Errorf("sources = %+v, want the one chunk the index still holds", sources) } - if a.gotThread.SourcesTotal != 2 { - t.Errorf("sources total = %d, want the two the record holds", a.gotThread.SourcesTotal) + if total != 2 { + t.Errorf("sources total = %d, want the two the record holds", total) } } diff --git a/backend/internal/httpapi/server.go b/backend/internal/httpapi/server.go index 570a224a..fcae8f91 100644 --- a/backend/internal/httpapi/server.go +++ b/backend/internal/httpapi/server.go @@ -80,6 +80,9 @@ type Threads interface { // and undo survive a reload. SetMemory(ctx context.Context, messageID, memoryID int64) error Sources(ctx context.Context, subject string, messageID int64) (sources []ask.Source, total int, err error) + // SourceRefs is what an answer's basis is, without the text: one read of + // the database, where Sources reads every file from git. + SourceRefs(ctx context.Context, subject string, messageID int64) ([]ask.Source, error) // SaveUsage records the paid calls one turn made, however it ended. SaveUsage(ctx context.Context, messageID int64, calls []usage.Call) error // SaveFollowups records what the finished answer offered to ask next. diff --git a/backend/internal/indexer/chunk.go b/backend/internal/indexer/chunk.go index 50742bc0..a66308c9 100644 --- a/backend/internal/indexer/chunk.go +++ b/backend/internal/indexer/chunk.go @@ -449,6 +449,13 @@ func windows(lines []string, from, to int, opts ChunkOptions) []span { // thousands of tokens, and sending it would fail the request — losing the whole // FILE, not just that line. Cutting it here keeps every byte in the index and // costs only a boundary in the middle of a line nobody reads anyway. +// Splits reports whether a window of raw source is one the chunker cuts into +// siblings rather than stores whole: what reads a window back needs to know +// that no single chunk held it. +func (o ChunkOptions) Splits(raw string) bool { + return len(splitOverlongLine(raw, o.MaxTokens)) > 1 +} + func splitOverlongLine(raw string, maxTokens int) []string { if maxTokens <= 0 || estimateTokens(raw) <= maxTokens { return []string{raw} diff --git a/backend/internal/sourceview/sourceview.go b/backend/internal/sourceview/sourceview.go index 8c605d5d..b75d5714 100644 --- a/backend/internal/sourceview/sourceview.go +++ b/backend/internal/sourceview/sourceview.go @@ -81,6 +81,23 @@ func New(db *sql.DB, git FileReader, maxBytes int) *Service { // was last indexed at" — citations recorded before the commit travelled with // them have none. func (s *Service) Read(ctx context.Context, repo, path, sha string) (File, error) { + return s.read(ctx, repo, path, sha, false) +} + +// ReadRecorded is Read for a thread's own record: a file an answer was +// written from, at the commit it was read at. A path the index no longer +// lists — deleted or renamed since — is still served there, because the +// answer was read from it; a path the index now SKIPS is not, because that +// verdict (secret, excluded) is exactly what the files row is the permission +// for. The commit is required: the record always carries one. +func (s *Service) ReadRecorded(ctx context.Context, repo, path, sha string) (File, error) { + if sha == "" { + return File{}, fmt.Errorf("%w: a recorded source without its commit", ErrInvalid) + } + return s.read(ctx, repo, path, sha, true) +} + +func (s *Service) read(ctx context.Context, repo, path, sha string, recorded bool) (File, error) { if err := validatePath(path); err != nil { return File{}, err } @@ -109,10 +126,12 @@ func (s *Service) Read(ctx context.Context, repo, path, sha string) (File, error var indexedSHA, skipReason string err = s.db.QueryRowContext(ctx, `SELECT sha, skip_reason FROM files WHERE repo = ? AND path = ?`, repo, path).Scan(&indexedSHA, &skipReason) - if errors.Is(err, sql.ErrNoRows) { + switch { + case errors.Is(err, sql.ErrNoRows) && !recorded: return File{}, fmt.Errorf("%w: %s/%s is not indexed", ErrNotFound, repo, path) - } - if err != nil { + case errors.Is(err, sql.ErrNoRows): + // Gone from the index since the answer; the commit still holds it. + case err != nil: return File{}, fmt.Errorf("look up %s/%s: %w", repo, path, err) } if skipReason != "" { diff --git a/backend/internal/sourceview/sourceview_test.go b/backend/internal/sourceview/sourceview_test.go index 3ce6d582..30791121 100644 --- a/backend/internal/sourceview/sourceview_test.go +++ b/backend/internal/sourceview/sourceview_test.go @@ -104,6 +104,33 @@ func newFixture(t *testing.T, maxBytes int) fixture { return fixture{svc: New(db, client, maxBytes).WithCommits(client), db: db, first: first, second: second} } +// TestReadRecorded_servesAFileTheIndexNoLongerListsButNeverOneItSkips: an +// answer read internal/a.go; a later poll deleted it and purged the row. +// The record still reads it at its commit. A file the index now SKIPS +// stays refused — that verdict is the permission — and so does a record +// with no commit. +func TestReadRecorded_servesAFileTheIndexNoLongerListsButNeverOneItSkips(t *testing.T) { + f := newFixture(t, 1<<20) + ctx := context.Background() + if _, err := f.db.Exec(`DELETE FROM files WHERE path = 'internal/a.go'`); err != nil { + t.Fatal(err) + } + + got, err := f.svc.ReadRecorded(ctx, "peeq", "internal/a.go", f.first) + if err != nil || !strings.Contains(got.Content, "func One") { + t.Fatalf("ReadRecorded = %q, %v", got.Content, err) + } + if _, err := f.svc.Read(ctx, "peeq", "internal/a.go", f.first); !errors.Is(err, ErrNotFound) { + t.Errorf("the viewer serves an unlisted path: %v", err) + } + if _, err := f.svc.ReadRecorded(ctx, "peeq", "config/prod.env", f.first); !errors.Is(err, ErrNotFound) { + t.Errorf("a skipped file was served: %v", err) + } + if _, err := f.svc.ReadRecorded(ctx, "peeq", "internal/a.go", ""); !errors.Is(err, ErrInvalid) { + t.Errorf("no commit: %v", err) + } +} + func TestRead_showsTheFileAtTheCitedCommitNotTheBranchHead(t *testing.T) { // Given f := newFixture(t, 1<<20) diff --git a/backend/internal/threads/source_identity_test.go b/backend/internal/threads/source_identity_test.go index 80f85a6e..68bf554a 100644 --- a/backend/internal/threads/source_identity_test.go +++ b/backend/internal/threads/source_identity_test.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "fmt" + "strings" "testing" "time" @@ -20,7 +21,7 @@ type fakeEvidence struct { reads int } -func (f *fakeEvidence) Read(_ context.Context, repo, path, sha string) (sourceview.File, error) { +func (f *fakeEvidence) ReadRecorded(_ context.Context, repo, path, sha string) (sourceview.File, error) { f.reads++ body, ok := f.files[repo+"|"+path+"|"+sha] if !ok { @@ -106,13 +107,17 @@ func TestASourceGitCannotProduceIsMissing(t *testing.T) { } } -func TestSplitSiblingsOfOneLineAreOneSource(t *testing.T) { - // An overlong line is stored as sibling chunks sharing one line range. - // Read back from git they are the same text; counting them twice would - // report a basis short by one that is in fact whole. - s, ctx, threadID, _ := newThreadStore(t) - s.WithEvidence(&fakeEvidence{files: map[string]string{"peeq|min.js|deadbeef": "x=1\n"}}) +func TestASplitLineIsNeverReadBackWhole(t *testing.T) { + // An overlong line is stored as sibling chunks sharing one line range, + // each holding a part. Read from git the range is the whole line — + // hundreds of kilobytes of minified code for one sibling an answer + // used. Each sibling reads back as its own chunk while that exists, and + // is missing once it does not: never the whole line. + s, ctx, threadID, db := newThreadStore(t) + long := strings.Repeat("x", 4*chunkCeiling.MaxTokens*3) + s.WithEvidence(&fakeEvidence{files: map[string]string{"peeq|min.js|deadbeef": long + "\n"}}) msg, _ := s.AddQuestion(ctx, threadID, "ba", "en", "q", 0) + insertChunk(t, db, 1, "peeq", "min.js", "part one") if err := s.SaveSources(ctx, msg.ID, []ask.Source{ {ChunkID: 1, Repo: "peeq", Path: "min.js", SHA: "deadbeef", StartLine: 1, EndLine: 1, Reason: "hit"}, {ChunkID: 2, Repo: "peeq", Path: "min.js", SHA: "deadbeef", StartLine: 1, EndLine: 1, Reason: "hit"}, @@ -124,8 +129,36 @@ func TestSplitSiblingsOfOneLineAreOneSource(t *testing.T) { if err != nil { t.Fatalf("sources: %v", err) } - if total != 1 || len(got) != 1 || got[0].Text != "x=1" { - t.Errorf("total %d, got %+v; want one source", total, got) + if total != 2 || len(got) != 1 || got[0].Text != "part one" { + t.Errorf("total %d, got %+v; want the sibling whose chunk remains, alone", total, got) + } +} + +func TestSourceRefsReadNoGit(t *testing.T) { + // Every follow-up reads what the basis IS; only a rework reads its text. + s, ctx, threadID, _ := newThreadStore(t) + ev := &fakeEvidence{} + s.WithEvidence(ev) + msg, _ := s.AddQuestion(ctx, threadID, "ba", "en", "q", 0) + if err := s.SaveSources(ctx, msg.ID, []ask.Source{ + {ChunkID: 1, Repo: "peeq", Path: "a.go", SHA: "deadbeef", StartLine: 1, EndLine: 2, Reason: "hit"}, + {Kind: ask.SourceCommit, CommitID: 5, Repo: "peeq", SHA: "aaa1111", Reason: "hit"}, + }); err != nil { + t.Fatalf("save sources: %v", err) + } + + got, err := s.SourceRefs(ctx, testSubject, msg.ID) + if err != nil { + t.Fatalf("refs: %v", err) + } + if len(got) != 2 || got[0].Repo != "peeq" || got[0].Path != "a.go" || !got[1].IsCommit() || got[1].SHA != "aaa1111" { + t.Errorf("refs = %+v", got) + } + if ev.reads != 0 { + t.Errorf("%d git reads for the refs, want none", ev.reads) + } + if other, _ := s.SourceRefs(ctx, "someone-else", msg.ID); len(other) != 0 { + t.Errorf("a foreign subject read %d refs", len(other)) } } diff --git a/backend/internal/threads/sources.go b/backend/internal/threads/sources.go index a32ff4c2..bf277fc3 100644 --- a/backend/internal/threads/sources.go +++ b/backend/internal/threads/sources.go @@ -11,15 +11,16 @@ import ( "time" "github.com/trick77/rongo/internal/ask" + "github.com/trick77/rongo/internal/indexer" "github.com/trick77/rongo/internal/sourceview" ) // Evidence is the checkout as the record reads it back: a file at the commit // an answer read it at, and a commit an answer was written from. The source -// viewer satisfies it, so a rework reads exactly what a citation opens — -// same permission, same redaction. +// viewer satisfies it, so a rework reads what a citation opens — same +// redaction, and a file the index now skips stays refused. type Evidence interface { - Read(ctx context.Context, repo, path, sha string) (sourceview.File, error) + ReadRecorded(ctx context.Context, repo, path, sha string) (sourceview.File, error) RecordedCommit(ctx context.Context, repo, sha string) (sourceview.Commit, error) } @@ -31,6 +32,12 @@ func (s *Store) WithEvidence(e Evidence) *Store { return s } +// chunkCeiling is the chunker's own ceiling. A window read back from git +// larger than a chunk can be was an overlong line the chunker split into +// siblings; the whole line is not what any one of them held, and it can be +// hundreds of kilobytes of minified code. +var chunkCeiling = indexer.DefaultChunkOptions() + // SaveSources records what an answer was actually written from: every source // by what it is — repository, path, commit, lines — beside the row it came // from. The row id is not stable across a re-index; the commit is. @@ -68,25 +75,56 @@ type recorded struct { hop int } -// key is what makes two rows one source. Siblings of one overlong line share -// a line range and read back as the same text, so they are one; a row with -// no identity is its own id. -func (r recorded) key() string { - switch { - case r.commitID != 0 && r.repo != "": - return "commit|" + r.repo + "|" + r.sha - case r.commitID != 0: - return fmt.Sprintf("commit#%d", r.commitID) - case r.repo != "": - return fmt.Sprintf("file|%s|%s|%s|%d|%d", r.repo, r.path, r.sha, r.start, r.end) - } - return fmt.Sprintf("chunk#%d", r.chunkID) +// records reads a message's rows, chunks by hop then id, commits after. +// A message that does not belong to a thread owned by subject has none. +func (s *Store) records(ctx context.Context, subject string, messageID int64) ([]recorded, error) { + rows, err := s.db.QueryContext(ctx, ` + SELECT ms.chunk_id, ms.commit_id, ms.repo, ms.path, ms.sha, ms.start_line, ms.end_line, ms.symbol, ms.reason, ms.hop + FROM message_sources ms + JOIN messages m ON m.id = ms.message_id + JOIN threads t ON t.id = m.thread_id + WHERE ms.message_id = ? AND t.user_subject = ? + ORDER BY ms.commit_id <> 0, ms.hop, ms.chunk_id, ms.commit_id`, messageID, subject) + if err != nil { + return nil, fmt.Errorf("read sources: %w", err) + } + defer func() { _ = rows.Close() }() + var out []recorded + for rows.Next() { + var r recorded + if err := rows.Scan(&r.chunkID, &r.commitID, &r.repo, &r.path, &r.sha, &r.start, &r.end, &r.symbol, &r.reason, &r.hop); err != nil { + return nil, fmt.Errorf("scan source: %w", err) + } + out = append(out, r) + } + return out, rows.Err() +} + +// SourceRefs is the record of an answer's basis without its text: what the +// sources are, read from the database alone. Every follow-up needs this +// much — whether there is a basis, which repositories it spans — and only a +// rework needs the text, so the git reads wait for Sources. +func (s *Store) SourceRefs(ctx context.Context, subject string, messageID int64) ([]ask.Source, error) { + recs, err := s.records(ctx, subject, messageID) + if err != nil { + return nil, err + } + out := make([]ask.Source, 0, len(recs)) + for _, r := range recs { + src := ask.Source{ChunkID: r.chunkID, Repo: r.repo, Path: r.path, SHA: r.sha, Symbol: r.symbol, + StartLine: r.start, EndLine: r.end, Reason: r.reason, Hop: r.hop} + if r.commitID != 0 { + src = ask.Source{Kind: ask.SourceCommit, CommitID: r.commitID, Repo: r.repo, SHA: r.sha, Reason: r.reason, Hop: r.hop} + } + out = append(out, src) + } + return out, nil } // Sources reads an answer's basis back, chunks ordered by hop, then commits // newest first. A source is read from git at the commit it was read at, so a // re-index since the answer changes nothing; one git can no longer produce — -// a purged repository, a replaced snapshot, a file now excluded — is +// a purged repository, a replaced snapshot, a file the index now skips — is // silently omitted. A message that does not belong to a thread owned by // subject yields an empty slice, the same shape as "no sources yet", never // another user's evidence. @@ -98,31 +136,8 @@ func (r recorded) key() string { // by SOME of its evidence looks identical to a whole one if only the // resolved slice is visible. func (s *Store) Sources(ctx context.Context, subject string, messageID int64) (sources []ask.Source, total int, err error) { - rows, err := s.db.QueryContext(ctx, ` - SELECT ms.chunk_id, ms.commit_id, ms.repo, ms.path, ms.sha, ms.start_line, ms.end_line, ms.symbol, ms.reason, ms.hop - FROM message_sources ms - JOIN messages m ON m.id = ms.message_id - JOIN threads t ON t.id = m.thread_id - WHERE ms.message_id = ? AND t.user_subject = ? - ORDER BY ms.hop, ms.chunk_id, ms.commit_id`, messageID, subject) + recs, err := s.records(ctx, subject, messageID) if err != nil { - return nil, 0, fmt.Errorf("read sources: %w", err) - } - var recs []recorded - seen := map[string]bool{} - for rows.Next() { - var r recorded - if err := rows.Scan(&r.chunkID, &r.commitID, &r.repo, &r.path, &r.sha, &r.start, &r.end, &r.symbol, &r.reason, &r.hop); err != nil { - _ = rows.Close() - return nil, 0, fmt.Errorf("scan source: %w", err) - } - if k := r.key(); !seen[k] { - seen[k] = true - recs = append(recs, r) - } - } - _ = rows.Close() - if err := rows.Err(); err != nil { return nil, 0, err } @@ -166,29 +181,11 @@ type fileRead struct { // chunk it came from while that row still exists — a chunk id is never // reused, so a row that still joins holds the text it held. func (s *Store) chunkSource(ctx context.Context, r recorded, files map[string]fileRead) (ask.Source, bool, error) { - if r.repo != "" && s.evidence != nil { - k := r.repo + "|" + r.path + "|" + r.sha - f, ok := files[k] - if !ok { - file, err := s.evidence.Read(ctx, r.repo, r.path, r.sha) - f = fileRead{branch: file.Branch, err: err} - if err == nil { - // Split the way the chunker splits, so a window is the lines - // it was cut from. - f.lines = strings.Split(strings.TrimSuffix(file.Content, "\n"), "\n") - } - files[k] = f - } - if f.err == nil && r.start >= 1 && r.end >= r.start && r.end <= len(f.lines) { - return ask.Source{ - ChunkID: r.chunkID, Repo: r.repo, Branch: f.branch, Path: r.path, SHA: r.sha, Symbol: r.symbol, - StartLine: r.start, EndLine: r.end, Text: strings.Join(f.lines[r.start-1:r.end], "\n"), - Reason: r.reason, Hop: r.hop, - }, true, nil - } - if f.err != nil { - slog.Debug("source not readable from git", "repo", r.repo, "path", r.path, "sha", r.sha, "err", f.err) - } + if text, branch, ok := s.window(ctx, r, files); ok { + return ask.Source{ + ChunkID: r.chunkID, Repo: r.repo, Branch: branch, Path: r.path, SHA: r.sha, Symbol: r.symbol, + StartLine: r.start, EndLine: r.end, Text: text, Reason: r.reason, Hop: r.hop, + }, true, nil } src := ask.Source{ChunkID: r.chunkID, Reason: r.reason, Hop: r.hop} err := s.db.QueryRowContext(ctx, ` @@ -206,6 +203,38 @@ func (s *Store) chunkSource(ctx context.Context, r recorded, files map[string]fi return src, true, nil } +// window is r's lines read from git at its commit, or false: no identity, +// no checkout, git cannot produce the file, or the window is larger than a +// chunk can be — an overlong line split into siblings, which only the +// chunk rows hold part by part. +func (s *Store) window(ctx context.Context, r recorded, files map[string]fileRead) (string, string, bool) { + if r.repo == "" || s.evidence == nil { + return "", "", false + } + k := r.repo + "|" + r.path + "|" + r.sha + f, ok := files[k] + if !ok { + file, err := s.evidence.ReadRecorded(ctx, r.repo, r.path, r.sha) + f = fileRead{branch: file.Branch, err: err} + if err == nil { + // Split the way the chunker splits, so a window is the lines + // it was cut from. + f.lines = strings.Split(strings.TrimSuffix(file.Content, "\n"), "\n") + } else { + slog.Debug("source not readable from git", "repo", r.repo, "path", r.path, "sha", r.sha, "err", err) + } + files[k] = f + } + if f.err != nil || r.start < 1 || r.end < r.start || r.end > len(f.lines) { + return "", "", false + } + text := strings.Join(f.lines[r.start-1:r.end], "\n") + if chunkCeiling.Splits(text) { + return "", "", false + } + return text, f.branch, true +} + // commitSource resolves a commit source: from the commit lane while it still // holds the commit, else from git — the lane's window slides with every push. func (s *Store) commitSource(ctx context.Context, r recorded) (ask.Source, bool, error) { From 76dde11ec05be587f6d495b571bcfc6f68e38387 Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 29 Sep 2026 21:33:39 +0200 Subject: [PATCH 3/3] Keep doc comments on the methods they document --- backend/internal/ask/understand.go | 4 ++-- backend/internal/indexer/chunk.go | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/backend/internal/ask/understand.go b/backend/internal/ask/understand.go index d5bbb7dd..235da1aa 100644 --- a/backend/internal/ask/understand.go +++ b/backend/internal/ask/understand.go @@ -159,8 +159,6 @@ func (ids *IDs) UnmarshalJSON(b []byte) error { return nil } -// Directive is what the understanding read as a standing instruction, or -// nothing. // standingOnly drops a rule the question gives no word of the reader's for. // A rule lasts because the reader said so — "ab jetzt", "never" — and the // model has to quote that word; a quote the question does not hold is the @@ -214,6 +212,8 @@ func (u Understanding) KeptRule(question string) string { return u.Memory } +// Directive is what the understanding read as a standing instruction, or +// nothing. func (u Understanding) Directive() memory.Directive { return memory.Directive{ Text: u.Memory, diff --git a/backend/internal/indexer/chunk.go b/backend/internal/indexer/chunk.go index a66308c9..19d0ffa3 100644 --- a/backend/internal/indexer/chunk.go +++ b/backend/internal/indexer/chunk.go @@ -440,6 +440,13 @@ func windows(lines []string, from, to int, opts ChunkOptions) []span { return out } +// Splits reports whether a window of raw source is one the chunker cuts into +// siblings rather than stores whole: what reads a window back needs to know +// that no single chunk held it. +func (o ChunkOptions) Splits(raw string) bool { + return len(splitOverlongLine(raw, o.MaxTokens)) > 1 +} + // splitOverlongLine cuts a window that is one enormous line into pieces the // embedding endpoint will accept. Normal windows come back unchanged. // @@ -449,13 +456,6 @@ func windows(lines []string, from, to int, opts ChunkOptions) []span { // thousands of tokens, and sending it would fail the request — losing the whole // FILE, not just that line. Cutting it here keeps every byte in the index and // costs only a boundary in the middle of a line nobody reads anyway. -// Splits reports whether a window of raw source is one the chunker cuts into -// siblings rather than stores whole: what reads a window back needs to know -// that no single chunk held it. -func (o ChunkOptions) Splits(raw string) bool { - return len(splitOverlongLine(raw, o.MaxTokens)) > 1 -} - func splitOverlongLine(raw string, maxTokens int) []string { if maxTokens <= 0 || estimateTokens(raw) <= maxTokens { return []string{raw}