From f5ec4d18d339c06474b6da5f55bd7d315604365d Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 5 Oct 2026 22:21:00 -0700 Subject: [PATCH 01/11] Let named operators assign the agent work, when the operator opts them in The local agent connector's --allow-assignments-from-authorized lets the people named with --allow trigger the agent by assignment, not only the operator. basecamp connect kept assignments the operator's alone in every mode, and there was no way to say otherwise. The local connector's caution doesn't carry over. It reads the assigner from a webhook payload it cannot corroborate, so there the opt-in rests on a secret URL path. Here the assigner is the account feed's performer, which Basecamp writes from the session that made the change; no payload carries it, and the gate trusts that id. Opting a named person in trusts that person, not text claiming to be them. Admission still reads the recording's events to confirm this assignment added the agent, and that the agent is still assigned. setup takes --allow-assignments-from-authorized (=false to undo) and records it as trust.allow_assignments. It rides with the allowlist: setting it with nobody named is refused, and a run that leaves nobody named drops it. Project members, strangers and agents gain nothing. --- internal/commands/connect.go | 43 +++++++++++- internal/commands/connect_setup_guided.go | 1 + internal/commands/connect_setup_test.go | 23 +++++++ .../connector/admission/assignments_test.go | 65 +++++++++++++++++++ internal/connector/admission/doc.go | 7 +- internal/connector/admission/gate.go | 3 + internal/connector/admission/matrix.go | 8 ++- internal/connector/admission/policy.go | 6 ++ internal/connector/setup/apply.go | 23 +++++++ internal/connector/setup/apply_test.go | 38 +++++++++++ skills/basecamp-connect/SKILL.md | 17 +++-- 11 files changed, 219 insertions(+), 15 deletions(-) create mode 100644 internal/connector/admission/assignments_test.go diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 316d963ec..42c46b157 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -186,6 +186,9 @@ func connectShowDisplay(path string, f setup.File, markdown bool) map[string]any } trust += ": operators " + strings.Join(ids, ", ") } + if f.Trust.AllowAssignments { + trust += "; they may assign" + } d := map[string]any{ "file": exact(path), "account": f.AccountID, @@ -260,6 +263,9 @@ type connectSetupFlags struct { trust string allow []string + // allowAssignments is --allow-assignments-from-authorized, unset when + // not passed so setup keeps what connect.json has. + allowAssignments optionalBool serve []string classes []string @@ -308,9 +314,13 @@ Trust. operator (default): the operator alone. allowlist: the operator and the people passed with --allow. project: any non-client member of the event's project, as a participant, beside the operator and any people passed with --allow, now or in an earlier run. The operator and the people passed with ---allow are operators; a project member is a participant. Each request the connector -prints names its role. -Assignments are the operator's alone in every mode. +--allow are operators; a project member is a participant. Each request the +connector prints names its role. Assignments are the operator's alone in every +mode, unless --allow-assignments-from-authorized opts in the people the +allowlist names (this run's --allow, or the list connect.json keeps): the +assigner is the account feed's performer, which Basecamp writes, so naming +someone trusts that person and nothing that claims to be them. +--allow-assignments-from-authorized=false takes it back. Projects. connect.json is the local list of Basecamp projects this agent serves: --serve , --unserve . Nothing in a project it @@ -374,6 +384,8 @@ Examples: fl.StringVar(&f.operatorProfile, "operator-profile", "", "Profile whose identity is the operator") fl.StringVar(&f.trust, "trust", "", "Who may drive the agent: operator, allowlist or project") fl.StringArrayVar(&f.allow, "allow", nil, "Person id to trust as an operator besides the operator (repeatable; with no --trust it implies allowlist; works with --trust allowlist or project, not operator)") + fl.Var(&f.allowAssignments, "allow-assignments-from-authorized", "Let the people --allow names assign the agent work too (default: the operator's alone; =false to turn off)") + fl.Lookup("allow-assignments-from-authorized").NoOptDefVal = "true" fl.StringArrayVar(&f.serve, "serve", nil, "Serve a Basecamp project: (repeatable)") fl.StringArrayVar(&f.unserve, "unserve", nil, "Stop serving a project (repeatable)") fl.StringArrayVar(&f.classes, "class", nil, "Classify a served project: =, or = to clear it (repeatable)") @@ -786,6 +798,10 @@ func canonicalAccount(raw string) (string, error) { // changes turns setup's flags into the changes connect.json takes. func (f *connectSetupFlags) changes() (setup.Changes, error) { var ch setup.Changes + if f.allowAssignments.set { + on := f.allowAssignments.value + ch.AllowAssignments = &on + } if f.trust != "" { ch.Trust = admission.TrustMode(f.trust) switch ch.Trust { @@ -1263,3 +1279,24 @@ func connectSDKOptions() []basecamp.ClientOption { basecamp.WithUserAgent(version.UserAgent() + " " + basecamp.DefaultUserAgent), } } + +// optionalBool is a boolean flag that knows whether it was given, so a setup +// run that leaves it out keeps what connect.json has. +type optionalBool struct { + set, value bool +} + +func (b *optionalBool) String() string { return strconv.FormatBool(b.value) } +func (b *optionalBool) Type() string { return "bool" } +func (b *optionalBool) IsBoolFlag() bool { + return true +} + +func (b *optionalBool) Set(s string) error { + v, err := strconv.ParseBool(s) + if err != nil { + return err + } + b.set, b.value = true, v + return nil +} diff --git a/internal/commands/connect_setup_guided.go b/internal/commands/connect_setup_guided.go index 6a000a5c3..344f31e4f 100644 --- a/internal/commands/connect_setup_guided.go +++ b/internal/commands/connect_setup_guided.go @@ -47,6 +47,7 @@ const connectAgentProfileName = "agent" var connectSetupPolicyFlags = []string{ "expect-identity", "operator", "operator-profile", "trust", "allow", "serve", "unserve", "class", "watch-completions", "no-watch-completions", + "allow-assignments-from-authorized", } // connectGOOS is the platform guided setup checks before connecting. A diff --git a/internal/commands/connect_setup_test.go b/internal/commands/connect_setup_test.go index bbb13ca3b..483878937 100644 --- a/internal/commands/connect_setup_test.go +++ b/internal/commands/connect_setup_test.go @@ -1045,6 +1045,29 @@ func TestConnectSetupNamesOperatorsAlongsideProjectTrust(t *testing.T) { assert.Equal(t, []int64{setupOperatorPerson + 1}, f.Trust.AllowlistIDs) } +// --allow-assignments-from-authorized opts the named people in to assigning +// the agent work; =false takes it back, and leaving it out keeps it. +func TestConnectSetupOptsNamedOperatorsInToAssignments(t *testing.T) { + s := startConnectSetupServer(t) + connectSetupApp(t, s, "agent") + storeConnectProfile(t, s, "me", setupOperatorToken) + run := func(args ...string) *setup.File { + t.Helper() + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), append([]string{"--operator-profile", "me", serveArg()}, args...)...) + require.NoError(t, err, out) + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + return &f + } + + assert.True(t, run("--allow", fmt.Sprint(setupOperatorPerson+1), "--allow-assignments-from-authorized").Trust.AllowAssignments) + assert.True(t, run().Trust.AllowAssignments, "kept when not passed") + assert.False(t, run("--allow-assignments-from-authorized=false").Trust.AllowAssignments) + + out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator-profile", "me", "--trust", "operator", "--allow-assignments-from-authorized") + require.Error(t, err, out, "an opt-in with nobody named is refused") +} + // The scope that decides readiness is the one the credential was granted, // not the one the profile's configuration names. func TestConnectSetupReadsTheGrantedScopeNotTheProfiles(t *testing.T) { diff --git a/internal/connector/admission/assignments_test.go b/internal/connector/admission/assignments_test.go new file mode 100644 index 000000000..20083baee --- /dev/null +++ b/internal/connector/admission/assignments_test.go @@ -0,0 +1,65 @@ +package admission + +import ( + "testing" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// allow_assignments lets the people the allowlist names assign the agent work, +// as the operator does. The assigner is the feed's performer, which Basecamp +// writes and no payload carries, so naming someone here trusts them, not text +// that claims to be them. Nobody it doesn't name gains anything. +func TestNamedOperatorsMayAssignWhenOptedIn(t *testing.T) { + optedIn := func() Policy { + p := basePolicy() + p.Trust = Trust{Mode: TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{allowedID}, AllowAssignments: true} + return p + } + assignment := func(performer int64) Event { + return Event{ID: eventID, EventType: "card.assignment_changed", BucketID: servedProj, RecordingID: recordingID, CreatorID: performer} + } + card := func(f *fakeReads) { + f.summaries[recordingID] = summaryWith(recordingID, servedProj, "Kanban::Card", strangerID, "
a card
") + f.summaries[recordingID].Assignees = []basecamp.Person{{ID: agentID}} + f.assignments[eventID] = []int64{agentID} + } + + t.Run("a named operator assigning the agent is admitted", func(t *testing.T) { + f := newFakeReads() + card(f) + v := decide(t, newAdmitter(t, optedIn(), f), assignment(allowedID)) + require.Equal(t, StateAdmitted, v.State, "reason %q", v.Reason) + assert.Equal(t, TriggerAssigned, v.Trigger) + assert.Equal(t, allowedID, v.RequesterID) + assert.Equal(t, 1, f.assignCalls, "and only once the events show this assignment added the agent") + }) + + t.Run("anyone else is still refused at the gate", func(t *testing.T) { + for _, performer := range []int64{strangerID, memberID} { + f := newFakeReads() + card(f) + v := decide(t, newAdmitter(t, optedIn(), f), assignment(performer)) + assert.Equal(t, StateDiscarded, v.State) + assert.Equal(t, ReasonAssignmentNotOperator, v.Reason) + assert.Zero(t, f.totalReads()) + } + }) + + t.Run("without the opt-in, a named operator's assignment is refused", func(t *testing.T) { + f := newFakeReads() + card(f) + p := optedIn() + p.Trust.AllowAssignments = false + v := decide(t, newAdmitter(t, p, f), assignment(allowedID)) + assert.Equal(t, ReasonAssignmentNotOperator, v.Reason) + }) + + t.Run("an opt-in that names nobody is refused", func(t *testing.T) { + p := basePolicy() + p.Trust.AllowAssignments = true + assert.Error(t, p.Validate()) + }) +} diff --git a/internal/connector/admission/doc.go b/internal/connector/admission/doc.go index f3be6b2a8..1ac1af6f9 100644 --- a/internal/connector/admission/doc.go +++ b/internal/connector/admission/doc.go @@ -33,7 +33,9 @@ // actor type, and an event performed on someone's behalf by any agent are all // discarded before anything else is considered, so an agent mentioning an // agent stops at zero hops. Assignments are operator-only in every trust -// mode. For the triggers that carry an instruction in content (mentioned, +// mode, unless allow_assignments opts in the people the allowlist names; the +// assigner is the feed's performer, which Basecamp writes, so naming someone +// trusts that person and not a payload that claims to be them. For the triggers that carry an instruction in content (mentioned, // subscribed) the recording's author must be trusted too, because the // performer of a *.created event is not always the person who wrote it — a // to-do moved in from another project is created by the mover. @@ -62,7 +64,8 @@ // write (a completion). Allowlisting an agent is an operator error that // Validate cannot detect; the feed's actor_types=person filter excludes // agents at source. -// 2. An assignment is admitted only when the operator performed it, the +// 2. An assignment is admitted only when the operator performed it (or a +// person the allowlist names, under allow_assignments), the // recording's events show this event added the agent, and the agent is // still assigned. Missing, unfound or partial assignment data blocks as // delta_unverified; it never admits and never discards. diff --git a/internal/connector/admission/gate.go b/internal/connector/admission/gate.go index d6d79904f..9beaaff76 100644 --- a/internal/connector/admission/gate.go +++ b/internal/connector/admission/gate.go @@ -70,6 +70,9 @@ func Gate(ev Event, p Policy, m Matrix) GateResult { needsMembership := false switch { case isOperator: + case rule.OperatorOnly && p.Trust.AllowAssignments && slices.Contains(p.Trust.AllowlistIDs, performer): + // A named operator the operator opted in to assigning. The + // performer is the feed's, written by Basecamp, never a payload's. case rule.OperatorOnly: drop(ReasonAssignmentNotOperator) continue diff --git a/internal/connector/admission/matrix.go b/internal/connector/admission/matrix.go index c009978ae..2ab174ca9 100644 --- a/internal/connector/admission/matrix.go +++ b/internal/connector/admission/matrix.go @@ -16,8 +16,10 @@ const ( type Rule struct { Trigger Trigger // OperatorOnly restricts the rule to events the operator performed, - // whatever the trust mode. Assignments run the agent against a recording - // on the assigner's say-so, so no broadened mode extends to them. + // whatever the trust mode, and to the people the allowlist names when + // Trust.AllowAssignments opts them in. Assignments run the agent against + // a recording on the assigner's say-so, so no broadened mode extends to + // them by itself. OperatorOnly bool // RequiresServed discards the rule at the gate when connect.json does not // serve the project. Only mentioned and assigned are answered in an @@ -100,7 +102,7 @@ const ( // ReasonUntrustedPerformer: the performer is not in the trust set. ReasonUntrustedPerformer Reason = "untrusted_performer" // ReasonAssignmentNotOperator: an assignment performed by anyone but the - // operator. + // operator, or a named operator allow_assignments opts in. ReasonAssignmentNotOperator Reason = "assignment_not_operator" // ReasonUntrustedAuthor: the recording carrying the instruction was // written by someone outside the trust set. diff --git a/internal/connector/admission/policy.go b/internal/connector/admission/policy.go index c9cc9ef74..bcded5aed 100644 --- a/internal/connector/admission/policy.go +++ b/internal/connector/admission/policy.go @@ -46,6 +46,9 @@ type Trust struct { // AllowlistIDs are the Person ids trusted as operators besides the // operator, in allowlist or project mode. AllowlistIDs []int64 `json:"allowlist_ids,omitempty"` + // AllowAssignments lets the people AllowlistIDs names assign the agent + // work, as the operator does. Off, assignments are the operator's alone. + AllowAssignments bool `json:"allow_assignments,omitempty"` } // roleOf is the role a trusted person holds, before any membership read: an @@ -306,6 +309,9 @@ func (p Policy) Validate() error { return errors.New("admission: policy needs the operator's Person id") case p.Trust.OperatorID == p.AgentID: return errors.New("admission: the operator cannot be the agent itself") + case p.Trust.AllowAssignments && len(p.Trust.AllowlistIDs) == 0: + // An opt-in nobody can use reads as a widening that is not there. + return errors.New("admission: allow_assignments is set but the allowlist names nobody") } switch p.Trust.Mode { case TrustOperator: diff --git a/internal/connector/setup/apply.go b/internal/connector/setup/apply.go index 5a6e6d203..38977e59b 100644 --- a/internal/connector/setup/apply.go +++ b/internal/connector/setup/apply.go @@ -19,6 +19,9 @@ type Changes struct { // same run asks for project mode, which admits the project's members as // participants beside them. Allow []int64 + // AllowAssignments turns the allowlist's assignment opt-in on or off; + // nil keeps the file's. + AllowAssignments *bool // Serve are the project (bucket) ids to serve. A project already served // keeps its class and watch_completions. @@ -46,6 +49,9 @@ func Apply(f File, ch Changes) (File, error) { if err := applyTrust(&out.Trust, ch); err != nil { return File{}, err } + if err := applyAssignmentOptIn(&out.Trust, ch); err != nil { + return File{}, err + } for _, id := range ch.Remove { if slices.Contains(ch.Serve, id) { @@ -136,3 +142,20 @@ func sortedIDs(ids []int64) []int64 { slices.Sort(out) return slices.Compact(out) } + +// applyAssignmentOptIn sets the opt-in that lets the allowlist's people +// assign the agent work. It rides with that list: setting it needs someone +// named, and a run that leaves nobody named takes it away, so the file never +// carries a widening nobody can use. +func applyAssignmentOptIn(t *admission.Trust, ch Changes) error { + if ch.AllowAssignments != nil { + t.AllowAssignments = *ch.AllowAssignments + } + if len(t.AllowlistIDs) == 0 { + if ch.AllowAssignments != nil && *ch.AllowAssignments { + return errors.New("--allow-assignments-from-authorized opts in the people --allow names, and nobody is named") + } + t.AllowAssignments = false + } + return nil +} diff --git a/internal/connector/setup/apply_test.go b/internal/connector/setup/apply_test.go index be53561bf..04f9bda86 100644 --- a/internal/connector/setup/apply_test.go +++ b/internal/connector/setup/apply_test.go @@ -146,3 +146,41 @@ func TestApplyServedProjects(t *testing.T) { assert.Error(t, err, "serving and removing one project in one run") }) } + +// The assignment opt-in rides with the allowlist it opts in: setting it needs +// someone named, and a run that empties the list takes it away too. +func TestApplyAssignmentOptIn(t *testing.T) { + on, off := true, false + base := validFile(t) + + t.Run("set with the people it opts in", func(t *testing.T) { + out, err := Apply(base, Changes{Allow: []int64{9}, AllowAssignments: &on}) + require.NoError(t, err) + assert.True(t, out.Trust.AllowAssignments) + _, err = out.Policy(agentID) + assert.NoError(t, err, "admission accepts the result") + }) + t.Run("refused with nobody named", func(t *testing.T) { + _, err := Apply(base, Changes{AllowAssignments: &on}) + assert.Error(t, err) + }) + optedIn := base + optedIn.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}, AllowAssignments: true} + t.Run("kept when not passed", func(t *testing.T) { + out, err := Apply(optedIn, Changes{Allow: []int64{8}}) + require.NoError(t, err) + assert.True(t, out.Trust.AllowAssignments) + }) + t.Run("turned off", func(t *testing.T) { + out, err := Apply(optedIn, Changes{AllowAssignments: &off}) + require.NoError(t, err) + assert.False(t, out.Trust.AllowAssignments) + }) + t.Run("dropped with the list", func(t *testing.T) { + out, err := Apply(optedIn, Changes{Trust: admission.TrustOperator}) + require.NoError(t, err) + assert.False(t, out.Trust.AllowAssignments) + _, err = out.Policy(agentID) + assert.NoError(t, err) + }) +} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index e24bf2a63..081c854ed 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -75,6 +75,7 @@ Each becomes a flag on setup (*First-time setup*, step 4): | "me and Jane" | `--allow ` (and everyone else who stays: the list is replaced) | | "anyone in the project can ask it things" | `--trust project` (they ask as participants) | | "Jane operates it, and anyone in the project can ask" | `--trust project --allow ` | +| "Jane can assign it work too" | `--allow-assignments-from-authorized` (with Jane in `--allow`) | | "also work in project X" | `--serve ` | | "stop working in X" | `--unserve ` | @@ -193,12 +194,13 @@ The operator and the people named with `--allow` are **operators**. Project members admitted only by `project` are **participants**: they reach the agent by mention, by commenting on a thread it follows, and by completing something it has a stake in (context, not a request), never by assignment. Assignments -count only from the operator, in every mode. For a -personal agent pass no operator flag: setup takes its owner. For any other -agent, name the operator by their own CLI profile with `--operator-profile -''`. For allowlist, or operators beside project trust, look up each -person's id with `basecamp people list --json`, -and pass `--allow ` for each. +count only from the operator, in every mode, unless they ask for the people +named with `--allow` to assign work too: `--allow-assignments-from-authorized` +(and `=false` to undo it). For a personal agent pass no operator flag: setup +takes its owner. For any other agent, name the operator by their own CLI +profile with `--operator-profile ''`. For allowlist, or operators +beside project trust, look up each person's id with +`basecamp people list --json`, and pass `--allow ` for each. A run that passes `--allow` replaces the list, a run that passes none keeps it, and `--trust operator` clears it; setup refuses `--trust operator` with `--allow`. @@ -309,7 +311,8 @@ thing to check. - **`trigger`**: why it reached you. - `mentioned`: someone @mentioned the agent. - - `assigned`: the operator assigned it a card, to-do or step. + - `assigned`: the operator, or someone they opted in, assigned it a card, + to-do or step. - `subscribed`: a new comment on a thread the agent follows, with no mention. - `completed`: something completed in a project it watches. - **`acknowledge`**: true when a person asked for something. False for From 06de5f29c0b11d89b30e362b80b0e767bda5bf62 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 5 Oct 2026 22:41:16 -0700 Subject: [PATCH 02/11] Name the assignment opt-in where setup's command is built, and say whom it covers --- internal/connector/admission/matrix.go | 5 +++-- skills/basecamp-connect/SKILL.md | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/connector/admission/matrix.go b/internal/connector/admission/matrix.go index 2ab174ca9..6270fb9ff 100644 --- a/internal/connector/admission/matrix.go +++ b/internal/connector/admission/matrix.go @@ -101,8 +101,9 @@ const ( ReasonOutOfScope Reason = "out_of_scope" // ReasonUntrustedPerformer: the performer is not in the trust set. ReasonUntrustedPerformer Reason = "untrusted_performer" - // ReasonAssignmentNotOperator: an assignment performed by anyone but the - // operator, or a named operator allow_assignments opts in. + // ReasonAssignmentNotOperator: an assignment performed by anyone other + // than the operator or a person the allowlist names whom + // allow_assignments opts in. ReasonAssignmentNotOperator Reason = "assignment_not_operator" // ReasonUntrustedAuthor: the recording carrying the instruction was // written by someone outside the trust set. diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 081c854ed..0ab085229 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -218,8 +218,9 @@ each project by name. Then: basecamp connect setup -P '' --serve --serve --json ``` -adding `--trust`, `--allow` or `--operator-profile` as chosen, and on the -bot-user path `--expect-identity `. +adding `--trust`, `--allow`, `--allow-assignments-from-authorized` or +`--operator-profile` as chosen, and on the bot-user path +`--expect-identity `. ### Reading setup's result From d10b9eed7c133567f941f81951cbf97183fb60ee Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 12:00:29 -0700 Subject: [PATCH 03/11] Carry the assignment opt-in over to operators named beside project trust Rebased onto #843: the gate settles a performer's role once, so the opt-in reads that role instead of searching the allowlist again, and a test holds that it covers operators named beside project trust while the project's members still cannot assign. --- .../connector/admission/assignments_test.go | 17 +++++++++++++++++ internal/connector/admission/gate.go | 2 +- skills/basecamp-connect/SKILL.md | 3 ++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/internal/connector/admission/assignments_test.go b/internal/connector/admission/assignments_test.go index 20083baee..29607bf53 100644 --- a/internal/connector/admission/assignments_test.go +++ b/internal/connector/admission/assignments_test.go @@ -37,6 +37,23 @@ func TestNamedOperatorsMayAssignWhenOptedIn(t *testing.T) { assert.Equal(t, 1, f.assignCalls, "and only once the events show this assignment added the agent") }) + t.Run("so is one named beside project trust, and the members still are not", func(t *testing.T) { + p := optedIn() + p.Trust.Mode = TrustProject + f := newFakeReads() + card(f) + f.members[servedProj] = map[int64]bool{memberID: true} + v := decide(t, newAdmitter(t, p, f), assignment(allowedID)) + require.Equal(t, StateAdmitted, v.State, "reason %q", v.Reason) + assert.Equal(t, RoleOperator, v.Role) + + f = newFakeReads() + card(f) + f.members[servedProj] = map[int64]bool{memberID: true} + v = decide(t, newAdmitter(t, p, f), assignment(memberID)) + assert.Equal(t, ReasonAssignmentNotOperator, v.Reason) + }) + t.Run("anyone else is still refused at the gate", func(t *testing.T) { for _, performer := range []int64{strangerID, memberID} { f := newFakeReads() diff --git a/internal/connector/admission/gate.go b/internal/connector/admission/gate.go index 9beaaff76..ea6a86293 100644 --- a/internal/connector/admission/gate.go +++ b/internal/connector/admission/gate.go @@ -70,7 +70,7 @@ func Gate(ev Event, p Policy, m Matrix) GateResult { needsMembership := false switch { case isOperator: - case rule.OperatorOnly && p.Trust.AllowAssignments && slices.Contains(p.Trust.AllowlistIDs, performer): + case rule.OperatorOnly && p.Trust.AllowAssignments && role == RoleOperator: // A named operator the operator opted in to assigning. The // performer is the feed's, written by Basecamp, never a payload's. case rule.OperatorOnly: diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 0ab085229..73a84acf2 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -200,7 +200,8 @@ named with `--allow` to assign work too: `--allow-assignments-from-authorized` takes its owner. For any other agent, name the operator by their own CLI profile with `--operator-profile ''`. For allowlist, or operators beside project trust, look up each person's id with -`basecamp people list --json`, and pass `--allow ` for each. +`basecamp people list --json`, +and pass `--allow ` for each. A run that passes `--allow` replaces the list, a run that passes none keeps it, and `--trust operator` clears it; setup refuses `--trust operator` with `--allow`. From e1a6210dce416c6feaa2bf0bbddc321f572eb36e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 13:08:34 -0700 Subject: [PATCH 04/11] Hold the opt-in to someone besides the operator, and say it covers every named operator An allowlist naming only the operator gives the opt-in nobody to cover, and setup's refusal of an opt-in now checks connect.json is unchanged. The skill presents the opt-in as what it is, every named operator at once, so asking for one person's assignments confirms who else gains them. --- internal/commands/connect_setup_test.go | 4 ++++ internal/connector/admission/assignments_test.go | 6 ++++++ internal/connector/admission/policy.go | 7 ++++--- skills/basecamp-connect/SKILL.md | 2 +- 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/commands/connect_setup_test.go b/internal/commands/connect_setup_test.go index 483878937..d5acb3dc8 100644 --- a/internal/commands/connect_setup_test.go +++ b/internal/commands/connect_setup_test.go @@ -1066,6 +1066,10 @@ func TestConnectSetupOptsNamedOperatorsInToAssignments(t *testing.T) { out, err := runConnectSetupCmd(t, newConnectSetupApp(t, s, "agent"), "--operator-profile", "me", "--trust", "operator", "--allow-assignments-from-authorized") require.Error(t, err, out, "an opt-in with nobody named is refused") + f, err := setup.Load(connectSetupPath(t, "agent")) + require.NoError(t, err) + assert.False(t, f.Trust.AllowAssignments, "and nothing was written") + assert.Equal(t, []int64{setupOperatorPerson + 1}, f.Trust.AllowlistIDs) } // The scope that decides readiness is the one the credential was granted, diff --git a/internal/connector/admission/assignments_test.go b/internal/connector/admission/assignments_test.go index 29607bf53..c90581901 100644 --- a/internal/connector/admission/assignments_test.go +++ b/internal/connector/admission/assignments_test.go @@ -79,4 +79,10 @@ func TestNamedOperatorsMayAssignWhenOptedIn(t *testing.T) { p.Trust.AllowAssignments = true assert.Error(t, p.Validate()) }) + + t.Run("and so is one that names only the operator, who needs none", func(t *testing.T) { + p := optedIn() + p.Trust.AllowlistIDs = []int64{operatorID} + assert.Error(t, p.Validate()) + }) } diff --git a/internal/connector/admission/policy.go b/internal/connector/admission/policy.go index bcded5aed..58f4640be 100644 --- a/internal/connector/admission/policy.go +++ b/internal/connector/admission/policy.go @@ -309,9 +309,10 @@ func (p Policy) Validate() error { return errors.New("admission: policy needs the operator's Person id") case p.Trust.OperatorID == p.AgentID: return errors.New("admission: the operator cannot be the agent itself") - case p.Trust.AllowAssignments && len(p.Trust.AllowlistIDs) == 0: - // An opt-in nobody can use reads as a widening that is not there. - return errors.New("admission: allow_assignments is set but the allowlist names nobody") + case p.Trust.AllowAssignments && !slices.ContainsFunc(p.Trust.AllowlistIDs, func(id int64) bool { return id != p.Trust.OperatorID }): + // An opt-in nobody can use reads as a widening that is not there: + // the operator assigns without one. + return errors.New("admission: allow_assignments is set but the allowlist names nobody besides the operator") } switch p.Trust.Mode { case TrustOperator: diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 73a84acf2..70b3d7a56 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -75,7 +75,7 @@ Each becomes a flag on setup (*First-time setup*, step 4): | "me and Jane" | `--allow ` (and everyone else who stays: the list is replaced) | | "anyone in the project can ask it things" | `--trust project` (they ask as participants) | | "Jane operates it, and anyone in the project can ask" | `--trust project --allow ` | -| "Jane can assign it work too" | `--allow-assignments-from-authorized` (with Jane in `--allow`) | +| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers everyone `--allow` names, not one of them, so say who that is and confirm | | "also work in project X" | `--serve ` | | "stop working in X" | `--unserve ` | From ea8bdc5db45327cb2e6b9f2aebddca004865ade3 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 13:25:27 -0700 Subject: [PATCH 05/11] Turn the opt-in off when a run names a new list without restating it The opt-in was given for the people named then. A later run that names others now leaves their assignments off unless it passes the opt-in again, so nobody newly named gains assignments unasked. The skill says the opt-in covers the whole allowlist setup leaves, and tests cover it set over a kept list and refused under project trust with nobody named. --- internal/connector/setup/apply.go | 11 ++++++++--- internal/connector/setup/apply_test.go | 25 ++++++++++++++++++++++++- skills/basecamp-connect/SKILL.md | 4 ++-- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/internal/connector/setup/apply.go b/internal/connector/setup/apply.go index 38977e59b..528f0b8de 100644 --- a/internal/connector/setup/apply.go +++ b/internal/connector/setup/apply.go @@ -145,11 +145,16 @@ func sortedIDs(ids []int64) []int64 { // applyAssignmentOptIn sets the opt-in that lets the allowlist's people // assign the agent work. It rides with that list: setting it needs someone -// named, and a run that leaves nobody named takes it away, so the file never -// carries a widening nobody can use. +// named, a run that leaves nobody named takes it away, so the file never +// carries a widening nobody can use, and a run that names a new list without +// restating it takes it away too, so nobody newly named gains assignments +// unasked. func applyAssignmentOptIn(t *admission.Trust, ch Changes) error { - if ch.AllowAssignments != nil { + switch { + case ch.AllowAssignments != nil: t.AllowAssignments = *ch.AllowAssignments + case len(ch.Allow) > 0: + t.AllowAssignments = false } if len(t.AllowlistIDs) == 0 { if ch.AllowAssignments != nil && *ch.AllowAssignments { diff --git a/internal/connector/setup/apply_test.go b/internal/connector/setup/apply_test.go index 04f9bda86..7b82d22bb 100644 --- a/internal/connector/setup/apply_test.go +++ b/internal/connector/setup/apply_test.go @@ -166,10 +166,33 @@ func TestApplyAssignmentOptIn(t *testing.T) { }) optedIn := base optedIn.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}, AllowAssignments: true} - t.Run("kept when not passed", func(t *testing.T) { + t.Run("kept when neither it nor the list is passed", func(t *testing.T) { + out, err := Apply(optedIn, Changes{Serve: []int64{777}}) + require.NoError(t, err) + assert.True(t, out.Trust.AllowAssignments) + }) + // The opt-in was given for the people named then. A run that names + // others does not hand them assignments unless it says so again. + t.Run("reset when the list is replaced without it", func(t *testing.T) { out, err := Apply(optedIn, Changes{Allow: []int64{8}}) require.NoError(t, err) + assert.False(t, out.Trust.AllowAssignments) + + out, err = Apply(optedIn, Changes{Allow: []int64{8}, AllowAssignments: &on}) + require.NoError(t, err) + assert.True(t, out.Trust.AllowAssignments, "restated with the new list") + }) + t.Run("set over the list connect.json keeps", func(t *testing.T) { + f := base + f.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}} + out, err := Apply(f, Changes{AllowAssignments: &on}) + require.NoError(t, err) assert.True(t, out.Trust.AllowAssignments) + assert.Equal(t, []int64{7}, out.Trust.AllowlistIDs) + }) + t.Run("refused under project trust with nobody named", func(t *testing.T) { + _, err := Apply(base, Changes{Trust: admission.TrustProject, AllowAssignments: &on}) + assert.Error(t, err, "the members are participants, and the opt-in never covers them") }) t.Run("turned off", func(t *testing.T) { out, err := Apply(optedIn, Changes{AllowAssignments: &off}) diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 70b3d7a56..17913eea5 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -75,7 +75,7 @@ Each becomes a flag on setup (*First-time setup*, step 4): | "me and Jane" | `--allow ` (and everyone else who stays: the list is replaced) | | "anyone in the project can ask it things" | `--trust project` (they ask as participants) | | "Jane operates it, and anyone in the project can ask" | `--trust project --allow ` | -| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers everyone `--allow` names, not one of them, so say who that is and confirm | +| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers the whole allowlist setup leaves (this run's `--allow`, or the list kept), not one person, so say who that is and confirm. A run that names a new list turns it off unless it's passed again | | "also work in project X" | `--serve ` | | "stop working in X" | `--unserve ` | @@ -196,7 +196,7 @@ by mention, by commenting on a thread it follows, and by completing something it has a stake in (context, not a request), never by assignment. Assignments count only from the operator, in every mode, unless they ask for the people named with `--allow` to assign work too: `--allow-assignments-from-authorized` -(and `=false` to undo it). For a personal agent pass no operator flag: setup +(and `=false` to undo it), which covers everyone on the allowlist. For a personal agent pass no operator flag: setup takes its owner. For any other agent, name the operator by their own CLI profile with `--operator-profile ''`. For allowlist, or operators beside project trust, look up each person's id with From 738deceb87cac27f4b59bed8f383f0146243f5ca Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 14:18:47 -0700 Subject: [PATCH 06/11] Keep the opt-in when a run's list names nobody new Only someone newly named could gain assignments unasked, so the same list again, or a shorter one, keeps the opt-in; a list adding anyone still turns it off unless it is restated. --- internal/connector/setup/apply.go | 10 +++++----- internal/connector/setup/apply_test.go | 11 +++++++++++ skills/basecamp-connect/SKILL.md | 2 +- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/internal/connector/setup/apply.go b/internal/connector/setup/apply.go index 528f0b8de..82bbc7d77 100644 --- a/internal/connector/setup/apply.go +++ b/internal/connector/setup/apply.go @@ -49,7 +49,7 @@ func Apply(f File, ch Changes) (File, error) { if err := applyTrust(&out.Trust, ch); err != nil { return File{}, err } - if err := applyAssignmentOptIn(&out.Trust, ch); err != nil { + if err := applyAssignmentOptIn(&out.Trust, f.Trust.AllowlistIDs, ch); err != nil { return File{}, err } @@ -147,13 +147,13 @@ func sortedIDs(ids []int64) []int64 { // assign the agent work. It rides with that list: setting it needs someone // named, a run that leaves nobody named takes it away, so the file never // carries a widening nobody can use, and a run that names a new list without -// restating it takes it away too, so nobody newly named gains assignments -// unasked. -func applyAssignmentOptIn(t *admission.Trust, ch Changes) error { +// restating it takes it away too when it names someone the list before did +// not, so nobody newly named gains assignments unasked. +func applyAssignmentOptIn(t *admission.Trust, before []int64, ch Changes) error { switch { case ch.AllowAssignments != nil: t.AllowAssignments = *ch.AllowAssignments - case len(ch.Allow) > 0: + case slices.ContainsFunc(ch.Allow, func(id int64) bool { return !slices.Contains(before, id) }): t.AllowAssignments = false } if len(t.AllowlistIDs) == 0 { diff --git a/internal/connector/setup/apply_test.go b/internal/connector/setup/apply_test.go index 7b82d22bb..07d1172be 100644 --- a/internal/connector/setup/apply_test.go +++ b/internal/connector/setup/apply_test.go @@ -182,6 +182,17 @@ func TestApplyAssignmentOptIn(t *testing.T) { require.NoError(t, err) assert.True(t, out.Trust.AllowAssignments, "restated with the new list") }) + // Only someone newly named would gain assignments unasked: the same + // list again, or a shorter one, keeps the opt-in. + t.Run("kept when the list names nobody new", func(t *testing.T) { + f := base + f.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7, 9}, AllowAssignments: true} + for _, allow := range [][]int64{{9, 7}, {7}} { + out, err := Apply(f, Changes{Allow: allow}) + require.NoError(t, err) + assert.True(t, out.Trust.AllowAssignments, "allow %v names nobody new", allow) + } + }) t.Run("set over the list connect.json keeps", func(t *testing.T) { f := base f.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 17913eea5..aa1a9218e 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -75,7 +75,7 @@ Each becomes a flag on setup (*First-time setup*, step 4): | "me and Jane" | `--allow ` (and everyone else who stays: the list is replaced) | | "anyone in the project can ask it things" | `--trust project` (they ask as participants) | | "Jane operates it, and anyone in the project can ask" | `--trust project --allow ` | -| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers the whole allowlist setup leaves (this run's `--allow`, or the list kept), not one person, so say who that is and confirm. A run that names a new list turns it off unless it's passed again | +| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers the whole allowlist setup leaves (this run's `--allow`, or the list kept), not one person, so say who that is and confirm. A run whose `--allow` names someone new turns it off unless it's passed again | | "also work in project X" | `--serve ` | | "stop working in X" | `--unserve ` | From 456496a2a68d987b96df36937766554b66f66a77 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 14:32:19 -0700 Subject: [PATCH 07/11] Say when an assignment reaches the agent, and that the opt-in needs someone named --- skills/basecamp-connect/SKILL.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index aa1a9218e..1839effeb 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -75,7 +75,7 @@ Each becomes a flag on setup (*First-time setup*, step 4): | "me and Jane" | `--allow ` (and everyone else who stays: the list is replaced) | | "anyone in the project can ask it things" | `--trust project` (they ask as participants) | | "Jane operates it, and anyone in the project can ask" | `--trust project --allow ` | -| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers the whole allowlist setup leaves (this run's `--allow`, or the list kept), not one person, so say who that is and confirm. A run whose `--allow` names someone new turns it off unless it's passed again | +| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers the whole allowlist setup leaves (this run's `--allow`, or the list kept), not one person, so say who that is and confirm. With nobody on the allowlist setup refuses it, so name someone first. A run whose `--allow` names someone new turns it off unless it's passed again | | "also work in project X" | `--serve ` | | "stop working in X" | `--unserve ` | @@ -314,7 +314,8 @@ thing to check. - **`trigger`**: why it reached you. - `mentioned`: someone @mentioned the agent. - `assigned`: the operator, or someone they opted in, assigned it a card, - to-do or step. + to-do or step. Only when the recording's events show this assignment + added the agent, and the agent is still assigned. - `subscribed`: a new comment on a thread the agent follows, with no mention. - `completed`: something completed in a project it watches. - **`acknowledge`**: true when a person asked for something. False for From dae262ea081295a5be068ed73a3276e40c7c2371 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 14:41:31 -0700 Subject: [PATCH 08/11] Drop the opt-in when the new operator is the only person it covered Making the allowlist's one person the operator left the opt-in nobody to cover, and connect.json then refused to validate over a change setup itself made. Setting the operator now drops an opt-in with nobody left. --- internal/commands/connect.go | 2 +- internal/connector/setup/apply.go | 11 +++++++++++ internal/connector/setup/apply_test.go | 13 +++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 42c46b157..2b795cc78 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -602,7 +602,7 @@ func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) if kind == setup.KindBotUser { next.Agent.IdentityID = expect } - next.Trust.OperatorID = trust.Operator.ID + setup.SetOperator(&next.Trust, trust.Operator.ID) if err := next.Validate(); err != nil { return output.ErrUsage("connect.json was not written: " + err.Error()) } diff --git a/internal/connector/setup/apply.go b/internal/connector/setup/apply.go index 82bbc7d77..3c85f32cb 100644 --- a/internal/connector/setup/apply.go +++ b/internal/connector/setup/apply.go @@ -164,3 +164,14 @@ func applyAssignmentOptIn(t *admission.Trust, before []int64, ch Changes) error } return nil } + +// SetOperator records the operator. When the allowlist then names nobody +// besides them, the assignment opt-in has nobody left to cover and is +// dropped: the operator assigns without one, and keeping it would make the +// file refuse to validate over a change setup itself made. +func SetOperator(t *admission.Trust, id int64) { + t.OperatorID = id + if !slices.ContainsFunc(t.AllowlistIDs, func(a int64) bool { return a != id }) { + t.AllowAssignments = false + } +} diff --git a/internal/connector/setup/apply_test.go b/internal/connector/setup/apply_test.go index 07d1172be..3a6183026 100644 --- a/internal/connector/setup/apply_test.go +++ b/internal/connector/setup/apply_test.go @@ -218,3 +218,16 @@ func TestApplyAssignmentOptIn(t *testing.T) { assert.NoError(t, err) }) } + +// Making the one person the allowlist names the operator leaves the opt-in +// nobody to cover. Setup drops it rather than refusing the change. +func TestSetOperatorDropsAnOptInWithNobodyLeftToCover(t *testing.T) { + tr := admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}, AllowAssignments: true} + SetOperator(&tr, 7) + assert.Equal(t, int64(7), tr.OperatorID) + assert.False(t, tr.AllowAssignments) + + tr = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7, 9}, AllowAssignments: true} + SetOperator(&tr, 7) + assert.True(t, tr.AllowAssignments, "9 is still covered") +} From 106a2b8873953734f355f7d8d7b1d45f22fb7f26 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 14:56:30 -0700 Subject: [PATCH 09/11] Refuse an opt-in asked for in a run that leaves it nobody, and drop only a kept one Settle when the opt-in has nobody to cover with one rule, decided once the run's operator is known: asked for in this run, it is refused so the person learns it does nothing; kept from an earlier run, it is dropped. The refusal names the allowlist rather than --allow, since the list can come from connect.json. --- internal/commands/connect.go | 4 +++- internal/connector/setup/apply.go | 21 ++++++++++++++------- internal/connector/setup/apply_test.go | 8 ++++++-- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 2b795cc78..c34a847e2 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -602,7 +602,9 @@ func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) if kind == setup.KindBotUser { next.Agent.IdentityID = expect } - setup.SetOperator(&next.Trust, trust.Operator.ID) + if err := setup.SetOperator(&next.Trust, trust.Operator.ID, f.allowAssignments.set && f.allowAssignments.value); err != nil { + return output.ErrUsage("connect.json was not written: " + err.Error()) + } if err := next.Validate(); err != nil { return output.ErrUsage("connect.json was not written: " + err.Error()) } diff --git a/internal/connector/setup/apply.go b/internal/connector/setup/apply.go index 3c85f32cb..885694b7b 100644 --- a/internal/connector/setup/apply.go +++ b/internal/connector/setup/apply.go @@ -158,7 +158,7 @@ func applyAssignmentOptIn(t *admission.Trust, before []int64, ch Changes) error } if len(t.AllowlistIDs) == 0 { if ch.AllowAssignments != nil && *ch.AllowAssignments { - return errors.New("--allow-assignments-from-authorized opts in the people --allow names, and nobody is named") + return errors.New("--allow-assignments-from-authorized opts in the people the allowlist names, and after this run it names nobody") } t.AllowAssignments = false } @@ -166,12 +166,19 @@ func applyAssignmentOptIn(t *admission.Trust, before []int64, ch Changes) error } // SetOperator records the operator. When the allowlist then names nobody -// besides them, the assignment opt-in has nobody left to cover and is -// dropped: the operator assigns without one, and keeping it would make the -// file refuse to validate over a change setup itself made. -func SetOperator(t *admission.Trust, id int64) { +// besides them, the assignment opt-in has nobody left to cover. Kept from an +// earlier run, it is dropped: the operator assigns without one, and keeping +// it would make the file refuse to validate over a change setup itself made. +// Asked for in this run (requested), it is refused, so the person learns it +// does nothing rather than finding it quietly off. +func SetOperator(t *admission.Trust, id int64, requested bool) error { t.OperatorID = id - if !slices.ContainsFunc(t.AllowlistIDs, func(a int64) bool { return a != id }) { - t.AllowAssignments = false + if !t.AllowAssignments || slices.ContainsFunc(t.AllowlistIDs, func(a int64) bool { return a != id }) { + return nil } + if requested { + return errors.New("--allow-assignments-from-authorized covers the people the allowlist names besides the operator, and it names nobody else") + } + t.AllowAssignments = false + return nil } diff --git a/internal/connector/setup/apply_test.go b/internal/connector/setup/apply_test.go index 3a6183026..b02f775e4 100644 --- a/internal/connector/setup/apply_test.go +++ b/internal/connector/setup/apply_test.go @@ -223,11 +223,15 @@ func TestApplyAssignmentOptIn(t *testing.T) { // nobody to cover. Setup drops it rather than refusing the change. func TestSetOperatorDropsAnOptInWithNobodyLeftToCover(t *testing.T) { tr := admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}, AllowAssignments: true} - SetOperator(&tr, 7) + require.NoError(t, SetOperator(&tr, 7, false)) assert.Equal(t, int64(7), tr.OperatorID) assert.False(t, tr.AllowAssignments) tr = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7, 9}, AllowAssignments: true} - SetOperator(&tr, 7) + require.NoError(t, SetOperator(&tr, 7, false)) assert.True(t, tr.AllowAssignments, "9 is still covered") + + // Asked for in this very run, it is refused rather than quietly dropped. + tr = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7}, AllowAssignments: true} + assert.Error(t, SetOperator(&tr, 7, true)) } From 57c36eb246cb6b4a244cd5115c0dc9305e91a2fd Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 15:11:19 -0700 Subject: [PATCH 10/11] Let the opt-in ride with --allow, with no history to compare Four rounds of review each found a case where deciding whether a new list named someone new went wrong: the same list again, a former operator retained, an operator promoted. Drop the comparison. A run that passes --allow restates the opt-in with the list, off unless it passes the flag too; a run that passes neither keeps both. The help, the field contract and the skill say so. --- internal/commands/connect.go | 7 ++++--- internal/connector/setup/apply.go | 21 +++++++++++---------- internal/connector/setup/apply_test.go | 13 ++++++++----- skills/basecamp-connect/SKILL.md | 2 +- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/internal/commands/connect.go b/internal/commands/connect.go index c34a847e2..8192041cd 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -317,9 +317,10 @@ project, as a participant, beside the operator and any people passed with --allow are operators; a project member is a participant. Each request the connector prints names its role. Assignments are the operator's alone in every mode, unless --allow-assignments-from-authorized opts in the people the -allowlist names (this run's --allow, or the list connect.json keeps): the -assigner is the account feed's performer, which Basecamp writes, so naming -someone trusts that person and nothing that claims to be them. +allowlist names: the assigner is the account feed's performer, which +Basecamp writes, so naming someone trusts that person and nothing that claims +to be them. It rides with --allow: a run that passes --allow turns it off +unless it passes the flag again, and a run that passes neither keeps both. --allow-assignments-from-authorized=false takes it back. Projects. connect.json is the local list of Basecamp projects this agent diff --git a/internal/connector/setup/apply.go b/internal/connector/setup/apply.go index 885694b7b..8808ff7ec 100644 --- a/internal/connector/setup/apply.go +++ b/internal/connector/setup/apply.go @@ -19,8 +19,10 @@ type Changes struct { // same run asks for project mode, which admits the project's members as // participants beside them. Allow []int64 - // AllowAssignments turns the allowlist's assignment opt-in on or off; - // nil keeps the file's. + // AllowAssignments turns the allowlist's assignment opt-in on or off. + // Nil keeps the file's, except in a run that passes Allow, which + // restates the opt-in with the list it covers: off unless set here. + // SetOperator drops a kept one that is left covering nobody. AllowAssignments *bool // Serve are the project (bucket) ids to serve. A project already served @@ -49,7 +51,7 @@ func Apply(f File, ch Changes) (File, error) { if err := applyTrust(&out.Trust, ch); err != nil { return File{}, err } - if err := applyAssignmentOptIn(&out.Trust, f.Trust.AllowlistIDs, ch); err != nil { + if err := applyAssignmentOptIn(&out.Trust, ch); err != nil { return File{}, err } @@ -144,16 +146,15 @@ func sortedIDs(ids []int64) []int64 { } // applyAssignmentOptIn sets the opt-in that lets the allowlist's people -// assign the agent work. It rides with that list: setting it needs someone -// named, a run that leaves nobody named takes it away, so the file never -// carries a widening nobody can use, and a run that names a new list without -// restating it takes it away too when it names someone the list before did -// not, so nobody newly named gains assignments unasked. -func applyAssignmentOptIn(t *admission.Trust, before []int64, ch Changes) error { +// assign the agent work. It rides with the --allow that names them: a run +// that passes --allow restates it, off unless this run sets it, so whoever a +// new list names gains assignments only when asked, with no comparison to +// what the file held before. Setting it needs someone named. +func applyAssignmentOptIn(t *admission.Trust, ch Changes) error { switch { case ch.AllowAssignments != nil: t.AllowAssignments = *ch.AllowAssignments - case slices.ContainsFunc(ch.Allow, func(id int64) bool { return !slices.Contains(before, id) }): + case len(ch.Allow) > 0: t.AllowAssignments = false } if len(t.AllowlistIDs) == 0 { diff --git a/internal/connector/setup/apply_test.go b/internal/connector/setup/apply_test.go index b02f775e4..c92f30d6e 100644 --- a/internal/connector/setup/apply_test.go +++ b/internal/connector/setup/apply_test.go @@ -182,15 +182,18 @@ func TestApplyAssignmentOptIn(t *testing.T) { require.NoError(t, err) assert.True(t, out.Trust.AllowAssignments, "restated with the new list") }) - // Only someone newly named would gain assignments unasked: the same - // list again, or a shorter one, keeps the opt-in. - t.Run("kept when the list names nobody new", func(t *testing.T) { + // One rule, no history: a run that passes --allow says the opt-in too, + // off unless it passes the flag, whoever the list names. + t.Run("a run that passes --allow restates it", func(t *testing.T) { f := base f.Trust = admission.Trust{Mode: admission.TrustAllowlist, OperatorID: operatorID, AllowlistIDs: []int64{7, 9}, AllowAssignments: true} - for _, allow := range [][]int64{{9, 7}, {7}} { + for _, allow := range [][]int64{{9, 7}, {7}, {7, 9, 11}} { out, err := Apply(f, Changes{Allow: allow}) require.NoError(t, err) - assert.True(t, out.Trust.AllowAssignments, "allow %v names nobody new", allow) + assert.False(t, out.Trust.AllowAssignments, "allow %v without the flag", allow) + out, err = Apply(f, Changes{Allow: allow, AllowAssignments: &on}) + require.NoError(t, err) + assert.True(t, out.Trust.AllowAssignments, "allow %v with it", allow) } }) t.Run("set over the list connect.json keeps", func(t *testing.T) { diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 1839effeb..620b62537 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -75,7 +75,7 @@ Each becomes a flag on setup (*First-time setup*, step 4): | "me and Jane" | `--allow ` (and everyone else who stays: the list is replaced) | | "anyone in the project can ask it things" | `--trust project` (they ask as participants) | | "Jane operates it, and anyone in the project can ask" | `--trust project --allow ` | -| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers the whole allowlist setup leaves (this run's `--allow`, or the list kept), not one person, so say who that is and confirm. With nobody on the allowlist setup refuses it, so name someone first. A run whose `--allow` names someone new turns it off unless it's passed again | +| "the people I named can assign it work too" | `--allow-assignments-from-authorized`: it covers the whole allowlist setup leaves (this run's `--allow`, or the list kept), not one person, so say who that is and confirm. With nobody on the allowlist setup refuses it, so name someone first. A run that passes `--allow` turns it off unless it's passed again | | "also work in project X" | `--serve ` | | "stop working in X" | `--unserve ` | From 1f5ed625da9744969173ddbc4269eb067b21842b Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 6 Oct 2026 15:24:31 -0700 Subject: [PATCH 11/11] Say the opt-in flag covers the kept allowlist too --- internal/commands/connect.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 8192041cd..cdc89e196 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -385,7 +385,7 @@ Examples: fl.StringVar(&f.operatorProfile, "operator-profile", "", "Profile whose identity is the operator") fl.StringVar(&f.trust, "trust", "", "Who may drive the agent: operator, allowlist or project") fl.StringArrayVar(&f.allow, "allow", nil, "Person id to trust as an operator besides the operator (repeatable; with no --trust it implies allowlist; works with --trust allowlist or project, not operator)") - fl.Var(&f.allowAssignments, "allow-assignments-from-authorized", "Let the people --allow names assign the agent work too (default: the operator's alone; =false to turn off)") + fl.Var(&f.allowAssignments, "allow-assignments-from-authorized", "Let the people the allowlist names, this run's --allow or the list kept, assign the agent work too (default: the operator's alone; =false to turn off)") fl.Lookup("allow-assignments-from-authorized").NoOptDefVal = "true" fl.StringArrayVar(&f.serve, "serve", nil, "Serve a Basecamp project: (repeatable)") fl.StringArrayVar(&f.unserve, "unserve", nil, "Stop serving a project (repeatable)")