Serve a connector worker its dispatch over MCP

[jorgemanrubia]

A worker the connector starts has no way to get its instruction without the connector pasting content into its prompt. That is the path this design rules out: content crosses the front thread, costs tokens, and leaves no record of what the worker saw. There is also no way for a worker to report that it acknowledged or finished something, so the connector can't tell a delivered instruction from a lost one.

Context: 17 basecamp_connect MCP domain. Follows Hand intake's ledger to admission, whose stored verdict this serves.

The dispatcher on card 18 starts workers with basecamp mcp pointed at the ledger. What the worker pulls, and what it reports back, goes through this domain.

What changes:

• basecamp mcp --connect-state <dir> --connect-token-fd <n> serves a basecamp_connect domain from the connector's ledger. The task token arrives on an inherited pipe or socket and is read and closed before anything else runs. It never exists at a path, in argv or in the environment. A descriptor that is standard I/O, a regular file or anything else is refused and left alone. A token left in $BASECAMP_CONNECT_TASK_TOKEN is removed, and the server refuses to start.
  • The directory must be the canonical state directory for this account, $XDG_STATE_HOME/basecamp/connect/<account>-<agent>, checked in one place. The ledger must already exist, and the server never creates or migrates one.
  • Without the flags the domain doesn't exist, and it refuses to be served read-only.
• The dispatch lifecycle is written down as one state machine at the top of the ledger dispatch code. It covers every state a record, a task, a delivery and a guard can be in, every transition and who performs it, and the invariants across them. Each transition is enforced by the ledger's single write or a database trigger, and a table-driven test tries every pair, allowed and refused.
• Each event is on at most one live task. A unique index enforces this, so a retried or concurrent launch can't hand one event to two workers. Creating a task moves its records to dispatched in the same transaction, which means a record is dispatched exactly while a live task carries it. Superseding a task retires its events, so a redispatch can put them on a new task. Task creation also works inside a caller's transaction, so the dispatcher can write the task, its attempt and the exposure in one commit.
• get_dispatch returns the instruction for an event on the task, or the earliest one not yet acknowledged. It skips events whose record has left the path to a worker, so one blocked or withdrawn event never hides the rest.
  • Before returning, it records that the worker was handed the event and cancels an armed acknowledgement guard. A repeat writes nothing.
  • The result is an allowlist: recording pointer, reply destination, requester, class, whether to acknowledge, whether the guard already did, and the content with the agent's own mention stripped. Mentions are found by walking the markup the way the SDK's mention reader does. It never includes a route, a feed position or the token.
• ack_dispatch marks the event delivered and records the worker's own acknowledgement. complete_dispatch records the outcome and acknowledges.
  • A report is recorded whatever happened to the record since the worker was handed it, because it is what the worker did. The record moves to completed only when it is still dispatched.
  • Both are safe to retry. A conflicting second report is refused, because a reported outcome stands.
• Every call is bound to the task its token names. A token that names no live task is refused at startup, and the check runs again in every call's own transaction, so a superseded token is refused at once. There is no listing, and an event on another task reads the same as an unknown one.
  • Ledger failures reach the model as a generic message; only what the worker can fix is spelled out.
• The worker's server opens the ledger as it is. It never creates or migrates one, and refuses a schema version it doesn't write. --domains narrowing always keeps the connect domain, and --read-only is refused before the token or the ledger is touched.
• The ledger gains tasks and task_events with only the columns these three actions need. That means a hash of the token (never the token itself), the time a redispatch superseded it, and each event's delivery state, report and retirement. A database trigger holds that delivery never moves backwards. Card 18's dispatcher adds attempts and the rest to these same tables.

CreateTask and SupersedeTask exist so the ledger can be driven end to end in tests and by card 18. Nothing in the CLI calls them yet. Firing the guard, attaching follow-ups to a live task and settling siblings when a task ends belong to cards 18 and 20.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate dispatch and ledger issues must be fixed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a task-bound basecamp_connect MCP domain backed by the connector ledger, allowing workers to retrieve, acknowledge, and complete dispatches.

Changes:

• Adds task/event ledger schema and dispatch lifecycle.
• Adds MCP handlers, token binding, and --connect-state.
• Adds ledger, MCP, CLI, and surface tests.

Review findings:

• Critical (2 votes): Prevent duplicate active tasks for the same event.
• Moderate (1 vote each): Fix ledger creation race, validate supersession targets, clear acknowledgement guards, and reject reports for no-longer-dispatchable events.

File summaries

