Repository navigation
Let named operators assign the agent work, when the operator opts them in - #847
Conversation
|
@codex review |
github-actions
Bot
added
commands
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcb8127086
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@cubic-dev-ai review |
@jeremy I have started the AI code review. It will take a few minutes to complete. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Review state at head cbc0db1
How the opt-in behaves across setup runs. Four review rounds each found a case where inferring "someone new was named" went wrong, so there's now one history-free rule:
The help, the field contract and the skill all say this. The skill presents the opt-in as covering the whole allowlist, to be confirmed with the person. Declined earlier: |
e944b9f to
b5cf878
Compare
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
@jeremy I have started the AI code review. It will take a few minutes to complete. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…m 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.
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.
…ery 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.
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.
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.
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.
…nly 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.
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.
cbc0db1 to
1f5ed62
Compare
|
@codex review |
|
@cubic-dev-ai review |
@jeremy I have started the AI code review. It will take a few minutes to complete. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Ports
--allow-assignments-from-authorizedfrom basecamp-local-agent-connector. Stacked on #843 (baseconnect-role): the two touched the same trust code and setup text, so this is rebased onto #843 with the conflicts resolved. It retargets tomainonce #843 merges. On #843 the opt-in also covers operators named beside--trust project, and a test holds that the project's members still can't assign.Why the local connector's caution doesn't carry over
The local connector keeps assignments operator-only by default because it reads the assigner from a webhook payload it can't corroborate, so there the opt-in rests on a secret URL path. Here the assigner is the account feed's
creator_id/performed_by_id. Basecamp writes that from the session that made the change; no payload carries it, and the gate already trusts it for every other trigger.So naming someone and opting them 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 (invariant 2).
What changes
connect.jsongainstrust.allow_assignments.Policy.Validaterefusesallow_assignmentswhen the allowlist names nobody. The gate opens the assigned rule to a performer the allowlist names only when the opt-in is on. Project members, strangers and agents gain nothing.connect setup --allow-assignments-from-authorizedturns it on, and=falseturns it off. Leaving the flag out keeps what the file has. Setting it with nobody named is refused, and a run that leaves nobody named drops it, so the file never carries an opt-in nobody can use. It's added to guided setup's policy flags.connect showsays "they may assign". The setup help, the admission invariants and the skill's phrase table follow.Tests
Each fails without the change:
TestNamedOperatorsMayAssignWhenOptedIn(admission)TestApplyAssignmentOptIn(setup)TestConnectSetupOptsNamedOperatorsInToAssignments(command)make checkpasses on linux.Summary by cubic
Lets named operators assign the agent work, when the operator opts them in.
--allownow works with--trust project, and the assignment opt-in (--allow-assignments-from-authorized) rides with--allow: a run that passes--allowrestates it, off unless the flag is passed again, and a run that passes neither keeps both.trust.allow_assignments. Setting it with nobody named is refused, as is an allowlist naming only the operator; a run that leaves nobody named drops it. The assigner is the account feed's performer, which Basecamp writes, so naming someone trusts that person.Written for commit 1f5ed62. Summary will update on new commits.