Skip to content

chore(test): mock openshell in TestRunAgent_* to prevent hangs#2986

Open
rh-hemartin wants to merge 1 commit into
mainfrom
fix/test-runagent-openshell-mock
Open

chore(test): mock openshell in TestRunAgent_* to prevent hangs#2986
rh-hemartin wants to merge 1 commit into
mainfrom
fix/test-runagent-openshell-mock

Conversation

@rh-hemartin

Copy link
Copy Markdown
Member

What changed

Added stub openshell binary and PATH manipulation to five TestRunAgent_* tests that were hanging when openshell is installed.

Why

TestRunAgent_HarnessLoadPipeline and four related tests (lines 142-354 of internal/cli/run_test.go) are shallow integration tests that exercise harness loading. They expect to fail fast at sandbox.EnsureAvailable with "openshell not found", but on machines where openshell IS installed, they proceed to actually run agents in sandboxes, hanging for 25-36 seconds until timeout.

How

  1. Created internal/cli/testdata/openshell — stub binary that exits 1
  2. Added useFakeOpenshell(t) helper that prepends testdata/ to PATH
  3. Called helper in all five affected tests

Tests now pass in ~0.13s instead of hanging.

Fixes #2985

@rh-hemartin rh-hemartin requested a review from a team as a code owner July 3, 2026 09:43
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 3, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:43 AM UTC · Completed 9:52 AM UTC
Commit: ec6f704 · View workflow run →

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Mock openshell in TestRunAgent_* to avoid slow/hanging integration runs

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Add a test helper that prepends a stub openshell to PATH.
• Force five shallow harness-loading tests to fail fast at sandbox availability checks.
• Prevent 25–36s hangs/timeouts on machines with openshell installed.
Diagram

