Skip to content

fix: make approval smoke tests portable - #2

Merged
fengyincheng merged 2 commits into
fengyincheng:mainfrom
mel0nyrame:fix/ci-approval-smoke-tests
Jul 25, 2026
Merged

fengyincheng merged 2 commits into
fengyincheng:mainfrom
mel0nyrame:fix/ci-approval-smoke-tests

Conversation

@mel0nyrame

Copy link
Copy Markdown
Contributor

Problem

The core GitHub Actions job failed in npm run test:core with three misleading approval errors:

  • the expired proposal test received approval_not_found instead of expired
  • concurrent execution received [404, 404] instead of [200, 409]
  • proposal-integrity validation received 404 instead of 409

The approval records were not disappearing from SQLite. They were never created.

getApprovalSmokeState() intentionally requires both the smoke directory and its enabled file 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 unprivileged runner user. As a result, POST /v1/shell/approval-smoke returned approval_smoke_disabled before creating a proposal.

The tests did not assert that preparation returned 202, so they read a missing approval_id as undefined and continued with /v1/shell/approvals/undefined. This hid the original failure behind approval_not_found responses.

Solution

Keep the production policy unchanged

buildApp now accepts an optional resolveApprovalSmokeState dependency. Production callers do not provide it, so the default remains getApprovalSmokeState() with its existing checks for:

  • root ownership
  • safe directory and file modes
  • a regular enabled file
  • canonical path containment

The 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:

  • immutable smoke state validation
  • enabled-file replacement or removal detection
  • expiration
  • concurrent claiming
  • replay prevention
  • proposal integrity checks

Fail at the actual preparation boundary

Every test that consumes a smoke approval_id now first asserts that proposal preparation returned 202. 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.ts directly exercises the production getApprovalSmokeState() implementation with mocked filesystem metadata. It covers:

  • a valid root-owned, non-writable regular file
  • an enabled file outside the smoke directory
  • a non-root-owned directory
  • a non-root-owned enabled file
  • a group-writable directory
  • a world-writable enabled file
  • a non-regular enabled path

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 — passed
  • npm run test:core — passed
  • npm run build — passed
  • privileged-acceptance-note — passed

CI run: https://github.com/mel0nyrame/ShellBridge/actions/runs/30155940164

The branch contains only these two commits:

  • 3f87ce1 fix: make approval smoke tests portable
  • 3257569 test: cover approval smoke security policy

@fengyincheng
fengyincheng merged commit 76e118b into fengyincheng:main Jul 25, 2026
2 checks passed
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.

2 participants