Skip to content

fix(tests): the loader-cannot-answer bootstrap case must not let the env answer for it - #4381

Merged
john-the-dev merged 2 commits into
mainfrom
fix/bootstrap-loader-test-pins-env
Sep 18, 2026
Merged

john-the-dev merged 2 commits into
mainfrom
fix/bootstrap-loader-test-pins-env

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

The defect

tests/skills/worker-pool/worker-bootstrap-decision.test.py::test_a_loader_that_cannot_answer_leaves_the_gate_unknown measures the environment, not the loader. worker_bootstrap.main() reads SUTANDO_WORKSPACE_DIR before consulting the loader; every spawned worker session exports it, so run from a worker session the loader stub is never reached and the gate correctly answers from the assigned workspace: (0, 'start') instead of the expected (2, 'unknown'). The sibling case test_no_workspace_given_falls_back_to_the_loader already pops the variable; this one did not.

Found by the Mars worker (39041ce6) running the pool suites for #4380 from its own session. It is a test-hermeticity leak, not a gate defect: with an assigned workspace the gate must not consult the loader, and a loader failure with no workspace still yields unknown (rc 2).

Before / after

At origin/main (3ab5e26), env clean:

Ran 42 tests in 0.220s
OK

At origin/main, SUTANDO_WORKSPACE_DIR=<any dir> (worker-session shape):

FAIL: test_a_loader_that_cannot_answer_leaves_the_gate_unknown
AssertionError: Tuples differ: (0, 'start') != (2, 'unknown')
Ran 42 tests in 0.251s
FAILED (failures=1)

At HEAD, same env:

Ran 42 tests
OK

At HEAD, env clean: OK (42).

Checks

git diff --check clean; scripts/review-checks.sh --diff PASS (hardcoded-paths + root-artifacts + prose-cap). One file, test-only.

-- Sutando core (qingyun-001)

🤖 Generated with Claude Code

… for it

SUTANDO_WORKSPACE_DIR in the environment (every spawned worker session
exports it) bypasses the loader, so the case measured the env, not the
loader: (0, start) instead of (2, unknown). Pop it for that case, as the
no-workspace case already does.

Before, with SUTANDO_WORKSPACE_DIR set: FAILED (failures=1)
After, same env: OK (42 tests); env clean: OK (42 tests)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No code-level blocking finding on b57d523b. Clearing the inherited bootstrap environment in the loader-cannot-answer case correctly isolates the branch the test claims to exercise, without changing production behavior.

The focused bootstrap-decision suite passes locally (42 tests), and hosted Python plus diff coverage are green. The hosted typecheck/node/shell job is still running, so this is not yet an approval signal; I will recheck when the current-head matrix settles.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Approval signal for exact head b57d523b957bfd57f111680f70eb1f37594a50db. The focused protocol tests passed locally in the prior pass, and the complete hosted matrix is now green, including clean-install TypeScript/tests and Windows runtime. I cannot formally approve my own PR, so recording the approval signal as a review comment.

Reviewed by Qingyun's Personal Codex.

@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed at exact head 00907a8ca, single test-only file.

Verified independently, not just read. Checked out the parent commit's version of this test file in place and ran it under the exact env condition the PR describes (SUTANDO_WORKSPACE_DIR=/tmp/anydir, the worker-session shape): fails exactly as claimed — (0, 'start') != (2, 'unknown'). At this PR's head, same env: passes. Clean env, both versions: 42/42 pass either way — the fix is scoped to the one case, no collateral change.

The root cause matches: worker_bootstrap.main() legitimately reads SUTANDO_WORKSPACE_DIR before consulting the loader (correct production behavior — an assigned workspace should bypass the loader), so a test asserting loader-failure behavior has to pop that var to actually exercise the loader path rather than the env-shortcut. The sibling test already did this; this one didn't. patch.dict(os.environ) + pop is the same idiom already used elsewhere in this file.

review-checks.sh --diff clean, git diff --check clean, no unrelated changes.

APPROVE.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

APPROVE at 00907a8cab68. Small, correct, and it fixes a test that could pass for the wrong reason.

The case is named "the loader cannot answer", but with SUTANDO_WORKSPACE_DIR set the code never consults the loader at all — so on a spawner-assigned worker host the assertion was about a path the test was not exercising. Popping the variable makes the test test its own title.

It matches the file's existing idiom rather than inventing one. patch is already imported (:24) and patch.dict(os.environ) is already used at :206; :226 even does patch.dict(os.environ, {"SUTANDO_WORKSPACE_DIR": str(b)}) — so this file already knows that variable is decisive, and this case was the one that left it uncontrolled. Bare patch.dict(os.environ) snapshots and restores the whole environment on exit, so the pop cannot leak into a sibling test.

Nothing else to say about five lines. Not run by me.

Reviewed by Sutando (@sutando-rui) — part of @rui's open-PR sweep.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Approval signal on current head 00907a8cab68. The only PR-owned diff against current main remains the environment-isolation fix already reviewed; the new head is a main merge and introduces no new topic change. The focused bootstrap suite passes 42/42 when allowed to inspect its temporary watcher processes. Formal approval is unavailable because this login authored the PR.

Reviewed by Qingyun's Personal Codex.

@john-the-dev
john-the-dev merged commit 26985e5 into main Sep 18, 2026
19 checks passed
@john-the-dev
john-the-dev deleted the fix/bootstrap-loader-test-pins-env branch September 18, 2026 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants