From 99bc72d040eda155a5660aed81a0e4382fa11d70 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 12 Oct 2026 09:59:47 -0700 Subject: [PATCH 01/11] connect: the operator's boost on a declared ask is the go A boost on the agent's own post arrives as a boosted reply (#877), and the skill treated it as approving nothing. An agent asking the operator for a go had to wait for a mention. The agent now declares an ask when it posts one, with `basecamp connect ask `, recorded in the ledger's new asks table. Nothing reads a post's text to decide it is an ask. The operator's boost on a declared ask, and only the operator's in every trust mode, is the go: admission names the boosted post on the operator's verdict, and the ledger takes the ask's go in the verdict's own transaction, so one ask gets one go. A second boost, or one removed and given again, is an ordinary boosted reply; the event that took the go keeps it when decided again; a go closed unwritten (before this run, unreadable) is given back. Asks do not expire. The request line carries "go": true on that request. The skill says the go approves what the ask names and nothing else: not a merge or deploy the ask did not name, and never a connector change. --- .surface-skill-drift | 1 + internal/commands/connect.go | 3 +- internal/commands/connect_operator.go | 48 ++++++ internal/commands/connect_operator_test.go | 28 ++++ internal/connector/admission/boost_test.go | 54 ++++++ internal/connector/admission/doc.go | 6 +- internal/connector/admission/verdict.go | 11 ++ internal/connector/handoff.go | 23 ++- internal/connector/ledger.go | 5 + internal/connector/ledger_admission.go | 9 + internal/connector/ledger_asks.go | 131 +++++++++++++++ internal/connector/ledger_asks_test.go | 176 ++++++++++++++++++++ internal/connector/request_contract_test.go | 1 + 13 files changed, 492 insertions(+), 4 deletions(-) create mode 100644 internal/connector/ledger_asks.go create mode 100644 internal/connector/ledger_asks_test.go 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..b42f682e4 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -451,6 +451,54 @@ 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 URL or id the post returned. 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 { + raw, _ := extractCommentWithProject(args[0]) + id, err := strconv.ParseInt(raw, 10, 64) + if err != nil || id <= 0 { + return output.ErrUsage(fmt.Sprintf("No recording id in %q", args[0])) + } + p, err := loadConnectProfile(cmd) + if err != nil { + return err + } + ledger, done, err := openConnectLedger(cmd.Context(), p, false) + if err != nil { + return err + } + defer done() + res, err := ledger.DeclareAsk(cmd.Context(), id, operatorName()) + if err != nil { + return err + } + summary := fmt.Sprintf("Recording %d is an ask: the operator's boost on it is the go", id) + switch { + case res.GoEventID > 0: + summary = fmt.Sprintf("Recording %d was already an ask, and event %d took its go", id, res.GoEventID) + case res.Already: + summary = fmt.Sprintf("Recording %d was already an ask", id) + } + return p.app.OK(res, output.WithSummary(summary)) + }, + } +} + // --- release -------------------------------------------------------------- func newConnectReleaseCmd() *cobra.Command { diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 8719715c3..ccd740132 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -169,6 +169,34 @@ func TestConnectDiscardAndRelease(t *testing.T) { assert.Equal(t, connector.ReasonByOperator, two.Reason) } +// connect ask declares the comment a URL names, not the recording it hangs +// off, and declaring it again leaves the first declaration standing. +func TestConnectAskDeclaresTheCommentTheURLNames(t *testing.T) { + f := newOperatorFixture(t) + require.NoError(t, f.ledger(t, false).Close()) + + out, err := f.run(t, output.FormatJSON, "ask", "https://3.basecamp.com/999/buckets/456/todos/789#__recording_1011") + require.NoError(t, err, out) + var first struct { + Data connector.AskResult `json:"data"` + } + require.NoError(t, json.Unmarshal([]byte(out), &first), out) + assert.Equal(t, int64(1011), first.Data.RecordingID) + assert.False(t, first.Data.Already) + + out, err = f.run(t, output.FormatJSON, "ask", "1011") + require.NoError(t, err, out) + var again struct { + Data connector.AskResult `json:"data"` + } + require.NoError(t, json.Unmarshal([]byte(out), &again), out) + assert.True(t, again.Data.Already) + + _, err = f.run(t, output.FormatJSON, "ask", "nope") + require.Error(t, err) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) +} + func TestConnectShadowPromoteAndImport(t *testing.T) { f := newOperatorFixture(t) require.NoError(t, f.ledger(t, true).Close()) diff --git a/internal/connector/admission/boost_test.go b/internal/connector/admission/boost_test.go index 6158eb3af..46b5b4764 100644 --- a/internal/connector/admission/boost_test.go +++ b/internal/connector/admission/boost_test.go @@ -351,3 +351,57 @@ 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) + }) +} 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..6ddad5a7d 100644 --- a/internal/connector/admission/verdict.go +++ b/internal/connector/admission/verdict.go @@ -112,6 +112,14 @@ 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 + // 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 +455,9 @@ 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 = ev.RecordingID + } v.Snapshot = &Snapshot{ Type: summary.Type, Title: summary.Title, diff --git a/internal/connector/handoff.go b/internal/connector/handoff.go index 52abde89e..4ccf9c106 100644 --- a/internal/connector/handoff.go +++ b/internal/connector/handoff.go @@ -54,6 +54,12 @@ 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"` Content string `json:"content"` ContentUpdatedAt time.Time `json:"content_updated_at"` @@ -183,14 +189,24 @@ func handOffReady(ctx context.Context, opts HandoffOptions) error { func handOff(ctx context.Context, opts HandoffOptions, record Record) error { if record.CreatedAt.Before(opts.Started.Add(-HandoffGrace)) { opts.log().Info("connector: a request from before this run started is not handed off", "event_id", record.ID, "created_at", record.CreatedAt) + if err := opts.Ledger.releaseGo(ctx, record.ID); err != nil { + return err + } 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 // start, for one record. opts.log().Warn("connector: a request could not be handed off", "event_id", record.ID, "error", err) + if err := opts.Ledger.releaseGo(ctx, record.ID); err != nil { + return err + } return opts.Ledger.SetState(ctx, record.ID, StateDiscarded, ReasonUnreadable) } if err := opts.Lines.WriteLine(line); err != nil { @@ -200,7 +216,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"` @@ -237,6 +253,9 @@ 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, + // Only a boosted record can hold a go; the trigger is checked here + // too, so no other line ever says go. + Go: goes && record.Decision.Trigger == string(admission.TriggerBoosted), // 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..5d2a88818 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.EventID); err != nil { + return "", err + } + } } if l.hooks.VerdictCommitted != nil { committed := CommittedVerdict{ diff --git a/internal/connector/ledger_asks.go b/internal/connector/ledger_asks.go new file mode 100644 index 000000000..d3a6e40ce --- /dev/null +++ b/internal/connector/ledger_asks.go @@ -0,0 +1,131 @@ +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 `), 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 nobody was handed (its record closed unwritten, as from before +// this run) is given back (releaseGo). 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 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 <> ''), + 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"` + // 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. Declaring one twice is not an error, and does not reopen an +// ask whose go was taken. +func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, by string) (AskResult, error) { + switch { + case recordingID <= 0: + return AskResult{}, errors.New("connector: an ask names a recording") + 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) VALUES (?, ?, ?) ON CONFLICT (recording_id) DO NOTHING`, + recordingID, l.timestamp(), by) + 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 + if err := tx.QueryRowContext(ctx, `SELECT go_event_id FROM asks WHERE recording_id = ?`, recordingID).Scan(&goEvent); 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, 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, +// or one another event already took, gives nothing. +func takeGo(ctx context.Context, tx *sql.Tx, recordingID, eventID int64) (bool, error) { + res, err := tx.ExecContext(ctx, + `UPDATE asks SET go_event_id = ? WHERE recording_id = ? AND (go_event_id IS NULL OR go_event_id = ?)`, + eventID, recordingID, eventID) + 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 +} + +// releaseGo gives back a go eventID took and nobody will be handed: a record +// closed unwritten, because it was made before this run started or had +// nothing to hand off. The ask is open again, so the operator's next boost +// on it is the go, not a reply to one nobody saw. Released before the record +// is closed, so a crash between the two leaves the record to be closed again +// and the release a no-op. +func (l *Ledger) releaseGo(ctx context.Context, eventID int64) error { + return retryBusy(func() error { + if _, err := l.db.ExecContext(ctx, `UPDATE asks SET go_event_id = NULL WHERE go_event_id = ?`, eventID); err != nil { + return fmt.Errorf("connector: release the go for %d: %w", eventID, err) + } + return 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..5d583b43b --- /dev/null +++ b/internal/connector/ledger_asks_test.go @@ -0,0 +1,176 @@ +package connector + +import ( + "bytes" + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/connector/admission" +) + +const askRecordingID int64 = 10304028972 + +// 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 = askRecordingID + } + 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, "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, "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, "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, 1) + require.NoError(t, err) + assert.True(t, again, "the same event takes the same go") + other, err := takeGo(ctx, tx, askRecordingID, 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, "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, "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, "local:agent") + require.Error(t, err) + _, err = ledger.DeclareAsk(context.Background(), askRecordingID, " ") + 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, "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, "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 nobody was handed does not use up the ask: the operator's boost made +// while no connector ran is closed unwritten, and their next boost is the go. +func TestAGoNobodyWasHandedIsGivenBack(t *testing.T) { + ledger := newTestLedger(t) + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, "local:agent") + require.NoError(t, err) + commitBoost(t, ledger, 1, true) + require.True(t, goes(t, ledger, 1)) + + 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) + assert.False(t, goes(t, ledger, 1)) + + commitBoost(t, ledger, 2, true) + assert.True(t, goes(t, ledger, 2), "the ask is open again") +} diff --git a/internal/connector/request_contract_test.go b/internal/connector/request_contract_test.go index ac2e9bafc..21fcde42e 100644 --- a/internal/connector/request_contract_test.go +++ b/internal/connector/request_contract_test.go @@ -25,6 +25,7 @@ var requestLineContract = []string{ "content_updated_at", "event_id", "event_type", + "go", "owner", "recording.bucket_id", "recording.project_name", From 623d226d9de9f1924f6ba6221c5af1d028f3f82b Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 12 Oct 2026 10:13:53 -0700 Subject: [PATCH 02/11] connect: a go taken stays taken, even when its line is never handed off Giving the go back when its record was closed before_this_run could approve twice: a connector that printed the line and stopped before closing the record looks the same on restart, and the next boost would become a second go after the session already acted on the first. Losing a go costs the operator a mention; a second go costs a second action. The release is removed, and the skill says a boost given while the connector isn't running uses the ask up. Also: declare the ask before anything else after posting it, and list connect ask as taking a url or an id. --- internal/commands/connect.go | 2 +- internal/connector/handoff.go | 6 ------ internal/connector/ledger_asks.go | 22 +++++----------------- internal/connector/ledger_asks_test.go | 12 ++++++------ 4 files changed, 12 insertions(+), 30 deletions(-) diff --git a/internal/commands/connect.go b/internal/commands/connect.go index f11338ea1..6d32cac5a 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -64,7 +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 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`, diff --git a/internal/connector/handoff.go b/internal/connector/handoff.go index 4ccf9c106..16f7c3659 100644 --- a/internal/connector/handoff.go +++ b/internal/connector/handoff.go @@ -189,9 +189,6 @@ func handOffReady(ctx context.Context, opts HandoffOptions) error { func handOff(ctx context.Context, opts HandoffOptions, record Record) error { if record.CreatedAt.Before(opts.Started.Add(-HandoffGrace)) { opts.log().Info("connector: a request from before this run started is not handed off", "event_id", record.ID, "created_at", record.CreatedAt) - if err := opts.Ledger.releaseGo(ctx, record.ID); err != nil { - return err - } return opts.Ledger.SetState(ctx, record.ID, StateDiscarded, ReasonBeforeThisRun) } goes, err := opts.Ledger.isGo(ctx, record.ID) @@ -204,9 +201,6 @@ func handOff(ctx context.Context, opts HandoffOptions, record Record) error { // Returning the error instead would stop the connector on every // start, for one record. opts.log().Warn("connector: a request could not be handed off", "event_id", record.ID, "error", err) - if err := opts.Ledger.releaseGo(ctx, record.ID); err != nil { - return err - } return opts.Ledger.SetState(ctx, record.ID, StateDiscarded, ReasonUnreadable) } if err := opts.Lines.WriteLine(line); err != nil { diff --git a/internal/connector/ledger_asks.go b/internal/connector/ledger_asks.go index d3a6e40ce..9ca5ded3e 100644 --- a/internal/connector/ledger_asks.go +++ b/internal/connector/ledger_asks.go @@ -16,8 +16,11 @@ import ( // 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 nobody was handed (its record closed unwritten, as from before -// this run) is given back (releaseGo). Another boost on it afterwards — the operator's second, or one +// 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. @@ -105,21 +108,6 @@ func takeGo(ctx context.Context, tx *sql.Tx, recordingID, eventID int64) (bool, return n == 1, nil } -// releaseGo gives back a go eventID took and nobody will be handed: a record -// closed unwritten, because it was made before this run started or had -// nothing to hand off. The ask is open again, so the operator's next boost -// on it is the go, not a reply to one nobody saw. Released before the record -// is closed, so a crash between the two leaves the record to be closed again -// and the release a no-op. -func (l *Ledger) releaseGo(ctx context.Context, eventID int64) error { - return retryBusy(func() error { - if _, err := l.db.ExecContext(ctx, `UPDATE asks SET go_event_id = NULL WHERE go_event_id = ?`, eventID); err != nil { - return fmt.Errorf("connector: release the go for %d: %w", eventID, err) - } - return nil - }) -} - // isGo reports whether eventID took an ask's go. func (l *Ledger) isGo(ctx context.Context, eventID int64) (bool, error) { var goes bool diff --git a/internal/connector/ledger_asks_test.go b/internal/connector/ledger_asks_test.go index 5d583b43b..4807ffa4c 100644 --- a/internal/connector/ledger_asks_test.go +++ b/internal/connector/ledger_asks_test.go @@ -154,14 +154,14 @@ func TestOnlyABoostedLineSaysGo(t *testing.T) { assert.False(t, line.Go) } -// A go nobody was handed does not use up the ask: the operator's boost made -// while no connector ran is closed unwritten, and their next boost is the go. -func TestAGoNobodyWasHandedIsGivenBack(t *testing.T) { +// 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, "local:agent") require.NoError(t, err) commitBoost(t, ledger, 1, true) - require.True(t, goes(t, ledger, 1)) var out bytes.Buffer opts := handoffOptions(ledger, &out) @@ -169,8 +169,8 @@ func TestAGoNobodyWasHandedIsGivenBack(t *testing.T) { require.NoError(t, handOffReady(context.Background(), opts)) assert.Empty(t, out.String()) assert.Equal(t, ReasonBeforeThisRun, getRecord(t, ledger, 1).Reason) - assert.False(t, goes(t, ledger, 1)) commitBoost(t, ledger, 2, true) - assert.True(t, goes(t, ledger, 2), "the ask is open again") + 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") } From 0d331bf31bd50e06fb048b2755450bb1509b4d2e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 12 Oct 2026 11:09:32 -0700 Subject: [PATCH 03/11] connect ask: refuse a URL that does not plainly name one post in the agent's account A declaration that names the wrong recording fails silently later: the operator's boost on the real post arrives as a reply, not a go. A listing URL such as /chats/{id}/lines parsed to its parent's id and was declared with a success message. Follow the mutating-URL pattern files.go uses: a trusted host, the profile's account, and a single recording (the fragment's comment when there is one), else a usage error and nothing declared. --- internal/commands/connect_operator.go | 44 +++++++++++++++++++--- internal/commands/connect_operator_test.go | 38 +++++++++++++++++++ 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index b42f682e4..bea64b724 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -20,8 +20,10 @@ import ( "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/connector" "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, @@ -469,15 +471,14 @@ 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 { - raw, _ := extractCommentWithProject(args[0]) - id, err := strconv.ParseInt(raw, 10, 64) - if err != nil || id <= 0 { - return output.ErrUsage(fmt.Sprintf("No recording id in %q", args[0])) - } p, err := loadConnectProfile(cmd) if err != nil { return err } + id, err := askRecordingID(args[0], p) + if err != nil { + return err + } ledger, done, err := openConnectLedger(cmd.Context(), p, false) if err != nil { return err @@ -499,6 +500,39 @@ text to decide it is an ask: this declaration is the only way to make one.`, } } +// askRecordingID is the recording an ask argument names: a bare id, or a URL +// for one post in the agent's own account. 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. +func askRecordingID(arg string, p connectProfile) (int64, error) { + raw := strings.TrimSpace(arg) + if urlarg.IsURL(raw) { + if !hostutil.IsTrustedBasecampHost(raw, p.app.Config.BaseURL) { + return 0, output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") + } + parsed := urlarg.Parse(raw) + switch { + case parsed == nil: + return 0, output.ErrUsage(fmt.Sprintf("No recording in %q", arg)) + case parsed.AccountID != "" && parsed.AccountID != p.file.AccountID: + return 0, output.ErrUsage(fmt.Sprintf("URL is for account %s, but this agent is in account %s", parsed.AccountID, p.file.AccountID)) + case parsed.CommentID != "": + raw = parsed.CommentID + case parsed.IsCollection || parsed.RecordingID == "": + return 0, output.ErrUsage("URL names a listing, not the one post the ask is") + default: + raw = parsed.RecordingID + } + } + id, err := strconv.ParseInt(raw, 10, 64) + if err != nil || id <= 0 { + return 0, output.ErrUsage(fmt.Sprintf("No recording id in %q", arg)) + } + return id, nil +} + // --- release -------------------------------------------------------------- func newConnectReleaseCmd() *cobra.Command { diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index ccd740132..994fba779 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -195,6 +195,44 @@ func TestConnectAskDeclaresTheCommentTheURLNames(t *testing.T) { _, err = f.run(t, output.FormatJSON, "ask", "nope") require.Error(t, err) assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + + // A chat line, in the web's @ form, is the line and not its Campfire. + out, err = f.run(t, output.FormatJSON, "ask", "https://3.basecamp.com/999/buckets/456/chats/789@1213") + require.NoError(t, err, out) + var line struct { + Data connector.AskResult `json:"data"` + } + require.NoError(t, json.Unmarshal([]byte(out), &line), out) + assert.Equal(t, int64(1213), line.Data.RecordingID) +} + +// A URL that does not plainly name one post in the agent's account is +// refused, and declares nothing: a wrong declaration would only fail later, +// silently, when the operator's boost on the real post came as a reply. +func TestConnectAskRefusesAURLThatIsNotOnePost(t *testing.T) { + f := newOperatorFixture(t) + require.NoError(t, f.ledger(t, false).Close()) + + for name, url := range map[string]string{ + "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", + "a listing": "https://3.basecamp.com/999/buckets/456/chats/789/lines", + } { + t.Run(name, func(t *testing.T) { + out, err := f.run(t, output.FormatJSON, "ask", url) + require.Error(t, err, out) + assert.Equal(t, output.CodeUsage, usageError(t, err).Code) + }) + } + + 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 n int + require.NoError(t, db.QueryRowContext(context.Background(), `SELECT COUNT(*) FROM asks`).Scan(&n)) + assert.Zero(t, n, "nothing was declared") } func TestConnectShadowPromoteAndImport(t *testing.T) { From 88f1a4fae648053ec56a26f758f4d0d1e76a89c8 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 12 Oct 2026 14:48:06 -0700 Subject: [PATCH 04/11] connect: Owner and the go are separate approvals; say so where Owner is set The Owner rationale still said a boost names nothing it approves, which the declared-ask go made untrue. Owner stays false for every boost: it is the owner's word on the connector, and a go approves only what its ask names. --- internal/connector/admission/verdict.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/connector/admission/verdict.go b/internal/connector/admission/verdict.go index 6ddad5a7d..a65f02418 100644 --- a/internal/connector/admission/verdict.go +++ b/internal/connector/admission/verdict.go @@ -63,7 +63,8 @@ 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"` @@ -467,9 +468,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, From c2de25d48f9085764f67e81eb53edbb6a55bda45 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 12 Oct 2026 21:03:07 -0700 Subject: [PATCH 05/11] connect: an ask edited after it was declared gives no go The go was keyed by recording id alone, and the worker reads the live post, so an edit after the operator's boost would change what the go approved without the operator seeing it. connect ask now reads the post once, as the agent, through the same summary read admission makes of the boosted recording, refuses one the agent did not write or that is not published, and stores the SHA-256 of its content exactly as the read returns it, with nothing normalized. At admission the operator's boost carries the hash of the content that decision read; the ledger gives the go only when it equals the declared one. An edited ask's boost is an ordinary boosted reply with go false, and the ask is not used up. Re-declaring keeps the first hash, so an edited ask stays void: the agent posts a new one. connect ask takes the post's URL, not a bare id: the read routes by the type the URL names. Migration 15 gains content_hash; it has not shipped. --- internal/commands/connect.go | 2 +- internal/commands/connect_operator.go | 108 +++++++++++++------ internal/commands/connect_operator_test.go | 114 +++++++++++++------- internal/commands/connect_setup_test.go | 7 ++ internal/connector/admission/ask.go | 18 ++++ internal/connector/admission/verdict.go | 7 +- internal/connector/ledger_admission.go | 2 +- internal/connector/ledger_admission_test.go | 78 ++++++++++++++ internal/connector/ledger_asks.go | 42 +++++--- internal/connector/ledger_asks_test.go | 81 +++++++++++--- 10 files changed, 359 insertions(+), 100 deletions(-) create mode 100644 internal/connector/admission/ask.go diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 6d32cac5a..f11338ea1 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -64,7 +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 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`, diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index bea64b724..ec88c5211 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -19,6 +19,7 @@ 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" @@ -457,7 +458,7 @@ outcome is unknown. A lifecycle message still pending for it is not sent.`, func newConnectAskCmd() *cobra.Command { return &cobra.Command{ - Use: "ask ", + 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 @@ -465,9 +466,13 @@ 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 URL or id the post returned. 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.`, +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 { @@ -475,62 +480,101 @@ text to decide it is an ask: this declaration is the only way to make one.`, if err != nil { return err } - id, err := askRecordingID(args[0], p) + 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)) + } ledger, done, err := openConnectLedger(cmd.Context(), p, false) if err != nil { return err } defer done() - res, err := ledger.DeclareAsk(cmd.Context(), id, operatorName()) + res, err := ledger.DeclareAsk(cmd.Context(), ref.RecordingID, admission.AskContentHash(post.Content), operatorName()) if err != nil { return err } - summary := fmt.Sprintf("Recording %d is an ask: the operator's boost on it is the go", id) + 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", id, res.GoEventID) + 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", id) + summary = fmt.Sprintf("Recording %d was already an ask", ref.RecordingID) } return p.app.OK(res, output.WithSummary(summary)) }, } } -// askRecordingID is the recording an ask argument names: a bare id, or a URL -// for one post in the agent's own account. A declaration that names the wrong +// 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. -func askRecordingID(arg string, p connectProfile) (int64, error) { +// 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) { - if !hostutil.IsTrustedBasecampHost(raw, p.app.Config.BaseURL) { - return 0, output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") - } - parsed := urlarg.Parse(raw) - switch { - case parsed == nil: - return 0, output.ErrUsage(fmt.Sprintf("No recording in %q", arg)) - case parsed.AccountID != "" && parsed.AccountID != p.file.AccountID: - return 0, output.ErrUsage(fmt.Sprintf("URL is for account %s, but this agent is in account %s", parsed.AccountID, p.file.AccountID)) - case parsed.CommentID != "": - raw = parsed.CommentID - case parsed.IsCollection || parsed.RecordingID == "": - return 0, output.ErrUsage("URL names a listing, not the one post the ask is") - default: - raw = parsed.RecordingID + 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 != "" && 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 } - id, err := strconv.ParseInt(raw, 10, 64) - if err != nil || id <= 0 { - return 0, output.ErrUsage(fmt.Sprintf("No recording id in %q", arg)) + 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)) } - return id, nil + ref.BucketID, ref.RecordingID = bucket, recording + return ref, nil } // --- release -------------------------------------------------------------- diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 994fba779..f7e3d4a95 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,70 +170,101 @@ func TestConnectDiscardAndRelease(t *testing.T) { assert.Equal(t, connector.ReasonByOperator, two.Reason) } -// connect ask declares the comment a URL names, not the recording it hangs -// off, and declaring it again leaves the first declaration standing. -func TestConnectAskDeclaresTheCommentTheURLNames(t *testing.T) { +// 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, + } +} + +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 +} + +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", "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 struct { - Data connector.AskResult `json:"data"` - } + 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.AskContentHash(content), askHashIn(t, f, 1011), "the hash is of the content as read") - out, err = f.run(t, output.FormatJSON, "ask", "1011") + // 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 struct { - Data connector.AskResult `json:"data"` - } + var again askOut require.NoError(t, json.Unmarshal([]byte(out), &again), out) assert.True(t, again.Data.Already) + assert.False(t, again.Data.Edited) - _, err = f.run(t, output.FormatJSON, "ask", "nope") - require.Error(t, err) - assert.Equal(t, output.CodeUsage, usageError(t, err).Code) - - // A chat line, in the web's @ form, is the line and not its Campfire. - out, err = f.run(t, output.FormatJSON, "ask", "https://3.basecamp.com/999/buckets/456/chats/789@1213") + 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 line struct { - Data connector.AskResult `json:"data"` - } - require.NoError(t, json.Unmarshal([]byte(out), &line), out) - assert.Equal(t, int64(1213), line.Data.RecordingID) + var edited askOut + require.NoError(t, json.Unmarshal([]byte(out), &edited), out) + assert.True(t, edited.Data.Edited) + assert.Equal(t, admission.AskContentHash(content), askHashIn(t, f, 1011), "the first declaration stands") } -// A URL that does not plainly name one post in the agent's account is -// refused, and declares nothing: a wrong declaration would only fail later, -// silently, when the operator's boost on the real post came as a reply. -func TestConnectAskRefusesAURLThatIsNotOnePost(t *testing.T) { +// 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) { f := newOperatorFixture(t) require.NoError(t, f.ledger(t, false).Close()) - - for name, url := range map[string]string{ - "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", - "a listing": "https://3.basecamp.com/999/buckets/456/chats/789/lines", + f.askComment(1011, setupOperatorPerson, "
the operator's comment
") + + for name, arg := range map[string]string{ + "someone else's post": "https://3.basecamp.com/999/buckets/456/todos/789#__recording_1011", + "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", url) + out, err := f.run(t, output.FormatJSON, "ask", arg) require.Error(t, err, out) assert.Equal(t, output.CodeUsage, usageError(t, err).Code) }) } - - 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 n int - require.NoError(t, db.QueryRowContext(context.Background(), `SELECT COUNT(*) FROM asks`).Scan(&n)) - assert.Zero(t, n, "nothing was declared") + assert.Empty(t, askHashIn(t, f, 1011), "nothing was declared") } func TestConnectShadowPromoteAndImport(t *testing.T) { 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..61830ba4a --- /dev/null +++ b/internal/connector/admission/ask.go @@ -0,0 +1,18 @@ +package admission + +import ( + "crypto/sha256" + "encoding/hex" +) + +// AskContentHash is the fingerprint an ask is declared with and checked +// against: SHA-256 of the recording's content exactly as the summary read +// returns it, hex-encoded. Nothing is normalized — no whitespace folded, no +// HTML canonicalized — because both sides read the same field through the +// same read (basecamp.RecordingsService.Summarize), so an unedited post +// hashes the same, and any change at all, an edit's or Basecamp's, voids the +// go rather than approving words the operator may not have seen. +func AskContentHash(content string) string { + sum := sha256.Sum256([]byte(content)) + return hex.EncodeToString(sum[:]) +} diff --git a/internal/connector/admission/verdict.go b/internal/connector/admission/verdict.go index a65f02418..95c1d3b6e 100644 --- a/internal/connector/admission/verdict.go +++ b/internal/connector/admission/verdict.go @@ -120,6 +120,11 @@ type Verdict struct { // 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 AskContentHash of the boosted recording's 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 // agentCaps is set on a verdict admitting an allowed agent's mention: // the committer counts it against the caps before writing it. @@ -457,7 +462,7 @@ func (a *Admitter) Decide(ctx context.Context, ev Event) (out Verdict, err error } v.State = StateAdmitted if rule.Trigger == TriggerBoosted && ev.PerformedByID == nil && ev.Performer() == policy.Trust.OperatorID { - v.AskRecordingID = ev.RecordingID + v.AskRecordingID, v.AskContentHash = ev.RecordingID, AskContentHash(summary.Content) } v.Snapshot = &Snapshot{ Type: summary.Type, diff --git a/internal/connector/ledger_admission.go b/internal/connector/ledger_admission.go index 5d2a88818..139ca4ff9 100644 --- a/internal/connector/ledger_admission.go +++ b/internal/connector/ledger_admission.go @@ -194,7 +194,7 @@ func (a Admission) commit(ctx context.Context, v admission.Verdict, state Record // 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.EventID); err != nil { + if _, err := takeGo(ctx, tx, v.AskRecordingID, v.AskContentHash, v.EventID); err != nil { return "", err } } diff --git a/internal/connector/ledger_admission_test.go b/internal/connector/ledger_admission_test.go index 5041efe75..2e45f959a 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,80 @@ 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 = "
Shall I push abc123 to fix-date-picker?
" + run := func(t *testing.T, 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: "Comment", Title: "Shall I?", + 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: "Comment"}, + }}, + } + 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.AskContentHash(declared), "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", func(t *testing.T) { + _, line := run(t, declared) + assert.Equal(t, "boosted", line.Trigger) + assert.True(t, line.Go) + assert.Equal(t, commentID, line.Recording.RecordingID) + }) + + t.Run("edited since it was declared: a reply, and the ask stays open", func(t *testing.T) { + ledger, line := run(t, "
Shall I push abc123 and deploy production?
") + assert.Equal(t, "boosted", line.Trigger) + assert.False(t, line.Go) + 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 index 9ca5ded3e..1e5b10a87 100644 --- a/internal/connector/ledger_asks.go +++ b/internal/connector/ledger_asks.go @@ -25,6 +25,13 @@ import ( // 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.AskContentHash), 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 @@ -36,6 +43,7 @@ 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 <> ''), go_event_id INTEGER UNIQUE REFERENCES events (id) ); ` @@ -46,18 +54,24 @@ type AskResult struct { // 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. Declaring one twice is not an error, and does not reopen an -// ask whose go was taken. -func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, by string) (AskResult, error) { +// 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. +func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, contentHash, 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 strings.TrimSpace(by) == "": return AskResult{}, errors.New("connector: an ask records who declared it") } @@ -69,8 +83,8 @@ func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, by string) ( } defer func() { _ = tx.Rollback() }() res, err := tx.ExecContext(ctx, - `INSERT INTO asks (recording_id, declared_at, declared_by) VALUES (?, ?, ?) ON CONFLICT (recording_id) DO NOTHING`, - recordingID, l.timestamp(), by) + `INSERT INTO asks (recording_id, declared_at, declared_by, content_hash) VALUES (?, ?, ?, ?) ON CONFLICT (recording_id) DO NOTHING`, + recordingID, l.timestamp(), by, contentHash) if err != nil { return fmt.Errorf("connector: declare ask on %d: %w", recordingID, err) } @@ -78,14 +92,17 @@ func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, by string) ( if err != nil { return fmt.Errorf("connector: declare ask on %d: %w", recordingID, err) } - var goEvent sql.NullInt64 - if err := tx.QueryRowContext(ctx, `SELECT go_event_id FROM asks WHERE recording_id = ?`, recordingID).Scan(&goEvent); err != nil { + 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, GoEventID: goEvent.Int64} + out = AskResult{RecordingID: recordingID, Already: n == 0, Edited: declared != contentHash, GoEventID: goEvent.Int64} return nil }) return out, err @@ -93,11 +110,12 @@ func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, by string) ( // takeGo gives a declared ask's go to eventID, inside the verdict's // transaction, and reports whether eventID holds it. An ask nobody declared, -// or one another event already took, gives nothing. -func takeGo(ctx context.Context, tx *sql.Tx, recordingID, eventID int64) (bool, error) { +// 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. +func takeGo(ctx context.Context, tx *sql.Tx, recordingID int64, contentHash string, eventID int64) (bool, error) { res, err := tx.ExecContext(ctx, - `UPDATE asks SET go_event_id = ? WHERE recording_id = ? AND (go_event_id IS NULL OR go_event_id = ?)`, - eventID, recordingID, eventID) + `UPDATE asks SET go_event_id = ? WHERE recording_id = ? AND content_hash = ? AND (go_event_id IS NULL OR go_event_id = ?)`, + eventID, recordingID, contentHash, eventID) if err != nil { return false, fmt.Errorf("connector: take the go on %d for %d: %w", recordingID, eventID, err) } diff --git a/internal/connector/ledger_asks_test.go b/internal/connector/ledger_asks_test.go index 4807ffa4c..1b1208702 100644 --- a/internal/connector/ledger_asks_test.go +++ b/internal/connector/ledger_asks_test.go @@ -3,6 +3,7 @@ package connector import ( "bytes" "context" + "database/sql" "testing" "time" @@ -14,6 +15,9 @@ import ( const askRecordingID int64 = 10304028972 +// askHash is the declared ask's content hash, as admission computes it. +var askHash = admission.AskContentHash("
Shall I push abc123 to fix-date-picker?
") + // 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 { @@ -21,7 +25,7 @@ func boostVerdict(id int64, operator bool) admission.Verdict { v.EventType, v.Trigger, v.Acknowledge = "boost.created", admission.TriggerBoosted, false v.Snapshot.Content = "yes, go" if operator { - v.AskRecordingID = askRecordingID + v.AskRecordingID, v.AskContentHash = askRecordingID, askHash } return v } @@ -46,7 +50,7 @@ func goes(t *testing.T, ledger *Ledger, id int64) bool { func TestOneAskGetsOneGo(t *testing.T) { ledger := newTestLedger(t) ctx := context.Background() - declared, err := ledger.DeclareAsk(ctx, askRecordingID, "local:agent") + declared, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") require.NoError(t, err) assert.False(t, declared.Already) @@ -58,7 +62,7 @@ func TestOneAskGetsOneGo(t *testing.T) { 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, "local:agent") + again, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "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") @@ -70,17 +74,17 @@ func TestOneAskGetsOneGo(t *testing.T) { func TestTheGoIsTheEventsToKeep(t *testing.T) { ledger := newTestLedger(t) ctx := context.Background() - _, err := ledger.DeclareAsk(ctx, askRecordingID, "local:agent") + _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "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, 1) + again, err := takeGo(ctx, tx, askRecordingID, askHash, 1) require.NoError(t, err) assert.True(t, again, "the same event takes the same go") - other, err := takeGo(ctx, tx, askRecordingID, 2) + other, err := takeGo(ctx, tx, askRecordingID, askHash, 2) require.NoError(t, err) assert.False(t, other) } @@ -91,14 +95,14 @@ func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.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, "local:agent") + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "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, "local:agent") + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") require.NoError(t, err) commitBoost(t, ledger, 1, false) commitBoost(t, ledger, 2, true) @@ -108,9 +112,9 @@ func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.T) { t.Run("an ask names a recording and who declared it", func(t *testing.T) { ledger := newTestLedger(t) - _, err := ledger.DeclareAsk(context.Background(), 0, "local:agent") + _, err := ledger.DeclareAsk(context.Background(), 0, askHash, "local:agent") require.Error(t, err) - _, err = ledger.DeclareAsk(context.Background(), askRecordingID, " ") + _, err = ledger.DeclareAsk(context.Background(), askRecordingID, askHash, " ") require.Error(t, err) }) } @@ -118,7 +122,7 @@ func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.T) { // 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, "local:agent") + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") require.NoError(t, err) commitBoost(t, ledger, 1, true) commitBoost(t, ledger, 2, true) @@ -159,7 +163,7 @@ func TestOnlyABoostedLineSaysGo(t *testing.T) { // 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, "local:agent") + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") require.NoError(t, err) commitBoost(t, ledger, 1, true) @@ -174,3 +178,56 @@ func TestAGoClosedUnwrittenIsNotGivenBack(t *testing.T) { 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, "local:agent") + require.NoError(t, err) + + edited := boostVerdict(1, true) + edited.AskContentHash = admission.AskContentHash("
Shall I push abc123 and deploy production?
") + 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, "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, 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, "local:agent") + require.NoError(t, err) + edited := boostVerdict(1, true) + edited.AskContentHash = admission.AskContentHash("
something else
") + 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) +} From fb43b2f55ec653beb375ab0203aa2e4eacdd7bd2 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 12 Oct 2026 21:14:54 -0700 Subject: [PATCH 06/11] connect ask: test URL refusals against the agent's own post; say when later boosts lose the go The URL cases ran against a post the operator wrote, so the creator check refused them all and a regressed URL guard would have passed. They now run against the agent's own post; someone else's post is its own case. The skill now says later boosts arrive with go false once the operator's boost has taken the go: another person's boost doesn't use the ask up. --- internal/commands/connect_operator_test.go | 29 ++++++++++++++++------ 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index f7e3d4a95..0d5620ec6 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -246,17 +246,29 @@ func TestConnectAskFingerprintsTheCommentTheURLNames(t *testing.T) { // 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()) - f.askComment(1011, setupOperatorPerson, "
the operator's comment
") - + for _, id := range []int64{789, 1011} { + f.askComment(id, setupAgentPerson, "
the agent's comment
") + } for name, arg := range map[string]string{ - "someone else's post": "https://3.basecamp.com/999/buckets/456/todos/789#__recording_1011", - "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", + "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) @@ -264,6 +276,7 @@ func TestConnectAskRefusesWhatIsNotTheAgentsOnePost(t *testing.T) { 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") } From 14a2bf6c3d165ba83eca8007ca58b35b9a8bf9c3 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 12 Oct 2026 23:45:31 -0700 Subject: [PATCH 07/11] connect: a go carries the ask's words as read, and they include its title The fingerprint covered content alone, and the worker read the live post, so a title edit or an edit after admission could change what a go approved without the operator seeing it. AskFingerprint now hashes a titled post's title and content, each length-prefixed (":<len>:<content>") so the two can't run together; a comment or chat line, whose title is not its own, hashes its content alone. Both ends compute it from the same summary read. Admission keeps the ask's title and content from that same read in the snapshot, and a go is handed off with them under "ask": the exact words the ledger matched against the declaration. The skill acts on those words only, never on a fresh read of the post; "recording" stays for replying. A go whose record carries no such words is not handed off as a go. --- internal/commands/connect_operator.go | 2 +- internal/commands/connect_operator_test.go | 4 +- internal/connector/admission/ask.go | 48 +++++++++++++++++---- internal/connector/admission/boost_test.go | 15 +++++++ internal/connector/admission/verdict.go | 23 ++++++++-- internal/connector/handoff.go | 32 ++++++++++++-- internal/connector/ledger_admission_test.go | 28 +++++++++--- internal/connector/ledger_asks.go | 2 +- internal/connector/ledger_asks_test.go | 14 ++++-- internal/connector/request_contract_test.go | 2 + 10 files changed, 140 insertions(+), 30 deletions(-) diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index ec88c5211..7b72416c8 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -505,7 +505,7 @@ only way to make one.`, return err } defer done() - res, err := ledger.DeclareAsk(cmd.Context(), ref.RecordingID, admission.AskContentHash(post.Content), operatorName()) + res, err := ledger.DeclareAsk(cmd.Context(), ref.RecordingID, admission.AskFingerprint(post.Type, post.Title, post.Content), operatorName()) if err != nil { return err } diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 0d5620ec6..51f2b94b5 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -224,7 +224,7 @@ func TestConnectAskFingerprintsTheCommentTheURLNames(t *testing.T) { 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.AskContentHash(content), askHashIn(t, f, 1011), "the hash is of the content as read") + assert.Equal(t, admission.AskFingerprint("Comment", "", content), askHashIn(t, f, 1011), "the hash is of the content as read") // The same post, read again: the same hash, so nothing is edited. out, err = f.run(t, output.FormatJSON, "ask", url) @@ -240,7 +240,7 @@ func TestConnectAskFingerprintsTheCommentTheURLNames(t *testing.T) { var edited askOut require.NoError(t, json.Unmarshal([]byte(out), &edited), out) assert.True(t, edited.Data.Edited) - assert.Equal(t, admission.AskContentHash(content), askHashIn(t, f, 1011), "the first declaration stands") + 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 diff --git a/internal/connector/admission/ask.go b/internal/connector/admission/ask.go index 61830ba4a..4370239cf 100644 --- a/internal/connector/admission/ask.go +++ b/internal/connector/admission/ask.go @@ -3,16 +3,46 @@ package admission import ( "crypto/sha256" "encoding/hex" + "strconv" + "strings" ) -// AskContentHash is the fingerprint an ask is declared with and checked -// against: SHA-256 of the recording's content exactly as the summary read -// returns it, hex-encoded. Nothing is normalized — no whitespace folded, no -// HTML canonicalized — because both sides read the same field through the -// same read (basecamp.RecordingsService.Summarize), so an unedited post -// hashes the same, and any change at all, an edit's or Basecamp's, voids the -// go rather than approving words the operator may not have seen. -func AskContentHash(content string) string { - sum := sha256.Sum256([]byte(content)) +// 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>:<title><len>:<content>"), so no split of one string into the two +// fields reads as another. A comment or a chat line has no title of its own +// (a comment's title is its parent's, which a rename of the card would +// change), so its 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 { + return recordingType != "Comment" && !strings.HasPrefix(recordingType, "Chat::Lines::") +} + +// AskTitle is the title an ask is approved by: its own, or none for a +// comment or a chat line. +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 46b5b4764..b32d924ba 100644 --- a/internal/connector/admission/boost_test.go +++ b/internal/connector/admission/boost_test.go @@ -405,3 +405,18 @@ func TestOnlyTheOperatorsBoostMayBeAGo(t *testing.T) { 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.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/verdict.go b/internal/connector/admission/verdict.go index 95c1d3b6e..96dbba440 100644 --- a/internal/connector/admission/verdict.go +++ b/internal/connector/admission/verdict.go @@ -68,6 +68,18 @@ type Snapshot struct { // 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. @@ -120,8 +132,8 @@ type Verdict struct { // 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 AskContentHash of the boosted recording's content as - // this decision read it. The ledger gives the go only when it equals the + // 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 @@ -462,7 +474,7 @@ func (a *Admitter) Decide(ctx context.Context, ev Event) (out Verdict, err error } v.State = StateAdmitted if rule.Trigger == TriggerBoosted && ev.PerformedByID == nil && ev.Performer() == policy.Trust.OperatorID { - v.AskRecordingID, v.AskContentHash = ev.RecordingID, AskContentHash(summary.Content) + v.AskRecordingID, v.AskContentHash = ev.RecordingID, AskFingerprint(summary.Type, summary.Title, summary.Content) } v.Snapshot = &Snapshot{ Type: summary.Type, @@ -496,6 +508,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 16f7c3659..c64c0c397 100644 --- a/internal/connector/handoff.go +++ b/internal/connector/handoff.go @@ -60,11 +60,23 @@ type HandoffLine struct { // 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"` @@ -221,6 +233,10 @@ func handoffLine(record Record, agentID int64, goes bool) (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") @@ -228,6 +244,17 @@ func handoffLine(record Record, agentID int64, goes bool) (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, @@ -247,9 +274,8 @@ func handoffLine(record Record, agentID int64, goes bool) (HandoffLine, error) { Role: handoffRole(snapshot.Role), Owner: snapshot.Owner && handoffRole(snapshot.Role) == string(admission.RoleOperator), Acknowledge: record.Decision.Acknowledge, - // Only a boosted record can hold a go; the trigger is checked here - // too, so no other line ever says go. - Go: goes && record.Decision.Trigger == string(admission.TriggerBoosted), + 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_admission_test.go b/internal/connector/ledger_admission_test.go index 2e45f959a..3b8bfb9f9 100644 --- a/internal/connector/ledger_admission_test.go +++ b/internal/connector/ledger_admission_test.go @@ -917,14 +917,14 @@ func TestAnUnreadableConfigHoldsItsRecordsUntilTheFileAndAPersonAreBack(t *testi 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, content string) (*Ledger, HandoffLine) { + 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: "Comment", Title: "Shall I?", + 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}, @@ -934,7 +934,7 @@ func TestAGoIsOnlyForTheAskAsDeclared(t *testing.T) { 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: "Comment"}, + Recording: &basecamp.Parent{ID: commentID, Type: typ}, }}, } admitter, err := admission.NewAdmitter(admission.Policy{ @@ -943,7 +943,7 @@ func TestAGoIsOnlyForTheAskAsDeclared(t *testing.T) { 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.AskContentHash(declared), "local:agent") + _, err = ledger.DeclareAsk(context.Background(), commentID, admission.AskFingerprint(typ, declaredTitle, declared), "local:agent") require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) @@ -971,17 +971,31 @@ func TestAGoIsOnlyForTheAskAsDeclared(t *testing.T) { return ledger, lines[0] } - t.Run("unedited: the go", func(t *testing.T) { - _, line := run(t, declared) + 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, "<div>Shall I push abc123 and deploy production?</div>") + 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 index 1e5b10a87..6a7685d25 100644 --- a/internal/connector/ledger_asks.go +++ b/internal/connector/ledger_asks.go @@ -26,7 +26,7 @@ import ( // 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.AskContentHash), and the go is given only while the +// 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 diff --git a/internal/connector/ledger_asks_test.go b/internal/connector/ledger_asks_test.go index 1b1208702..4ed487080 100644 --- a/internal/connector/ledger_asks_test.go +++ b/internal/connector/ledger_asks_test.go @@ -15,8 +15,11 @@ import ( const askRecordingID int64 = 10304028972 -// askHash is the declared ask's content hash, as admission computes it. -var askHash = admission.AskContentHash("<div>Shall I push abc123 to fix-date-picker?</div>") +// 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. @@ -26,6 +29,7 @@ func boostVerdict(id int64, operator bool) admission.Verdict { v.Snapshot.Content = "yes, go" if operator { v.AskRecordingID, v.AskContentHash = askRecordingID, askHash + v.Snapshot.Ask = &admission.AskCopy{Content: askText} } return v } @@ -140,6 +144,8 @@ func TestTheGoIsHandedOffAsSuch(t *testing.T) { 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) @@ -188,7 +194,7 @@ func TestAnAskEditedSinceItWasDeclaredGivesNoGo(t *testing.T) { require.NoError(t, err) edited := boostVerdict(1, true) - edited.AskContentHash = admission.AskContentHash("<div>Shall I push abc123 and deploy production?</div>") + 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) @@ -219,7 +225,7 @@ func TestAnEditedAsksBoostIsHandedOffAsAReply(t *testing.T) { _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") require.NoError(t, err) edited := boostVerdict(1, true) - edited.AskContentHash = admission.AskContentHash("<div>something else</div>") + edited.AskContentHash = admission.AskFingerprint("Comment", "", "<div>something else</div>") seenRecord(t, ledger, 1) _, err = ledger.Admission().Commit(context.Background(), edited) require.NoError(t, err) diff --git a/internal/connector/request_contract_test.go b/internal/connector/request_contract_test.go index 21fcde42e..f9ba5671f 100644 --- a/internal/connector/request_contract_test.go +++ b/internal/connector/request_contract_test.go @@ -21,6 +21,8 @@ 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", From 23a9fc6e140bb7aae34de27f30418459b3762f9b Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Tue, 13 Oct 2026 00:08:52 -0700 Subject: [PATCH 08/11] connect: a check-in answer's ask is its content, not its question's title An answer's title is its parent question's: hashing it let a rename of the question void an unchanged ask, and handing it off put the question's words in ask.title as if the operator had approved them. A check-in answer now fingerprints and hands off its content alone, like a comment or a chat line. --- internal/connector/admission/ask.go | 17 ++++++++++++----- internal/connector/admission/boost_test.go | 3 +++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/internal/connector/admission/ask.go b/internal/connector/admission/ask.go index 4370239cf..18d20bc94 100644 --- a/internal/connector/admission/ask.go +++ b/internal/connector/admission/ask.go @@ -19,9 +19,10 @@ import ( // 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>:<title><len>:<content>"), so no split of one string into the two -// fields reads as another. A comment or a chat line has no title of its own -// (a comment's title is its parent's, which a rename of the card would -// change), so its content alone is hashed, bare. +// 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) { @@ -35,11 +36,17 @@ func AskFingerprint(recordingType, title, content string) string { // askHasTitle says a recording type's title is its own words. func askHasTitle(recordingType string) bool { - return recordingType != "Comment" && !strings.HasPrefix(recordingType, "Chat::Lines::") + 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 or a chat line. +// comment, a chat line or a check-in answer. func AskTitle(recordingType, title string) string { if askHasTitle(recordingType) { return title diff --git a/internal/connector/admission/boost_test.go b/internal/connector/admission/boost_test.go index b32d924ba..ac18d1144 100644 --- a/internal/connector/admission/boost_test.go +++ b/internal/connector/admission/boost_test.go @@ -413,6 +413,9 @@ func TestAskFingerprint(t *testing.T) { 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"), From 84ca8045cfa024b8975a0f49dcc099f465a6953f Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Tue, 13 Oct 2026 00:37:48 -0700 Subject: [PATCH 09/11] connect skill: the declared-ask go, on the split skill and inside the cut main moved the subagent's instructions into subagent.md and capped the per-request part of SKILL.md at 14,000 characters (the compaction cut), with main itself at 13,995. This branch's skill changes were written against the single file, so they are re-applied here in one place. SKILL.md carries only what the request line needs: the example's "go" and "ask" keys and a one-line pointer, paid for by tightening the reply_to, recording and log-line wording and a shorter example. The declaration, the go, the ask's words and the boosted trigger live in subagent.md, where the subagent handling the request reads them. --- skills/basecamp-connect/SKILL.md | 13 +++---- skills/basecamp-connect/subagent.md | 56 +++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 4661bfbf7..d23b8a03f 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. @@ -117,11 +117,10 @@ thing to check. - `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 +- **`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..ee3c4686b 100644 --- a/skills/basecamp-connect/subagent.md +++ b/skills/basecamp-connect/subagent.md @@ -185,9 +185,28 @@ 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 decided before + the declaration is only a reply, 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 +222,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 +259,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 From eab59c96a9bc6ecf1ef4c3c0e21ee8034a938b5f Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Tue, 13 Oct 2026 00:56:32 -0700 Subject: [PATCH 10/11] connect: a boost made before its ask was declared is no go, however late it commits takeGo checked only that a declaration existed when the boost's verdict committed, so a boost the connector held (blocked for a retry, or seen late) from before the declaration could take a go nobody gave. The go now also requires the event's Basecamp time to be after the declaration's. Skew between the two clocks can only turn a boost given in the seconds after a declaration into a reply. Also: connect ask compares the URL's account the way the profile's is compared (accountIDsEqual), so "0999" is account 999; and the request line's acknowledge says a boost is context unless it is a go. --- internal/commands/connect_operator.go | 2 +- internal/commands/connect_operator_test.go | 4 ++ internal/connector/ledger_admission_test.go | 2 + internal/connector/ledger_asks.go | 13 +++++- internal/connector/ledger_asks_test.go | 52 +++++++++++++++++---- skills/basecamp-connect/SKILL.md | 2 +- skills/basecamp-connect/subagent.md | 5 +- 7 files changed, 64 insertions(+), 16 deletions(-) diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index 7b72416c8..2485462b3 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -555,7 +555,7 @@ func askRef(arg string, p connectProfile) (basecamp.RecordingRef, error) { switch { case parsed == nil: return ref, output.ErrUsage(fmt.Sprintf("No recording in %q", arg)) - case parsed.AccountID != "" && parsed.AccountID != p.file.AccountID: + 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" diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 51f2b94b5..452f82b72 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -234,6 +234,10 @@ func TestConnectAskFingerprintsTheCommentTheURLNames(t *testing.T) { 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, "<div>Shall I push abc123 and deploy production?</div>") out, err = f.run(t, output.FormatJSON, "ask", url) require.NoError(t, err, out) diff --git a/internal/connector/ledger_admission_test.go b/internal/connector/ledger_admission_test.go index 3b8bfb9f9..52200dc4c 100644 --- a/internal/connector/ledger_admission_test.go +++ b/internal/connector/ledger_admission_test.go @@ -943,6 +943,8 @@ func TestAGoIsOnlyForTheAskAsDeclared(t *testing.T) { Projects: map[int64]admission.Project{adapterBucketID: {Class: "internal"}}, }, admission.Reads{Summaries: reads, Subscriptions: reads, Assignments: reads, Boosts: reads}) require.NoError(t, err) + // Declared a minute before the boost testEvent makes. + ledger.now = func() time.Time { return time.Date(2026, 9, 16, 9, 59, 0, 0, time.UTC) } _, err = ledger.DeclareAsk(context.Background(), commentID, admission.AskFingerprint(typ, declaredTitle, declared), "local:agent") require.NoError(t, err) diff --git a/internal/connector/ledger_asks.go b/internal/connector/ledger_asks.go index 6a7685d25..f46fdf08d 100644 --- a/internal/connector/ledger_asks.go +++ b/internal/connector/ledger_asks.go @@ -112,10 +112,19 @@ func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, contentHash, // 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 made 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. The event's own time is Basecamp's and +// the declaration's this machine's, so clock skew between them can only turn +// a boost given in the seconds after a declaration into a reply, never the +// other way round. func takeGo(ctx context.Context, tx *sql.Tx, recordingID int64, contentHash string, 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 = ?)`, - eventID, recordingID, contentHash, eventID) + `UPDATE asks SET go_event_id = ? WHERE recording_id = ? AND content_hash = ? AND (go_event_id IS NULL OR go_event_id = ?) + AND declared_at < (SELECT created_at FROM events WHERE id = ?)`, + eventID, recordingID, contentHash, eventID, eventID) if err != nil { return false, fmt.Errorf("connector: take the go on %d for %d: %w", recordingID, eventID, err) } diff --git a/internal/connector/ledger_asks_test.go b/internal/connector/ledger_asks_test.go index 4ed487080..576b0539b 100644 --- a/internal/connector/ledger_asks_test.go +++ b/internal/connector/ledger_asks_test.go @@ -23,6 +23,15 @@ 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. +// askLedger is a test ledger whose clock declares asks a minute before +// testEvent's boosts were made, as an agent declares right after posting. +func askLedger(t *testing.T) *Ledger { + t.Helper() + ledger := newTestLedger(t) + ledger.now = func() time.Time { return time.Date(2026, 9, 16, 9, 59, 0, 0, time.UTC) } + return ledger +} + 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 @@ -52,7 +61,7 @@ func goes(t *testing.T, ledger *Ledger, id int64) bool { // 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) + ledger := askLedger(t) ctx := context.Background() declared, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") require.NoError(t, err) @@ -76,7 +85,7 @@ func TestOneAskGetsOneGo(t *testing.T) { // The event that took the go keeps it when it is decided again. func TestTheGoIsTheEventsToKeep(t *testing.T) { - ledger := newTestLedger(t) + ledger := askLedger(t) ctx := context.Background() _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") require.NoError(t, err) @@ -95,7 +104,7 @@ func TestTheGoIsTheEventsToKeep(t *testing.T) { func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.T) { t.Run("a boost on a post nobody declared an ask", func(t *testing.T) { - ledger := newTestLedger(t) + ledger := askLedger(t) commitBoost(t, ledger, 1, true) assert.False(t, goes(t, ledger, 1)) // Declared after the boost was decided: not retroactive. @@ -105,7 +114,7 @@ func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.T) { }) t.Run("someone else's boost leaves the ask open for the operator's", func(t *testing.T) { - ledger := newTestLedger(t) + ledger := askLedger(t) _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") require.NoError(t, err) commitBoost(t, ledger, 1, false) @@ -115,7 +124,7 @@ func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.T) { }) t.Run("an ask names a recording and who declared it", func(t *testing.T) { - ledger := newTestLedger(t) + ledger := askLedger(t) _, err := ledger.DeclareAsk(context.Background(), 0, askHash, "local:agent") require.Error(t, err) _, err = ledger.DeclareAsk(context.Background(), askRecordingID, askHash, " ") @@ -125,7 +134,7 @@ func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.T) { // The session tells the go from a thumbs-up by the line alone. func TestTheGoIsHandedOffAsSuch(t *testing.T) { - ledger := newTestLedger(t) + ledger := askLedger(t) _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") require.NoError(t, err) commitBoost(t, ledger, 1, true) @@ -156,7 +165,7 @@ func TestTheGoIsHandedOffAsSuch(t *testing.T) { // 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) + ledger := askLedger(t) admitOn(t, ledger, 1, "recording:1") return getRecord(t, ledger, 1) }(), 999, true) @@ -168,7 +177,7 @@ func TestOnlyABoostedLineSaysGo(t *testing.T) { // 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) + ledger := askLedger(t) _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") require.NoError(t, err) commitBoost(t, ledger, 1, true) @@ -188,7 +197,7 @@ func TestAGoClosedUnwrittenIsNotGivenBack(t *testing.T) { // 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) + ledger := askLedger(t) ctx := context.Background() _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") require.NoError(t, err) @@ -221,7 +230,7 @@ func TestAnAskEditedSinceItWasDeclaredGivesNoGo(t *testing.T) { // The handed-off line says so: the edited ask's boost is a reply. func TestAnEditedAsksBoostIsHandedOffAsAReply(t *testing.T) { - ledger := newTestLedger(t) + ledger := askLedger(t) _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") require.NoError(t, err) edited := boostVerdict(1, true) @@ -237,3 +246,26 @@ func TestAnEditedAsksBoostIsHandedOffAsAReply(t *testing.T) { assert.Equal(t, "boosted", lines[0].Trigger) assert.False(t, lines[0].Go) } + +// A boost Basecamp made 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. +func TestABoostFromBeforeTheDeclarationIsNoGo(t *testing.T) { + ledger := newTestLedger(t) + ctx := context.Background() + seenRecord(t, ledger, 1) // made at 10:00, seen, not yet decided + ledger.now = func() time.Time { return time.Date(2026, 9, 16, 10, 1, 0, 0, time.UTC) } + _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") + require.NoError(t, err) + _, err = ledger.Admission().Commit(ctx, boostVerdict(1, true)) + require.NoError(t, err) + assert.False(t, goes(t, ledger, 1), "a boost made before the declaration is a reply") + + later := testEvent(2) + later.CreatedAt = time.Date(2026, 9, 16, 10, 2, 0, 0, time.UTC) + _, err = ledger.RecordSeen(ctx, later, LanePoll) + require.NoError(t, err) + _, err = ledger.Admission().Commit(ctx, boostVerdict(2, true)) + require.NoError(t, err) + assert.True(t, goes(t, ledger, 2), "the ask was left open for a boost made after it") +} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index d23b8a03f..60ff282de 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -116,7 +116,7 @@ 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. + `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 diff --git a/skills/basecamp-connect/subagent.md b/skills/basecamp-connect/subagent.md index ee3c4686b..416854088 100644 --- a/skills/basecamp-connect/subagent.md +++ b/skills/basecamp-connect/subagent.md @@ -193,8 +193,9 @@ detail, load the `basecamp` skill. basecamp connect ask '<posted app_url>' -P '<profile>' ``` - It reads the post once and fingerprints its words. A boost decided before - the declaration is only a reply, so declare at once. Never edit a post once + 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. From 3d11d3c6d5e2319ed2fec77cc68eb98ba6a0048a Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Tue, 13 Oct 2026 01:15:12 -0700 Subject: [PATCH 11/11] connect: order a go's boost against its declaration by boost id, not clock 254f27cb compared the declaration's time, from this machine's clock, with the boost event's, from Basecamp's. A local clock behind Basecamp's let a boost given before the declaration pass as one given after. connect ask now reads the boosts already on the post and records the highest id as the ask's boost_floor (migration 15, unshipped). Admission carries the boost's own id, and the go requires it to be above the floor. Basecamp's boost ids only grow, so both sides are ordered on Basecamp's clock alone, and no skew can reorder them. --- internal/commands/connect_operator.go | 13 +++- internal/commands/connect_operator_test.go | 33 ++++++++ internal/connector/admission/verdict.go | 5 ++ internal/connector/ledger_admission.go | 2 +- internal/connector/ledger_admission_test.go | 4 +- internal/connector/ledger_asks.go | 29 ++++--- internal/connector/ledger_asks_test.go | 85 +++++++++------------ 7 files changed, 105 insertions(+), 66 deletions(-) diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index 2485462b3..b800562ce 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -500,12 +500,23 @@ only way to make one.`, 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), operatorName()) + res, err := ledger.DeclareAsk(cmd.Context(), ref.RecordingID, admission.AskFingerprint(post.Type, post.Title, post.Content), floor, operatorName()) if err != nil { return err } diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 452f82b72..8f8261616 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -185,6 +185,9 @@ func (f operatorFixture) askComment(id, creator int64, content string) { "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 { @@ -203,6 +206,35 @@ func askHashIn(t *testing.T, f operatorFixture, id int64) string { 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, "<div>Shall I?</div>") + 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"` } @@ -225,6 +257,7 @@ func TestConnectAskFingerprintsTheCommentTheURLNames(t *testing.T) { 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) diff --git a/internal/connector/admission/verdict.go b/internal/connector/admission/verdict.go index 96dbba440..5766d674f 100644 --- a/internal/connector/admission/verdict.go +++ b/internal/connector/admission/verdict.go @@ -137,6 +137,10 @@ type Verdict struct { // 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. @@ -475,6 +479,7 @@ func (a *Admitter) Decide(ctx context.Context, ev Event) (out Verdict, err error 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, diff --git a/internal/connector/ledger_admission.go b/internal/connector/ledger_admission.go index 139ca4ff9..73dd1885b 100644 --- a/internal/connector/ledger_admission.go +++ b/internal/connector/ledger_admission.go @@ -194,7 +194,7 @@ func (a Admission) commit(ctx context.Context, v admission.Verdict, state Record // 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.EventID); err != nil { + if _, err := takeGo(ctx, tx, v.AskRecordingID, v.AskContentHash, v.AskBoostID, v.EventID); err != nil { return "", err } } diff --git a/internal/connector/ledger_admission_test.go b/internal/connector/ledger_admission_test.go index 52200dc4c..ffa885458 100644 --- a/internal/connector/ledger_admission_test.go +++ b/internal/connector/ledger_admission_test.go @@ -943,9 +943,7 @@ func TestAGoIsOnlyForTheAskAsDeclared(t *testing.T) { Projects: map[int64]admission.Project{adapterBucketID: {Class: "internal"}}, }, admission.Reads{Summaries: reads, Subscriptions: reads, Assignments: reads, Boosts: reads}) require.NoError(t, err) - // Declared a minute before the boost testEvent makes. - ledger.now = func() time.Time { return time.Date(2026, 9, 16, 9, 59, 0, 0, time.UTC) } - _, err = ledger.DeclareAsk(context.Background(), commentID, admission.AskFingerprint(typ, declaredTitle, declared), "local:agent") + _, err = ledger.DeclareAsk(context.Background(), commentID, admission.AskFingerprint(typ, declaredTitle, declared), 31336, "local:agent") require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/connector/ledger_asks.go b/internal/connector/ledger_asks.go index f46fdf08d..6f7c2eaf6 100644 --- a/internal/connector/ledger_asks.go +++ b/internal/connector/ledger_asks.go @@ -44,6 +44,7 @@ CREATE TABLE asks ( 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) ); ` @@ -66,12 +67,18 @@ type AskResult struct { // 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. -func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, contentHash, by string) (AskResult, error) { +// +// 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") } @@ -83,8 +90,8 @@ func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, contentHash, } defer func() { _ = tx.Rollback() }() res, err := tx.ExecContext(ctx, - `INSERT INTO asks (recording_id, declared_at, declared_by, content_hash) VALUES (?, ?, ?, ?) ON CONFLICT (recording_id) DO NOTHING`, - recordingID, l.timestamp(), by, contentHash) + `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) } @@ -113,18 +120,18 @@ func (l *Ledger) DeclareAsk(ctx context.Context, recordingID int64, contentHash, // 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 made before the ask was declared. A boost the +// 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. The event's own time is Basecamp's and -// the declaration's this machine's, so clock skew between them can only turn -// a boost given in the seconds after a declaration into a reply, never the -// other way round. -func takeGo(ctx context.Context, tx *sql.Tx, recordingID int64, contentHash string, eventID int64) (bool, error) { +// 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 declared_at < (SELECT created_at FROM events WHERE id = ?)`, - eventID, recordingID, contentHash, eventID, eventID) + 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) } diff --git a/internal/connector/ledger_asks_test.go b/internal/connector/ledger_asks_test.go index 576b0539b..8d7b54ec5 100644 --- a/internal/connector/ledger_asks_test.go +++ b/internal/connector/ledger_asks_test.go @@ -23,21 +23,12 @@ 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. -// askLedger is a test ledger whose clock declares asks a minute before -// testEvent's boosts were made, as an agent declares right after posting. -func askLedger(t *testing.T) *Ledger { - t.Helper() - ledger := newTestLedger(t) - ledger.now = func() time.Time { return time.Date(2026, 9, 16, 9, 59, 0, 0, time.UTC) } - return ledger -} - 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 = askRecordingID, askHash + v.AskRecordingID, v.AskContentHash, v.AskBoostID = askRecordingID, askHash, 1000+id v.Snapshot.Ask = &admission.AskCopy{Content: askText} } return v @@ -61,9 +52,9 @@ func goes(t *testing.T, ledger *Ledger, id int64) bool { // 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 := askLedger(t) + ledger := newTestLedger(t) ctx := context.Background() - declared, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") + declared, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, 0, "local:agent") require.NoError(t, err) assert.False(t, declared.Already) @@ -75,7 +66,7 @@ func TestOneAskGetsOneGo(t *testing.T) { 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, "local:agent") + 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") @@ -85,37 +76,37 @@ func TestOneAskGetsOneGo(t *testing.T) { // The event that took the go keeps it when it is decided again. func TestTheGoIsTheEventsToKeep(t *testing.T) { - ledger := askLedger(t) + ledger := newTestLedger(t) ctx := context.Background() - _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") + _, 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, 1) + 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, 2) + 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 := askLedger(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, "local:agent") + _, 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 := askLedger(t) - _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") + 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) @@ -124,18 +115,18 @@ func TestNoGoWithoutADeclaredAskOrTheOperator(t *testing.T) { }) t.Run("an ask names a recording and who declared it", func(t *testing.T) { - ledger := askLedger(t) - _, err := ledger.DeclareAsk(context.Background(), 0, askHash, "local:agent") + ledger := newTestLedger(t) + _, err := ledger.DeclareAsk(context.Background(), 0, askHash, 0, "local:agent") require.Error(t, err) - _, err = ledger.DeclareAsk(context.Background(), askRecordingID, askHash, " ") + _, 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 := askLedger(t) - _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") + 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) @@ -165,7 +156,7 @@ func TestTheGoIsHandedOffAsSuch(t *testing.T) { // 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 := askLedger(t) + ledger := newTestLedger(t) admitOn(t, ledger, 1, "recording:1") return getRecord(t, ledger, 1) }(), 999, true) @@ -177,8 +168,8 @@ func TestOnlyABoostedLineSaysGo(t *testing.T) { // 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 := askLedger(t) - _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") + ledger := newTestLedger(t) + _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, 0, "local:agent") require.NoError(t, err) commitBoost(t, ledger, 1, true) @@ -197,9 +188,9 @@ func TestAGoClosedUnwrittenIsNotGivenBack(t *testing.T) { // 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 := askLedger(t) + ledger := newTestLedger(t) ctx := context.Background() - _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") + _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, 0, "local:agent") require.NoError(t, err) edited := boostVerdict(1, true) @@ -215,7 +206,7 @@ func TestAnAskEditedSinceItWasDeclaredGivesNoGo(t *testing.T) { 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, "local:agent") + 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) @@ -223,15 +214,15 @@ func TestAnAskEditedSinceItWasDeclaredGivesNoGo(t *testing.T) { tx, err := ledger.db.BeginTx(ctx, nil) require.NoError(t, err) defer func() { _ = tx.Rollback() }() - took, err := takeGo(ctx, tx, askRecordingID, edited.AskContentHash, 3) + 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 := askLedger(t) - _, err := ledger.DeclareAsk(context.Background(), askRecordingID, askHash, "local:agent") + 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>") @@ -247,25 +238,19 @@ func TestAnEditedAsksBoostIsHandedOffAsAReply(t *testing.T) { assert.False(t, lines[0].Go) } -// A boost Basecamp made before the ask was declared is no go, even when the +// 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. +// 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() - seenRecord(t, ledger, 1) // made at 10:00, seen, not yet decided - ledger.now = func() time.Time { return time.Date(2026, 9, 16, 10, 1, 0, 0, time.UTC) } - _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, "local:agent") + // Boost 1001 was already on the post when it was declared. + _, err := ledger.DeclareAsk(ctx, askRecordingID, askHash, 1001, "local:agent") require.NoError(t, err) - _, err = ledger.Admission().Commit(ctx, boostVerdict(1, true)) - require.NoError(t, err) - assert.False(t, goes(t, ledger, 1), "a boost made before the declaration is a reply") + commitBoost(t, ledger, 1, true) // boost 1001 + assert.False(t, goes(t, ledger, 1), "a boost from before the declaration is a reply") - later := testEvent(2) - later.CreatedAt = time.Date(2026, 9, 16, 10, 2, 0, 0, time.UTC) - _, err = ledger.RecordSeen(ctx, later, LanePoll) - require.NoError(t, err) - _, err = ledger.Admission().Commit(ctx, boostVerdict(2, true)) - require.NoError(t, err) - assert.True(t, goes(t, ledger, 2), "the ask was left open for a boost made after it") + 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") }