Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions docs/specs/product/compass.md
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,29 @@ read-modify-write on the message row.
- **Then** the server returns not-found — the same result as a nonexistent ask,
never a distinct not-authorized error.

### Requirement: An ask may name the issue or pull request it concerns

An `Ask` MAY carry `ref`, one issue or pull request reference (`RIG-12` or
`owner/repo#12`). The server SHALL store it in canonical form (a forge ref
lowercased) and SHALL reject a `ref` that is not exactly one reference with
invalid-argument, storing nothing. The server SHALL return the stored `ref` on
every read of the ask, and `RespondToAsk` SHALL keep it on the answered ask and
on the answer's ask snapshot. The server SHALL NOT fill a default `ref`.

#### Scenario: An ask ref survives the answer

- **Given** an ask posted with `ref` `RigelBuild/Compass#12`
- **When** a member answers it with `RespondToAsk`
- **Then** `ListMessages` returns the ask and the answer snapshot, each with
`ref` `rigelbuild/compass#12`.

#### Scenario: A malformed ask ref is refused

- **Given** an ask whose `ref` is `see RIG-12`
- **When** the caller posts it with `PostMessage`
- **Then** the server returns invalid-argument and the channel has no new
message.

### Requirement: Channel membership carries a separate subscribe opt-in

`UpdateChannelMembers` SHALL manage both the channel's member set and each
Expand Down
19 changes: 16 additions & 3 deletions go/gen/compass/v1/comms.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion go/internal/comms/agent_update_helper_pgtest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,12 @@ func updateBlocksFromWire(blocks []*compassv1.MessageBlock) ([]store.MessageBloc
if body == nil {
return nil, connect.NewError(connect.CodeInvalidArgument, errEmptyBlock)
}
out = append(out, store.MessageBlock{Ask: &store.Ask{AskID: body.Ask.GetAskId(), Questions: askQuestionsFromWire(body.Ask)}})
ask, err := askFromWire(body.Ask)
if err != nil {
return nil, err
}
ask.AskID = body.Ask.GetAskId()
out = append(out, store.MessageBlock{Ask: ask})
case *compassv1.MessageBlock_AskAnswer:
return nil, connect.NewError(connect.CodeInvalidArgument, errServerOwnedBlock)
default:
Expand Down
47 changes: 45 additions & 2 deletions go/internal/comms/ask_mapping_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"strconv"
"testing"

"connectrpc.com/connect"
"google.golang.org/protobuf/reflect/protoreflect"

compassv1 "github.com/RigelBuild/compass/go/gen/compass/v1"
Expand All @@ -40,7 +41,7 @@ func fieldSet(names ...protoreflect.Name) map[protoreflect.Name]struct{} {

var (
askOwnership = askFieldOwnership{
content: fieldSet("questions"),
content: fieldSet("questions", "ref"),
serverOwned: fieldSet("ask_id", "answered"),
}
askQuestionOwnership = askFieldOwnership{
Expand Down Expand Up @@ -76,7 +77,14 @@ func TestAskFromWireDropsEveryServerOwnedField(t *testing.T) {
t.Fatalf("reflection populated %d questions, want 2", len(wire.GetQuestions()))
}

got := askFromWire(wire)
// Reflection fills ref with text that is not a ref; use a valid mixed-case
// one so the content assertion also checks canonicalization.
wire.Ref = "RigelBuild/Compass#12"

got, err := askFromWire(wire)
if err != nil {
t.Fatalf("askFromWire: %v", err)
}

// Server-owned: dropped.
if got.AskID != "" {
Expand All @@ -87,6 +95,9 @@ func TestAskFromWireDropsEveryServerOwnedField(t *testing.T) {
}

// Content: survives, with the values reflection set.
if got.Ref != "rigelbuild/compass#12" {
t.Errorf("Ref = %q, want the canonical %q", got.Ref, "rigelbuild/compass#12")
}
if len(got.Questions) != len(wire.GetQuestions()) {
t.Fatalf("Questions length = %d, want %d", len(got.Questions), len(wire.GetQuestions()))
}
Expand Down Expand Up @@ -268,3 +279,35 @@ func nonZeroScalar(t *testing.T, fd protoreflect.FieldDescriptor, suffix string)
return protoreflect.Value{}
}
}

// TestAskFromWireRef pins the ref edge: an absent ref stays empty, a ref is
// stored canonical, and anything that is not exactly one ref is refused.
func TestAskFromWireRef(t *testing.T) {
for _, tc := range []struct {
name, in, want string
wantErr bool
}{
{name: "absent", in: "", want: ""},
{name: "tracker key", in: "RIG-12", want: "RIG-12"},
{name: "forge ref lowercased", in: "Owner/Repo#7", want: "owner/repo#7"},
{name: "free text", in: "see RIG-12", wantErr: true},
{name: "two refs", in: "RIG-12 RIG-13", wantErr: true},
{name: "url", in: "https://github.com/o/r/pull/1", wantErr: true},
} {
t.Run(tc.name, func(t *testing.T) {
got, err := askFromWire(&compassv1.Ask{Ref: tc.in})
if tc.wantErr {
if connect.CodeOf(err) != connect.CodeInvalidArgument {
t.Fatalf("askFromWire(%q) err = %v, want invalid_argument", tc.in, err)
}
return
}
if err != nil {
t.Fatalf("askFromWire(%q): %v", tc.in, err)
}
if got.Ref != tc.want {
t.Fatalf("Ref = %q, want %q", got.Ref, tc.want)
}
})
}
}
119 changes: 119 additions & 0 deletions go/internal/comms/ask_ref_pgtest_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
//go:build pgtest

package comms

import (
"context"
"testing"

"connectrpc.com/connect"

compassv1 "github.com/RigelBuild/compass/go/gen/compass/v1"
"github.com/RigelBuild/compass/go/internal/store"
)

func askWithRef(ref string) *compassv1.MessageBlock {
return &compassv1.MessageBlock{Block: &compassv1.MessageBlock_Ask{Ask: &compassv1.Ask{
Ref: ref,
Questions: []*compassv1.AskQuestion{{
QuestionId: "q1",
Question: "Ship it?",
Options: []*compassv1.AskOption{{Id: "yes", Label: "yes"}},
}},
}}}
}

// TestAskRefRoundTrip posts an ask with a ref and follows it through the
// ListMessages read and RespondToAsk: the ask and the answer snapshot both
// carry the canonical ref.
func TestAskRefRoundTrip(t *testing.T) {
svc, st := newHandler(t)
ctx := context.Background()
author := mustUser(t, st, "author")
ch, err := st.CreateChannel(ctx, author.ID, store.NewChannel{Name: "room", Kind: store.ChannelKindChannel})
if err != nil {
t.Fatalf("CreateChannel: %v", err)
}
const want = "rigelbuild/compass#12"
posted, err := svc.PostMessage(WithActor(ctx, author.ID), connect.NewRequest(&compassv1.PostMessageRequest{
Container: &compassv1.PostMessageRequest_ChannelId{ChannelId: string(ch.ID)},
Topic: &compassv1.PostMessageRequest_TopicName{TopicName: testTopic},
CreateTopic: true,
Blocks: []*compassv1.MessageBlock{askWithRef("RigelBuild/Compass#12")},
}))
if err != nil {
t.Fatalf("PostMessage: %v", err)
}
ask := posted.Msg.GetMessage().GetBlocks()[0].GetAsk()
if ask.GetRef() != want {
t.Fatalf("posted ask ref = %q, want %q", ask.GetRef(), want)
}

// refsOnWire reads the ask's ref and, once answered, the answer snapshot's
// ref off the ListMessages wire read.
refsOnWire := func() (askRef, answerRef string) {
t.Helper()
list, err := svc.ListMessages(WithActor(ctx, author.ID), connect.NewRequest(&compassv1.ListMessagesRequest{
Container: &compassv1.ListMessagesRequest_ChannelId{ChannelId: string(ch.ID)},
}))
if err != nil {
t.Fatalf("ListMessages: %v", err)
}
for _, m := range list.Msg.GetMessages() {
for _, b := range m.GetBlocks() {
if a := b.GetAsk(); a.GetAskId() == ask.GetAskId() {
askRef = a.GetRef()
}
if aa := b.GetAskAnswer(); aa.GetAsk().GetAskId() == ask.GetAskId() {
answerRef = aa.GetAsk().GetRef()
}
}
}
return askRef, answerRef
}
if got, _ := refsOnWire(); got != want {
t.Fatalf("ListMessages ask ref = %q, want %q", got, want)
}

if _, err := svc.RespondToAsk(WithActor(ctx, author.ID), connect.NewRequest(&compassv1.RespondToAskRequest{
AskId: ask.GetAskId(),
Answers: []*compassv1.AskQuestionAnswer{{QuestionId: "q1", ChosenOptionIds: []string{"yes"}}},
})); err != nil {
t.Fatalf("RespondToAsk: %v", err)
}
askRef, answerRef := refsOnWire()
if askRef != want {
t.Fatalf("answered ask ref = %q, want %q", askRef, want)
}
if answerRef != want {
t.Fatalf("answer snapshot ref = %q, want %q", answerRef, want)
}
}

// TestPostMessageRejectsMalformedAskRef refuses an ask ref that is not exactly
// one ref, and stores nothing.
func TestPostMessageRejectsMalformedAskRef(t *testing.T) {
svc, st := newHandler(t)
ctx := context.Background()
author := mustUser(t, st, "author")
ch, err := st.CreateChannel(ctx, author.ID, store.NewChannel{Name: "room", Kind: store.ChannelKindChannel})
if err != nil {
t.Fatalf("CreateChannel: %v", err)
}
_, err = svc.PostMessage(WithActor(ctx, author.ID), connect.NewRequest(&compassv1.PostMessageRequest{
Container: &compassv1.PostMessageRequest_ChannelId{ChannelId: string(ch.ID)},
Topic: &compassv1.PostMessageRequest_TopicName{TopicName: testTopic},
CreateTopic: true,
Blocks: []*compassv1.MessageBlock{askWithRef("see RIG-12")},
}))
connectCodeIs(t, err, connect.CodeInvalidArgument, "post an ask with a malformed ref")
list, err := svc.ListMessages(WithActor(ctx, author.ID), connect.NewRequest(&compassv1.ListMessagesRequest{
Container: &compassv1.ListMessagesRequest_ChannelId{ChannelId: string(ch.ID)},
}))
if err != nil {
t.Fatalf("ListMessages: %v", err)
}
if len(list.Msg.GetMessages()) != 0 {
t.Fatalf("ListMessages = %d messages after a refused post, want 0", len(list.Msg.GetMessages()))
}
}
29 changes: 25 additions & 4 deletions go/internal/comms/mapping.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

compassv1 "github.com/RigelBuild/compass/go/gen/compass/v1"
"github.com/RigelBuild/compass/go/internal/fabric"
"github.com/RigelBuild/compass/go/internal/refs"
"github.com/RigelBuild/compass/go/internal/store"
)

Expand Down Expand Up @@ -285,7 +286,7 @@ func askToWire(a *store.Ask) *compassv1.Ask {
TimedOut: q.TimedOut,
}
}
return &compassv1.Ask{AskId: a.AskID, Questions: questions, Answered: a.Answered}
return &compassv1.Ask{AskId: a.AskID, Questions: questions, Answered: a.Answered, Ref: a.Ref}
}

