Repository navigation
Conversation
The scenarios in e2e/connect ran only against the fake Basecamp. Nothing showed that the fake behaves as Basecamp does. Now the same scenarios also run against a local BC3, so passing on both is the conformance check between the two. - Target says what a scenario needs in its own words: the CLI to run, the serving agent, the operator mentions the agent, an untrusted person mentions the agent. Posted is the comment as Basecamp holds it, so the request line is checked field by field on either target. - fakeTarget is the earlier behavior through fakebasecamp. - devTarget drives a local Basecamp with the CLI under test and the developer's own config and credentials. It reads connect.json with `connect show` and never runs setup. Each scenario gets a temporary XDG_STATE_HOME (its own ledger and instance lock), a new message posted by the operator with nobody subscribed, and comments made with `comments create` and `[@agent](person:<id>)`. It trashes the message at the end. It refuses to run while `connect status` shows a connector running for the agent, and it refuses a BASECAMP_BASE_URL that is not local. - These run on both targets: a trusted mention, an untrusted mention, stdout only NDJSON, SIGINT, a second connector refused, and a request handed off after --hold, kill -9, release and restart. Scenarios that need a fault only the fake can make call fakeOnly with their reason. The dev run shows them as skipped. - `make test-connect-dev` sets BASECAMP_CONNECT_DEV=1 and fails when a required variable is missing. A plain `go test` runs only the fake. - CONTRIBUTING.md tells how to run it and what it covers.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The dev-target lock check is unreliable, and cleanup can overlap supposedly serialized scenarios.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Adds a local-Basecamp target for existing connector end-to-end scenarios while retaining fake-only fault testing.
Changes:
- Introduces shared fake/dev target abstractions.
- Adds local environment validation, state isolation, and cleanup.
- Documents and exposes
make test-connect-dev.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
Makefile |
Adds the local connector test target. |
CONTRIBUTING.md |
Documents prerequisites and behavior. |
e2e/connect/target_test.go |
Defines shared target abstractions. |
e2e/connect/dev_target_test.go |
Implements the local Basecamp target. |
e2e/connect/fake_target_test.go |
Adapts the fake to the target interface. |
e2e/connect/connect_test.go |
Runs core scenarios across targets. |
e2e/connect/failures_test.go |
Shares applicable failure scenarios. |
e2e/connect/harness_test.go |
Adds working-directory and cleanup support. |
e2e/connect/connector_test.go |
Uses the harness working directory. |
e2e/connect/main_test.go |
Updates package documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| BucketID: p.BucketID, | ||
| ProjectName: p.ProjectName, | ||
| RecordingID: p.RecordingID, | ||
| Type: "Comment", |
Comment on lines
+344
to
+345
| if status.LockHolder != nil && status.LockHolder.PIDStatus == "present" { | ||
| t.Fatalf("a connector for %s may be running (pid %d): stop it first, or it is handed the scenarios' mentions too", d.cfg.AgentProfile, status.LockHolder.PID) |
Comment on lines
+87
to
+89
| devSerial.Lock() | ||
| defer devSerial.Unlock() | ||
| scenario(t, newDevTarget(t)) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="e2e/connect/harness_test.go">
<violation number="1" location="e2e/connect/harness_test.go:122">
P3: `Run`'s doc comment still says the command runs "in the harness's home," but commands now run in `h.Dir`; for a devTarget harness `Home` is documented as empty and the CLI runs in the fresh `Dir` with the developer's home. Update the comment to say the harness's directory (`Dir`) so it does not conflate the two fields this change separates.</violation>
</file>
<file name="e2e/connect/dev_target_test.go">
<violation number="1" location="e2e/connect/dev_target_test.go:139">
P3: `devEnv` drops every BASECAMP_* variable that would change who a command acts as or where, but not `BASECAMP_OAUTH_ISSUER`. A developer with that escape hatch pinned (README describes it as a temporary dark-pilot pin) will have every dev-target command authenticate against the wrong issuer — often production — while `BASECAMP_BASE_URL` points at the local Basecamp, failing later and confusingly. Drop it alongside the others so the issuer is discovered from the dev Basecamp URL.</violation>
<violation number="2" location="e2e/connect/dev_target_test.go:226">
P2: This check permits a distinct profile authenticated as the agent. Admission discards agent-authored events before the `untrusted_performer` verdict, so the scenario waits until timeout; also reject `d.agent.PersonID`.</violation>
<violation number="3" location="e2e/connect/dev_target_test.go:334">
P3: This preflight validates a `--json` command's output by substring match: `connect status` returns a structured error envelope (ErrUsageHint JSON), and `require.Contains(stdout+stderr, "has no ledger yet")` matches text inside it. Decode the envelope and assert the message field instead, so a misplaced key or a reworded hint surfaces as a failure rather than a silent pass.</violation>
<violation number="4" location="e2e/connect/dev_target_test.go:344">
P1: Use the actual agent instance lock to gate the dev target instead of relying on `pid_status`; status metadata can be absent or stale, allowing a running connector to consume the scenario mentions concurrently.</violation>
</file>
<file name="CONTRIBUTING.md">
<violation number="1" location="CONTRIBUTING.md:134">
P2: This target only runs on Linux and macOS: the `e2e/connect` package is build-tagged for `linux || darwin`, so Windows contributors following these instructions will not run the scenarios. State the platform prerequisite here.</violation>
</file>
<file name="e2e/connect/connector_test.go">
<violation number="1" location="e2e/connect/connector_test.go:75">
P3: The doc comments directly above `Connect` and `ConnectProfile` still say the command runs "in the harness's home", but this line now starts it in `h.Dir`. For the dev target `Home` is empty and `Dir` is the temporary working directory, so the comments are wrong there. Update both to say the commands run in the harness's working directory.</violation>
</file>
<file name="Makefile">
<violation number="1" location="Makefile:99">
P3: `test-connect-dev` omits `$(BUILD_TAGS)` that every other test recipe passes (`test`, `race-test`, `test-coverage`, etc. all use `$(GOTEST) $(BUILD_TAGS)`). Since `make test` runs `go test -tags dev -v ./...` and therefore includes `./e2e/connect`, the fake-based runs are built with `devBuild=true`, which makes e2e/connect's `binary()` build the basecamp binary with `-tags dev` (dev_tag_test.go, main_test.go). This target builds the test binary without the tag, so against the local Basecamp the connector under test is a release-stub build — the conformance comparison is between two differently-tagged binaries, and any future dev-gated behavior silently diverges. Add `$(BUILD_TAGS)` to the recipe.</violation>
</file>
<file name="e2e/connect/target_test.go">
<violation number="1" location="e2e/connect/target_test.go:88">
P2: The deferred unlock runs before this subtest's cleanups, so another dev scenario can start while the previous connector is still alive or its message is still being trashed. Register the unlock with `t.Cleanup` so target and connector cleanup finishes before the next scenario acquires the mutex.</violation>
</file>
<file name="e2e/connect/connect_test.go">
<violation number="1" location="e2e/connect/connect_test.go:144">
P2: Compare `p.Type` here instead of hardcoding `"Comment"`; otherwise the dev conformance test cannot detect a mismatch between Basecamp’s returned type and the connector’s request line.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } `json:"lock_holder"` | ||
| } | ||
| d.data(t, res, &status) | ||
| if status.LockHolder != nil && status.LockHolder.PIDStatus == "present" { |
Contributor
There was a problem hiding this comment.
P1: Use the actual agent instance lock to gate the dev target instead of relying on pid_status; status metadata can be absent or stale, allowing a running connector to consume the scenario mentions concurrently.
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/dev_target_test.go, line 344:
<comment>Use the actual agent instance lock to gate the dev target instead of relying on `pid_status`; status metadata can be absent or stale, allowing a running connector to consume the scenario mentions concurrently.</comment>
<file context>
@@ -0,0 +1,500 @@
+ } `json:"lock_holder"`
+ }
+ d.data(t, res, &status)
+ if status.LockHolder != nil && status.LockHolder.PIDStatus == "present" {
+ t.Fatalf("a connector for %s may be running (pid %d): stop it first, or it is handed the scenarios' mentions too", d.cfg.AgentProfile, status.LockHolder.PID)
+ }
</file context>
| func (d *devTarget) UntrustedMentionsAgent(t *testing.T) Posted { | ||
| t.Helper() | ||
| p := d.post(t, d.cfg.UntrustedProfile) | ||
| require.NotEqual(t, d.operatorID, p.AuthorID, "%s is the operator", envUntrustedProfile) |
Contributor
There was a problem hiding this comment.
P2: This check permits a distinct profile authenticated as the agent. Admission discards agent-authored events before the untrusted_performer verdict, so the scenario waits until timeout; also reject d.agent.PersonID.
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/dev_target_test.go, line 226:
<comment>This check permits a distinct profile authenticated as the agent. Admission discards agent-authored events before the `untrusted_performer` verdict, so the scenario waits until timeout; also reject `d.agent.PersonID`.</comment>
<file context>
@@ -0,0 +1,500 @@
+func (d *devTarget) UntrustedMentionsAgent(t *testing.T) Posted {
+ t.Helper()
+ p := d.post(t, d.cfg.UntrustedProfile)
+ require.NotEqual(t, d.operatorID, p.AuthorID, "%s is the operator", envUntrustedProfile)
+ return p
+}
</file context>
Suggested change
| require.NotEqual(t, d.operatorID, p.AuthorID, "%s is the operator", envUntrustedProfile) | |
| require.NotEqual(t, d.operatorID, p.AuthorID, "%s is the operator", envUntrustedProfile) | |
| require.NotEqual(t, d.agent.PersonID, p.AuthorID, "%s is the agent", envUntrustedProfile) |
| `basecamp connect status -P agent` shows one, because that connector would | ||
| get the scenarios' mentions too. | ||
|
|
||
| Then run: |
Contributor
There was a problem hiding this comment.
P2: This target only runs on Linux and macOS: the e2e/connect package is build-tagged for linux || darwin, so Windows contributors following these instructions will not run the scenarios. State the platform prerequisite here.
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 CONTRIBUTING.md, line 134:
<comment>This target only runs on Linux and macOS: the `e2e/connect` package is build-tagged for `linux || darwin`, so Windows contributors following these instructions will not run the scenarios. State the platform prerequisite here.</comment>
<file context>
@@ -103,6 +103,79 @@ make test-e2e
+ `basecamp connect status -P agent` shows one, because that connector would
+ get the scenarios' mentions too.
+
+Then run:
+
+```bash
</file context>
Suggested change
| Then run: | |
| On Linux or macOS, run: |
| // One scenario at a time against a local Basecamp: they share its | ||
| // agent, its credential and its project. | ||
| devSerial.Lock() | ||
| defer devSerial.Unlock() |
Contributor
There was a problem hiding this comment.
P2: The deferred unlock runs before this subtest's cleanups, so another dev scenario can start while the previous connector is still alive or its message is still being trashed. Register the unlock with t.Cleanup so target and connector cleanup finishes before the next scenario acquires the mutex.
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/target_test.go, line 88:
<comment>The deferred unlock runs before this subtest's cleanups, so another dev scenario can start while the previous connector is still alive or its message is still being trashed. Register the unlock with `t.Cleanup` so target and connector cleanup finishes before the next scenario acquires the mutex.</comment>
<file context>
@@ -0,0 +1,152 @@
+ // One scenario at a time against a local Basecamp: they share its
+ // agent, its credential and its project.
+ devSerial.Lock()
+ defer devSerial.Unlock()
+ scenario(t, newDevTarget(t))
+ })
</file context>
Suggested change
| defer devSerial.Unlock() | |
| t.Cleanup(devSerial.Unlock) |
| BucketID: p.BucketID, | ||
| ProjectName: p.ProjectName, | ||
| RecordingID: p.RecordingID, | ||
| Type: "Comment", |
Contributor
There was a problem hiding this comment.
P2: Compare p.Type here instead of hardcoding "Comment"; otherwise the dev conformance test cannot detect a mismatch between Basecamp’s returned type and the connector’s request line.
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/connect_test.go, line 144:
<comment>Compare `p.Type` here instead of hardcoding `"Comment"`; otherwise the dev conformance test cannot detect a mismatch between Basecamp’s returned type and the connector’s request line.</comment>
<file context>
@@ -114,48 +112,55 @@ func TestSetupThroughTheCLIWritesConnectJSON(t *testing.T) {
+ BucketID: p.BucketID,
+ ProjectName: p.ProjectName,
+ RecordingID: p.RecordingID,
+ Type: "Comment",
+ Title: p.Title,
+ URL: p.URL,
</file context>
Suggested change
| Type: "Comment", | |
| Type: p.Type, |
| cmd := exec.CommandContext(ctx, bin, args...) | ||
| cmd.Env = h.env | ||
| cmd.Dir = h.Home | ||
| cmd.Dir = h.Dir |
Contributor
There was a problem hiding this comment.
P3: Run's doc comment still says the command runs "in the harness's home," but commands now run in h.Dir; for a devTarget harness Home is documented as empty and the CLI runs in the fresh Dir with the developer's home. Update the comment to say the harness's directory (Dir) so it does not conflate the two fields this change separates.
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 122:
<comment>`Run`'s doc comment still says the command runs "in the harness's home," but commands now run in `h.Dir`; for a devTarget harness `Home` is documented as empty and the CLI runs in the fresh `Dir` with the developer's home. Update the comment to say the harness's directory (`Dir`) so it does not conflate the two fields this change separates.</comment>
<file context>
@@ -98,13 +106,20 @@ type Result struct {
cmd := exec.CommandContext(ctx, bin, args...)
cmd.Env = h.env
- cmd.Dir = h.Home
+ cmd.Dir = h.Dir
var stdout, stderr bytes.Buffer
cmd.Stdout, cmd.Stderr = &stdout, &stderr
</file context>
| func (h *Harness) ConnectProfile(t *testing.T, p Profile, extra ...string) *Connector { | ||
| t.Helper() | ||
| return h.Start(t, h.Home, append([]string{"connect", "-P", p.Name}, extra...)...) | ||
| return h.Start(t, h.Dir, append([]string{"connect", "-P", p.Name}, extra...)...) |
Contributor
There was a problem hiding this comment.
P3: The doc comments directly above Connect and ConnectProfile still say the command runs "in the harness's home", but this line now starts it in h.Dir. For the dev target Home is empty and Dir is the temporary working directory, so the comments are wrong there. Update both to say the commands run in the harness's working directory.
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 75:
<comment>The doc comments directly above `Connect` and `ConnectProfile` still say the command runs "in the harness's home", but this line now starts it in `h.Dir`. For the dev target `Home` is empty and `Dir` is the temporary working directory, so the comments are wrong there. Update both to say the commands run in the harness's working directory.</comment>
<file context>
@@ -72,7 +72,7 @@ func (h *Harness) Connect(t *testing.T, extra ...string) *Connector {
func (h *Harness) ConnectProfile(t *testing.T, p Profile, extra ...string) *Connector {
t.Helper()
- return h.Start(t, h.Home, append([]string{"connect", "-P", p.Name}, extra...)...)
+ return h.Start(t, h.Dir, append([]string{"connect", "-P", p.Name}, extra...)...)
}
</file context>
| @test -n "$(BASECAMP_CONNECT_DEV_OPERATOR_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_OPERATOR_PROFILE (the agent's operator, logged in); see CONTRIBUTING.md"; exit 1; } | ||
| @test -n "$(BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE (someone else on the project, logged in); see CONTRIBUTING.md"; exit 1; } | ||
| @test -n "$(BASECAMP_CONNECT_DEV_PROJECT_ID)" || { echo "Set BASECAMP_CONNECT_DEV_PROJECT_ID (the served project the scenarios post in); see CONTRIBUTING.md"; exit 1; } | ||
| BASECAMP_CONNECT_DEV=1 $(GOTEST) -count=1 -failfast -v ./e2e/connect |
Contributor
There was a problem hiding this comment.
P3: test-connect-dev omits $(BUILD_TAGS) that every other test recipe passes (test, race-test, test-coverage, etc. all use $(GOTEST) $(BUILD_TAGS)). Since make test runs go test -tags dev -v ./... and therefore includes ./e2e/connect, the fake-based runs are built with devBuild=true, which makes e2e/connect's binary() build the basecamp binary with -tags dev (dev_tag_test.go, main_test.go). This target builds the test binary without the tag, so against the local Basecamp the connector under test is a release-stub build — the conformance comparison is between two differently-tagged binaries, and any future dev-gated behavior silently diverges. Add $(BUILD_TAGS) to the recipe.
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 Makefile, line 99:
<comment>`test-connect-dev` omits `$(BUILD_TAGS)` that every other test recipe passes (`test`, `race-test`, `test-coverage`, etc. all use `$(GOTEST) $(BUILD_TAGS)`). Since `make test` runs `go test -tags dev -v ./...` and therefore includes `./e2e/connect`, the fake-based runs are built with `devBuild=true`, which makes e2e/connect's `binary()` build the basecamp binary with `-tags dev` (dev_tag_test.go, main_test.go). This target builds the test binary without the tag, so against the local Basecamp the connector under test is a release-stub build — the conformance comparison is between two differently-tagged binaries, and any future dev-gated behavior silently diverges. Add `$(BUILD_TAGS)` to the recipe.</comment>
<file context>
@@ -79,6 +79,25 @@ test: check-toolchain
+ @test -n "$(BASECAMP_CONNECT_DEV_OPERATOR_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_OPERATOR_PROFILE (the agent's operator, logged in); see CONTRIBUTING.md"; exit 1; }
+ @test -n "$(BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE (someone else on the project, logged in); see CONTRIBUTING.md"; exit 1; }
+ @test -n "$(BASECAMP_CONNECT_DEV_PROJECT_ID)" || { echo "Set BASECAMP_CONNECT_DEV_PROJECT_ID (the served project the scenarios post in); see CONTRIBUTING.md"; exit 1; }
+ BASECAMP_CONNECT_DEV=1 $(GOTEST) -count=1 -failfast -v ./e2e/connect
+
# Record cassettes for happy-path replay tests.
</file context>
Suggested change
| BASECAMP_CONNECT_DEV=1 $(GOTEST) -count=1 -failfast -v ./e2e/connect | |
| BASECAMP_CONNECT_DEV=1 $(GOTEST) $(BUILD_TAGS) -count=1 -failfast -v ./e2e/connect |
| own := &Harness{t: t, Dir: d.h.Dir, env: devEnv(os.Environ(), "")} | ||
| res := own.Run(t, "connect", "status", "-P", d.cfg.AgentProfile, "--json") | ||
| if res.ExitCode != 0 { | ||
| require.Contains(t, res.Stdout+res.Stderr, "has no ledger yet", "basecamp connect status -P %s\n%s%s", d.cfg.AgentProfile, res.Stdout, res.Stderr) |
Contributor
There was a problem hiding this comment.
P3: This preflight validates a --json command's output by substring match: connect status returns a structured error envelope (ErrUsageHint JSON), and require.Contains(stdout+stderr, "has no ledger yet") matches text inside it. Decode the envelope and assert the message field instead, so a misplaced key or a reworded hint surfaces as a failure rather than a silent pass.
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/dev_target_test.go, line 334:
<comment>This preflight validates a `--json` command's output by substring match: `connect status` returns a structured error envelope (ErrUsageHint JSON), and `require.Contains(stdout+stderr, "has no ledger yet")` matches text inside it. Decode the envelope and assert the message field instead, so a misplaced key or a reworded hint surfaces as a failure rather than a silent pass.</comment>
<file context>
@@ -0,0 +1,500 @@
+ own := &Harness{t: t, Dir: d.h.Dir, env: devEnv(os.Environ(), "")}
+ res := own.Run(t, "connect", "status", "-P", d.cfg.AgentProfile, "--json")
+ if res.ExitCode != 0 {
+ require.Contains(t, res.Stdout+res.Stderr, "has no ledger yet", "basecamp connect status -P %s\n%s%s", d.cfg.AgentProfile, res.Stdout, res.Stderr)
+ return
+ }
</file context>
Suggested change
| require.Contains(t, res.Stdout+res.Stderr, "has no ledger yet", "basecamp connect status -P %s\n%s%s", d.cfg.AgentProfile, res.Stdout, res.Stderr) | |
| var envelope struct { | |
| Error struct { | |
| Message string `json:"message"` | |
| } `json:"error"` | |
| } | |
| require.NoError(t, json.Unmarshal([]byte(res.Stdout), &envelope), "basecamp connect status -P %s\n%s%s", d.cfg.AgentProfile, res.Stdout, res.Stderr) | |
| require.Contains(t, envelope.Error.Message, "has no ledger yet", "basecamp connect status -P %s\n%s%s", d.cfg.AgentProfile, res.Stdout, res.Stderr) | |
| return |
| "BASECAMP_ACCOUNT_ID": true, | ||
| "BASECAMP_PROJECT_ID": true, | ||
| "BASECAMP_TODOLIST_ID": true, | ||
| "BASECAMP_NO_UPDATE_CHECK": true, |
Contributor
There was a problem hiding this comment.
P3: devEnv drops every BASECAMP_* variable that would change who a command acts as or where, but not BASECAMP_OAUTH_ISSUER. A developer with that escape hatch pinned (README describes it as a temporary dark-pilot pin) will have every dev-target command authenticate against the wrong issuer — often production — while BASECAMP_BASE_URL points at the local Basecamp, failing later and confusingly. Drop it alongside the others so the issuer is discovered from the dev Basecamp URL.
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/dev_target_test.go, line 139:
<comment>`devEnv` drops every BASECAMP_* variable that would change who a command acts as or where, but not `BASECAMP_OAUTH_ISSUER`. A developer with that escape hatch pinned (README describes it as a temporary dark-pilot pin) will have every dev-target command authenticate against the wrong issuer — often production — while `BASECAMP_BASE_URL` points at the local Basecamp, failing later and confusingly. Drop it alongside the others so the issuer is discovered from the dev Basecamp URL.</comment>
<file context>
@@ -0,0 +1,500 @@
+ "BASECAMP_ACCOUNT_ID": true,
+ "BASECAMP_PROJECT_ID": true,
+ "BASECAMP_TODOLIST_ID": true,
+ "BASECAMP_NO_UPDATE_CHECK": true,
+ }
+ if stateHome != "" {
</file context>
Suggested change
| "BASECAMP_NO_UPDATE_CHECK": true, | |
| "BASECAMP_NO_UPDATE_CHECK": true, | |
| "BASECAMP_OAUTH_ISSUER": true, |
Collaborator
Author
|
Closing: we are not adding another local Basecamp testing target or its dependent harness. The verification skill and existing tests are the chosen tools, with targeted regressions only for actual bugs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR 6 of a stack of 8 that tests
basecamp connectend to end. Stacked on #826 (the failure scenarios), which is stacked on #823 (the harness) and #822 (the fake Basecamp). It changes no product code: onlye2e/connect, the Makefile and CONTRIBUTING.md.The dev target was not run against a real Basecamp in this PR. No BC3 was reachable where it was written. It compiles, its environment parsing and helpers have unit tests, and each CLI command it runs was checked against the command code. How to run it is below.
Why
The scenarios in
e2e/connectran only against the fake Basecamp. A pass there shows that the connector agrees with the fake. It does not show that the fake agrees with Basecamp. If both run the same scenarios and both pass, the fake is held to Basecamp's behavior. Then a fake-only fault scenario means something.What
Target(e2e/connect/target_test.go) says what a scenario needs in the scenario's words:CLI()(the harness that runs commands against this Basecamp),Agent()(profile, person id, account),OperatorMentionsAgent(t)andUntrustedMentionsAgent(t). Both mention methods returnPosted, the comment as Basecamp holds it: recording id, parent (wherereply_topoints), bucket and project name, type, title,app_url, author, and the content with the agent's mention replaced by a space. The scenarios check the request line against it field by field, so the same assertions run on either target. They find their lines by recording id andcomment.created, because the event id is not known before the connector sees it on a real Basecamp.fakeTargetis the earlier behavior: the default world plus a bystander, setup through the CLI, a comment put in the world and published live.devTargetdrives a local Basecamp with the CLI under test:BASECAMP_TOKEN,BASECAMP_PROFILEand a default account, project or to-do list. Commands run in a new temporary directory, so no repository's.basecamp/config.jsonis read.auth agent connectorconnect setup, because setup rewrites the developer'sconnect.json. It reads the setup withbasecamp connect show -P <agent> --jsonand requires operator trust (setup's default) and the project served.XDG_STATE_HOME. The ledger and the per-agent instance lock are there, so a scenario cannot read or move the developer's ledger and cannot hold their lock. That also means the lock cannot keep a scenario's connector apart from the developer's real one. So it first runsbasecamp connect status -P <agent> --jsonagainst the developer's own state (status takes no lock). If a connector holds the lock, it stops with an error, because that connector would get the scenario's mentions too.messages create --project <id> --no-subscribe), and checks that the author is the personconnect.jsontrusts. It comments withcomments create <message> "... [@agent](person:<agent id>)? Thanks". The CLI's Markdown resolves that to abc-attachmentmention, and agents resolve by id. Every command gets-P <profile> --account <agent's account> --json. A cleanup runsmessages trashon the message, with its comments. The expected content comes from the comment Basecamp returned. A regex removes its one mention element, so the check does not use the connector's own stripping code.BASECAMP_BASE_URLthat is notlocalhost, a loopback address or*.localhost(hostutil.IsLocalhost), because it posts as the developer's profiles. The SDK acceptsws://cables on the same hosts.untrusted_performerwith nothing on the recording handed off, stdout only NDJSON with stderr saying it runs, SIGINT exits 130, a second connector for the same agent exits 5 at once, and a request admitted under--holdhanded off afterkill -9,connect releaseand a restart. In the second-connector scenario, the check that the refused process asked Basecamp for nothing stays on the fake: only the fake sees every request.fakeOnly(t, why): both lanes published apart, the cable down during a commit, a stale cable, a rate-limited token, failed reads, held reads (backlog), a disconnected agent (two scenarios), an agent connected on another computer, two agents side by side, andconnect setupitself. When the dev target is requested they skip with their reason, so the run lists what it did not cover.make test-connect-devchecks the five variables likerecord-cassettesand fails (it does not skip) with the missing one's name. Then it runsBASECAMP_CONNECT_DEV=1 go test -count=1 -failfast -v ./e2e/connect. It builds without-tags dev, as a release builds. Run directly withBASECAMP_CONNECT_DEV=1, the Go side fails too, and names every missing variable at once. A plaingo test ./...runs only the fake. The help list has the target.HarnessgainsDir(where commands run) besideHome, and an internalrun(ctx, ...)so a cleanup can run a command after the test's context has ended.How to run it against your local BC3
Prerequisites: the event feed enabled and Action Cable served; an operator profile logged in as you; a second profile logged in as another person on the project; an agent connected and set up to serve the project in operator trust; no connector running for that agent.
Things a first real run may show (each is a real difference, either in the fake or in an assumption here): the event type BC3 sends for a new comment, the pointer line's
lane(expectedlive, because the poll lane is about 30s behind), the commenttitleandapp_urlin the request line, and how BC3 writes a mention element in stored content.Tests
devConfigFromnames every missing variable at once and points at CONTRIBUTING.md, reads a valid environment, and refuses a non-numeric or non-positive project id, a non-local or non-http(s) base URL, and two roles on one profile.devEnvkeeps the developer's credentials (HOME, XDG_CONFIG_HOME, D-Bus), drops the token, profile and default ids, and replaces onlyXDG_STATE_HOME.withoutMentionremoves one mention element, whatever is rendered inside it (a BC3-style figure, or the fake's empty one), and refuses content with no mention or two.make test-connect-devwithBASECAMP_BASE_URLunset exits non-zero with "Set BASECAMP_BASE_URL ...".BASECAMP_CONNECT_DEV=1 go test ./e2e/connectwith nothing set fails the dev subtests with the full list of missing variables and skips the fake-only scenarios with their reasons.go test -race -count=10 ./e2e/connectpasses.bin/ciis green.Summary by cubic
Runs the connector end-to-end scenarios against a local Basecamp as well as the fake, so the fake's behavior is held against the real one. Only test code, the Makefile, and CONTRIBUTING.md change.
Targetinterface that describes each Basecamp in the scenario's own words.devTargetdrives a local Basecamp with the real CLI and the developer's own config and credentials, isolating only the connector's state (XDG_STATE_HOME) per test.fakeOnlyand are skipped on the dev target, with their reason.make test-connect-devruns the dev target and fails, not skips, when a required environment variable is missing.Written for commit 7a8632d. Summary will update on new commits.