The connector runs where it is started: no directory is a project's

[jorgemanrubia]

The connector decided where work happened. Each Basecamp project was mapped to a directory on this machine, and a worker was started there. The mapping never held anything in place — the worker runs with the operator's ambient authority — so what it bought was a configuration step, a resolution step, a repository check and a rule that only one task at a time could run in a directory. What it cost was the idea that the connector owns workspaces.

It does not. It runs where it is started, like the legacy connector did, and the agent decides the rest from its skills, its CLAUDE.md and its memory. If a task needs a clone or a directory of its own, the agent makes one. Sandboxing is being built separately, and that is where containment belongs.

Originally tracked in The connector runs where it is started: no routes to directories, no worktrees. This is the second and last half; the first was The connector runs where it is started: no worktrees.

Three other things an operator will notice

These came out of the review rounds below and are easy to miss under the route removal, so they are here rather than buried:

• The connector re-reads its served projects while it runs, instead of freezing them at startup. --serve and --unserve take effect without a restart, in admission as well as dispatch; before, admission decided against the file as it was when the process started, so a newly served project stayed blocked until a restart and an unserved one went on being admitted and then silently never started. Re-read, not locked: a reading is reused briefly and nothing holds the setup lock, so an unserve landing between a read and the write it authorizes is not caught — see the last section.
• An unreadable connect.json holds work rather than discarding it or claiming projects are unserved. A parse, permission or ownership failure used to read as "the operator serves nothing", which posted a holding reply saying the project is not served, logged that projects were no longer served, and could permanently discard a subscribed comment. Such records are now held as config_unreadable: nothing is posted, nothing is claimed about the project, and the work waits for basecamp connect redispatch <id> once the file is repaired — which is what every other blocked record does.
• --unserve of the last served project is now permitted. It writes an empty served list with a warning instead of refusing, which is how the agent is turned off through the interface that turned it on. A first setup still has to serve at least one project.

What is left of a route

The list of Basecamp projects the agent serves. That is the only local answer to "which projects may drive this agent". Project membership in Basecamp is an allowlist too, but anyone who can add members maintains it; connect.json is local and only the operator writes it. It matters most under trust mode project, where any non-client member of a served project can drive the agent, and it is belt and braces under the default. Each entry still carries class and watch_completions, neither of which is filesystem-shaped.

It has stopped being called a route, in the config, in the CLI and in the ledger. A route routes something to somewhere; with no somewhere, leaving the word in place is how the directory idea creeps back when someone reads it in six months and assumes there must be a destination. --route <id>=<dir> and --remove-route <id> are now --serve <id> and --unserve <id>.

What the removed check was also doing

The path requirement on a project entry was doing two jobs and looked like one. It required a path — and because a JSON null decodes into a struct as the zero value without an error, a null entry had no path and so was refused. Removing the path made "projects":{"123":null} a valid served project in both readers, so a half-edited file, a bad merge or a truncated write would have granted authorization where it used to withhold it. That is the inverse of what connect.json is for.

Project.UnmarshalJSON now refuses a null and decodes the object strictly — the second thing the outer decoder had been doing there, which a type's own unmarshaller does not inherit. Both readers are closed by the one change because both decode the same type.

The same check was guaranteeing the legacy path too, and that was a second hole one layer down: null, "", a relative or an unclean path were all swallowed on the way to being discarded, and the entry then served — so again, precisely the files older setup refused. A present path key is now held to the clean absolute string such a connector could only have written, before it is dropped; an absent one is a newer file and is left alone.

Both found in review, not by me, and the rule they leave is worth stating twice over: a field you are deleting is still load-bearing while you are reading it for compatibility, and a compatibility check validates what the old writer could produce, not what the current reader would accept. Discarding a value is not the same as not caring what it is; and the legacy path is checked in POSIX terms, because a Linux connector wrote it and this parser builds everywhere — checked with the reader's path rules it would have refused a legitimate file on Windows.

Where the remaining risk in this change lives: the legacy fields. They are validated against the format that wrote them, and four of the last five review findings were fail-opens or fail-closeds in that one small region — the place where the code is deliberately reading something it does not intend to honour, and so the place where "we still validate this" quietly stops being true. Every compatibility field on this branch was then checked against that question — LegacyPath was the gap; LegacyWorktrees loses nothing, because main's validation constrained it in no way and the decode still enforces the only guarantee it had (a test pins that decision rather than leaving it to be re-derived); the outbox's legacy key and reason compare values the ledger already holds, with nothing decoded.

The gap this leaves, on purpose

The connector's permission policy refused any edit resolving outside the record's working directory, symlinks followed, dangling ones too. Removing the route removes the only directory it had to name.

