Implement git-agent protocol for remote task execution - #46
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesGit-agent support now includes protocol handling, enrollment, SSH transport, dispatch, receive hooks, admission, verdicts, integration, remote sandbox execution, and scoped credential proxying. The CLI exposes Git-agent lifecycle and execution commands. Tests cover protocol, Git substrate, conformance, and end-to-end flows. Git-agent execution
CLI and sandbox integration
Sequence Diagram(s)sequenceDiagram
participant Prompt
participant RemoteSandbox
participant GitAgent
participant Agent
participant Mailbox
Prompt->>RemoteSandbox: prepare remote execution
RemoteSandbox->>GitAgent: dispatch task with policy and metadata
GitAgent->>Agent: create workspace and launch task
Agent->>Mailbox: push result and verdict
Mailbox-->>GitAgent: return tiered outcome
GitAgent-->>RemoteSandbox: return sandbox response
RemoteSandbox-->>Prompt: complete remote request
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Gavel summary
Totals: 0 passed · 0 failed · 0 skipped · - |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Gavel summary
Totals: 3133 passed · 26 failed · 8 skipped · 3m2s Failing testsgitagent — admission > agent branch pushes on the sidecar > requires a dispatched task and fast-forward updatesgitagent — admission > agent branch pushes on the sidecar > applies the content gates to agent workgitagent — admission > result admission on the mailbox > enforces parentage, agent namespace and attempt capsgitagent — admission > admits a well-formed dispatch pair on the sidecargitagent — admission > rejects deletes, updates to existing refs, and unpaired refs (R3.2/R3.4)... and 21 more failing tests — see the full gavel-results artifact. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
pkg/sandbox/adapter/gitagent.go-64-69 (1)
64-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAn empty user prompt drops the system prompt.
systemis assigned only inside thespec.Prompt.User != ""branch. If a spec carries a system prompt and an empty user prompt, the dispatch sends neither. The guard does not protect anything else, because both fields are plain strings.Assign both unconditionally.
🐛 Proposed fix
- prompt := "" - system := "" - if spec.Prompt.User != "" { - prompt = spec.Prompt.User - system = spec.Prompt.System - } + prompt := spec.Prompt.User + system := spec.Prompt.System🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sandbox/adapter/gitagent.go` around lines 64 - 69, Update the prompt initialization in the dispatch flow to assign both system and user values from spec.Prompt unconditionally, removing the spec.Prompt.User guard so a system-only prompt is preserved.pkg/cli/gitagent_e2e_test.go-438-452 (1)
438-452: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe workspace-discovery loop can fail after it finds the worktree.
The loop sets
worktreeinside the directory scan, and then always enters theselect. IfdispatchDonefires while theselectblocks, the test callst.Fatalf("dispatch exited before creating a workspace")even though the scan just found the workspace. The loop condition is checked only after theselectreturns.Break out as soon as the worktree is found.
🐛 Proposed fix
for time.Now().Before(deadline) && worktree == "" { entries, _ := os.ReadDir(tasksDir) for _, e := range entries { candidate := filepath.Join(tasksDir, e.Name(), "worktree") if info, err := os.Stat(candidate); err == nil && info.IsDir() { worktree, taskID = candidate, e.Name() } } + if worktree != "" { + break + } select { case err := <-dispatchDone: t.Fatalf("dispatch exited before creating a workspace (%v):\n%s", err, dispatchOut.String()) case <-time.After(250 * time.Millisecond): } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_e2e_test.go` around lines 438 - 452, Update the workspace-discovery loop around the worktree assignment to exit immediately once a valid worktree is found, before entering the dispatchDone/timer select. Preserve the existing failure handling and polling behavior when no workspace has been discovered.pkg/gitagent/enroll.go-197-199 (1)
197-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle a bracketed IPv6 host without a port.
For an endpoint such as
ssh://[::1],u.Hostis[::1].net.SplitHostPortfails on that value because it carries no port. The code then callsnet.JoinHostPort("[::1]", "22"), which adds a second pair of brackets and produces[[::1]]:22. That address never dials.Strip the brackets before joining.
🐛 Proposed fix
if _, _, splitErr := net.SplitHostPort(target); splitErr != nil { - target = net.JoinHostPort(target, "22") + host := strings.TrimSuffix(strings.TrimPrefix(target, "["), "]") + target = net.JoinHostPort(host, "22") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/enroll.go` around lines 197 - 199, Update the target normalization logic around net.SplitHostPort to remove one surrounding bracket pair from a bracketed IPv6 host before passing it to net.JoinHostPort, so an endpoint like [::1] becomes [::1]:22 rather than receiving nested brackets; preserve existing handling for unbracketed hosts and endpoints that already include a port.pkg/cli/gitagent_runtask.go-18-30 (1)
18-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove the doc comment onto the declaration it describes.
Lines 18-24 describe "The default coding agent a sidecar launches" and why
agentCommandis not left empty. The comment is attached toGitAgentRunTaskOptions, which is the flag struct. godoc will render this text as the documentation for that struct. The subject of the text isDefaultAgentCommandat line 134.Move the paragraph above
DefaultAgentCommandand giveGitAgentRunTaskOptionsits own comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_runtask.go` around lines 18 - 30, The doc comment currently attached to GitAgentRunTaskOptions describes DefaultAgentCommand instead. Move that explanatory paragraph above the DefaultAgentCommand declaration, and add a separate comment describing GitAgentRunTaskOptions as the sidecar run-task flag options.pkg/cli/gitagent.go-237-241 (1)
237-241: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the agent exists in the
revokedry run.The dry-run branch returns before any lookup. For an unknown agent,
revoke --dry-runprints "would remove agent ..." and reports success, but the realrevokereturnsagent %q is not enrolled in backend %q. A dry run must predict the real outcome.Load the config and check the enrollment before printing.
🐛 Proposed change
if opts.DryRun { + cfg, _, err := captainconfig.Load() + if err != nil { + return nil, err + } + backend, ok := cfg.Sandbox.Backends[opts.Backend] + if !ok { + return nil, fmt.Errorf("backend %q has no enrolled agents", opts.Backend) + } + agents, _ := backend.Options["agents"].(map[string]any) + if _, ok := agents[opts.Name].(map[string]any); !ok { + return nil, fmt.Errorf("agent %q is not enrolled in backend %q", opts.Name, opts.Backend) + } clicky.Printf("[dry-run] would remove agent %q from sandbox.backends.%s.agents in %s\n", opts.Name, opts.Backend, configPathForDisplay()) return GitAgentRevokeResult{Backend: opts.Backend, Agent: opts.Name, DryRun: true}, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent.go` around lines 237 - 241, Update the revoke dry-run branch in the relevant git-agent revoke flow to load the configuration and validate that opts.Name is enrolled in opts.Backend before printing or returning success. Preserve the existing “agent is not enrolled” error behavior for unknown agents, and only emit the dry-run removal message after validation.
🧹 Nitpick comments (33)
pkg/gitagent/proxy/proxy_test.go (3)
189-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe header slice is shared with the first proxy.
w.grant.Headersis a slice. TheGrantvalue stored in the first proxy'sGrantscopies the slice header, not the backing array. Line 191 therefore also changes the grant the proxy created innewWorlduses. The test passes today because it never sends a request through that proxy after the mutation, but the aliasing will make a future addition fail in a confusing way.Copy the grant before mutating it.
♻️ Proposed fix
- w := newWorld(t, secret) - w.grant.Headers[0].Value = types.EnvVar{} // nothing to resolve - p := &Proxy{Grants: []Grant{w.grant}} + w := newWorld(t, secret) + unresolvable := w.grant + unresolvable.Headers = []HeaderGrant{{Name: "Authorization"}} // nothing to resolve + p := &Proxy{Grants: []Grant{unresolvable}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/proxy/proxy_test.go` around lines 189 - 196, Copy w.grant into a separate grant before modifying its Headers slice, ensuring the mutation cannot affect the grant already stored in the proxy created by newWorld. Use the copied grant when constructing the broken Proxy in TestUnresolvableCredentialFailsTheRequest.
89-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd locked accessors for
decisionsandrequests.
upstreamHeaderValuestakesw.mu, but the tests readw.decisionsat Line 132 andw.requestsat Line 157 directly. Both slices are appended from the proxy handler goroutine and the upstream handler goroutine. The reads carry no lock, sogo test -racecan report a race onw.decisions, whose writes definitely happen on another goroutine.Add accessors that mirror
upstreamHeaderValues, and use them in the tests.♻️ Proposed accessors
func (w *world) auditDecisions() []Decision { w.mu.Lock() defer w.mu.Unlock() return append([]Decision(nil), w.decisions...) } func (w *world) upstreamRequestCount() int { w.mu.Lock() defer w.mu.Unlock() return len(w.requests) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/proxy/proxy_test.go` around lines 89 - 97, Add locked accessors on world mirroring upstreamHeaderValues: auditDecisions should return a copied decisions slice, and upstreamRequestCount should return the locked requests length. Replace direct reads of w.decisions and w.requests in the affected tests with these accessors to eliminate unsynchronized access while preserving assertions.
177-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a dot-segment case to the path-scope test.
TestScopeByMethodAndPathcovers a plainly out-of-scope path. It does not cover a path that passes the prefix check but resolves elsewhere at the upstream. That is the gap behind theAllowsPathfinding inpkg/gitagent/proxy/grants.go.💚 Proposed test
resp = w.do(t, "POST", w.upstream.URL+"/gists", nil, "") if resp.StatusCode != http.StatusForbidden { t.Fatalf("path outside scope: status = %d, want 403 (R9.6/H7)", resp.StatusCode) } + // A prefix match on an unnormalized path lets the upstream resolve a + // different target than the proxy authorized (R9.6/H7). + resp = w.do(t, "POST", w.upstream.URL+"/repos/acme/../gists", nil, "") + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("dot-segment escape: status = %d, want 403 (R9.6/H7)", resp.StatusCode) + }Build this request with
http.NewRequeston a raw URL so the client does not normalize the path before it reaches the proxy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/proxy/proxy_test.go` around lines 177 - 187, Extend TestScopeByMethodAndPath with a raw http.NewRequest case using a dot-segment path that passes the apparent prefix check but resolves outside the allowed upstream scope; send it through the existing proxy test helper and assert HTTP 403. Construct the request from the unnormalized URL so the client preserves the dot segment before proxy evaluation.pkg/gitagent/proxy/proxy.go (1)
64-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAudit the absolute-form rejection, and confirm first-match grant selection.
Two points on this block:
- The absolute-form rejection at Line 64 returns without calling
p.audit. Every other rejection path emits aDecision. R9.5 expects each decision to be recorded. Add an audit call so a malformed proxy request is visible.grantForreturns the first grant matching host, port, and scheme. If an operator configures two grants for the same destination with differentPathsorMethods, only the first is ever consulted, and requests valid under the second are rejected with "method or path outside the grant's scope". Confirm this is intended, or merge matching grants before evaluation.🛠️ Proposed fix for the missing audit
if !r.URL.IsAbs() { + p.audit(Decision{Method: r.Method, Destination: r.Host, Verdict: "rejected", Reason: "not an absolute-form proxy request"}) http.Error(w, "captain-proxy: absolute-form proxy requests only", http.StatusBadRequest) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/proxy/proxy.go` around lines 64 - 73, Audit the absolute-form rejection in the proxy handler before returning, recording a rejected Decision with the request method, available destination information, and a reason indicating the request was not absolute-form. Also update grantFor to evaluate all grants matching host, port, and scheme—or merge their Paths and Methods—so later matching grants can authorize requests instead of being ignored after the first match.pkg/api/sandbox_ginkgo_test.go (1)
117-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the predicate-false branch of the new verifiers.
This spec covers an adapter that does not implement
EgressProxied. The verifier inpkg/api/sandbox_registry.goat Lines 99-102 also requiresProvidesEgressProxy()to returntrue. An adapter that implements the interface and returnsfalsetakes a different branch, and no spec exercises it. The same gap applies toCapabilityIsolateWorkspace.Add a stub that implements
ProvidesEgressProxy() bool { return false }and assert the same error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/sandbox_ginkgo_test.go` around lines 117 - 129, Extend the verifier tests around the existing capability-rejection spec to cover adapters implementing the capability interface but returning false. Add a stub with ProvidesEgressProxy() returning false, register it through the same SandboxSRT path, and assert the existing egress-proxy error; add the equivalent predicate-false coverage for CapabilityIsolateWorkspace and its verifier.pkg/sandbox/adapter/gitagent.go (1)
198-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAn invalid
waitTimeoutfalls back silently.
WaitTimeoutignores a value that is not a string, does not parse, or is not positive. It then returnsDefaultWaitTimeout, one hour. An operator who writeswaitTimeout: "15min"gets a one-hour wait and no signal that the setting was discarded.Return the parse error to the caller, or log the discarded value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sandbox/adapter/gitagent.go` around lines 198 - 205, Update WaitTimeout to surface invalid waitTimeout values instead of silently returning DefaultWaitTimeout: either change the API to return the parsing/validation error to its caller or log the discarded value, covering non-string, unparsable, and non-positive inputs while preserving the default for absent settings.pkg/gitagent/control.go (1)
30-37: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReject
.and..as payload names.Line 32 rejects an empty name,
/, and NUL. It accepts.and... The error text states that the name must be a bare file name.git mktreeandgit fscktreat.and..as invalid tree entries, so the failure surfaces later with an unclear message.♻️ Proposed fix
- if name == "" || strings.ContainsAny(name, "/\x00") { + if name == "" || name == "." || name == ".." || strings.ContainsAny(name, "/\x00") { return "", fmt.Errorf("control payload name %q must be a bare file name", name) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/control.go` around lines 30 - 37, Update the payload-name validation loop to also reject the exact names "." and "..", alongside the existing empty, slash, and NUL checks. Keep the existing error response and sorting behavior unchanged.pkg/gitagent/receiver_ginkgo_test.go (1)
84-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert what the foreign-hook rejection leaves behind.
The case proves that
InstallHookShimsreturns an error whenpre-receiveis foreign. It does not assert the state after the failure.InstallHookShimsinstalls two hooks, so the failure can be partial:post-receivemay already carry the newcaptainBinwhilepre-receivekeeps the foreign content. A receiver with a mismatched hook pair is worse than one that was never touched.Add assertions for both hook files after the error.
♻️ Proposed test addition
err = gitagent.InstallHookShims(repo, "/opt/captain", "/home/agent/.captain.yaml", gitagent.RoleSidecar) Expect(err).To(MatchError(ContainSubstring("not installed by captain"))) + Expect(os.ReadFile(foreign)).To(Equal([]byte("#!/bin/sh\nexit 0\n")), "the foreign hook must be left untouched") + post, err := os.ReadFile(filepath.Join(repo, "hooks", "post-receive")) + Expect(err).NotTo(HaveOccurred()) + Expect(string(post)).To(ContainSubstring("/opt/captain"), "pin whether a partial install is the intended behaviour")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/receiver_ginkgo_test.go` around lines 84 - 89, Extend the foreign-hook rejection case after the InstallHookShims error assertion to inspect both pre-receive and post-receive hook files. Assert that the foreign pre-receive content remains unchanged and that post-receive was not installed or otherwise retains its prior state, proving InstallHookShims leaves the hook pair consistent after rejection.pkg/gitagent/envelope.go (2)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFour files declare a second package doc comment. Each of these files opens with a file-level note placed directly above
package gitagentwith no blank line between them. Go treats every such block as a package doc comment.pkg/gitagent/refs.goalready declares the canonical one, sogo docrenders a single arbitrary block and the other notes disappear from the rendered documentation. The fix at every site is the same: insert a blank line between the comment and thepackageclause.
pkg/gitagent/envelope.go#L1-L4: add a blank line after the push-option envelope note, beforepackage gitagent.pkg/gitagent/admit.go#L1-L4: add a blank line after the admission-tier note, beforepackage gitagent.pkg/gitagent/state.go#L1-L4: add a blank line after the task-state note, beforepackage gitagent.pkg/gitagent/receiver.go#L1-L5: add a blank line after the receiver-repository note, beforepackage gitagent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/envelope.go` around lines 1 - 4, Separate each file-level note from the package clause by inserting a blank line before package gitagent in pkg/gitagent/envelope.go, pkg/gitagent/admit.go, pkg/gitagent/state.go, and pkg/gitagent/receiver.go; no other changes are needed.
168-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParse
depthas strictly asattempt.
ParseAttemptrejects leading zeros and signs.strconv.Atoiaccepts"+2","-0", and"007", so several option strings decode to the same envelope.Validatestill bounds the range, so this is a canonical-form inconsistency, not a bypass. Reject non-canonical decimals here to keep one wire form per envelope.♻️ Proposed fix
case "depth": - n, err := strconv.Atoi(value) - if err != nil { + n, err := strconv.Atoi(value) + if err != nil || strconv.Itoa(n) != value { return fmt.Errorf("depth %q is not an integer", value) } e.Depth = n🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/envelope.go` around lines 168 - 179, Update the "depth" handling in the envelope parser to use the same strict canonical-decimal rules as ParseAttempt, rejecting signs and leading zeros while still accepting the valid zero representation. Preserve the existing integer error behavior and assignment to e.Depth for canonical values, and continue relying on Validate for range checks.pkg/gitagent/envelope_ginkgo_test.go (1)
106-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the push-option count bounds.
DecodeEnveloperejects more thanmaxPushOptionsoptions, andEnvelopeFromEnvrejects aGIT_PUSH_OPTION_COUNToutside[0,maxPushOptions]. Both guards bound work done for an untrusted pusher. Neither guard has a test. Add cases for a non-numeric count and for a count above the limit.♻️ Proposed test additions
It("names advertisePushOptions when the count is unset", func() { _, err := gitagent.EnvelopeFromEnv(func(string) string { return "" }) Expect(err).To(MatchError(ContainSubstring("advertisePushOptions"))) }) + + It("rejects a non-numeric or oversized option count", func() { + for _, count := range []string{"abc", "-1", "17"} { + _, err := gitagent.EnvelopeFromEnv(func(k string) string { + if k == "GIT_PUSH_OPTION_COUNT" { + return count + } + return "" + }) + Expect(err).To(MatchError(ContainSubstring("out of range")), "count %q", count) + } + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/envelope_ginkgo_test.go` around lines 106 - 123, Add tests in the “envelope from hook environment” Describe block covering a non-numeric GIT_PUSH_OPTION_COUNT and a numeric count greater than maxPushOptions. For each case, call gitagent.EnvelopeFromEnv with the relevant environment value and assert it returns an error, preserving the existing unset-count test.pkg/gitagent/admit_ginkgo_test.go (1)
224-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a mailbox push with an empty
Agent.The suite covers
Agent: "worker-1"andAgent: "worker-2". It never coversAgent: "". InadmitProtocolRefthe ownership check, the dispatch-existence check, and theMaxAttemptscheck are all skipped whenreq.Agentis empty. Add a case that pins the intended behavior for that input, so the decision stays explicit after a refactor. See the related comment onpkg/gitagent/admit.golines 128-144.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/admit_ginkgo_test.go` around lines 224 - 274, Add a mailbox admission case in the existing “result admission on the mailbox” test that submits the valid result updates with Agent set to an empty string. Assert the intended rejection behavior from admitProtocolRef, covering the empty-agent path without changing the existing worker ownership, parentage, or MaxAttempts cases.pkg/gitagent/refs_ginkgo_test.go (1)
16-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the accepted boundary values to the rejection suites.
Line 17 builds
"-" + string(make([]byte, 64)), which is a 65-byte string of NUL bytes after a hyphen. It proves NUL rejection, not length rejection. Thelongloop below covers length, so the intent of line 17 is unclear.The attempt suite rejects
"9999999"but never asserts that"999999"(MaxAttempt) is accepted. Add the accepted boundary values so a future change toattemptReortaskIDRefails the suite.♻️ Proposed boundary assertions
It("rejects everything outside ^[a-z0-9-]{1,64}$", func() { - for _, bad := range []string{"", "UPPER", "has space", "dot.dot", "a/b", "..", "-" + string(make([]byte, 64))} { + for _, bad := range []string{"", "UPPER", "has space", "dot.dot", "a/b", "..", "a\x00b"} { Expect(gitagent.ValidateTaskID(bad)).NotTo(Succeed(), "task id %q", bad) } long := "" for range 65 { long += "a" } Expect(gitagent.ValidateTaskID(long)).NotTo(Succeed()) + Expect(gitagent.ValidateTaskID(long[:64])).To(Succeed(), "64 characters is the accepted maximum") })It("rejects zero, leading zeros, negatives and junk", func() { for _, bad := range []string{"0", "01", "-1", "", "1x", "0x1", "1.0", "9999999"} { _, err := gitagent.ParseAttempt(bad) Expect(err).To(HaveOccurred(), "attempt %q", bad) } + n, err := gitagent.ParseAttempt("999999") + Expect(err).NotTo(HaveOccurred()) + Expect(n).To(Equal(gitagent.MaxAttempt)) })Also applies to: 38-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/refs_ginkgo_test.go` around lines 16 - 25, Update the validation tests to explicitly accept the maximum valid boundary values: `"999999"` for attempts and a 64-character valid task ID for task IDs. Replace the ambiguous NUL-containing rejection case in the task-ID suite with a clear invalid input if needed, while retaining the separate 65-character length rejection coverage; anchor changes in the existing task-ID and attempt validation suites..gitignore (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
hack/rule.Line 30 ignores the directory and Line 31 immediately un-ignores it, so the pair has no net effect.
hack/*on Line 32 already ignores the contents while letting git descend, which is what makes the Line 33 negation work. Removing the first two lines keeps the same behaviour and removes the apparent contradiction.♻️ Proposed change
# hack/ is local scratch space, except the git-agent substrate harness the # protocol tests rerun on every git version (SPEC-git-agent-protocol §1). -hack/ -!hack/ hack/* !hack/gitagent_empirical.sh🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore around lines 28 - 33, Remove the redundant hack/ ignore and un-ignore entries from .gitignore, while preserving hack/* and !hack/gitagent_empirical.sh so the directory remains traversable and only the protocol harness is tracked.pkg/gitagent/conformance_ginkgo_test.go (2)
60-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote
exeand addGinkgoHelper().Two points in
testSSHCommand.
exeis concatenated into aGIT_SSH_COMMANDvalue without quoting. Git runs that value through a shell, so a test binary path with a space breaks every push in this suite.installTestShimsat Line 41 already uses%qfor the same path.The function calls
Expectbut does not callGinkgoHelper(), so a failure reports this line instead of the caller.♻️ Proposed change
func testSSHCommand() string { + GinkgoHelper() exe, err := os.Executable() Expect(err).NotTo(HaveOccurred()) - return "env CAPTAIN_TEST_SSH_CLIENT=1 CAPTAIN_TEST_HOOK= " + exe + return fmt.Sprintf("env CAPTAIN_TEST_SSH_CLIENT=1 CAPTAIN_TEST_HOOK= %q", exe) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/conformance_ginkgo_test.go` around lines 60 - 64, Update testSSHCommand to call GinkgoHelper() before its assertions and quote exe when constructing the GIT_SSH_COMMAND value, matching the existing installTestShims quoting behavior so paths containing spaces work and failures report the caller.
265-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
resultand shorten the detached sleep.
resultis never read; Line 275 exists only to silence the compiler.w.workdiris set insidedispatchTask, so the return value is not needed.The agent command sleeps 30 seconds and the spec never terminates it. The process outlives the suite by up to 30 seconds on every run. The spec only needs the sleep to outlast the 15-second dispatch assertion and the 10-second
Eventually.♻️ Proposed change
- w := newConformanceWorld(ctx, nil, nil, "echo started > agent-marker.txt && sleep 30") + w := newConformanceWorld(ctx, nil, nil, "echo started > agent-marker.txt && sleep 20") start := time.Now() - result := w.dispatchTask(ctx) + w.dispatchTask(ctx) Expect(time.Since(start)).To(BeNumerically("<", 15*time.Second), "the dispatch push must not wait for the agent") marker := filepath.Join(w.workdir, "agent-marker.txt") Eventually(func() error { _, err := os.Stat(marker); return err }, "10s", "200ms").Should(Succeed(), "the detached agent runs on after the push returned") Expect(strings.TrimSpace(string(mustRead(marker)))).To(Equal("started")) - _ = result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/conformance_ginkgo_test.go` around lines 265 - 276, Remove the unused result assignment and the `_ = result` workaround from the dispatchTask test, calling w.dispatchTask(ctx) only for its side effects. Shorten the detached agent command’s sleep from 30 seconds to the minimum duration that still exceeds the 15-second dispatch assertion and 10-second Eventually window, while preserving the test’s verification that the agent continues running after dispatch returns.pkg/gitagent/snapshot.go (1)
283-288: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDerive the null OID from the repository object format.
zeroOIDis a 40-character SHA-1 null OID. In a SHA-256 repository every OID is 64 characters, soupdate-index --index-inforejects this deletion record andTakeSnapshotfails.basealready holds an OID of the repository's native width, so you can size the null OID from it.♻️ Proposed change: pass the base OID width into
indexEntry-func indexEntry(ctx context.Context, dir string, env []string, path string) (string, error) { +func indexEntry(ctx context.Context, dir string, env []string, base, path string) (string, error) { full := filepath.Join(dir, path) fi, err := os.Lstat(full) if errors.Is(err, fs.ErrNotExist) { - return "0 " + zeroOID + "\t" + path, nil + return "0 " + strings.Repeat("0", len(base)) + "\t" + path, nil }Update the call site in
buildSnapshotTree:line, err := indexEntry(ctx, dir, env, base, p)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/snapshot.go` around lines 283 - 288, Update indexEntry and its call site in buildSnapshotTree to accept the repository’s base OID (or its width), then generate the deletion record’s null OID using that native width instead of the fixed zeroOID value. Preserve the existing SHA-1 behavior while supporting SHA-256 repositories.pkg/gitagent/snapshot_ginkgo_test.go (1)
243-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the specific cap message.
Both assertions match
"cap", which the file-count message and the file-size message both contain. A regression that fires the wrong cap still passes. Match the distinguishing text instead.♻️ Proposed change
_, err := gitagent.TakeSnapshot(ctx, dir, gitagent.SnapshotPolicy{MaxFiles: 1}) - Expect(err).To(MatchError(ContainSubstring("cap"))) + Expect(err).To(MatchError(ContainSubstring("exceed the 1-file cap"))) _, err = gitagent.TakeSnapshot(ctx, dir, gitagent.SnapshotPolicy{MaxFileSize: 1}) - Expect(err).To(MatchError(ContainSubstring("cap"))) + Expect(err).To(MatchError(ContainSubstring("over the 1-byte cap")))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/snapshot_ginkgo_test.go` around lines 243 - 247, Update the two TakeSnapshot assertions in the cap-limit test to match the distinguishing file-count and file-size cap message text rather than the shared “cap” substring. Keep each assertion tied to its corresponding SnapshotPolicy limit so a wrong cap error fails the test.pkg/gitagent/dispatch.go (1)
159-164: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueWrite both mailbox refs in one transaction.
The two
update-refcalls are independent. If the control ref write fails, the dispatch ref stays behind without a matching control ref or task state. The push to the sidecar already uses--atomicfor the same pair, so the local record is the weaker half.
git update-ref --stdinapplies all commands in one transaction.♻️ Proposed change
- if _, err := runGit(ctx, req.MailboxPath, env, "update-ref", dispatchRef, snapshot.Commit); err != nil { - return err - } - if _, err := runGit(ctx, req.MailboxPath, env, "update-ref", controlRef, control); err != nil { - return err - } + txn := fmt.Sprintf("start\ncreate %s %s\ncreate %s %s\nprepare\ncommit\n", + dispatchRef, snapshot.Commit, controlRef, control) + if _, err := runGitIn(ctx, req.MailboxPath, env, strings.NewReader(txn), + "update-ref", "--stdin"); err != nil { + return err + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/dispatch.go` around lines 159 - 164, Replace the two independent update-ref calls in the dispatch flow with a single transactional git update-ref --stdin operation that updates both dispatchRef and controlRef together. Preserve the existing refs and values, and return the transaction error so neither local ref is changed if either update fails.hack/gitagent_empirical.sh (1)
94-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture the status before calling
assert.Shellcheck reports SC2319 at Lines 95, 98, 135, and 189.
$?currently carries the status of the preceding condition. The values are correct today, but any inserted line between the condition andassertsilently changes what is asserted. Assign the status first.♻️ Proposed pattern
-test -s "$out11/quarantine_path" -assert $? "1.1 GIT_QUARANTINE_PATH is set in the pre-receive environment" +test -s "$out11/quarantine_path"; rc=$? +assert "$rc" "1.1 GIT_QUARANTINE_PATH is set in the pre-receive environment"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/gitagent_empirical.sh` around lines 94 - 105, Update each affected test in hack/gitagent_empirical.sh, including the checks around quarantine_path, leak_rc, scrub_rc, and the corresponding later cases, to capture the condition’s exit status immediately in a local status variable before any intervening command, then pass that variable to assert. Preserve the existing assertions and expected statuses while eliminating reliance on a later `$?` value.Source: Linters/SAST tools
pkg/gitagent/enroll.go (1)
96-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompare the trimmed fingerprint.
Line 96 trims
hostFingerprintfor the emptiness test, but line 107 compares the untrimmed value. A fingerprint copied with a trailing newline or space fails the comparison. The error then prints two strings that look identical, which is hard to diagnose. The failure is closed, so this is a usability issue only.♻️ Proposed fix
func Enroll(ctx context.Context, endpoint, token, hostFingerprint string, signer gossh.Signer, req EnrollRequest) (*EnrollResponse, error) { - if strings.TrimSpace(hostFingerprint) == "" { + hostFingerprint = strings.TrimSpace(hostFingerprint) + if hostFingerprint == "" { return nil, fmt.Errorf("enrollment requires the supervisor's host-key fingerprint (printed by `captain sandbox git-agent add`)") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/enroll.go` around lines 96 - 111, Normalize hostFingerprint once by trimming surrounding whitespace after validating it is non-empty, then have the HostKeyCallback comparison and mismatch error in the enrollment flow use that normalized value.pkg/cli/gitagent_serve.go (1)
141-161: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA config write failure leaves a half-enrolled topology and a burned token.
gitagent.Enrollsucceeds first. The supervisor has already burned the join token and recorded this agent at that point. Ifcaptainconfig.Updatethen fails, the agent never authorizes the supervisor's dispatch key and never records the relay URL. The operator must mint a new token, because the old one cannot be replayed.Name that recovery step in the error, so the operator does not retry the same join command.
♻️ Proposed change
if err != nil { - return err + return fmt.Errorf("enrolled with the supervisor but could not record the result locally: %w\n"+ + "the join token is now burned; mint a new one with `captain sandbox git-agent add`", err) } clicky.Printf("enrolled as %s\n", resp.Agent)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_serve.go` around lines 141 - 161, Update the error handling after captainconfig.Update in the enrollment flow to include explicit recovery guidance when the config write fails: tell the operator that enrollment consumed the token and they must mint a new token before retrying. Preserve the original error while adding this context at the existing err return in the surrounding gitagent enrollment method.pkg/gitagent/server_ginkgo_test.go (1)
175-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
AdvertiseURLbranch of enrollment.The enrollment test supplies
ListenPortonly, so it exercises the derived-URL branch ofagentDispatchURL. TheAdvertiseURLbranch atpkg/gitagent/server.goline 177 returns the client value with no scheme check and is untested.Add a case that enrolls with an explicit
AdvertiseURL, and assert the recorded URL. If you apply the scheme restriction I proposed onpkg/gitagent/server.golines 133-143, add a companion case that asserts a non-ssh://value is refused.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/server_ginkgo_test.go` around lines 175 - 216, Extend the enrollment coverage in the test around the existing ListenPort flow to also submit an explicit AdvertiseURL and assert that the recorded enrollment URL matches it. If agentDispatchURL or the enrollment validation is updated with an ssh:// scheme restriction, add a companion assertion that a non-ssh URL is rejected.pkg/gitagent/keys.go (1)
19-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTwo processes can generate different keys for the same path.
EnsureKeyPairreads the path, and onENOENTgenerates and writes a new pair. Two processes that start at the same time both observeENOENTand both generate.writeFileAtomicmakes the last writer win. The losing process then returns a signer whose public half is no longer on disk.
RunGitAgentAddandRunGitAgentServeboth callEnsureKeyPairon the shared host key path from separate processes (seepkg/cli/gitagent.goline 129 andpkg/cli/gitagent_serve.goline 59). On a concurrent first start, the fingerprint printed for pinning would not match the key the server serves.Take a lock file around the generate path, or re-read the file after the atomic write and return the key that actually landed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/gitagent/keys.go` around lines 19 - 32, Make EnsureKeyPair coordinate concurrent first-time creation so callers return the key actually stored at path. Protect the generateKeyPair path with an appropriate lock file and, after acquiring the lock, re-check path before generating; if another process created it, parse and return that existing key instead. Preserve the current error handling and fingerprint behavior for existing keys.pkg/cli/gitagent_e2e_test.go (1)
44-56: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
freeLocalPortcan hand back a port another process takes.The helper binds port 0, reads the assigned port, and closes the listener before
servebinds it. Another process can claim the port in that window. Under parallel CI runs this produces a bind failure.The
servehelper reports the child's output on failure, so the flake is diagnosable rather than silent. Consider passing the open listener to the served process, or retrying the bind, if this test proves flaky.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_e2e_test.go` around lines 44 - 56, Update the freeLocalPort/serve test setup to avoid releasing the ephemeral listener before the server binds, preferably by passing the open net.Listener into serve and transferring ownership; otherwise add retry logic around the bind and retain the child-output diagnostics on failure.pkg/cli/gitagent_hook.go (2)
129-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject unknown fields when decoding a hook workflow.
json.Unmarshaldrops keys thatapi.Workflowdoes not declare. A misspelled key inhooks.sidecarorhooks.supervisortherefore produces a workflow that passesValidate()with the intended gate missing. These workflows are the receiver's verification gates, so a silently dropped gate means work is admitted unchecked.Use a decoder with
DisallowUnknownFields.♻️ Proposed change
var wf api.Workflow - if err := json.Unmarshal(data, &wf); err != nil { + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + if err := dec.Decode(&wf); err != nil { return nil, err }Add
"bytes"to the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_hook.go` around lines 129 - 139, Replace json.Unmarshal in the workflow decoding path with a bytes-backed JSON decoder configured with DisallowUnknownFields, while preserving the existing error propagation and subsequent wf.Validate() call.
104-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
agentKeyNameconstant instead of the literal.
pkg/cli/gitagent.goline 33 declaresagentKeyName = "agent_ed25519"in this same package, next tohostKeyNameanddispatchKeyName. Line 107 repeats the literal. The comment on that constant block states the reason it exists: enrollment and dispatch must agree on the key layout. A future rename of the constant would leave this relay pointing at a path with no key, and the failure would appear only when a relay push runs.♻️ Proposed change
rt.Relay = gitagent.RelayTarget{ URL: url, HostFingerprint: hostFP, - KeyPath: filepath.Join(keysDir, "agent_ed25519"), + KeyPath: filepath.Join(keysDir, agentKeyName), SSHCommand: exe + " sandbox git-agent ssh", }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_hook.go` around lines 104 - 109, Update the KeyPath assignment in the gitagent relay configuration to use the existing agentKeyName constant with keysDir instead of repeating the "agent_ed25519" literal, preserving the current path construction.pkg/cli/gitagent.go (1)
121-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a result struct from the
adddry run.
RunGitAgentAddreturns(nil, nil)for a dry run.RunGitAgentRevokereturnsGitAgentRevokeResult{DryRun: true}for the same case, and its doc comment states the reason:--format jsonmust emit a well-formed document rather than prose. With the current code,captain sandbox git-agent add --dry-run --format jsonemitsnull.Add a
DryRunfield toGitAgentAddResultand return a populated value.♻️ Proposed change
if opts.DryRun { clicky.Printf("[dry-run] would ensure host key at %s\n", hostKeyPath) clicky.Printf("[dry-run] would ensure dispatch key at %s\n", dispatchKeyPath) clicky.Printf("[dry-run] would mint a single-use join token (TTL %s) for agent %q\n", gitagent.JoinTokenTTL, opts.Name) clicky.Printf("[dry-run] would record the pending enrollment under sandbox.backends.%s in %s\n", opts.Backend, configPathForDisplay()) clicky.Printf("[dry-run] would print the join command for endpoint %s\n", endpoint) - return nil, nil + return GitAgentAddResult{Backend: opts.Backend, Agent: opts.Name, DryRun: true}, nil }type GitAgentAddResult struct { Backend string `json:"backend" pretty:"label=Backend"` Agent string `json:"agent" pretty:"label=Agent"` Expires time.Time `json:"expires" pretty:"label=Token expires"` HostFingerprint string `json:"hostFingerprint" pretty:"label=Host key"` DispatchKey string `json:"dispatchKey" pretty:"label=Dispatch key"` JoinCommand string `json:"joinCommand" pretty:"label=Join command"` + DryRun bool `json:"dryRun,omitempty" pretty:"label=Dry Run"` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent.go` around lines 121 - 128, Update GitAgentAddResult to include a DryRun field, and change the dry-run branch of RunGitAgentAdd to return a populated GitAgentAddResult with DryRun set to true instead of returning nil. Preserve the existing dry-run messages and normal add behavior.pkg/cli/gitagent_test.go (1)
97-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the
RecordAgentrejection paths.
RecordAgentrefuses an enrollment with an emptyURLor an emptyHostFingerprint(pkg/cli/gitagent_directory.golines 91-96). No test exercises either refusal. Those two guards protect the invariant that an enrollment must not look complete while being undispatchable, so a regression there would be silent.Add table cases that assert each refusal, and extend them when the
Fingerprintguard is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_test.go` around lines 97 - 108, Extend TestGitAgentEnrollListRevoke with table-driven cases covering RecordAgent rejection when URL is empty and when HostFingerprint is empty, asserting both return errors. Keep the existing valid enrollment test, and include the empty-Fingerprint rejection case when the corresponding guard is added.pkg/cli/ai_sandbox_remote_test.go (1)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a malformed
waitTimeout.The subtest covers a parseable
"15m". It does not cover an unparseable value such as"fifteen". IfremoteAwareTimeoutreturns0for a malformed value,runContextapplies its 120-second fallback (pkg/cli/ai.golines 367-369). That produces exactly the failure this file documents at lines 46-48: a dispatch that is still progressing is killed and reported ascontext deadline exceeded.Add a case that asserts the fallback is the remote wait budget, not zero.
♻️ Proposed case
+ t.Run("a malformed waitTimeout falls back to the remote budget", func(t *testing.T) { + broken := ai.Config{SandboxSelection: &api.SandboxConfig{ + Kind: registry.SandboxGitAgent, + Options: map[string]any{"waitTimeout": "fifteen"}, + }} + if got := remoteAwareTimeout(ai.Request{}, broken, requestDefault); got != adapter.DefaultWaitTimeout { + t.Fatalf("timeout = %s, want the remote wait budget %s", got, adapter.DefaultWaitTimeout) + } + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/ai_sandbox_remote_test.go` around lines 61 - 69, Add a subtest alongside “the backend's waitTimeout is honoured” that supplies a malformed waitTimeout such as “fifteen” and asserts remoteAwareTimeout returns the remote wait-budget fallback rather than zero. Reuse the existing requestDefault or established remote timeout value so the test documents the behavior used by runContext.pkg/captainconfig/config.go (1)
239-251: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument or guard the concurrency constraint on the exported
pathOverride.
SetPathwrites an unsynchronized package global thatPath()reads at line 257. While the only writer wasSetPathForTesting, sequential test use made this safe.SetPathis now public and called from two CLI entrypoints (pkg/cli/gitagent_hook.goline 29 andpkg/cli/gitagent_runtask.goline 38).Both current callers run before any goroutine starts, so no race exists today. The exported surface carries no stated constraint, and
captain serveexecutes prompt runs concurrently in task groups, each reachingPath()throughloadSavedConfig. A future call toSetPathafter those goroutines start is a data race that-racewill report.State the constraint in the doc comment, or make the value atomic.
♻️ Option 1: document the constraint
// SetPath redirects Path() to an explicit config file. It exists for // processes that cannot rely on $HOME: a git receive hook runs as a child of // whoever pushed, so its ambient home is the pusher's, not the one the // receiver was configured under. +// +// Call it during process start, before any goroutine can reach Path(). +// It writes an unsynchronized package global. func SetPath(p string) { pathOverride = p }♻️ Option 2: make the value atomic
-var pathOverride string +var pathOverride atomic.Pointer[string] -func SetPath(p string) { pathOverride = p } +func SetPath(p string) { pathOverride.Store(&p) }
Path()then reads throughpathOverride.Load()and treats a nil pointer or an empty string as "use os.UserHomeDir". Add"sync/atomic"to the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/captainconfig/config.go` around lines 239 - 251, Document the concurrency constraint for the exported SetPath API and its backing pathOverride: callers must invoke SetPath only before concurrent use begins, because Path reads the unsynchronized value. Update the SetPath comment to state that it is not safe for concurrent calls with Path or other SetPath calls; leave the existing behavior unchanged.pkg/cli/gitagent_directory.go (2)
118-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject an existing backend whose kind is not git-agent.
ensureGitAgentBackendsetsKindonly when it creates the backend. If the named backend already exists with another kind, the function returns it unchanged, andRunGitAgentAddthen writespendinganddispatchKeyinto it. The command reports success, but no git-agent serve path will ever read that backend.Return an error when the existing backend declares a different kind.
♻️ Proposed change
-func ensureGitAgentBackend(cfg *captainconfig.Config, name string) captainconfig.SandboxBackend { +func ensureGitAgentBackend(cfg *captainconfig.Config, name string) (captainconfig.SandboxBackend, error) { if cfg.Sandbox.Backends == nil { cfg.Sandbox.Backends = map[string]captainconfig.SandboxBackend{} } backend, ok := cfg.Sandbox.Backends[name] if !ok { backend = captainconfig.SandboxBackend{Kind: string(registry.SandboxGitAgent)} + } else if backend.Kind != "" && backend.Kind != string(registry.SandboxGitAgent) { + return backend, fmt.Errorf("backend %q is kind %q, not %s", name, backend.Kind, registry.SandboxGitAgent) } if backend.Options == nil { backend.Options = map[string]any{} } cfg.Sandbox.Backends[name] = backend - return backend + return backend, nil }The three call sites (
RunGitAgentAdd,RecordAgent, andpkg/cli/gitagent_test.goline 74) need the extra return value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_directory.go` around lines 118 - 131, Update ensureGitAgentBackend to return an error alongside the backend, and reject existing entries whose Kind differs from registry.SandboxGitAgent before modifying their options. Propagate the new error return through RunGitAgentAdd, RecordAgent, and the gitagent test call site, preserving normal initialization for missing or correctly typed backends.
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport a malformed expiry separately from an expired one.
time.Parsefailure and a genuine expiry share one message. For a corruptexpiresvalue the operator is told the token expired, which points at the wrong cause. The burn already happened above, so the entry is gone either way.Split the two cases.
♻️ Proposed change
expires, _ := entry["expires"].(string) - if t, err := time.Parse(time.RFC3339, expires); err != nil || time.Now().After(t) { + t, err := time.Parse(time.RFC3339, expires) + if err != nil { + refusal = fmt.Errorf("join token has a malformed expiry %q; mint a new one with `captain sandbox git-agent add`", expires) + return nil + } + if time.Now().After(t) { refusal = fmt.Errorf("join token has expired; mint a new one with `captain sandbox git-agent add`") return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gitagent_directory.go` around lines 66 - 70, Update the expiry validation in the git-agent join-token flow to handle time.Parse failure and a successfully parsed past timestamp separately. Keep the existing expired-token refusal for valid timestamps that are after their expiry, but return a distinct refusal message for malformed expires values; preserve the current token-burn and return behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cli/ai_prompt_file_test.go (1)
216-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert preservation of
Paths.The input policy contains
Paths: []string{"pkg/**"}. The test only checksMaxAttemptsand pointer identity. A replacement policy can discardPaths, assign that replacement to both values, and still pass.Proposed test update
-if req.Sandbox.Policy == nil || req.Sandbox.Policy.MaxAttempts != 3 { +if req.Sandbox.Policy == nil || + req.Sandbox.Policy.MaxAttempts != 3 || + len(req.Sandbox.Policy.Paths) != 1 || + req.Sandbox.Policy.Paths[0] != "pkg/**" { t.Fatalf("request policy = %#v", req.Sandbox.Policy) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/ai_prompt_file_test.go` around lines 216 - 220, The test does not verify that the sandbox policy preserves its Paths field. Update the assertions around req.Sandbox.Policy and cfg.SandboxSelection.Policy to require Paths contains the original []string{"pkg/**"} value, while retaining the existing MaxAttempts and pointer-identity checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gitagent/hookmain.go`:
- Around line 204-210: Update the UpdateTaskState callback so current.Attempts =
attempt is assigned and persisted by returning true even when attempt exceeds
current.Policy.MaxAttempts; perform the over-limit rejection after recording the
increment, preserving the existing acceptance behavior for attempts within the
limit.
In `@pkg/gitagent/proxy/proxy.go`:
- Around line 86-99: Before calling p.substitute in the proxy authorization
flow, reject grants with non-empty grant.Headers when grant.scheme() is not
"https"; audit the rejection and return HTTP 403 without contacting the
upstream. Add a regression test covering an HTTP grant with a resolved
Authorization placeholder and verify no upstream request occurs.
---
Nitpick comments:
In `@pkg/cli/ai_prompt_file_test.go`:
- Around line 216-220: The test does not verify that the sandbox policy
preserves its Paths field. Update the assertions around req.Sandbox.Policy and
cfg.SandboxSelection.Policy to require Paths contains the original
[]string{"pkg/**"} value, while retaining the existing MaxAttempts and
pointer-identity checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5094f099-2a12-4b7d-b7a6-32bede219f4a
📒 Files selected for processing (36)
pkg/captainconfig/config.gopkg/cli/ai.gopkg/cli/ai_prompt_file.gopkg/cli/ai_prompt_file_test.gopkg/cli/ai_sandbox_remote.gopkg/cli/ai_sandbox_remote_test.gopkg/cli/gitagent.gopkg/cli/gitagent_directory.gopkg/cli/gitagent_e2e_test.gopkg/cli/gitagent_hook.gopkg/cli/gitagent_runtask.gopkg/cli/gitagent_runtask_test.gopkg/cli/gitagent_serve.gopkg/cli/gitagent_test.gopkg/gitagent/admit.gopkg/gitagent/conformance_ginkgo_test.gopkg/gitagent/control.gopkg/gitagent/dispatch.gopkg/gitagent/enroll.gopkg/gitagent/envelope.gopkg/gitagent/git.gopkg/gitagent/hookmain.gopkg/gitagent/hookset_ginkgo_test.gopkg/gitagent/keys.gopkg/gitagent/materialize.gopkg/gitagent/proxy/grants.gopkg/gitagent/proxy/proxy.gopkg/gitagent/proxy/proxy_test.gopkg/gitagent/receiver.gopkg/gitagent/regression_test.gopkg/gitagent/snapshot.gopkg/gitagent/sshclient.gopkg/gitagent/state.gopkg/gitagent/workspace.gopkg/sandbox/adapter/gitagent.gopkg/sandbox/adapter/gitagent_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/captainconfig/config.go
- pkg/cli/ai_prompt_file.go
- pkg/gitagent/git.go
- pkg/gitagent/control.go
- pkg/cli/ai.go
- pkg/gitagent/envelope.go
- pkg/cli/gitagent_runtask.go
- pkg/gitagent/workspace.go
- pkg/cli/ai_sandbox_remote.go
- pkg/gitagent/snapshot.go
- pkg/sandbox/adapter/gitagent.go
- pkg/gitagent/admit.go
- pkg/cli/gitagent.go
- pkg/gitagent/enroll.go
- pkg/cli/gitagent_directory.go
- pkg/gitagent/proxy/grants.go
- pkg/gitagent/hookset_ginkgo_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gitagent/relay.go`:
- Around line 47-60: Bound the accumulated data in relayFeedbackWriter.Write
before appending to w.pending, so an unterminated stream cannot grow memory
without limit. Enforce a fixed byte limit, forward or discard only bounded
content once reached, and emit exactly one fixed truncation message; preserve
normal newline-delimited handling through writeLine and avoid repeating the
truncation notice.
- Around line 75-79: Update the captain-json parsing logic in the relay verdict
handling to store the parsed TierVerdict only when it is a supervisor verdict
bound to the active envelope. Validate the verdict’s envelope identity against
the current envelope before assigning w.verdict, while preserving the existing
behavior for unrelated or invalid JSON.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef1ac2dd-ed10-470a-acbd-e2eb02b7c7d4
📒 Files selected for processing (3)
pkg/gitagent/conformance_ginkgo_test.gopkg/gitagent/hookmain.gopkg/gitagent/relay.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/gitagent/conformance_ginkgo_test.go
- pkg/gitagent/hookmain.go
…core
Port the SPEC-git-agent-protocol §1 verification harness as
hack/gitagent_empirical.sh: quarantine env leakage and the
--local-env-vars omission of GIT_QUARANTINE_PATH (R1.1), push-option
survival and the advertisePushOptions default (R1.2), quarantined-tree
materialization and the relative work-tree trap (R1.3/H18), and the
no-copy relay (R1.4). A colocated test reruns it against the installed
git so a git upgrade that shifts any of these behaviours fails loudly.
On git 2.43.0 the relative-work-tree probe materializes instead of
silently no-opping; the harness records the divergence and the
implementation absolutizes regardless.
pkg/gitagent gains the pure-data core: ref naming and parsing for
refs/captain/tasks/<task>/{dispatch,control,result,verdict}/<attempt>
and the agent branch, task-id and attempt validation, separator-aware
namespace containment (R8.3/H11), and the push-option control envelope
with strict decode and envelope↔ref agreement checks (R4.1).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
…first TakeSnapshot captures the supervisor's dirty worktree as a commit parented on HEAD, built against a throwaway index: read-tree of the base, then update-index --index-info records for exactly the dirty paths — never read-tree + add --all, which stages mass deletions under sparse-checkout and skip-worktree (R6.1/H4, covered by a sparse fixture test). Blobs are hashed with --no-filters and every git call pins core.autocrlf=false, core.eol=lf and core.attributesFile=/dev/null, so the bytes committed are the bytes on disk even under a hostile text attribute (R6.2). Dispatch refuses loudly — never degrades — on LFS-filtered paths, required clean/smudge filters, dirty submodules, unmerged entries and policy caps (H5), and policy path globs bound what a snapshot may carry. The A4.3 fidelity fixture (modifications, staged and unstaged deletions, a rename, exec bits, symlinks, nested/odd-named untracked files, CRLF bytes) gates the snapshot, and a companion audit pins commons-db's Checkout.Dirty behaviour: deletions, renames, exec bits, symlinks and untracked files round-trip; the skip-worktree silent drop is asserted as a documented upstream gap so a commons-db release that fixes it fails the audit and triggers a re-review (A4.2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
InitMailbox/InitSidecar create the two receiving repos with the mandated R2.2 config — advertisePushOptions, fsckObjects, denyDeletes, autogc off, full ref logging and a finite maxInputSize — and the mailbox shares the real repository's object store via objects/info/alternates so protocol refs never touch the user's working repo (R2.1/H8). Hook shims exec the captain binary with stdin and env flowing through, refuse to clobber foreign hooks, and re-install idempotently. Admit is the sub-second pre-receive tier (§6.2 step 7): protocol ref shape, create-only with deletes and updates refused (R3.2), the dispatch+control / result+control atomic pairing (R3.4), envelope↔ref agreement (R4.1), separator-aware namespace and per-agent task ownership (R8.3/H11), attempt caps, fast-forward-only agent branches, and content gates that need no materialized tree: policy path globs, secret-shaped names via the exported commit.LooksSecret (A5.4), and blob size caps read from object metadata. Control payloads travel as parentless commits, never bare trees (R3.3). Receiver-side task state lives under <repo>/captain/, outside the object store, so it survives a rejected push (R6.9 groundwork). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
…edback Materialize runs read-tree + checkout-index against an absolutized destination — never git archive (R6.5/H9) — and asserts the tree is non-empty with a file count matching ls-tree before any hook may see it (R1.3/H18): exit 0 from checkout-index is not proof of materialization. A .git path component refuses to materialize as defense in depth behind receive.fsckObjects. RunHookSet executes an api.Workflow as one tier's single-verdict chain over the materialized tree, reusing the exact builders the local run path uses (A5.1): commit gates, then CmdVerifier exec hooks, then LLM-judge prompt hooks, stopping at the first failure (R5.1). Exec hooks gain the confinement seam — CmdVerifier now carries Env and a Wrap func mirroring api.CommandWrapper — and a hook set with exec hooks but no wrap refuses to run rather than exec agent-authored commands on the host (R5.2/H1). Prompt hooks are depth-bounded (R5.4/H15) and run against an injected provider; tests judge through a stub with no live model call. Every failure folds into a verdict whose error status rejects (R7.5), persisted outside git keyed by (task, attempt) before pre-receive exits non-zero (R6.9). Feedback renders the §7 wire format: CR-free (R7.1), one greppable captain-json line that degrades before it can blow the budget (R7.2), a 64 KiB cap with an explicit truncation marker and full-log pointer (R7.3), and a keepalive ticker for long hooks (R7.4). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
An embedded gliderlabs/ssh server speaks git-receive-pack and the enrollment exchange, nothing else — upload-pack is refused by name so a shared endpoint cannot leak task namespaces or user branches (R2.3/H11). Keys authenticate; authorization maps the SHA256 fingerprint to an enrolled agent through an AgentDirectory consulted per handshake, so revocation takes effect for the next connection (R8.5). Client-supplied repo paths are containment-checked after resolution, rejecting .. traversal (R8.4/H13) — the three gavel-serve defects issue #39 §9 records are each corrected. receive-pack runs with the agent identity and receiver role injected for the hook shims. Enrollment issues a single-use short-TTL join token, never a key (R8.2): the agent generates its ed25519 pair on first start, presents the token over a host-fingerprint-pinned connection (no trust on first use), and the supervisor binds the fingerprint and burns the token — replay fails, and burning an expired token persists through the flocked captainconfig.Update (A3.4). A minimal GIT_SSH_COMMAND client rides the same transport so dispatch and relay pushes need no system ssh binary. captain sandbox git-agent add|list|revoke|serve land under the existing sandbox group; add/revoke stay MCP-excluded via ^sandbox (A7.3), add prints the join command and supports --dry-run with every mutation spelled out (A7.2), and serve prunes orphaned worktrees at startup (R10.3). The push test drives a real git push through the served endpoint using the test binary as the ssh client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
…the run path Dispatch (§6.1) snapshots the dirty worktree, records the audit refs and task state in the local mailbox, and pushes dispatch+control atomically to the sidecar with the envelope on push options, over captain's own GIT_SSH_COMMAND transport. The sidecar's post-receive — where quarantine has ended (R6.4/H2) — records state, creates the task branch, clones the agent's workspace with branch and upstream set so a bare git commit + git push suffices (R3.1/H17), materializes task.json outside the worktree, and launches the agent fully detached so the dispatch push returns promptly (R6.3/H12). A submit runs the whole chain inside one blocking push (§6.2): sidecar admission, materialized tier-1 hook set, then the nested relay — inside pre-receive, where rejection is still possible (R6.6/H16) — pushing the squashed result plus the original control commit with quarantine unset and object dirs kept (R1.4). A non-zero upstream exit rejects the agent's push (R6.7); the supervisor's sideband streams back through the sidecar's stderr. Attempts are consumed per submit, so a retry after rejection is attempt n+1 bounded by maxAttempts (§6.3). Acceptance integrates three-way against the envelope's base — HEAD may have moved (R10.1) — onto a captain/<task> branch, reports conflicts instead of auto-resolving them (R10.2), and writes the verdict ref and file. Run-path wiring: the git-agent adapter implements RemoteExecutor plus the isolate-workspace and egress-proxy markers, construction-time verifiers cover both capabilities, the resolveSandboxSelection guard admits the kind, SandboxRef.Agent/Policy thread through SandboxConfig, and buildProvider routes a remote-exec selection around local setup and provider construction entirely — refusing to combine with a setup checkout. The §12 conformance suite drives the real loop over loopback SSH and local repos, with the test binary standing in for the captain binary in both shims and transport. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
…ver in The sandbox holds captain-placeholder-<grant>-<header> stand-ins; the proxy substitutes the real value only when the placeholder appears in exactly the granted header for the granted destination. A placeholder anywhere else — another header, the URL, the body, another host — is rejected and logged, never stripped and forwarded: the appearance is an exfiltration attempt and silently continuing would hide it (R9.2). Destinations are deny-by-default, grants are scoped by method and path prefix because a host allowlist alone still permits POST /gists (R9.6/H7), CONNECT is refused so every request stays inspectable, DNS is resolved by the proxy itself with TLS validating the granted name — never the sandbox-controlled Host or SNI (R9.1) — an unresolvable credential fails the request rather than forwarding the placeholder (R9.4), and every decision is audited without ever logging a value (R9.5). Grant headers are types.EnvVar (A4.1); the static resolver covers inline values and store-backed resolvers plug in behind the same seam. TokenResult gains its Placeholder and a PlaceholderEnv projection, so a sandbox environment can be built that provably never carried the real credential (issue #39 §6.2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
Testing found the protocol sound but the product unusable: enrollment produced a topology no dispatch could use. The conformance suite could not see it because it wires its topology programmatically, so this adds end-to-end coverage that drives the compiled binary with a real supervisor and agent in separate processes and separate homes. Enrollment is now bidirectional, because trust is. The agent sends its endpoint and host key; the supervisor records both alongside the key (dispatch had nothing to connect to), and hands back its dispatch-key fingerprint and mailbox path. The agent authorizes that key so the supervisor's push is accepted (it previously was not) and composes a relay URL that carries a repository path (it previously did not). When no endpoint is advertised the supervisor derives one from the connection's source address and the agent's listen port, with --advertise as the override for NAT. serve gains the mailbox half: --role mailbox --repo creates the mailbox where dispatch writes, shares the real repository's objects, installs mailbox-role hooks and records the integration target. Previously only a sidecar repository was ever created, so a relayed result met a receiver that rejects result refs by construction. Two defects sat behind those. The GIT_SSH_COMMAND client mistook git's own "-o SendEnv=..." option for the hostname, failing every dispatch and relay push with a DNS lookup of the option itself. And the receive hooks resolved their configuration through the home directory, which for a co-located agent belongs to whoever pushed rather than to the receiver, so they silently loaded no hook sets and no relay target. Hook shims now carry their config path explicitly, and captainconfig grows SetPath for processes that cannot trust an ambient home. Also: the git-agent group prints its own help and setup steps instead of inheriting the parent's, list emits an empty array rather than null for an empty roster, revoke returns a result document rather than prose followed by null, and an enrollment carrying no reachable endpoint is refused at enrollment time, where it is actionable, instead of at dispatch time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
A dispatch to a git-agent sandbox failed at exactly two minutes with "dispatched but not concluded: context deadline exceeded". The dispatch had succeeded; what expired was the run's own deadline. A relocated run blocks while a remote coding agent does real work, but it inherited the request timeout meant for a model call, so it killed work that was still progressing and reported it as a failure. The run now sizes its deadline for where the work happens: when the resolved sandbox declares remote execution and no timeout was declared, it uses the backend's wait budget (waitTimeout, default one hour) — the same value the adapter already waits on, so the two can no longer disagree. An explicit --timeout or budget.timeout still wins, and local sandboxes are untouched. Applied on the direct, stream, workflow and batch paths so no entry point keeps the old default. Behind it sat a second defect: --timeout carried default:"120s", and that default was folded into the request ahead of the prompt file, so a frontmatter budget.timeout was silently overridden and could never take effect. The flag now defaults to empty and the 120s fallback lives in one place, which both restores frontmatter precedence and makes "the user asked for a timeout" distinguishable from "nobody did". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
The advertised add -> join -> ai prompt flow prepared a worktree and then went quiet. LaunchAgent treated an empty agentCommand as a no-op, enrollment never set one, and --model only travelled as metadata, so a dispatch waited out its whole budget for work that was never started. Nothing in the suite could see it: the end-to-end test drove the agent by hand, which is precisely the step meant to be automatic. A sidecar now launches captain itself when the backend configures no agent. The new run-task subcommand reads the dispatched task.json, runs the prompt in the prepared worktree with the sandbox pinned to none (it is already the relocated run; resolving a relocating sandbox here would dispatch onward, H15), then performs the agent's half of the protocol: stage, commit, push. The resolved backend now travels beside the model in task.json, so the agent runs the runtime the supervisor selected rather than re-resolving the name against its own defaults — which is what made --model cli:codex look like it selected nothing. An empty agentCommand is now an error rather than a silent no-op, since launching nothing is indistinguishable from an agent still thinking. Opting out is spelled agentCommand: none, and records in the task directory why nothing ran and where to push from. Tests are split so neither can hide the other: one proves a dispatch completes with no human touching the worktree (scripted agent, no credentials needed), one proves an unconfigured backend still launches the default agent, and one pins that the default command is captain's own run-task. The manual-push cycle test keeps its protocol coverage but now opts out explicitly and says in its name what it does not prove. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xfz36FVah9mBujveWyKRze
Map edit-capable permission modes to Codex workspace-write so run-task can create files. Mark inherited receive-pack descriptors close-on-exec so detached agents do not keep dispatch pushes open.
Remote runs could lose sandbox metadata, inherit the local model timeout, and bypass provider middleware. Receiver, SSH, and proxy edge cases could also hang work, lose concurrent state, or route credentialed requests outside the exact grant. Preserve remote selection and deadlines, supply hook judges, serialize durable state and key writes, harden Git/SSH transport and enrollment, and enforce canonical proxy scopes with reusable bounded transports. Add focused protocol and concurrency regressions.
Amp-Thread-ID: https://ampcode.com/threads/T-019fd21e-b226-70bc-b705-dfbbfd091e4d Co-authored-by: Aditya Thebe <contact@adityathebe.com>
Task hooks were serialized at the wrong level and never loaded from the control payload, so failing checks could be silently accepted. Integration also detected conflicts in post-receive, after receive-pack had already reported success. Preserve and validate tiered hook sets in task state, use them during receive vetting, and preflight three-way integration while result objects remain quarantined. Reject conflicts before ref acceptance and fail closed if HEAD changes before post-receive. Add protocol and adapter regressions.
Amp-Thread-ID: https://ampcode.com/threads/T-019fd5cc-e03d-7056-b868-774c78b47738 Co-authored-by: Aditya Thebe <contact@adityathebe.com>
Exec hooks declared as shell strings never ran under hookSandbox: srt — the adapter's per-CLI switch rejected "sh" before sandbox-runtime started. Allowing it naively would have been worse: the hook path never called Prepare, so the filesystem policy fell back to the hook process's working directory — the bare receiving repository, whose hooks/ dir the mandatory deny scan does not cover — making the receive shims themselves writable from inside the sandbox. The SRT adapter gains an explicit hook profile, selected at construction (api.SandboxProfileHook), never inferred from the wrapped argv: writes are confined to the materialized tree plus a private scratch directory that becomes the run's TMPDIR and HOME (host /tmp stays read-only), network isolation is on with every domain denied, no credential env is passed through, and reads of provider state, captain's own config and key material, and the receiving repository are hidden on top of the host-credential deny list. Prepare with no workspace fails closed. ResolveHookWrap now returns a per-workspace factory instead of a bare wrap func: the confining sandbox is built after materialization, against the tree it confines, and closed once the hook set has run. Unknown kinds and wrapper-less adapters still fail at hook startup. Two boundary leaks are closed alongside: CmdVerifier no longer widens a wrapper's nil env into full process inheritance (the pre-wrap env is the boundary), and hooks now receive an allowlisted environment (HookExecEnv) rather than a scrubbed copy of the host environ, so ambient provider credentials cannot reach agent-authored commands and R1.1's git scrub falls out of the allowlist (issue #40 R5.2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0168pEfMnMsDZRcvX6D1ynMY
Detached agent runs wrote normal Captain output only to task log files, leaving the long-running sidecar terminal silent while work was executing. Tail new task output from the serve process and add task, attempt, verdict, submission, failure, and duration lifecycle messages. Preserve detached receive-pack semantics and avoid replaying historical logs after restart.
A single global mailbox was rebound to whichever repository dispatched most recently. Existing task refs then resolved through the wrong object alternate, poisoning later admissions and limiting each supervisor endpoint to one worktree. Derive an immutable mailbox per canonical repository, carry its opaque route through dispatch and relay state, and resolve integration from the mailbox-local binding. Keep synthetic dispatch objects in the mailbox and scope blob admission to the current task range.
Remote tasks lost the supervisor's effort selection and committed under a generic identity. Over-limit retries also reused attempt numbers, credential grants could resolve over cleartext HTTP, and an unterminated relay stream could grow without bound. Carry effort into the relocated run and commit metadata, persist every submit attempt, reject credential-bearing HTTP grants before resolution, and cap relay line buffering with a single truncation notice. Add focused regressions for each path.
4b74948 to
74eac35
Compare
3 participants
Summary
Implements the git-agent protocol from #40: a supervisor snapshots a repository, dispatches the task over Git to an enrolled remote agent, vets the result at both the sidecar and supervisor tiers, and integrates accepted work without changing the user's checkout.
A supervisor endpoint is host-scoped rather than repository-scoped. Start it once, enroll an agent once, and dispatch work from multiple local repositories. Each repository receives an isolated mailbox while the remote agent continues to use one shared sidecar.
End-to-end flow
git push.refs/heads/captain/<task-id>using the recorded dispatch base.Multi-repository mailbox routing
~/.captain/sandbox/repos/mailboxes/<repository-hash>.git.Setup
Start the supervisor endpoint once:
Create a single-use enrollment:
Run the generated join command on the agent host, adding its reachable addresses:
The same supervisor and agent processes can then serve any repository:
Security and integrity
git-receive-packonly and enforce task/agent namespace ownership.