Repository navigation
Conversation
e2e/connect runs the real binary as subprocesses against internal/connector/fakebasecamp. Each test gets a home of its own and sets the agent up the way a person does, with `auth agent connect --no-browser` and `connect setup`, then runs `basecamp connect` and reads both outputs continuously: stdout as typed NDJSON (pointer, event and request lines), stderr as slog records. Waits are on lines or on the fake's state, with a deadline that only bounds a failure; SIGINT is sent only after the connector says it is running. TestMain's run builds the binary once, the way the test binary was built (-race for a race build, -tags dev for a dev build), so a race in the connector fails the test that provoked it. The scenarios: setup writes connect.json; a trusted mention becomes one request line with the right fields and the agent's mention stripped; an event on both lanes is one pointer and one request; an untrusted mention is an event line with untrusted_performer and no request; stdout holds only protocol lines and stderr says the connector runs.
b809bcc to
15e53c0
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The stdout parser accepts malformed objects as valid protocol pointer lines.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds end-to-end coverage for running the real basecamp connect binary against the fake Basecamp.
Changes:
- Adds isolated subprocess, output-capture, and synchronization harnesses.
- Tests setup, trusted/untrusted mentions, lane deduplication, and output protocols.
- Preserves race and dev build tags when building the tested binary.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| switch { | ||
| case head.Type == nil: | ||
| l.Pointer = &PointerLine{} | ||
| l.Err = strictUnmarshal(raw, l.Pointer) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found and verified against the latest diff
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="e2e/connect/lines_test.go">
<violation number="1" location="e2e/connect/lines_test.go:131">
P3: `strictUnmarshal` only rejects a non-`{` prefix; `json.Unmarshal` silently ignores unknown keys and absent fields, so a line like `{\"type\":\"request\"}` or a request line with an extra/renamed field still parses as a valid protocol line with zero values. That undercuts the file's stated guarantee that a line \"that is not one JSON object of a known shape is refused\" and that protocol drift surfaces in this package. Decode the full line with a `json.Decoder` that calls `DisallowUnknownFields()`, and keep the plain head pre-decode (which only knows `type`) permissive.\n\nFix sketch:\n\n\tfunc parseLine(raw string) Line {\n\t\tl := Line{Raw: raw}\n\t\tif !strings.HasPrefix(raw, \"{\") {\n\t\t\tl.Err = errors.New(\"not a JSON object\")\n\t\t\treturn l\n\t\t}\n\t\tvar head struct {\n\t\t\tType *string `json:\"type\"`\n\t\t}\n\t\tif err := json.Unmarshal([]byte(raw), &head); err != nil {\n\t\t\tl.Err = err\n\t\t\treturn l\n\t\t}\n\t\tdecodeShape := func(v any) error {\n\t\t\tdec := json.NewDecoder(strings.NewReader(raw))\n\t\t\tdec.DisallowUnknownFields()\n\t\t\tif err := dec.Decode(v); err != nil {\n\t\t\t\treturn err\n\t\t\t}\n\t\t\tvar extra any\n\t\t\treturn dec.Decode(&extra) // must be io.EOF for trailing-data refusal\n\t\t}\n\t\tswitch {\n\t\tcase head.Type == nil:\n\t\t\tl.Pointer = &PointerLine{}\n\t\t\tl.Err = decodeShape(l.Pointer)\n\t\tcase *head.Type == \"event\":\n\t\t\tl.Event = &EventLine{}\n\t\t\tl.Err = decodeShape(l.Event)\n\t\tcase *head.Type == \"request\":\n\t\t\tl.Request = &RequestLine{}\n\t\t\tl.Err = decodeShape(l.Request)\n\t\tdefault:\n\t\t\tl.Err = fmt.Errorf(\"unknown line type %q\", *head.Type)\n\t\t}\n\t\treturn l\n\t}</violation>
</file>
<file name="e2e/connect/connector_test.go">
<violation number="1" location="e2e/connect/connector_test.go:258">
P2: `cleanup` blocks forever on `<-c.exited` with no timeout. The exit goroutine only finishes after both pipe readers hit EOF and `cmd.Wait()` returns; if the connector ever fails to terminate (or a child it spawned inherits its stdout/stderr and keeps them open), the readers never reach EOF and the entire test binary hangs instead of failing this test. Every other wait in the harness is bounded by `waitFor`; this one should be too.</violation>
<violation number="2" location="e2e/connect/connector_test.go:261">
P3: A final line torn by SIGKILL is reported as a protocol violation. If a test uses `Kill(t)` while the connector is mid-write, `readLines` flags the unterminated last line with "the line was not ended: torn", and `cleanup` then fails the test with "stdout line N is not a protocol line" even though the test's own assertions passed. Skip the protocol check for torn lines when the process was signaled (`out.ExitCode == -1`).</violation>
</file>
<file name="e2e/connect/harness_test.go">
<violation number="1" location="e2e/connect/harness_test.go:35">
P3: `Harness.t` is never read: `NewHarness` stores the `*testing.T` in it, but every method (`Run`, `MustRun`, `Setup`) takes `t` as a parameter and nothing in the package accesses the field (the only `.t` use is `Connector.t`). Drop the field and its assignment in `NewHarness`.</violation>
<violation number="2" location="e2e/connect/harness_test.go:72">
P2: A `GORACE=log_path=...` setting redirects race reports away from stderr, but `requireNoRace` only scans stderr, so subprocess races can pass unnoticed. Force reports to stderr or inspect the configured report file.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // do: write only protocol lines to stdout, and provoke no race. | ||
| func (c *Connector) cleanup() { | ||
| t := c.t | ||
| <-c.exited |
There was a problem hiding this comment.
P2: cleanup blocks forever on <-c.exited with no timeout. The exit goroutine only finishes after both pipe readers hit EOF and cmd.Wait() returns; if the connector ever fails to terminate (or a child it spawned inherits its stdout/stderr and keeps them open), the readers never reach EOF and the entire test binary hangs instead of failing this test. Every other wait in the harness is bounded by waitFor; this one should be too.
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 e2e/connect/connector_test.go, line 258:
<comment>`cleanup` blocks forever on `<-c.exited` with no timeout. The exit goroutine only finishes after both pipe readers hit EOF and `cmd.Wait()` returns; if the connector ever fails to terminate (or a child it spawned inherits its stdout/stderr and keeps them open), the readers never reach EOF and the entire test binary hangs instead of failing this test. Every other wait in the harness is bounded by `waitFor`; this one should be too.</comment>
<file context>
@@ -0,0 +1,364 @@
+// do: write only protocol lines to stdout, and provoke no race.
+func (c *Connector) cleanup() {
+ t := c.t
+ <-c.exited
+ out := c.Output()
+ for i, l := range out.Lines {
</file context>
| // and the race detector's options when the run sets them. | ||
| for _, key := range []string{"PATH", "TMPDIR", "GORACE"} { | ||
| if v, ok := os.LookupEnv(key); ok { | ||
| env = append(env, key+"="+v) |
There was a problem hiding this comment.
P2: A GORACE=log_path=... setting redirects race reports away from stderr, but requireNoRace only scans stderr, so subprocess races can pass unnoticed. Force reports to stderr or inspect the configured report file.
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 e2e/connect/harness_test.go, line 72:
<comment>A `GORACE=log_path=...` setting redirects race reports away from stderr, but `requireNoRace` only scans stderr, so subprocess races can pass unnoticed. Force reports to stderr or inspect the configured report file.</comment>
<file context>
@@ -0,0 +1,148 @@
+ // and the race detector's options when the run sets them.
+ for _, key := range []string{"PATH", "TMPDIR", "GORACE"} {
+ if v, ok := os.LookupEnv(key); ok {
+ env = append(env, key+"="+v)
+ }
+ }
</file context>
| if !strings.HasPrefix(raw, "{") { | ||
| return errors.New("not a JSON object") | ||
| } | ||
| return json.Unmarshal([]byte(raw), v) |
There was a problem hiding this comment.
P3: strictUnmarshal only rejects a non-{ prefix; json.Unmarshal silently ignores unknown keys and absent fields, so a line like {\"type\":\"request\"} or a request line with an extra/renamed field still parses as a valid protocol line with zero values. That undercuts the file's stated guarantee that a line "that is not one JSON object of a known shape is refused" and that protocol drift surfaces in this package. Decode the full line with a json.Decoder that calls DisallowUnknownFields(), and keep the plain head pre-decode (which only knows type) permissive.\n\nFix sketch:\n\n\tfunc parseLine(raw string) Line {\n\t\tl := Line{Raw: raw}\n\t\tif !strings.HasPrefix(raw, "{") {\n\t\t\tl.Err = errors.New("not a JSON object")\n\t\t\treturn l\n\t\t}\n\t\tvar head struct {\n\t\t\tType *string json:\"type\"\n\t\t}\n\t\tif err := json.Unmarshal([]byte(raw), &head); err != nil {\n\t\t\tl.Err = err\n\t\t\treturn l\n\t\t}\n\t\tdecodeShape := func(v any) error {\n\t\t\tdec := json.NewDecoder(strings.NewReader(raw))\n\t\t\tdec.DisallowUnknownFields()\n\t\t\tif err := dec.Decode(v); err != nil {\n\t\t\t\treturn err\n\t\t\t}\n\t\t\tvar extra any\n\t\t\treturn dec.Decode(&extra) // must be io.EOF for trailing-data refusal\n\t\t}\n\t\tswitch {\n\t\tcase head.Type == nil:\n\t\t\tl.Pointer = &PointerLine{}\n\t\t\tl.Err = decodeShape(l.Pointer)\n\t\tcase *head.Type == "event":\n\t\t\tl.Event = &EventLine{}\n\t\t\tl.Err = decodeShape(l.Event)\n\t\tcase *head.Type == "request":\n\t\t\tl.Request = &RequestLine{}\n\t\t\tl.Err = decodeShape(l.Request)\n\t\tdefault:\n\t\t\tl.Err = fmt.Errorf("unknown line type %q", *head.Type)\n\t\t}\n\t\treturn l\n\t}
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 e2e/connect/lines_test.go, line 131:
<comment>`strictUnmarshal` only rejects a non-`{` prefix; `json.Unmarshal` silently ignores unknown keys and absent fields, so a line like `{\"type\":\"request\"}` or a request line with an extra/renamed field still parses as a valid protocol line with zero values. That undercuts the file's stated guarantee that a line \"that is not one JSON object of a known shape is refused\" and that protocol drift surfaces in this package. Decode the full line with a `json.Decoder` that calls `DisallowUnknownFields()`, and keep the plain head pre-decode (which only knows `type`) permissive.\n\nFix sketch:\n\n\tfunc parseLine(raw string) Line {\n\t\tl := Line{Raw: raw}\n\t\tif !strings.HasPrefix(raw, \"{\") {\n\t\t\tl.Err = errors.New(\"not a JSON object\")\n\t\t\treturn l\n\t\t}\n\t\tvar head struct {\n\t\t\tType *string `json:\"type\"`\n\t\t}\n\t\tif err := json.Unmarshal([]byte(raw), &head); err != nil {\n\t\t\tl.Err = err\n\t\t\treturn l\n\t\t}\n\t\tdecodeShape := func(v any) error {\n\t\t\tdec := json.NewDecoder(strings.NewReader(raw))\n\t\t\tdec.DisallowUnknownFields()\n\t\t\tif err := dec.Decode(v); err != nil {\n\t\t\t\treturn err\n\t\t\t}\n\t\t\tvar extra any\n\t\t\treturn dec.Decode(&extra) // must be io.EOF for trailing-data refusal\n\t\t}\n\t\tswitch {\n\t\tcase head.Type == nil:\n\t\t\tl.Pointer = &PointerLine{}\n\t\t\tl.Err = decodeShape(l.Pointer)\n\t\tcase *head.Type == \"event\":\n\t\t\tl.Event = &EventLine{}\n\t\t\tl.Err = decodeShape(l.Event)\n\t\tcase *head.Type == \"request\":\n\t\t\tl.Request = &RequestLine{}\n\t\t\tl.Err = decodeShape(l.Request)\n\t\tdefault:\n\t\t\tl.Err = fmt.Errorf(\"unknown line type %q\", *head.Type)\n\t\t}\n\t\treturn l\n\t}</comment>
<file context>
@@ -0,0 +1,235 @@
+ if !strings.HasPrefix(raw, "{") {
+ return errors.New("not a JSON object")
+ }
+ return json.Unmarshal([]byte(raw), v)
+}
+
</file context>
| <-c.exited | ||
| out := c.Output() | ||
| for i, l := range out.Lines { | ||
| if l.Err != nil { |
There was a problem hiding this comment.
P3: A final line torn by SIGKILL is reported as a protocol violation. If a test uses Kill(t) while the connector is mid-write, readLines flags the unterminated last line with "the line was not ended: torn", and cleanup then fails the test with "stdout line N is not a protocol line" even though the test's own assertions passed. Skip the protocol check for torn lines when the process was signaled (out.ExitCode == -1).
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 e2e/connect/connector_test.go, line 261:
<comment>A final line torn by SIGKILL is reported as a protocol violation. If a test uses `Kill(t)` while the connector is mid-write, `readLines` flags the unterminated last line with "the line was not ended: torn", and `cleanup` then fails the test with "stdout line N is not a protocol line" even though the test's own assertions passed. Skip the protocol check for torn lines when the process was signaled (`out.ExitCode == -1`).</comment>
<file context>
@@ -0,0 +1,364 @@
+ <-c.exited
+ out := c.Output()
+ for i, l := range out.Lines {
+ if l.Err != nil {
+ t.Errorf("%s: stdout line %d is not a protocol line (%v): %q", c.what, i+1, l.Err, l.Raw)
+ }
</file context>
There was a problem hiding this comment.
P3: Harness.t is never read: NewHarness stores the *testing.T in it, but every method (Run, MustRun, Setup) takes t as a parameter and nothing in the package accesses the field (the only .t use is Connector.t). Drop the field and its assignment in NewHarness.
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 e2e/connect/harness_test.go, line 35:
<comment>`Harness.t` is never read: `NewHarness` stores the `*testing.T` in it, but every method (`Run`, `MustRun`, `Setup`) takes `t` as a parameter and nothing in the package accesses the field (the only `.t` use is `Connector.t`). Drop the field and its assignment in `NewHarness`.</comment>
<file context>
@@ -0,0 +1,148 @@
+// its config, its credentials and the connector's state are the test's
+// alone.
+type Harness struct {
+ t *testing.T
+ // Fake is the Basecamp the CLI talks to.
+ Fake *fakebasecamp.Server
</file context>
PR 2 of a stack of 8 that tests
basecamp connectend to end. Stacked on #822 (the fake Basecamp). It adds a test package and changes no behaviour of the CLI. It does not touch the fake.Why
#822 gives us a Basecamp that speaks both lanes of the event feed. Nothing yet runs the real binary against it. The unit tests prove the parts. They do not prove that a person who runs
auth agent connect,connect setupandconnectgets one request line on stdout when the operator mentions the agent.The failure scenarios that come next (PR 3), the grace knob (PR 5) and the dev BC3 target (PR 6) all need the same harness: a real
basecamp connectprocess, its two outputs read as they are written, and waits that never sleep. This PR builds that harness and holds it to the happy path.What
e2e/connect, a Go test package (//go:build linux || darwin, the platforms the connector runs on). It is insidego test ./..., somake test,bin/ci, the unit test job and the race shards all run it. It has no-shortskip.TestMain's run buildsbasecamponce, the first time a test needs it, sogo test -listbuilds nothing. It is built the way the test binary was:-racewhen the test binary is a race build, and-tags devwhen it is a dev build. Smallrace_tag/dev_tagtest files set the constants, asinternal/commands/dev_tag.godoes. If a subprocess reportsWARNING: DATA RACE, the test fails.NewHarnessstarts the fake and builds the CLI's whole environment from nothing: HOME, XDG_CONFIG_HOME, XDG_STATE_HOME, XDG_CACHE_HOME,BASECAMP_NO_KEYRING=1,BASECAMP_NO_UPDATE_CHECK=1, andBASECAMP_BASE_URL/BASECAMP_OAUTH_ISSUERat the fake. Only PATH, TMPDIR and GORACE pass through. So no BASECAMP_TOKEN, BASECAMP_NONINTERACTIVE or real config from the host can get in.Setuprunsbasecamp auth agent connect -P agent --no-browser, thenbasecamp connect setup -P agent --operator <id> --serve <id> --json.Run/MustRunrun any one-shot command.Connect(orStartfor any command, in any directory) starts the process. Goroutines read stdout and stderr to the end the whole time, so the process never blocks on a full pipe. Stdout is parsed into typed lines: a pointer (notype), aneventverdict or arequest. The wire types are written out in the test rather than borrowed from the connector, so a protocol change shows up here. Stderr is parsed as slog records.WaitFor(predicate)checks again each time the process writes, never on a timer. A 30s deadline only bounds a failure. If the process exits before the condition is true, the test fails at once with the transcript.WaitRunningandWaitStreaming(n)wait forconnector: runningand for the nth time the feed walk reaches its head.Interruptsends SIGINT only afterconnector: running.Signal,Kill,Wait(exit code) andStop(SIGINT, then exit 130) are also there.Tests
live), oneadmittedevent line and exactly one request line. The request line has the event id,comment.created, triggermentioned, and the recording with its URL and project name. It also hasreply_to(the parent message), the requester id and name,acknowledge: true, and the content with the agent's mention replaced by a space.discarded/untrusted_performerand no request line.connector: runningwith the profile, account and agent.null, arrays, unknown types, two objects on a line, a torn line).The five scenarios run in parallel in about 3s (about 7s under
-race).go test -race -count=10 ./e2e/connectpasses, and also passes when it runs at the same time as-count=30without race.bin/ciis green.Summary by cubic
Adds an end-to-end test package that runs the real
basecamp connectbinary against the fake Basecamp, covering the setup and connector flow a person runs.-shortskip.-race,-tags dev), so a race in the connector fails its test.Coverage
auth agent connect,connect setup) writes owner-only connect.json.admittedevent, and exactly one request line with the expected fields.discardedevent line and no request.Written for commit 15e53c0. Summary will update on new commits.