v0.18 — contract hardening 2 (audit fixes before the 1.0 freeze)#18
Merged
Conversation
Cluster C of the v0.18 contract-hardening audit fixes: - CRITICAL: CSV --upsert no longer deletes a matched record's extra emails/phones. The match (identity) field is now excluded from the UPDATE diff via stripMatchField, so re-asserting the single match email never narrows the record's email/phone set to one value. - Matching --by a monetary/address custom field is refused (exit 64) instead of silently failing to match and creating a duplicate every run: v2 returns those as objects, which the scalar compare can't match. - parseCsv strips a leading UTF-8 BOM so Excel 'CSV UTF-8' exports import cleanly instead of failing with a misleading 'missing name column'. - Non-integer org_id/owner_id CSV cells are rejected (exit 65) instead of serializing as null and silently unlinking the relation (new intCell); prepareImportBodies now wraps special-column errors with the row number. - Non-numeric values for numeric custom fields are rejected (exit 65) instead of writing null (input.js coerceValue). 1444 tests, 100% coverage.
…ross-host redirects Cluster A + D of the v0.18 contract-hardening audit: - Network unreachable / DNS failure / timeout now exits 69 (service unavailable), not 70 (internal bug): the fetch rejection is caught, retried like a 5xx, and surfaced as ServiceUnavailableError with the cause. The user-set --timeout no longer reports an 'internal bug'. - SECURITY: transport sets redirect:'manual' and refuses any 3xx (exit 78) so the x-api-token is never re-sent to a redirect target off the host-locked company domain (Node strips authorization cross-origin but not custom headers). - Exhausted 429 retries now exit 75 (rate limited) with the last wait, not 69 'API is unavailable' — a sleep-and-retry script keys on 75. - The alias hook maps oclif ExitError/CLIError exit codes (carried on err.oclif.exit, parse errors → 64) instead of collapsing every aliased failure to 1, and prints the human message that oclif's bypassed top-level handler would have shown. - Failed/rejected deal & lead conversions exit 65 (bad data), not 70. - A rejected OAuth refresh (invalid_grant 400 / invalid_client 401) exits 77 with re-auth guidance, not 65; 5xx refresh failures stay 69. Tests pin real oclif error shapes (not fabricated ones) and the corrected 429/network/redirect codes. 1453 tests, 100% coverage.
Cluster B of the v0.18 contract-hardening audit — the machine-output
contract that v1.0 will freeze:
- New BaseCommand.outputAction(machineObject, humanMessage): prints the
human one-liner in interactive table mode, else emits the structured
object through outputResults (honoring --output/--jq/--fields).
- Every delete/remove, both converts, deal bulk-update, person/org import,
and backup now route their result through it instead of printing prose
to stdout. `deal delete 42 --output json | jq` yields { id, deleted }
instead of an unparseable string; the converts expose conversion_id and
the NEW record id (deal_id/lead_id) in JSON — no more scraping prose.
Per-row failure detail in bulk-update/import moves to stderr so a piped
stdout stays a clean parseable summary.
- formatCsv with no explicit columns (the machine-format path passes {})
now derives columns from the union of the rows' keys and JSON-encodes
nested values, instead of returning a silently-blank header+rows — which
in `changes --output csv` advanced the watermark over never-written rows
(silent feed data loss).
- --fields now projects keys for json/yaml output, not only table/csv.
1485 tests, 100% coverage.
Cluster E of the v0.18 contract-hardening audit: - api: the documented "pipe stdin" body now works (resolveBody is called for any body-bearing method, not gated on --body); malformed --body JSON exits 65 (was an internal 70); the dead --body-required path exits 64 (was an off-ladder 2). - errors: a usage/parse error in a TTY now honors an explicit --output by recovering it from argv (this.flags isn't populated when parsing fails), so --output json gets the JSON error envelope instead of a human message. - changes --limit: no longer silently skips rows that share the cut row's update-time second — on truncation it drops the trailing same-second rows so they replay next run, and warns (never silently loses) when one second exceeds --limit. - search: clamps a scoped per-entity --limit to 100 (the live API 400s above it) instead of surfacing the 400 as exit 65; corrects the stale comment. - bulk targets: blank/empty segments no longer become a phantom id 0; an empty target set exits 64; malformed piped JSON exits 65; entries without an integer id are rejected. - deal history --limit now caps collection (passes the limit to collectPages) instead of walking the whole changelog at 20 tokens/page. - digest rejects an explicit --output combined with --format (exit 64). 1500 tests, 100% coverage.
…ing help topics, isolate config tests Cluster F (build/hygiene) of the v0.18 audit: - docs:commands now runs 'oclif manifest' first, so it works from a clean checkout instead of ENOENT-ing on the gitignored manifest. - Removed the 'undici' dependency: it was declared and pinned but never imported (the client uses Node's global fetch). - Added the 10 missing oclif.topics entries (file, filter, goal, lead, note, pipeline, product, project, stage, webhook) so 'pdcli --help' shows real topic summaries instead of a random subcommand's description. - config tests now isolate the conf store to a temp dir via PDCLI_CONFIG_DIR (a new override hook) so a test run never reads or mutates the user's real pdcli profiles.
- exit-codes.mdx: corrected to the actual ladder (parse errors 64, network/ timeout 69, 429-exhaust 75, conversion-fail 65, refresh-fail 77, redirect refusal 78) and the error-output-mirrors-success-format rule; documents watch's exit 8. It previously described the pre-0.17 behavior. - Fixed two stale facts: scoped vs itemSearch both cost 20 tokens (not 20 vs 40); the daily-budget message says 'token budget', not 'request budget'. - Regenerated the command reference for the 10 new help-topic descriptions. - Bumped version to 0.18.0.
Adversarial review of the v0.18 diff confirmed the fixes are correct (no functional regressions); these address the papercuts it surfaced: - upsert stripMatchField no longer over-strips: it drops the match field on update ONLY when the body holds just the single injected match value (the CSV path). A multi-value emails/phones body is an explicit full set and is written as-is; scalar/custom matches are left to diffBody (which already drops equal values and allows a genuine rename) — so matching by a field no longer blocks editing it. - lookup: the not-searchable error no longer lists 'address' as a valid type (it was removed from SEARCHABLE_TYPES in v0.18). - changes: the 'splits a single second' warning no longer fires spuriously when --limit truncation lands on null-update_time rows (they can't be resumed by updated_since anyway); corrected the secondOf comment. 1503 tests, 100% coverage.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
2 participants
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.
Second contract-hardening pass from a full quality audit + adversarial contrarian review, closing the exit-code, machine-output, and data-safety gaps a 1.0 freeze would lock in. 1503 tests, 100% coverage; live-verified on the sandbox.
Data safety (headline)
--upsertno longer deletes a matched record's other emails/phones (the match/identity field is excluded from the update diff). Live-verified: a matched update kept both emails.org_id/owner_idcells rejected (65) instead of nulling the link.changes --limitno longer skips rows sharing the cut second;--output csvno longer emits silently-blank rows (thechangeswatermark feed-loss).Security
redirect: 'manual'and refuses any 3xx (78) — the API token is never re-sent off the host-locked domain.Exit-code ladder (BREAKING — observable codes)
api --body→ 65 (was 70). 70 is now internal-bug-only.watch's 8) instead of collapsing to 1.Machine output
--output; converts expose the new record id in JSON;--fieldsworks for json/yaml.Also
apireads piped stdin; TTY usage errors honor--output json;search --limitclamp; bulk-target validation;deal history --limitcost cap;digestflag guard.docs:commandsclean-build, removed unusedundici, 10 missing help topics, config tests isolated (PDCLI_CONFIG_DIR); exit-codes docs rewritten for the real contract.See CHANGELOG for the full list. Deferred to v1.0: a set of LOW cosmetic items (funnel shape, date-grammar unification, etc.).