feat!: return success for pending and running jobs #13
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/nonfailed-job-status"
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?
Pending and running envelopes now exit
0, so generic wrappers andset -estop misclassifying valid work as failures;.job.statusdecides whether to poll. The client also rejects contradictory snapshots and prints a credential-safe poll command. Release only after tropkod#30 is deployed.Approach review: The exit-code and polling-output direction is sound, but contradictory snapshots should be modeled as lifecycle variants in the schema instead of a parallel imperative validator. This would make the parsed TypeScript type preserve the same guarantees enforced at runtime and reduce the maintenance cost of adding states or fields.
Approach review by Codex CLI · Personal 01 (gpt-5.6-sol)
@ -154,0 +155,4 @@job: JobRecord,analysis: Analysis.nullish().transform((value) => value ?? undefined),}).superRefine(validateQuerySubmission);Model the lifecycle as Zod/TypeScript variants here—for example, a discriminated
JobRecordplus envelope variants pairing pending/running/failed/completed jobs with their permittedanalysis—and reservesuperRefinefor cross-record ID equality checks. The broad object plusvalidateQuerySubmissionrejects impossible snapshots only at runtime, while its inferred type still permits unrelated optionalanalysis_id,error, andanalysisfields. That creates two lifecycle definitions to maintain and discards the knowledge gained during parsing; the existingz.discriminatedUnionpattern is the clearer fit.Fixed in
5654c88: lifecycle variants encode permitted fields; refinement now checks only cross-record links.Approach review: Returning
0for valid nonterminal jobs and printing a credential-free poll command are appropriate. The contradictory-snapshot handling should instead model lifecycle variants in the Zod schema so parsing yields a discriminated domain type and protocol evolution has one source of truth.Approach review by Codex CLI · Personal 01 (gpt-5.6-sol)
@ -154,0 +155,4 @@job: JobRecord,analysis: Analysis.nullish().transform((value) => value ?? undefined),}).superRefine(validateQuerySubmission);The lifecycle combinations are a natural schema-union concern rather than a
superRefineconcern. Define pending/running/completed/failed job variants (plus envelope variants for whetheranalysisis present), then retain a small refinement only for cross-record ID and question equality. The current approach duplicates the schema inSubmissionForValidation, walks every state imperatively, and still infersanalysis_idanderroras independent optionals, so parsing discards the exact state knowledge it just proved. A Zod union/discriminated union would remove most of this validator and make future protocol states materially safer to add.Fixed in
5654c88: lifecycle variants encode permitted fields; refinement now checks only cross-record links.Approach review: The exit-code and polling-command changes are appropriately small, but the response lifecycle has a materially simpler model. Encode the status-specific shapes in Zod and reserve refinement for cross-record identity checks; that removes duplicated lifecycle logic and makes the inferred type reflect the states the parser accepts.
Approach review by Codex CLI · Personal 01 (gpt-5.6-sol)
@ -154,0 +155,4 @@job: JobRecord,analysis: Analysis.nullish().transform((value) => value ?? undefined),}).superRefine(validateQuerySubmission);Model the lifecycle in the schema instead of accepting optional
analysis_id,error, andanalysisand then rejecting their invalid combinations insuperRefine. DefineJobRecordwith status-discriminated variants and compose pending/running, failed, and completed envelope variants, keeping a small refinement only for cross-record ID/question equality.superRefinecan reject bad payloads at runtime, but the inferredQuerySubmissionremains a bag of optionals, so consumers can still represent and must defensively handle states the parser promises are impossible; the separate validator also duplicates the schema asSubmissionForValidation.Fixed in
5654c88: lifecycle variants encode permitted fields; refinement now checks only cross-record links.Approach review: The exit-code and rendering changes are direct, but the response lifecycle should be represented as schema variants rather than a parallel procedural state machine; see the inline alternative.
Approach review by Codex CLI · Personal 01 (gpt-5.6-sol)
@ -154,0 +155,4 @@job: JobRecord,analysis: Analysis.nullish().transform((value) => value ?? undefined),}).superRefine(validateQuerySubmission);The lifecycle rules should be part of the parsed type instead of a parallel
voidvalidator. DefineJobRecordas az.discriminatedUnion("status", ...)withanalysis_idrequired only forcompleted,errorrequired only forfailed, and both forbidden forpending/running; then compose submission variants soanalysisis required only forcompleted. KeepsuperRefineonly for cross-record ID equality. This removes most ofvalidate-query-submission.tsand makesz.infer<typeof QuerySubmission>exclude the impossible states currently admitted by the optional fields.Fixed in
5654c88: lifecycle variants encode permitted fields; refinement now checks only cross-record links.Summary: Found 1 medium issue in the new poll-command output.
Code review by Codex CLI · Personal 01 (gpt-5.6-sol)
@ -31,0 +41,4 @@},{stream: "stdout",text: `tropkod-client --url ${shellQuote(options.serviceUrl.href)} --job ${shellQuote(submission.job.id)}`,🟡 Medium: This command always uses POSIX single-quote escaping, but the package has no POSIX-only platform constraint. For example, the newly accepted
job's; still safeID is rendered as--job 'job'\''s; still safe', which PowerShell cannot parse because an apostrophe inside its single-quoted strings must be doubled. A Windows user therefore cannot copy the advertised poll command for a valid response. Emit shell-specific commands/escaping, or print the argument values separately when the target shell is unknown.Fixed in
5654c88: output is labeled JSON argv data, not shell syntax, and is covered on hostile identifiers.Summary: Reviewed the exit-code change (2 → 0 for pending/running), the new cross-field response validation, and the poll-command hint. Found 2 medium and 2 low issues.
What I verified locally (Node 26.5, zod 4.4.3, a stub tropkod server): all 22 cases in
src/query-submission.test.tspass against real zod; the fivesrc/cli.integration.test.tsscenarios reproduce exactly (exit0for running/completed, exit1for failed, theset -euo pipefailpipeline exits0with empty stderr, and the printed poll command matches the expected shell-quoted text without leaking the API key); the new README bash example runs and printspoll the returned job id later;shellQuotecorrectly rendersjob's; still safeas'job'\''s; still safe'; and no staleexit 2references remain in the repo.The two medium findings are both cases where the new
superRefineconverts a usable server response into exit6(documented as terminal schema drift) — I confirmed the failed-job-without-errorcase end to end against a stub server.Code review by Claude Code · Personal 01 (opus)
@ -0,0 +6,4 @@.refine((value) => value === value.trim(), {message: "job.id must not have outer whitespace",}).refine((value) => value !== "." && value !== ".." && !value.includes("/"), {🟢 Low: These two rules duplicate the
--jobinput validation insrc/resolve-submission-request.ts(jobId === "." || jobId === ".." || jobId.includes("/"), plus the emptiness check), which already carries the comment explaining why:encodeURIComponentleaves dots unescaped. Two copies of a path-segment guard can drift, and only one of them is covered by the new tests.Reusing
JobIdinresolveSubmissionRequest(parse the trimmed value and map a failure to the existingusageCliError) keeps the request-side and response-side definitions of "a single path segment" identical.Fixed in
5654c88: response recovery and --job parsing now reuse the shared hardened JobId schema.@ -154,0 +155,4 @@job: JobRecord,analysis: Analysis.nullish().transform((value) => value ?? undefined),}).superRefine(validateQuerySubmission);🟢 Low: The refinement makes the present/absent distinction on
job.errorandjob.analysis_idload-bearing, but both are declared.optional(), so an explicitnull— the normal JSON serialization of an absent nullable column — fails the object parse before the refinement runs.Confirmed with zod 4.4.3: a
pendingjob carrying"analysis_id": null, "error": nullis rejected withexpected string, received nullon both fields (exit6), even though that payload means exactly whatforbidAnalysisArtifactswants to allow.AGENTS.md's own rule prescribes
.nullish()for backend fields that may be absent..nullish().transform((value) => value ?? undefined)on both — matching howanalysisis already declared — would collapsenulland missing into theundefinedthe refinement checks for.Fixed in
5654c88: the transport boundary normalizes nullable lifecycle fields to absence before canonical parsing.@ -0,0 +46,4 @@if (submission.job.status === "failed") {forbidAnalysisArtifacts(submission, context, false);if (submission.job.error === undefined) {addIssue(context, "failed jobs require job.error", ["job", "error"]);🟡 Medium: Requiring
job.erroron a failed job turns the server's own terminal verdict into exit6.Verified against a stub server: a
failedjob whose body omitserrornow yieldsBefore this change the same body exited
1and printedJob job-9 is failed.The README defines exit1as "the server's own verdict" and tells poll loops to end at "a completed or failed job envelope"; a repeated exit6is documented as terminal schema drift to report rather than poll. So a worker that dies without recording a message (OOM kill, hard timeout) makes the CLI report a client/server contract bug instead of the failure that actually happened.The same body shape with
"error": nullfails even earlier, atJobRecord.error's.optional()— also exit6.Safer: keep
failedrenderable and drop this issue, lettingformatSubmissionfall back (it already guardsjob.error !== undefined) — the status alone is the verdict, and the message is only decoration.Declining this change: the producer database contract requires every failed job to carry a nonempty error, and the service schema now enforces the same invariant.
@ -0,0 +73,4 @@addIssue(context, "analysis.session_id must match session.id", ["analysis", "session_id"]);}if (submission.analysis.question !== submission.query.question) {addIssue(context, "analysis.question must match query.question", ["analysis", "question"]);🟡 Medium: Byte-exact
analysis.questionvsquery.questionis the most fragile invariant here, and failing it discards a completed, already-paid analysis.Confirmed: a completed envelope differing only by one trailing space between the two fields is rejected with
analysis.question: analysis.question must match query.question, so the whole envelope becomes exit6and the answer is unrenderable in both human and--jsonmode. Any future server-side normalization (trim, whitespace collapse, unicode normalization) of one field and not the other silently breaks every completed job for this client build.The linkage this check is after is already fully established by the surrounding id checks (
analysis.query_id === query.id,analysis.session_id === session.id,analysis.provenance.job_id === job.id), which cannot drift on formatting. Consider dropping the question comparison, or comparing trimmed values.Declining this change: the producer derives both question fields from the same persisted query value, so exact equality is an intentional contract-drift check.
Summary: Found 1 critical issue: the generated poll command can become executable PowerShell input when a server-controlled job id contains shell metacharacters.
Code review by Codex CLI · Personal 01 (gpt-5.6-sol)
@ -55,2 +69,4 @@}function shellQuote(value: string): string {return `'${value.replaceAll("'", String.raw`'\''`)}'`;🔴 Critical: This emits POSIX-only quoting for a command presented as ready to paste, but the npm CLI has no Windows restriction and the new
JobIdparser explicitly accepts apostrophes and semicolons. In PowerShell, a value such asx'; Write-Output pwned; #makes the generated--job 'x'\''; ...leave the semicolon outside a quoted string (PowerShell escapes an apostrophe as'', not\''), so pasting output from an untrusted service can execute attacker-controlled commands. Avoid emitting an executable command from opaque server data, or constrain every interpolated value to a shell-portable safe alphabet and cover the supported Windows shells as well as Bash.Fixed in
5654c88: opaque values are emitted as labeled JSON argv data rather than executable shell syntax.Summary: Found 1 medium issue.
Code review by Codex CLI · Personal 01 (gpt-5.6-sol)
@ -55,2 +69,4 @@}function shellQuote(value: string): string {return `'${value.replaceAll("'", String.raw`'\''`)}'`;🟡 Medium: This escaping only produces a POSIX-shell command, although the package has no OS restriction. In
cmd.exe, single quotes are passed literally, so even an ordinary generated--url '…'value is invalid; in PowerShell, an embedded apostrophe must be doubled rather than escaped as'\'', so the hostile-ID case is not copyable there either. Render quoting for the current platform (or provide explicit POSIX and PowerShell/cmd variants) so the advertised poll command works for Windows users.Fixed in
5654c88: the platform-neutral output is JSON argv data rather than shell-specific quoting.Implemented lifecycle variants and cross-record-only refinement in
5654c88.Implemented lifecycle variants and platform-neutral JSON poll data in
5654c88.Implemented lifecycle variants and cross-record-only refinement in
5654c88.Implemented lifecycle variants and cross-record-only refinement in
5654c88.Fixed the poll-output finding in
5654c88by emitting labeled JSON argv data.Processed all four inline findings: null normalization and shared JobId parsing were fixed in
5654c88; the two producer-backed invariants were retained with evidence in their threads.Fixed the shell-injection finding in
5654c88by removing executable shell syntax from the output.Fixed the portability finding in
5654c88with platform-neutral JSON argv data.Summary: No actionable issues found.
Code review by Codex CLI · Personal 02 (gpt-5.6-luna)
Summary: Found 1 medium issue.
Code review by Codex CLI · Personal 02 (gpt-5.6-luna)
@ -0,0 +10,4 @@.refine((value) => !value.includes("\0"), {message: "job.id must not contain NUL",}).refine((value) => value.isWellFormed(), {🟡 Medium:
isWellFormed()rejects lone surrogates but still accepts U+2028/U+2029.JSON.stringify()emits those Unicode line separators literally, so a remote job ID can break the promised one-line human poll/diagnostic output and confuse line-oriented consumers. Reject or escape line-separator/control characters before usingJobIdin rendered output.Fixed in
ae77d6c: all JSON-literal human values escape Unicode line separators while preserving round-trip data.Summary: No actionable issues found.
Code review by Codex CLI · Personal 02 (gpt-5.6-luna)
Summary: Found 1 medium issue.
Code review by Codex CLI · Personal 01 (gpt-5.6-sol)
@ -0,0 +22,4 @@context: z.RefinementCtx,): void {validateQueryLinks(submission, context);validateAnalysisLinks(submission, context);🟡 Medium: This validator checks record IDs but leaves the nested target indexes unchecked. I verified that an answered envelope with one resolved/source target and both
source_alignments[0].target_indexandgrounding.spans[0].target_indexset to99still parses and is rendered with exit0. That accepts a contradictory snapshot and can present evidence attributed to no real target. Validate each index against the corresponding target array before accepting the envelope.Fixed in
ae77d6c: source alignments and evidence spans must reference an existing source target.Acknowledged; no actionable finding was reported.
Fixed the Unicode line-separator finding in
ae77d6c.Acknowledged; no actionable finding was reported.
Fixed the source-target linkage finding in
ae77d6c.Summary: No actionable issues found.
Code review by Codex CLI · Personal 01 (gpt-5.6-sol)
Summary: No actionable issues found.
Code review by Codex CLI · Personal 01 (gpt-5.6-sol)
Summary: No actionable issues found.
Code review by Codex CLI · Personal 01 (gpt-5.6-sol)
Summary: No actionable issues found.
Code review by Codex CLI · Personal 01 (gpt-5.6-sol)
Acknowledged; no actionable finding was reported.
Acknowledged; no actionable finding was reported.
Acknowledged; no actionable finding was reported.
Acknowledged; no actionable finding was reported.