That bound is not repointed at the connector's own directory. A boundary that moves with wherever the operator happened to start the process looks like a guarantee and behaves like an accident, which is worse to reason about than none. It was policy and never containment — nothing in the kernel enforced it, so anything escaping the tool layer was already unstopped — and containment is what the sandbox launcher will give. Until it lands, a worker edits wherever the account can. Jorge's call, recorded on the card, and there is a test that asserts the absence so nobody later reads the removal as free.

What the deleted code had already written to disk

Three compatibility shims, each the same shape: a row is read as it was written, and new rows are written the new way.

A new migration renames the ledger's routed column to served — the value it always held, nothing about a filesystem — and drops the three columns that named a path, with the unique index over the last of them, which with every task in one directory would otherwise admit one live task on the whole machine. The two earlier migrations stay byte for byte.

connect.json keeps path on every project as a read-and-discard field. The parse refuses unknown keys, which is what makes a misspelled watch_completion a refusal rather than a silent nothing, and every file ever written has path on every project — deleting the field outright would make every connector already set up refuse to start.

The blocked reason keeps its stored spelling, no_route. It is written onto the record, and both the holding reply and the retraction that answers it read the record's reason back; a ledger in use carries rows and pending outbox intents with that value.

Each of the first two was proven load-bearing by reverting it and watching the test go red, then restoring.

What the cascade costs a person

Nothing holds a directory, so two conversations in one project now run side by side, bounded only by concurrency. A worker the connector cannot identify still keeps its attempt live and still takes its slot; what it no longer does is quarantine a directory. connect status drops the per-task directory column, because there is no per-task directory. connect doctor runs the ACP adapter's preflight once, in the directory doctor itself was started in, and names that directory — it used to run it per routed directory, and there is no longer a set to iterate.

One thing that does get worse: a project with a backlog could not previously fill more than one slot, because its records shared a directory. Now it can fill the window, and another project's work waits behind it. The card takes concurrency as explicitly out of scope — two workers in one directory is managed by the people running them until boxes are real — so this is accepted rather than fixed here.

After the first review

Copilot's six findings were one theme: the code stopped associating a directory with a project, and the things that describe it went on saying it did. All six held. The same shape turned up in five more places the review did not name — four in the one-owner rule in worker.go, one in drivertest's package doc — and in two justifications in the ACP driver for refusing a tool call whose paths it could not carry whole. The refusal stays; its reason is now that a call the driver cannot describe to the policy is one the policy is never shown, which fails closed, rather than a bound that no longer exists.

Two worth naming on their own. The worker's own prompt told it to "do the work in this directory" — the one place a model reads the claim, and the agent is the one deciding whether it needs a clone; that line is gone. And the skill told an agent to ask which local directory each project's work runs in and to check it exists, which would have stopped a first-time setup to collect input --serve cannot take.

The commit touching internal/connector/driver/codex/codex.go is comment-only and deliberately last on the branch: that file has a change in flight on another PR, so it can be dropped whole if the two collide.

The second review round: an authorization hole this change created

A conversation key is the recording's or the Campfire's, never the bucket's, so two records on one conversation can sit in two projects — a recording moved between them is the ordinary way. With the route gone, the follow-up query matched on the conversation and the record's stored served bit and nothing else, so a record from another project could join a live task and be exposed through a worker authorized against the originating project. The dispatcher's served-project and --project filters cover only the record a task starts from.

The stored served bit was not the authorization either: admission wrote it when it decided the record, so it says the project was served then. A project the operator stopped serving an hour ago still has records carrying it.

Both are now held in the ledger's own query rather than in the caller: a joined record must be in the task's own project, and that project must be among the ones connect.json serves right now, cut to the run's --project scope. The task's project is read from its originating record inside the same transaction rather than taken from the caller. Two tests, each proven red before the fix.

This is the seam the whole change turns on. The served list survives precisely because it is the local answer to which projects may drive this agent; a join that crossed projects would have made that answer leaky at the one place it exists to hold.

A check that measures nothing, and three that measured the wrong thing

The Skill Evals job is a no-op on this repo: ANTHROPIC_API_KEY is not configured, so the step warns and exits 0 without running a case. A green Skill Evals check on a PR that rewrites four eval cases has measured nothing, and no API key was available to run them by hand either.

Which is how three of them came to assert less than they read as asserting. --serve[ =]'?222\b matches before the =, so --serve 222=work satisfied the accept while the broad setup mock reported success — an eval green-lighting a command parsePositiveID refuses. That is worse than no eval, because it reports coverage that does not exist. The id now has to end the shell argument, verified both ways: the pattern refuses 222=work, 222=/home/me/x and 2223, still accepts --serve 222, --serve=222 and --serve '222', and the CLI really does refuse those pairs.

