fix: address full code-review findings (correctness, security, CI)#3
Merged
Conversation
- telemetry: redact --api-key/--env/--app-url values from argv before shipping events; pass auth headers to curl via 0600 config file instead of process arguments - polling: report PASSED only when every test passed (unknown/cancelled statuses and empty result sets no longer exit 0 in CI); retry on empty results; stop spinner on fatal polling errors; follow retry_of chains when grouping retries; fix off-by-one in failure limit - cloud: defer process.exit until after finally so --json output prints on failed runs and temp files are cleaned; ship failure telemetry on RUN_FAILED; re-emit non-punycode warnings; guard failure-path artifact downloads from masking the run-failed exit code - index: replace citty runMain with a replica that records failure telemetry, honors CliError.exitCode, and prints clean errors for all commands instead of stack traces - gateways: stream downloads via pipeline() so mid-download network errors fail fast instead of hanging or truncating silently; typed ApiError with HTTP status; parse JSON responses inside error handling with operation context; auth-mode-neutral 401 message - auth: serialize session refresh behind a lockfile and re-read/merge config to survive concurrent invocations (CI matrices); sessionOnly option so switch-org works with DEVICE_CLOUD_API_KEY exported; switch-org defaults to the logged-in env's API URL instead of prod - paths: segment-aware common-root computation and anchored prefix stripping shared via src/utils/paths.ts (replaceAll corrupted paths when the root substring recurred or collapsed to '') - methods: abort before upload on 401/403 from SHA dedup check; accept zips without explicit directory entries (ignoring __MACOSX/.DS_Store) and close zip handles on all paths; remove dead code - execution-plan: apply --exclude-flows to workspace config globs; fix double directory join for string runFlow/runScript deps; parse YAML docs beyond the second separator; dedupe sequential flows - cloud/upload: positional flow file works with --app-file/--app-url; '.apk' extension check no longer matches 'myapk'; moropo temp dirs cleaned up after runs and on download errors - metadata-extractor: route sync throws in zip ready handler to reject instead of crashing; close handles on all paths; dedupe parseInfoPlist - misc: corrupt config warns instead of silently logging out; version compare handles prereleases; pagination hint uses actual page size; parseIntFlag rejects negatives/garbage; abort timer cleanup in connectivity checks; no retries on 4xx for Expo URL downloads; env-aware console URLs; correct --json/--json-file exit-code docs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rtion - test-runner: run the suite with an isolated DCD_CONFIG_DIR so the CLI under test can never read or refresh the developer's real session (five auth tests previously depended on local login state) - test-runner: replace the fixed 3s sleep with readiness polling against the mock API (30s deadline, fails loudly), forward mock API output with a prefix instead of leaving pipes unread, fail the run if the server dies mid-suite, kill the server's whole process group on POSIX, and treat signal-killed mocha as failure instead of exit 0 - metadata-extractor test: the expect.fail call was caught by its own catch block (AssertionError instanceof Error), so the test could never fail; assert via a threw flag instead Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- cli-ci: drop --ignore-registry-errors so a failed audit can't pass silently; scope the workflow token to contents: read - npm-publish: refuse prod-tag publishes from any ref other than the production branch (workflow_dispatch could previously publish any non-beta version to npm latest, bypassing release-please) - install.sh: wrap the body in main() so a truncated curl | sh download executes nothing - install.ps1: pin TLS 1.2 on Windows PowerShell 5.x; read the raw unexpanded User PATH and write it back as ExpandString (previously froze %VAR% entries); append instead of prepend and skip when already present Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- src/types/schema.types.ts was a byte-identical, hand-maintained copy of the generated file with no importers; delete it and its eslint ignore entry - CLAUDE.md described the abandoned loopback-server login flow; it now documents the implemented PKCE + claim-polling flow, and the telemetry section reflects the new index.ts runner Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The self-hosted runner's git upgrade made cone-mode sparse-checkout
hard-error on file patterns ("'api/swagger.json' is not a directory"),
breaking every CI run since June 10 with no repo change. Switch the
dcd checkout to non-cone patterns, which support file paths.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
Implements the findings from a full code review of the repo: correctness and security fixes across the command/service/gateway layers, test-harness reliability fixes, and CI/install hardening. Four commits, reviewable independently: core
src/fixes, test harness, CI/install scripts, cleanup/docs.Highest-impact fixes
PASSEDonly when every test passed — unknown/cancelled statuses or an empty result set previously exited 0.cloudno longer skips itsfinallyviaprocess.exit, so--jsonoutput prints on failed runs and temp files are cleaned.--api-key/--env/--app-urlvalues from argv before shipping to Axiom, andflushSyncpasses auth headers to curl via a 0600 config file instead of world-visible process arguments.pipeline()instead of.pipe()+finished(), so mid-download network errors fail fast instead of hanging forever or leaving truncated zips.index.tsreplaces citty's error-swallowingrunMainwith a replica that records failure telemetry, honorsCliError.exitCode, and prints one clean error line (previously: stack trace + duplicated message, no telemetry).DCD_CONFIG_DIR(it previously read — and could refresh — the developer's real session, making five tests depend on local login state), polls mock-API readiness instead of sleeping 3s, and fails loudly when the mock API dies.The first commit's message has the full per-area breakdown (~35 fixes).
Deliberately deferred (follow-up PRs)
strict: trueburn-down (tsconfig currentlystrict: false)live.tsraw-fetch → ApiGateway refactorTest plan
pnpm buildclean,pnpm lint0 errors (28 warnings, down from 36 on dev)pnpm test: 121/121 passing against the mock API--help/--versionexit 0; unknown command shows usage + single error line; unauthenticated command exits 1 with actionable message;verifyAppZipvalidated against the realwikipedia.zipfixture (including__MACOSXentries)🤖 Generated with Claude Code