graph TD
  T(["TestRunAgent_* tests"]) --> H["useFakeOpenshell(t)"] --> P["PATH updated (prepend testdata)"] --> O["testdata/openshell stub"] --> S["sandbox.EnsureAvailable"] --> F["Fail fast (no sandbox run)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Inject/mocking hook for sandbox availability
  • ➕ Avoids reliance on PATH/OS executable semantics
  • ➕ More explicit unit-test boundary (no external process lookup)
  • ➖ Requires production code changes and a test seam/interface
  • ➖ More refactor than needed for a targeted flake/hang fix
2. Skip/guard tests when openshell is detected
  • ➕ Very small test change (e.g., exec.LookPath and t.Skip)
  • ➕ No need to ship a stub executable
  • ➖ Reduces coverage on developer machines where openshell exists
  • ➖ Doesn’t validate the intended fail-fast path; just avoids it
3. Set PATH to an empty/minimal value in these tests
  • ➕ No new file added to repo
  • ➕ Simple and direct to force “not found”
  • ➖ Risk of breaking other subprocess/tool lookups used during tests
  • ➖ Harder to reason about if the test suite expands

Recommendation: The PR’s approach (prepend a deterministic stub openshell via a helper) is the best tradeoff: it keeps production code untouched, preserves coverage of the intended fail-fast path, and avoids brittle global PATH clearing. The main thing to watch is platform/CI expectations around executing a /bin/sh stub; if Windows is in scope, consider a Go-based helper binary or platform-specific stubs.

Files changed (2) +21 / -0

Tests (2) +21 / -0
run_test.goAdd PATH-based fake openshell helper and apply it to five tests +16/-0

Add PATH-based fake openshell helper and apply it to five tests

• Introduces useFakeOpenshell(t) to prepend the testdata directory to PATH. Calls the helper in five TestRunAgent_* tests so they consistently hit the expected sandbox.EnsureAvailable failure instead of running real sandboxes when openshell is installed.

internal/cli/run_test.go

openshellAdd stub openshell executable for tests +5/-0

Add stub openshell executable for tests

• Adds a shell-script openshell stub that prints a message and exits with status 1. This ensures sandbox availability checks fail quickly and prevents slow/hanging execution paths in tests.

internal/cli/testdata/openshell

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Site preview

Preview: https://9d011383-site.fullsend-ai.workers.dev

Commit: bf72e31c0757fbd29a384d52ebacb9c1d07121c4

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 54 rules

Grey Divider


Action required

1. useFakeOpenshell bypasses EnsureAvailable mock 📎 Requirement gap ☼ Reliability
Description
The harness-loading tests now rely on PATH manipulation plus a stub openshell binary instead of
mocking sandbox.EnsureAvailable() to deterministically return an error, which violates the
hermetic-test requirement and still couples behavior to the real EnsureAvailable implementation
and machine environment. Additionally, useFakeOpenshell does not verify that openshell actually
resolves to the stub, so if the stub isn’t runnable the tests can still pick up a real openshell
later in PATH and reintroduce hangs.
Code

internal/cli/run_test.go[R142-151]

+// useFakeOpenshell prepends testdata/ to PATH so the stub openshell binary
+// is found instead of a real installation, causing tests to fail fast at
+// sandbox.EnsureAvailable instead of actually running agents.
+func useFakeOpenshell(t *testing.T) {
+	t.Helper()
+	testdataDir, err := filepath.Abs("testdata")
+	require.NoError(t, err)
+	origPath := os.Getenv("PATH")
+	t.Setenv("PATH", testdataDir+string(filepath.ListSeparator)+origPath)
+}
Relevance

⭐⭐ Medium

Mixed history: env-dependent test hardening accepted (#2391), but PATH/mock hermeticity suggestions
rejected (#735).

PR-#2391
PR-#735

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1525850 explicitly requires stubbing/mocking sandbox.EnsureAvailable() itself for
the TestRunAgent_* harness-loading tests, but the change shown instead introduces a helper that
prepends testdata/ to PATH so exec.LookPath("openshell") and subsequent
exec.Command("openshell", ...) calls fail via resolution behavior rather than by mocking
EnsureAvailable. Because the production code’s guard is simply exec.LookPath("openshell"), and
the tests never assert that PATH resolution returns the intended stub at
internal/cli/testdata/openshell, a non-runnable stub (e.g., due to filemode/OS/runtime
differences) can cause LookPath to skip it and resolve a real openshell elsewhere in PATH,
recreating the original environment-dependent hang scenario.

Mock sandbox.EnsureAvailable() in TestRunAgent_* harness-loading tests to be hermetic
internal/cli/run_test.go[142-151]
internal/cli/testdata/openshell[1-5]
internal/sandbox/sandbox.go[190-197]
internal/cli/run.go[552-568]

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 compliance rule requires the `TestRunAgent_*` harness-loading tests to deterministically stub/mock `sandbox.EnsureAvailable()` to return an error, rather than relying on PATH manipulation and a stub `openshell` binary. The current approach is not hermetic and can still depend on the real `sandbox.EnsureAvailable()` implementation and developer-machine environment; it also does not verify that `openshell` resolution actually selects the stub, so a non-runnable stub can lead to resolving a real `openshell` later in PATH and reintroducing hangs.

## Issue Context
These tests are intended to be shallow integration/unit-style tests focused on harness loading and must not be affected by the developer machine environment. The rule’s success criteria explicitly calls for mocking `sandbox.EnsureAvailable()` (not just making `openshell` fail via PATH), and `sandbox.EnsureAvailable()` relies on `exec.LookPath("openshell")` and then `exec.Command("openshell", ...)`; without asserting the resolved path points at the intended stub, PATH-based forcing can be flaky across OSes and repo filemode metadata.

## Fix Focus Areas
- internal/cli/run_test.go[142-151]
- internal/cli/run_test.go[153-159]
- internal/sandbox/sandbox.go[190-197]
- internal/cli/testdata/openshell[1-5]

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


Grey Divider

Qodo Logo

Comment thread internal/cli/run_test.go
@rh-hemartin rh-hemartin self-assigned this Jul 3, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [test-correctness] internal/cli/run_test.go:145 — Two comments incorrectly state the failure point. (1) The existing comment in TestRunAgent_HarnessLoadPipeline (line 145) says "fails later at sandbox.EnsureAvailable (no openshell in test env)" but with the fake stub in PATH, EnsureAvailable succeeds (it only calls exec.LookPath) and the actual failure occurs at sandbox.CheckGateway (line 564 of run.go), which invokes openshell gateway list and gets exit code 1 from the stub. (2) The new useFakeOpenshell doc comment also says "causing tests to fail fast at sandbox.EnsureAvailable" — this is equally wrong for the same reason. Both comments should reference sandbox.CheckGateway as the failure point.
    Remediation: Update both comments to reference sandbox.CheckGateway instead of sandbox.EnsureAvailable.
Previous run

Review

Findings

Medium

  • [scope-creep] internal/cli/run_test.go — The PR adds useFakeOpenshell(t) to only 5 of the 11 TestRunAgent_* tests that assert on "openshell" errors. Six additional tests have the same hanging risk but are not patched: TestRunAgent_ConfigAgentLocalPath (line 481), TestRunAgent_ConfigAgentURL (line 510), TestRunAgent_ConfigAgentOverridesScaffold (line 545), TestRunAgent_ScaffoldFallback (line 578), TestRunAgent_FallsBackToFULLSEND_MINT_URL (line 2824), and TestRunAgent_StatusNotifierSetup (line 2939). All of these call runAgent and assert err.Error() contains "openshell", meaning they reach sandbox.EnsureAvailable and will hang on machines where openshell is installed — exactly the bug this PR is intended to fix.
    Remediation: Add useFakeOpenshell(t) to all six remaining tests that assert on "openshell" in the error.

  • [implementation-coherence] internal/cli/testdata/openshell — The implementation uses PATH manipulation to inject a fake binary rather than mocking sandbox.EnsureAvailable directly as the issue suggests. If sandbox.EnsureAvailable ever changes to use a hardcoded path or environment variable override, these tests would silently start hanging again. The PATH manipulation approach is a reasonable testing pattern in Go, but this architectural coupling is worth noting.

Low

  • [test correctness] internal/cli/run_test.go:145 — The comment in TestRunAgent_HarnessLoadPipeline still says "(no openshell in test env)" to explain why the test fails at sandbox.EnsureAvailable. With the new useFakeOpenshell helper, the failure mechanism is now "openshell exits 1", not "no openshell". The comment is misleading about the actual failure mode.

Labels: PR modifies sandbox-related test infrastructure (openshell mocking) in Go test files

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added component/sandbox OpenShell sandbox environment go Pull requests that update go code labels Jul 3, 2026
TestRunAgent_HarnessLoadPipeline and four related tests hang when
openshell is installed. These shallow integration tests exercise
harness loading and expect to fail fast at sandbox.EnsureAvailable,
but on machines with openshell they proceed to actually run agents.

Add stub openshell binary that exits 1 and prepend testdata/ to PATH
in affected tests. Tests now pass in ~0.13s instead of hanging.

Fixes #2985

Signed-off-by: Hector Martinez <hemartin@redhat.com>
@rh-hemartin rh-hemartin force-pushed the fix/test-runagent-openshell-mock branch from ec6f704 to bf72e31 Compare July 3, 2026 10:01
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 3, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:02 AM UTC · Completed 10:11 AM UTC
Commit: bf72e31 · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot dismissed their stale review July 3, 2026 10:11

Superseded by updated review

Comment thread internal/cli/run_test.go
// useFakeOpenshell prepends testdata/ to PATH so the stub openshell binary
// is found instead of a real installation, causing tests to fail fast at
// sandbox.EnsureAvailable instead of actually running agents.
func useFakeOpenshell(t *testing.T) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] test-correctness

Two comments incorrectly state the failure point. (1) The existing comment in TestRunAgent_HarnessLoadPipeline (line 145) says fails later at sandbox.EnsureAvailable (no openshell in test env) but with the fake stub in PATH, EnsureAvailable succeeds (it only calls exec.LookPath) and the actual failure occurs at sandbox.CheckGateway (line 564 of run.go), which invokes openshell gateway list and gets exit code 1 from the stub. (2) The new useFakeOpenshell doc comment also says causing tests to fail fast at sandbox.EnsureAvailable — this is equally wrong for the same reason.

Suggested fix: Update both comments: (1) in TestRunAgent_HarnessLoadPipeline, change fails later at sandbox.EnsureAvailable (no openshell in test env) to fails later at sandbox.CheckGateway (stub exits 1); (2) in useFakeOpenshell, change causing tests to fail fast at sandbox.EnsureAvailable to causing tests to fail fast at sandbox.CheckGateway.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/sandbox OpenShell sandbox environment go Pull requests that update go code ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TestRunAgent_* tests hang when openshell is installed

1 participant