feat!: non-blocking default, distinct exit codes, JSON error envelopes #3
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/agent-harness-ergonomics"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #2.
Implements the five agent-harness ergonomics fixes from the issue. Breaking major release.
Non-blocking default.
--wait-msdefaults to0: a bare submit returns in seconds with the job id (exit2) instead of blocking 120 s — exactly Claude Code's default Bash timeout, which orphaned paid analyses when the harness killed the wait.Distinct exit codes. Request-level failures leave exit
1, which now means only the server's own failed-job verdict. New codes partition by "could a job have been created?":3usage error (no request dispatched),4HTTP error (status in message and envelope),5transport failure or timeout (unknowable — assume yes),6invalid response on an accepted 2xx (job created; the id is recovered into the envelope when the body still carries a readablejob.id, lost otherwise),7unexpected client error (which additionally dumps the original error, stack included, to stderr). Commander's own usage errors route throughexitOverride()into code3;--help/--versionstay0.JSON error envelopes. Under
--json, every failure prints exactly one JSON document on stdout:{"error": {"kind", "message", "status?", "job_id?", "exit_code"}}withkind∈usage/transport/http-error/invalid-response/unexpected.jq -r '.error.kind // .job.status'discriminates both shapes in one expression. The--jsonflag is recovered by an argv scan for failures that occur before commander's action runs.Orphan semantics documented. The README now states what a killed submit means: the server runs the paid analysis to completion, there is no listing or recovery route, and
--sessiondoes not recover it. The resubmit rule is a single predicate — safe iffcode == 3 || (code == 4 && status < 500 && status != 408);408is excluded alongside 5xx because an intermediary can mint either after forwarding the POST. The stdin read-to-EOF hang is documented alongside. (The server-side listing route stays out of scope for this client-only repo.)HTTP status preserved.
readErrorMessagesuffixes(HTTP <status>)even when the body parses as a recognized JSON error shape, so a 5xx whose body reads like a refusal can no longer masquerade as a real 4xx.Pre-dispatch guards. Eleven review rounds hardened the exit-
3boundary so it only ever claims what is provable: URL shape validation (http(s) only, no credentials/query/fragment, the WHATWG bad-port list copied from undici 8.7.0), API-key field-value validation vianode:http'svalidateHeaderValue(which matches undici's own dispatch-time check exactly), single-path-segment--jobids, and non-blank--sessionids. Anything the guards cannot prove classifies astransport— "assume a job exists" is the deliberate default, since a false "safe to resubmit" costs a duplicate paid analysis while a false "stop" costs one wasted stop. No credential can reach an error message or cause chain, and--helpnow ends with a two-line exit-code legend.The design went through an adversarial debate before implementation; behavior was verified empirically against throwaway local servers (401/500/schema-invalid/pending bodies, unreachable hosts, stalled and truncated bodies, bad flags, all 65535 ports against the bad-port list) — every documented exit code matches observed behavior. 106 tests, lint, typecheck, build, knip, and format all pass.
Approach review: The approach is solid and well-suited to the problem domain.
The exit-code taxonomy (3–7) partitioning by "could a job have been created?" is the right question for an agent harness deciding whether to retry a submit. The JSON error envelope shape
{error: {kind, message, status?, exit_code}}is concise and machine-parseable; thejq -r '.error.kind // .job.status'discrimination pattern is elegant. The non-blocking default (--wait-ms 0) directly addresses the orphaned-job problem from harness timeouts.The
CliErrorclass carrying a discriminatedCliErrorDetailis clean: the class provides stack traces and cause chains, the detail provides structured classification for the envelope.classifyFailurecorrectly maps all throw sources (commander errors, plain errors, non-Error values) into the sameFailureReporttype, whichformatFailurethen renders uniformly.The
readJsonFlagFromArgvargv scan to recover the--jsonflag for errors that occur before commander's action runs is a pragmatic workaround for commander's lack of parsed-option access in error callbacks. The false-positive risk (another option's value matching--json) is documented and benign — it only produces an extra stdout envelope.createUrlextraction fromremote-query-client.tsinto its own module is justified now thatread-remote-client-options.tsalso needs it for early URL validation. Theparse-integer-option→parse-wait-ms-optionreplacement is an appropriate specialization (range validation,InvalidArgumentErrorfor commander integration).Using
process.exitCoderather thanprocess.exit()is correct — it lets the event loop drain and flush stdout, preventing truncated JSON output.No materially better alternative presents itself. The design decisions are well-reasoned and the decomposition is appropriately granular.
Approach review by Approach Review 3 (OpenCode Wafer) (GLM-5.2)
Approach review: The overall design holds up well. Making
--wait-ms 0the default and partitioning failures by "could a job have been created?" is the right axis for an agent-facing client, and routing every failure through oneCliError->classifyFailure->formatFailurepipeline keeps the exit-code and JSON-envelope contract in one place rather than scattered across call sites. Using commander'sexitOverride()so its own usage errors land in the same taxonomy (code3) is the standard facility for this and avoids a hand-rolled pre-parse. The new modules are pure and independently testable, matching the repo's functional-core/imperative-shell split, and thereadErrorMessage/readCauseChainMessageextractions are the right granularity.One material alternative, noted inline:
readRemoteClientOptionsvalidates the base URL by building a throwaway URL and discarding it, then hands back a rawbase_urlstring thatremote-query-clientre-parses per request — a validate-and-discard shape the repo's "Parse, Don't Validate" rule asks you to avoid.Two smaller observations that did not seem to warrant inline comments. (1) The failure path is rendered in two places — the action's own
catchand theparseAsynccatch — differing only in how--jsonis discovered (parsedoptions.jsonvs. the argv scan). The sharedformatFailurekeeps the duplication to a few lines and the split has a real justification (commander can throw before--jsonis parsed), so this reads as a deliberate trade rather than a problem; it is just a second place to update if the failure output ever grows. (2) The contract this PR exists to establish — process exit code plus exactly one stdout document — is exercised only through pure-function unit tests, while the risk concentrates in the wiring (exitOverride, the argv-scan fallback, the bin wrapper'sexitCode = 7). The PR body says this was verified by hand against local servers; AGENTS.md explicitly discourages shell tests, so this is a judgment call worth making consciously rather than a change to request.Approach review by Claude Code Opus (opus)
@ -15,0 +19,4 @@// Opaque-path bases (mailto:, data:) parse alone but throw once a path resolves against themtry {createUrl(url, "/v1/queries");This constructs a URL purely to see whether it throws, discards the result, and returns the original string as
base_url—remote-query-clientthen callscreateUrl(options.base_url, path)again on every request. AGENTS.md's "Parse, Don't Validate" rule targets exactly this shape ("Treatvoid-returning checks with suspicion").Consider having this function keep what it learned: parse once and return the parsed base (e.g.
base_url: new URL(normalized)or aresolveRoute(path)closure onRemoteClientOptions), so the request path receives an already-valid base andcreateUrlcan no longer throw at dispatch time. That also removes the need for the probe path"/v1/queries"to stand in for the real routes — today a base that happens to be valid for/v1/queriesbut not for/v1/jobs/<id>would slip past this check (unlikely in practice, but it is the kind of gap the throwaway probe invites).The validation itself is correct and the opaque-path comment is genuinely useful; the suggestion is only about carrying the parse result forward instead of dropping it.
Approach review: The overall design holds up well. The failure taxonomy is modeled as a
CliErrorcarrying a discriminatedCliErrorDetail, classified once by a pureclassifyFailure, rendered by a pureformatFailure, and emitted by a thin shell — that is the right shape for this repo's functional-core/imperative-shell rule, and it keeps the published exit-code contract testable without spawning processes. Extractingcreate-url,read-error-message, andread-cause-chain-messageout ofremote-query-clientis a clear improvement, and splitting the invalid-response case (2xx + unparseable body, exit6) from transport failures (exit5) is what makes the "was a job created?" table decidable at all..exitOverride()routing commander's own usage errors into the same envelope is the right call, and usingprocess.exitCoderather thanprocess.exit()avoids the stdout-truncation trap that would otherwise undercut the single-JSON-document guarantee.Two structural notes, both non-blocking and left as inline comments: the failure path is rendered at two sites with two different sources of truth for
--json, and the--urlcheck validates then discards its parse.Smaller observations, not worth changing on their own:
300000client-side cap on--wait-msduplicates a server-side business limit, so a server-side raise needs a client release to become usable. The trade is deliberate and documented (fast exit3, plus keeping the timer below theAbortSignal.timeoutbound), and given the orphan-safety story that "provably no job created" is worth more than the coupling costs.classify-failure.tsimportsCommanderError, which couples an otherwise pure module to the CLI framework. Translating commander errors intoCliErrorat thecli.tsboundary would keep the core framework-free, but the current form is one smallinstanceofand reads fine.exitCode === 0help/version branch, the stdout-only line filter for commander errors, and the argv--jsonfallback. That matches the repo's stated "test the core, not the shell" convention, so it is a conscious trade rather than an oversight — worth remembering if this wiring grows.Approach review by Claude Code Opus (opus)
@ -81,0 +81,4 @@emitLines(rendered.lines);process.exitCode = rendered.exitCode;} catch (error) {const rendered = formatFailure(classifyFailure(error), options.json === true);The action's
catchand the top-levelcatchdo the same three things (classify, format, emit, set exit code) and differ only in how they learn about--json:options.jsonhere, an argv scan there. SinceparseAsyncrejects with whatever an async action handler throws, the innercatchcan be dropped entirely and the outer one can prefer the parsed value:program.opts()is exact for anything commander parsed successfully — which covers every error that reaches the outer handler from the action — and the argv scan then only backstops failures that occur before option parsing completes, which is the case it was written for. That leaves one rendering site, so a future change to the envelope or the stream routing cannot apply to one path and miss the other. Non-blocking; the current split is correct as written.@ -15,0 +19,4 @@// Opaque-path bases (mailto:, data:) parse alone but throw once a path resolves against themtry {createUrl(url, "/v1/queries");This calls
createUrlpurely for its throw and discards the result, thenrequestQuerySubmissioncallscreateUrlagain per request — the validate-and-discard shape the repo's "Parse, Don't Validate" rule warns about. Returning the parsed value instead (a normalized/validated base URL inRemoteClientOptions, or aURL) would mean the request path consumes something already known-good rather than re-deriving it, and would make the leftover rawTypeErrorat request time — currently classifiedunexpected, exit7— structurally unreachable rather than unreachable by argument about which paths the probe covers. No behavioral bug today:/v1/queriesand theencodeURIComponent-escaped/v1/jobs/<id>resolve identically against any base the probe accepts.Summary: Reviewed the new failure taxonomy (
CliError+classifyFailure+formatFailure), the non-blocking--wait-ms 0default, the commanderexitOverride()wiring, and the README rewrite. The core design holds up: I verified against Node 26 thatresponse.json()rejects withSyntaxErroron a non-JSON 2xx body (→ exit 6) and with aTimeoutErrorDOMException(which isinstanceof Error, soisAbortErrormatches) when the timeout fires mid-body (→ exit 5), thatcreateUrlthrows for opaque-path and schemeless bases, and that commander's_exit(0, ...)for--help/--versionmakes theerror.exitCode === 0guard insrc/cli.tscorrect.parseWaitMsOptionis also safe for oversized digit strings (Number("99999999999999999999") > 300000), so dropping the oldBigIntcheck loses nothing.Found 3 issues, all in the boundary between "usage error, provably no job" (exit
3) and "transport failure, assume a job exists" (exit5). Because the README instructs agents to stop and report after an exit5on a submit, every pre-dispatch failure that leaks into thetransportbucket costs a real recovery.Code review by Claude Code Opus (opus)
@ -15,0 +19,4 @@// Opaque-path bases (mailto:, data:) parse alone but throw once a path resolves against themtry {createUrl(url, "/v1/queries");🟡 Medium: This validates that a path resolves against the base but not that the resulting URL is fetchable, so any non-HTTP scheme passes and the failure lands in the wrong exit-code bucket.
Verified on Node 26:
Both reject before a single byte is dispatched, but
remote-query-client.tscatches them as{ kind: "transport" }→ exit5. The README defines exit5as "dispatched, but no response completed" and tells the caller to assume a job was created and to "report and stop — do not resubmit". So a scheme typo inTROPKOD_URLpermanently blocks the caller on a config error that provably created nothing.Safe fix — reject non-HTTP schemes here so it stays a usage error (exit
3):@ -65,2 +60,3 @@);}throw error;throw new CliError({ kind: "transport" }, readCauseChainMessage(error), error);🟢 Low: This catch-all treats every non-abort throw from the
tryblock astransport(exit5= "assume a job was created"), but the block also contains work that happens strictly before dispatch:createUrl(...)andfetch's own header validation.Verified on Node 26 — an API key containing a control character is rejected synchronously by the Headers constructor, before any connection:
(
.trim()inreadRemoteClientOptionsstrips surrounding whitespace, so this needs an interior control character — a mangled copy-paste of a provisioned key.) The result is exit5on a submit, which per the README's orphan protocol means "report and stop", for a credential typo that created nothing and is safe to retry.Narrowing the pre-dispatch cases keeps the taxonomy honest: build the request URL and
Headersoutside thetry(or classify aTypeErrorwhose message comes from header construction as{ kind: "usage" }), leaving this line for genuine network failures. Same root cause as the URL-scheme comment onread-remote-client-options.ts.@ -24,3 +26,3 @@if (options.job !== undefined) {if (questionParts.length > 0 || stdinQuestion.length > 0) {throw new Error("pass either a question or --job, not both");throw new CliError({ kind: "usage" }, "pass either a question or --job, not both");🟢 Low:
--jobis only checked for!== undefined, so an empty or whitespace-only id passes through unvalidated.tropkod-client --job ""buildsGET /v1/jobs/and dispatches it, turning a usage error into an HTTP404(exit4) — and--job " "sends/v1/jobs/%20%20%20. It also makes--job "" "some question"fail with the misleading "pass either a question or --job, not both" rather than pointing at the empty id.Given that this PR's contract is that exit
3means "no HTTP request was dispatched", a guard here keeps that partition intact:Summary: Found 2 medium and 1 low issue.
I ran the changed source directly (Node 26, commander 15, zod 4) against throwaway stub servers and confirmed every exit code the README claims:
3for commander usage errors / missing--url/ bad--url/ question+--job,4with status for 401 and 500,6for a non-JSON and a schema-invalid 2xx,5for ECONNREFUSED and for both the pre-response and mid-body timeout abort,2for a pending submit and for a--jobpoll,0for--help/--version. The--jsonstdout contract holds in all of them, including the commander-error path where only the stdout envelope is emitted and commander's prose stays on stderr. The 74 tests pass. The README's commander caveat (message carries theerror:prefix and the suggestion newline, minus the(add --help for usage)hint) is byte-accurate.The findings are about the one path the PR's own premise cares most about: a 2xx whose body the client cannot parse. The job id sitting in that body is discarded, and the README then tells a polling agent to keep polling a condition that will never change.
Code review by Claude Code Opus (opus)
@ -80,0 +105,4 @@| `3` | Usage error — invalid flags/arguments/environment; no HTTP request was dispatched. | No | Nothing was sent — fix the input, resume polling the same id || `4` | HTTP error — non-2xx response; numeric status in message and envelope. | 4xx: no. 5xx: unknown (a proxy may mask an accepted request) — branch on `status` | The GET touched nothing: 401/403 → fix credential, keep polling; 404 → re-check the id (a confirmed-correct-id 404 ends the poll); 408/429/5xx → keep polling || `5` | Transport failure — dispatched, but no response completed (network failure, or the client abort after `wait_ms + 5000` ms / the fixed 30 s poll timeout — including an abort mid-body). | Unknown — assume yes | Job untouched — keep polling || `6` | Invalid response — 2xx received but the body was not a parseable submission envelope. | Yes — the id is lost inside the unreadable body | Job untouched — keep polling; report if persistent |🟡 Medium: "keep polling; report if persistent" is unbounded advice for a condition that is frequently deterministic. When exit
6comes from schema drift rather than a truncated body, the job is already in a terminal state and every subsequentGET /v1/jobs/<id>returns the same unparseable payload — I confirmed this by polling a stub that returns acompletedjob whoseanalysis.statusis outside the union: exit6, identical message, forever. An agent following this row loops until its own budget runs out on a job that finished long ago.Worth making the escape hatch explicit, e.g. "keep polling, but treat a repeated exit
6carrying an identical message as terminal — the job may have already ended in a shape this client cannot render — and report." If the id-recovery suggestion onsrc/remote-query-client.tslands, theOn a submit: job created?cell for this row ("the id is lost inside the unreadable body") also stops being true.@ -77,1 +53,3 @@console.error(line.text);try {let stdinText: string | undefined;if (options.job === undefined && questionParts.length === 0 && !process.stdin.isTTY) {🟢 Low: stdin is read to EOF before
readRemoteClientOptionsvalidates--url/--api-key, so an invocation that is provably a usage error hangs instead of exiting3. With noTROPKOD_URLset and stdin inherited as a pipe that never closes, the process blocks indefinitely even though the URL check alone already determines the verdict — the hang the README warns about, in a case where nothing about stdin can change the outcome.Hoisting the
readRemoteClientOptions({ url: options.url, apiKey: options.apiKey })call above this block makes credential and URL errors fail fast at exit3. The missing-question check necessarily still waits on stdin, since stdin is the question source.@ -78,3 +95,1 @@const normalizedBaseUrl = baseUrl.endsWith("/") ? baseUrl : `${baseUrl}/`;const relativePath = pathname.startsWith("/") ? pathname.slice(1) : pathname;return new URL(relativePath, normalizedBaseUrl).href;const parsed = QuerySubmission.safeParse(payload);🟡 Medium: A 2xx body that fails
QuerySubmission.safeParsediscards the job id even when the body plainly contains it, which is exactly the orphaned-paid-analysis case this PR exists to prevent.Verified against a stub returning HTTP 200 with
{"session":…,"query":…,"job":{"id":"job-ABC","status":"completed",…},"analysis":{"status":"partially-answered"}}:job-ABCwas readable in the payload and appears nowhere in the output.QuerySubmissionis strict in ways the server can trivially outgrow — a newanalysis.status, a newgrounding.type, anUnresolvedAnalysisReasonoutside the literal union,resolved_targetsnot exactlylength(1)— and every one of those turns a successful submit into an unrecoverable paid job.A lenient pre-parse before the strict one keeps the id:
Then carry
recovered.data.job.idinto the message (… (HTTP 200, job <id>)) and into the error envelope as ajob_idfield, so an exit6on a submit degrades to "keep polling this id" instead of "report and stop".All seven distinct findings from the five reviews are addressed in
d6e4549(every one reproduced or verified before fixing; none rejected). Per conversation:read-remote-client-options.ts— URL parsed then discarded / non-http(s) scheme → exit 5 (30893, 30901, 30915): Agreed on both counts, one fix:readRemoteClientOptionsnow parses once and returnsbase_urlas a slash-normalizedURL, rejecting non-http(s) schemes as usage/exit 3 — verified on Node 26.5 thathtp://andfile:///previously passed the probe and died at fetch as transport/exit 5. The protocol gate subsumes the opaque-path probe, and requests consume the parsed base, making the dispatch-time URL throw structurally unreachable.remote-query-client.ts— headerTypeErrormisclassified as transport (30916): Confirmed the repro (Headersthrows on an interior control character pre-dispatch). Fixed at the boundary: the key is validated as a header value inreadRemoteClientOptionsand carried forward as theauthorizationstring, throwing usage/exit 3. With URL construction also out of the try, the transport catch now covers only genuine network failures.cli.ts— two failure-render sites (30900): Adopted with one refinement:asJson = error instanceof CommanderError ? readJsonFlagFromArgv(process.argv) : program.opts().json === truerather than the proposed OR — the OR would extend the argv scan's documented false positive to action-phase failures. Verified on commander 15 thatparseAsyncrejects with the action's rejection andopts()stays readable, so the inner catch is gone and one render site remains.resolve-submission-request.ts— empty--jobdispatchesGET /v1/jobs/(30917): Agreed — it broke the exit-3 "no request dispatched" partition. The id is trimmed, empty throws usage/exit 3 (--job requires a job id) before the question/--jobconflict check, and the trimmed id is dispatched.remote-query-client.ts— exit 6 discards a recoverable job id (30925): Agreed — this is the orphan case the PR exists to close. A lenient{ job: { id } }pre-parse on schema-mismatched 2xx now appends the recovered id to the message and emits.error.job_idin the envelope, so exit 6 on a submit degrades to "poll this id" whenever the body was readable JSON.README — unbounded "keep polling" on exit 6 (30926): Agreed. The row now distinguishes a transient blip (retry briefly) from repeated identical exit 6 (terminal for this client build — report), and the submit cell documents the
.error.job_idrecovery.cli.ts— stdin read before URL/key validation can hang (30927): Agreed — options are fully parsed before the action, so validation is hoisted above the stdin read; URL/credential mistakes exit 3 immediately. The missing-question check necessarily still waits on stdin, which the README's open-pipe warning continues to cover.Verification: 88/88 tests, lint/typecheck/build/knip/format green, plus empirical spot-checks of each fix against throwaway local servers (mailto:/htp:// → exit 3; newline key → exit 3; schema-mismatch 2xx → exit 6 with
.error.job_id;--job " "→ exit 3; never-closing stdin + missing URL → immediate exit 3; envelope/stderr contracts under--jsonpreserved).Summary: Reviewed the exit-code taxonomy, JSON error envelopes, and the non-blocking default. The failure classification is coherent end to end —
CliErrordetail →FailureReport→ exit code → envelope — and I verified the trickier claims hold: commander writes its own prose andshowHelpAfterErroroutput to stderr (so the "exactly one JSON document on stdout" contract survives usage errors),--help/--versionthrowCommanderErrorwithexitCode === 0and correctly fall through to exit0, the argv--jsonscan matchesprocess.argv.slice(2)under the bin wrapper,Headers.setdoes reject the interior CR/LF/NUL the api-key check relies on, and the api key never reaches any printed message (onlyreport.messageis emitted; causes are dropped). Docs and code agree on every exit code I traced.Two findings, neither blocking correctness of the taxonomy itself:
src/remote-query-client.ts: the default submit now has a 5 s total timeout, and blowing it yields exit5, the README's unrecoverable orphaned-job case.src/read-remote-client-options.ts: trailing-slash normalization before parsing drops the base path for query/fragment-bearing base URLs.Code review by Claude Code Opus (opus)
@ -15,0 +17,4 @@}// Trailing slash so request paths resolve under the base path instead of replacing itconst normalized = url.endsWith("/") ? url : `${url}/`;🟢 Low: The trailing slash is appended to the raw string before parsing, so it lands in the last URL component rather than the path. For a base URL carrying a query or fragment the base path is then silently dropped:
Requests (with the bearer token) go to the wrong path with no error. Normalizing after parsing avoids it:
(The plain no-slash case,
https://h/api, is handled correctly today — only query/fragment-bearing bases misroute.)@ -22,2 +13,3 @@input: { question: string; session_id: string | undefined; wait_ms: number },): Promise<QuerySubmission> {const timeoutMs = (input.wait_ms ?? 0) + requestTimeoutSlackMs;const timeoutMs = input.wait_ms + requestTimeoutSlackMs;🟡 Medium: With
--wait-msnow defaulting to0, this makes the total abort budget for a submit exactlyrequestTimeoutSlackMs= 5000 ms — down from 125 s before this change. The slack constant was sized as headroom on top of a server-side wait, not as a standalone request budget, so it now also has to cover DNS, TLS handshake, any cold start, and the server's own session/query/job insert.When 5 s is exceeded on a submit that the server did accept,
AbortSignal.timeoutfires and the client reports exit5— which the README defines as the unrecoverable case ("Unknown — assume yes", "After losing a submit without an id, report and stop — do not resubmit"). That is precisely the orphaned-paid-job outcome this PR is trying to eliminate, reintroduced on a much shorter fuse. Verification against local throwaway servers wouldn't surface it, since loopback latency is ~0.Suggest giving the non-blocking submit a floor rather than raw slack, e.g.
const timeoutMs = Math.max(input.wait_ms + requestTimeoutSlackMs, defaultJobFetchTimeoutMs);— the 30 s already deemed acceptable for a poll. A synchronous--wait-mssubmit is unaffected.Summary: Reviewed the exit-code partition, JSON envelope contract, and request plumbing. The failure taxonomy holds up well — I ran the suite (88 tests pass) and exercised the built CLI against local servers covering 401, 500-with-JSON-body, non-JSON 200, schema-drift 200 (with and without a recoverable
job.id), unreachable host, slow server, commander usage errors,--help/--version, and a crashingdist/cli.js; every observed exit code and stdout/stderr split matched the README.Three actionable findings, all medium: the new
--wait-ms 0default collapses the submit budget to 5 s (a slow-but-successful create now aborts as exit5, i.e. an orphan — the exact failure this PR targets); the trailing-slash normalization runs on the raw string, so a base URL carrying a query or fragment silently drops its base path; and the README's exit-6orphan discriminator overstates recovery for readable JSON that carries nojob.id.Code review by Claude Code Opus (opus)
@ -80,0 +106,4 @@| `3` | Usage error — invalid flags/arguments/environment; no HTTP request was dispatched. | No | Nothing was sent — fix the input, resume polling the same id || `4` | HTTP error — non-2xx response; numeric status in message and envelope. | 4xx: no. 5xx: unknown (a proxy may mask an accepted request) — branch on `status` | The GET touched nothing: 401/403 → fix credential, keep polling; 404 → re-check the id (a confirmed-correct-id 404 ends the poll); 408/429/5xx → keep polling || `5` | Transport failure — dispatched, but no response completed (network failure, or the client abort after `wait_ms + 5000` ms / the fixed 30 s poll timeout — including an abort mid-body). | Unknown — assume yes | Job untouched — keep polling || `6` | Invalid response — 2xx received but the body was not a parseable submission envelope. | Yes — id recovered in `.error.job_id` when the body was readable JSON; lost only when it wasn't | Job untouched. A transient blip (truncated body, proxy garbling) recovers — retry once or twice. A repeated exit `6` with an identical message is deterministic schema drift, terminal for this client build: the job has likely already ended in a shape this client cannot render — report it, don't keep looping |🟡 Medium: This row — and the matching parenthetical on line 120 ("a readable-JSON exit
6recovers the id in the envelope and orphans nothing") — state that a readable-JSON body always yields.error.job_id. It does not:recoverJobIdrequires ajob.idstring specifically, so readable JSON without that shape orphans exactly like a non-JSON body.Verified against a local server returning
200 {"foo":1}:Readable JSON, no
job_id. That is a realistic drift shape (a renamed envelope key, a proxy-injected error document served as 200), and it is the case where the id is lost.Since this section is the contract an agent branches on to decide "report and stop" versus "I have the id," the discriminator should be the field itself rather than the body's parseability — e.g. "id recovered in
.error.job_idwhen the body carried a readablejob.id; lost otherwise," and on line 120 "an exit6whose envelope carries.error.job_idorphans nothing; an exit6without it does."@ -15,0 +17,4 @@}// Trailing slash so request paths resolve under the base path instead of replacing itconst normalized = url.endsWith("/") ? url : `${url}/`;🟡 Medium: The trailing slash is appended to the raw string before parsing, so when the URL carries a query or fragment the slash lands inside the search/hash instead of the path — and the base path is then silently discarded during relative resolution.
Observed against a local server:
--urlhttp://host/apiPOST /api/v1/queries✅http://host/api?tenant=1POST /v1/queries❌http://host/api#fragPOST /v1/queries❌http://host/api?tenant=1becomeshttp://host/api?tenant=1/, whosepathnameis/api(no trailing slash) and whosesearchis?tenant=1/;new URL("v1/queries", base)then replaces the last path segment and drops the search. The result is a request to an unintended path on the same host, with the bearer token attached, surfacing as a confusing 404 (exit4) rather than a usage error.Parse first, then normalize the parsed path — and drop the parts that base resolution cannot carry anyway:
(Rejecting a URL that carries a query or fragment as a usage error is the other defensible option — either way it should not fail silently.)
@ -22,2 +13,3 @@input: { question: string; session_id: string | undefined; wait_ms: number },): Promise<QuerySubmission> {const timeoutMs = (input.wait_ms ?? 0) + requestTimeoutSlackMs;const timeoutMs = input.wait_ms + requestTimeoutSlackMs;🟡 Medium: With
--wait-msnow defaulting to0, this makes the total client budget for a submit 5 s — down from 125 s before this PR.requestTimeoutSlackMswas network slack layered on top of a server-side wait; it is now the entire budget for DNS + TCP + TLS + server accept.Verified against a local server that delays 8 s:
Per the README, exit
5on a submit means "assume yes" for job-created and "report and stop" — so a create that merely took 6 s (cold start, TLS handshake on a fresh connection, brief queueing at the edge) becomes an unrecoverable orphaned paid analysis, which is the failure mode this PR sets out to eliminate.A floor keeps the non-blocking promise (the timeout is only an upper bound; a healthy server still returns in milliseconds) while removing the cliff:
That also lines the submit budget up with
defaultJobFetchTimeoutMs, which already grants a plain GET 30 s.Round-2 findings are addressed in
6f80771(all three reproduced before fixing; both code issues were flagged independently by both reviewers). Per conversation:remote-query-client.ts— 5 s submit budget with--wait-ms 0(31053, 31056): Confirmed against an 8 s-delay server — the default submit aborted at 5000 ms with exit 5, the exact orphaned-job case this PR exists to eliminate. Fixed with the suggested floor:Math.max(wait_ms + requestTimeoutSlackMs, baseRequestTimeoutMs), wherebaseRequestTimeoutMsis the renamed 30 s constant the poll GET already uses, so the submit floor and poll timeout share one source of truth. Verified: the 8 s server now returns the pending envelope (exit 2); a 45 s server aborts at exactly 30 s;--wait-ms 30000aborts at 35 s. README exit-5 row documentsmax(wait_ms + 5000, 30000).read-remote-client-options.ts— trailing slash appended pre-parse (31054, 31057): Confirmed —--url https://h/api?tenant=1produced basehttps://h/api?tenant=1/and requests hit the host root, silently dropping/apiwith the bearer token attached. Fixed by parsing first, then rejecting any base carrying a query or fragment as usage/exit 3 and normalizingparsed.pathnamewith the trailing slash. Rejection beats silent clearing: relative resolution can never honor a base query/fragment, so accepting one always discards caller intent — and the taxonomy has an exact pre-dispatch bucket for it. Tests cover no-slash, trailing-slash, query, and fragment cases; verified zero requests reach the server on rejection.README — exit-6 row overstates recovery (31058): Confirmed —
recoverJobIdrequires ajob.idstring, and a local 200{"foo":1}yields an exit-6 envelope with nojob_id: readable JSON, id lost. Both the exit-6 row and the line-120 parenthetical now discriminate on the envelope field itself — id recovered iff.error.job_idis present, lost otherwise — with the lead-in broadened since schema-drift JSON orphans identically to non-JSON.Checks: 92/92 tests, lint/typecheck/build/knip/format green, 18/18 empirical spot-checks passed.
Summary: 1 low-severity defect and 1 observation. No critical, high, or medium issues found.
I exercised the head sources against throwaway local HTTP servers rather than relying on reading alone. Verified as documented: exit
2on a pending submit (JSON and human output),3for every usage path (missing URL, URL with a query, header-invalid API key, empty stdin, empty--job, question+--job, commander's unknown-option and rejected--wait-ms),4with the status preserved on a 401 and on a 5xx whose body reads like a refusal,5on an unreachable host and on the 30 s abort while the body hung mid-stream, and6(withjob_idrecovered) on a schema-drifted 2xx. Also confirmed: commander prints its own prose to stderr beforeexitOverride()throws,--help/--versionthrow aCommanderErrorwithexitCode === 0,program.opts()is populated when the action rejects,CommanderErrorfrom@commander-js/extra-typingsis a runtime value andinstanceofholds for errors thrown by commander itself, andAbortSignal.timeoutdoes not keep the event loop alive after a fast response.The usage-error-before-stdin ordering, the
max(wait_ms + 5000, 30000)floor, the always-appended(HTTP <status>)suffix, and the stdout-only envelope for commander errors all behave as the README describes.Code review by Claude Code Opus (opus)
@ -80,0 +109,4 @@| `6` | Invalid response — 2xx received but the body was not a parseable submission envelope. | Yes — id recovered iff the envelope carries `.error.job_id` (the body held a readable `job.id`); lost otherwise | Job untouched. A transient blip (truncated body, proxy garbling) recovers — retry once or twice. A repeated exit `6` with an identical message is deterministic schema drift, terminal for this client build: the job has likely already ended in a shape this client cannot render — report it, don't keep looping || `7` | Unexpected client error (a bug in this CLI). | Unknown — assume yes | Job untouched — report the bug; polling the same id stays safe |After a failed **submit**, resubmitting is safe iff `code == 3 || (code == 4 && status < 500)` — and only when every earlier submit in the task is accounted for. A failed **poll** never licenses a resubmit: `GET /v1/jobs/<id>` can neither create nor destroy a job — repair the input and keep polling.ℹ️ Info: No action needed — recording a consequence of the taxonomy that is worth being deliberate about.
Exit
5covers both "never reached the server" and "aborted after dispatch", and this rule plus the orphaned-jobs section then tells an agent to report and stop without resubmitting. A mistypedTROPKOD_URLlands here: I confirmed an unreachable host produces{"kind":"transport","exit_code":5}with messagefetch failed: bad port, indistinguishable in the envelope from a mid-body abort even though no job can possibly exist. So after fixing the URL, an agent following the documented rule still refuses to resubmit.If that trade is intentional (conservative beats a duplicate paid analysis), it reads fine as written. If not, the cheap version is surfacing the connect-time cause in
.error.message—readCauseChainMessagealready yieldsECONNREFUSED/ENOTFOUND/bad port— and documenting that those substrings prove nothing was dispatched, without adding an exit code.@ -25,2 +27,2 @@if (questionParts.length > 0 || stdinQuestion.length > 0) {throw new Error("pass either a question or --job, not both");const jobId = options.job.trim();if (jobId.length === 0) {🟢 Low:
--jobis only checked for emptiness, so a job id consisting of dot segments silently rewrites the request path instead of being rejected.fetchRemoteJobbuilds/v1/jobs/${encodeURIComponent(jobId)}and resolves it against the base URL.encodeURIComponentescapes/,?and#, but leaves.and..untouched, and the URL parser then collapses them. Verified against a local server:--job ".."issuesGET /v1/(the server logged exactly that), and because my stub answered with a valid envelope the client exited2for a job id that does not exist. Against the real service that request lands on some unrelated route, and a 2xx there yields exit6— which the README tells agents to read as "deterministic schema drift, terminal for this client build: report it", sending them after a phantom client bug.A guard here keeps the id a single path segment, alongside the existing empty check:
Fixing it in
remote-query-client.tsinstead does not work: theURLpathnamesetter normalizes dot segments too, so the rejection belongs at this parse boundary.Summary: Found 1 low-severity issue. The exit-code taxonomy, JSON envelopes, and error classification hold up under empirical testing.
I reconstructed the CLI from the PR head in a scratch directory (real
commander@15/zod@4, Node 26) and drove it against stub servers to verify the documented contract rather than reason about it. All 92 tests pass. Verified end to end:3: missing--url, unparseable/mailto:/htp:URL, URL with query or fragment, API key with a newline or a non-latin1 character,--job " ", question-plus---job, commander's unknown option and rejected--wait-ms(stdout carries exactly one envelope, commander's prose stays on stderr,(add --help for usage)is absent frommessageas the README states);4: 401 and 500 with a nested error body, 404 on a poll, status echoed in bothmessageandstatus;5: refused connection, DNS failure, connection reset mid-body, and a real 30 sAbortSignal.timeoutagainst a server that never responds (tropkod request timed out after 30000ms, process exits promptly — the timeout timer does not hold the loop open);6: HTML body on a 2xx, schema drift with a recoverablejob.id(job_idin the envelope) and without;2/0: pending envelope in both JSON and text mode, base-path normalization (--url http://h:8746/proxy→POST /proxy/v1/queries),--terminator correctly suppressing the argv--jsonscan;--url, the client exits3immediately instead of blocking, and a question passed as an argument never touches stdin.The removal of
parseIntegerOption'sisSafeInteger/BigIntguard is safe inparseWaitMsOption: any digit-only string that survives the> 300000check is exactly representable, and oversized inputs (99999999999999999999) are rejected by the range branch.Code review by Claude Code Opus (opus)
@ -51,3 +43,4 @@headers["content-type"] = "application/json";}const requestUrl = new URL(request.path.slice(1), options.base_url);🟢 Low: A
--jobid that is exactly.or..silently retargets the request instead of failing.encodeURIComponentleaves dots unescaped, sonew URL("v1/jobs/..", base)collapses the segment during relative resolution.Reproduced against a stub server:
--job ".."→GET /v1/--job "."→GET /v1/jobs/Both carry the
authorizationheader and neither is a job fetch. Ids containing a slash are safe (encodeURIComponentyields%2F, whichURLdoes not normalize), so the blast radius is just these two degenerate values — but the failure is silent, and whatever the alternate route returns gets mapped into the exit-code taxonomy as if it were a verdict about that job: a 2xx with an unparseable body becomes a repeated exit6, which the README tells agents to read as "deterministic schema drift, terminal for this client build".A guard in
resolveSubmissionRequest, next to the existing empty-id check, keeps the fix in the functional core:That preserves exit
3("nothing was sent — fix the input, resume polling the same id"), which is the honest verdict here.Round-3 feedback processed at
9ec02a6:--jobids (both inline comments onresolve-submission-request.tsandremote-query-client.ts) — fixed. The guard sits at the parse boundary next to the existing empty-id check, exactly as suggested:.,.., and any id containing/now throw a usage error (--job id must be a single path segment, exit3— "nothing was sent, fix the input, resume polling the same id"). Tests cover both dot segments, a slash-bearing id, and an id with interior dots (job.v2.42) that must stay accepted.5conflation of never-dispatched vs aborted-after-dispatch (README, info) — acknowledged; the trade is intentional. Conservative-beats-duplicate is the design: a false "report and stop" costs one wasted stop, a false "safe to resubmit" costs a paid analysis. The connect-time cause already surfaces in.error.messagevia the cause chain (ECONNREFUSED,ENOTFOUND, …), but we deliberately don't document those substrings as dispatch proofs — branching on stderr wording is exactly what the exit-code taxonomy exists to replace, and Node's cause chains aren't a stable contract.Summary: Found 2 medium issues.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
@ -15,0 +22,4 @@} catch (error) {throw new CliError({ kind: "usage" }, "--url or TROPKOD_URL is not a valid URL", error);}if (parsed.protocol !== "http:" && parsed.protocol !== "https:") {🟡 Medium: URLs with userinfo pass this validation, but Node Fetch rejects a URL such as
https://user:password@host/before dispatch withRequest cannot be constructed from a URL that includes credentials. The catch inrequestQuerySubmissionthen reports exit 5 (transport) even though no job could have been created, and its error message echoes the full URL—including the password—to stderr or the JSON envelope. Reject non-emptyparsed.usernameorparsed.passwordhere as ausageerror with a constant message.@ -69,3 +67,2 @@if (!response.ok) {const text = await response.text();throw new Error(readErrorMessage(text, response.status));const text = await response.text().catch(() => "");🟡 Medium: This expression converts every failure while consuming a non-2xx body into an empty body and still returns exit 4. With the request timeout signal, a 503 that sends headers and stalls mid-body makes
response.text()reject withTimeoutError, so the documented exit 5 timeout path is never used for HTTP error bodies; connection resets are hidden the same way. Handle the body read with the same abort/transport classification as the 2xxresponse.json()path, or narrow the documented exit-code contract to match this behavior.Summary: Reviewed the non-blocking-default / exit-code / JSON-envelope rework. I ran the branch end-to-end against throwaway local servers (Node 26, commander 15, zod 4) and the whole documented taxonomy reproduces exactly:
--help/--version→ 0, pending submit → 2 with the envelope, failed job → 1, usage/commander errors → 3 (one JSON doc on stdout, commander prose on stderr), 401/500 → 4 withstatus, unreachable host and mid-body termination → 5, non-JSON and schema-drift 200s → 6 (withjob_idrecovered when the body carried one). Base-path joining (/proxy/base+/v1/queries), job-id encoding (a b?c#d,%2e%2e,..,jobs/42), theBearerheader, and the pre-stdin URL/credential check (170 ms exit 3 against an open pipe) all behave as documented. The full suite passes (95 tests).Two findings, both in
read-remote-client-options.ts: a URL carrying userinfo escapes the new validation and is misclassified as exit 5 while echoing the password, and the header-validationcausecaptures the raw API key.Code review by Claude Code Opus (opus)
@ -15,0 +25,4 @@if (parsed.protocol !== "http:" && parsed.protocol !== "https:") {throw new CliError({ kind: "usage" }, "--url or TROPKOD_URL must be an http(s) URL");}if (parsed.search !== "" || parsed.hash !== "") {🟡 Medium: This validation rejects a query/fragment but not userinfo, and a base URL with credentials escapes to the fetch layer where it is misclassified as a transport failure.
The Fetch
Requestconstructor throwsTypeError: Request cannot be constructed from a URL that includes credentialsbefore dispatching anything. That TypeError lands in thecatcharoundfetchinremote-query-client.ts, is not an abort error, and so becomesCliError({ kind: "transport" })→ exit5. Reproduced on this branch:Two concrete consequences:
5is the one code the README defines as "dispatched, but no response completed… Unknown — assume yes", and the Orphaned jobs section instructs agents to "report and stop — do not resubmit the question". A purely local--urltypo therefore makes a harness believe it may have created and paid for an analysis, and forbids the retry that would actually fix it. The provably-correct code here is3(usage, no request dispatched).Fix in the same guard, next to the query/fragment check:
That keeps the failure inside the provably-no-job exit
3class and stops the password from being printed.@ -20,0 +48,4 @@throw new CliError({ kind: "usage" },"--api-key or TROPKOD_API_KEY contains characters not allowed in an HTTP header",error,🟢 Low: Passing this
erroras thecauseembeds the raw API key in the thrownCliError. undici's message interpolates the rejected header value verbatim:Nothing prints it today —
formatFailurerenders onlyreport.message, and theconsole.error(error)added inbin/tropkod-clientonly fires when the module import itself rejects — so this is latent rather than an active leak. But it is a credential sitting oneconsole.error(error)(or onereadCauseChainMessagereuse) away from stdout, and the cause carries no diagnostic value the CliError message doesn't already state. Dropping the third argument removes the exposure at no cost.Round-4 feedback processed at
7e15428:5and echoing the password (both Medium inline comments onread-remote-client-options.ts) — fixed with exactly the suggested guard, next to the query/fragment check: non-emptyparsed.usernameorparsed.passwordnow throws a usage error with the constant message--url or TROPKOD_URL must not include credentials(exit3, provably no request dispatched, retry licensed). Tests assert the message is constant, the detail isusage, and the thrown error carries no cause — so the password can never reach stdout, stderr, or the envelope. The flags table now lists credentials alongside query/fragment.4(Medium,remote-query-client.ts) — resolved by the second remedy offered: the contract is narrowed to match the behavior, deliberately. On a non-2xx, the status line is the verdict and the body only decorates the message; reclassifying a stalled/reset error body as transport would lose information — worst case, a 4xx (provably no job created, resubmit safe) whose body stalls would become exit5("assume a paid job exists, never resubmit"), strictly worse for the caller. The swallow now carries a why-comment in code, the exit-4row states that the status alone decides, and the exit-5row's "abort mid-body" is scoped to a 2xx response.cause(Low,read-remote-client-options.ts) — fixed; the third argument is dropped and a comment records why (undici interpolates the rejected header value verbatim). The existing interior-newline test now also assertscauseisundefined.Summary: Found 1 medium issue. The exit-code taxonomy, JSON envelopes, and README claims otherwise hold up under direct testing.
I built the branch and verified the documented behavior empirically rather than by reading alone:
pnpm test(97 passed),tsc -b --noEmit, andtsc -p tsconfig.app.jsonall pass.4withstatus; 500 with a{error:{message}}body -> exit4(the(HTTP 500)suffix is preserved, so it can no longer masquerade as a 4xx); non-JSON 2xx and empty 2xx -> exit6invalid-response; a drifted 2xx carryingjob.id-> exit6withjob_idrecovered in the envelope; pending/completed/failed ->2/0/1.5. Usage: missing URL,--wait-ms abc,--wait-ms 300001, unknown option -> exit3with exactly one JSON document on stdout, commander prose only on stderr.--help/--version-> exit0with human text.TROPKOD_URL, the client exits3immediately; with the URL set it blocks until EOF, exactly as the new README paragraph states.http://h/proxy->GET /proxy/v1/jobs/job-42, and--job '%2e%2e'is double-encoded to/v1/jobs/%252e%252e(no path collapse). The--url/--api-keyguards are complete for header injection:Headers.setrejects CR/LF/NUL and any code point above U+00FF before dispatch.dist/cli.jsyields exit7with empty stdout.Code review by Claude Code Opus (opus)
@ -15,0 +33,4 @@}// Fetch rejects credential-bearing URLs pre-dispatch; constant message so the password never printsif (parsed.username !== "" || parsed.password !== "") {throw new CliError({ kind: "usage" }, "--url or TROPKOD_URL must not include credentials");🟡 Medium: The pre-dispatch guards here cover credentials and header-invalid API keys, but not a port that
fetchrefuses to connect to. WHATWGfetchblocks a fixed list of ports (9, 22, 6000, 10080, ...), and it rejects before opening a socket. That rejection lands in thecatcharoundfetchinremote-query-client.ts, is not an abort, and so becomesCliError({kind:"transport"})-> exit5.Verified on the built branch:
Per the README this is the worst possible verdict for a purely local config typo: exit
5on a submit means "job created: unknown — assume yes", and the Orphaned jobs section instructs the caller to "report and stop — do not resubmit the question". So a mistyped port makes an agent abandon the task and warn about a paid analysis that was never queued, when the truthful verdict is exit3("no HTTP request was dispatched").Safe fix: reject the blocked ports here, alongside the existing scheme/query/credential checks, so the failure keeps the provably-no-job code:
Sniffing the
bad portcause string inremote-query-client.tswould also work but is fragile across runtimes; validating at the boundary matches how the credential and header-value cases are already handled.Summary: Found 1 medium and 1 low issue. The exit-code taxonomy holds up under testing.
I ran the CLI (native TS execution, Node 26) against throwaway local servers and confirmed every documented code path: exit
2on a pending submit,3for missing url / missing api-key / empty--job/--job ../ url-with-query / url-with-credentials,4for 401 and for a 500 whose body stalls (30 s, status preserved — not downgraded to transport),5for DNS failure, blocked port, no-response, and a stalled 2xx body,6for a non-JSON 2xx and for schema drift both with and without a recoverable.error.job_id. Commander'sexitOverridebehavior matches the README caveat exactly:--help/--versionthrowCommanderErrorwithexitCode === 0and exit0; usage errors carry commander's message (including the(Did you mean …)suggestion newline) without the(add --help for usage)hint, print prose on stderr, and still emit exactly one JSON document on stdout under--json. Base-path joining (/proxy→/proxy/v1/queries),encodeURIComponenton job ids, and theHeaders.setprobe for control characters in the api key all behave correctly, and the probe's constant message keeps the key out of the output.AbortSignal.timeoutis unref'd, so the longer submit budget cannot hold the process open after a verdict.QuerySubmissionstill tolerates unknown server fields, so exit6really is reserved for drift the client cannot render. All 97 tests pass.Code review by Claude Code Opus (opus)
@ -83,3 +122,3 @@## Agent RuleReach for `tropkod-client` when you need to check a claim about a public npm or crates.io dependency ("does `fastify@5.6.0` expose `schemaCompiler`?") against the dependency's real source rather than guessing from memory or package metadata. Run `tropkod-client --help` before first use to confirm the current flags, then submit one inline-named question (`npm:<pkg>@<version>` or `crate:<name>@<version>`) and read `--json` output for a scriptable verdict. It needs `TROPKOD_URL` and `TROPKOD_API_KEY` in the environment.Reach for `tropkod-client` when you need to check a claim about a public npm or crates.io dependency ("does `fastify@5.6.0` expose `schemaCompiler`?") against the dependency's real source rather than guessing from memory or package metadata. Run `tropkod-client --help` before first use to confirm the current flags, then submit one inline-named question (`npm:<pkg>@<version>` or `crate:<name>@<version>`) with `--json`. The default submit is non-blocking: it prints the job envelope and exits `2`; poll `--job <id> --json` while it exits `2`, branching per the exit-code table otherwise. Branch on exit codes `3`–`7` and on `.error.kind`/`.error.status` instead of stderr wording; on polls, only a stdout verdict, a confirmed-correct-id 404, or a repeated exit `6` with an identical message (deterministic schema drift — report, don't loop) ends the loop. It needs `TROPKOD_URL` and `TROPKOD_API_KEY` in the environment.🟡 Medium: The poll protocol has no cadence and no bound. This Agent Rule says to "poll
--job <id> --jsonwhile it exits2", and the exit-code table adds "keep polling" for exit5, and for exit4on 401/403/408/429/5xx — but nothing anywhere in the README states an interval between polls or a maximum number of attempts. An agent implementing this literally runsGET /v1/jobs/<id>in a tight loop for the full duration of an analysis, which is exactly the behavior that earns the 429 the table then instructs it to keep polling through. The termination conditions listed here (a stdout verdict, a confirmed-correct-id 404, a repeated identical exit6) never fire for a service that is simply down, so an exit4/5xx or exit5loop has no exit at all.This is the one piece of the new non-blocking contract that isn't pinned down; every other branch in the table is precise enough to implement mechanically. Suggested fix: state a concrete interval (e.g. wait ~5 s between polls, backing off on 429/5xx and honoring
Retry-Afterif present) and an overall bound after which the agent reports the job id and stops, rather than polling forever.@ -0,0 +7,4 @@});const FastifyErrorBody = z.object({message: z.string(),🟢 Low: An error body with an empty
messageproduces a message that is just a leading space plus the status. Both schemas acceptz.string(), so{"message":""}at 503 returns" (HTTP 503)"and{"error":{"message":""}}at 502 returns" (HTTP 502)"(verified by callingreadErrorMessagedirectly). That lands in the JSON envelope as"message": " (HTTP 503)"and on stderr as a line beginning with a space — strictly less informative than thetropkod request failed (HTTP 503)fallback it displaces, and it makes.error.messageawkward to log.Use
z.string().min(1)on bothmessagefields (line 5 and line 10) so an empty or missing message falls through to the generic status-only text.Round-5 feedback processed at
6ab48b1:5(Medium,read-remote-client-options.ts) — fixed at the parse boundary as suggested. The guard carries the full WHATWG bad-port list, copied verbatim from undici 8.7.0'slib/web/fetch/constants.js— the exact fetch implementation bundled in the Node this client targets — rather than from memory or a partial sample. Verified the original repro end-to-end:TROPKOD_URL=http://127.0.0.1:22 … --json "q"now prints{"error":{"kind":"usage","message":"--url or TROPKOD_URL uses a port that HTTP clients refuse to connect to","exit_code":3}}and exits3. On the structural alternative (classifying pre-dispatch fetchTypeErrors as usage): it can't close this family, and the repro shows why — undici reports a bad port asfetch failed: bad port, a network-error shape per the fetch spec's own wording ("return a network error"), structurally indistinguishable from a real connection failure without the cause-string sniffing the finding rightly calls fragile. The parse boundary is the only place this can be caught deterministically, and with credentials, header values, scheme, query/fragment, and now ports all guarded there, the remainingfetch failedcauses are genuine post-dispatch network operations — honest exit-5territory. If the runtime's list ever grows past our copy, the failure degrades to today's exit5, never anything worse.5/5xx for that long, means stop and report the job id. The Usage section's poll sentence carries the cadence too. 30 s rather than the suggested ~5 s: polls are cheap but analyses take minutes, so a faster cadence buys nothing while multiplying requests sixfold." (HTTP 503)"(Low,read-error-message.ts) — fixed exactly as suggested:z.string().min(1)on both message fields, so an empty message falls through to thetropkod request failed (HTTP <status>)fallback. Literal tests cover both body shapes.Summary: No actionable issues found.
I verified the change rather than reading it alone: installed the runtime deps in a scratch copy, ran the suite (11 files / 101 tests pass), and exercised
src/cli.tson Node 26 against stub servers for each documented path.Observed behavior matched the README's contract in every case I could reach:
2with the envelope;--jobpoll → exit2; stdin submit → exit2--wait-ms→ exit3, exactly one JSON doc on stdout, commander's prose on stderr only--help/--version→ exit0, human text (documented exception)4withstatusand the(HTTP n)suffix preserved on both6withoutjob_id; schema drift carryingjob.id→ exit6withjob_id: "job-XYZ"recovered5(fetch failed: connect ECONNREFUSED …, the AggregateError descent works); headers-then-stall 2xx → exit5after 30.2 s with the timeout message, confirming the mid-body abort claim in the exit-5 row:1,:9) → exit3pre-dispatch, so they never masquerade as transport failures; the port list matches the WHATWG bad-port list exactly--url/--api-keywith a never-closing stdin fifo → exit3immediately, confirming the pre-stdin ordering insrc/cli.ts:53--job ../../etc,--job a/b→ exit3;encodeURIComponentplus the dot/slash guard closes the path-traversal surface (%2e%2ere-encodes to%252e%252e)Two security-relevant points I checked specifically and found sound: a cross-origin redirect drops the
Authorizationheader (verified with a redirect + sink server — the sink sawnull), and the credential/header-validation errors inread-remote-client-options.tsare raised with a constant message and nocause, so neither the URL password nor the API key can reach stderr.The
readJsonFlagFromArgvfalse positive is real but correctly bounded:--session --json qis accepted by commander with--jsonas the session value, the run succeeds, andprogram.opts()(not the scan) governs output, so the scan only ever over-prints on a commander error path — as its comment states.Code review by Claude Code Opus (opus)
Summary: Found 1 medium issue.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
@ -20,0 +140,4 @@const authorization = `Bearer ${apiKey}`;try {new Headers().set("authorization", authorization);🟡 Medium:
Headers#setis weaker than the validation undici applies when serializing a request. It accepts API keys containingU+007Fand several C0 controls (for exampleU+0001), butfetchthen rejects them withinvalid authorization headerbefore opening a socket. That path is classified as exit5/transport, so automation is told to assume a job may exist even though no HTTP request was dispatched; correcting and resubmitting would actually be safe. Usenode:http'svalidateHeaderValuehere (while continuing to discard the key-bearing cause) or explicitly reject the additional controls so these inputs remain exit3usage errors.Round-6 feedback processed at
6a7c270— this one closes the whole failure family, not just the instance:Headers#setlaxer than undici's dispatch-time validation (Medium,read-remote-client-options.ts) — fixed at both layers.node:http'svalidateHeaderValue, as suggested — verified empirically to reject exactly what undici's field-value check rejects (C0 controls includingU+0001,U+007F, CR/LF, NUL) while accepting TAB and obs-text, whereHeaders#setacceptedU+0001/U+007F. Constant message, still no cause. Verified end-to-end: a key containing0x01now prints{"error":{"kind":"usage","message":"--api-key or TROPKOD_API_KEY contains characters not allowed in an HTTP header","exit_code":3}}and exits3.5, so the family is now closed at the classification layer too. Probing the real rejection shapes showed the discriminator the earlier rounds lacked: undici tags every client-side-invalid request withcode: "UND_ERR_INVALID_ARG"on the cause chain (TypeError: fetch failed→InvalidArgumentError: invalid authorization header, constant message, no value interpolation) and rejects before any connection attempt, while genuine network failures carryECONNREFUSED/ENOTFOUND-class codes.isRejectedBeforeDispatchwalks the cause chain for that code and classifies the rejection asusage/exit3(no cause on the thrown error, keeping whatever undici found invalid out of every detail). Any future input shape undici validates more strictly than the boundary does now degrades to a truthful exit3, not a false "assume a paid job exists". The boundary guards stay for their better messages; the backstop is the safety net the enumeration approach was missing. Exit-code table updated accordingly (exit3now names requests the HTTP client itself refuses to send before connecting).Summary: Reviewed the non-blocking default, exit-code partition, and JSON error envelopes. The implementation matches the documented contract; I found no correctness or security defects, only 2 minor items (1 low, 1 info).
What I verified empirically on Node v26 (no
node_modulespresent, so the suite itself was not run):TypeError: fetch failedwith causeError: bad portand nocode, soisRejectedBeforeDispatchcannot see it — the client-sideblockedPortsset is genuinely needed, and its entries match the WHATWG bad-port list exactly.TypeErrorthat echoes the password in its message; rejecting those inreadRemoteClientOptionsbefore dispatch is what keeps that string out ofreadCauseChainMessage.Headers#setbut rejects at dispatch asInvalidArgumentError/UND_ERR_INVALID_ARG(messageinvalid authorization header, no value echoed) — thevalidateHeaderValuepre-check and theusageclassification are both justified.DOMExceptionis aninstanceof Errorin Node, soisAbortErrorcorrectly catches theTimeoutErrorfromAbortSignal.timeout; a mid-body abort that surfaces asTypeError: terminatedinstead still lands on thetransportfallback, so exit5holds either way.new URL("v1/jobs/x", base)resolves under the base path oncepathnameis slash-terminated, and--jobids that are./../contain/are the only onesencodeURIComponentwould let collapse the path — all three are rejected.The commander integration also checks out:
showHelpAfterError(string)writes the hint to stderr (not full help to stdout), so the "exactly one JSON document on stdout" contract survives usage errors, and--help/--versionthrowCommanderErrorwithexitCode === 0as the catch assumes.Code review by Claude Code Opus (opus)
@ -41,3 +51,3 @@### JSON output`--json` prints the full submission envelope. Combine with `jq` for scripting:`--json` makes every invocation that reaches a verdict print exactly one JSON document on stdout: `.job` present means the server's submission envelope; `.error` present means the invocation failed. Two exceptions: (1) `--help`/`--version` print human text and exit `0`; (2) a crashed client exits with empty stdout — exit `7` from the bin wrapper's catch, or the runtime's own exit `1` on a crash outside the handlers. Empty stdout is never a verdict.ℹ️ Info:
.jobpresent is described as "the server's submission envelope", but what--jsonprints isJSON.stringifyof the Zod-parsed value:z.objectstrips unknown keys, andanalysis's.nullish().transform(v => v ?? undefined)drops the key entirely when the server sendsnull(JSON.stringifyomits undefined-valued keys — confirmed).So a server that adds a field (say
job.progress) sees it silently vanish from--jsonoutput rather than pass through. That's pre-existing behavior and arguably the right call given the exit-6schema-drift contract, but "the server's submission envelope" reads as pass-through to an agent building on this. Something like "the parsed submission envelope (fields this client build knows)" would set the expectation correctly.@ -15,0 +109,4 @@try {parsed = new URL(url);} catch (error) {throw new CliError({ kind: "usage" }, "--url or TROPKOD_URL is not a valid URL", error);🟢 Low: This is the one
CliErrorin the file that keeps a credential-bearing cause.new URL()throws aTypeErrorwhoseinputproperty holds the raw string, so--url 'https://user:hunter2@'(unparseable host, so it fails here rather than at the credentials check 12 lines below) parks the password inCliError.cause.input.Nothing currently prints causes — only
bin/tropkod-client'sconsole.error(error)would, and that path isn't reachable from here — so the impact is latent. But it contradicts the deliberate policy stated twice in this same file (// No cause: keep the raw key out of every error detail, and the constant message on the credentials branch). Dropping the third argument costs nothing, since the message is already constant and the cause is never surfaced.Summary: Found 1 low-severity documentation gap. No correctness, security, or error-handling defects.
I verified the behavioral claims empirically rather than by reading alone: built the branch with TypeScript 7.0.2 (
tsc -b --noEmitandtsc -p tsconfig.app.jsonboth clean), ran the suite (12 files / 107 tests pass), and exercised the builtbin/tropkod-clientplussrc/cli.tsagainst throwaway local HTTP servers.Every exit code in the new README table matched observed behavior:
0--help/--version;2pending submit (envelope withjob.idon stdout)3missing--url, unparseable URL, credential-bearing URL (password not echoed), query/fragment, WHATWG-blocked port, header-invalid API key,--job ""/".."/"a/b", and every commander-originated usage error (unknown option,--wait-ms -1/300001/1e5/"")4401 and 500 with(HTTP <status>)appended andstatusin the envelope5ECONNREFUSED("fetch failed: connect ECONNREFUSED …")6non-JSON 2xx, and schema drift both withjob_id: "job-ABC"recovered and without7from the bin wrapper's catch (verified by removingdist/)Also confirmed: the stdout contract holds under
--jsonfor commander errors (exactly one JSON document on stdout, commander's prose only on stderr, and the envelopemessageis commander's wording minus the(add --help for usage)hint, exactly as the README's caveat says); base-path URL joining is correct (http://h/base/api+--job job-42→GET /base/api/v1/jobs/job-42,Authorization: Bearer k); the submit body is{"question","session_id"?,"wait_ms"}withwait_ms: 0by default; movingreadRemoteClientOptionsabove the stdin read really does avoid the open-pipe hang (process exits in ~0.1 s against a FIFO held open for 25 s); andAbortSignal.timeout(305000)does not keep the event loop alive after a fast response.The blocked-port list matches the WHATWG bad-port list exactly, and the
--jobdot-segment/slash rejection correctly covers whatencodeURIComponentleaves unescaped.Code review by Claude Code Opus (opus)
@ -69,1 +88,3 @@| `--json` | — | `false` | Print the full JSON envelope || Flag | Env fallback | Default | Notes || ----------------- | ----------------- | --------------- | ------------------------------------------------------------------------------------------------------------------------------------------- || `--url <url>` | `TROPKOD_URL` | none (required) | tropkod service URL; the hosted deployment is `https://api.tropkod.ai`; must not include credentials, a query, or a fragment |🟢 Low: This row enumerates the
--urlrejection rules ("must not include credentials, a query, or a fragment") but omits two more thatreadRemoteClientOptionsalso enforces with exit3: a non-http(s)scheme (--url or TROPKOD_URL must be an http(s) URL) and any port on the WHATWG bad-port list (--url or TROPKOD_URL uses a port that HTTP clients refuse to connect to).The port rule is the one that can surprise a self-hoster:
--url http://host:6000or:10080— both plausible service ports — fail before any request with a message that has no counterpart in the docs. Since the README is written as the exhaustive agent-facing contract and this row already lists the other constraints inline, worth appending them, e.g.; must be an http(s) URL with no credentials, query, or fragment, on a port fetch will connect to.Round-7 feedback processed at
f001f01:CliErrorkeeps a credential-bearing cause (Low,read-remote-client-options.ts) — fixed; the third argument is dropped, with a comment recording why (new URL()'sTypeErrorcarries the raw string in itsinputproperty). A test now covers the exact scenario from the finding:https://user:hunter2@throws the constant message withdetail: usageandcauseundefined. EveryCliErrorin the file is now cause-free..jobdescribed as "the server's submission envelope" (Info, README) — reworded as suggested: the JSON-output section now says "the parsed submission envelope (the fields this client build knows — unknown server keys are stripped, and anullanalysisis omitted)", so nobody builds on pass-through expectations.--urlrow omits the scheme and bad-port rules (Low, README) — completed with the suggested inline shape: the row now reads "must be anhttp(s)URL with no credentials, query, or fragment, on a port fetch will connect to (the WHATWG bad-port list is rejected client-side)" — the port rule a self-hoster on:6000/:10080would otherwise meet undocumented.Summary: Found 1 low-severity issue. The exit-code taxonomy, JSON envelopes, and README claims hold up under direct testing.
I ran the head revision against throwaway local servers (Node 26, commander 15, zod 4) and confirmed the documented contract end to end:
0for--help/--version(human text even under--json), exit2for a pending submit, exit3for missing--url, unknown options,--wait-ms 300001, blocked ports (:22), credential-bearing URLs,--job .., and a newline-bearing API key; exit4for 401/500/404 withstatusin the envelope; exit5for ECONNREFUSED, ENOTFOUND, and a mid-body stall on a 2xx (tropkod request timed out after 30000ms); exit6for a non-JSON 2xx and for schema drift, withjob_idrecovered from{"job":{"id":…}}.--json, stdout carried exactly one document in every failing case, including commander-originated errors where commander's own prose went only to stderr — matching the README's caveat.3invariant ("no HTTP request was dispatched") holds at every{ kind: "usage" }construction site: URL/key parsing, argument resolution, stdin collection, and the undiciUND_ERR_INVALID_ARGpre-dispatch branch.--url http://h/proxy --job "job 42"producedGET /proxy/v1/jobs/job%2042, and--url http://h/deep/baseproducedPOST /deep/base/v1/queries. The./..//job-id rejection is load-bearing, sinceencodeURIComponentleaves dots unescaped andnew URLwould collapse the path.--url/--api-keybefore stdin does prevent the hang: an open pipe with noTROPKOD_URLexited3immediately.3.tsc -p tsconfig.app.json --noEmitis clean and the suite passes (108 tests, 12 files).No credential leaks: every URL/key rejection uses a constant message and drops the
cause, and the port list matches the current WHATWG bad-port set exactly.Code review by Claude Code Opus (opus)
@ -0,0 +10,4 @@const parsed = Number(value);if (parsed > maxWaitMs) {throw new InvalidArgumentError(`expected a value between 0 and 300000, received "${value}"`);🟢 Low: The range message hard-codes
300000while the bound itself lives inmaxWaitMson line 4, so the two can drift silently — a future cap change would keep telling users "between 0 and 300000" while rejecting at a different value. Interpolate the constant instead:The wording stays byte-identical today, so the existing assertions in
parse-wait-ms-option.test.tskeep passing.Summary: Reviewed the exit-code/JSON-envelope rework end to end and exercised the built CLI against throwaway local servers. The taxonomy holds up: verified exit
0(help/version),2(pending submit and poll),3(missing URL, blocked port, bad--job, commander usage errors),4(401 body, and a 503 whose body stalls — still exit4after the abort, as documented),5(ECONNREFUSED, ENOTFOUND, and an abort mid-body of a stalled 2xx), and6(non-JSON 2xx, schema drift with and without a recoverablejob.id). The--jsonstdout contract, the argv--jsonscan on commander errors, base-path joining, unknown-key stripping, and the pre-stdin URL check (an open pipe still exits3immediately) all behave as the README states. I also swept all 65535 ports throughfetch: theblockedPortsset matches the runtime's bad-port set exactly (82/82, no diff in either direction). Test suite passes (108 tests) andtsc -p tsconfig.app.jsonis clean.Found 2 low-severity issues; nothing blocking.
Code review by Claude Code Opus (opus)
@ -0,0 +30,4 @@return { kind: "usage", message: error.message };}if (error instanceof Error) {return { kind: "unexpected", message: error.message };🟢 Low: An
unexpectedfailure keeps onlyerror.message, so the exit7path the README describes as "a bug in this CLI — report the bug" surfaces a bare one-liner with no stack and no cause chain. ATypeError: Cannot read properties of undefinedon stderr gives whoever hits it nothing to file. The stdout--jsonenvelope shape does not need to change; writing the original error (e.g.console.error(error), asbin/tropkod-clientalready does for a load failure) to stderr for this kind alone would keep the verdict contract intact while making the report actionable.@ -24,3 +26,2 @@if (options.job !== undefined) {if (questionParts.length > 0 || stdinQuestion.length > 0) {throw new Error("pass either a question or --job, not both");const jobId = options.job.trim();🟢 Low:
--jobnow gets trimmed and rejected when blank, but--sessionis still passed through untouched, so--session " "is serialized verbatim into the submit body. Confirmed against a local server:body={"question":"q","session_id":" ","wait_ms":0}. That spends a real request to earn an exit4(or, if the service is lenient, silently queues a paid analysis under a whitespace session id) where the same input on--jobis caught client-side as exit3. Trimmingoptions.sessionand rejecting an empty result with aCliError({ kind: "usage" })would make the two id-bearing flags consistent.Round-8 feedback processed at
3b0a8e3— all three fixed:--session " "serialized verbatim into the submit body (Low,resolve-submission-request.ts) — fixed as suggested:--sessionis now trimmed and a whitespace-only value throws--session requires a session id(usage, exit3) before any request, making the two id-bearing flags consistent. The check lives in the question branch only, so the documented "--sessionis ignored when--jobis given" behavior is preserved — a test pins all three cases (trimmed, rejected-blank, ignored-alongside-job).300000in the range message (Low,parse-wait-ms-option.ts) — fixed with the exact suggested interpolation; wording is byte-identical today and the existing test assertions pass unchanged.7surfaces a bare one-liner (Low,classify-failure.ts) — fixed per the suggestion: after emitting the normal envelope/stderr lines, the CLI nowconsole.errors the original error — stack and cause chain included — for theunexpectedkind alone, in both modes. The stdout--jsoncontract is untouched (still exactly one JSON document), and the README's JSON-output section documents the extra stderr output so it can't read as contract drift.Summary: No blocking defects found. Verified the new exit-code taxonomy and JSON envelope contract empirically against throwaway HTTP servers (401, 500 with a refusal-shaped body, non-JSON 2xx, schema-drift 2xx with a recoverable job id, a 2xx truncated mid-body, and a 503 whose error body never completes), plus commander paths (
--help,-V, unknown option, rejected--wait-ms,--json=true), the missing---url/--api-keyordering ahead of the stdin read, base-path preservation (--url http://h/base->POST /base/v1/queries), and--jobsegment validation. Every observed exit code and stdout shape matched the README's tables. The full suite passes (12 files, 111 tests).Two low-severity notes below, both about the "could a job have been created?" claims rather than the implementation.
Code review by Claude Code Opus (opus)
@ -80,0 +109,4 @@| `6` | Invalid response — 2xx received but the body was not a parseable submission envelope. | Yes — id recovered iff the envelope carries `.error.job_id` (the body held a readable `job.id`); lost otherwise | Job untouched. A transient blip (truncated body, proxy garbling) recovers — retry once or twice. A repeated exit `6` with an identical message is deterministic schema drift, terminal for this client build: the job has likely already ended in a shape this client cannot render — report it, don't keep looping || `7` | Unexpected client error (a bug in this CLI). | Unknown — assume yes | Job untouched — report the bug; polling the same id stays safe |After a failed **submit**, resubmitting is safe iff `code == 3 || (code == 4 && status < 500)` — and only when every earlier submit in the task is accounted for. A failed **poll** never licenses a resubmit: `GET /v1/jobs/<id>` can neither create nor destroy a job — repair the input and keep polling.🟢 Low: The resubmit predicate treats every 4xx as proof that no job was created, but the client cannot distinguish a 4xx minted by the tropkod service from one minted by an intermediary after it forwarded the POST. A reverse proxy or gateway that gives up on a slow upstream can answer
408(and some edge networks synthesize other 4xx codes) once the request has already reached the service and created a paid job. The exit-code table already carves out this exact reasoning for 5xx ("a proxy may mask an accepted request") but applies the opposite assumption to the whole 4xx range, socode == 4 && status < 500can license a duplicate paid analysis — the failure mode this PR exists to prevent.Suggest narrowing the predicate to the statuses that are provably pre-dispatch verdicts, e.g.
code == 3 || (code == 4 && status < 500 && status != 408), and mirroring the carve-out in the4row's "On a submit: job created?" cell.@ -0,0 +9,4 @@let depth = 0;while (current instanceof Error && depth < maxCauseDepth) {if ((current as { code?: unknown }).code === invalidArgumentCode) {ℹ️ Info: This maps any
UND_ERR_INVALID_ARGfound in the cause chain tousage/exit3, which the README documents as provably no job created. That holds for the rejections this guards against, andreadRemoteClientOptionsalready rejects the known triggers (bad port, credentials in the URL, header-invalid key) at parse time — so what actually reaches this branch is, by construction, an invalid-argument shape nobody enumerated. If one of those ever arises after bytes hit the wire (e.g. undici re-dispatching a redirect hop for a POST that the service already accepted), exit3tells the agent rule it is safe to resubmit a paid analysis. Downgrading the unrecognized residue totransport(exit5, "assume a job exists") would fail safe instead; keeping exit3is defensible, just worth being a deliberate choice.Summary: No correctness, security, or error-handling defects found. One low-severity documentation gap.
I verified the load-bearing runtime claims against Node 26.5 rather than taking them on trust:
fetchand diffed the results against the hardcoded set inread-remote-client-options.ts: 82 ports blocked by undici, 82 in the list, zero in either direction. The scan also confirmed the premise for the list — undici's bad-port rejection isTypeError: fetch failedwith causeError: bad portand nocodeproperty, soisRejectedBeforeDispatchgenuinely cannot see it and the parse-time check is what keeps exit3.fetchwith aDOMExceptionnamedTimeoutError; a timeout firing mid-body rejectsresponse.json()with the same shape (andDOMException extends Errorin Node, soisAbortErrormatches both). Theresponse.text().catch(() => "")on the non-2xx path therefore degrades to a status-only message and keeps exit4, as documented.validateHeaderValuerejects LF/CR/NUL/C0/DEL and accepts interior tab; sinceapiKeyistrim()ed (which also strips NBSP), a leading/trailing space or tab — the case undici rejects butnode:httpallows — is unreachable. The stricter-check comment holds, and no error path can echo the key.pathname += "/"plusnew URL(path.slice(1), base)resolves under a base path instead of replacing it, andURLnormalizes:0080/:022before the string-keyed port lookup.encodeURIComponentescapes%, the only inputs that can produce a WHATWG dot-segment are literal.and.., both rejected;%2e%2eencodes to%252e%252eand stays one segment.7.Exit-code contract review: every exit-
3path (option parsing,readRemoteClientOptions,resolveSubmissionRequest,collectStdinText, commander viaexitOverride,isRejectedBeforeDispatch) precedes dispatch, so "no HTTP request was sent" holds. Nothing reaches exit7from a server-caused condition — every network/parse failure is wrapped inCliError, andQuerySubmission/recoverJobId/readErrorMessageall usesafeParse, so a ZodError cannot escape asunexpected. The commander caveat in the README is accurate:Command#errorwrites theshowHelpAfterErrorhint to stderr separately and does not include it in theCommanderErrormessage.Code review by Claude Code Opus (opus)
@ -18,2 +19,3 @@tropkod-client --json "Does npm:fastify@5.6.0 expose schemaCompiler?"# Read the question from stdin# Poll while the exit code is 2; branch per the README's exit-code table otherwise🟢 Low: This epilog points at "the README's exit-code table," but the help output never carries that table, and this PR is the one that makes exit codes the primary contract — the README's own Agent Rule tells an agent to run
--helpbefore first use and then to "branch on exit codes3–7." An agent that discovers the tool through--helpalone gets a dangling reference.A compact list in the same
addHelpText("after", ...)block closes the loop without duplicating the whole table, e.g.:Round-9 disposition (fixes in
4fe7d42):code == 3 || (code == 4 && status < 500 && status != 408), the exit-4 submit cell says "4xx except 408: no. 408 and 5xx: unknown (an intermediary may mask an accepted request)", and a sentence explains the carve-out: an intermediary can mint a 408 after forwarding the POST.is-rejected-before-dispatch.tsand its tests are deleted. The reviewer's fail-safe argument is exactly the PR's own stated principle — a false "safe to resubmit" costs a duplicate paid analysis, a false "report and stop" costs one wasted stop — and the backstop traded the cardinal failure for the tolerable one at precisely the margin where neither is provable. Every known trigger ofUND_ERR_INVALID_ARG(C0/DEL in the key) is already caught at parse time byvalidateHeaderValue, so the branch was unreachable for known inputs and only ever fired on residue we cannot prove pre-dispatch (e.g. rejections after a redirect hop). The fetch catch now classifies all unrecognized refusals astransport(exit 5, "assume a job exists"), with a comment naming that as the deliberate default.--helpnow ends with a two-line exit-code table pointing at the README for the full version.All three conversations resolved.
Summary: Found 1 medium issue (documentation self-contradiction). The implementation itself held up under verification.
What I verified (sandbox copy with deps installed; the repo working tree was left untouched):
tsc -p tsconfig.app.json --noEmitpasses clean.vitest run: 11 files, 106 tests, all passing.--url→3; unknown option / bad--wait-ms/ over-cap--wait-ms→3with commander prose on stderr and the stdout envelope; pending submit →2withanalysisomitted;401/500→4with the(HTTP <status>)suffix preserved; non-JSON 2xx →6; schema drift carryingjob.id→6with.error.job_idrecovered; body truncated mid-stream →5; connection refused →5with the flattened cause chain;--version→0.--jobpath-segment rejection ("job 1/x"→3) and base-URL joining (http://h/api→http://h/api/v1/queries,.../v1/jobs/<encoded>) behave as intended.validateHeaderValuepre-dispatch guard does not over-reject: keys containing\tor U+00E9 pass locally and through realfetch, while DEL and astral code points are rejected pre-dispatch (exit3), so no provably-no-job case is misrouted to exit5.Code review by Claude Code Opus (opus)
@ -82,1 +117,4 @@## Orphaned jobsA submit that dies before printing its job id — a harness-killed process, a transport failure (exit `5`), an accepted response without a recoverable id (exit `6` with no `.error.job_id`; an exit `6` whose envelope carries `.error.job_id` orphans nothing), or a client crash (exit `7`, or a bare runtime exit `1` with empty stdout) — orphans the job, not the analysis: the server never cancels a job because the client went away, and runs the paid analysis to completion on its single processing slot. The client has exactly two routes, `POST /v1/queries` and `GET /v1/jobs/<id>`; there is no job-listing or lookup route, so an id that was never printed is unrecoverable. `--session` does not help: it is submit-only input, and reusing it queues a **new** paid analysis behind the invisible first one. After losing a submit without an id, report and stop — do not resubmit the question. Exit `3`, and exit `4` on a 4xx, are the codes that prove no job was created by the failed invocation.🟡 Medium: This closing sentence contradicts the exit-code table two sections above and the resubmit rule on line 112, specifically for
408.4, "On a submit"): "4xx except 408: no. 408 and 5xx: unknown (an intermediary may mask an accepted request)"code == 3 || (code == 4 && status < 500 && status != 408)"3, and exit4on a 4xx, are the codes that prove no job was created by the failed invocation."408is a 4xx, so this sentence asserts a job was provably not created in exactly the case the other two passages call unknowable. The failure mode is the one this section exists to prevent: an agent that reads the "Orphaned jobs" section (which ends with "do not resubmit the question", making this the operative rule at that moment) will treat a408on a submit as proof of no job and resubmit, queueing a second paid analysis behind an invisible first one.Suggested fix — align the wording with line 112:
Summary: Found 1 medium documentation issue. The implementation itself held up under empirical verification.
I checked out the head, ran the suite in a scratch copy (106/106 pass), typechecked cleanly, and exercised the CLI end-to-end against throwaway local servers on Node 26. Every exit code in the new README table reproduced as documented:
2with the envelope (analysis: nullomitted);--job401/500 →4withstatus; non-JSON 2xx body →6; schema drift carryingjob.id→6with.error.job_id;--job ..and blocked port22→3;ECONNREFUSED→5; stalled 2xx body →5after exactly 30 s; 500 with a truncated body →4(not reclassified as transport).readCauseChainMessageproducedfetch failed: connect ECONNREFUSED …as intended;response.json()really does reject withSyntaxErroron a non-JSON body and aTimeoutErrorDOMException(which isinstanceof Errorin Node) on a mid-body abort, so both branches inrequestQuerySubmissionare reachable and correctly discriminated.CommanderError.messageis"error: unknown option '--josn'\n(Did you mean --json?)"while stderr additionally carries(add --help for usage).--help/--versionthrow withexitCode === 0and are correctly passed through as exit0.validateHeaderValueis genuinely aligned with undici here — I tested\u0001,\u007f,\n,\u0000,\t, Latin-1 and a >0xFF code point, and node's check accepts exactly the set fetch accepts, so the exit-3 pre-dispatch claim holds.dist/cli.js, yielding exit7with empty stdout.--jobpath-segment validation plusencodeURIComponentleaves no traversal route, and the blocked-port set matches the current WHATWG bad-port list exactly.Code review by Claude Code Opus (opus)
@ -82,1 +117,4 @@## Orphaned jobsA submit that dies before printing its job id — a harness-killed process, a transport failure (exit `5`), an accepted response without a recoverable id (exit `6` with no `.error.job_id`; an exit `6` whose envelope carries `.error.job_id` orphans nothing), or a client crash (exit `7`, or a bare runtime exit `1` with empty stdout) — orphans the job, not the analysis: the server never cancels a job because the client went away, and runs the paid analysis to completion on its single processing slot. The client has exactly two routes, `POST /v1/queries` and `GET /v1/jobs/<id>`; there is no job-listing or lookup route, so an id that was never printed is unrecoverable. `--session` does not help: it is submit-only input, and reusing it queues a **new** paid analysis behind the invisible first one. After losing a submit without an id, report and stop — do not resubmit the question. Exit `3`, and exit `4` on a 4xx, are the codes that prove no job was created by the failed invocation.🟡 Medium: The closing sentence contradicts the exit-code table and the resubmit rule for HTTP 408.
Here: "Exit
3, and exit4on a 4xx, are the codes that prove no job was created by the failed invocation."But the table row for
4says "4xx except 408: no. 408 and 5xx: unknown (an intermediary may mask an accepted request)", and the rule just above says resubmitting is safe iffcode == 3 || (code == 4 && status < 500 && status != 408).408 is a 4xx, so this section tells an agent it proves no job was created — the one conclusion the rest of the document is careful to deny, and the exact case that would queue a second paid analysis. Since this paragraph is the part an agent reads after losing a submit, the looser wording is the one that will be acted on.
Suggested fix:
Exit3, and exit4on a 4xx other than408, are the codes that prove no job was created by the failed invocation.Round-10 disposition (fix in
c386d9b):3, and exit4on a 4xx other than408, are the codes that prove no job was created by the failed invocation." Both reviewers were right that this was the operative passage: it sits at the exact decision point an agent reaches after losing a submit, so the round-9 carve-out had landed in the table and the predicate but not in the sentence that actually governs the resubmit choice. The three passages (exit-4 table row, resubmit predicate, orphaned-jobs closing sentence) now state the same rule.Both conversations resolved.
Summary: No actionable issues found.
Code review by Codex GPT-5.6 SOL (gpt-5.6-sol)
Summary: No actionable issues found.
I reviewed the full diff and verified the documented behavior empirically rather than by reading alone: I reconstructed the CLI in a throwaway sandbox (Node 26, commander 15, zod 4) against local HTTP servers and exercised every exit code in the README table.
Verified against real runs:
--help/--versionexit0underexitOverride(); unknown option and a rejected--wait-msexit3and print exactly one stdout envelope under--jsonwhile commander's own prose stays on stderr (themessagefield matched commander's wording minus the(add --help for usage)hint, as documented).3: missing/blank URL, unparseable URL, non-http(s)scheme, query/fragment, credentials (message carries no password,causeis dropped), blocked port (:1,:22,:10080), and a newline/control-bearing API key. The hard-coded bad-port set matches the current WHATWG fetch list.401and a 5xx with a refusal-shaped body both exit4with the status suffixed, so a 5xx cannot masquerade as a 4xx.6;job_idappears iffrecoverJobIdfindsjob.id.5(tropkod request timed out after 30000ms), and a content-length truncation exits5, while an EOF-delimited truncation exits6— both consistent with the table's retry guidance.5with the cause-chain message; a pending envelope exits2; completed exits0; failed exits1./, no slash,/proxy,/proxy/) andencodeURIComponentplus the./..//guard keeps--jobto one path segment; the request body carries the trimmedsession_idandwait_ms.--joband a positional question skip the read entirely.Repo-hygiene checks in the sandbox: all 106 tests pass,
tsc -p tsconfig.app.jsonandtsconfig.test.jsonare clean, and knip reports no unused exports for the new modules. The--jsonargv scan's only false positive (a--jsonconsumed as another option's value on a commander-error path) is already noted in the code and costs at most one extra stdout envelope.Code review by Claude Code Opus (opus)