fix(dci-cli): validate typed path parameters before sending the request

[ektasawant]

Closes CMP-49366.

Summary

dci substituted whatever string it was given into a path parameter slot and sent the request, with no check against the parameter's declared type. restish loads that type onto every Param but never consults it — cli/param.go:40-44, Param.Parse, is a stub that returns its input unchanged — so dci get-ticket 'ticket-id: 318240' built GET /support/v1/tickets/ticket-id:%20318240 and could only fail as a 404. Every 404 the CLI has received on the ticket routes in the last 12 months (14 of 1,156 requests) was this defect, and all of them came from agent-mode sessions, where a 404 reads as "no such ticket" rather than "your identifier was malformed".

New path_validation.go checks each positional argument against its declared type before the request is built: integer via ParseInt(v, 10, 64), which bounds to 64 bits and so matches the spec's format: int64 exactly, number via ParseFloat, boolean via ParseBool, and anything else — including string — accepted unchanged. It handles restish's array[...] type notation by validating each comma-separated element.

Rejection is a USAGE_ERROR with exit code 2 and retryable: false. When the rejected value still carries a recoverable identifier, the error suggests the corrected invocation, which all nine malformed URLs observed in production do:

{"error":{"code":"USAGE_ERROR","message":"invalid value for path argument \"ticket-id\": \"ticket-id: 318240\" is not an integer","hint":"Pass only the value, not the argument name — e.g. dci get-ticket 318240","retryable":false}}

Three properties worth calling out:

• Safe by construction. A parameter with no declared schema loads as string, which accepts anything, so validation cannot start rejecting values for a parameter the spec leaves untyped. Behaviour changes for the 7 ticket commands and cannot regress the other 102 string-typed path parameters.
• Fails open. If operation metadata is unavailable — offline, cold cache — validation is skipped rather than fatal. This check must never be the reason a valid command fails.
• Runs before the request exists. The hook sits in addOutputFlag's PersistentPreRunE, ahead of both stdin buffering for body validation and the destructive-confirmation gate, so --dry-run validates too and no HTTP request is ever built.

Integers must also be in canonical form. A leading + or 0 parses cleanly but reaches the API exactly as written and 404s there, which is the same defect class — so +318240 is rejected and the suggestion is canonicalized, ensuring it cannot be rejected again on the retry.

No server, API-contract, or data changes. The optional server-side complement in omni and the upstream Param.Parse implementation are tracked separately and deliberately not blocking.

Test plan

• go test ./... and go vet ./... pass; gofmt clean. Failure set is identical to main for the same invocation: TestCustomerContextFlag subtests, which are pre-existing and environment-dependent (they time out without network access to api.doit.com). status_shows_oauth_by_default also fails, but only when DCI_API_KEY is set in the environment — unset it before baselining, or it looks like a real failure on both branches.
• Table-driven unit tests over the three shapes seen in production, both bare placeholder names, empty string, -1/0 accepted, 99999999999999999999 rejected via ErrRange, and an array[integer] case. The same table asserts every value is accepted for Type: "string" — the regression guard for the 102 string-typed parameters.
• Error contract asserted: USAGE_ERROR, usage exit code, retryable: false, and the suggestion recovering 318240 from ticket-id: 318240.
• One test drives the real PersistentPreRunE and asserts the operation's RunE is never reached, so the validator cannot be silently disconnected from the command tree.
• End to end against a built binary: baseline reproduced all three production shapes; after the fix all 7 ticket commands exit 2 with no request built, including remove-ticket-tags (a DELETE), which confirms validation precedes the destructive gate.
• Regression checks: a valid ID still builds /support/v1/tickets/318240, the string-typed get-allocation finops:squad:customers is untouched, and --dry-run rejects a bad value while still previewing a good one.
• dci get-ticket <real-ticket-id> returns 200 against the live API. Verified against a real ticket with a live OAuth session: get-ticket, list-ticket-comments and list-ticket-tags all exit 0 and return responses byte-identical to main.
• Edge-case matrix over the branches in path_validation.go — int64 min/max ±1, +/leading-zero/-0 non-canonical forms, whitespace, 1e5/hex/underscore/thousands-separator notations, non-ASCII digits, zero/one/two recovery candidates, command aliases, flags interleaved with positionals, --help, shell completion, and a blocking stdin pipe to prove rejection precedes body buffering.
• Post-release, confirm on Mixpanel board 11443580 that no malformed ticketId appears from a CLI version at or above the release containing this fix. Scope to that version: historical events are permanent and older builds keep emitting bad requests until users upgrade, so the total will not drop to zero.