What was checked here without a model: the YAML parses, every accept, reject and mock pattern compiles and behaves on worked examples, and the mock bodies are the JSON the runner hands back. Someone with a key should still run make skill-eval before trusting the four cases — nothing in CI will.

The third review round

Two more, both verified before fixing.

The launch itself was not held to the served set. The join was, after the previous round, but LaunchTask still checked only the bit admission wrote when it decided the record, so the ledger would launch whatever it was handed. It now refuses a record its caller's served set does not cover, and a launch that names no served project authorizes nothing rather than defaulting open. (Written here at the time as closing the window between choosing a record and launching it. It does not: a later round made one pass hand its single reading down, so the check compares that reading against itself — belt and braces at the ledger's boundary, not a fix for the race. The race is described in the last section.)

Admission was deciding against the startup file. This one predates the change — the admitter has frozen its policy at construction all along — but the dispatcher's half is live now, and half a live answer is worse than none. Two wrong behaviours came out of it: after --unserve, admission went on admitting and the dispatcher then skipped, so the person who mentioned the agent got no work and no holding reply; after --serve, events stayed blocked until a restart, and the holding reply's own documented remedy could not work, because a redispatch re-runs admission against the same stale policy.

The served projects are now read at each decision, in the gate and the verdict alike, from the same reader the dispatcher uses — one reader, so the two halves cannot disagree. Only the projects: trust stays frozen, because who may drive the agent is a different kind of decision and should not become hot-reloadable as a side effect.

The unbounded edit policy was also flagged, and is not being changed. It is factually right that a task's content can now direct an edit anywhere the connector account can reach. That is the accepted cost, taken before this branch existed, and the alternative proposed — a run-wide boundary at the connector's start directory — was considered and rejected: it would move with wherever the operator happened to launch the process, reading as a guarantee while being an accident. The reasoning is on the review thread and in policy.go's own doc comment.

The fourth round: one question in three places

Making authorization live raised a question the frozen version never had to answer — which snapshot applies, and do they all agree? They did not.

An unreadable connect.json was telling people their project is not served. The reader returned the same empty map for "the operator serves no projects" and "the file could not be read", and admission consumes it now. So a parse, permission or read failure made every mention blocked(no_route) and posted the public holding reply, while the project was there and the file was the problem — and no_route has no timed retry, so repairing the file did not reconsider those records. Fail-closed is right; saying the wrong reason out loud is not. The failure now propagates separately: the record is held as config_unreadable, which posts nothing and claims nothing about the project. (The automatic retry described in the rounds below was later taken back out — see the last section; the record waits for a redispatch, as every blocked record does.) It is held rather than discarded at the gate too, because throwing work away over an unreadable file is the one outcome fixing the file cannot undo.

One verdict was being built from two configurations. Decide captured a snapshot, and match then read the served map again after the admission reads — long enough for a watch_completions turned on in between to admit an event that then carried the previous entry's class. Captured once and passed down now. That is the general rule the whole round comes down to: capture once, pass it explicitly, never re-read mid-decision.

Redispatch was authorized by the admission-time bit. Same shape as the launch race, one command over: a record in a project since unserved was admitted, the command reported success, and the dispatcher then refused it — so the person was told it worked while the record sat stranded with no holding reply. connect redispatch now reads the served set from connect.json when it runs.

Four rounds of findings, each one a consequence of the previous round's fix rather than a failure of it: closing the launch race exposed redispatch, and making admission live exposed both the snapshot split and the failure-mode conflation. That is what happens when something frozen is made honest.

The fifth round

There was no way to turn the agent off through the interface that turned it on. --unserve of the last served project produces a perfectly valid empty file, and the readiness check then failed the run — so the write never happened, and the only way to revoke the agent's last authorization was to edit or delete connect.json by hand. That is the trust anchor, and the file this whole change has been making the live, local authority, which makes the gap worse than incidental. Serving nothing was two questions answered by one rule: a first setup that serves nothing has not been set up and is still refused; withdrawing the last project from an existing setup is written, with a warning saying what it leaves behind.

The false claim had moved from the card to the log. Stopping an unreadable connect.json from telling people their project is unserved fixed the holding reply and not the stranded report, which went on counting every startable record and advising the operator to serve or discard projects that may be served already. The failure reaches dispatch now; everything that decides still fails closed, and the one consumer that speaks says what happened.

