Let a person see, authorize, close and hold what the connector runs - #741
jorgemanrubia wants to merge 50 commits into
Conversation
github-actions
Bot
added
commands
There was a problem hiding this comment.
🟡 Changes recommended
Production outbox wiring is missing, terminal decision invariants are bypassable, and doctor can approve an unsupported driver.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds operator visibility and control for connector execution, including durable holds, record decisions, migration tooling, lifecycle messaging, and worker selection.
Changes:
- Adds status, doctor, redispatch, discard, release, shadow promotion, and import commands.
- Introduces held records, operator decisions, lifecycle outbox processing, and migration safeguards.
- Adds Claude worker dispatch infrastructure and extensive invariant tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
STYLE.md |
Documents the bare connect command exception. |
scripts/check-bare-groups.sh |
Allows the runnable connect command group. |
internal/connector/setup/file.go |
Adds worker configuration and validation. |
internal/connector/setup/file_test.go |
Tests worker defaults and validation. |
internal/connector/setup/apply.go |
Applies worker configuration changes. |
internal/connector/sdk_dispatch.go |
Adds SDK reply and membership adapters. |
internal/connector/promote.go |
Implements atomic shadow-ledger promotion. |
internal/connector/policy_test.go |
Tests worker permission and prompt policy. |
internal/connector/outbox.go |
Defines lifecycle-message storage and resolution. |
internal/connector/outbox_run.go |
Implements sending and reconciliation. |
internal/connector/outbox_basecamp.go |
Implements Basecamp message posting/listing. |
internal/connector/operator_status_test.go |
Tests read-only, secret-free status. |
internal/connector/operator_migration_test.go |
Tests promotion/import crash safety. |
internal/connector/lock.go |
Exposes diagnostic lock-holder information. |
internal/connector/lifecycle.go |
Renders and records lifecycle messages. |
internal/connector/ledger.go |
Registers held state and new migrations. |
internal/connector/ledger_tasks.go |
Prevents task selection during holds. |
internal/connector/ledger_tasks_test.go |
Expands task lifecycle coverage. |
internal/connector/ledger_import.go |
Implements reconciliation imports. |
internal/connector/ledger_hold.go |
Implements holds, generations, and decision invariants. |
internal/connector/ledger_events.go |
Adds operator-only state transitions. |
internal/connector/ledger_decisions.go |
Implements redispatch and discard decisions. |
internal/connector/ledger_admission.go |
Reports database-enforced held verdicts. |
internal/connector/driver/spawn/spawn.go |
Selects spawn drivers by worker. |
internal/connector/driver/spawn/spawn_test.go |
Verifies worker-driver coverage. |
internal/connector/driver/driver_test.go |
Tests process isolation and termination. |
internal/connector/driver/claude/claude_test.go |
Tests Claude session safety and lifecycle. |
internal/connector/dispatcher.go |
Adds workspace recovery and safer prompts. |
internal/connector/admission/matrix.go |
Adds the held admission state. |
internal/connector/admission/commit.go |
Accepts database-produced held results. |
internal/connector/admission/commit_test.go |
Updates admission state coverage. |
internal/commands/connect.go |
Registers runnable and operator commands. |
internal/commands/connect_run.go |
Implements the foreground connector runtime. |
internal/commands/connect_run_test.go |
Tests run flags and state paths. |
internal/commands/connect_process_unix.go |
Detects live processes on Unix. |
internal/commands/connect_process_other.go |
Supplies the non-Unix fallback. |
internal/commands/connect_operator_test.go |
Tests operator command workflows. |
internal/commands/connect_doctor.go |
Adds connector readiness diagnostics. |
internal/commands/connect_doctor_mcp_unix.go |
Performs Unix MCP handshake checks. |
internal/commands/connect_doctor_mcp_other.go |
Skips MCP checks off Unix. |
e2e/smoke/smoke_lifecycle.bats |
Records operator commands as smoke-test exclusions. |
.surface |
Updates the generated CLI surface. |
Review details
- Files reviewed: 46/46 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ff18d50 to
4bb399e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Outbox recovery, exhaustive reply listing, promotion recovery, and platform-specific tests have unresolved correctness issues.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
internal/connector/outbox_run.go:145
- Recovery cannot safely reconcile every
sendingintent immediately. A previous process may have been killed just after committing the claim while its HTTP request is still in flight; listing at once can find zero matches and permanently mark the intentindeterminatebefore the request lands. ApplyReconcileAfterduring startup as well so every request gets the documented landing window.
internal/connector/outbox_run.go:326 - A failed destination listing leaves the intent eligible for reconciliation on every outbox tick, so an outage causes one request per second forever. This also never reaches the promised terminal
indeterminateoutcome for an unlistable destination. Persist a bounded retry count/next-at time, back off failed listings, and settle after the configured attempt/deadline limit.
internal/connector/promote.go:120 - Promotion is not crash-idempotent when the shadow was already started with
--hold.SetHoldintentionally preserves the existing marker's cause, so the moved ledger still has causehold; after a crash following the rename,promotedrejects it because it requiresshadow_promote, even though this transaction recorded a shadow-promotion decision. Ensure promotion leaves an idempotence marker thatpromotedrecognizes (for example, validate the recordedshadow_promotedecision rather than only the preserved hold cause).
internal/connector/sdk_dispatch.go:25 - The implementation does not satisfy this exhaustive-listing contract: comments ignore
Meta.Truncated, and Campfire reads only one response even though the adjacentBasecampPoster.Listimplementation explicitly checks comment truncation and paginates chat lines. A busy destination can therefore hide a second reply and causeAdoptableReplyto adopt the visible one as unique. Return an error for truncated comments and page Campfire until reachingsince, as the poster does.
- Files reviewed: 57/57 changed files
- Comments generated: 1
- Review effort level: Balanced
4bb399e to
7e5d478
Compare
65257e0 to
44d228d
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-cutting database invariants, crash recovery, process control, and stacked dependency warrant final human review.
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 0 new
- Review effort level: Balanced
b5b4a52 to
6479301
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Lock-based migration safety, process identity verification, and empty permission locations have unresolved correctness and security issues.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 61/61 changed files
- Comments generated: 3
- Review effort level: Balanced
5201c33 to
5e3043e
Compare
88addc4 to
6b5f88d
Compare
b871864 to
85b9470
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Blocked redispatches can still produce stale prompts, connection status remains inaccurate, and doctor does not exercise the worker MCP mode.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 33/33 changed files
- Comments generated: 4
- Review effort level: Balanced
|
🤖 The three suppressed findings are fixed: the profile is shell-quoted in the command doctor's pass message prints, and both the command's help and the skill now say what doctor actually promises — it writes nothing to the connector's ledger and posts nothing to Basecamp, while renewing the profile's own credential, which every command does when its token is due, may still write the credential store. |
There was a problem hiding this comment.
🟡 Changes recommended
Import validation and several operator-status messages can currently produce unsafe mutations or inaccurate results.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/commands/connect_operator.go:475
RerunSkippedalso coversLoadUndecidedfinding that the record is no longer blocked andCommitlosing the revision race to the running connector. In those cases this summary incorrectly says the event is still blocked and tells the operator to redispatch again even though it may already be admitted or otherwise decided. Use race-safe wording that asks the operator to check status before retrying.
internal/commands/connect_process_other.go:6- On non-Unix platforms this
falseis rendered as “no process has that pid” and serialized aspid_exists:false, even though the implementation explicitly cannot answer the question. Represent process existence as unknown (for example with a tri-state result) so status does not report stale lock metadata as proven dead on these platforms.
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Balanced
…check cannot answer Import checked only what ParseReconciliation had already checked, so a caller that built the value itself could tag every record under a version this build does not read. Status reports a lock file's pid as present, absent or unknown rather than calling it dead where the platform cannot say, and a redispatch whose prerequisite did not run here sends the operator to status rather than promising the record is still blocked.
There was a problem hiding this comment.
🟡 Changes recommended
The database hold invariant can be bypassed by inserting a tagged unauthorized event directly in an admitted or queued state.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Balanced
…s canceled The hold's refusal of a first hand-off reached the dispatcher as an error, so a hold arriving mid-task ended that task as failed; ExposeEvent reports it as held and the follow-up loop stops asking, so the task finishes and its sibling waits for a person.
There was a problem hiding this comment.
🟡 Changes recommended
Hold handling can incorrectly fail live tasks, and timestamp ties can suppress required follow-up decisions.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/connector/lifecycle.go:323
- This
>=timestamp comparison can treat the authorization that launched this attempt as a decision on the attempt's later failure. Ledger timestamps come directly fromtime.Nowand are not guaranteed to be unique or strictly increasing, so a fast launch/failure (or coarse clock) can giveauthorized_at == ended_at; the completion notice then suppresses the required new redispatch request. Track causal order with a decision/attempt identifier (or another strictly ordered ledger value) rather than accepting equality between wall-clock timestamps.
internal/connector/outbox_run.go:913 - The claim-time fast path repeats the non-causal
authorized_at >= ended_atcheck. If the authorization that launched a retry and that retry's end receive the same wall-clock timestamp, this treats the old authorization as a post-outcome decision and re-renders/cancels the notice without asking for the newly failed outcome. Use the same causally ordered decision/attempt marker as settlement rendering rather than timestamp equality.
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Balanced
The dispatcher now treats ExposeEvent's held refusal as no follow-up, so a hold that lands while a task runs lets that task finish and leaves its unexposed events for a person; a test drives it through the dispatcher. An authorization counts as a decision on an attempt only when it is stamped strictly after that attempt ended: a stamp equal to the end is not evidence it came after, and the notice asks again.
There was a problem hiding this comment.
🟡 Changes recommended
The missing-shadow promotion retry bypasses the required normal connector instance lock.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 1
- Review effort level: Balanced
Until the worktree driver lands, status cannot say whether a worktree is retained, and an operator must not read that as a clean slate.
There was a problem hiding this comment.
🟡 Changes recommended
An exposed but not yet pulled instruction can still reach a worker while the durable hold stands.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 1
- Review effort level: Balanced
An event the launch exposed carries only a pointer until a worker pulls its instruction, so a crash during launch and a restart under --hold could still let that worker fetch the instruction and start work a person had held. The database refuses a first pull now, and a repeat of one already pulled is answered; get_dispatch says the connector is held.
There was a problem hiding this comment.
🔵 Needs a closer look
The crash-sensitive ledger migration, SQL state invariants, and worker process termination require final human validation, and PR #739 must merge first.
Review details
- Files reviewed: 39/39 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The merge with connect-outbox brings in #736 as main squashed it, and three things it changed under this branch. migrationOperator is 9 now, behind the acknowledgement trigger main took as 6 and the tasks, attempts and outbox it pushed to 7 and 8. Close releases the ledger's registry entry, so a read-only open has to take one: it went through setup.CheckPrivateFile directly and built a Ledger with no entry at all, which panicked on Close and, worse, would have dropped the locks of any Ledger open beside it in the same process — the privacy check opens a descriptor and closes it. It claims the same per-file entry the writer's open claims. Superseding a task now retires its rows in the same statement, by trigger, so a redispatch that wrote superseded_at by hand retired an unexposed sibling without returning it: the record stayed dispatched on a dead task and held its conversation for good. The redispatch supersedes through supersedeTask, which returns what the task never exposed. The tests acknowledge and complete as a worker does, pulling the instruction first: exposure written at launch is not a pull.
|
🤖 Landed in #748, Combining them is what found the defects none of us could see alone — a |
Stacked on Post the connector's own lifecycle messages exactly once, or not at all and must not merge before it.
The connector can run an agent's work, but a person has no way to see what it did or to decide what it will not decide for itself. An event whose worker may have acted (
completed(unknown)) or failed waits for a redispatch nobody can issue; a blocked record cannot be retried or closed; and the cutover from the Ruby connector needs a shadow ledger promoted, and old entries reconciled, without a moment in which the new connector could dispatch something twice.Originally tracked in status, doctor, redispatch, discard, the hold marker and migration commands. Spec: "Commands", "Human decisions on records", "Hold and migration" in the Connector spec.
What changes:
basecamp connect statusreads the ledger read-only, without the instance lock, beside a running connector: hold, feed position (held or not, never the token), last poll-served id, gaps and losses, queue depths, live tasks, lifecycle messages waiting for a person, held records, the last 20 dispatches with outcomes.doctorchecks token, identity, ticket mint, feed poll, the ledger, the worker binary and an MCP handshake with the agent's server, started as a worker's is.redispatch <event_id>anddiscard <event_id>are a person's decisions, recorded with who made them.--holdon the run command sets a durable hold marker;releaseclears it.shadow promoteandimport <file>do the cutover.held: where a record tagged for review would have waited for a worker.Invariants
Held by the database where SQL can say it (triggers in the new migration), each with a test that was watched going red with the rule reverted:
release; held records stay held on release.shadow promotetakes both instance locks, commits the hold in the shadow ledger, folds it into one file and renames it into place: a crash at any point leaves the untouched shadow or a held ledger, and running it again finishes (tested by SIGKILLing a helper process at each step, including a shadow already held by--hold).importis one transaction: tombstones only for entries decided done, everything else tagged with its state and blocking reason kept, and a file with an entry it cannot apply changes nothing.One enforcement point
Redispatch has no lifecycle path of its own. Every state change it makes goes through the ledger's single state-changing write, and the edges out of a terminal record exist only against a decision row the database checks: the record must name the redispatch decision, it must be that record's, it must postdate the outcome it overrides, and the move consumes it. The replaced task's token is superseded in the same transaction, and the record is admitted only when that task ends. What redispatch adds on top is the authorization itself, which is the part the spec makes a person's.
Not here
statuswait on card 19's worktree ledger, which is a sibling of the PR below this one rather than under it. The seam is in place (connector.WorktreeLister, and status says "not tracked by this build" while it is nil); it is wired to the real rows — path, task, state and reason, size, and the line that says an orphan's record and branch are left as they are — in the rebase onto main after card 19 lands.Ledger.AuthorizedBlocked); todayredispatchruns the prerequisite itself and says so when it could not.