[ektasawant]

Manual test steps + verification evidence

Verified against b1ae241 (current head). Everything below runs offline against a local echo server, so nothing is sent to production and no real credentials are needed — with one exception, the live API check at the end, which is called out explicitly.

Result

	requests that reached the wire
main (6cdcac3)	18 — every malformed shape, on every ticket command
this PR (b1ae241)	2 — only the two legitimate calls

The 18 captured on main include exactly the shapes seen in production:

GET /support/v1/tickets/ticket-id:%20318240
GET /support/v1/tickets/ticketId:309353
GET /support/v1/tickets/%7B%22ticket-id%22:%20310201%7D
GET /support/v1/tickets/ticketid
GET /support/v1/tickets/+318240
...plus PATCH, /comments and /tags variants

On this PR all 18 exit 2 with no request built, while get-ticket 318240 and get-allocation finops:squad:customers still produce byte-identical URLs to main.

remove-ticket-tags (a DELETE) returns USAGE_ERROR rather than the destructive-confirmation prompt, which confirms validation runs ahead of that gate.

Reproduce it

Requires dci login to have been run once, so the spec cache is warm — -s retargets only the request, the spec still loads from the real API base.

verify-pr68.sh — self-verifying, prints ALL CHECKS PASSED or the failures

#!/usr/bin/env bash
# Verify CMP-49366 path-parameter validation. Sends nothing to production.
set -u

PORT=${PORT:-8123}
LOG=$(mktemp)
BIN=${BIN:-/tmp/dci-pr68}

node -e "require('http').createServer((q,s)=>{console.log('CAPTURED:',q.method,q.url);
  s.writeHead(200,{'content-type':'application/json'});s.end('{}')})
  .listen($PORT,'127.0.0.1',()=>console.log('ready'))" > "$LOG" 2>&1 &
echo_pid=$!
trap 'kill $echo_pid 2>/dev/null' EXIT
for _ in $(seq 20); do grep -q ready "$LOG" && break; sleep 0.2; done
grep -q ready "$LOG" || { echo "FAIL: echo server did not start (port $PORT in use?)"; exit 1; }

export DCI_API_KEY=${DCI_API_KEY:-any-non-empty-value}
export DCI_NO_UPDATE_CHECK=1
S=http://127.0.0.1:$PORT
fail=0

# Every malformed shape must exit 2 (usage) and build no request.
for v in 'ticket-id: 318240' 'ticketId:309353' '{"ticket-id": 310201}' \
         'ticketid' 'ticket-id' 'ticket-id-318240' \
         '+318240' '0318240' '99999999999999999999' '318240.0'; do
  "$BIN" get-ticket "$v" -s $S </dev/null >/dev/null 2>&1
  [ $? -eq 2 ] || { echo "FAIL: get-ticket '$v' should exit 2"; fail=1; }
done

# All 7 ticket commands, not just the two seen in production.
for c in get-ticket update-ticket list-ticket-comments create-ticket-comment \
         list-ticket-tags add-ticket-tags remove-ticket-tags; do
  "$BIN" "$c" 'ticket-id: 318240' -s $S </dev/null >/dev/null 2>&1
  [ $? -eq 2 ] || { echo "FAIL: $c should exit 2"; fail=1; }
done

# --dry-run validates too.
"$BIN" get-ticket 'ticket-id: 318240' --dry-run </dev/null >/dev/null 2>&1
[ $? -eq 2 ] || { echo "FAIL: --dry-run skipped validation"; fail=1; }

# Regressions: these must still be accepted and still reach the wire.
"$BIN" get-ticket 318240 -s $S </dev/null >/dev/null 2>&1 \
  || { echo "FAIL: valid integer ID was blocked"; fail=1; }
"$BIN" get-allocation finops:squad:customers -s $S </dev/null >/dev/null 2>&1 \
  || { echo "FAIL: string-typed path parameter was blocked"; fail=1; }

# The load-bearing assertion: only the 2 valid calls above may appear.
captured=$(grep -c CAPTURED "$LOG")
if [ "$captured" -ne 2 ]; then
  echo "FAIL: expected exactly 2 captured requests, got $captured"
  grep CAPTURED "$LOG"
  fail=1
fi

echo
echo "captured requests (should be only the 2 valid ones):"
grep CAPTURED "$LOG" || echo "  (none)"
echo
[ $fail -eq 0 ] && echo "ALL CHECKS PASSED" || echo "SOME CHECKS FAILED"
exit $fail

git fetch origin && git checkout fix/ekta/dci-cli-path-param-validation/CMP-49366
go build -o /tmp/dci-pr68 .
bash verify-pr68.sh                              # this PR  -> ALL CHECKS PASSED

git stash && git checkout main && go build -o /tmp/dci-main .
BIN=/tmp/dci-main PORT=8124 bash verify-pr68.sh  # main     -> SOME CHECKS FAILED, 18 captured

To see the agent-facing error shape:

DCI_AGENT_MODE=1 /tmp/dci-pr68 get-ticket 'ticket-id: 318240' -s http://127.0.0.1:8123
{"error":{"code":"USAGE_ERROR","message":"invalid value for path argument \"ticket-id\": \"ticket-id: 318240\" is not an integer","hint":"Pass only the value, not the argument name — e.g. dci get-ticket 318240","retryable":false}}

Full-surface regression sweep

Beyond the two spot checks above, every command that takes a path argument (93 of them) was invoked against the echo server on both binaries and the URLs diffed:

	result
URLs byte-identical on both	86
URLs present on main only	7 — exactly the ticket commands
URLs present on this PR only	0

Exit codes differ on those 7 commands and nowhere else. The spec census behind this: 109 path parameters total, 102 string and 7 integer, and the 7 integers are precisely the ticket commands — so there is no other command this change can reach.

Edge cases

41 cases derived from the branches in path_validation.go, asserting both the exit code and whether a request was built:

• Canonical form — int64 min/max and ±1 past each, 0, -0, +318240, 0318240, empty, leading/trailing whitespace, 318240.0, 1e5, 0x4dae0, 3_18240, 318,240, Arabic-Indic and fullwidth digits.
• Recovery — zero, one, and two digit-candidate inputs. ticket 318240 v2 (two candidates) and 99999999999999999999 (overflows) correctly decline to guess and fall back to the generic hint; ticket-id: 0318240 canonicalizes to 318240.
• Integration — the id-of-ticket-get alias, flags before and after the positional, -D, --help (wins over a bad value), and __complete (not blocked).
• Ordering — update-ticket fed a stdin pipe that never produces data exits 2 immediately instead of hanging, which demonstrates rejection precedes body buffering more directly than reading the diff does.

Everything behaved as designed. Negative integers are the one branch that is unreachable in practice: dci get-ticket -1 is rejected by the flag parser (unknown shorthand flag: '1' in -1) identically on main, so the validator never sees it.

Known limitation — the hint is agent-only

In an interactive terminal a human sees one line and no suggestion:

Error: invalid value for path argument "ticket-id": "ticket-id: 318240" is not an integer

The hint above is only rendered in agent mode's JSON envelope, so a human gets told what is wrong but not how to fix it, whereas other error classes in the CLI do print a Hint: line. This is not a regression — pre-existing usage errors such as the arg-count error behave identically on main — and all 14 production occurrences came from agent sessions, so the agent path is the one that matters. Flagging it rather than hiding it; happy to surface the hint in human mode here or in a follow-up.

Test suite

go test ./... failure set is identical to main for the same invocation: TestCustomerContextFlag subtests, pre-existing and environment-dependent (they time out without network access to api.doit.com). Note status_shows_oauth_by_default also fails whenever DCI_API_KEY is set in the environment — unset it before baselining or it reads as a real failure on both branches. The suite is also order-dependent: a subset run fails TestCustomerContextFlagOverride instead, on main too, so compare like-for-like invocations. go vet clean, gofmt clean.

Live API check — done

dci get-ticket <a-real-ticket-id>   # must still return 200

Run against a real ticket with a live OAuth session. get-ticket, list-ticket-comments and list-ticket-tags all exit 0, and each response is byte-identical to main. The valid-ID URL is unchanged before and after, so this confirms the substitution logic end to end against production.