feat(dedupe): observe or reject ingest rows missing the id_field - #364
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (8)
|
| Layer / File(s) | Summary |
|---|---|
Config schema for require_id internal/config/config.go |
Dedupe gains a RequireID boolean field with yaml/env tags. |
Ingest handler counter and enforcement logic internal/api/ingest.go |
Adds the missing-id counter, IngestHandler.RequireID, and a branch that logs and counts missing IDs or rejects them with 400. |
Startup wiring and config defaults cmd/wavehouse/main.go, config.yaml, deployments/compose/standalone.yaml |
Wires cfg.Dedupe.RequireID into the ingest handler and adds the new config/env default values. |
Tests for missing-id behavior internal/api/ingest_test.go |
Updates the existing missing-id case and adds single-insert and NDJSON strict-mode tests. |
Documentation and changelog CHANGELOG.md, docs/src/content/docs/api.md, docs/src/content/docs/architecture.md, docs/src/content/docs/configuration.mdx, docs/src/content/docs/deployment.md |
Documents the new option, error response, ingest behavior, and deployment/config examples. |
Estimated code review effort: 2 (Simple) | ~15 minutes
Possibly related issues
Wave-RF/WaveHouse#370— addsrequire_idand missing-ID enforcement on the same ingest dedupe path.Wave-RF/WaveHouse#369— concerns the same dedupe missing-ID handling, including treating explicitnullas missing ID.
Suggested reviewers: EricAndrechek
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main change: handling ingest rows missing the dedupe id_field with observability or rejection. |
| Description check | ✅ Passed | The description is directly about the same dedupe missing-id observability and optional strict mode changes. |
| Linked Issues check | ✅ Passed | The PR implements #219 by adding a warn/counter path and an optional reject mode for missing dedupe ids. |
| Out of Scope Changes check | ✅ Passed | All changed files support dedupe missing-id observability, configuration, docs, tests, or deployment defaults. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
dedupe-missing-id-warning
✨ Simplify code
- Create PR with simplified code
- Commit simplified code in branch
dedupe-missing-id-warning
Warning
Tools execution failed with the following error:
Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error)
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 help to get the list of available commands.
github-actions
Bot
added
documentation
|
📚 Docs preview is live → https://b038baf7-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bff1b4d8-193e-4f1a-a066-cfa6cc2fc35d
📒 Files selected for processing (10)
CHANGELOG.mdcmd/wavehouse/main.goconfig.yamldeployments/compose/standalone.yamldocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxinternal/api/ingest.gointernal/api/ingest_test.gointernal/config/config.go
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Coverage
- GitHub Check: E2E tests
- GitHub Check: Docs build
- GitHub Check: Integration tests
- GitHub Check: Lint
- GitHub Check: Analyze (go)
⚠️ CI failures not shown inline (2)
GitHub Actions: PR housekeeping / 0_PR housekeeping.txt: feat(dedupe): observe or reject ingest rows missing the id_field
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
GitHub Actions: PR housekeeping / PR housekeeping: feat(dedupe): observe or reject ingest rows missing the id_field
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
🧰 Additional context used
📓 Path-based instructions (5)
cmd/wavehouse/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Wire new dependencies in
cmd/wavehouse/main.gorather than relying on global state or hidden initialization.
Files:
cmd/wavehouse/main.go
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
Author Mermaid diagrams vertically by default, keep labels short, and avoid placing two large diagrams side by side.
Files:
docs/src/content/docs/configuration.mdx
docs/src/content/docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Update the relevant docs pages when APIs, configuration, architecture, ingest format, deployment, or development workflows change; keep examples and prose accurate.
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.md
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Core packages should be interface-first: behaviors such as cache, dedupe, publisher, and subscriber should be defined behind interfaces to keep implementations swappable.
Files:
internal/config/config.gointernal/api/ingest.gointernal/api/ingest_test.go
internal/config/config.go
📄 CodeRabbit inference engine (AGENTS.md)
Add configuration fields with matching
yaml,env, andenv-defaultstruct tags, and keep the config struct in sync with documented settings.
Files:
internal/config/config.go
🧠 Learnings (4)
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.mdCHANGELOG.md
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/ingest_test.go
📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.
Applied to files:
internal/api/ingest_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/api/ingest_test.go
🔇 Additional comments (11)
internal/config/config.go (1)
135-139: LGTM!cmd/wavehouse/main.go (1)
328-336: LGTM!config.yaml (1)
73-75: LGTM!deployments/compose/standalone.yaml (1)
36-38: LGTM!CHANGELOG.md (1)
14-14: LGTM!internal/api/ingest.go (2)
24-24: LGTM!Also applies to: 42-42
392-403: Missing-id branch logic looks correct.Increments the counter unconditionally, then branches cleanly on
RequireIDfor reject-vs-warn-and-publish, matching the PR's stated non-breaking default.internal/api/ingest_test.go (1)
387-403: LGTM!Also applies to: 429-456
docs/src/content/docs/api.md (1)
228-228: LGTM!docs/src/content/docs/architecture.md (1)
77-77: LGTM!Also applies to: 171-172
docs/src/content/docs/configuration.mdx (1)
132-132: LGTM!
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Clean change — non-breaking default, and both the observe and reject paths are covered. One doc-sync nit before merge:
The reference .env in docs/src/content/docs/deployment.md (the # Optional dedup block) still lists only WH_DEDUPE_ENABLED / WH_DEDUPE_ID_FIELD. Add WH_DEDUPE_REQUIRE_ID=false there too — otherwise an operator wiring up dedup from the deployment guide never discovers strict mode. configuration.mdx, config.yaml, and the compose block are all updated correctly, just this one got missed.
There was a problem hiding this comment.
Requesting one change before merge.
The reference .env in docs/src/content/docs/deployment.md (the # Optional dedup block) still lists only WH_DEDUPE_ENABLED / WH_DEDUPE_ID_FIELD. Add WH_DEDUPE_REQUIRE_ID=false there per AGENTS.md §Documentation Sync — otherwise an operator wiring up dedup from the deployment guide never discovers strict mode. configuration.mdx, config.yaml, and the compose block are all updated correctly; just this one env reference got missed.
One line, e.g.:
WH_DEDUPE_REQUIRE_ID=false # reject rows missing the id field instead of publishing un-dedupedEverything else looks good — non-breaking default, both observe and reject paths tested. The null-id gap is tracked separately in #370, so it's not blocking here.
github-project-automation
Bot
moved this from Ready
to In review
in WaveHouse Task Board
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Approved, but I think we should consider moving the global strict mode config stuff to per-table config stuff when #222 is addressed too.
github-project-automation
Bot
moved this from In review
to In progress
in WaveHouse Task Board
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
2 participants
Summary
With dedupe enabled,
POST /v1/ingestsilently dropped its idempotency guarantee: a row missing the configuredid_fieldwas published un-deduped with no log or metric (internal/api/ingest.go), so a producer bug that dropped the id disabled dedupe for those rows unnoticed.This makes that case observable and, optionally, enforceable:
id_fieldnow emits aWARNand increments a newwavehouse_ingest_dedupe_missing_id_totalcounter (labeled bytable).dedupe.require_id(WH_DEDUPE_REQUIRE_ID, defaultfalse) rejects such rows instead of publishing them —400for a single insert, a per-record failure in a batch. Default behavior is unchanged, so this is non-breaking.Complements the client-side id auto-injection tracked in #202 with the server-side signal/guardrail that was missing.
Related Issues
Closes #219
Relates to #202