Skip to content

fix(cli,submit): reject misapplied task flags; don't false-fail slow-but-ok jobs - #224

Merged
saadqbal merged 2 commits into
developfrom
fix/cli-flag-guards-watch-outcome
Jul 10, 2026
Merged

fix(cli,submit): reject misapplied task flags; don't false-fail slow-but-ok jobs#224
saadqbal merged 2 commits into
developfrom
fix/cli-flag-guards-watch-outcome

Conversation

@saadqbal

@saadqbalsaadqbal commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Five findings from a full adversarial review of the v0.8.0 payload (proactive — Bugbot hasn't flagged these, but they're the same silent-drop / false-outcome class).

cli/data.go — task-scoped flags silently dropped (Med)

--schema, --label-policy, --time-column, --number-of-keypoints, and --label-column (on self-supervised text) were read only inside their one category branch, so passing one on a task that doesn't consume it silently discarded the value and the user's intent — even though the help text says each is scoped. Now rejected explicitly (exit 2), mirroring the existing --target-size/--min-size guard and spec.go's build gates.

cli/interactive.go — picker name→id collision (Low, latent)

pickTask mapped the chosen display name back to a task ID via a DisplayName→ID map; two tasks sharing a display name would collide and return the wrong ID. Maps by the offered option's position now.

submit/watch.go — false exit 9 on a slow-but-successful job (Med)

finalJobStatus timing out waiting for the Job's terminal condition (slow/contended apiserver) returned Unknown → a false exit 9 even when the ingest succeeded. Now falls back to the Pod phase (Succeeded/Failed) before giving up. The 30s bound is a package var so the fallback is unit-testable.

submit/watch.go — same-second pod tie-break (Low)

Most-recent-Pod selection tied on CreationTimestamp's 1s granularity; a same-second backoffLimit retry could leave the watch on the old Failed Pod. Tie now breaks toward the live/succeeded Pod.

Tests

Wrong-task rejection (5 cases) + a correct-task negative control; the pod-phase fallback; the same-second tie-break. Full suite green.

Scope

Review findings on #219; rolls up under release ticket #220; joins the v0.8.0 payload. (Deferred: D3, the unbounded parser buffer on a newline-less tqdm stream — noted for a follow-up.)

🤖 Generated with Claude Code


Note

Medium Risk
Changes affect CLI validation UX and ingest exit-code semantics (fewer false exit 9s); behavior is well covered by new tests but touches the critical watch→orchestrator path.

Overview
data ingest now fails fast (exit 2) when task-specific flags are used on the wrong --task, matching the existing --target-size / --min-size behavior: --schema, --label-policy, --time-column, --number-of-keypoints, and --label-column on self-supervised text are rejected instead of silently ignored.

The interactive task picker maps the user’s choice by option index rather than a display-name→ID map, avoiding a wrong task ID if two tasks ever share the same display name.

Ingest job watching adds shared mostRecentUsefulPod selection (newest useful pod; on a 1s timestamp tie, Running/Succeeded beats Failed). When finalJobStatus times out with Unknown, the CLI re-lists pods and uses Succeeded/Failed phase so slow apiserver lag does not produce a false exit 9; finalJobStatusTimeout is a test-overridable package var.

Tests cover wrong-task flags, pod-phase fallback, same-second pod ties, and Unknown fallback preferring a newer Succeeded pod over an older Failed one.

Reviewed by Cursor Bugbot for commit ffef672. Bugbot is set up for automated code reviews on this repo. Configure here.

