Code-quality pass: fix real defects, hoist shared helpers, harden tests#26
Merged
Conversation
Defect fixes: - --version read from package.json instead of a hardcoded literal that had drifted to 1.0.1 - login now distinguishes "server rejected the key" (refuses to save) from "API unreachable" (saves with a warning), and the validation request gets a 10s timeout so login can't hang forever - createClient() throws the friendly "No API key configured" error itself, so a command that forgets requireApiKey() can't ship a raw 401 - client's public type (FormoClient) now matches runtime behavior — the response interceptor unwraps bodies, so AxiosInstance types lied - readConfig() tolerates null/non-object JSON instead of crashing every command; clearConfig() no longer swallows real write failures - contracts get/update/pipeline/delete validate the chain arg like create does; events/import reject mutually exclusive flags instead of silently preferring one; segments/labels JSON flags validate entries - NO_COLOR honored per spec (presence, not truthiness); color keyed to stderr where decorated output actually goes; parseApiError renders nested details as JSON instead of [object Object] Maintainability: - pagination options/params hoisted to src/lib/pagination.ts (was copy-pasted in five commands); alert create/update share one schema - config path exported (getConfigFile) and honored via FORMO_CONFIG_DIR so tests no longer rewrite the developer's real config file - test suite runs without TEST_TOKEN (live tests skip via the existing probe) — fixes red CI on fork PRs; liveApi probe honors FORMO_API_BASE_URL; charts offline tests no longer skipped with live - tsconfig.test.json wired up as `pnpm typecheck` and run in CI - engines >=22.12 declared (CJS bin require()s ESM incur; older Node crashes at startup); dead npm/pnpm override blocks removed; incur pinned exactly while patched; prepublishOnly builds before publish - new direct tests for lib/json, lib/pagination, chain validation, and the new flag guards (123 -> 156 passing) Docs: README scopes/flags drift (charts scopes are boards:*, import uses the main API), SKILLS.md query syntax (`formo query run`), and a CONTRIBUTING.md rewrite to match the actual repo layout and mocha/tsx test setup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Summary
Full-codebase review (three parallel reviewers: lib layer, command surface, tests/packaging/docs) followed by implementation of the confirmed findings.
Defect fixes
--versionhad drifted — it reported 1.0.1 on the 1.1.0 release. Now read from package.json (src/index.ts); can't drift again.formo loginsaved server-rejected keys with the message "The API might be unreachable" even when the API had explicitly returned 401. Validation is now a discriminated result: rejected keys are refused with a clear error; only genuinely unreachable-API logins save with a warning. The validation fetch also gets a 10s timeout (previously it could hang forever).createClient()now throws the friendly "No API key configured" error itself — previously auth enforcement was opt-in viarequireApiKey()at 40+ call sites, and a forgotten call shipped a confusing server-side 401.AxiosInstancetypes still promisedAxiosResponse<T>. NewFormoClientinterface states what actually comes back (this also let the test tree typecheck — see below).readConfig()no longer crashes every command when the config file containsnullor other non-object JSON;clearConfig()no longer swallows real write failures (EACCES/EROFS), soformo logoutcan't claim success while the key remains on disk.contracts get/update/pipeline/deletenow validate the chain arg likecreatedoes;events ingestandimport walletsreject mutually exclusive flags (--event+--events,--addresses+--rows) instead of silently preferring one;segments create --filter-setsandprofiles labels create --labelsvalidate entries; page/size flags are.int().positive().2>logcaptured raw ANSI);parseApiErrorrenders nested detail values as JSON instead of[object Object];formo -hshows the banner like--help.Maintainability
src/lib/pagination.ts— was copy-pasted byte-identical in five commands; alertscreate/updateshare one schema (were duplicated).getConfigFile()) and overridable viaFORMO_CONFIG_DIR— the four hardcoded~/.config/formo/config.jsondisplay strings now can't lie, and unit tests no longer rewrite the developer's real config file (they use a temp dir).process.exit(1). This fixes red CI on fork PRs where secrets are unavailable. The probe also honorsFORMO_API_BASE_URL, and charts' offline validation tests no longer get skipped along with the live ones.tsconfig.test.jsonwas dead config (referenced by nothing). It's nowpnpm typecheck, runs in CI, and passes.engines: node >=22.12declared — the published bin is CJS andrequire()s ESM incur, which only works unflagged on Node ≥22.12; older Node crashed at startup with no npm warning (root cause family of this week's CI saga).overrides+pnpm.overridesin package.json; the live copy is in pnpm-workspace.yaml); incur pinned exactly while patched (patch hunks shift on minor bumps);prepublishOnlybuilds before publish so a stale localdist/can't ship;devscript uses tsx (ts-node dropped).lib/json,lib/pagination, chain validation, board title validation, and all the new flag guards.Docs
boards:read/write(was documenting nonexistentcharts:*scopes → guaranteed 403s), import-wallets flow corrected (main API +profiles:write, not "events API"), missing flags documented (--slack-property-keys,--search,--events, charts update typed flags, seven lifecycle tuning flags), alerts update full-replace semantics stated.formo query "<sql>"→formo query run "<sql>"(agent-facing file; the old form errors), funnel params list synced,logout/--logicadded.Deliberately not changed
chartskeeping--board-idas an option vs positional (internally consistent; changing is a breaking UX change).--recipient(takes a JSON array) not renamed to--recipients— worth doing with a deprecated alias in a dedicated PR.profiles search --conditionskeeps its custom parse for the richer FilterCondition error message.alerts updatefull-replace and charts GET-merge-PUT semantics (API-driven; now documented instead).Test plan
pnpm build,pnpm lint,pnpm typecheckcleanpnpm test: 156 passing, 18 pending — both with and without TEST_TOKEN--version→ 1.1.0,status, no-key error path🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.