File	Reviewed changes
internal/mcpserver/server.go	Registers the conditional connect domain.
internal/mcpserver/dispatch.go	Routes connect operations to ledger handlers.
internal/mcpserver/connect.go	Defines MCP dispatch actions and responses.
internal/mcpserver/connect_test.go	Tests connect-domain behavior and refusals.
internal/connector/ledger.go	Adds task and task-event schema.
internal/connector/ledger_dispatch.go	Implements task-bound dispatch lifecycle and reporting.
internal/connector/ledger_dispatch_test.go	Tests dispatch, reporting, tokens, and idempotency.
internal/connector/ledger_admission_test.go	Updates migration-version coverage.
internal/commands/mcp.go	Adds connect-state setup and token handling.
internal/commands/mcp_connect_test.go	Tests CLI integration and state validation.
.surface	Records the new CLI flag.

Review details

Suppressed comments (5)

internal/commands/mcp.go:149

• The preflight Lstat does not uphold the guarantee that a worker server never creates a connector ledger: if ledger.db is removed between this check and OpenLedger, OpenLedger creates and migrates a new file. Open the existing file in a no-create mode (or provide an existing-ledger opener) so the check and open cannot create a replacement.

	ledger, err := connector.OpenLedger(path)

internal/connector/ledger_dispatch.go:129

• SupersedeTask reports success even when taskID does not exist, because the UPDATE's affected-row count is ignored. A stale or incorrect dispatcher task id therefore leaves the old token usable while the caller believes the worker was fenced. Check RowsAffected and return a not-found error, while keeping an existing supersession idempotent.

	_, err := l.db.ExecContext(ctx, `UPDATE tasks SET superseded_at = COALESCE(superseded_at, ?) WHERE id = ?`, l.timestamp(), taskID)

internal/connector/ledger_dispatch.go:379

• Ack accepts an already-exposed event, which is the state the dispatcher can leave before the worker calls get_dispatch, but this update never clears an armed acknowledgement guard. A worker that acknowledges directly can therefore still have the 30-second guard fire and post a duplicate acknowledgement. Cancel an armed guard in this transaction (and in the analogous Complete update below) when applying the worker's report.

UPDATE task_events
SET delivery = CASE WHEN delivery = 'exposed' THEN 'delivered' ELSE delivery END,
    delivered_at = COALESCE(delivered_at, ?),
    ack_id = COALESCE(ack_id, ?)
WHERE task_id = ? AND event_id = ?`, d.ledger.timestamp(), nullableID(ackID), taskID, eventID)

internal/connector/ledger_dispatch.go:425

• Complete also accepts an already-exposed event and is documented as acknowledging it, but this write leaves guard = 'armed'. If the worker completes without a preceding get_dispatch, the connector can later fire the guard after completion and duplicate the acknowledgement. Clear an armed guard in this same transaction before committing the completed report.

UPDATE task_events
SET delivery = 'completed', delivered_at = COALESCE(delivered_at, ?), completed_at = ?,
    outcome = ?, links = ?, reply_id = ?
WHERE task_id = ? AND event_id = ?`, now, now, string(c.Outcome), string(encoded), nullableID(c.ReplyID), taskID, eventID)

internal/connector/ledger_dispatch.go:455

• report only checks that the task row is not admitted; it never checks the underlying event state before applying an acknowledgement. A dispatched record can move to blocked (which clears its snapshot) while its task_events row remains exposed, and ack_dispatch will still mark it delivered; complete_dispatch correctly refuses the same transition via ledger.move. Re-read/validate the event state in this transaction and return ErrNotDispatchable before applying an ack when the exposed record is no longer dispatched.

	if te.delivery == DeliveryAdmitted {
		return Receipt{}, fmt.Errorf("connector: event %d: %w", eventID, ErrNotExposed)
	}

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The existing-ledger path can recreate a missing SQLite file, violating the no-create guarantee.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🔵 Needs a closer look

Supersession can orphan dispatched records, completed-but-unexposed work can be served, and existing-ledger opening has a file-creation race.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

internal/connector/ledger_dispatch.go:192

• Retiring these rows leaves every unfinished record in StateDispatched, even though no live task carries it anymore. liveConversation treats any dispatched record as live (internal/connector/ledger_admission.go:169-175), so if replacement creation is delayed or fails, later events on that conversation remain queued indefinitely; this also breaks the invariant documented on CreateTask that dispatched records exactly match live tasks. Either move still-dispatched records back to the appropriate non-live state in this transaction, or expose an atomic supersede-and-replace operation so there is never an orphaned interval.
  internal/connector/ledger_dispatch.go:371
