ci(e2e): add local GitHub emulator driver for scm.Driver tests

[waynesun09]

Summary

Adds a scm.Driver implementation backed by a locally spawned vercel-labs/emulate GitHub instance, for tests that only need SCM-level state (issues, labels, comments) — no live GitHub org, no mint, no secrets. Standalone by design: not registered as a BEHAVIOUR_SCM value, since emulate's Actions endpoints are record-level only and can never back a real ci.Driver.

Related Issue

None filed — follow-up on the Layer-2 (mocked-external-dependency) testing gap named in ADR 0052 and loosely tracked under #73.

Changes

• e2e/behaviour/drivers/scm/emulate/: Instance/Start/Close spawn a version-pinned npx emulate@0.8.0 subprocess, health-check it via /rate_limit, and wire forge/github.LiveClient at its base URL via WithBaseURL — reuses the existing scm/github driver unmodified.
• .github/workflows/scm-emulate.yml: new secret-free CI job on plain pull_request (no gate dependency, safe for fork PRs), runs go test -tags behaviour scoped to this package only.
• docs/guides/dev/behaviour-drivers.md: documents the package and explains why it deliberately does not follow the "Adding an SCM driver" checklist (CI-driver mismatch).

Testing

• make lint passes — verified go-fmt, go-vet, pinact (SHA-pin check), and actionlint individually against the changed files (no golangci-lint step in this repo's CI)
• Tests added — emulate_test.go exercises create issue → add label → read back → add comment → close, against a real spawned emulator instance; passes locally in under a second

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• Commits are signed off (DCO) — human and human-directed agent sessions only
• I wrote this contribution myself and can explain all changes in it

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:33 AM UTC · Completed 2:43 AM UTC
Commit: 68834f5 · View workflow run →

[qodo-code-review]

PR Summary by Qodo

ci(e2e): add local emulate-backed SCM driver for behaviour tests

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 📝 Documentation 🕐 20-40 Minutes

AI Description

• Add a local GitHub emulator-backed SCM driver to test issue/label/comment flows without secrets.
• Add behaviour-tagged integration tests that exercise real multi-endpoint GitHub semantics via
 emulate.
• Add a secret-free GitHub Actions workflow and document the driver’s intended scope/limitations.

Diagram

graph TD
  W["GitHub Actions: scm-emulate.yml"] --> T["Go tests (behaviour tag)"] --> E["scm/emulate (Start/Close)"] --> P(["npx emulate@0.8.0 subprocess"]) --> A{{"Emulated GitHub API"}} --> C["forge/github LiveClient (WithBaseURL)"] --> G["scm/github driver"] --> T
  E --> S[("seed YAML (temp)")] --> P

  subgraph Legend
    direction LR
    _mod["Module/Test"] ~~~ _proc(["Process"]) ~~~ _file[("File")] ~~~ _ext{{"External API"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Run emulate as a pinned container image (Docker)

• ➕ Eliminates reliance on host Node/npx availability
• ➕ Potentially more reproducible runtime environment across CI and dev machines
• ➖ Adds Docker-in-Docker/container runtime requirements to CI runners
• ➖ More moving parts than npx --yes for a small, scoped test suite

2. Use `httptest` + hand-stubbed REST responses

• ➕ No external process startup; tests are fully in-process and fast
• ➕ No Node toolchain needed
• ➖ High maintenance as GitHub API surface evolves
• ➖ Misses multi-endpoint behavior/consistency (create→label→read-back) that this PR specifically targets

3. Register as a full BEHAVIOUR_SCM backend and run in suite

• ➕ Uniform suite entrypoint and env-based selection
• ➕ Potentially broader adoption for behaviour tests
• ➖ Risk of misleading green suites when paired with CI-driver assertions (emulate doesn’t execute workflows)
• ➖ Would require extra guardrails to prevent invalid SCM/CI combinations

Recommendation: Keep the PR’s current approach: a standalone, directly-imported SCM driver that composes the existing scm/github implementation via WithBaseURL, plus a dedicated secret-free CI job. It maximizes realism for SCM-only state (issues/labels/comments) while avoiding the semantic trap of implying it can back ci.Driver/workflow-execution assertions. If CI portability becomes an issue due to Node/npx availability, consider the containerized emulate alternative later.

Files changed (5) +429 / -1

Enhancement (2) +265 / -0

emulate.goImplement emulator instance lifecycle and compose scm/github via base URL +174/-0

Implement emulator instance lifecycle and compose scm/github via base URL

• Adds an 'Instance' that spawns a version-pinned 'npx emulate@0.8.0' subprocess, writes a seed file, waits for health via '/rate_limit', and constructs a 'forge/github.LiveClient' targeting the emulator. Embeds the existing 'scm/github' driver so SCM operations reuse production driver code paths unchanged, and provides 'Close()' cleanup.

e2e/behaviour/drivers/scm/emulate/emulate.go

seed.goGenerate emulate seed YAML for org/repo/user/token fixtures +91/-0

Generate emulate seed YAML for org/repo/user/token fixtures

• Adds 'SeedOptions' with defaults and renders a minimal emulate seed configuration to a temp YAML file. Ensures the token login maps to a seeded user and creates an auto-initialized repo, returning the seed path and token used by the Go forge client.

e2e/behaviour/drivers/scm/emulate/seed.go

Tests (1) +89 / -0

emulate_test.goAdd behaviour-tagged integration tests against the local emulator +89/-0

Add behaviour-tagged integration tests against the local emulator

• Adds package-level 'TestMain' to start one shared emulator instance per test binary and skip when 'npx' is unavailable. Implements end-to-end SCM assertions for issue creation, labeling, commenting, and closing using the composed 'scm.Driver'.

e2e/behaviour/drivers/scm/emulate/emulate_test.go

Documentation (1) +23 / -1

behaviour-drivers.mdDocument the new local 'scm/emulate' driver and usage constraints +23/-1

Document the new local 'scm/emulate' driver and usage constraints

• Adds a section describing the emulate-backed SCM driver, why it is intentionally not wired into the BEHAVIOUR_SCM matrix, and how to use it for SCM-only state tests. Updates testing guidance to prefer 'scm/emulate' for multi-endpoint SCM scenarios over 'httptest' stubs or live credentials.

docs/guides/dev/behaviour-drivers.md

Other (1) +52 / -0

scm-emulate.ymlAdd secret-free CI job for scm/emulate behaviour tests +52/-0

Add secret-free CI job for scm/emulate behaviour tests

• Introduces a new GitHub Actions workflow triggered on main push/PR changes in the SCM driver/emulator-related paths. Installs Go and Node, then runs 'go test -tags behaviour' scoped to the emulator driver package to keep fork PRs safe (no secrets, no pull_request_target).

.github/workflows/scm-emulate.yml

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (2)

Context used

✅ Tickets: 🎫 Add regression tests / evals for all agents and skills

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

1. Silent skip without npx ✓ Resolved 🐞 Bug ☼ Reliability

Description

TestMain exits with code 0 if npx is not found, so CI can go green without running any tests if
Node setup breaks or PATH is misconfigured. This undermines the workflow’s purpose by allowing
false-positive passes.

Code

e2e/behaviour/drivers/scm/emulate/emulate_test.go[R33-37]

+func TestMain(m *testing.M) {
+	if _, err := exec.LookPath("npx"); err != nil {
+		// No Node/npx on PATH — skip the whole binary rather than fail.
+		os.Exit(0)
+	}

Relevance

⭐⭐⭐ High

Repo has accepted work to avoid CI silent skips/false-green behavior; os.Exit(0) likely
unacceptable.

PR-#2398
PR-#3425

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test binary explicitly exits 0 when npx is missing, while the CI workflow relies on setup-node
and then runs this package’s tests—meaning a broken Node setup could still yield a successful job
without executing tests.

e2e/behaviour/drivers/scm/emulate/emulate_test.go[33-37]
.github/workflows/scm-emulate.yml[47-52]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test suite exits successfully when `npx` is missing, which can mask CI/environment regressions and produce false-green runs.

### Issue Context
The workflow explicitly installs Node, so a missing `npx` should be treated as an error in CI.

### Fix Focus Areas
- e2e/behaviour/drivers/scm/emulate/emulate_test.go[33-37]
- .github/workflows/scm-emulate.yml[47-52]

### Implementation sketch
- Detect CI (e.g. `if os.Getenv("GITHUB_ACTIONS") == "true" || os.Getenv("CI") == "true"`) and `os.Exit(1)` when `npx` is missing.
- For local runs, keep the current skip behavior or gate it behind an env var like `ALLOW_EMULATE_SKIP=1` so skipping is explicit.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unreaped killed subprocess ✓ Resolved 🐞 Bug ☼ Reliability

Description

If the emulator fails to become healthy, Start kills the started process but never calls
cmd.Wait(), which can leave a zombie process/resource leak. This is especially problematic in
longer-running test binaries or repeated retries.

Code

e2e/behaviour/drivers/scm/emulate/emulate.go[R96-101]

+	baseURL := fmt.Sprintf("http://127.0.0.1:%d", port)
+	if err := waitHealthy(ctx, baseURL); err != nil {
+		_ = cmd.Process.Kill()
+		_ = os.Remove(seedPath)
+		return nil, fmt.Errorf("emulate: did not become healthy: %w", err)
+	}

Relevance

⭐⭐ Medium

No prior repo evidence requiring cmd.Wait() after Kill() in failure paths.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The startup failure path explicitly kills the process and returns without any cmd.Wait() call,
while the normal Close() path does wait; this makes the unhealthy-start path inconsistent and
potentially leaky.

e2e/behaviour/drivers/scm/emulate/emulate.go[96-101]
e2e/behaviour/drivers/scm/emulate/emulate.go[122-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
On startup failure, `Start()` kills the subprocess but does not reap it with `cmd.Wait()`. A started child process should always be waited on to ensure OS resources are released.

### Issue Context
This happens specifically on the `waitHealthy` error path.

### Fix Focus Areas
- e2e/behaviour/drivers/scm/emulate/emulate.go[96-101]

### Implementation sketch
- After `cmd.Process.Kill()`, call `cmd.Wait()` (best-effort), e.g.:
 - `if cmd.Process != nil { _ = cmd.Process.Kill(); _ = cmd.Wait() }`
- Consider factoring cleanup into a helper so both the error-path and `Close()` share consistent shutdown semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Timeout kills emulator ✓ Resolved 🐞 Bug ☼ Reliability

Description

emulate.Start uses exec.CommandContext(ctx, ...), so the caller context controls the emulator
subprocess lifetime; in TestMain that context has a 30s timeout, which can kill the emulator
mid-test under slow CI or expanded test suites. This can create flaky failures that look like random
GitHub API errors/timeouts.

Code

e2e/behaviour/drivers/scm/emulate/emulate.go[R66-81]

+func Start(ctx context.Context, opts SeedOptions, logf func(string, ...any)) (*Instance, error) {
+	port, err := freePort()
+	if err != nil {
+		return nil, fmt.Errorf("emulate: reserving port: %w", err)
+	}
+
+	seedPath, token, err := writeSeedFile(opts)
+	if err != nil {
+		return nil, fmt.Errorf("emulate: writing seed config: %w", err)
+	}
+
+	cmd := exec.CommandContext(ctx, "npx", "--yes", emulatePackage,
+		"--service", "github",
+		"--port", strconv.Itoa(port),
+		"--seed", seedPath,
+	)

Relevance

⭐⭐ Medium

No historical evidence on avoiding short-lived CommandContext killing long-running test
subprocesses.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Start creates the subprocess with exec.CommandContext(ctx, ...) so ctx governs the process
lifetime, and TestMain passes a 30-second timeout context into Start, meaning the subprocess can
be killed when that deadline expires even if tests are still running.

e2e/behaviour/drivers/scm/emulate/emulate.go[66-81]
e2e/behaviour/drivers/scm/emulate/emulate_test.go[39-45]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Start()` binds the emulator subprocess lifetime to the passed `ctx` via `exec.CommandContext`. When the caller uses a short timeout context for startup, that deadline continues to apply after startup and can terminate the emulator while tests are still running.

### Issue Context
In `emulate_test.go`, `TestMain` uses a 30s timeout context intended for startup, but that same context is passed into `Start()` and therefore governs the process lifetime.

### Fix Focus Areas
- e2e/behaviour/drivers/scm/emulate/emulate.go[66-112]
- e2e/behaviour/drivers/scm/emulate/emulate_test.go[33-55]

### Implementation sketch
- In `Start`, use a long-lived context for `exec.CommandContext` (e.g. `procCtx, procCancel := context.WithCancel(context.Background())`) and store `procCancel` on `Instance` for `Close()`.
- Use a separate startup/health-check context with timeout (e.g. `startCtx, cancel := context.WithTimeout(ctx, startTimeout)` or a dedicated `ctx` just for `waitHealthy`).
- Update `TestMain` to avoid using a deadline-bound context to control process lifetime.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. Architecture rationale stated inline 📜 Skill insight ⚙ Maintainability

Description

The guide explains architectural constraints/behavioral limitations of the emulator driver inline
but does not link to an ADR/spec/docs/architecture.md for that context. This violates the
requirement to link to architectural references instead of restating them in guides.

Code

docs/guides/dev/behaviour-drivers.md[R55-60]

+This package deliberately does **not** follow the "Adding an SCM driver" checklist above — it
+is not registered as a `BEHAVIOUR_SCM` value and is not wired into `suite_test.go`. emulate's
+Actions endpoints are REST record-level only (list/get/dispatch/cancel/logs as data); it does
+not execute real workflow YAML. So it can never satisfy `ci.Driver` for scenarios like
+`triage.feature` that assert on real agent execution — pairing it with `BEHAVIOUR_CI=githubactions`
+would silently produce a suite that can never observe what it's supposed to test. Import it

Relevance

⭐⭐ Medium

No clear prior reviews enforcing “link ADR vs inline architecture rationale” in guides.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062081 requires documentation guides to link to ADRs/specs/docs/architecture.md
for architectural context rather than explaining architecture inline. The added section provides
architectural rationale about emulator vs ci.Driver but provides no such reference link.

docs/guides/dev/behaviour-drivers.md[55-60]
Skill: writing-user-docs

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This guide includes architectural reasoning about why the local emulator cannot satisfy `ci.Driver`, but it does not link to an architectural reference (ADR/spec/`docs/architecture.md`).

## Issue Context
The newly added `scm/emulate` section describes emulator limitations and test-layer intent. Per docs standards, guides should link to architectural references rather than restating them inline.

## Fix Focus Areas
- docs/guides/dev/behaviour-drivers.md[55-60]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Health check can hang ✓ Resolved 🐞 Bug ☼ Reliability

Description

healthCheck uses http.DefaultClient with no timeout, so if Start is called with a context that
has no deadline and the endpoint stalls, a single Do() can block indefinitely and startTimeout
won’t bound startup time. This can hang test binaries/CI jobs instead of failing fast.

Code

e2e/behaviour/drivers/scm/emulate/emulate.go[R147-158]

+func healthCheck(ctx context.Context, baseURL string) bool {
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, baseURL+"/rate_limit", nil)
+	if err != nil {
+		return false
+	}
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		return false
+	}
+	defer resp.Body.Close()
+	return resp.StatusCode == http.StatusOK
+}

Relevance

⭐ Low

Team previously rejected adding explicit http.Client timeouts over http.DefaultClient in e2e tests.

PR-#1215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

healthCheck performs the request via http.DefaultClient (no timeout), while waitHealthy relies
on repeated checks until startTimeout—but a single stuck Do() prevents the loop from
continuing/expiring.

e2e/behaviour/drivers/scm/emulate/emulate.go[132-158]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`healthCheck()` uses `http.DefaultClient.Do(req)` without a client timeout. If the caller context has no deadline, the request can hang indefinitely, defeating `waitHealthy()`'s loop timeout.

### Issue Context
`waitHealthy()` intends to limit startup to `startTimeout`, but that only works if each individual health check attempt returns promptly.

### Fix Focus Areas
- e2e/behaviour/drivers/scm/emulate/emulate.go[132-158]

### Implementation sketch
- Use a dedicated `http.Client{Timeout: ...}` for health checks (e.g. 1s) instead of `http.DefaultClient`.
- Or wrap each attempt with `ctxAttempt, cancel := context.WithTimeout(ctx, 1*time.Second)` and use that context in `NewRequestWithContext`.
- Optionally read/discard response body before closing to avoid any resource issues on non-200 responses.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Planned work lacks > **Planned:** 📜 Skill insight ≡ Correctness

Description

The guide mentions not-yet-implemented work (future dispatch-routing test, eval/ fixture setup)
without using the required > **Planned:** callout format and without an issue link. This makes
planned functionality easy to misread as current behavior and violates the documentation callout
requirement.

Code

docs/guides/dev/behaviour-drivers.md[R61-62]

+directly in Go test code that only needs SCM-level state (issues, labels, comments) and no
+live Actions run — e.g. a future dispatch-routing test, or `eval/`-layer fixture setup.

Relevance

⭐ Low

Similar “Planned” callout/issue-link requirement was rejected in docs review.

PR-#3903

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062082 requires any planned/not-yet-implemented functionality to be documented
using a > **Planned:** blockquote callout and to include a link to the relevant issue. The added
text mentions future work without that callout or an issue link.

docs/guides/dev/behaviour-drivers.md[61-62]
Skill: writing-user-docs

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The guide mentions future work without the required `> **Planned:**` callout format and without linking to an issue.

## Issue Context
In `docs/guides/dev/behaviour-drivers.md`, the new `scm/emulate` section includes examples of future tests / fixture setup.

## Fix Focus Areas
- docs/guides/dev/behaviour-drivers.md[61-62]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

2. Architecture rationale stated inline 📜 Skill insight ⚙ Maintainability

The guide explains architectural constraints/behavioral limitations of the emulator driver inline
but does not link to an ADR/spec/docs/architecture.md for that context. This violates the
requirement to link to architectural references instead of restating them in guides.

Agent Prompt

## Issue description
This guide includes architectural reasoning about why the local emulator cannot satisfy `ci.Driver`, but it does not link to an architectural reference (ADR/spec/`docs/architecture.md`).

## Issue Context
The newly added `scm/emulate` section describes emulator limitations and test-layer intent. Per docs standards, guides should link to architectural references rather than restating them inline.

## Fix Focus Areas
- docs/guides/dev/behaviour-drivers.md[55-60]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[waynesun09]

Review

Verdict: request-changes. (Posted as a comment, not a formal GitHub review state — GitHub does not allow requesting changes on your own PR, and this review is running under the PR author's own credentials rather than a separate bot identity.)

Findings

High

• [protected-path] .github/workflows/scm-emulate.yml — This PR adds a new file under .github/, a protected path per this repo's review policy, and has no linked issue. Human approval is always required for protected-path changes; with no linked issue (only a PR-body reference to ADR 0052 and issue #73, neither of which specifically authorizes this workflow — ADR 0052 calls the underlying Layer 2 testing gap "an open opportunity," not an approved plan, and #73 is about agent-prompt/skill regression evals, not SCM driver infra), this is insufficient context per policy.
  Remediation: File or link an issue that authorizes this CI workflow addition (and ideally scopes the whole package — see the related process finding below), or get explicit maintainer sign-off before merging.

Low

• [api-contract] e2e/behaviour/drivers/scm/emulate/emulate.go:129 — Close() calls Process.Kill() then returns cmd.Wait(), which returns a non-nil *exec.ExitError ("signal: killed") on every clean shutdown since the process was terminated by our own signal. The only caller (TestMain) discards the error, so it's latent today, but Close() is exported and documented as the normal shutdown API — a future caller checking the error will fail spuriously on every successful teardown.
  Remediation: Treat a signal-kill exit as success (ignore Wait()'s error, or check that ProcessState reflects the signal we sent) so Close() only surfaces genuine failures.

• [resource-leak] e2e/behaviour/drivers/scm/emulate/emulate.go:97-101 — The waitHealthy failure path in Start calls Process.Kill() and removes the seed file but never calls cmd.Wait(), unlike Close(). This leaks the stderr pipe fd and leaves an un-reaped child for every failed Start (e.g. a slow or unhealthy emulator).
  Remediation: Call cmd.Wait() (ignoring its error) after Kill() in this path too, mirroring Close().

• [edge-case] e2e/behaviour/drivers/scm/emulate/emulate_test.go:39 — TestMain's context.WithTimeout(30s) drives both the startup health-check and (via exec.CommandContext in Start) the subprocess's entire lifetime, since it's the same ctx. Harmless with today's two sub-second tests, but as more tests are added to this binary (the docs explicitly invite a future dispatch-routing test), the 30s cap will silently kill the emulator mid-run.
  Remediation: Use context.Background() (or similar) for the subprocess lifetime, and a separate short-lived context only for the startup health-check loop.

• [injection] e2e/behaviour/drivers/scm/emulate/emulate.go:77 — npx --yes emulate@0.8.0 fetches and executes an unscoped npm package on every pull_request run (including fork PRs). The version is pinned but not integrity/hash-pinned; blast radius is limited by the job's read-only token and lack of secrets, but a compromised publish under that name/version would still run arbitrary code on the runner.
  Remediation: Pin by integrity hash (e.g. install from a committed lockfile with npm ci instead of npx --yes), or otherwise verify package integrity before execution.

• [process] e2e/behaviour/drivers/scm/emulate/emulate.go — This is a sizeable standalone addition (5 files, ~430 lines) whose only consumer in this PR is its own test; it's deliberately not wired into suite_test.go or BEHAVIOUR_SCM. The stated future consumers (a dispatch-routing test, eval/ fixtures) don't exist yet. Worth an issue that scopes this package's intended near-term use, both to close this gap and to satisfy the protected-path finding above.
  Remediation: File a scoping issue and link it, or get explicit maintainer sign-off that this is intentionally speculative infrastructure.

Info

• [race-condition] e2e/behaviour/drivers/scm/emulate/emulate.go:159 — freePort()'s listen-then-close-then-let-subprocess-bind pattern has a standard TOCTOU race; low impact for a single serial test binary today.
• [permission-reduction] .github/workflows/scm-emulate.yml:33 — permissions: contents: read is correctly least-privilege for this job; no action needed.
• [secret-exposure] e2e/behaviour/drivers/scm/emulate/seed.go:13 — The //nolint:gosec-annotated token constant is verified to never authenticate against anything but the local emulator; not a real secret.
• [architecture-fit] docs/guides/dev/behaviour-drivers.md:55 — The deliberate deviation from the "Adding an SCM driver" checklist is well-reasoned and documented; not a concern.

[fullsend-ai-review]

Review

Clean, well-documented addition of a local GitHub emulator driver for scm.Driver tests. The package reuses the existing scm/github driver via forge/github.LiveClient.WithBaseURL, so the real code paths get exercised against a local emulator — solid architectural choice that aligns with ADR 0052 which explicitly identifies vercel-labs/emulate as useful for mocked-external-dependency testing. The CI workflow correctly uses pull_request (not pull_request_target) with minimal contents: read permissions and no secrets. The prior medium finding (Close() always returning non-nil error) has been resolved — killAndReap now handles kill+wait internally with a clear comment explaining why Wait's error is discarded. The TestMain CI detection was also improved: missing npx in CI now fails loud.

Findings

High

• [protected-path] .github/workflows/scm-emulate.yml — PR adds a new file under .github/ (protected path) with no linked issue providing authorization. The PR description adequately explains the workflow's purpose and security posture, but protected-path policy requires a linked issue. Human approval is required for all protected-path changes, regardless of the review outcome.

Low

#	Category	File	Description
1	race-condition	emulate.go	TOCTOU race in freePort(): the port is released before npx emulate binds it. Low probability on single-purpose CI runners, but could cause flaky "did not become healthy" failures.
2	test-adequacy	emulate_test.go	TestMain calls os.Exit(0) when npx is not on PATH locally — silent skip with zero tests run. Improved from prior review: CI now correctly fails loud via inCI() check.
3	test-adequacy	emulate_test.go	CommitFile — the most complex scm.Driver method — is tested but skipped (t.Skip) due to upstream emulate response-shape bug (vercel-labs/emulate#190). Already covered against live GitHub via the existing behaviour suite.
4	test-integrity	emulate_test.go	TestCommentAndClose cannot verify CloseIssue's effect: forge.Issue has no State field. Documented in the test comment as a known limitation.
5	error-handling	emulate.go	If the emulate subprocess exits immediately, waitHealthy blocks for 15 s before returning an opaque timeout error. Detecting early process exit would speed up debugging.
6	supply-chain	emulate.go	npx --yes emulate@0.8.0 without integrity verification. Accepted risk: workflow grants no secrets, uses pull_request with read-only GITHUB_TOKEN.
7	network-exposure	emulate.go	Emulator binding address not explicitly passed to subprocess; freePort() binds 127.0.0.1 but the emulator's own default is not verified. Low-impact since the emulator serves test fixture data with no real credentials.
8	scope-authorization	N/A	No linked issue. PR references ADR 0052 and #73 as context — ADR 0052 explicitly identifies vercel-labs/emulate for Layer-2 infrastructure, so the reference is consistent, but the work itself (driver unit tests) is not the full Layer-2 prompt eval framework that #73 tracks.

Resolved from prior review

Prior finding	Resolution
Medium — Close() always returns non-nil error from cmd.Wait() after Kill()	Fixed: killAndReap() now handles kill+wait internally and discards the expected "signal: killed" error. Close() returns nil. Clear comment explains the design.
Low — TestMain exits 0 in CI when npx missing	Improved: inCI() check now differentiates CI (exits 1) from local (exits 0).

Previous run

Review

Findings

High

• [protected-path] .github/workflows/scm-emulate.yml — PR modifies a file under .github/ (protected path) with no linked issue providing authorization. Human approval is required for all protected-path changes, regardless of the review outcome.

Low

#	Category	File	Description
1	race-condition	emulate.go:143	TOCTOU race in freePort(): the port is released before npx emulate binds it. Low probability on single-purpose CI runners, but could cause flaky "did not become healthy" failures.
2	test-adequacy	emulate_test.go:30	TestMain calls os.Exit(0) when npx is not on PATH — CI reports success with zero tests run if setup-node fails silently.
3	test-adequacy	emulate_test.go	CommitFile — the most complex scm.Driver method (multi-step Git Trees/Blobs/Commits API) — is not exercised. Reasonable to defer, but it is the likeliest code path to diverge between real GitHub and the emulator.
4	test-integrity	emulate_test.go:85	TestCommentAndClose cannot verify CloseIssue's effect: forge.Issue has no State field. Documented in the test comment as a known limitation.
5	error-handling	emulate.go:121	If the emulate subprocess exits immediately, waitHealthy blocks for 15 s before returning an opaque timeout error. Detecting early process exit would speed up debugging.
6	supply-chain	emulate.go:49	npx --yes emulate@0.8.0 without integrity verification. Accepted risk: workflow grants no secrets, uses pull_request with read-only GITHUB_TOKEN.
7	network-exposure	emulate.go:50	Emulator binding address not explicitly passed to subprocess; code assumes 127.0.0.1. Low-impact since the emulator serves test fixture data with no real credentials.
8	scope-authorization-mismatch	N/A	PR body references ADR 0052 / #73 as context for Layer-2 testing gap; implementation is driver unit test infrastructure. PR documentation is internally consistent about its purpose. Consider clarifying the PR description.
9	documentation	emulate.go:4	Package comment focuses on external tool rather than the package's role in the system.
10	struct-organization	emulate.go:29	Instance embeds scm.Driver; established driver pattern uses a named Client field. Deviation is intentional (lifecycle wrapper vs. direct implementation).
11	missing-doc	behaviour-drivers.md	New section documents the package well but could include a brief code example showing Start/Close lifecycle. Test file (emulate_test.go) serves as de facto example.

Previous run

Review — comment

Clean, well-documented addition of a local GitHub emulator driver for scm.Driver tests. The package reuses the existing scm/github driver via forge/github.LiveClient.WithBaseURL, so the real code paths get exercised against a local emulator — solid architectural choice. The CI workflow correctly uses pull_request with minimal contents: read permissions, and the docs update in behaviour-drivers.md thoroughly explains the design trade-offs (why it's not a BEHAVIOUR_SCM value, no reset endpoint, per-test isolation via unique issue numbers).

Findings

Medium

#	Category	File	Description
1	error handling	e2e/behaviour/drivers/scm/emulate/emulate.go	Close() always returns a non-nil error. After Kill(), cmd.Wait() returns the signal-killed exit status, so Close() never returns nil on the happy path. Current callers discard the error (_ = inst.Close()), but any future caller checking the return value will see a spurious error. Consider: _ = i.cmd.Wait(); return nil or filter *exec.ExitError after Kill.

Low

#	Category	File	Description
2	race condition	e2e/behaviour/drivers/scm/emulate/emulate.go	TOCTOU race in freePort(): the port is released before npx emulate --port binds it. Low probability on single-purpose CI runners, but could cause flaky "did not become healthy" failures if another process claims the port in the gap.
3	resource leak	e2e/behaviour/drivers/scm/emulate/emulate.go	If Close() is never called (e.g., TestMain panics after Start returns), subprocess cleanup depends on context cancellation or process exit. The streamLog goroutine also leaks. Low-impact for test infrastructure where process exit handles cleanup.
4	test adequacy	e2e/behaviour/drivers/scm/emulate/emulate_test.go	CommitFile — the most complex scm.Driver method (multi-step Git Trees/Blobs/Commits API) — is not exercised. Reasonable to defer to a follow-up, but it's the code path most likely to behave differently between real GitHub and the emulator.
5	edge case	e2e/behaviour/drivers/scm/emulate/emulate_test.go	TestMain calls os.Exit(0) when npx is not on PATH, so CI reports success (exit 0) with zero tests run. If actions/setup-node fails silently, all tests pass vacuously. Consider logging a SKIP: marker to stderr.
6	error handling	e2e/behaviour/drivers/scm/emulate/emulate.go	If the emulate subprocess exits immediately (bad seed, missing package), waitHealthy blocks for 15 s before returning an opaque timeout error. Detecting early process exit and surfacing the exit status would speed up debugging.
7	test integrity	e2e/behaviour/drivers/scm/emulate/emulate_test.go	TestCommentAndClose cannot verify CloseIssue's effect since forge.Issue has no State field — it only asserts the PATCH succeeded. Documented in the test comment as a known limitation.
8	supply-chain	e2e/behaviour/drivers/scm/emulate/emulate.go	npx --yes emulate@0.8.0 pulls an npm package at runtime without integrity verification. The version pin mitigates drive-by upgrades but does not verify package integrity. Low because the repo already uses npx without integrity hashes elsewhere (e.g., wrangler in site-deploy.yml).
9	network-exposure	e2e/behaviour/drivers/scm/emulate/emulate.go	Worth confirming that emulate binds exclusively to 127.0.0.1 (loopback) and not 0.0.0.0. On a shared CI runner, binding to all interfaces would expose the unauthenticated emulator API to other jobs.

Overall this is a well-scoped, well-documented PR. The medium finding (Close error handling) is the only item worth addressing before merge; the low findings are suggestions for hardening.

---

Labels: PR adds e2e test driver infrastructure with a new CI workflow

Previous run

Review

Findings

High

• [protected-path] .github/workflows/scm-emulate.yml — PR modifies a file under .github/ (protected path) with no linked issue providing authorization. Human approval is required for all protected-path changes, regardless of the review outcome.

Low

#	Category	File	Description
1	race-condition	emulate.go:143	TOCTOU race in freePort(): the port is released before npx emulate binds it. Low probability on single-purpose CI runners, but could cause flaky "did not become healthy" failures.
2	test-adequacy	emulate_test.go:30	TestMain calls os.Exit(0) when npx is not on PATH — CI reports success with zero tests run if setup-node fails silently.
3	test-adequacy	emulate_test.go	CommitFile — the most complex scm.Driver method (multi-step Git Trees/Blobs/Commits API) — is not exercised. Reasonable to defer, but it is the likeliest code path to diverge between real GitHub and the emulator.
4	test-integrity	emulate_test.go:85	TestCommentAndClose cannot verify CloseIssue's effect: forge.Issue has no State field. Documented in the test comment as a known limitation.
5	error-handling	emulate.go:121	If the emulate subprocess exits immediately, waitHealthy blocks for 15 s before returning an opaque timeout error. Detecting early process exit would speed up debugging.
6	supply-chain	emulate.go:49	npx --yes emulate@0.8.0 without integrity verification. Accepted risk: workflow grants no secrets, uses pull_request with read-only GITHUB_TOKEN.
7	network-exposure	emulate.go:50	Emulator binding address not explicitly passed to subprocess; code assumes 127.0.0.1. Low-impact since the emulator serves test fixture data with no real credentials.
8	scope-authorization-mismatch	N/A	PR body references ADR 0052 / #73 as context for Layer-2 testing gap; implementation is driver unit test infrastructure. PR documentation is internally consistent about its purpose. Consider clarifying the PR description.
9	documentation	emulate.go:4	Package comment focuses on external tool rather than the package's role in the system.
10	struct-organization	emulate.go:29	Instance embeds scm.Driver; established driver pattern uses a named Client field. Deviation is intentional (lifecycle wrapper vs. direct implementation).
11	missing-doc	behaviour-drivers.md	New section documents the package well but could include a brief code example showing Start/Close lifecycle. Test file (emulate_test.go) serves as de facto example.

Previous run (2)

Review — comment

Clean, well-documented addition of a local GitHub emulator driver for scm.Driver tests. The package reuses the existing scm/github driver via forge/github.LiveClient.WithBaseURL, so the real code paths get exercised against a local emulator — solid architectural choice. The CI workflow correctly uses pull_request with minimal contents: read permissions, and the docs update in behaviour-drivers.md thoroughly explains the design trade-offs (why it's not a BEHAVIOUR_SCM value, no reset endpoint, per-test isolation via unique issue numbers).

Findings

Medium

#	Category	File	Description
1	error handling	e2e/behaviour/drivers/scm/emulate/emulate.go	Close() always returns a non-nil error. After Kill(), cmd.Wait() returns the signal-killed exit status, so Close() never returns nil on the happy path. Current callers discard the error (_ = inst.Close()), but any future caller checking the return value will see a spurious error. Consider: _ = i.cmd.Wait(); return nil or filter *exec.ExitError after Kill.

Low

#	Category	File	Description
2	race condition	e2e/behaviour/drivers/scm/emulate/emulate.go	TOCTOU race in freePort(): the port is released before npx emulate --port binds it. Low probability on single-purpose CI runners, but could cause flaky "did not become healthy" failures if another process claims the port in the gap.
3	resource leak	e2e/behaviour/drivers/scm/emulate/emulate.go	If Close() is never called (e.g., TestMain panics after Start returns), subprocess cleanup depends on context cancellation or process exit. The streamLog goroutine also leaks. Low-impact for test infrastructure where process exit handles cleanup.
4	test adequacy	e2e/behaviour/drivers/scm/emulate/emulate_test.go	CommitFile — the most complex scm.Driver method (multi-step Git Trees/Blobs/Commits API) — is not exercised. Reasonable to defer to a follow-up, but it's the code path most likely to behave differently between real GitHub and the emulator.
5	edge case	e2e/behaviour/drivers/scm/emulate/emulate_test.go	TestMain calls os.Exit(0) when npx is not on PATH, so CI reports success (exit 0) with zero tests run. If actions/setup-node fails silently, all tests pass vacuously. Consider logging a SKIP: marker to stderr.
6	error handling	e2e/behaviour/drivers/scm/emulate/emulate.go	If the emulate subprocess exits immediately (bad seed, missing package), waitHealthy blocks for 15 s before returning an opaque timeout error. Detecting early process exit and surfacing the exit status would speed up debugging.
7	test integrity	e2e/behaviour/drivers/scm/emulate/emulate_test.go	TestCommentAndClose cannot verify CloseIssue's effect since forge.Issue has no State field — it only asserts the PATCH succeeded. Documented in the test comment as a known limitation.
8	supply-chain	e2e/behaviour/drivers/scm/emulate/emulate.go	npx --yes emulate@0.8.0 pulls an npm package at runtime without integrity verification. The version pin mitigates drive-by upgrades but does not verify package integrity. Low because the repo already uses npx without integrity hashes elsewhere (e.g., wrangler in site-deploy.yml).
9	network-exposure	e2e/behaviour/drivers/scm/emulate/emulate.go	Worth confirming that emulate binds exclusively to 127.0.0.1 (loopback) and not 0.0.0.0. On a shared CI runner, binding to all interfaces would expose the unauthenticated emulator API to other jobs.

Overall this is a well-scoped, well-documented PR. The medium finding (Close error handling) is the only item worth addressing before merge; the low findings are suggestions for hardening.

---

Labels: PR adds e2e test driver infrastructure with a new CI workflow

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:52 AM UTC · Completed 3:09 AM UTC
Commit: a969069 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 12:11 PM UTC · Ended 12:17 PM UTC
Commit: 2941769 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 12:17 PM UTC · Ended 12:30 PM UTC
Commit: 2941769 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:31 PM UTC · Completed 12:45 PM UTC
Commit: ea15167 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[waynesun09]

Closing — no current or near-term consumer for this driver, and every forward integration path it was meant to enable turned out to be solved elsewhere or unworkable:

• Dispatch-routing tests (the original motivation): internal/harnessdispatch's JSON input driver (feat(dispatch): harness CEL dispatch for custom agents (#2889) #3820) already makes CEL trigger matching fast-unit-testable without live GitHub Actions or an emulator. Solved independent of this PR.
• eval/ functional-pipeline tests: architecturally incompatible, not just inconvenient. gh CLI has no base-URL override to redirect at a local emulator (checked gh help environment / gh config — no such key). Even a full script rewrite wouldn't close the gap: run-fullsend.sh does a literal git clone, which needs git's smart-HTTP transport (info/refs, git-upload-pack) — vercel-labs/emulate only implements GitHub's REST git-data API, not that protocol.
• Fork-PR SCM behavior (test(behaviour): add SCM driver support for fork pull requests #3731–test(behaviour): interim bash-routing fork smoke until harness cutover #3733): already tracked under the same epic (Epic: ADR 0061 — per-repo harness CEL dispatch #2888) as the CEL work.
• Fault-injection/retry-path testing: vercel-labs/emulate has no built-in fault injection.
• "Fast regression net" for the methods this covers: doesn't hold up either — they're thin wrappers already covered by LiveClient's ~100 httptest unit tests plus the live triage.feature suite.

The driver itself works correctly and surfaced real value along the way: three real bugs in this package (Close() reporting its own kill signal as failure, a resource leak on the startup-failure path, and a context coupling that would've silently killed the emulator mid-test-run once more tests were added) and a genuine upstream bug in vercel-labs/emulate itself (the git-data commit endpoint nests tree.sha under a commit wrapper instead of the documented top-level shape), reported as vercel-labs/emulate#190. Those fixes and that report stand regardless of this PR's fate.

Not merging speculative test infrastructure with no consumer. Happy to revisit if a concrete task ever needs local GitHub-API-level testing.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 1:07 PM UTC · Completed 1:16 PM UTC
Commit: ea15167 · View workflow run →

[fullsend-ai-retro]

PR #4089 added a local GitHub emulator driver for scm.Driver tests (~430 lines, 5 files). The review agent ran 4 times (2 successful, 2 correctly cancelled on intermediate force-pushes), producing high-quality technical reviews that identified 3 real bugs (Close() error handling, resource leak on startup failure, context-timeout coupling). The author fixed all bugs across iterations. However, the PR was ultimately closed unmerged by the author, who concluded that no current or near-term consumer exists for the driver — every integration path it was meant to enable was either already solved elsewhere or architecturally incompatible. The review agent never flagged this fundamental strategic concern despite noting the lack of a linked issue and the speculative nature of the infrastructure. This is a known gap tracked by #849 (strategic fitness evaluation). Additionally, the first review run submitted a formal GitHub review under the PR author's own identity instead of the bot identity — a known issue tracked by #2990.

Proposals filed

• Review agent scope-authorization finding should escalate when a PR adds standalone infrastructure with no consumer (in fullsend-ai/agents)
• First review run posts formal GitHub review under PR author's identity instead of bot identity, weakening review blocking power (in fullsend-ai/fullsend)
• Fullsend review agent and Qodo bot produce overlapping code-level findings, creating noise on PRs with both reviewers active (in fullsend-ai/fullsend)