{{ message }}
Repository navigation
Conversation
The connector starts nothing since #814/#815: the session running the basecamp-connect skill hands each request to a subagent of its own. That dispatch is the one place a worker starts, so it is where a launcher goes. connect.json gains an optional "launcher", argv words set with repeated --launcher and removed with --no-launcher. With one, the skill runs each worker as `<launcher words> claude -p < prompt` from the repo it chose, instead of an in-process subagent. Words are never passed through a shell; the program is a name on PATH or an absolute path. Show prints it word by word, doctor checks the program can be found, and Parse refuses a launcher setup would refuse.
github-actions
Bot
added
commands
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/connect.go">
<violation number="1" location="internal/commands/connect.go:876">
P2: This accepts launcher words that the dispatch command cannot safely represent. Reject apostrophes in the shared launcher validation or execute the launcher with an argv-safe API instead of interpolating words into single-quoted Bash.</violation>
</file>
<file name="internal/connector/setup/file.go">
<violation number="1" location="internal/connector/setup/file.go:278">
P2: `Parse` can accept malformed launcher strings after `encoding/json` silently rewrites them, so the executed argv differs from the value in `connect.json` despite this validation. Reject decoder-lossy raw JSON strings before decoding, as admission does for legacy paths.</violation>
<violation number="2" location="internal/connector/setup/file.go:280">
P3: `ValidLauncher` rejects only Cc (control) characters, but the class the comment says must be refused — "a control character, which a terminal would act on when the launcher is shown" — also covers Unicode format characters (Cf): U+202E (right-to-left override), U+200B, U+FEFF. Those pass through the check, and show is safe (strconv.Quote/escapeControls escape non-printable runes) but `launcherCheck` is not: `ShellQuote` + `SanitizeSingleLine` leave Cf runes intact, so a hand-edited connect.json can put a bidi override into `connect doctor`'s "Workers run through …" message and render spoofed text in the terminal. Reject Cf runes as well, e.g. `unicode.IsControl(r) || unicode.Is(unicode.Cf, r)`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
P2: This accepts launcher words that the dispatch command cannot safely represent. Reject apostrophes in the shared launcher validation or execute the launcher with an argv-safe API instead of interpolating words into single-quoted Bash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/connect.go, line 876:
<comment>This accepts launcher words that the dispatch command cannot safely represent. Reject apostrophes in the shared launcher validation or execute the launcher with an argv-safe API instead of interpolating words into single-quoted Bash.</comment>
<file context>
@@ -839,6 +869,14 @@ func (f *connectSetupFlags) changes() (setup.Changes, error) {
+ if err := setup.ValidLauncher(f.launcher); err != nil {
+ return ch, output.ErrUsage("Invalid --launcher: " + err.Error())
+ }
+ ch.Launcher = slices.Clone(f.launcher)
+ }
+ ch.ClearLauncher = f.noLauncher
</file context>
| return fmt.Errorf("word %d is empty", i+1) | ||
| case len(w) > MaxLauncherWordBytes: | ||
| return fmt.Errorf("word %d is longer than %d bytes", i+1, MaxLauncherWordBytes) | ||
| case !utf8.ValidString(w): |
Contributor
There was a problem hiding this comment.
P2: Parse can accept malformed launcher strings after encoding/json silently rewrites them, so the executed argv differs from the value in connect.json despite this validation. Reject decoder-lossy raw JSON strings before decoding, as admission does for legacy paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/connector/setup/file.go, line 278:
<comment>`Parse` can accept malformed launcher strings after `encoding/json` silently rewrites them, so the executed argv differs from the value in `connect.json` despite this validation. Reject decoder-lossy raw JSON strings before decoding, as admission does for legacy paths.</comment>
<file context>
@@ -225,6 +238,56 @@ func (f File) Validate() error {
+ return fmt.Errorf("word %d is empty", i+1)
+ case len(w) > MaxLauncherWordBytes:
+ return fmt.Errorf("word %d is longer than %d bytes", i+1, MaxLauncherWordBytes)
+ case !utf8.ValidString(w):
+ return fmt.Errorf("word %d is not valid UTF-8", i+1)
+ case strings.IndexFunc(w, unicode.IsControl) >= 0:
</file context>
| return fmt.Errorf("word %d is longer than %d bytes", i+1, MaxLauncherWordBytes) | ||
| case !utf8.ValidString(w): | ||
| return fmt.Errorf("word %d is not valid UTF-8", i+1) | ||
| case strings.IndexFunc(w, unicode.IsControl) >= 0: |
Contributor
There was a problem hiding this comment.
P3: ValidLauncher rejects only Cc (control) characters, but the class the comment says must be refused — "a control character, which a terminal would act on when the launcher is shown" — also covers Unicode format characters (Cf): U+202E (right-to-left override), U+200B, U+FEFF. Those pass through the check, and show is safe (strconv.Quote/escapeControls escape non-printable runes) but launcherCheck is not: ShellQuote + SanitizeSingleLine leave Cf runes intact, so a hand-edited connect.json can put a bidi override into connect doctor's "Workers run through …" message and render spoofed text in the terminal. Reject Cf runes as well, e.g. unicode.IsControl(r) || unicode.Is(unicode.Cf, r).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/connector/setup/file.go, line 280:
<comment>`ValidLauncher` rejects only Cc (control) characters, but the class the comment says must be refused — "a control character, which a terminal would act on when the launcher is shown" — also covers Unicode format characters (Cf): U+202E (right-to-left override), U+200B, U+FEFF. Those pass through the check, and show is safe (strconv.Quote/escapeControls escape non-printable runes) but `launcherCheck` is not: `ShellQuote` + `SanitizeSingleLine` leave Cf runes intact, so a hand-edited connect.json can put a bidi override into `connect doctor`'s "Workers run through …" message and render spoofed text in the terminal. Reject Cf runes as well, e.g. `unicode.IsControl(r) || unicode.Is(unicode.Cf, r)`.</comment>
<file context>
@@ -225,6 +238,56 @@ func (f File) Validate() error {
+ return fmt.Errorf("word %d is longer than %d bytes", i+1, MaxLauncherWordBytes)
+ case !utf8.ValidString(w):
+ return fmt.Errorf("word %d is not valid UTF-8", i+1)
+ case strings.IndexFunc(w, unicode.IsControl) >= 0:
+ return fmt.Errorf("word %d holds a control character", i+1)
+ }
</file context>
Suggested change
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Jorge's note on the connector (10-01, "Fine tuning") asked for it to "eventually let you configure a sandbox launcher command". When he wrote it, the connector started its own workers. Since #814 and #815 it starts nothing:
basecamp connectonly listens and prints each trusted request, and the session running/basecamp-connecthands each request to a subagent of its own. So there is no spawn site in the connector for a launcher to wrap, and adding one back would bring back the worker side #815 removed.What's left is one place where a worker starts: the skill's dispatch step. That's where this PR puts the launcher.
Solution
launcher, a list of argv words. Only setup writes it:--launcheris given once per word, in order, and--no-launcherremoves it. The two flags can't be used together. Like every other setting, a setup run that doesn't mention it keeps it. Words are never passed through a shell. The first word must be a program name looked up on PATH or an absolute path. A relative path is refused, because the session runs the launcher from the repo it chose, so a relative path would name a program inside that repo. Empty words, control characters, a first word starting with-, more than 16 words, and words over 1 KiB are refused too.Parserefuses a hand-edited file with a launcher that setup would refuse.cd '<repo>' && '<word1>' '<word2>' claude -p < '<prompt file>'in the background. The launcher controls how the worker runs, including its permission mode. The session adds onlyclaude -p. If the launcher fails, the skill never drops it or falls back to a subagent.Launchercheck when one is set: whether the program can be found here. It doesn't start anything.How agent-sandbox plugs in
Each worker then runs as
sandbox claude -p < promptfrom the chosen repo. In agent-sandbox-nono, that means a box working in a snapshot of that repo, with the agent user's Basecamp profile brokered in, and the operator's Claude settings regenerated into the box (that work is in progress). The connector and the orchestrating session stay on the host. The connector needs the operator identity, tailscale and the agent's credential, none of which the box has. The orchestrator only acknowledges and picks a repo; the worker is what reads the thread and runs code, and that part is what goes in the box.Tests
go test ./internal/connector/setup/covers: round-trip, admission reads the same policy from a file with a launcher, no key written when there's no launcher, a validation table,Parserefusing bad launchers (empty list, relative path, a string instead of a list), andApplysetting, keeping, replacing, clearing and refusing launchers without modifying its input.go test ./internal/commands/covers: setup recording, keeping, replacing and removing a launcher against the existing fake setup server; refused launchers leave nothing written; the flags count as scriptable, so a launcher is never handed to guided setup; show's display; doctor's check passing and failing, with its hint.bin/ci(make check) passes locally. That's fmt, vet, golangci-lint (0 issues), unit and e2e tests, surface snapshot, skill drift, naming, provenance and tidy..surface-skill-driftgainsFLAG basecamp --launcher, becauseconnectis hidden and never appears in.surface, the same way--serveand--operator-profileare baselined.make -C skill-evals eval-connect), which need model calls. The new dispatch branch has no eval case yet.Open questions
Parserefuses unknown keys, so an older CLI refuses a connect.json that haslauncherand won't start the connector. It's the same for any new key. Do we want a version bump, or a note in the release?launcher, only the skill does. I put it here because setup is the only sanctioned writer and show is the safe reader. The alternative is an ordinary config key,connect.launcherthroughbasecamp config.show --jsonwhen it starts. Restarting the session after a change is enough, since that's already needed for trust changes.--launcher env) gets aclaude -pin default mode, which can't run tools without asking. That fails safe, but it isn't useful. Should setup refuse a known non-confining launcher, or should the skill pass a mode? I left that to the launcher on purpose.Summary by cubic
Lets
connect setuprun each Basecamp worker through a sandbox launcher. The session running the skill previously handed each request to an in-process subagent; with a launcher set, it instead starts<launcher words> claude -p < promptfile>from the chosen repo. Without a launcher, behavior is unchanged; with one, failures are reported to the person, never silently dropped.--launcher(repeated, one argv word each) records the command inconnect.json;--no-launcherremoves it. Both flags are scriptable, never guided.Parserefuse empty words, control characters, relative program paths, a program starting with-, more than 16 words, or words over 1 KiB; the program must be a name on PATH or an absolute path.showprints the launcher word by word;doctorchecks the program resolves without starting it. The connector itself never reads the setting.Written for commit 5702996. Summary will update on new commits.