Repository navigation
fix(ci): make classify-paths fail loudly when grep fails - #549
Merged
Merged
Conversation
Both decisions in classify-paths.sh were `if printf … | grep -qE …; then A; else B; fi`. grep exits 0 on match, 1 on no match, and 2 on error — can't fork/exec, read error, bad pattern — and the else branch collapsed 1 and 2 into the same answer. `set -euo pipefail` does not help: `set -e` is suppressed for a command used as an `if` condition. Observed twice while gating this stack, in runs whose static checks run at -j 14. A different single case failed each time — `mixed-docs-go` answering docs=false, then `dep-bump-go` answering code=false — while every other case passed. That is the signature of a transient grep failure under load, not a pattern bug; the script and its test are unchanged from main and both pass standalone. The test caught it because it asserts expected values. The production path has no such check: CI's `changes` job gates the docs pipeline on this answer, so a docs=false produced by an errored grep skips the docs build and still reports success. Both greps now go through a `matches` helper that aborts with a diagnostic on any exit above 1. The test suite stubs grep onto PATH to prove the abort fires — that case fails against the previous script, which answers confidently instead. Closes #545. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (7)
🧰 Additional context used📓 Path-based instructions (1)Never hard-wrap prose.📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (5)
📝 SummarySummary by CodeRabbit
WalkthroughThe path classifier now distinguishes grep errors from no matches and avoids SIGPIPE failures for large inputs. The CI wrapper captures classifier failures and incomplete output, then runs both code and documentation checks. Tests cover grep errors and large input. The Biome schema URL is updated. ChangesClassification reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized CI change makes path-classification failures fail safely instead of silently skipping code or documentation checks. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
6 tasks done
EricAndrechek
marked this pull request as ready for review
September 2, 2026 14:49
EricAndrechek
enabled auto-merge
September 2, 2026 14:50
3 tasks done
taitelee
approved these changes
Sep 2, 2026
github-project-automation
Bot
moved this from Backlog
to In progress
in WaveHouse Task Board
EricAndrechek
disabled auto-merge
September 2, 2026 15:38
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
This was referenced Sep 2, 2026
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 4, 2026
## Summary `POST /v1/ingest` used to sniff the body and treat the header as a hint: the first non-whitespace byte chose between a single object and an array, and an NDJSON body whose first line happened to start with `[` was re-framed as a JSON array — silently, as a whole-request reinterpretation rather than a per-record error. The header is now required and authoritative. A request declaring nothing, or a media type not in the accepted list, is `415` before the body is parsed, with the supported types named in the body. The declared type chooses the format *family* — `application/json` versus the four NDJSON spellings — and within the JSON family the first non-whitespace byte still picks array versus single object. The bytes never choose the family, so an NDJSON body is read as NDJSON whatever its first byte and a bad line fails as a per-record error. The header is parsed by `mime.ParseMediaType`, so the grammar is RFC 9110's §8.3 `media-type` rather than one of ours. Only the media type selects the format; parameters are ignored, so no malformed parameter costs the request — `; charset`, `;;`, a value left mid-quote, even a name repeated with different values all read as `application/json`. One exception, below: a malformed parameter on a line that also contains a comma. **Duplicate declarations are the part most worth reviewing.** `Content-Type` is a *singleton* field (§8.3), and §5.3 forbids repeating a field line unless the field allows comma-list recombination — `media-type` does not. So both duplicate spellings are malformed input, and §8.3 says so directly, warning that recipients who resolve the resulting pseudo-list "by using the last syntactically valid member" cause "interoperability and security issues". Ingest therefore takes no member: - **Repeated header lines** (what curl and many proxies send) are all resolved and must agree on the format *and* on whether ingest reads it at all. `application/x-ndjson` alongside `application/ndjson; charset=utf-8` reads as NDJSON, because once they agree, which one gets honored stops mattering. Disagreement is `415` rather than resolution to the first — honoring the first would let an NDJSON body be read as one JSON object, ingesting record one and discarding the rest behind a `200`. - **A comma-joined value** (what a proxy merging duplicates sends) is refused. Where it fails outright there is no media type to take. Where it does parse a media type — `application/json; charset=utf-8, application/x-ndjson` yields `application/json` — it is still refused, because the comma may be a second declaration joined on and the error cannot distinguish that from a comma inside data. The security-critical detail is in `ingestFormatOne`. `ParseMediaType` returns `ErrInvalidMediaParameter` both for a merely-malformed parameter *and* when a second declaration was comma-joined on after a parameter — `application/json; charset=utf-8, application/x-ndjson` yields mediatype `application/json`. Tolerating that error unconditionally silently resolves a joined disagreement to its first member, which is exactly the truncation above. The two are indistinguishable from the error alone, so a comma on a line that did not parse cleanly is refused. That guard fails closed, and it is where I would look first. **Four additional shapes 0.1.0 accepted now `415`** (beyond repeated header lines that disagree, which it also accepted; see the CHANGELOG): a present-but-empty header; a value with a leading or trailing comma; a comma-joined value that does not parse as a single media type; and a malformed parameter on a line that also carries a comma. The last is an over-rejection and is tracked in #563. Note a comma is not disqualifying on its own: inside a quoted parameter value it is legal data, so `application/json; a=", application/x-ndjson; b="` is one media type and is accepted, even though an intermediary may have built it by illegally joining two lines. The server cannot tell. The TS SDK sends `application/json` for a single object and `application/x-ndjson` for arrays and `insertNDJSON`, on every path, so no SDK caller is affected. A hand-rolled client that relied on sniffing must now declare the type. ## Stacked PR This is **part 3 of 7** (parts 0, 1 and 2 merged) in a stack that replaces #540. Each PR is based on the one above it, so review this PR's own diff against its base — GitHub shows only this layer's changes. | # | Branch | Base | | | - | ------ | ---- | - | | 0 | `stack/0-classify-paths` | `main` | ✅ merged as #549 | | 1 | `stack/1-discovery` | `main` | ✅ merged as #550 | | 2 | `stack/2-policy` | `main` | ✅ merged as #551 | | 3 | `stack/3-content-type` | `main` | **→ this PR** | | 4 | `stack/4-seams` | `stack/3-content-type` | | | 5 | `stack/5-positional-wire` | `stack/4-seams` | | | 6 | `stack/6-computed-columns` | `stack/5-positional-wire` | | Merge in order, top to bottom. Rebasing or squashing out of order will make the later PRs' diffs unreadable. ## Response-size bound The 415 echoes what the caller declared, and it is decided **before the body is read** — so a header-only request, needing no credentials under the shipped compose policy, could size the response. Measured before the fix: 1.03 MB of headers produced a 4.65 MB body (and far worse over HTTP/2, where HPACK indexes a repeated header line to about a byte on the wire). Both dimensions are caller-controlled and both are bounded now: at most **four distinct** declarations, each capped at 128 bytes, then `"…and N more"`. The declaration that actually disagreed is pinned into the echo, so four agreeing spellings cannot crowd it out. Pinned by tests that fail if either bound is removed. ## Test plan - [x] `make ci` green locally on this branch's exact tree (verify, unit, integration against live ClickHouse, e2e, all coverage gates). Now that #551 has merged and this PR's base is `main`, GitHub CI runs on `pull_request` automatically — earlier runs on this branch were manual dispatches, so check the SHA before relying on an old one. - [x] The branch descends from its base and carries only this layer's change (plus any follow-up commits answering review) - [x] `go.mod` / `go.sum` untouched; no new dependencies ## Review Both pre-push reviewers ran against this branch — no logged skip. They gated every push, over several rounds; the last rounds reviewed the `mime.ParseMediaType` rewrite as a fresh change rather than as a delta, since the earlier approvals covered code the rewrite deleted. Reviewer findings that shaped the result, rather than only polishing it: - The first cut of the `ErrInvalidMediaParameter` guard tolerated the error unconditionally, which reintroduced the silent-truncation hazard the agreement rule exists to prevent. - A fourth (now fifth) tightening was neither listed nor pinned, and the known-limit test could not have caught a regression in it. - "A duplicate parameter name is a 415" was false: Go only errors when the values differ. - The docs' RFC delegation was falsifiable by their own example with its closing quote dropped. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Oct 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Both decisions in classify-paths.sh were
if printf … | grep -qE …; then A; else B; fi. grep exits 0 on match, 1 on no match, and 2 on error — can'tfork/exec, read error, bad pattern — and the else branch collapsed 1 and 2 into
the same answer.
set -euo pipefaildoes not help:set -eis suppressed for acommand used as an
ifcondition.Observed twice while gating this stack, in runs whose static checks run at
-j 14. A different single case failed each time —
mixed-docs-goansweringdocs=false, then
dep-bump-goanswering code=false — while every other casepassed. That is the signature of a transient grep failure under load, not a
pattern bug; the script and its test are unchanged from main and both pass
standalone.
The test caught it because it asserts expected values. The production path has
no such check: CI's
changesjob gates the docs pipeline on this answer, so adocs=false produced by an errored grep skips the docs build and still reports
success.
Both greps now go through a
matcheshelper that aborts with a diagnostic onany exit above 1. The test suite stubs grep onto PATH to prove the abort fires
— that case fails against the previous script, which answers confidently
instead.
Closes #545.
Stacked PR
This is part 0 of 7 in a stack that replaces #540. Each PR is based on the one above it, so review this PR's own diff against its base — GitHub shows only this layer's changes.
stack/0-classify-pathsmainstack/1-discoverystack/0-classify-pathsstack/2-policystack/1-discoverystack/3-content-typestack/2-policystack/4-seamsstack/3-content-typestack/5-positional-wirestack/4-seamsstack/6-computed-columnsstack/5-positional-wireMerge in order, top to bottom. Rebasing or squashing out of order will make the later PRs' diffs unreadable.
Test plan
make cigreen on this branch's exact tree (verify, unit, integration against live ClickHouse, e2e, all coverage gates)go.mod/go.sumuntouched; no new dependenciesReview
Both pre-push reviewers gate the tip of the stack (#555), whose delta against
mainis the union of all seven branches. This branch carries a logged skip (scripts/skip-pre-push-review.sh) on that basis, recorded intmp/review-skips-*.log.🤖 Generated with Claude Code
https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