You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
connect: the operator's boost on a declared ask is the go - #879
Stacked on #877 (connect-boosted). This PR's base is that branch, so the diff is only the follow-up. #877 is unchanged: there, a boost is a reply.
Problem
In #877 a boost on the agent's own post arrives as a boosted reply, and the skill says it approves nothing. An agent asking the operator for a go still has to wait for an @mention. The operator's decision: a boost counts as the go, but only on an explicit ask.
Declaring an ask is structural, never textual. The agent declares an ask when it posts one: basecamp connect ask <app_url> -P <profile>. It reads the post once, as the agent, through the same summary read admission uses, and refuses a post the agent didn't write or that isn't published. It then writes the recording id and a hash of the post's content into a new ledger table, asks (migration 15). Nothing reads a post's words to decide it is an ask.
Nothing existing carried this. The outbox records only the connector's own lifecycle messages (guard_ack, holding_reply, still_running, completion, retraction). In handoff mode the session's replies go through the CLI, not the outbox, and the events ledger holds only what the feed delivered. So a small table is the least new state that can say "this post is an ask".
The command sits beside discard and opens the ledger the same way: it works beside a running connector, and refuses when the ledger needs migrating under one.
Only the operator's boost is a go, in every trust mode. Admission sets Verdict.AskRecordingID only on an admitted boosted verdict whose performer is Trust.OperatorID and which was not delegated. That is the same spirit as OperatorOnly, but stricter: AllowAssignments, the allowlist and project trust never widen it. A boost on an ask from anyone else is an ordinary boosted reply, and leaves the ask open for the operator.
One ask gets one go. The ledger takes the go in the verdict's own transaction (takeGo in Admission.commit): UPDATE asks SET go_event_id = ? WHERE recording_id = ? AND (go_event_id IS NULL OR go_event_id = ?). Two boosts commit in order, and only the first finds the ask open.
A second boost from the operator is a reply, go: false.
So is a boost removed and given again. Basecamp records that as a new boost and a new event.
Idempotent: the event that took the go keeps it if it is decided again (a redispatch), because go_event_id names that event.
A go taken stays taken, even when its record is closed unwritten (before_this_run). 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. So a boost given while no connector runs uses the ask up, and the skill tells the agent to ask for the go by mention. Losing a go costs the operator a mention; a second go costs a second action. (An earlier head released the go here; Copilot showed the double approval, and the release is removed.)
Declaring an ask again doesn't reopen one whose go was taken. Declaring one after the boost was decided isn't retroactive, so the skill has the agent declare right after posting, before anything else.
The session can tell a go from a thumbs-up. The request line gains "go" (boolean, always present, in the contract and the skill's example). It is true only on the boosted request that took the go, and recording is the ask. The handoff also checks that the trigger is boosted, so no other line can say go.
Skill.
The agent declares a post an ask right after posting it, and the post tells the operator that a boost on it is a go too. It declares only a post that asks for a go and names exactly what it approves, one ask per post.
An operator's go now includes a boosted request with go: true. The go approves what the ask names, and only that. It is never a person's word on a merge, a deploy or anything else the ask didn't name, and never a change to the connector, which only the owner's own mention makes.
owner stays false for a boost.
An edited ask gives no go, and a go approves the words as read
These are the operator's choices on the edited-ask threads.
Hash:admission.AskFingerprint is SHA-256 of the ask's words exactly as the summary read returns them; nothing is normalized and no HTML is canonicalized.
A titled post (message, card, to-do, document) hashes its title and its content, each length-prefixed (<len>:<title><len>:<content>) so the two fields can't run together.
A comment or chat line hashes its content alone, because its title is its parent's and a card rename would change it.
Same read at both ends:connect ask takes the fingerprint when it declares the ask. Admission takes it again from its own read of the boosted recording. I chose reading the post at declaration over taking the content the agent passes in, because both sides then hash the same fields from the same read.
No-op reads match: two reads of an unedited post fingerprint the same. This is tested, and checked live against production on a card.
On an edit: the ledger gives the go only when the two fingerprints match. Otherwise the boost is an ordinary boosted reply with go: false, and the ask isn't used up.
Re-declaring: keeps the first fingerprint and reports edited, so an edited ask stays void and the agent posts a new one.
The worker acts on the read copy. A go: true request carries ask: {title, content}: the exact words admission read and the ledger matched. The skill acts only on that text, never on a fresh read of recording, which stays for replying. An edit after admission therefore can't change what a go approves.
A go whose record carries no such words (one admitted before this build) isn't handed off as a go.
Bare ids:connect ask takes the post's URL, not a bare id, because the read routes by the type the URL names.
Asks don't expire
The go is the operator's deliberate boost on the post itself. What it approves is exactly what the post names, and the skill already has the worker check that this is still current before acting: the branch still at the commit the ask named, an ask already answered as done no longer waiting. A deadline would add a number with nothing to choose it by, and would silently turn a late but deliberate approval into a thumbs-up.
Not done
No new state beyond the asks table; no text matching.
No network check that the declared recording is the agent's own. This isn't needed: a go also requires the boost to pass connect: admit boosts on the agent's own posts as a boosted trigger #877's boosted rule, which admits only boosts on recordings the agent created. A declared ask on someone else's post can never fire.
Nothing in code limits what a go may approve. That limit lives in the skill, as the operator asked.
Tests
Admission (TestOnlyTheOperatorsBoostMayBeAGo): AskRecordingID is set for the operator's boost only. It is not set for an allowlisted operator who is opted in to assigning, a project member, the operator's mention, or a boost on someone else's post.
Ledger (ledger_asks_test.go):
one go per ask: second and re-given boosts are not go, and re-declaring doesn't reopen
the same event keeps its go
an undeclared post gives no go, and declaring after the boost isn't retroactive
a non-operator boost leaves the ask open for the operator's
a go closed before_this_run stays taken, and the next boost is a reply
Handoff: the line says go: true only for the go and is false for the second boost and for a mention. A non-boosted record never says go.
CLI:connect ask reads and fingerprints the comment a #__recording_N URL names, not its parent. A second read of the unedited post gives the same hash. An edited post makes a re-declaration report edited and keeps the first hash. Someone else's post, a bare id, another account, an untrusted host, a listing, or a non-post type is refused and declares nothing.
End to end (TestAGoIsOnlyForTheAskAsDeclared, real admitter, ledger and handoff): an unedited ask gives go: true; an edited one gives go: false and the ask stays open. Removing the hash comparison, or hashing anything but the read content, fails a test.
Mutation-checked: dropping the operator check, the one-go condition or the boosted-only check in the handoff each fails a test.
Gate: thelio was unreachable (it needs a Tailscale re-auth), so I ran the bin/ci targets locally on macOS. fmt, vet, lint on the changed packages, e2e, sync-skills, surface, skill drift and the rest all pass. Not covered locally:
repo-wide make lint stops on the darwin-only internal/connector/driver/proctime_darwin.go, which this PR doesn't touch;
TTY-dependent unit tests fail here because the run has no terminal, and they fail the same way on the base branch.
The reason will be displayed to describe this comment to others. Learn more.
All reported issues were addressed across 14 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Adds declared connector asks, allowing an operator’s first boost to represent approval.
Changes:
Adds basecamp connect ask and persisted ask tracking.
Marks qualifying handoffs with "go": true.
Documents and tests the approval workflow.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Validate approval URLs before recording comment or recording IDs
internal/commands/connect_operator.go:475
extractCommentWithProject discards the URL's host, account, and collection shape before this command validates it. As a result, a look-alike or foreign-account URL is accepted into this profile's approval ledger, and collection URLs such as /chats/{id}/lines can be recorded using the parent ID while the command reports success. Parse URL-shaped arguments first, reject untrusted hosts, collections/missing recording IDs, and accounts that do not match p.file.AccountID, then choose the fragment comment ID or recording ID. This is the established mutating-URL validation pattern in internal/commands/files.go:1782-1829 and internal/hostutil/hostutil.go:69-76.
Copilot: reviewed 7b6f85b with 0 open findings. Its finding on 43f7420, that releasing the go on before_this_run could approve twice, is fixed in 7b6f85b by removing the release.
Declined: Copilot's review-body note to validate the URL's host, account and resource shape in connect ask. A declaration produces a go only when the operator boosts that exact recording and connect: admit boosts on the agent's own posts as a boosted trigger #877's rule finds the agent created it. Recording ids are global, so an id from another account or host can never match an admitted boost here, and validation would guard nothing that can fire. The realistic mistake is declaring the parent card instead of the comment. No URL check can catch that, because the card is a valid recording too. The skill guards against it by having the agent declare from the id its post returned.
Open threads: 0.
For a human: none beyond the operator's decision this PR implements.
🤖 Fixed — reconsidered the review-body point from #879 (review), which I'd declined in the last summary. Its listing case is a real accident path: /chats/{id}/lines declared the Campfire's id and reported success, so the operator's boost on the real line would have arrived as a plain reply. As of 69dbcfa, connect ask follows the files.go pattern for URLs: a trusted host, the agent's own account, and exactly one post (the fragment's comment when there is one). Anything else is a usage error and declares nothing. TestConnectAskRefusesAURLThatIsNotOnePost covers it.
The reason will be displayed to describe this comment to others. Learn more.
All reported issues were addressed across 14 files
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
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.
…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.
…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.
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.
… 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.
…itle
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>:<title><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.
…itle
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.
… 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.
🤖 Rebased onto #877's new head f1bbe9a (head 203aafa), with force-with-lease.
What conflicted: only skills/basecamp-connect/SKILL.md, for the same reason as #877: main split the subagent's instructions into subagent.md and capped the per-request part of SKILL.md at 14,000 characters. No code conflicted.
How I resolved it: each commit took the base's skill files during the rebase, so the commits that only touched the skill dropped out. This PR's whole skill change is re-applied in one final commit, 203aafa:
SKILL.md: the example's go and ask keys and a one-line pointer. To fit under the cap I shortened the example's content, the reply_to and recording wording, and the log-line sentence. It now ends at 13,997 characters.
subagent.md: the declaration step (connect ask with the app_url, never edit a declared ask); Only an operator's word approves; An operator's go, including acting only on ask's text; and the boosted trigger with go.
The text is the same as before the rebase, only placed where the subagent reads it. Locally the connector tests, the request-line contract and skill-layout tests, skill drift, and the connect command tests pass.
…ate 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.
…clock
254f27c 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.
CI: 26 checks pass and 5 are skipped, Security and Trivy Security Scan included. Pull-request checks fired normally on this head, so nothing was dispatched by hand.
Mergeable: yes.
Reviews: Codex and Copilot reviewed this head.
Review threads this round: 6 resolved (6 fixed); 3 open for a decision.
Fixed:
connect: the operator's boost on a declared ask is the go #879 (comment) — the third variation of the declaration-ordering class: a boost given between connect ask listing the post's boosts and writing the ask. I stopped patching. Options for the operator: (a) accept it, which is my recommendation; (b) refuse to declare a post that has boosts; (c) build a reservation.
Edit-then-restore (r4256001582): declined, as before. Only the agent, the operator or an admin can do it, and each already has a more direct route.
A boost that lands during the declaration (r4256298990) and one that was removed before it (r4256330872): the gap is accepted. Either one is the operator's boost on the ask as posted, and the title and content hash must still match the declared text.
The declaration-ordering class is closed at the boost-id floor. Further variations of it will be declined on the same reasoning, not patched.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
commandsCLI command implementationsskillsAgent skillstestsTests (unit and e2e)
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
commands
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Stacked on #877 (
connect-boosted). This PR's base is that branch, so the diff is only the follow-up. #877 is unchanged: there, a boost is a reply.Problem
In #877 a boost on the agent's own post arrives as a
boostedreply, and the skill says it approves nothing. An agent asking the operator for a go still has to wait for an @mention. The operator's decision: a boost counts as the go, but only on an explicit ask.Card: https://app.basecamp.com/2914079/buckets/46292715/card_tables/cards/10396224165
Change
basecamp connect ask <app_url> -P <profile>. It reads the post once, as the agent, through the same summary read admission uses, and refuses a post the agent didn't write or that isn't published. It then writes the recording id and a hash of the post's content into a new ledger table,asks(migration 15). Nothing reads a post's words to decide it is an ask.guard_ack,holding_reply,still_running,completion,retraction). In handoff mode the session's replies go through the CLI, not the outbox, and the events ledger holds only what the feed delivered. So a small table is the least new state that can say "this post is an ask".discardand opens the ledger the same way: it works beside a running connector, and refuses when the ledger needs migrating under one.Verdict.AskRecordingIDonly on an admittedboostedverdict whose performer isTrust.OperatorIDand which was not delegated. That is the same spirit asOperatorOnly, but stricter:AllowAssignments, the allowlist and project trust never widen it. A boost on an ask from anyone else is an ordinaryboostedreply, and leaves the ask open for the operator.takeGoinAdmission.commit):UPDATE asks SET go_event_id = ? WHERE recording_id = ? AND (go_event_id IS NULL OR go_event_id = ?). Two boosts commit in order, and only the first finds the ask open.go: false.go_event_idnames that event.before_this_run). 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. So a boost given while no connector runs uses the ask up, and the skill tells the agent to ask for the go by mention. Losing a go costs the operator a mention; a second go costs a second action. (An earlier head released the go here; Copilot showed the double approval, and the release is removed.)"go"(boolean, always present, in the contract and the skill's example). It is true only on the boosted request that took the go, andrecordingis the ask. The handoff also checks that the trigger isboosted, so no other line can say go.boostedrequest withgo: true. The go approves what the ask names, and only that. It is never a person's word on a merge, a deploy or anything else the ask didn't name, and never a change to the connector, which only the owner's own mention makes.ownerstays false for a boost.An edited ask gives no go, and a go approves the words as read
These are the operator's choices on the edited-ask threads.
admission.AskFingerprintis SHA-256 of the ask's words exactly as the summary read returns them; nothing is normalized and no HTML is canonicalized.<len>:<title><len>:<content>) so the two fields can't run together.connect asktakes the fingerprint when it declares the ask. Admission takes it again from its own read of the boosted recording. I chose reading the post at declaration over taking the content the agent passes in, because both sides then hash the same fields from the same read.boostedreply withgo: false, and the ask isn't used up.edited, so an edited ask stays void and the agent posts a new one.go: truerequest carriesask: {title, content}: the exact words admission read and the ledger matched. The skill acts only on that text, never on a fresh read ofrecording, which stays for replying. An edit after admission therefore can't change what a go approves.connect asktakes the post's URL, not a bare id, because the read routes by the type the URL names.Asks don't expire
The go is the operator's deliberate boost on the post itself. What it approves is exactly what the post names, and the skill already has the worker check that this is still current before acting: the branch still at the commit the ask named, an ask already answered as done no longer waiting. A deadline would add a number with nothing to choose it by, and would silently turn a late but deliberate approval into a thumbs-up.
Not done
askstable; no text matching.boostedrule, which admits only boosts on recordings the agent created. A declared ask on someone else's post can never fire.Tests
Admission (
TestOnlyTheOperatorsBoostMayBeAGo):AskRecordingIDis set for the operator's boost only. It is not set for an allowlisted operator who is opted in to assigning, a project member, the operator's mention, or a boost on someone else's post.Ledger (
ledger_asks_test.go):before_this_runstays taken, and the next boost is a replyHandoff: the line says
go: trueonly for the go and is false for the second boost and for a mention. A non-boosted record never says go.CLI:
connect askreads and fingerprints the comment a#__recording_NURL names, not its parent. A second read of the unedited post gives the same hash. An edited post makes a re-declaration reporteditedand keeps the first hash. Someone else's post, a bare id, another account, an untrusted host, a listing, or a non-post type is refused and declares nothing.End to end (
TestAGoIsOnlyForTheAskAsDeclared, real admitter, ledger and handoff): an unedited ask givesgo: true; an edited one givesgo: falseand the ask stays open. Removing the hash comparison, or hashing anything but the read content, fails a test.Mutation-checked: dropping the operator check, the one-go condition or the boosted-only check in the handoff each fails a test.
Gate: thelio was unreachable (it needs a Tailscale re-auth), so I ran the
bin/citargets locally on macOS. fmt, vet, lint on the changed packages, e2e, sync-skills, surface, skill drift and the rest all pass. Not covered locally:make lintstops on the darwin-onlyinternal/connector/driver/proctime_darwin.go, which this PR doesn't touch;GitHub CI is the gate for both.