…but-ok jobs
Five findings from a full adversarial review of the v0.8.0 payload:
cli/data.go — task-scoped flags (--schema, --label-policy, --time-column,
--number-of-keypoints, and --label-column on self-supervised text) were read
only inside their one category branch, so passing one on a task that doesn't
use it silently dropped the value and the user's intent — despite the help
text saying each is scoped. Reject them explicitly (exit 2), mirroring the
existing --target-size/--min-size guard and spec.go's build gates.
cli/interactive.go — pickTask mapped the chosen display name back to a task ID
through a DisplayName→ID map; two tasks sharing a display name would collide
and return the wrong ID. Map by the offered option's position instead.
submit/watch.go —
* finalJobStatus timing out on a slow apiserver returned Unknown → a false
exit 9 even when the ingest actually succeeded. Fall back to the Pod phase
(Succeeded/Failed) before giving up. The 30s bound is now a package var so
the fallback is unit-testable.
* the most-recent-Pod selection tied on CreationTimestamp's 1s granularity;
a same-second backoffLimit retry could leave the watch on the old Failed
Pod. Break the tie toward the live/succeeded Pod.
Tests added for every case (wrong-task rejection + a correct-task negative
control; the pod-phase fallback; the same-second tie-break).
Addresses review findings on #219 (C1/C2/D1/D2 + the --label-column echo).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9e7b285. Configure here.

Comment threadinternal/submit/watch.go
Bugbot follow-up on #224: the pod-phase fallback read the podName captured at
the start of the watch. waitForJobPod can return an early Failed Pod while a
backoffLimit retry is still Pending, so classifying off that stale name could
report Failed for a run that ultimately succeeds. Re-list Pods and re-select
the current most-recent useful one (extracted as mostRecentUsefulPod, shared
with waitForJobPod) so the fallback reflects the retry's real outcome.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal

Copy link
Copy Markdown
CollaboratorAuthor

Right — fixed in ffef672. The Unknown-status fallback now re-lists pods and re-selects the current most-recent useful one (shared mostRecentUsefulPod) instead of the podName captured at watch start, so a succeeded retry isn't misreported as Failed.

@LukasWodka

Copy link
Copy Markdown
Contributor

Reviewed this (adversarial pass, code-quoted). The data.go flag-guard fix reads clean and complete — nice catch on rejecting --label-column on self-supervised text, that one's subtle. Two things on the other changes.

[Medium] The watch Unknown-fallback trusts a stale podName → can re-introduce the false-fail this PR targetsinternal/submit/watch.go:320-331

podName is latched once by waitForJobPod (line 201) and never re-resolved. For a backoffLimit>0 Job: attempt #1 (the latched pod) fails, a retry succeeds, but the Job's terminal Complete condition lags past finalJobStatusTimeoutfinalJobStatus returns Unknown → the fallback Gets the stale attempt-#1 pod → PodFailedoutcome = JobOutcomeFailed. So a job that succeeded on a retry is reported failed — the exact slow-but-ok false-fail this PR sets out to fix.

(Exit code is 9 either way — Unknown also maps to 9 at data.go:1173 — so this is a misleading message regression, definitive "ingestion Job exited non-zero" vs the honest "final status couldn't be determined … check kubectl", not an exit-code flip.)

The comment "the Pod itself is authoritative" holds for a single-attempt job but not a multi-attempt one. Two options:

  • re-list the Job's pods and pick the terminal/most-recent — what waitForJobPod itself was fixed to do (TestWaitForJobPod_PicksMostRecentNotFirst); or
  • scope the fallback to PodSucceeded only (leave PodFailedUnknown). This is the minimal safe fix: it still rescues a slow-but-ok job (its pod Succeeded → Succeeded) without asserting failure from one stale attempt.

[Low, latent] The picker "collision fix" doesn't actually disambiguate, and it's the one change here with no testinternal/cli/interactive.go:278

prompter.Select returns the chosen value string, not an index (interactive.go:34), so:

fori, o:=rangeopts { ifo==ans { returnavailable[i].ID } }

returns the first option equal to ans. With opts[i] = s.DisplayName(), if two tasks in a family ever shared a display name, this still returns the first — it swaps the old map's "keep last" for "keep first" rather than resolving the collision, so the comment ("returns the one the user actually saw") over-claims. Latent today (all registry display names are distinct), so not urgent — but a genuine fix needs an index-returning select (or de-duplicated option strings), and at minimum the comment could be softened to match what the code does. pickTask also has no test (the fake prompter ignores options), so a collision can't currently be exercised.

@saadqbalsaadqbal self-assigned this Jul 10, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@saadqbal@LukasWodka