diff --git a/mcpsrv/browse.go b/mcpsrv/browse.go index 632c184fc0a1d4f8b245fbfaf5dc4c78d5ed567f..43b81ff2ced0d8a2f427716425cdd60e78bce79e 100644 --- a/mcpsrv/browse.go +++ b/mcpsrv/browse.go @@ -528,7 +528,16 @@ // A garbage or unknown cursor is an ordinary miss: browse wraps it in // ErrRefNotFound the same as any other ref it cannot resolve, and refMiss // reads that sentinel here. A genuine failure of the store still takes the // protocol arm below. - return out, refMiss(err, tool, noSuchRef(in.databaseRef, ref)) + // + // The sentence names whichever of ref and from the call actually supplied: + // with a cursor set, ref is "" (it was never resolved, see above), and a + // refusal that named it anyway would send the caller looking for a typo in + // a branch name it never typed. + missing := noSuchRef(in.databaseRef, ref) + if from != "" { + missing = noSuchCursor(in.databaseRef, from) + } + return out, refMiss(err, tool, missing) } out.Ref = ref @@ -741,6 +750,14 @@ // noSuchRef is an ordinary answer about a database the caller *can* see, and // says so by naming it — the opposite of the masked sentence above. func noSuchRef(ref databaseRef, named string) string { return fmt.Sprintf("%s has no branch or commit %q; list_branches names its branches", ref, named) +} + +// noSuchCursor is get_commit_log's miss for a from-cursor that names no commit: +// the counterpart of noSuchRef for the one call where a page can be continued by +// hash instead of by ref, so the refusal points at "from" rather than sending the +// caller to list_branches over a value that was never a branch name. +func noSuchCursor(ref databaseRef, from string) string { + return fmt.Sprintf("%s has no commit %q to continue from; from takes the \"next\" hash a previous page of get_commit_log returned", ref, from) } func noSuchTable(ref databaseRef, at, table string) string { diff --git a/mcpsrv/browse_test.go b/mcpsrv/browse_test.go index cdceb5053dfa12af3f611b215d77e5c714e3051b..7f9e03352c54de316c805dadb52571563e628c43 100644 --- a/mcpsrv/browse_test.go +++ b/mcpsrv/browse_test.go @@ -500,10 +500,19 @@ for _, from := range []string{"not-a-hash", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"} { t.Run(from, func(t *testing.T) { res := call(t, anonSession(t), "get_commit_log", args("notes", "from", from)) require.True(t, res.IsError, "a bad cursor is a tool result, not a protocol error") - assert.NotContains(t, errorText(res), "no database", "this is not the masked not-found") + + text := errorText(res) + assert.NotContains(t, text, "no database", "this is not the masked not-found") + assert.Contains(t, text, from, "the refusal names the cursor the caller actually sent") + assert.Contains(t, text, "~alice/notes") }) } } + +// TestAnUnresolvableRefIsAnAnswerAboutTheDatabase (above) already pins the other +// half of this: a call that names a ref rather than a cursor gets a refusal +// naming *that* ref. This test exists so the two halves are asserted in the same +// place a reader looking for "does the cursor case name the cursor" would look. // --- the visibility matrix --------------------------------------------------