Repository navigation
FAL-311: Add isolated read-only live CLI smoke check - #28
Conversation
![dealmachine-alma[bot]](https://avatars.githubusercontent.com/in/5203151?s=60&v=4){kind=small}
There was a problem hiding this comment.
Alma: Code and cleanup
Changes need attention.
The smoke check itself works and meets the three acceptance criteria as far as I could test them on Linux with Node 22.23.2. npm run check passed: 12 test files and 71 tests, including the new built-CLI integration test, which ran in about 4 seconds. npm run test:package also passed with the updated npm runner. Running the smoke with either setting missing exits 1 and names the missing variable. A blank key counts as missing, and the key is never printed. The relay forwards only the four approved read-only requests. It refuses property search unless the request asks for a cost estimate, and it checks the source header, User-Agent and authentication. Each CLI run uses a temporary home with an encrypted-file credential that the real config reader accepts, so the OS credential store is never called.
I found one problem. The new developer doc says CI runs the integration test on Linux, macOS and Windows, but CI runs only on Ubuntu.
I didn't find any leftover code to remove. I did not run anything against staging or on macOS or Windows; the ticket already lists those as unverified.
- low: Docs say the smoke integration test runs in CI on macOS and Windows, but CI only runs on Ubuntu (source)
Expected: The developer guide describes the platforms CI actually covers. Or, if Windows and macOS coverage is intended, the CI matrix runs on those operating systems.
Observed: The new paragraph says the built-CLI integration test "runs in CI on Linux, macOS and Windows with Node 22 and 24". But .github/workflows/ci.yml, which this PR does not change, has one job withruns-on: ubuntu-latestand a matrix over Node 22 and 24 only. Readers would think Windows and macOS isolation is proven by CI when it is not. The new regression check itself says Linux results alone do not verify both platforms.
Suggested fix: The conservative fix is to change the sentence to say CI runs it on Linux with Node 22 and 24, and that macOS and Windows still need a separate check. If cross-platform CI is wanted, add an OS matrix to ci.yml in a change a person merges, because CI files are a hold-back area.
Reviewed commit: 4ecca4f20de4.
This is a comment review. It does not approve a merge or release; required checks and QA remain separate.
Factory-Review-Run: a6a94ec8244c37862fcf3aaac4dd8328c2625051bb25d1ef05196a23a2a19a94
There was a problem hiding this comment.
Alma: Code and cleanup
No findings in this review.
Passed. I read every diff page and ran the PR's exact head in the sandbox on Node 22.23.2. npm ci exited 0. npm run check exited 0: typecheck, build, 71 of 71 tests including both new smoke test files, and the public artifact check. npm run test:package exited 0. Logs are in /mnt/session/outputs/check.log and test-package.log.
AC-1: If either variable is missing, the smoke exits 1 and names it before doing any network setup. Only the redacted key is printed. The built-CLI integration test covers both cases.
AC-2: The smoke runs five built Commands through a local relay. The relay forwards only GET account, filters and fields, plus property search when estimate_cost is true. Each request is checked for the source header, the dm-cli/<version> User-Agent, an Authorization header and a 200 status. Redirects are refused, and the target URL must be a plain HTTPS /v1 address (HTTP only for loopback).
AC-3: Each child CLI gets a temporary home. Its home-directory lookup is replaced with that path and the call the CLI uses for Keychain and DPAPI is disabled. The seeded encrypted-file config matches src/lib/config.ts, and the unit test reads it back through the real readConfig. I confirmed execFileSync is the only child-process call in the config code. The temporary directory is removed in finally.
The build, npm-runner and package-test changes keep the existing checks passing, and the repository's AGENTS.md rules for this kind of change are met.
Not verified here: the live staging run and the macOS and Windows runs. The ticket says those were done separately, and I could not repeat them in this Linux sandbox without credentials. I didn't find any leftover code to remove.
Reviewed commit: 601c1141c17d.
This is a comment review. It does not approve a merge or release; required checks and QA remain separate.
Factory-Review-Run: 33929e8327905f4269a0776d17768978bd4e328868a37be0792bc2770887bf00
There was a problem hiding this comment.
Alma: Security
No findings in this review.
Security pass: no findings. I traced the new live smoke's trust boundaries on the exact head (601c114).
Credentials: the API key reaches the CLI child only through an encrypted-file config in a fresh 0700 temporary home. The key is not in the child's environment. A preload points the home directory at that folder and disables the child-process call the CLI uses for the macOS Keychain and Windows DPAPI. The folder is removed in a finally block. Output lines and the top-level error pass through redaction. The relay never injects the key, so a local process calling the loopback relay gets nothing it didn't already have.
Request destination: DM_API_URL must be HTTPS, or HTTP only on loopback. It must be a plain /v1 base with no userinfo, query or fragment, and redirects are refused upstream. I probed unusual relay request paths (absolute-form, protocol-relative, dot segments, encoded traversal) with a throwaway script. Every allowed path still resolves to the configured API host. Disallowed traversal is refused. No SSRF.
Read-only boundary: only GET account, filters and fields are forwarded, plus POST property search with estimate_cost exactly true. Other methods and paths get a 403 before leaving the machine.
Build and npm helper changes run the local TypeScript compiler and npm's own entrypoint through Node. They add no shell interpolation. The factory JSON and docs files are metadata only.
Commands run: npm ci passed. npx vitest run tests/scripts/smokeLive.test.ts tests/scripts/smokeLive.integration.test.ts passed 9 of 9 tests, exit 0.
Not covered: an interrupted run (Ctrl+C or a kill signal) skips cleanup and can leave the reversibly encrypted key in the user's own temp folder. That folder is readable only by the same user, so I see no privilege gain, but the person running the smoke should know. I also can't confirm from source alone that the live API really charges nothing for estimate requests, and I did no macOS or Windows execution, live API testing or dependency scanning.
Reviewed commit: 601c1141c17d.
This is a comment review. It does not approve a merge or release; required checks and QA remain separate.
Factory-Review-Run: 1108a8661ddc781b807522754aba35f4183544e2bb28e9c180196bdc5ddbd0c2
Adds
npm run smoke:live:coverageto check the built CLI version, account, filters, fields and estimated-cost property search against an explicitly supplied API. A local relay permits only the four approved read-only requests and verifies source, User-Agent and authentication. Runs use temporary encrypted credentials and remove them on success or setup failure; missing environment variables exit 1 without exposing the key.This scoped staging delivery supersedes #25 for FAL-311 and excludes unrelated master changes. The source-owned acceptance manifest maps all three ticket criteria to the candidate checks and the commissioned staging smoke. Windows verification also found and fixed portable Node/npm launch and import-path issues in the build and package checks.
Validation on 601c114:
FAL-311