test(server): retire duplicated resolveShell darwin fallback row (#16639)

<!-- Write all pull request text in Simplified Technical English
(ASD-STE100): short sentences, one instruction per sentence, simple
approved vocabulary, and the active voice. -->

## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work.
> - The server prepares task workspaces in `workspace-runtime.ts`. One
helper, `resolveShell()`, picks the shell that workspace commands use.
> - `resolveShell()` computes its fallback with one two-arm ternary:
`"sh"` for win32, else `"/bin/sh"`. There is no separate darwin arm.
> - The tests for this helper had three unset-SHELL fallback rows:
linux, win32, and darwin. The darwin row repeated the linux row. It
observed the same ternary arm and asserted the same value through the
same code.
> - This pull request removes the duplicated darwin row. The linux row
keeps the non-win32 arm pinned.
> - The benefit is one less repeated registration per suite run and a
smaller platform matrix to maintain. No wall-time saving is claimed.

## Linked Issues or Issue Description

- No public GitHub issue exists for this cleanup. The underlying
problem: the `resolveShell` fallback tests registered two cases that
could not fail separately. Any change to the non-win32 fallback arm
breaks both rows at the same assertion. One row is enough to protect
that arm. The audit receipt in
`doc/plans/2026-10-17-workspace-runtime-resolveshell-test-audit.md`
records the full decision table, the deferred candidate, and the
retained rows.

## What Changed

- Deleted `falls back to /bin/sh on darwin when SHELL is unset` from the
`resolveShell (shell fallback)` describe in
`server/src/__tests__/workspace-runtime.test.ts` (6 test lines).
- Kept the win32 row (the only observation of the `"sh"` arm), the linux
row (the only needed observation of the `"/bin/sh"` arm), the
empty-string and whitespace-only rows (missing versus invalid input;
whitespace-only also pins the trim-before-check order), and the
missing-absolute-path row (the only observation of the `existsSync`
guard).
- Added the audit receipt
`doc/plans/2026-10-17-workspace-runtime-resolveshell-test-audit.md` with
evidence, commands, and the deferred candidate.
- Corrected the receipt's mutation-control record (head d99284b): the
`/bin/bash` control fails three retained rows, not one. See
Verification.
- Changed zero production lines, zero lockfile lines, and zero workflow
lines.

## Verification

- Focused owner run: `pnpm --filter @paperclipai/server exec vitest run
-t "shell fallback" src/__tests__/workspace-runtime.test.ts` (vitest
5.0.3, Node 25.9.0).
- Baseline before the edit: 9/9 passed.
- Candidate after the edit: 8/8 passed.
- Control (re-verified on head d99284b): the production fallback arm was
temporarily changed to `"/bin/bash"`. Exactly three retained rows then
failed with the same assertion (`expected '/bin/bash' to be '/bin/sh'`):
the linux unset row (the deleted darwin row's keeper), the empty-`SHELL`
row, and the missing-absolute-path row. The win32 rows and the
set-`SHELL` rows still passed (3 failed, 5 passed). This proves the
retained row carries the deleted row's failure sensitivity, and the
shared fallback value keeps two more independent guards. Production was
restored byte-exact (sha256
`335fd5332b4242ab0ff2a565323e27db48a69ddfb7bbd8d913e5148f79ff961b`
before and after), and the focused run passed 8/8 again.
- The full `workspace-runtime.test.ts` suite and the repo-wide
typecheck/test/build were not run locally; the isolated container cannot
finish the git-fixture cases inside their 15-second budgets (three
unrelated reconciliation cases time out at 15,000 ms on this host both
before and after the change). PR CI runs those lanes.
- PR CI on head d99284b: all 57 checks green (workflow run 39022073948;
the only non-success conclusions are conditional skips).

## Risks

- Low risk. The change is a six-line deletion inside one describe block,
plus a receipt correction. The only theoretical loss is a
darwin-specific fallback change, which does not exist today: darwin and
linux share the single non-win32 ternary arm. If a darwin-specific arm
is added later, that arm needs its own row and its own contract
decision.
- Open PRs #12658, #11753, and #16213 also touch
`server/src/__tests__/workspace-runtime.test.ts`, in different regions
(pnpm provisioning case, restart scoping, and worktree lock work). This
change edits only the `resolveShell` describe, so the hunks do not
collide.

> For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and
discuss it in `#dev` before opening the PR. Feature PRs that overlap
with planned core work may be redirected — check the roadmap first. See
[`CONTRIBUTING.md`](CONTRIBUTING.md).

## Model Used

- GLM, a large language model from Z.ai. Exact model ID: `z-ai/glm-5.3`
served through OpenRouter (`openrouter/z-ai/glm-5.3`). Context window:
the session that produced this change. Capabilities used: repository
file search and read, shell command execution, vitest runs, and a
production mutation control. No model context-window size beyond the
session is asserted because it was not disclosed to the agent.

## Checklist

- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` /
`Closes: #` / `Refs #` OR (b) described the issue in-PR following the
relevant issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [ ] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [x] All Paperclip CI gates are green
- [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge
This commit is contained in:
Dotta authored and GitHub committed 2026-10-19 07:28:35 -05:00
1 parent 1d24bd8e47
commit c6a0cf4bce
2 files changed
+88 -6

No files matched your search

@@ -0,0 +1,88 @@
# workspace-runtime resolveShell duplicate-row retirement — 2026-10-17
One redundant registered execution is retired from
`server/src/__tests__/workspace-runtime.test.ts`. No production file
changes. No execution-time or CI-time benefit is claimed.
Discovery base: `5555817c2b18a1a5ebc3a3b01883c1b12b86df5a` (master head).
Applied skill: installed test-audit, SKILL.md sha256
`d2569031196778bebb7fd623d87b4379ed1a0d372d1ea004b0b3d330b352d1d2`.
## Change and keeper
| Candidate | Decision | Contract | Keeper | Evidence |
| --- | --- | --- | --- | --- |
| `falls back to /bin/sh on darwin when SHELL is unset` (resolveShell describe) | Delete | Non-win32 fallback returns `/bin/sh`. | `falls back to /bin/sh on non-Windows when SHELL is unset` (linux row) | `resolveShell()` computes the fallback from a single two-arm ternary, `process.platform === "win32" ? "sh" : "/bin/sh"`. darwin and linux both evaluate the same non-win32 arm and assert the same value through the same production path, so the darwin row cannot fail while the linux row passes. Inputs and assertions are otherwise identical (unset SHELL, mocked platform, fallback value). |
Retained siblings in the same describe: the win32 row (only observation of
the `"sh"` arm), the empty-string and whitespace-only rows (missing versus
invalid input; whitespace-only additionally pins trim-before-falsy-check),
and the missing-absolute-path row (only observation of the `existsSync`
guard). The non-Windows/linux row keeps the non-win32 arm pinned after this
change, verified by control below.
## Validation
Focused owner runs, `pnpm --filter @paperclipai/server exec vitest run
-t "shell fallback" src/__tests__/workspace-runtime.test.ts` (vitest 5.0.3,
Node 25.9.0, isolated checkout with `pnpm install --filter @paperclipai/server...`):
1. Baseline (before edit): 9/9 passed.
2. Candidate (darwin row deleted): 8/8 passed.
3. Control: temporarily changed the production fallback arm to
`"/bin/bash"` and reran the same focused selection (re-verified
2026-10-17). Exactly three rows failed at the same assertion shape
(`expected '/bin/bash' to be '/bin/sh'`): the retained linux unset
row (the deleted darwin row's keeper), the empty-`SHELL` row, and
the missing-absolute-path row. The win32 rows and the set-`SHELL`
rows still passed (3 failed, 5 passed). This proves the retained
linux row carries the deleted row's failure sensitivity; it also
shows the same non-win32 fallback value is observed by the
empty-`SHELL` and missing-absolute-path rows, so that value keeps
multiple guards after this change. An earlier draft of this receipt
wrongly said the mutation "breaks nothing else" in the selected run;
that claim is corrected here (Greptile review finding, PR #16639).
4. Production restored byte-exact (sha256
`335fd5332b4242ab0ff2a565323e27db48a69ddfb7bbd8d913e5148f79ff961b`
before and after), rerun: 8/8 passed.
The full `workspace-runtime.test.ts` owner and the repo-wide
typecheck/test/build suite were not run locally in this session; this
container cannot complete the git-fixture cases inside their 15-second
budgets (three unrelated reconciliation cases time out at 15,000 ms here
before and after this change, and the production-internal 8-second git scan
budget also fires), so those runs are left to PR CI. The change is a
six-line deletion inside one describe; no production file, lockfile,
workflow, or shard manifest is touched.
## Deferred candidate (evidence, not applied)
`keeps forward reconciliation fail-closed for an unrelated task branch`
(line ~3491) repeats `keeps forward reconciliation fail-closed for
same-content rewritten history` (line ~3461): identical git topology (each
branch one commit ahead of main), identical assertion union through
`expectPersistedBranchMismatchRejected` (verdict `diverged`, reason
`expected branch and current HEAD differ`), and the differing fixture bytes
(file names, commit messages) are never read by
`inspectGitWorktreeBranchIncoherence`, which decides on cleanliness, branch
existence, ancestry, and worktree registration only. The third sibling
(`live branch is behind`) is retained: its topology makes the actual branch
an ancestor in the reverse direction, so it uniquely guards the ancestry
comparison's direction. Deletion of the unrelated-task row is deferred
because the local environment cannot produce a passing baseline for these
git-fixture cases (15-second per-test timeout); it needs validation on a
host where the fixtures complete, or via CI evidence.
## Other examined-and-retained areas
- `native-runner-input-boundary.test.ts` alias: retained per the 2026-10-13
execution-wrapper receipt; still the only entry point for the Runner
contract suite in the root default run. Historical weight 204 ms.
- Repo-wide duplicate-title and near-identical-body scans over the top-30
duration-weighted server suites: all other matches protect distinct
contracts (different function, input class, platform arm, or boundary) and
are retained.
Test source changes: −6 test lines, zero production lines, zero docs lines
outside this receipt. One registration removed per complete-suite
invocation. No measured wall-time saving is claimed.
@@ -5635,12 +5635,6 @@ describe("resolveShell (shell fallback)", () => {
expect(resolveShell()).toBe("sh");
});
it("falls back to /bin/sh on darwin when SHELL is unset", () => {
delete process.env.SHELL;
Object.defineProperty(process, "platform", { value: "darwin" });
expect(resolveShell()).toBe("/bin/sh");
});
it("treats empty SHELL as unset and uses platform fallback", () => {
process.env.SHELL = "";
Object.defineProperty(process, "platform", { value: "linux" });