diff --git a/.surface-skill-drift b/.surface-skill-drift index 96ba7ae92..2fc94a80b 100644 --- a/.surface-skill-drift +++ b/.surface-skill-drift @@ -26,6 +26,7 @@ FLAG basecamp --operator-profile FLAG basecamp --serve FLAG basecamp --shadow CMD basecamp connect discard +CMD basecamp connect ask CMD basecamp connect doctor CMD basecamp connect import CMD basecamp connect release diff --git a/internal/commands/connect.go b/internal/commands/connect.go index c59318449..f11338ea1 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -64,6 +64,7 @@ release. Linux and macOS only. basecamp connect status what it heard and holds basecamp connect doctor what it needs to run basecamp connect discard close a record without running it + basecamp connect ask declare the agent's post an ask for a go basecamp connect release clear the hold basecamp connect shadow promote make the shadow ledger the connector's, held basecamp connect import apply a cutover reconciliation file`, @@ -81,7 +82,7 @@ release. Linux and macOS only. } addConnectRunFlags(cmd, &run) cmd.AddCommand(newConnectSetupCmd(), newConnectShowCmd(), newConnectStatusCmd(), newConnectDoctorCmd(), - newConnectDiscardCmd(), newConnectReleaseCmd(), newConnectShadowCmd(), newConnectImportCmd()) + newConnectDiscardCmd(), newConnectAskCmd(), newConnectReleaseCmd(), newConnectShadowCmd(), newConnectImportCmd()) return cmd } diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index ad4f71f19..b800562ce 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -19,9 +19,12 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/admission" "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/hostutil" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/richtext" + "github.com/basecamp/basecamp-cli/internal/urlarg" ) // The operator's commands on a connector's ledger: status, discard, release, @@ -451,6 +454,140 @@ outcome is unknown. A lifecycle message still pending for it is not sent.`, } } +// --- ask ------------------------------------------------------------------ + +func newConnectAskCmd() *cobra.Command { + return &cobra.Command{ + Use: "ask ", + Short: "Declare a post of the agent's as asking the operator for a go", + Long: `Declare a comment, line or post the agent just wrote as an ask: a request for +the operator's go on exactly what it names. The operator's boost on it is then +handed off with "go": true, once. Any other boost on it, and the operator's +second, is an ordinary boosted reply. Only the operator's boost counts, in +every trust mode. + +Run it right after posting the ask, with the app_url the post returned +(--json). It reads the post once, as the agent, and refuses one the agent did +not write. The post's content is fingerprinted then: if the post is edited +afterwards, the ask is void and its boosts are only replies. Post a new ask +instead of editing one. A boost decided before the declaration is not a go. +Nothing reads the post's text to decide it is an ask: this declaration is the +only way to make one.`, + Example: ` basecamp connect ask -P agent 'https://3.basecamp.com/999/buckets/456/todos/789#__recording_1011'`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + p, err := loadConnectProfile(cmd) + if err != nil { + return err + } + ref, err := askRef(args[0], p) + if err != nil { + return err + } + agent, err := verifiedConnectAgent(cmd.Context(), p) + if err != nil { + return err + } + // The same read admission makes of the boosted recording, so an + // unedited post fingerprints the same at both ends. + post, err := agent.client.ForAccount(agent.account).Recordings().Summarize(cmd.Context(), ref) + if err != nil { + return fmt.Errorf("could not read the ask: %w", err) + } + switch { + case post.Creator == nil || post.Creator.ID != agent.personID: + return output.ErrUsage(fmt.Sprintf("Recording %d is not the agent's own post; only a post the agent wrote can be an ask", ref.RecordingID)) + case post.Status != "active": + return output.ErrUsage(fmt.Sprintf("Recording %d is %s; only a published post can be an ask", ref.RecordingID, post.Status)) + } + // The boosts already on the post: only one Basecamp gives after + // these, which has a higher id, can be the go. Ids, not times, so + // no clock on this machine is compared with Basecamp's. + boosts, err := agent.client.ForAccount(agent.account).Boosts().ListRecording(cmd.Context(), ref.RecordingID, &basecamp.BoostListOptions{Limit: -1}) + if err != nil { + return fmt.Errorf("could not read the ask's boosts: %w", err) + } + var floor int64 + for _, b := range boosts.Boosts { + floor = max(floor, b.ID) + } + ledger, done, err := openConnectLedger(cmd.Context(), p, false) + if err != nil { + return err + } + defer done() + res, err := ledger.DeclareAsk(cmd.Context(), ref.RecordingID, admission.AskFingerprint(post.Type, post.Title, post.Content), floor, operatorName()) + if err != nil { + return err + } + summary := fmt.Sprintf("Recording %d is an ask: the operator's boost on it is the go", ref.RecordingID) + switch { + case res.GoEventID > 0: + summary = fmt.Sprintf("Recording %d was already an ask, and event %d took its go", ref.RecordingID, res.GoEventID) + case res.Edited: + summary = fmt.Sprintf("Recording %d was edited since it was declared: the ask is void. Post a new ask", ref.RecordingID) + case res.Already: + summary = fmt.Sprintf("Recording %d was already an ask", ref.RecordingID) + } + return p.app.OK(res, output.WithSummary(summary)) + }, + } +} + +// askRecordingTypes are the posts an ask can be, by the URL's path type, as +// the summary read names them. A chat line's subtype does not matter to the +// read: every Chat::Lines:: type routes to the same one. +var askRecordingTypes = map[string]string{ + "lines": "Chat::Lines::Text", + "messages": "Message", + "todos": "Todo", + "cards": "Kanban::Card", + "documents": "Document", + "question_answers": "Question::Answer", +} + +// askRef is the post an ask URL names. A declaration that names the wrong +// recording fails silently later — the operator's boost on the real post is +// then only a reply — so a URL is refused unless it plainly names one post: +// a trusted host, the profile's account, and a single recording (the comment +// a #__recording_ fragment names), never a listing whose id is its parent's. +// A URL, not a bare id: the read routes by the type the URL names. +func askRef(arg string, p connectProfile) (basecamp.RecordingRef, error) { + raw := strings.TrimSpace(arg) + if !urlarg.IsURL(raw) { + return basecamp.RecordingRef{}, output.ErrUsageHint(fmt.Sprintf("No post URL in %q", arg), "Pass the app_url the post returned with --json") + } + if !hostutil.IsTrustedBasecampHost(raw, p.app.Config.BaseURL) { + return basecamp.RecordingRef{}, output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") + } + parsed := urlarg.Parse(raw) + var ref basecamp.RecordingRef + var id string + switch { + case parsed == nil: + return ref, output.ErrUsage(fmt.Sprintf("No recording in %q", arg)) + case parsed.AccountID != "" && !accountIDsEqual(parsed.AccountID, p.file.AccountID): + return ref, output.ErrUsage(fmt.Sprintf("URL is for account %s, but this agent is in account %s", parsed.AccountID, p.file.AccountID)) + case parsed.CommentID != "": + id, ref.RecordingType = parsed.CommentID, "Comment" + case parsed.IsCollection || parsed.RecordingID == "": + return ref, output.ErrUsage("URL names a listing, not the one post the ask is") + default: + typ, ok := askRecordingTypes[parsed.Type] + if !ok { + return ref, output.ErrUsage(fmt.Sprintf("URL names a %s, which cannot be an ask: declare a comment, line, message, to-do, card, document or check-in answer", parsed.Type)) + } + id, ref.RecordingType = parsed.RecordingID, typ + } + recording, err := strconv.ParseInt(id, 10, 64) + bucket, bucketErr := strconv.ParseInt(parsed.ProjectID, 10, 64) + if err != nil || recording <= 0 || bucketErr != nil || bucket <= 0 { + return ref, output.ErrUsage(fmt.Sprintf("No project and recording in %q", arg)) + } + ref.BucketID, ref.RecordingID = bucket, recording + return ref, nil +} + // --- release -------------------------------------------------------------- func newConnectReleaseCmd() *cobra.Command { diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 8719715c3..8f8261616 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "errors" "flag" + "fmt" "os" "os/exec" "path/filepath" @@ -169,6 +170,153 @@ func TestConnectDiscardAndRelease(t *testing.T) { assert.Equal(t, connector.ReasonByOperator, two.Reason) } +// askComment is a comment the fake Basecamp serves at /999/comments/. +func (f operatorFixture) askComment(id, creator int64, content string) { + f.s.mu.Lock() + defer f.s.mu.Unlock() + if f.s.bodies == nil { + f.s.bodies = map[string]any{} + } + f.s.bodies[fmt.Sprintf("/999/comments/%d", id)] = map[string]any{ + "id": id, "status": "active", "type": "Comment", "title": "Re: a card", + "app_url": fmt.Sprintf("https://3.basecamp.com/999/buckets/456/todos/789#__recording_%d", id), + "bucket": map[string]any{"id": 456, "name": "Connector", "type": "Project"}, + "parent": map[string]any{"id": 789, "title": "a card", "type": "Todo"}, + "creator": map[string]any{"id": creator, "name": "Marie Chef"}, + "content": content, + } + if _, ok := f.s.bodies[fmt.Sprintf("/999/recordings/%d/boosts.json", id)]; !ok { + f.s.bodies[fmt.Sprintf("/999/recordings/%d/boosts.json", id)] = []any{} + } +} + +func askHashIn(t *testing.T, f operatorFixture, id int64) string { + t.Helper() + dir, err := connectStatePath(f.file, false) + require.NoError(t, err) + db, err := sql.Open("sqlite", filepath.Join(dir, connector.LedgerFile)) + require.NoError(t, err) + defer func() { _ = db.Close() }() + var hash string + err = db.QueryRowContext(context.Background(), `SELECT content_hash FROM asks WHERE recording_id = ?`, id).Scan(&hash) + if errors.Is(err, sql.ErrNoRows) { + return "" + } + require.NoError(t, err) + return hash +} + +func askFloorIn(t *testing.T, f operatorFixture, id int64) int64 { + t.Helper() + dir, err := connectStatePath(f.file, false) + require.NoError(t, err) + db, err := sql.Open("sqlite", filepath.Join(dir, connector.LedgerFile)) + require.NoError(t, err) + defer func() { _ = db.Close() }() + var floor int64 + require.NoError(t, db.QueryRowContext(context.Background(), `SELECT boost_floor FROM asks WHERE recording_id = ?`, id).Scan(&floor)) + return floor +} + +// The boosts already on the post set the floor a go's boost must be above. +func TestConnectAskRecordsTheBoostsAlreadyOnThePost(t *testing.T) { + f := newOperatorFixture(t) + require.NoError(t, f.ledger(t, false).Close()) + f.askComment(1011, setupAgentPerson, "
Shall I?
") + f.s.mu.Lock() + f.s.bodies["/999/recordings/1011/boosts.json"] = []any{ + map[string]any{"id": 42, "content": "👍", "booster": map[string]any{"id": setupOperatorPerson, "name": "Operator"}}, + map[string]any{"id": 7, "content": "🎉", "booster": map[string]any{"id": setupOperatorPerson, "name": "Operator"}}, + } + f.s.mu.Unlock() + + out, err := f.run(t, output.FormatJSON, "ask", "https://3.basecamp.com/999/buckets/456/todos/789#__recording_1011") + require.NoError(t, err, out) + assert.Equal(t, int64(42), askFloorIn(t, f, 1011)) +} + +type askOut struct { + Data connector.AskResult `json:"data"` +} + +// connect ask reads the comment a URL names, not the recording it hangs +// off, and records the hash of its content exactly as the read returned it. +// Read again unedited, it hashes the same; edited, declaring it again says +// the ask is void and keeps the first hash. +func TestConnectAskFingerprintsTheCommentTheURLNames(t *testing.T) { + f := newOperatorFixture(t) + require.NoError(t, f.ledger(t, false).Close()) + const content = "
Shall I push abc123 to fix-date-picker?
" + f.askComment(1011, setupAgentPerson, content) + url := "https://3.basecamp.com/999/buckets/456/todos/789#__recording_1011" + + out, err := f.run(t, output.FormatJSON, "ask", url) + require.NoError(t, err, out) + var first askOut + require.NoError(t, json.Unmarshal([]byte(out), &first), out) + assert.Equal(t, int64(1011), first.Data.RecordingID) + assert.False(t, first.Data.Already) + assert.Equal(t, admission.AskFingerprint("Comment", "", content), askHashIn(t, f, 1011), "the hash is of the content as read") + assert.Zero(t, askFloorIn(t, f, 1011), "no boost was on the post") + + // The same post, read again: the same hash, so nothing is edited. + out, err = f.run(t, output.FormatJSON, "ask", url) + require.NoError(t, err, out) + var again askOut + require.NoError(t, json.Unmarshal([]byte(out), &again), out) + assert.True(t, again.Data.Already) + assert.False(t, again.Data.Edited) + + // The same account spelled with a leading zero is the same account. + out, err = f.run(t, output.FormatJSON, "ask", "https://3.basecamp.com/0999/buckets/456/todos/789#__recording_1011") + require.NoError(t, err, out) + + f.askComment(1011, setupAgentPerson, "
Shall I push abc123 and deploy production?
") + out, err = f.run(t, output.FormatJSON, "ask", url) + require.NoError(t, err, out) + var edited askOut + require.NoError(t, json.Unmarshal([]byte(out), &edited), out) + assert.True(t, edited.Data.Edited) + assert.Equal(t, admission.AskFingerprint("Comment", "", content), askHashIn(t, f, 1011), "the first declaration stands") +} + +// Only a post the agent wrote, named by a URL that plainly names one post in +// the agent's account, is declared; anything else declares nothing. +func TestConnectAskRefusesWhatIsNotTheAgentsOnePost(t *testing.T) { + t.Run("someone else's post", func(t *testing.T) { + f := newOperatorFixture(t) + require.NoError(t, f.ledger(t, false).Close()) + f.askComment(1011, setupOperatorPerson, "
the operator's comment
") + out, err := f.run(t, output.FormatJSON, "ask", "https://3.basecamp.com/999/buckets/456/todos/789#__recording_1011") + require.Error(t, err, out) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + assert.Empty(t, askHashIn(t, f, 1011), "nothing was declared") + }) + + // The agent's own post is served for every id these URLs could resolve + // to, so each is refused by the URL's own check, never by the creator's. + f := newOperatorFixture(t) + require.NoError(t, f.ledger(t, false).Close()) + for _, id := range []int64{789, 1011} { + f.askComment(id, setupAgentPerson, "
the agent's comment
") + } + for name, arg := range map[string]string{ + "a bare id": "1011", + "another account": "https://3.basecamp.com/1000/buckets/456/todos/789#__recording_1011", + "an untrusted host": "https://3.basecamp.example.com/999/buckets/456/todos/789#__recording_1011", + "a listing": "https://3.basecamp.com/999/buckets/456/chats/789/lines", + "not a post": "https://3.basecamp.com/999/buckets/456/vaults/789", + } { + t.Run(name, func(t *testing.T) { + out, err := f.run(t, output.FormatJSON, "ask", arg) + require.Error(t, err, out) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + }) + } + assert.Empty(t, askHashIn(t, f, 789)) + assert.Empty(t, askHashIn(t, f, 1011), "nothing was declared") +} + func TestConnectShadowPromoteAndImport(t *testing.T) { f := newOperatorFixture(t) require.NoError(t, f.ledger(t, true).Close()) diff --git a/internal/commands/connect_setup_test.go b/internal/commands/connect_setup_test.go index 426a5becc..d965ae252 100644 --- a/internal/commands/connect_setup_test.go +++ b/internal/commands/connect_setup_test.go @@ -70,6 +70,8 @@ type connectSetupServer struct { duringMint func() // mintFailure, when set, answers the stream ticket mint. mintFailure func(w http.ResponseWriter) + // bodies answers a GET of a path not otherwise served, by path. + bodies map[string]any // boss is who the agent works for, in its own profile; nil for a plain // agent, or a Basecamp that does not say. boss map[string]any @@ -234,7 +236,12 @@ func startConnectSetupServer(t *testing.T) *connectSetupServer { mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { s.mu.Lock() s.paths = append(s.paths, r.Method+" "+r.URL.Path) + body, ok := s.bodies[r.URL.Path] s.mu.Unlock() + if ok && r.Method == http.MethodGet { + writeJSON(w, body) + return + } http.NotFound(w, r) }) diff --git a/internal/connector/admission/ask.go b/internal/connector/admission/ask.go new file mode 100644 index 000000000..18d20bc94 --- /dev/null +++ b/internal/connector/admission/ask.go @@ -0,0 +1,55 @@ +package admission + +import ( + "crypto/sha256" + "encoding/hex" + "strconv" + "strings" +) + +// AskFingerprint is what an ask is declared with and checked against: the +// SHA-256, hex-encoded, of the words the ask is made of, exactly as the +// summary read returns them. Both ends read the same fields through the same +// read (basecamp.RecordingsService.Summarize), so an unedited post +// fingerprints the same, and any change at all — an edit's, or Basecamp's own +// — voids the go rather than approving words the operator may not have seen. +// Nothing is normalized: no whitespace folded, no HTML canonicalized. +// +// A post with a title of its own — a message's subject, a card's or to-do's +// title, a document's — is approved by its title as much as its content, so +// both are hashed, each prefixed with its length in bytes and a colon +// (":<len>:<content>"), so no split of one string into the two +// fields reads as another. A comment, a chat line and a check-in answer have +// no title of their own — a comment's title is its parent's, an answer's is +// its question's, and someone else's rename would change either — so their +// content alone is hashed, bare. +func AskFingerprint(recordingType, title, content string) string { + var data string + if askHasTitle(recordingType) { + data = strconv.Itoa(len(title)) + ":" + title + strconv.Itoa(len(content)) + ":" + content + } else { + data = content + } + sum := sha256.Sum256([]byte(data)) + return hex.EncodeToString(sum[:]) +} + +// askHasTitle says a recording type's title is its own words. +func askHasTitle(recordingType string) bool { + switch { + case recordingType == "Comment", recordingType == "Question::Answer": + return false + case strings.HasPrefix(recordingType, "Chat::Lines::"): + return false + } + return true +} + +// AskTitle is the title an ask is approved by: its own, or none for a +// comment, a chat line or a check-in answer. +func AskTitle(recordingType, title string) string { + if askHasTitle(recordingType) { + return title + } + return "" +} diff --git a/internal/connector/admission/boost_test.go b/internal/connector/admission/boost_test.go index 6158eb3af..ac18d1144 100644 --- a/internal/connector/admission/boost_test.go +++ b/internal/connector/admission/boost_test.go @@ -351,3 +351,75 @@ func TestAnAllowlistedAgentsBoostIsRefusedByTheBoostRead(t *testing.T) { assert.Nil(t, v.Snapshot) assert.Zero(t, f.summaryCalls, "refused before the boosted recording is read") } + +// Only the operator's own boost on the agent's post may be a go, in every +// trust mode: the verdict names the ask it would answer, and the ledger +// decides whether the agent declared one. Anyone else's boost names none. +func TestOnlyTheOperatorsBoostMayBeAGo(t *testing.T) { + t.Run("the operator's boost names the post as the ask it would answer", func(t *testing.T) { + f := newFakeReads() + boostOn(f, "Comment", agentID, operatorID) + v := decide(t, newAdmitter(t, basePolicy(), f), boostEvent(operatorID)) + require.Equal(t, StateAdmitted, v.State, "reason %q", v.Reason) + assert.Equal(t, recordingID, v.AskRecordingID) + }) + + t.Run("an allowlisted operator's, even opted in to assigning, names none", func(t *testing.T) { + p := basePolicy() + p.Trust = Trust{Mode: TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{allowedID}, AllowAssignments: true} + f := newFakeReads() + boostOn(f, "Comment", agentID, allowedID) + v := decide(t, newAdmitter(t, p, f), boostEvent(allowedID)) + require.Equal(t, StateAdmitted, v.State, "reason %q", v.Reason) + assert.Equal(t, RoleOperator, v.Role) + assert.Zero(t, v.AskRecordingID) + }) + + t.Run("a project member's names none", func(t *testing.T) { + p := basePolicy() + p.Trust = Trust{Mode: TrustProject, OperatorID: operatorID} + f := newFakeReads() + f.members[servedProj] = map[int64]bool{memberID: true} + boostOn(f, "Comment", agentID, memberID) + v := decide(t, newAdmitter(t, p, f), boostEvent(memberID)) + require.Equal(t, StateAdmitted, v.State, "reason %q", v.Reason) + assert.Zero(t, v.AskRecordingID) + }) + + t.Run("the operator's mention names none", func(t *testing.T) { + f := newFakeReads() + s := summaryWith(recordingID, servedProj, "Message", operatorID, mentionOf(t, agentID)) + f.summaries[recordingID] = s + v := decide(t, newAdmitter(t, basePolicy(), f), Event{ + ID: eventID, EventType: "message.created", BucketID: servedProj, RecordingID: recordingID, CreatorID: operatorID, + }) + require.Equal(t, StateAdmitted, v.State) + assert.Zero(t, v.AskRecordingID) + }) + + t.Run("the operator's boost on someone else's post is not admitted at all", func(t *testing.T) { + f := newFakeReads() + boostOn(f, "Comment", strangerID, operatorID) + v := decide(t, newAdmitter(t, basePolicy(), f), boostEvent(operatorID)) + assert.Equal(t, StateDiscarded, v.State) + assert.Zero(t, v.AskRecordingID) + }) +} + +// The fingerprint covers a titled post's title and content, length-prefixed so +// the fields cannot run together, and a comment's or line's content alone. +func TestAskFingerprint(t *testing.T) { + const content = "<div>push abc123</div>" + assert.Equal(t, AskFingerprint("Comment", "Re: a card", content), AskFingerprint("Comment", "Re: renamed card", content), + "a comment's title is its parent's, and is not the ask's") + assert.Equal(t, AskFingerprint("Chat::Lines::RichText", "", content), AskFingerprint("Comment", "", content)) + assert.Equal(t, AskFingerprint("Question::Answer", "What did you work on?", content), AskFingerprint("Question::Answer", "Renamed question", content), + "a check-in answer's title is its question's, and is not the ask's") + assert.Empty(t, AskTitle("Question::Answer", "What did you work on?"), "nor is it handed off as the ask's words") + assert.NotEqual(t, AskFingerprint("Message", "Push abc123?", content), AskFingerprint("Message", "Deploy production?", content), + "a message's subject is part of what it asks") + assert.NotEqual(t, AskFingerprint("Message", "ab", "c"), AskFingerprint("Message", "a", "bc"), + "the fields cannot run together") + assert.Equal(t, AskFingerprint("Kanban::Card", "Push abc123?", content), AskFingerprint("Kanban::Card", "Push abc123?", content), + "an unedited post read twice fingerprints the same") +} diff --git a/internal/connector/admission/doc.go b/internal/connector/admission/doc.go index aee82c283..aa82d58aa 100644 --- a/internal/connector/admission/doc.go +++ b/internal/connector/admission/doc.go @@ -55,7 +55,11 @@ // they reach the agent by mention, by comment on a thread it follows, by a // completion it has a stake in, and by boosting something it wrote, never by // assignment. A boost's content is the boost, which its booster wrote, so the -// booster is held to the performer's trust and there is no other author. Every admitted +// booster is held to the performer's trust and there is no other author. +// The operator's boost alone, in every trust mode, also names the boosted +// post as the ask it may be the go for (Verdict.AskRecordingID); whether the +// agent declared that post an ask, and whether its go is still open, is the +// ledger's to decide in the verdict's transaction. Every admitted // verdict carries its Role, the lesser of the performer's and, for mentioned // and subscribed, the author's, so words a participant wrote stay a // participant's request whoever brought them in. diff --git a/internal/connector/admission/verdict.go b/internal/connector/admission/verdict.go index e3ccce99f..5766d674f 100644 --- a/internal/connector/admission/verdict.go +++ b/internal/connector/admission/verdict.go @@ -63,10 +63,23 @@ type Snapshot struct { // Owner says the operator connect.json names asked, in words they wrote: // they mentioned the agent, or assigned it a recording they wrote. A // comment on a followed thread, a completion and a boost are never the - // owner's word. Someone named with --allow is an operator but never the + // owner's word; an operator's go on a declared ask is a separate + // approval (AskRecordingID) and does not make one. Someone named with --allow is an operator but never the // owner, and neither is the owner bringing in someone else's words: an // allowlisted person's to-do the owner moves in is that person's. Owner bool `json:"owner,omitempty"` + // Ask is the boosted post as this decision read it, on an operator's + // boost that may be a declared ask's go: the exact words the fingerprint + // was taken over. A go approves these words, not the post as it is + // later, so the handoff gives them to the worker to act on. + Ask *AskCopy `json:"ask,omitempty"` +} + +// AskCopy is an ask's own words, as one read returned them. Title is empty +// for a comment or a chat line, whose title is not their own. +type AskCopy struct { + Title string `json:"title"` + Content string `json:"content"` } // Verdict is admission's decision about one event. @@ -112,6 +125,23 @@ type Verdict struct { // Snapshot is set on an admitted verdict only. Snapshot *Snapshot + // AskRecordingID is set on an admitted boost the operator gave, on the + // agent's own recording: the ask that boost would be the go for, if the + // agent declared that recording an ask. Admission cannot see asks; the + // ledger decides in the verdict's own transaction, so one ask gets one go. + // Only the operator's boost carries it, in every trust mode: as with an + // assignment, an allowlist or a broader trust mode never extends it. + AskRecordingID int64 + // AskContentHash is AskFingerprint of the boosted recording's title and + // content as this decision read it. The ledger gives the go only when it equals the + // hash the ask was declared with: an ask edited since it was declared is + // void, and its boost is an ordinary reply. + AskContentHash string + // AskBoostID is the boost's own id. Basecamp's boost ids only grow, so the + // ledger orders it against the boosts already on the ask when it was + // declared, on Basecamp's clock and no other. + AskBoostID int64 + // agentCaps is set on a verdict admitting an allowed agent's mention: // the committer counts it against the caps before writing it. agentCaps *agentCaps @@ -447,6 +477,10 @@ func (a *Admitter) Decide(ctx context.Context, ev Event) (out Verdict, err error return v.end(StateBlocked, ReasonNoRoute), nil } v.State = StateAdmitted + if rule.Trigger == TriggerBoosted && ev.PerformedByID == nil && ev.Performer() == policy.Trust.OperatorID { + v.AskRecordingID, v.AskContentHash = ev.RecordingID, AskFingerprint(summary.Type, summary.Title, summary.Content) + v.AskBoostID = boost.ID + } v.Snapshot = &Snapshot{ Type: summary.Type, Title: summary.Title, @@ -456,9 +490,11 @@ func (a *Admitter) Decide(ctx context.Context, ev Event) (out Verdict, err error Role: v.Role, // Only a request the owner made, in words the owner wrote: a mention, // or an assignment of a recording they wrote. A comment on a thread - // the agent follows asks it nothing, a completion is context, a boost - // answers the agent and names nothing it approves, and an assigned - // recording someone else wrote is that person's words. + // the agent follows asks it nothing, a completion is context, and an + // assigned recording someone else wrote is that person's words. A + // boost is never Owner either, even the operator's go on a declared + // ask (AskRecordingID): Owner is the owner's word on the connector + // itself, and a go approves only what its ask names, never that. Owner: v.Role == RoleOperator && ev.Performer() == policy.Trust.OperatorID && (rule.Trigger == TriggerMentioned || rule.Trigger == TriggerAssigned) && summary.Creator.ID == policy.Trust.OperatorID, @@ -477,6 +513,11 @@ func (a *Admitter) Decide(ctx context.Context, ev Event) (out Verdict, err error case summary.Creator != nil && summary.Creator.ID == v.RequesterID: v.Snapshot.RequesterName = summary.Creator.Name } + if v.AskRecordingID > 0 { + // The same read the fingerprint was taken over, so the words handed + // off are exactly the ones the ledger matches against the declaration. + v.Snapshot.Ask = &AskCopy{Title: AskTitle(summary.Type, summary.Title), Content: summary.Content} + } return v, nil } diff --git a/internal/connector/handoff.go b/internal/connector/handoff.go index 52abde89e..c64c0c397 100644 --- a/internal/connector/handoff.go +++ b/internal/connector/handoff.go @@ -54,11 +54,29 @@ type HandoffLine struct { // Acknowledge says a person asked for something. A comment on a thread // the agent follows, or a completion, is context and is not acknowledged. Acknowledge bool `json:"acknowledge"` + // Go says this boosted request is the operator's go: their boost on a + // post the agent declared an ask (`basecamp connect ask`), and the first + // boost to take that ask. The ask is Recording. It approves what the ask + // names and nothing more. False on every other request, a boost on an + // ask from anyone else and the operator's second boost included. + Go bool `json:"go"` + // Ask is the declared ask's own words as admission read them and matched + // them against the declaration, set only on a go: the text the go + // approves. The worker acts on these words, never on a fresh read of + // Recording, which may have been edited since. Empty on every other line. + Ask HandoffAsk `json:"ask"` Content string `json:"content"` ContentUpdatedAt time.Time `json:"content_updated_at"` } +// HandoffAsk is an ask's title and content. Title is empty for a comment or a +// chat line. +type HandoffAsk struct { + Title string `json:"title"` + Content string `json:"content"` +} + // HandoffRecording points at the recording the request is about. type HandoffRecording struct { BucketID int64 `json:"bucket_id"` @@ -185,7 +203,11 @@ func handOff(ctx context.Context, opts HandoffOptions, record Record) error { opts.log().Info("connector: a request from before this run started is not handed off", "event_id", record.ID, "created_at", record.CreatedAt) return opts.Ledger.SetState(ctx, record.ID, StateDiscarded, ReasonBeforeThisRun) } - line, err := handoffLine(record, opts.AgentID) + goes, err := opts.Ledger.isGo(ctx, record.ID) + if err != nil { + return err + } + line, err := handoffLine(record, opts.AgentID, goes) if err != nil { // Closed under its own reason, not handed_off: nothing was written. // Returning the error instead would stop the connector on every @@ -200,7 +222,7 @@ func handOff(ctx context.Context, opts HandoffOptions, record Record) error { } // handoffLine is a record as the reader gets it. -func handoffLine(record Record, agentID int64) (HandoffLine, error) { +func handoffLine(record Record, agentID int64, goes bool) (HandoffLine, error) { var snapshot struct { Type string `json:"type"` Title string `json:"title"` @@ -211,6 +233,10 @@ func handoffLine(record Record, agentID int64) (HandoffLine, error) { RequesterName string `json:"requester_name"` Role string `json:"role"` Owner bool `json:"owner"` + Ask *struct { + Title string `json:"title"` + Content string `json:"content"` + } `json:"ask"` } if record.ContentDropped || len(record.Decision.Snapshot) == 0 { return HandoffLine{}, errors.New("the record has no content") @@ -218,6 +244,17 @@ func handoffLine(record Record, agentID int64) (HandoffLine, error) { if err := json.Unmarshal(record.Decision.Snapshot, &snapshot); err != nil { return HandoffLine{}, fmt.Errorf("snapshot: %w", err) } + // Only a boosted record can hold a go; the trigger is checked here too, + // so no other line ever says go. A go without the words it approves is + // not handed off as one. + isGo := goes && record.Decision.Trigger == string(admission.TriggerBoosted) && snapshot.Ask != nil + var ask HandoffAsk + if isGo { + ask = HandoffAsk{ + Title: richtext.SanitizeTerminal(snapshot.Ask.Title), + Content: richtext.SanitizeTerminal(snapshot.Ask.Content), + } + } return HandoffLine{ Type: "request", EventID: record.ID, @@ -237,6 +274,8 @@ func handoffLine(record Record, agentID int64) (HandoffLine, error) { Role: handoffRole(snapshot.Role), Owner: snapshot.Owner && handoffRole(snapshot.Role) == string(admission.RoleOperator), Acknowledge: record.Decision.Acknowledge, + Go: isGo, + Ask: ask, // The line is read in a terminal as often as by a program: no // control sequences from Basecamp's text reach it. Content: richtext.SanitizeTerminal(StripMentionsOf(snapshot.Content, agentID)), diff --git a/internal/connector/ledger.go b/internal/connector/ledger.go index bf837272e..7710e2fad 100644 --- a/internal/connector/ledger.go +++ b/internal/connector/ledger.go @@ -946,6 +946,11 @@ END; // its ALTER TABLE would refuse a second run outright rather than // backfill twice. migrationBlockedRetrySchedule, + + // Migration 15. Asks: the posts the agent declared as asking the + // operator for a go, and which event's boost took each one's go. See + // ledger_asks.go. + migrationAsks, } // migrationBlockedRetrySchedule is migration 14. diff --git a/internal/connector/ledger_admission.go b/internal/connector/ledger_admission.go index fcfab5e1e..73dd1885b 100644 --- a/internal/connector/ledger_admission.go +++ b/internal/connector/ledger_admission.go @@ -189,6 +189,15 @@ func (a Admission) commit(ctx context.Context, v admission.Verdict, state Record return "", fmt.Errorf("connector: read verdict on %d back: %w", v.EventID, err) } state = RecordState(written) + if v.AskRecordingID > 0 { + // The go is taken with the verdict, or not at all: two boosts + // on one ask commit one after the other, and only the first + // finds it open. The record keeps it under any of the three + // states, since each still hands off. + if _, err := takeGo(ctx, tx, v.AskRecordingID, v.AskContentHash, v.AskBoostID, v.EventID); err != nil { + return "", err + } + } } if l.hooks.VerdictCommitted != nil { committed := CommittedVerdict{ diff --git a/internal/connector/ledger_admission_test.go b/internal/connector/ledger_admission_test.go index 5041efe75..ffa885458 100644 --- a/internal/connector/ledger_admission_test.go +++ b/internal/connector/ledger_admission_test.go @@ -3,6 +3,7 @@ package connector import ( "bytes" "context" + "database/sql" "encoding/base64" "encoding/json" "errors" @@ -909,3 +910,94 @@ func TestAnUnreadableConfigHoldsItsRecordsUntilTheFileAndAPersonAreBack(t *testi return getRecord(t, ledger, ev.ID).State == StateAdmitted }, 5*time.Second, 10*time.Millisecond, "and the work that was held runs") } + +// The go end to end, through a real admitter: the ask declared with the hash +// of its content as read, an unedited ask read again at admission hashes the +// same and gets its go, and an ask edited since gets none and stays open. +func TestAGoIsOnlyForTheAskAsDeclared(t *testing.T) { + const commentID, cardID int64 = 601, 600 + const declared = "<div>Shall I push abc123 to fix-date-picker?</div>" + run := func(t *testing.T, typ, declaredTitle, title, content string) (*Ledger, HandoffLine) { + t.Helper() + ledger := newTestLedger(t) + queue, err := NewQueue(10, 100) + require.NoError(t, err) + reads := &adapterReads{ + summaries: map[int64]*basecamp.RecordingSummary{commentID: { + ID: commentID, Status: "active", Type: typ, Title: title, + AppURL: "https://app.basecamp.com/2914079/buckets/48699913/recordings/601", + Bucket: &basecamp.Bucket{ID: adapterBucketID}, + Parent: &basecamp.Parent{ID: cardID}, + Creator: &basecamp.Person{ID: adapterAgentID}, + Content: content, MentionedPersonIDs: []int64{}, + }}, + boosts: map[int64]*basecamp.Boost{31337: { + ID: 31337, Content: "👍", CreatedAt: time.Date(2026, 9, 16, 10, 0, 5, 0, time.UTC), + Booster: &basecamp.Person{ID: adapterOperatorID, Name: "The Operator"}, + Recording: &basecamp.Parent{ID: commentID, Type: typ}, + }}, + } + admitter, err := admission.NewAdmitter(admission.Policy{ + AgentID: adapterAgentID, + Trust: admission.Trust{Mode: admission.TrustOperator, OperatorID: adapterOperatorID}, + Projects: map[int64]admission.Project{adapterBucketID: {Class: "internal"}}, + }, admission.Reads{Summaries: reads, Subscriptions: reads, Assignments: reads, Boosts: reads}) + require.NoError(t, err) + _, err = ledger.DeclareAsk(context.Background(), commentID, admission.AskFingerprint(typ, declaredTitle, declared), 31336, "local:agent") + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + done := make(chan error, 1) + go func() { + done <- RunAdmission(ctx, AdmissionOptions{Ledger: ledger, Queue: queue, Admitter: admitter, Workers: 1, Lines: ndjson.NewWriter(&bytes.Buffer{})}) + }() + ev := testEvent(1) + ev.EventType, ev.Kind, ev.Action, ev.RecordingID = "boost.created", "comment_boosted", "boosted", commentID + ev.Details = json.RawMessage(`{"boost_id":31337,"boosted_event_id":null,"boosted_event_type":null}`) + _, err = ledger.RecordSeen(ctx, ev, LanePoll) + require.NoError(t, err) + require.NoError(t, queue.Offer(ctx, ev.ID)) + require.Eventually(t, func() bool { return getRecord(t, ledger, ev.ID).State != StateSeen }, 5*time.Second, 10*time.Millisecond) + cancel() + require.NoError(t, <-done) + + var out bytes.Buffer + opts := handoffOptions(ledger, &out) + opts.AgentID = adapterAgentID + require.NoError(t, handOffReady(context.Background(), opts)) + lines := handedOffLines(t, &out) + require.Len(t, lines, 1) + return ledger, lines[0] + } + + t.Run("unedited: the go, carrying the words it approves", func(t *testing.T) { + _, line := run(t, "Comment", "", "Re: Shall I?", declared) + assert.Equal(t, "boosted", line.Trigger) + assert.True(t, line.Go) + assert.Equal(t, commentID, line.Recording.RecordingID) + assert.Equal(t, HandoffAsk{Content: declared}, line.Ask, "a comment's words are its content; its title is its parent's") + }) + + t.Run("a titled ask, unedited: the go carries its title too", func(t *testing.T) { + _, line := run(t, "Message", "Push abc123?", "Push abc123?", declared) + assert.True(t, line.Go) + assert.Equal(t, HandoffAsk{Title: "Push abc123?", Content: declared}, line.Ask) + }) + + t.Run("a titled ask whose title was edited: a reply", func(t *testing.T) { + _, line := run(t, "Message", "Push abc123?", "Deploy production?", declared) + assert.False(t, line.Go) + assert.Empty(t, line.Ask) + }) + + t.Run("edited since it was declared: a reply, and the ask stays open", func(t *testing.T) { + ledger, line := run(t, "Comment", "", "Re: Shall I?", "<div>Shall I push abc123 and deploy production?</div>") + assert.Equal(t, "boosted", line.Trigger) + assert.False(t, line.Go) + assert.Empty(t, line.Ask, "a reply carries no ask") + var goEvent sql.NullInt64 + require.NoError(t, ledger.db.QueryRowContext(context.Background(), `SELECT go_event_id FROM asks WHERE recording_id = ?`, commentID).Scan(&goEvent)) + assert.False(t, goEvent.Valid) + }) +} diff --git a/internal/connector/ledger_asks.go b/internal/connector/ledger_asks.go new file mode 100644 index 000000000..6f7c2eaf6 --- /dev/null +++ b/internal/connector/ledger_asks.go @@ -0,0 +1,153 @@ +package connector + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" +) + +// An ask is a post of the agent's that asks the operator for a go: approval +// of exactly what the post names. The agent declares it when it posts +// (`basecamp connect ask <url>`), and nothing else makes one: the connector +// never decides that a post is an ask by reading it. +// +// The operator's boost on a declared ask is the go. It is decided in the +// verdict's own transaction (Admission.commit), so the boost that takes an +// ask is the one whose verdict committed first, and no other: one ask gets +// one go. A go taken stays taken, even when its record is closed unwritten +// (before_this_run): a connector that printed the line and stopped before +// closing the record looks the same on restart, and giving that go back would +// let the next boost approve twice. Losing a go costs the operator a mention; +// a second go costs a second action. Another boost on it afterwards — the operator's second, or one +// removed and given again, which Basecamp records as a new boost and a new +// event — is an ordinary boosted reply. The event that took the go keeps it +// when it is decided again (a redispatch), because go_event_id names it. +// +// An ask is declared with a fingerprint of its content, read when it is +// declared (admission.AskFingerprint), and the go is given only while the +// content the boost's own decision read still matches it. An ask edited after +// it was declared is void: its boosts are ordinary replies, and the ask is not +// used up by them. Declaring it again does not take the edit in — the first +// declaration stands — so an edited ask stays void; the agent posts a new one. +// +// An ask does not expire. The go is the operator's deliberate boost on the +// post itself, and what it approves is what the post names, which a worker +// checks is still current before acting (a branch still at the commit the +// ask named, an action not already done). A deadline would add a number +// with nothing to choose it by, and turn a late but deliberate approval into +// a thumbs-up without saying so. +const migrationAsks = ` +CREATE TABLE asks ( + recording_id INTEGER PRIMARY KEY CHECK (recording_id > 0), + declared_at TEXT NOT NULL, + declared_by TEXT NOT NULL CHECK (declared_by <> ''), + content_hash TEXT NOT NULL CHECK (content_hash <> ''), + boost_floor INTEGER NOT NULL CHECK (boost_floor >= 0), + go_event_id INTEGER UNIQUE REFERENCES events (id) +); +` + +// AskResult is what declaring an ask did. +type AskResult struct { + RecordingID int64 `json:"recording_id"` + // Already says the recording was declared an ask before; the first + // declaration stands. + Already bool `json:"already"` + // Edited says the post no longer matches the content it was first + // declared with: the ask is void, and only a new ask can be a go. + Edited bool `json:"edited,omitempty"` + // GoEventID is the event whose boost took the ask's go, zero while none + // has. + GoEventID int64 `json:"go_event_id,omitempty"` +} + +// DeclareAsk records a recording the agent posted as an ask for the +// operator's go, with the hash of its content as read now. Declaring one +// twice is not an error, and changes nothing: not an ask whose go was taken, +// and not the hash of one edited since. +// +// boostFloor is the highest id of a boost already on the post when it was +// declared, zero for none: only a boost with a higher id, which Basecamp gave +// after that, can be the go. +func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, contentHash string, boostFloor int64, by string) (AskResult, error) { + switch { + case recordingID <= 0: + return AskResult{}, errors.New("connector: an ask names a recording") + case contentHash == "": + return AskResult{}, errors.New("connector: an ask records its content's hash") + case boostFloor < 0: + return AskResult{}, errors.New("connector: an ask's boost floor is a boost id or zero") + case strings.TrimSpace(by) == "": + return AskResult{}, errors.New("connector: an ask records who declared it") + } + var out AskResult + err := retryBusy(func() error { + tx, err := l.db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("connector: begin ask: %w", err) + } + defer func() { _ = tx.Rollback() }() + res, err := tx.ExecContext(ctx, + `INSERT INTO asks (recording_id, declared_at, declared_by, content_hash, boost_floor) VALUES (?, ?, ?, ?, ?) ON CONFLICT (recording_id) DO NOTHING`, + recordingID, l.timestamp(), by, contentHash, boostFloor) + if err != nil { + return fmt.Errorf("connector: declare ask on %d: %w", recordingID, err) + } + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("connector: declare ask on %d: %w", recordingID, err) + } + var ( + goEvent sql.NullInt64 + declared string + ) + if err := tx.QueryRowContext(ctx, `SELECT go_event_id, content_hash FROM asks WHERE recording_id = ?`, recordingID).Scan(&goEvent, &declared); err != nil { + return fmt.Errorf("connector: read ask on %d: %w", recordingID, err) + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("connector: commit ask on %d: %w", recordingID, err) + } + out = AskResult{RecordingID: recordingID, Already: n == 0, Edited: declared != contentHash, GoEventID: goEvent.Int64} + return nil + }) + return out, err +} + +// takeGo gives a declared ask's go to eventID, inside the verdict's +// transaction, and reports whether eventID holds it. An ask nobody declared, +// one another event already took, and one whose content no longer hashes to +// what it was declared with give nothing, and the last is left as it was. +// +// Nor does a boost Basecamp gave before the ask was declared. A boost the +// connector saw first and decides later — held blocked for a retry, or seen +// while no connector ran — would otherwise find the declaration waiting when +// it commits and take a go nobody gave. Which came first is settled on one +// clock, Basecamp's: boost ids only grow, and the declaration recorded the +// highest one already on the post (boost_floor). No wall clock is compared, +// so no skew between this machine and Basecamp can reorder them. +func takeGo(ctx context.Context, tx *sql.Tx, recordingID int64, contentHash string, boostID, eventID int64) (bool, error) { + res, err := tx.ExecContext(ctx, + `UPDATE asks SET go_event_id = ? WHERE recording_id = ? AND content_hash = ? AND (go_event_id IS NULL OR go_event_id = ?) + AND boost_floor < ?`, + eventID, recordingID, contentHash, eventID, boostID) + if err != nil { + return false, fmt.Errorf("connector: take the go on %d for %d: %w", recordingID, eventID, err) + } + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("connector: take the go on %d for %d: %w", recordingID, eventID, err) + } + return n == 1, nil +} + +// isGo reports whether eventID took an ask's go. +func (l *Ledger) isGo(ctx context.Context, eventID int64) (bool, error) { + var goes bool + err := l.db.QueryRowContext(ctx, `SELECT EXISTS (SELECT 1 FROM asks WHERE go_event_id = ?)`, eventID).Scan(&goes) + if err != nil { + return false, fmt.Errorf("connector: read the go for %d: %w", eventID, err) + } + return goes, nil +} diff --git a/internal/connector/ledger_asks_test.go b/internal/connector/ledger_asks_test.go new file mode 100644 index 000000000..8d7b54ec5 --- /dev/null +++ b/internal/connector/ledger_asks_test.go @@ -0,0 +1,256 @@ +package connector + +import ( + "bytes" + "context" + "database/sql" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector/admission" +) + +const askRecordingID int64 = 10304028972 + +// askText is the declared ask, a comment; askHash its fingerprint, as +// admission computes it. +const askText = "<div>Shall I push abc123 to fix-date-picker?</div>" + +var askHash = admission.AskFingerprint("Comment", "", askText) + +// boostVerdict is an admitted boost on askRecordingID; operator says it is +// the operator's, which is when admission names the ask it would answer. +func boostVerdict(id int64, operator bool) admission.Verdict { + v := admittedVerdict(id, 0, "recording:10304028989") + v.EventType, v.Trigger, v.Acknowledge = "boost.created", admission.TriggerBoosted, false + v.Snapshot.Content = "yes, go" + if operator { + v.AskRecordingID, v.AskContentHash, v.AskBoostID = askRecordingID, askHash, 1000+id + v.Snapshot.Ask = &admission.AskCopy{Content: askText} + } + return v +} + +func commitBoost(t *testing.T, ledger *Ledger, id int64, operator bool) { + t.Helper() + seenRecord(t, ledger, id) + _, err := ledger.Admission().Commit(context.Background(), boostVerdict(id, operator)) + require.NoError(t, err) +} + +func goes(t *testing.T, ledger *Ledger, id int64) bool { + t.Helper() + g, err := ledger.isGo(context.Background(), id) + require.NoError(t, err) + return g +} + +// One ask, one go: the operator's first boost on a declared ask takes it, +// and every boost after it is an ordinary reply — the operator's second, and +// one removed and given again, which Basecamp delivers as a new event. +func TestOneAskGetsOneGo(t *testing.T) { + ledger := newTestLedger(t) + ctx := context.Background() + declared, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + assert.False(t, declared.Already) + + commitBoost(t, ledger, 1, true) + commitBoost(t, ledger, 2, true) + commitBoost(t, ledger, 3, true) + + assert.True(t, goes(t, ledger, 1), "the first operator boost is the go") + assert.False(t, goes(t, ledger, 2), "a second boost is a reply") + assert.False(t, goes(t, ledger, 3), "a boost given again is a reply") + + again, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + assert.True(t, again.Already) + assert.Equal(t, int64(1), again.GoEventID, "declaring it again does not reopen the ask") + commitBoost(t, ledger, 4, true) + assert.False(t, goes(t, ledger, 4)) +} + +// The event that took the go keeps it when it is decided again. +func TestTheGoIsTheEventsToKeep(t *testing.T) { + ledger := newTestLedger(t) + ctx := context.Background() + _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + commitBoost(t, ledger, 1, true) + + tx, err := ledger.db.BeginTx(ctx, nil) + require.NoError(t, err) + defer func() { _ = tx.Rollback() }() + again, err := takeGo(ctx, tx, askRecordingID, askHash, 1001, 1) + require.NoError(t, err) + assert.True(t, again, "the same event takes the same go") + other, err := takeGo(ctx, tx, askRecordingID, askHash, 1002, 2) + require.NoError(t, err) + assert.False(t, other) +} + +func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.T) { + t.Run("a boost on a post nobody declared an ask", func(t *testing.T) { + ledger := newTestLedger(t) + commitBoost(t, ledger, 1, true) + assert.False(t, goes(t, ledger, 1)) + // Declared after the boost was decided: not retroactive. + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + assert.False(t, goes(t, ledger, 1)) + }) + + t.Run("someone else's boost leaves the ask open for the operator's", func(t *testing.T) { + ledger := newTestLedger(t) + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + commitBoost(t, ledger, 1, false) + commitBoost(t, ledger, 2, true) + assert.False(t, goes(t, ledger, 1)) + assert.True(t, goes(t, ledger, 2)) + }) + + t.Run("an ask names a recording and who declared it", func(t *testing.T) { + ledger := newTestLedger(t) + _, err := ledger.DeclareAsk(context.Background(), 0, askHash, 0, "local:agent") + require.Error(t, err) + _, err = ledger.DeclareAsk(context.Background(), askRecordingID, askHash, 0, " ") + require.Error(t, err) + }) +} + +// The session tells the go from a thumbs-up by the line alone. +func TestTheGoIsHandedOffAsSuch(t *testing.T) { + ledger := newTestLedger(t) + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + commitBoost(t, ledger, 1, true) + commitBoost(t, ledger, 2, true) + admitOn(t, ledger, 3, "recording:3") + + var out bytes.Buffer + opts := handoffOptions(ledger, &out) + for range 3 { + require.NoError(t, handOffReady(context.Background(), opts)) + } + lines := handedOffLines(t, &out) + require.Len(t, lines, 3) + byID := map[int64]HandoffLine{} + for _, l := range lines { + byID[l.EventID] = l + } + assert.True(t, byID[1].Go) + assert.Equal(t, HandoffAsk{Content: askText}, byID[1].Ask, "the go carries the words it approves") + assert.Empty(t, byID[2].Ask, "a reply carries none") + assert.Equal(t, "boosted", byID[1].Trigger) + assert.Equal(t, askRecordingID, byID[1].Recording.RecordingID, "the ask is the line's recording") + assert.Equal(t, "yes, go", byID[1].Content) + assert.False(t, byID[2].Go, "the second boost is a reply") + assert.False(t, byID[3].Go, "a mention is never a go") +} + +// Only a boosted record's line can say go, whatever the asks table holds. +func TestOnlyABoostedLineSaysGo(t *testing.T) { + line, err := handoffLine(func() Record { + ledger := newTestLedger(t) + admitOn(t, ledger, 1, "recording:1") + return getRecord(t, ledger, 1) + }(), 999, true) + require.NoError(t, err) + assert.False(t, line.Go) +} + +// A go taken stays taken, even when its record is closed unwritten: on +// restart that looks the same as a line printed just before a crash, and +// giving the go back would let the next boost approve twice. +func TestAGoClosedUnwrittenIsNotGivenBack(t *testing.T) { + ledger := newTestLedger(t) + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + commitBoost(t, ledger, 1, true) + + var out bytes.Buffer + opts := handoffOptions(ledger, &out) + opts.Started = opts.Started.Add(time.Hour) + require.NoError(t, handOffReady(context.Background(), opts)) + assert.Empty(t, out.String()) + assert.Equal(t, ReasonBeforeThisRun, getRecord(t, ledger, 1).Reason) + + commitBoost(t, ledger, 2, true) + assert.True(t, goes(t, ledger, 1), "the go stays with the event that took it") + assert.False(t, goes(t, ledger, 2), "the next boost is a reply, never a second go") +} + +// An ask edited after it was declared is void: the operator's boost on it is +// an ordinary reply, and the ask is not used up by it. +func TestAnAskEditedSinceItWasDeclaredGivesNoGo(t *testing.T) { + ledger := newTestLedger(t) + ctx := context.Background() + _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + + edited := boostVerdict(1, true) + edited.AskContentHash = admission.AskFingerprint("Comment", "", "<div>Shall I push abc123 and deploy production?</div>") + seenRecord(t, ledger, 1) + _, err = ledger.Admission().Commit(ctx, edited) + require.NoError(t, err) + assert.Equal(t, StateAdmitted, getRecord(t, ledger, 1).State, "still handed off, as a reply") + assert.False(t, goes(t, ledger, 1), "an edited ask is no go") + + var goEvent sql.NullInt64 + require.NoError(t, ledger.db.QueryRowContext(ctx, `SELECT go_event_id FROM asks WHERE recording_id = ?`, askRecordingID).Scan(&goEvent)) + assert.False(t, goEvent.Valid, "the ask is not used up") + + // Declaring the edited post again does not take the edit in. + again, err := ledger.DeclareAsk(ctx, askRecordingID, edited.AskContentHash, 0, "local:agent") + require.NoError(t, err) + assert.True(t, again.Already) + assert.True(t, again.Edited) + commitBoost(t, ledger, 2, false) + tx, err := ledger.db.BeginTx(ctx, nil) + require.NoError(t, err) + defer func() { _ = tx.Rollback() }() + took, err := takeGo(ctx, tx, askRecordingID, edited.AskContentHash, 1003, 3) + require.NoError(t, err) + assert.False(t, took, "the edited content never matches the declaration") +} + +// The handed-off line says so: the edited ask's boost is a reply. +func TestAnEditedAsksBoostIsHandedOffAsAReply(t *testing.T) { + ledger := newTestLedger(t) + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, 0, "local:agent") + require.NoError(t, err) + edited := boostVerdict(1, true) + edited.AskContentHash = admission.AskFingerprint("Comment", "", "<div>something else</div>") + seenRecord(t, ledger, 1) + _, err = ledger.Admission().Commit(context.Background(), edited) + require.NoError(t, err) + + var out bytes.Buffer + require.NoError(t, handOffReady(context.Background(), handoffOptions(ledger, &out))) + lines := handedOffLines(t, &out) + require.Len(t, lines, 1) + assert.Equal(t, "boosted", lines[0].Trigger) + assert.False(t, lines[0].Go) +} + +// A boost Basecamp gave before the ask was declared is no go, even when the +// connector decides it after the declaration — held blocked for a retry, or +// seen late — and it does not use the ask up. Which came first is read from +// boost ids, Basecamp's own order, whatever either wall clock says. +func TestABoostFromBeforeTheDeclarationIsNoGo(t *testing.T) { + ledger := newTestLedger(t) + ctx := context.Background() + // Boost 1001 was already on the post when it was declared. + _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, 1001, "local:agent") + require.NoError(t, err) + commitBoost(t, ledger, 1, true) // boost 1001 + assert.False(t, goes(t, ledger, 1), "a boost from before the declaration is a reply") + + commitBoost(t, ledger, 2, true) // boost 1002, given after + assert.True(t, goes(t, ledger, 2), "the ask was left open for a boost given after it") +} diff --git a/internal/connector/request_contract_test.go b/internal/connector/request_contract_test.go index ac2e9bafc..f9ba5671f 100644 --- a/internal/connector/request_contract_test.go +++ b/internal/connector/request_contract_test.go @@ -21,10 +21,13 @@ import ( // change made on purpose, here, or not at all. Adding a key is not. var requestLineContract = []string{ "acknowledge", + "ask.content", + "ask.title", "content", "content_updated_at", "event_id", "event_type", + "go", "owner", "recording.bucket_id", "recording.project_name", diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 4661bfbf7..60ff282de 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -104,7 +104,7 @@ thing to check. "title":"Fix the date picker","url":"https://3.basecamp.com/999/buckets/456/recordings/789"}, "reply_to":{"kind":"comment","recording_id":700}, "requester_id":1001,"requester_name":"Jorge Manrubia","role":"operator","owner":true,"acknowledge":true, - "content":"<p>the date picker is off by one, please fix</p>","content_updated_at":"..."} + "go":false,"ask":{"title":"","content":""},"content":"<p>fix the date picker</p>","content_updated_at":"..."} ``` - **`trigger`**: why it reached you. @@ -116,12 +116,11 @@ thing to check. - `completed`: something completed in a project it watches. - `boosted`: a boost on the agent's own post. - **`acknowledge`**: true when a person asked for something. False for - `subscribed`, `completed` and `boosted`, which are context, not instructions. -- **`recording`**: what the event is about. `bucket_id` is the project, and + `subscribed`, `completed` and `boosted`, context unless a `go`. +- **`recording`**: what the event is about. `bucket_id` is the project, `project_name` its name. `url` works with every command below. - **`reply_to`**: where the answer goes. `kind` `comment` means a comment on - `recording_id` (the card or message the comment belongs to, already chosen for - you). `kind` `chat_line` means a line in the Campfire `recording_id`. + `recording_id` (already chosen for you). `kind` `chat_line` means a line in the Campfire `recording_id`. - **`requester_id`**: the person who asked. Mention them on failure, except in Campfire. `requester_name` is their name when they wrote the recording; it's missing for an assignment someone else's recording carries. @@ -141,13 +140,15 @@ thing to check. - **`content`**: the request as it was written, with the agent's own mention removed. For an assignment, the recording itself (its title and content) is the task. The live recording may be newer. +- **`go`**, **`ask`**: the operator's go and the words it approves + (`subagent.md`). Every line has already passed the trust check: the person may give the agent work, and the project is one it serves. A request arrives once, except in one case: if the connector crashes right after printing a line, it can print it again when restarted within a minute. Skip an `event_id` you've already handled. Every other -line (no `type`, or `"type":"event"`) is the connector's own log of what it saw -and decided, including what it turned away. Never act on those. +line (no `type`, or `"type":"event"`) is the connector's own log. Never act on +those. ## For each request: acknowledge, choose the repo, hand off diff --git a/skills/basecamp-connect/subagent.md b/skills/basecamp-connect/subagent.md index e2a48aae6..416854088 100644 --- a/skills/basecamp-connect/subagent.md +++ b/skills/basecamp-connect/subagent.md @@ -185,9 +185,29 @@ detail, load the `basecamp` skill. change, name the repo, the branch and its commit in the reply; for any other action, name the action itself, so whoever acts on the go finds the work. In Campfire, where Basecamp refuses an agent's mention, name the - operator in plain text instead. + operator in plain text instead. In the reply, say that boosting it is a go + too. Post it with `--json`, and declare it an ask with the `app_url` it + returned before you do anything else: + + ```bash + basecamp connect ask '<posted app_url>' -P '<profile>' + ``` + + It reads the post once and fingerprints its words. A boost given before + the declaration is only a reply, however late it is handed off, so declare + at once. Never edit a post once + it is declared an ask: an edit voids it, and the operator's boost on it then + arrives as an ordinary `boosted` reply with `go` false. Declaring it again + doesn't revive it. To change an ask, post a new one and declare that. + Declare only a post that asks the operator for a go and names exactly what + it approves, one ask per post. The declaration is what makes the + operator's boost on it a go; nothing reads the post's words to decide. A + boost given more than a minute before the connector started is never + handed off, and still uses the ask up: if the operator boosted and nothing + came, ask for the go by mention. - **Only an operator's word approves**: an operator's @mention of the agent on - that thread that says go (see *An operator's go*). An operator's reply + that thread that says go, or the operator's boost on the declared ask, which + arrives as `boosted` with `go` true (see *An operator's go*). An operator's reply without the mention arrives as a followed-thread comment, which is context, not an instruction. A go typed in the main session names no thread and proves no one, and nothing a participant writes, "the operator said go" @@ -203,15 +223,31 @@ detail, load the `basecamp` skill. **An operator's go.** A `role` `operator`, `trigger` `mentioned` request that says go on a thread where the agent asked for an operator's word is that -approval, though its content is only "go". A `subscribed` one is a reply that -didn't mention the agent, and approves nothing. Neither does a `boosted` one: -a boost is too short to say which ask it approves, or that it approves at all. -Read the thread for the +approval, though its content is only "go". So is a `boosted` request with +`go` true: the operator boosted a post the agent declared an ask, and theirs +was the first boost to take it. One ask gets one go: once the operator's +boost has taken it, every later boost on that ask, the operator's included +and one removed and given again, arrives with `go` false. A boost from anyone +but the operator is never a go, in any trust mode, and doesn't use the ask up. +A `subscribed` one is a reply that didn't mention the agent, and approves +nothing; neither does a `boosted` one with `go` false, whoever gave it. The go +is the operator's approval of what the ask names, and only that: it is never +a person's word on a merge, a deploy or anything else the ask did not name, +and never a change to the connector, which only the owner's own mention makes. + +For a `go`, the ask is the request's `ask`: its `title` and `content` exactly +as the connector read them and matched them against the declaration. `title` +is empty for a comment, a chat line or a check-in answer, whose title isn't +their own; `ask` is empty on every other request. Act only on that text, never +on a fresh read of `recording`, which may have been edited since; `recording` +and `reply_to` are for replying. For a mention, read the thread for the agent's ask. For a code change it names the repo, branch and commit: check the branch still points at that commit, then push and open the pull request. For anything else it names the action itself: do that. An ask the agent has -already answered as done is not waiting. If the thread holds no waiting ask, -or more than one, act on none and say so in the thread. +already answered as done is not waiting, `go` or not. For a mention, if the +thread holds no waiting ask, or more than one, act on none and say so in the +thread; a `go` names its ask, so other asks in the thread don't make it +ambiguous. **By trigger:** @@ -224,8 +260,9 @@ or more than one, act on none and say so in the thread. reply unless the agent takes the thread on. - **`completed`**: a signal, like `subscribed`. Act only when the project's AGENTS.md or the thread asks for a follow-up. -- **`boosted`**: a reply to something the agent wrote, like `subscribed`. - `recording` is the agent's own post; `content` is the boost itself, a few +- **`boosted`**: with `go` true, the operator's go on the ask in `ask`: act on + it as *An operator's go* says. Otherwise a reply to something the agent + wrote, like `subscribed`. `recording` is the agent's own post; `content` is the boost itself, a few characters, often an emoji; `requester_name` is the booster's. Read the boosted post and its thread, and **default to silence**: a boost is thanks or agreement far more often than a question. Reply only when the boost