func accountIDsToWire(ids []store.AccountID) []string {
Expand Down Expand Up @@ -418,7 +419,11 @@ func blocksFromWire(blocks []*compassv1.MessageBlock) ([]store.MessageBlock, err
if body == nil {
return nil, connect.NewError(connect.CodeInvalidArgument, errEmptyBlock)
}
out = append(out, store.MessageBlock{Ask: askFromWire(body.Ask)})
ask, err := askFromWire(body.Ask)
if err != nil {
return nil, err
}
out = append(out, store.MessageBlock{Ask: ask})
case *compassv1.MessageBlock_AskAnswer:
// ask_answer is server-owned: constructed only by RespondToAsk. An
// inbound one on the POST path is rejected at this edge — the only
Expand All @@ -432,12 +437,28 @@ func blocksFromWire(blocks []*compassv1.MessageBlock) ([]store.MessageBlock, err
return out, nil
}

func askFromWire(a *compassv1.Ask) *store.Ask {
func askFromWire(a *compassv1.Ask) (*store.Ask, error) {
ref, err := askRefFromWire(a.GetRef())
if err != nil {
return nil, err
}
// ask_id is server-owned on the POST path: drop any caller-supplied value so
// the store's mintAskIDs assigns a fresh, globally-unique id on append. Honoring
// a wire value would let two posts share an ask_id, making RespondToAsk's
// containment SELECT match multiple rows and answer a nondeterministic one.
return &store.Ask{AskID: "", Questions: askQuestionsFromWire(a)}
return &store.Ask{AskID: "", Ref: ref, Questions: askQuestionsFromWire(a)}, nil
}

// askRefFromWire canonicalizes an ask's optional ref; "" means no ref.
func askRefFromWire(raw string) (string, error) {
if raw == "" {
return "", nil
}
r, err := refs.Parse(raw)
if err != nil {
return "", connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("ask ref: %w", err))
}
return r.String(), nil
}

// askQuestionsFromWire maps an ask's questions onto store types.
Expand Down
12 changes: 11 additions & 1 deletion packages/compass-agent/src/gen/compass/v1/comms_pb.ts

Large diffs are not rendered by default.

12 changes: 11 additions & 1 deletion packages/compass-client/src/gen/compass/v1/comms_pb.ts

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions proto/compass/v1/comms.proto
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,11 @@ message Ask {
// an inbound Ask (via PostMessage) is IGNORED — an ask being posted has by
// definition not been answered.
bool answered = 3;
// The issue or pull request this ask concerns, as one ref (RIG-12 or
// owner/repo#12). Optional. The server stores it in canonical form and
// rejects a value that is not exactly one ref with invalid_argument. Set only
// by the asker; the server never fills a default.
string ref = 4;
}

// One question within an Ask, carrying its own options and answer state.
Expand Down
Loading