The consumers of the served set, enumerated rather than found one review at a time: admission (holds the record as config_unreadable); the dispatcher's servedBuckets, feeding the startable query, the launch, the join, authorized and the stranded report — four of which fail closed and want nothing else; and connect redispatch, which never had the collapse because an unreadable file is already a hard error before the ledger opens.

A failed reload is now cached for the TTL like a success, instead of re-reading the file once per event in admission and once per tick in dispatch.

Five rounds, and this is the first to surface something that was broken before this branch: the missing withdrawal path. That is usually the sign the review has reached the bottom of what the change itself introduced.

The sixth round

A fourth consumer of the served set, and my enumeration had missed it. I listed them from memory last round; re-derived from the code this time. nextFollowUp read the set twice and collapsed the failure both times, so an unreadable connect.json logged "the task's project is no longer served" — the same false claim as the holding reply and the stranded report, in a third place. dispatchReady had the same double read beside it, which the review had not named: it captured the set, then authorized() read it again, and the join got the first snapshot. authorized no longer reads at all — it takes the set its caller is holding. The other three sites (admission's policyNow, the reader itself, and connect redispatch) were fine, and are listed with verdicts in the commit message.

A shell injection in a line we tell an operator to paste — the Projects hint interpolated the profile name raw. Not reachable today, because Validate runs first and constrains the name; but that is a call order in another package, not a guard at the point of use. Quoted now, with the encoding hoisted into richtext.ShellQuote and tested by handing it to a real /bin/sh and checking the word that comes back is the word that went in.

The evals are held by a rule and a test, not by the next review. Four rounds of narrower patterns each found another spelling through, which is the sign the pattern was the wrong thing to fix. What they guarantee is now written down — every connect setup in the trace is a command the CLI would accept — expressed as a reject on the shape of a wrong --serve value rather than a list of wrong ones, plus an explicit reject of the removed flags. And TestConnectSkillEvalsRejectEverySetupCommandTheCLIWould holds that property against parsePositiveID itself, so a future edit cannot narrow it silently. It is a Go test because CI's Skill Evals job runs no case at all here.

The seventh round: reviewing the guard

All of it about the guard added last round, and the middle finding is the one worth keeping.

The guard was green on a rule that was wrong in both directions, because its corpus omitted the values that would have shown it. parsePositiveID refuses 0, 000 and the empty value, and accepts +222 and 007; the patterns let zero and empty through and rejected the leading plus. None of those five were in the corpus — so a check asserting "what the CLI accepts" was asserting it over inputs that could not disagree. That is the same defect the guard exists to catch, one level up: choosing the corpus is the check, not setup for the check.

All five are in it now, and the patterns are aligned — with one narrowing kept deliberately and named in the test rather than left implicit. A trace may carry digits above zero, bare or single-quoted; 007 is allowed because the CLI reads it as 7; --serve +222 would work and a trace still may not use it. The guard asserts both directions — everything the patterns allow, the CLI must accept; everything the CLI refuses, the patterns must reject — and asserts the plus case explicitly. Proven red: last round's patterns fail twelve assertions against this corpus.

The guard's name promised more than it checks, so it was narrowed rather than widened. It holds the reject patterns for the --serve value shape and the removed flags, and its doc comment says that accept, mocks, sequences and responses are not modeled and that a malformed one of those can still land, because the keyed job runs nothing here.

The shell-dependent tests are behind the unix tag — the repository builds for Windows, where /bin/sh is not there — and a doc comment was telling a copier the wrong escape: a curly quote instead of the POSIX '\'' splice the code emits.

The eighth round

The unknown-configuration hold sat after match, and match can end a verdict. comment.created admits under mentioned, which needs no served project, or subscribed, which does — so with the list unreadable the gate kept the first, dropped the second, did not discard, and match then returned not_addressed for a comment the agent is subscribed to. Terminally, over a broken file: exactly the outcome the hold exists to prevent. It now runs immediately after the gate, before any read and before anything below can end the verdict — which also stops the connector spending the account's API budget on events it cannot decide.

The stranded report claimed work was waiting without counting any — the false-claim class inside the fix for the false-claim class. It says only what is certain now: the configuration cannot be read, so dispatch is paused.

The skill still documented the rule that changed, telling an operator the last served project cannot be removed.

And the reason one wrong escape kept coming back: gofmt rewrites it. Four files documented the POSIX single-quote splice as a curly closing quote, and the fix made last round was itself reformatted back — gofmt normalizes doc-comment prose and turns that sequence into a typographic quote. It was never a typo. The splice now lives in an indented block in all four files, which gofmt leaves alone.

The ninth round, and where the eval guard stops

--serve '222'x is one word to /bin/sh — 222x — which the CLI refuses, and no pattern caught it. Fixed, along with the three other concatenation spellings, verified by running the shell rather than reasoning about it.

The more useful half is what a third round on the same corpus says: a regex over a command line cannot model shell word-splitting. Quoting, concatenation, expansion and substitution all change what the CLI is handed, and each has more spellings than a pattern can enumerate — adding cases was never going to converge. The guard's own documentation now says that: the corpus is a list of literal command lines, neither it nor the patterns model /bin/sh, and a sufficiently exotic command line can satisfy the patterns and still be refused by the CLI. That is a limitation of the approach, not a gap to be closed by adding cases, and it is written where the next person extending these patterns will read it.

What the guard does catch is named too, because that is what makes it worth keeping: a change to the patterns that makes them disagree with the parser on an ordinary spelling, and a narrowing added without being declared. Every narrowing is now a recorded decision — two of those concatenation spellings produce a value the CLI would accept, so rejecting them is the rule being deliberately narrower, and each corpus entry carries whether a trace may use it and why not. An undeclared narrowing fails the test.

The tenth round

Superseded in part: the retry sweep described here was removed in the last round below. The rest of this round stands.

"Retried on a timer" was a description of a function nothing called. NextBlockedRetry named the reasons that come round again, and outside tests nothing asked it — the only path that re-offers a blocked record is a person's redispatch. So an operator repaired connect.json and nothing happened, while the code, this description and the card all said the records would be reconsidered. The hold was chosen over a discard precisely because a discard cannot be undone by repairing the file, and then the recovery was not built.

It is built now: a ledger query for blocked records whose reason and timestamps make them due, offered by the intake sweep that already re-offers stranded records and open losses, bounded per tick. That makes the timer real for every reason the function names, not only the one this change added — a failed read and a throttle were waiting for a person just as much. A configuration failure also retries past the 24-hour transient window, which is right for a server that is not coming back but wrong for a local file an operator will fix next week. Covered end to end, because that is the only way to know a promise is kept: broken, held, repaired, run, with no redispatch in the test.

A test that could not fail on an authorization leak. Both failure cases in the connect.json reader asserted over a map from an earlier successful call, so they passed however much stale authorization a broken read handed back — the assertion-over-nothing shape in the place it would cost the most, since failing closed on an unreadable config is this change's security property. Each case now starts from a non-empty last-good state and asserts on its own call's map, verified by injecting the leak and watching both fail.

And --serve "222" was an undeclared narrowing — the ordinary double-quoted spelling, which the shell and the CLI both accept and the patterns rejected. The guard caught it as designed; the corpus had simply omitted it. Allowed now, with entries either side of the line.

The eleventh round

Superseded in part: the sweep these findings were about was removed in the last round below.

The retry could cause the permanent discard it exists to prevent. The sweep offered due records from every project, so a run narrowed by --project would hand admission one outside its scope; the gate ends that discarded(out_of_scope), which is terminal. A record that was only ever waiting, destroyed by the machinery built so that waiting records are not destroyed. The sweep now passes the run's buckets down.

The wider question that came with it was the better half. Every other terminal verdict a re-offered record can reach — stale, not_addressed, untrusted_performer, agent_authored, no_route for a trigger that needs a served project — is correct: each is the verdict a fresh event would get, and each reflects something somebody decided. out_of_scope is the exception because --project is one run's narrowing rather than a policy, and the projects it leaves out are another run's to dispatch. That rule is now in the sweep's own doc comment, not just in a review thread.

And the test for the wiring did not test the wiring. It called the query and the queue itself, so it stayed green with the periodic hook deleted — asserting the conclusion while supplying the mechanism. There are two tests now: one drives intake's sweep and covers the path (broken, held, not due, repaired, run), one drives the real ticker with nothing offering anything and covers the join. Deleting the hook fails the second and leaves the first green, which is why both exist.

Twice in this review the fix was right and its proof stopped short of what was broken. The check that catches it — if I delete the thing I just built, does this test still pass? — was run against every change in this round.

The twelfth round, and one thing taken back out

An automatic sweep that re-offered due blocked records was built over the previous two rounds, and it is removed again. What stays is the hold: an unreadable connect.json holds its records rather than discarding them or answering that the project is unserved, and a person's redispatch runs them. That is what those records did before the sweep existed and what every other blocked record does, so nothing regresses.

The finding that prompted it was real — a due row was offered without being claimed, and the queue does not deduplicate, so a backlogged admission could re-queue one batch while later rows starved. The fix was smaller than it reads: AcquireInstanceLock is a flock keyed on account and agent that the kernel drops on process death, so exactly one connector sweeps a ledger and an in-memory once-per-revision memo would do, with no durable lease.

It is out for a different reason. Wiring the retry turned on automatic re-decision for read_failed, read_unresolved, delta_unverified, trust_unverified and throttled — five reasons with nothing to do with removing directories, none reviewed as a feature, all arriving sideways through a review thread. Three rounds of real findings were the symptom; a scheduler entering through a review comment was the cause. It is carded with the sizing above and with the case for letting a configuration failure retry past the 24-hour transient window.

The claim went with the code, which is the part that mattered: an unbacked "retried on a timer" is what opened this thread. NextBlockedRetry is back to the reasons it had before, the blocked reason's doc comment says plainly that it waits for a person and that NextBlockedRetry describes a schedule nothing asks for, and the test that asserted the timer now asserts the opposite.

What is left open, named rather than implied

The served set is read, never locked. A reading is reused for a couple of seconds and no part of this holds the setup file's lock, so basecamp connect setup --unserve can complete between the read that authorizes an action and the write that performs it — at launch, and at connect redispatch.

The size of it: one task. A follow-up is stopped at the next tick by the task's own authorization check, and the record's project is re-read every pass. The race is pre-existing in kind — the route had exactly the same shape before this change, authorized at selection and not re-checked at commit — and nothing here makes it wider.

It is carded rather than closed here, because the fix is a setup-file lock held across selection, launch and redispatch, and that is a lock on the dispatcher's hot path: its own piece of work, with its own review. The comments at each site say which of the two things they are — a reading or a guarantee — so the next reader does not have to find out.

[Copilot]

🔵 Needs a closer look

It combines a durable schema migration with removal of a filesystem policy boundary and changed concurrency semantics.

Pull request overview

Completes the connector workspace simplification begun in #761: projects are now served without directory routes, and workers run from the connector’s startup directory.

Changes:

• Replaces route configuration with --serve and --unserve.
• Removes directory-based dispatch and permission constraints.
• Migrates stored ledger paths while preserving legacy compatibility.

[!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.

File summaries

File	Description
skills/basecamp/SKILL.md	Updates connector setup guidance.
skills/basecamp-connect/SKILL.md	Documents served-project behavior.
skill-evals/cases/basecamp-connect/unconfirmed-identity.yml	Updates identity setup evaluation.
skill-evals/cases/basecamp-connect/serve-by-id.yml	Tests serving projects by ID.
skill-evals/cases/basecamp-connect/not-ready-agent-reads.yml	Updates readiness evaluation.
skill-evals/cases/basecamp-connect/first-time-setup.yml	Updates initial setup evaluation.
internal/mcpserver/connect.go	Removes route terminology.
internal/connector/setup/report.go	Reports project counts.
internal/connector/setup/file.go	Removes project paths from configuration.
internal/connector/setup/file_test.go	Tests legacy path compatibility.
internal/connector/setup/checks.go	Checks served projects.
internal/connector/setup/checks_test.go	Updates project readiness tests.
internal/connector/setup/apply.go	Applies serve and unserve changes.
internal/connector/setup/apply_test.go	Tests served-project updates.
internal/connector/recovery_harness_test.go	Updates recovery harness terminology.
internal/connector/recovery_fakes_test.go	Updates recovery fixtures.
internal/connector/recovery_dispatch_test.go	Tests recovery without directory locks.
internal/connector/recovery_connector_test.go	Runs recovery workers from one directory.
internal/connector/recovery_acp_test.go	Uses unrestricted edit mode.
internal/connector/policy.go	Removes directory-bounded edit policy.
internal/connector/policy_test.go	Tests intentionally unbounded edits.
internal/connector/outbox.go	Updates holding-reply terminology.
internal/connector/outbox_run.go	Updates legacy comments.
internal/connector/outbox_fakes_test.go	Removes route data from fixtures.
internal/connector/operator_invariants_test.go	Removes route launch requirements.
internal/connector/lifecycle.go	Revises unserved-project replies.
internal/connector/ledger.go	Adds route-path removal migration.
internal/connector/ledger_tasks.go	Removes task route and work-directory state.
internal/connector/ledger_tasks_test.go	Tests conversation-only task locking.
internal/connector/ledger_status.go	Removes task working directories.
internal/connector/ledger_migrations_test.go	Tests migration compatibility.
internal/connector/ledger_hold.go	Updates redispatch prerequisites.
internal/connector/ledger_events.go	Renames routed state to served.
internal/connector/ledger_dispatch.go	Removes route wording from instructions.
internal/connector/ledger_decisions.go	Updates redispatch validation.
internal/connector/ledger_admission.go	Persists served state.
internal/connector/ledger_admission_test.go	Tests served-state persistence.
internal/connector/driver/driver.go	Defines directory-independent permission mode.
internal/connector/driver/codex/codex.go	Adopts the new Codex permission mode.
internal/connector/driver/codex/codex_test.go	Updates Codex policy tests.
internal/connector/driver/claude/claude.go	Adopts the new Claude permission mode.
internal/connector/driver/claude/claude_test.go	Updates Claude policy tests.
internal/connector/driver/acp/compat_test.go	Updates ACP compatibility policy.
internal/connector/driver/acp/adapters.go	Maps adapters to the new mode.
internal/connector/driver/acp/acp.go	Removes policy-directory validation.
internal/connector/driver/acp/acp_test.go	Updates ACP policy tests.
internal/connector/dispatcher.go	Dispatches served projects from one directory.
internal/connector/dispatcher_test.go	Tests new dispatch and concurrency behavior.
internal/connector/admission/verdict.go	Replaces route verdicts with served state.
internal/connector/admission/sdk_test.go	Renames served-project fixtures.
internal/connector/admission/run.go	Removes routes from event output.
internal/connector/admission/policy.go	Defines served-project configuration.
internal/connector/admission/policy_test.go	Tests served-project policy parsing.
internal/connector/admission/matrix.go	Renames route admission requirements.
internal/connector/admission/gate.go	Gates events on served projects.
internal/connector/admission/fakes_test.go	Updates admission fixtures.
internal/connector/admission/doc.go	Revises admission documentation.
internal/connector/admission/commit.go	Updates blocked-retry documentation.
internal/connector/admission/commit_test.go	Removes routes from admission output tests.
internal/commands/profile_layers_test.go	Uses served projects in setup tests.
internal/commands/mcp_connect_test.go	Verifies directory data is excluded.
internal/commands/connect.go	Replaces route setup flags and output.
internal/commands/connect_run.go	Reloads served-project authorization.
internal/commands/connect_run_test.go	Tests live served-project updates.
internal/commands/connect_operator.go	Removes directories from status output.
internal/commands/connect_operator_test.go	Tests doctor in its current directory.
internal/commands/connect_doctor.go	Runs one current-directory preflight.
.surface-breaking	Records removed route flags.
.surface	Records new serve flags.

Review details

• Files reviewed: 71/71 changed files
• Comments generated: 6
• Review effort level: Balanced

---

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

[Copilot]

🔵 Needs a closer look

The persistent schema migration, concurrency changes, and intentional removal of a permission boundary require final human validation.

Review details

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

[Copilot]

🔵 Needs a closer look

It combines a persistent ledger migration with security-sensitive permission changes and unresolved authorization concerns.

Review details

Suppressed comments (2)

skill-evals/cases/basecamp-connect/serve-by-id.yml:23

• This required pattern treats 222 as complete before punctuation, so a rejected command such as --serve 222=work still passes the eval and receives the generic successful setup response. Require the project ID to end the shell argument so this case verifies a command that parsePositiveID will accept.
  skills/basecamp/SKILL.md:1456
• The PR description opens with an unrelated rate-limiter/Retry-After change and claims server-side limiter tests, while this patch changes connector project serving and working-directory semantics. Replace that opening section so the stated purpose and testing evidence describe what this PR ships.

• Files reviewed: 75/75 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Launch authorization can race with project removal, and the default worker path loses directory containment.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

skill-evals/cases/basecamp-connect/unconfirmed-identity.yml:15

• The updated mock uses served projects, but the scenario still says the project's work lives in a local directory and that the directory exists. That lets this eval preserve the removed routing workflow while still passing. Remove the obsolete directory details from the task and context.
  internal/connector/driver/acp/acp.go:206
• This removes the directory-mismatch check, but the package invariant still says ACP rejects “a policy for another directory.” PermissionRules no longer carries a directory, so that refusal cannot occur. Remove the stale item from the invariant.

	if !ok {
		return nil, fmt.Errorf("%w: %w: %w: %s has no asking mode for policy mode %q", driver.ErrNotStarted, driver.ErrUnusable, driver.ErrUnsafeMode, d.opts.Adapter.Name, rules.Mode)
	}

skills/basecamp/SKILL.md:1456

• The opening PR description promises a rate-limiter/Retry-After error-message change and new resilience tests, but this diff instead changes connector project serving and working-directory behavior and contains no resilience changes. Remove the unrelated rate-limiter section so the description matches what this PR delivers.

• Files reviewed: 78/78 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Admission and redispatch can still use inconsistent served-project authorization state.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

skill-evals/cases/basecamp-connect/serve-by-id.yml:23

• This accept pattern treats the closing quote as sufficient, without requiring the shell argument to end. Malformed commands such as --serve '222'junk can therefore satisfy the eval and receive the broad setup mock's success response even though the CLI rejects the resulting ID. The same pattern appears in not-ready-agent-reads.yml:25 and first-time-setup.yml:39; in all three cases, match either a fully quoted or bare ID followed by whitespace/end, such as --serve[ =]('222'|222)(\s|$).
  internal/connector/driver/worker.go:33
• The one-owner rule header immediately above this hunk still says an attempt owns and releases a task's working directory, although these revised steps remove per-task directory ownership from the invariant. Update lines 24-26 to describe only the process tree and ledger record.

• Files reviewed: 78/78 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

Copilot review overview

🔵 Needs a closer look

Cross-cutting authorization, migration, and worker-policy changes require final human validation.

Review effort: Balanced
Findings: 2 · 1

Open (3)

• Add plain decimal overflow to reject-pattern corpus · New
• Synchronize broken flag access to prevent data race · New
• Update dispatchReady snapshot behavior test comment · New

Resolved since last review (1)

• Reject legacy project paths containing NUL bytes

[Copilot]

Copilot review overview

🔵 Needs a closer look

Authorization, migration, live-reload, and worker-policy changes require final human validation.

Review effort: Balanced
Findings: 2

Open (2)

• Qualify zero-project holding-reply messages · New
• Update comment to describe malformed-project validation · New

Resolved since last review (3)

• Synchronize broken flag access to prevent data race
• Add plain decimal overflow to reject-pattern corpus
• Update dispatchReady snapshot behavior test comment

[Copilot]

Copilot review overview

🟡 Changes recommended

The admission parser can bypass legacy-path validation for case-variant keys and authorize malformed configuration.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Reject noncanonical legacy path key casing in admission parsing · New

Resolved since last review (2)

• Update comment to describe malformed-project validation
• Qualify zero-project holding-reply messages

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes authorization, persistence migrations, live configuration, and worker permission boundaries across the connector.

Review effort: Balanced
Findings: 5

Open (5)

• Correct holding-reply behavior in help text · New
• Fix grammar and unmatched parenthesis in quote-splice explanation · New
• Complete the NUL-path error message · New
• Correct blanket holding-reply promise in table · New
• Clarify holding replies apply only to eligible mentions · New

Resolved since last review (1)

• Reject noncanonical legacy path key casing in admission parsing

[Copilot]

Copilot review overview

🔵 Needs a closer look

It combines authorization, trust-anchor parsing, schema migration, and intentionally weakened directory safeguards, requiring final human validation.

Review effort: Balanced
Findings: 1

Open (1)

• Avoid implying snapshot consistency across admission and dispatch · New

Resolved since last review (5)

• Clarify holding replies apply only to eligible mentions
• Correct blanket holding-reply promise in table
• Complete the NUL-path error message
• Fix grammar and unmatched parenthesis in quote-splice explanation
• Correct holding-reply behavior in help text

[Copilot]

Copilot review overview

🔵 Needs a closer look

It combines unresolved trust-anchor validation concerns with broad authorization, migration, and worker-policy changes.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Reject lossy malformed UTF-8 in legacy trust-anchor paths · New
• Reject --unserve when setting up a new project · New
• Reject --unserve in serve-only setup eval · New

Resolved since last review (1)

• Avoid implying snapshot consistency across admission and dispatch

[Copilot]

Copilot review overview

🔵 Needs a closer look

Cross-cutting authorization, migration, and worker-policy changes require human validation, and setup evals still permit wrong-project commands.

Review effort: Balanced
Findings: 1

Open (1)

• Reject setup commands serving the wrong project ID · New

Resolved since last review (3)

• Reject lossy malformed UTF-8 in legacy trust-anchor paths
• Reject --unserve in serve-only setup eval
• Reject --unserve when setting up a new project

[Copilot]

Copilot review overview

🔵 Needs a closer look

It combines trust-boundary changes, live authorization reloads, worker permission changes, and a destructive ledger migration requiring human validation.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• UnmarshalJSON bypasses canonical key validation · New
• Rejects valid leading-zero project ID 0222 · New

Resolved since last review (1)

• Reject setup commands serving the wrong project ID

[Copilot]

Copilot review overview

🔵 Needs a closer look

It combines authorization semantics, a persistent schema migration, compatibility parsing, and removal of a filesystem policy boundary.

Review effort: Balanced
Findings: 1

Open (1)

• Rejects valid leading-zero project ID 0222

Resolved since last review (1)

• UnmarshalJSON bypasses canonical key validation