Repository navigation
fix: make approval smoke tests portable - #2
Merged
fengyincheng merged 2 commits intoJul 25, 2026
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
coreGitHub Actions job failed innpm run test:corewith three misleading approval errors:approval_not_foundinstead ofexpired[404, 404]instead of[200, 409]404instead of409The approval records were not disappearing from SQLite. They were never created.
getApprovalSmokeState()intentionally requires both the smoke directory and itsenabledfile to be root-owned and not group/world writable. That is the correct production security policy, but GitHub-hosted runners create test fixtures as the unprivilegedrunneruser. As a result,POST /v1/shell/approval-smokereturnedapproval_smoke_disabledbefore creating a proposal.The tests did not assert that preparation returned
202, so they read a missingapproval_idasundefinedand continued with/v1/shell/approvals/undefined. This hid the original failure behindapproval_not_foundresponses.Solution
Keep the production policy unchanged
buildAppnow accepts an optionalresolveApprovalSmokeStatedependency. Production callers do not provide it, so the default remainsgetApprovalSmokeState()with its existing checks for:enabledfileThe same resolver instance is used when preparing a smoke proposal, validating it before execution, and validating it again inside the atomic claim transaction. This preserves the existing immutable-state and replay protections.
Give tests a portable filesystem seam
The test application injects a resolver that reads the real canonical paths and captures the real device, inode, and modification time of the temporary fixture. This allows the proposal lifecycle to run under an unprivileged CI user while continuing to exercise:
Fail at the actual preparation boundary
Every test that consumes a smoke
approval_idnow first asserts that proposal preparation returned202. A future preparation regression will therefore fail at its source instead of producing unrelated downstream 404s.Preserve coverage of the production security policy
A separate
test/policy.test.tsdirectly exercises the productiongetApprovalSmokeState()implementation with mocked filesystem metadata. It covers:The policy test is included in
npm run test:core, so making the end-to-end fixture portable does not remove regression coverage for the production ownership and permission checks.Why this approach
Removing or weakening the root-ownership checks would make CI pass by changing the production security boundary. Detecting CI through an environment variable would also turn a test concern into a production security switch.
The dependency seam keeps the privileged policy as the production default, lets ordinary CI exercise the approval state machine, and tests the privileged policy independently with deterministic metadata. This separates two concerns that require different environments without reducing either set of assertions.
Verification
Validated on the fork with the same Linux GitHub Actions workflow:
npm ci— passednpm run test:core— passednpm run build— passedprivileged-acceptance-note— passedCI run: https://github.com/mel0nyrame/ShellBridge/actions/runs/30155940164
The branch contains only these two commits:
3f87ce1 fix: make approval smoke tests portable3257569 test: cover approval smoke security policy