Skip to content

fix: harden login env-validation, Inf/NaN schema inference, and log-scanner buffer (bug-hunt MED) - #208

Merged
LukasWodka merged 4 commits into
developfrom
fix/bughunt-med-hardening
Jul 10, 2026
Merged

fix: harden login env-validation, Inf/NaN schema inference, and log-scanner buffer (bug-hunt MED)#208
LukasWodka merged 4 commits into
developfrom
fix/bughunt-med-hardening

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

MED-severity hardening fixes from the 2026-07-09 adversarial bug-hunt of the CLI ingest/submit/auth paths. Independent fixes grouped as one PR (per the cli#199 batch-finding precedent).

Rebased onto develop (2026-07-10).#185/#210 landed the di#349 tabular-inference mirror, whose float grammar already rejects Inf/NaN — so fix#2's production change is now redundant and was dropped on rebase, kept only as a regression test (§2). Fix#3 was reworked per @saadqbal's review (§3) — a bigger buffer only moved the threshold; the real fix is to keep draining.

1. fix(auth) — reject unknown --env/$CLIENT_ENV at login

BaseURL falls unknown/typo env values back to prod (a lenient library default), so login --env staging or CLIENT_ENV=prd silently targeted productionand persisted it as the active session env for every later command. This is the class behind the earlier "did I hit dev or prod?" confusion.

login is where a human picks the session env, so a typo must fail there. Added api.IsKnownEnv (dev/stg/prod, case-insensitive) and validate the resolved env at runLogin entry, before any network call. ResolveEnv still maps empty→prod, so the no-flag default is unaffected; BaseURL's unknown→prod fallback is deliberately unchanged (TestBaseURL asserts it).

2. test(push) — pin Inf/NaN → not FLOAT (regression only, after #210)

Go's strconv.ParseFloat accepts "Inf"/"Infinity"/"NaN", so a naive inference would type such a column FLOAT — but the ingestor's FLOAT cast (DataValidator's non-finite guard) rejects it, so the cluster would refuse the auto-inferred schema only after the upload.

#185/#210's di#349 floatRE grammar pre-screens the token before ParseFloat, so inf/NaN already fall through to VARCHAR. The original production fix is therefore redundant and was dropped on rebase — kept as TestInferSchema_NonFiniteIsNotFloat, since no parity-fixture case covers non-finite.

3. fix(submit) — drain past ErrTooLong so a giant tqdm line can't force a false exit 9

tqdm (a data-ingestors dep) redraws its progress bar with \r and no \n, so a whole ingestion phase's redraws are one newline-delimited "line" that grows for the life of the run. Once it outgrows the display scanner's buffer, the scanner returns bufio.ErrTooLong.

The tee that feeds the summary parser is pulled only by that display scanner (thanks @saadqbal for catching this — the original "the parser is fed separately, so this cap never affects the verdict" premise was wrong). So an ErrTooLong bail stops the parser from ever seeing the rest of the stream, including the closing banner. A still-running Job then couldn't be confirmed terminal in the 30s finalJobStatus poll → watch returned a false exit 9 on a healthy large ingestion. Simply raising the cap only moves that threshold.

Fix: extracted the display/parse loop into streamDisplayAndParse; on bufio.ErrTooLong it drains the rest of the stream through the tee (io.Copyio.Discard) so the parser still resolves the summary, and treats the over-long display line as non-fatal — the Job status poll is the verdict's source of truth. Genuine read failures (network drop / ctx cancel) still propagate. 16 MB is kept as generous display headroom, but the drain (not the cap) is the correctness guarantee.

Test plan

  • go build ./..., go vet ./..., go test ./... — all green (rebased on develop).
  • auth: TestIsKnownEnv + TestResolveEnv (api) — known/unknown env classification; ResolveEnv empty→prod default unchanged.
  • push: TestInferSchema_NonFiniteIsNotFloat — a non-finite column infers VARCHAR, not FLOAT.
  • submit: TestStreamDisplayAndParse_DrainsPastOversizedLineSoParserSeesBanner — an oversized \r-line + the real ingestor banner → the parser still resolves the summary (no false exit 9); ..._GenuineReadErrorPropagates — a real mid-stream read error stays fatal.

🤖 Generated with Claude Code

@saadqbal
saadqbal self-requested a review July 10, 2026 10:11
Comment threadinternal/submit/watch.go Outdated
@saadqbal

Copy link
Copy Markdown
Collaborator

Solid PR — auth env-validation and the Inf/NaN demotion both look right (checked the other ResolveEnv callers; they fail safe). One non-blocking note inline on the scanner-buffer bump.

LukasWodka added a commit that referenced this pull request Jul 10, 2026
… false exit 9 (review)
Addresses @saadqbal's review on #208: the 16 MB buffer bump only MOVED the
false-exit-9 threshold, it didn't close it. The tee is pulled only by the
DISPLAY scanner, so when a line trips ErrTooLong the scan loop exits, the tee
stops being read, and the parser never sees the rest of the stream (the closing
banner) → streamFailed && outcome==Unknown → a false exit 9 on a healthy run.
A long enough single '\r'-line (> the buffer) still breaks it.
Class-level fix (his suggestion): keep draining past ErrTooLong. Extracted the
display/parse loop into streamDisplayAndParse; on ErrTooLong it drains the rest
of the stream THROUGH the tee (io.Copy to io.Discard) so the parser still sees
the banner, and it is NOT fatal — the Job status poll is the verdict's source of
truth. Genuine read failures (network drop, ctx cancel) still propagate.
Corrected the now-wrong "cap never affects the verdict" comment; kept 16 MB as a
generous display headroom (the drain is the correctness guarantee).
New tests (the #3 no-test gap Asad noted): an oversized '\r'-line + the real
ingestor banner → the parser still resolves the summary (no false exit 9); and
a genuine read error still propagates.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodkaand others added 4 commits July 10, 2026 12:50
…tly using prod
BaseURL falls unknown/typo env values back to prod (a lenient library
default), so `login --env staging` or `CLIENT_ENV=prd` silently targeted
production AND persisted it as the active session env for every later
command — the class behind earlier dev-vs-prod confusion.
login PICKS and persists the session env, so a typo must fail there. Add
api.IsKnownEnv (dev/stg/prod, case-insensitive) and validate the resolved
env at runLogin entry, before any network call. ResolveEnv still maps
empty->prod, so the no-flag default is unaffected. BaseURL's unknown->prod
fallback is deliberately unchanged (TestBaseURL asserts it).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… force a false exit 9
tqdm (a data-ingestors dep) redraws its progress bar with \r and no \n,
so a whole ingestion phase's redraws are one newline-delimited "line" that
grows for the life of the run. Past 1 MB the display scanner returned
bufio.ErrTooLong, cutting the log stream mid-run; a still-running Job then
couldn't be confirmed terminal in the 30s finalJobStatus poll, so watch
returned a false exit 9 on a healthy large ingestion — exactly the case the
1h JobWatchTimeout targets.
The parser is fed via the TeeReader, not the scanner, so this cap only ever
bounded the DISPLAY line and never the verdict. Raise it to 16 MB (clears a
fast ~10/s hour of redraws with headroom; the 1h cap bounds accumulation).
The buffer grows on demand, so ordinary log lines still cost 64 KB.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… false exit 9 (review)
Addresses @saadqbal's review on #208: the 16 MB buffer bump only MOVED the
false-exit-9 threshold, it didn't close it. The tee is pulled only by the
DISPLAY scanner, so when a line trips ErrTooLong the scan loop exits, the tee
stops being read, and the parser never sees the rest of the stream (the closing
banner) → streamFailed && outcome==Unknown → a false exit 9 on a healthy run.
A long enough single '\r'-line (> the buffer) still breaks it.
Class-level fix (his suggestion): keep draining past ErrTooLong. Extracted the
display/parse loop into streamDisplayAndParse; on ErrTooLong it drains the rest
of the stream THROUGH the tee (io.Copy to io.Discard) so the parser still sees
the banner, and it is NOT fatal — the Job status poll is the verdict's source of
truth. Genuine read failures (network drop, ctx cancel) still propagate.
Corrected the now-wrong "cap never affects the verdict" comment; kept 16 MB as a
generous display headroom (the drain is the correctness guarantee).
New tests (the #3 no-test gap Asad noted): an oversized '\r'-line + the real
ingestor banner → the parser still resolves the summary (no false exit 9); and
a genuine read error still propagates.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
#208's fix#2 (demote Inf/NaN columns to VARCHAR) is superseded by #185/#210:
the di#349 floatRE grammar pre-screens the token before ParseFloat, so
"inf"/"Infinity"/"NaN" already fall through to VARCHAR. Dropped the redundant
production change on rebase; kept the intent as a regression test, since no
parity-fixture case covers non-finite.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka
LukasWodkaforce-pushed the fix/bughunt-med-hardening branch from 8f8662d to 38d92dbCompareJuly 10, 2026 10:52
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Pushed + rebased onto develop. Two changes since your review:

  1. Fix Promote kubeconfig flags (--kubeconfig/--context/--namespace) to persistent on root #3 (the one you flagged) — you were right that 16 MB just moves the threshold. The tee is pulled only by the display scanner, so an ErrTooLong bail truncates the parser's view and the banner is lost → the same false exit 9. Now streamDisplayAndParse drains the rest of the stream through the tee on ErrTooLong (non-fatal; Job status is the source of truth), with tests for both the drain and the genuine-read-error path. Details in the inline thread.
  2. Fix Phase 2: kubeconfig discovery + parent release detection + SA token #2 (Inf/NaN)data ingest: collect + confirm the data schema for all tabular tasks (RFC-0002) #185/feat(data ingest): confirm the inferred tabular schema (#185) #210 landed the di#349 inference mirror, whose floatRE grammar already rejects Inf/NaN, so the production change is now redundant. Dropped it on rebase and kept it only as a regression test (no parity-fixture case covered non-finite).

Net PR is now: auth env-validation + the submit drain fix + the Inf/NaN regression test. All green — ready for another look.

@LukasWodka
LukasWodka merged commit f1afe2f into developJul 10, 2026
17 checks passed
@LukasWodka
LukasWodka deleted the fix/bughunt-med-hardening branch July 10, 2026 11:08
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

@LukasWodka@saadqbal