[robzolkos]

PR 5 of a stack of 8 that tests basecamp connect end to end. Stacked on #823 (the e2e harness), which is stacked on #822 (the fake Basecamp). It changes no behaviour of a release build.

Why

The connector hands off a request only if it was made at most HandoffGrace (one minute) before the run started. A request older than that is closed as before_this_run: nobody was there to take it, and it is not run late.

That matters most after a crash between admission and handoff. The connector admits a request, dies before it writes the request line, and starts again. Within the grace, the request is handed off (PR 3 tests that, with the default grace). After the grace, it must be closed and never written. Nothing tested that second half end to end, because a test would have to wait out a minute.

What

• HandoffOptions.Grace (internal/connector/handoff.go). Zero means HandoffGrace, so every caller that does not set it keeps the minute. The handoff reads it through a small grace() method, as it reads its logger.
• A dev-only knob. connect_handoff_grace_dev.go (//go:build dev) reads BASECAMP_CONNECT_HANDOFF_GRACE as a Go duration. connect_handoff_grace_stub.go (//go:build !dev) reads no environment at all and returns zero, so a release build (goreleaser builds without -tags dev) cannot be affected. This is the same pattern as dev_tag.go / dev_tag_stub.go. There is no hidden flag.
• runConnect reads it before it touches the account. A value that is not a positive duration is a usage error. A run with the override logs connector: the handoff grace is overridden for this dev build on stderr, with the grace and the default.
• Log order. The handoff logged a request from before this run started is not handed off before the ledger write. A run stopped between the two left the record admitted while its log said it was closed. The e2e test found this. The log now comes after the write.

Tests

• e2e/connect/grace_test.go (a new file; no harness changes). With a 2s grace: connect --hold, a trusted mention, wait for its admitted event line, kill -9, basecamp connect release -P agent, then a restart. On the restart there is no request line, the connector logs the close for that event id, and connect status --json shows no admitted or queued record.
  • No sleeping. The fake (WithClock) stamps the request 10s in the past instead of the test waiting out the grace. To the connector it is the same request: the handoff judges only the event's created_at, as the feed served it, against when the run started, and nothing else in intake or admission reads created_at. Before the restart, the test checks from the pointer line's created_at that the request is already older than the grace.
  • It needs the knob. 10s is well inside the default minute, so without the shortened grace the request would be handed off and the test would fail.
  • Without -tags dev the test skips and says why: a release build reads no environment for the grace. make test, bin/ci, the unit test job, the race shards and the release workflow all run go test -tags dev, so it always runs there.
• handoff_test.go: a grace of 30s hands off a request made 30s before the run (the boundary included), a grace of 29s closes it as before_this_run, and no grace means HandoffGrace.
• connect_handoff_grace_dev_test.go (dev-tagged): the parse accepts Go durations and empty (zero), refuses 2, soon, 0s and -1s with the variable's name in the error, and reads the environment.

go test -tags dev -race -count=10 passes on the new tests. bin/ci is green.

---

Summary by cubic

Lets dev builds shorten the connector's handoff grace so an end-to-end test can reach a crash-after-grace restart in seconds; release builds read no environment for the grace and keep the one-minute default.

• Adds HandoffOptions.Grace; zero means the existing HandoffGrace, and only a dev build sets it from BASECAMP_CONNECT_HANDOFF_GRACE, with invalid values a usage error.
• Moves the before_this_run log after the ledger write, so a stopped run no longer logs a close it has not yet written.
• Adds unit tests for the boundary and default and an e2e test that holds and kills a run, then restarts past a 2s grace; the fake backdates the request instead of the test sleeping.

^{Written for commit 0045407. Summary will update on new commits.}

[Copilot]

Copilot review overview

🟢 Approved

The implementation preserves release defaults and provides focused unit and end-to-end coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Adds a dev-only handoff-grace override to enable fast restart testing while preserving release behavior.

Changes:

• Makes handoff grace configurable, retaining the one-minute default.
• Adds dev-only environment parsing and override logging.
• Tests grace boundaries and crash/restart behavior end to end.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File	Description
internal/​connector/​handoff.go	Adds configurable grace and corrects log/write ordering.
internal/​connector/​handoff_test.go	Tests custom, boundary, and default grace behavior.
internal/​commands/​connect_run.go	Wires the dev override into connector startup.
internal/​commands/​connect_handoff_grace_stub.go	Keeps release builds unaffected.
internal/​commands/​connect_handoff_grace_dev.go	Parses the dev-only environment override.
internal/​commands/​connect_handoff_grace_dev_test.go	Tests duration parsing and environment reading.
e2e/​connect/​grace_test.go	Tests discarding an expired admitted request after restart.

---

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

[cubic-dev-ai]

2 issues found across 7 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/commands/connect_handoff_grace_dev.go">

<violation number="1" location="internal/commands/connect_handoff_grace_dev.go:33">
P2: An explicit zero override is rejected, despite zero being the documented way to keep the one-minute default. Reject only negative durations so `0s` can reach the handoff's default behavior.</violation>
</file>

<file name="internal/commands/connect_run.go">

<violation number="1" location="internal/commands/connect_run.go:192">
P3: Shadow runs never invoke `RunHandoff`, so this warning claims an override is active when it has no effect. Suppress it for shadow runs.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: An explicit zero override is rejected, despite zero being the documented way to keep the one-minute default. Reject only negative durations so 0s can reach the handoff's default behavior.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/connect_handoff_grace_dev.go, line 33:

<comment>An explicit zero override is rejected, despite zero being the documented way to keep the one-minute default. Reject only negative durations so `0s` can reach the handoff's default behavior.</comment>

<file context>
@@ -0,0 +1,37 @@
+	if err != nil {
+		return 0, fmt.Errorf("%s is not a duration: %w", connectHandoffGraceEnv, err)
+	}
+	if d <= 0 {
+		return 0, fmt.Errorf("%s must be a positive duration, not %s", connectHandoffGraceEnv, v)
+	}
</file context>

Suggested change

	if d <= 0 {
	if d < 0 {

[cubic-dev-ai]

P3: Shadow runs never invoke RunHandoff, so this warning claims an override is active when it has no effect. Suppress it for shadow runs.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/connect_run.go, line 192:

<comment>Shadow runs never invoke `RunHandoff`, so this warning claims an override is active when it has no effect. Suppress it for shadow runs.</comment>

<file context>
@@ -184,6 +189,9 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error {
 	}
 
 	logger := slog.New(slog.NewTextHandler(cmd.ErrOrStderr(), nil))
+	if grace > 0 {
+		logger.Warn("connector: the handoff grace is overridden for this dev build", "grace", grace, "default", connector.HandoffGrace)
+	}
</file context>

Suggested change

	if grace > 0 {
	if grace > 0 && !f.shadow {

[jeremy]

🤖 Lint is red only because of #822's G120 finding. I checked locally: with that fixed and #822, #823 and this PR rebased onto main, lint is clean, and the internal/connector, internal/commands and e2e/connect tests pass under -tags dev, including the restart grace test. Two cubic threads are still open: 0s is rejected, and the warning prints on --shadow runs. Fix or decline each, then resolve.

[robzolkos]

Closing the testing infrastructure and dev-only grace override as agreed. The small log-order correctness fix is preserved with a before/after regression in #960. Existing tests and focused verification are retained; no new harness is being added.