• This makes any completed record servable, even when this task never exposed it. Since the ledger permits a dispatched record to be completed through another lifecycle path, an explicit get_dispatch(event_id) can turn an admitted task event into exposed after the work was already settled. Preserve retries for previously handed work, but require a non-admitted task delivery before serving a completed record.

internal/connector/ledger.go:96

• This pre-check does not actually preserve the “never creates” guarantee. openLedger subsequently calls securePath, and setup.EnsurePrivateFile creates the file when it is missing (internal/connector/setup/private_state.go:112-134). If the ledger disappears between this Lstat and that call, the worker creates an empty ledger before rejecting its schema. Please add a non-creating secure-open path for OpenExistingLedger rather than relying on this racy name check.

// Neither the privacy check nor SQLite may create the file on this path: the
// check only inspects, and the database is opened with mode=rw, so a ledger
// removed at any moment is an error rather than a new empty one.
func OpenExistingLedger(ctx context.Context, path string) (*Ledger, error) {

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Critical and moderate findings remain unresolved in token handling and dispatch behavior.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

internal/connector/ledger_dispatch.go:352

• The no-ID path filters only dispatched, but the same function below treats completed records with retained content as servable. Ledger.SetState can complete a record without changing its task_events delivery, so an admitted/exposed task event can be returned when requested explicitly but is silently skipped by get_dispatch without an ID; a restarted worker can therefore be told there is no work. Use the same dispatched-or-completed predicate here.

SELECT te.event_id FROM task_events te JOIN events e ON e.id = te.event_id
WHERE te.task_id = ? AND te.delivery IN ('admitted', 'exposed')
  AND e.state = 'dispatched' AND e.content_dropped = 0 AND e.snapshot IS NOT NULL
ORDER BY te.event_id LIMIT 1`, taskID).Scan(&eventID)

internal/connector/ledger_dispatch.go:738

• This condition cannot distinguish an absent sgid from a present-but-empty first sgid, so a later duplicate attribute overwrites the first one. The SDK's MentionedPersonIDs parser treats the first sgid as authoritative, including an empty value; consequently this stripper can disagree with admission for duplicate-attribute markup. Track whether sgid was seen separately and only capture the first value.

				if t.sgid == "" && strings.EqualFold(attr, "sgid") {

internal/connector/ledger_dispatch.go:646

• This strips any <bc-attachment> whose SGID decodes to the agent's Person ID, regardless of whether it is a mention. In this codebase application/vnd.basecamp.mention is the marker that distinguishes mention attachments from other attachments (internal/richtext/richtext.go:1933-1948), so a non-mention attachment carrying a person SGID would be removed from the worker's content, contrary to the function's "leaves every other attachment" contract. Parse and require the mention content type before stripping.

		if strings.EqualFold(t.name, "bc-attachment") {
			if id, isPerson := basecamp.PersonIDFromSGID(html.UnescapeString(t.sgid)); isPerson && id == personID {

• Files reviewed: 13/13 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

One critical state-root validation issue and two moderate mention-parser issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

internal/connector/ledger_dispatch.go:779

• This uses t.sgid == "" to mean both “no sgid seen yet” and “the first sgid was explicitly empty.” The SDK mention reader treats the first sgid attribute as authoritative, including an empty value, so markup such as <bc-attachment sgid="" sgid="<agent-sgid>"> is not an agent mention there but is stripped here. Track whether the attribute has been seen separately so duplicate attributes preserve the SDK behavior.

internal/connector/ledger_dispatch.go:767

• This parser uses the same isTagNameByte rule for attribute names, so punctuation is treated as a separator even though the SDK's mention reader treats it as part of the attribute name. For example, data.sgid="..." can be split into a synthetic sgid attribute here and make a non-mention attachment get stripped; tag names such as bc-attachment.foo are similarly misidentified. Match the SDK's delimiter rules with separate tag-name and attribute-name scanners, and cover these hostile cases.

			case isTagNameByte(c):
				attrStart := at
				for at < len(text) && isTagNameByte(text[at]) {
					at++
				}
				attr := text[attrStart:at]
				for at < len(text) && isSpaceByte(text[at]) {
					at++
				}
				if at >= len(text) || text[at] != '=' {
					continue

• Files reviewed: 13/13 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

internal/commands/mcp.go:74

• A positive descriptor is pre-read for every mcp invocation, even when --connect-state is absent. Thus basecamp mcp --connect-token-fd 3 can drain or wait up to the five-second token timeout on an unrelated pipe before RunE reports that the flag requires --connect-state; the invalid combination should be rejected without consuming the descriptor. Gate the pre-scan on a valid, non-empty connect-state flag (or perform this combination check before reading).

func connectTokenFDArg(args []string) (int, bool) {
	if !isMCPInvocation(args) {
		return 0, false
	}

internal/connector/ledger.go:639

• task_events_pull_is_recorded_once only rejects repeated, retired, or withdrawn pulls; it never requires the row to be in delivery='exposed'. A raw write can therefore set pulled_at on an admitted task event, after which report paths accept work that was never exposed. Add the delivery-state check to keep the trigger aligned with the documented pull lifecycle.

WHEN NEW.pulled_at IS NOT OLD.pulled_at AND (
  OLD.pulled_at IS NOT NULL
  OR OLD.retired_at IS NOT NULL
  OR OLD.withdrawn_at IS NOT NULL)

internal/connector/ledger.go:704

• task_events_exposure_comes_first only rejects admitted→delivered/completed. It still permits an unpulled launch exposure (delivery='exposed', pulled_at IS NULL) to become delivered, and permits it to become completed while the record is still dispatched, bypassing the worker-handoff distinction. The trigger should require pulled_at for worker delivery and distinguish the explicit dispatcher-settlement path (completed record) from an invalid raw/worker transition.

CREATE TRIGGER task_events_exposure_comes_first
BEFORE UPDATE OF delivery ON task_events
WHEN OLD.delivery = 'admitted' AND NEW.delivery IN ('delivered', 'completed')
BEGIN
  SELECT RAISE(ABORT, 'nothing a worker was never handed is acknowledged or completed');

internal/connector/ledger.go:391

• claimLedger reads open.info while holding openLedgers.Mutex, but this assignment is performed without that mutex. An aliased open racing with the first check can therefore race on file.info (and the alias scan can observe it while it is being replaced), defeating the claimed race-safe one-check-per-file behavior. Protect the update with the same map mutex, while retaining file.mu for the per-file check.

	info, err := os.Lstat(abs)
	if err != nil {
		return fmt.Errorf("connector: inspect the ledger: %w", err)
	}
	file.info, file.checked = info, true

internal/connector/ledger_dispatch.go:1317

• For a basename such as -52007412, the agent portion parses successfully but ParseUint(account, ...) fails, so this returns StateDirOtherAccount with an empty account. The CLI then says it belongs to account `` rather than reporting a malformed state-directory name; reserve StateDirOtherAccount for a valid nonzero numeric account and classify parse failures as `StateDirMisnamed`.

	given, errGiven := strconv.ParseUint(account, 10, 64)
	want, errWant := strconv.ParseUint(accountID, 10, 64)
	if errGiven != nil || errWant != nil || given == 0 || given != want {
		return refuse(StateDirOtherAccount, account)

• Files reviewed: 25/25 changed files
• Comments generated: 3
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Unresolved critical ledger integrity and ownership findings, plus moderate CLI token-handling findings, block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

internal/commands/mcp.go:88

• This pre-scan consumes the token for basecamp mcp --connect-state ... --connect-token-fd N --help, even though Cobra handles --help without running the server. A pipe can therefore be drained or block for the read timeout just to print help, and the descriptor is consumed by an invocation that never serves the connect domain. Exclude help/version (and other Cobra early-exit paths) using the same command parsing before reading the descriptor.

	if *readOnly || strings.TrimSpace(*state) == "" || !flags.Changed("connect-token-fd") {

internal/commands/mcp.go:84

• This pre-scan whitelists unknown flags and declares only three of mcp's flags, so it can consume a token for an invocation Cobra will reject. For example, --connect-token-fd 3 --bogus (or a missing --domains value) is ignored here, then actual parsing fails after the descriptor was drained and closed. Build the preparse set from the command's complete local/inherited flag definitions, or leave the descriptor untouched whenever parsing cannot be proven equivalent.

	flags.ParseErrorsWhitelist.UnknownFlags = true
	flags.SetOutput(io.Discard)
	readOnly := flags.Bool("read-only", false, "")
	state := flags.String("connect-state", "", "")
	fd := flags.Int("connect-token-fd", -1, "")

internal/connector/strip_mentions_test.go:13

• The heading says “Three properties,” but the test immediately enumerates four numbered properties. Update the count so this contract comment remains accurate.

// parsers disagree. Three properties, for every input:

• Files reviewed: 25/25 changed files
• Comments generated: 5
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Unresolved findings include one critical Windows compilation issue and six moderate command, descriptor, and UID issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

internal/commands/mcp.go:164

• This environment check runs in RunE, but cli.Execute calls TakeConnectTaskToken before the root hooks and this check. If a stale environment token is present alongside a valid inherited descriptor, startup drains and closes the valid descriptor before refusing, losing the connector handoff; the environment token also remains available to pre-run work. Check and record/unset this refusal in the pre-scan before reading any descriptor.

			// A token in the environment is taken out and refused whatever
			// the flags: it is not a way in for any server.
			if _, set := os.LookupEnv(connectTaskTokenEnv); set {
				_ = os.Unsetenv(connectTaskTokenEnv)
				return output.ErrUsageHint("$"+connectTaskTokenEnv+" is not read",
					"Hand the task token over on an inherited descriptor with --connect-token-fd, so it never sits in an environment.")

internal/commands/mcp_token_unix.go:52

• Once Fstat has established that this is a pipe or socket, a SetNonblock failure returns without closing the descriptor. cli.Execute then continues through the root hooks with the token fd still open, allowing it to be inherited despite the contract that a valid token descriptor is consumed and closed; take ownership with os.File/defer Close before this operation (or close on this error path).

	if err := unix.SetNonblock(fd, true); err != nil {
		return "", output.ErrUsage(fmt.Sprintf("could not read the task token from descriptor %d: %v", fd, err))

• Files reviewed: 27/27 changed files
• Comments generated: 5
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Startup token handling and cached ledger path validation still have unresolved issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

internal/commands/mcp.go:83

• The pre-scan does not actually preserve the copied root flag's value validation: discardedValue.Set accepts every string. For example, basecamp --json=not-a-bool mcp --connect-state ... --connect-token-fd N is treated as a valid serving invocation here and consumes/closes N, but Cobra rejects the invalid --json before starting the server. Use non-mutating clones that retain the original value parsers (and other root validation) so a token is read only for an invocation the real command will run.

type discardedValue struct{ kind string }

func (discardedValue) String() string   { return "" }
func (discardedValue) Set(string) error { return nil }
func (v discardedValue) Type() string   { return v.kind }

internal/commands/mcp.go:138

• The pre-scan treats any successfully parsed flag set as a serving invocation, but the real root command performs additional semantic validation later: root.go:266-269 rejects mutually exclusive --stats/--no-stats and --hints/--no-hints, and root.go:188-203 rejects --jq with --count or --ids-only. For example, basecamp --stats --no-stats mcp --connect-state ... --connect-token-fd N consumes the one-shot descriptor and then Cobra refuses to run, so the worker loses its dispatch even though this server never starts. The startup decision needs to share these root validations (or otherwise leave the descriptor untouched for invocations that will be rejected).

	if err := flags.Parse(rest); err != nil {
		return 0, false
	}
	if flags.NArg() > 0 {
		return 0, false // cobra.NoArgs refuses it
	}
	if help, _ := flags.GetBool("help"); help {
		return 0, false // cobra prints help and serves nothing
	}

	readOnly, _ := flags.GetBool("read-only")
	state, _ := flags.GetString("connect-state")
	fd, err := flags.GetInt("connect-token-fd")
	if err != nil || readOnly || !connectStateGiven(state) || !flags.Changed("connect-token-fd") {
		return 0, false
	}
	return fd, true

• Files reviewed: 28/28 changed files
• Comments generated: 3
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Critical descriptor handling and moderate validation and acknowledgement issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

internal/commands/mcp.go:64

• The preflight marks a descriptor as close-on-exec before it knows whether the parsed invocation will serve anything. For --connect-state ... --read-only --connect-token-fd N (and for a descriptor flag without connect state), RunE promises to refuse before touching the token, but this already mutates the descriptor's flags. Parse the final mode first and leave rejected invocations untouched.

	if fd, ok := connectTokenFDArg(args); ok {
		markCloseOnExec(fd)
	}

internal/commands/mcp.go:72

• This pre-scan decides that the invocation is MCP by searching for any argument equal to mcp. A non-MCP command such as basecamp search --query mcp --connect-token-fd 3 (or a value after --) therefore changes an unrelated inherited descriptor before Cobra rejects the command. Determine the actual command path and parse flag values before touching descriptors.

	if !slices.Contains(args, "mcp") {
		return 0, false

internal/connector/ledger.go:433

• The cached-validation branch only rechecks the final directory's mode bits. It no longer revalidates the ancestor chain and directory ownership that CheckPrivateFile established, so a later open can accept the same ledger inode through a redirected or newly unsafe ancestor while SQLite uses that route for its -wal/-shm sidecars. Add a non-opening directory-chain/ownership check before accepting cached validation.

	// The whole chain, not only the last directory: a path later redirected
	// through a writable or foreign-owned ancestor is not the path the first
	// check passed. Directories are vetted without opening the ledger, which
	// is the one thing this path must not do.
	dir := filepath.Dir(path)
	if err := setup.CheckPrivateDir(dir); err != nil {

internal/mcpserver/connect.go:157

• Because ack_id is optional, this forwards a nil acknowledgement id to the ledger. If the first ack_dispatch succeeds without an id and a retry supplies the id of the acknowledgement it posted, the current TaskDispatch.Ack path treats the delivered row with a NULL ack_id as writable and changes the receipt instead of refusing a conflicting second report. Enforce the nil-versus-present case as a conflict (or persist an explicit acknowledgement-without-id state) and add a regression for this retry sequence.

	ackID, err := optionalID(params, "ack_id")
	if err != nil {
		return gateway.ErrorResult("%v", err), nil
	}
	receipt, err := d.Ack(ctx, eventID, ackID)

• Files reviewed: 30/30 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Unresolved critical dispatch-order and conversation-identity issues, plus descriptor-safety issues, block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

internal/commands/mcp.go:63

• PrepareConnectToken sets FD_CLOEXEC before readTaskToken verifies that this is a pipe or socket. Consequently a regular file or other inherited descriptor is not left untouched as the reader's contract promises; even --read-only/blank-state refusals mutate the descriptor before returning. Validate the descriptor type during the preflight (or restore its original flags on every refusal) while still avoiding a read or close.

internal/commands/mcp.go:78

• If --connect-token-fd is supplied more than once, Cobra/pflag uses the last value, but this helper returns after the first valid value. The worker can therefore read the last descriptor while only the first is marked close-on-exec, allowing the token pipe to be inherited by a child started during the persistent hooks. Track the last valid descriptor (or mark every specified descriptor) so the descriptor actually consumed is protected.

	for i, arg := range args {
		value, isFlag := strings.CutPrefix(arg, "--connect-token-fd")
		switch {
		case !isFlag:
			continue

• Files reviewed: 30/30 changed files
• Comments generated: 2
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Critical token-descriptor isolation issues and additional startup and ledger correctness issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

internal/commands/mcp.go:110

• The environment-token check runs before the --read-only branch below. Thus basecamp mcp --connect-state ... --read-only with BASECAMP_CONNECT_TASK_TOKEN set removes/touches the token and reports the environment error instead of refusing read-only before touching any token, as the startup contract requires. Move the connect/read-only guard ahead of this check while preserving the environment refusal for non-connect invocations.

			if _, set := os.LookupEnv(connectTaskTokenEnv); set {
				_ = os.Unsetenv(connectTaskTokenEnv)
				return output.ErrUsageHint("$"+connectTaskTokenEnv+" is not read",
					"Hand the task token over on an inherited descriptor with --connect-token-fd, so it never sits in an environment.")
			}

internal/connector/ledger.go:118

• OpenExistingLedger passes owner=false to this DSN, but it still executes _pragma=journal_mode(WAL) before checking the schema. Opening a pre-existing rollback-mode ledger therefore changes its persistent journal mode even when the worker then rejects it as an incompatible schema, contrary to the requirement that the worker open the ledger as-is. Apply the WAL pragma only for owner opens.

func ledgerDSN(path string, owner bool) string {
	dsn := "file:" + path + "?_pragma=busy_timeout(5000)&_pragma=journal_mode(WAL)&_pragma=foreign_keys(1)&_txlock=immediate"
	if !owner {
		dsn += "&mode=rw"
	}
	return dsn

internal/sysfd/sysfd.go:23

• Descriptor is an exported int, so callers can still construct sysfd.Descriptor(-1) or a value above the ceiling directly; Int/Uintptr then return or cast it without validation. That contradicts the stated Parse/Of-only invariant and lets future FD call sites bypass the central check. Use an unexported representation (for example, a struct with a private field) or make every conversion validate.

• Files reviewed: 33/33 changed files
• Comments generated: 2
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

The acknowledgement trigger permits storing an acknowledgement ID before delivery is acknowledged.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 35/35 changed files
• Comments generated: 1
• Review effort level: Balanced