HYPERFLEET-930 - docs: add CLAUDE.md and AGENTS.md#20
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 73 lines | +0 |
| Sensitive paths | none | +0 |
| Test coverage | No _test.go files in diff | +2 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
broker/error_test.go (1)
82-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFalse-positive paths in error-handling tests
When
expectErrorisfalsebuterr != nil, these branches perform no assertion, so the test can pass incorrectly. Add explicitassert.NoError(t, err)in the non-error path before resource checks.Suggested fix
- if tt.expectError { + if tt.expectError { assert.Error(t, err) assert.Nil(t, pub) if tt.errorMsg != "" { assert.Contains(t, err.Error(), tt.errorMsg) } - } else if err == nil { + } else { + assert.NoError(t, err) // If no error expected, verify publisher is created // Note: Some publishers might fail on actual use, but creation should succeed assert.NotNil(t, pub) if pub != nil { @@ - if tt.expectError { + if tt.expectError { assert.Error(t, err) assert.Nil(t, sub) if tt.errorMsg != "" { assert.Contains(t, err.Error(), tt.errorMsg) } - } else if err == nil { + } else { + assert.NoError(t, err) // If no error expected, verify subscriber is created assert.NotNil(t, sub) if sub != nil {As per coding guidelines, "the
error_test.go“missing rabbitmq url” test is a false-passing test due to conditional assertions—don’t treat it as intended behavior".Also applies to: 170-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@broker/error_test.go` around lines 82 - 99, The non-error test path currently skips assertions when err != nil causing false positives; update the test block that checks tt.expectError to explicitly assert NoError(t, err) when tt.expectError is false (i.e., replace the "else if err == nil" branch with an else that first calls assert.NoError(t, err)), then assert.NotNil(t, pub) and only after that defer closing pub (so the defer runs after successful creation). Apply the same change to the other similar block mentioned (lines 170-187) so both test cases validate err with assert.NoError(t, err) before inspecting or deferring pub.Close().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.golangci.yml:
- Around line 70-73: Update the exclusion glob in the golangci config where it
currently lists "examples(/|$)" to the correct singular module name
"example(/|$)" so the example/ module is properly excluded; locate occurrences
of the pattern string "examples(/|$)" in .golangci.yml (including the other
instances referenced around the same block) and replace them with "example(/|$)"
to match the repository layout and ensure lint/format exclusion is effective.
In `@AGENTS.md`:
- Line 35: AGENTS.md states the DLQ topic is hardcoded while README.md still
documents a configurable dead_letter_topic; update the docs to be consistent:
either remove/mark dead_letter_topic as deprecated from README.md and add a
clear note that DLQ is hardcoded in AGENTS.md (including the hardcoded topic
name and any future plan), or if configurability is intended, update AGENTS.md
to explain how dead_letter_topic is honored and point to the config key. Ensure
both files mention the same authoritative behavior and update any other
occurrences (the other doc sections referenced) so README.md and AGENTS.md do
not contradict each other.
In `@broker/error_test.go`:
- Around line 234-241: The test currently only asserts results when expectError
is true or when configMap is empty, leaving the successful non-empty-map case
unasserted; update the assertions in the test for buildConfigFromMap so that
whenever tt.expectError is false (regardless of tt.configMap length) you assert
no error and that cfg is not nil — i.e., in the else branch for expectError add
assert.NoError(t, err) and assert.NotNil(t, cfg) using the existing tt, cfg and
err variables so the “valid map” case is covered.
In `@broker/googlepubsub.go`:
- Around line 22-35: The config struct for Google Pub/Sub removed the
dead_letter_topic field which can cause older configs to be silently ignored;
update the config parsing/validation (e.g., in the code that builds or
unmarshals googlePubSubConfig in broker/googlepubsub.go) to detect the
deprecated key "dead_letter_topic" (and its env-var equivalent, e.g.,
BROKER_GOOGLEPUBSUB_DEAD_LETTER_TOPIC) and fail fast: if that key is present,
return an error that clearly states the field is deprecated, instructs to
migrate to the new DLQ naming scheme (or config entry), and include a migration
hint; ensure this validation runs before normal defaulting logic that sets
{subscriptionID}-dlq so old configs are not silently accepted.
In `@test/integration/broker_leak_test.go`:
- Around line 28-29: This test file is missing the integration build tag, so add
the directive to the top of test/integration/broker_leak_test.go: insert the Go
build constraint lines for integration (the modern "//go:build integration" and
the legacy "// +build integration" on the line immediately before the package
declaration) so the integration suite (tests in this file) is only run when the
"integration" build tag is specified.
In `@test/integration/googlepubsub/googlepubsub_test.go`:
- Around line 11-13: Add the integration build tag to the top of the
googlepubsub_test.go test file so it is only run when integration tests are
selected: place the line //go:build integration (and optionally the legacy //
+build integration) as the very first lines before the package declaration in
test/integration/googlepubsub/googlepubsub_test.go to gate the entire file;
ensure the tag appears above any imports or package statements so test runner
respects the build constraint.
In `@test/integration/rabbitmq/rabbitmq_test.go`:
- Around line 116-120: Add the integration build tag lines at the top of this
test file so it only runs when the "integration" build tag is specified: insert
"//go:build integration" followed by the legacy "// +build integration" and a
blank line before the package declaration in
test/integration/rabbitmq/rabbitmq_test.go; this ensures the tests (including
usage of broker.NewPublisher / NewPublisher) are gated as integration tests.
---
Outside diff comments:
In `@broker/error_test.go`:
- Around line 82-99: The non-error test path currently skips assertions when err
!= nil causing false positives; update the test block that checks tt.expectError
to explicitly assert NoError(t, err) when tt.expectError is false (i.e., replace
the "else if err == nil" branch with an else that first calls assert.NoError(t,
err)), then assert.NotNil(t, pub) and only after that defer closing pub (so the
defer runs after successful creation). Apply the same change to the other
similar block mentioned (lines 170-187) so both test cases validate err with
assert.NoError(t, err) before inspecting or deferring pub.Close().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c2a9c9ea-14b0-4c5d-9aef-3ef575fd1bdc
📒 Files selected for processing (31)
.golangci.yml.pre-commit-config.yamlAGENTS.mdCLAUDE.mdMakefilebroker/broker.gobroker/cloudevents_test.gobroker/config.gobroker/config_test.gobroker/error_test.gobroker/errors.gobroker/googlepubsub.gobroker/health_test.gobroker/metrics.gobroker/metrics_test.gobroker/options_test.gobroker/publisher.gobroker/rabbitmq.gobroker/subscriber.goexample/broker.example.yamlexample/cmd/publisher/main.goexample/cmd/subscriber/main.goexample/googlepubsub/broker.yamlpkg/logger/mock.gotest/integration/broker_leak_test.gotest/integration/broker_perf_test.gotest/integration/common/common.gotest/integration/googlepubsub/googlepubsub_test.gotest/integration/googlepubsub/setup.gotest/integration/rabbitmq/rabbitmq_test.gotest/integration/rabbitmq/setup.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
example/googlepubsub/broker.yaml (1)
25-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
log_config: trueto thisbroker.yamlexample.This example violates the YAML config requirement to enable config logging for broker.yaml files.
Proposed fix
subscriber: parallelism: 1 + +log_config: trueAs per coding guidelines,
**/*.yaml: "Enablelog_config: truein broker.yaml (or setLOG_CONFIG=trueenv var) to log full config as JSON at creation time with passwords masked".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/googlepubsub/broker.yaml` around lines 25 - 45, Add the missing log_config flag to this broker.yaml example by setting log_config: true at the top-level of the document (next to the existing dead_letter/deadline/topic/receive settings), so the broker will log the full config as JSON with secrets masked; update the top-level keys that include max_outstanding_messages, max_outstanding_bytes, num_goroutines, create_topic_if_missing, create_subscription_if_missing, and dead_letter_max_attempts to ensure log_config appears alongside them in the same root mapping.example/broker.example.yaml (1)
5-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
broker.log_config: trueto the example config.This example still omits
log_config, so it doesn’t reflect the repo-required runtime config logging behavior (with masked passwords).Suggested diff
broker: # Broker type: "rabbitmq" or "googlepubsub" type: googlepubsub + # Log full broker config as JSON at creation time (passwords are masked) + log_config: trueAs per coding guidelines,
**/*.yaml: "Enablelog_config: truein broker.yaml (or setLOG_CONFIG=trueenv var) to log full config as JSON at creation time with passwords masked".Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/broker.example.yaml` around lines 5 - 8, Add the missing broker.log_config setting to the example by enabling broker.log_config: true so the example reflects runtime logging behavior (and mention you can alternatively set LOG_CONFIG=true); update the broker stanza where broker.type is defined (the broker: / type: googlepubsub block) and any other broker example blocks (the other occurrence around the same config) to include broker.log_config: true so config is logged as JSON with secrets masked at creation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@broker/error_test.go`:
- Around line 88-99: The test branch lets unexpected errors slip by—make the
behavior explicit: where the code checks expectError (variable) and handles err,
replace the loose "else if err == nil" logic with explicit assertions using the
test assert helpers (e.g., assert.NoError(t, err) when expectError == false and
assert.Error(t, err) when expectError == true), then assert pub is not nil and
schedule defer pub.Close() only after the NoError assertion; apply the same
change for the other occurrence referenced (lines around the second block with
pub/Close) so both non-error and error paths are explicitly asserted.
In `@pkg/logger/mock.go`:
- Around line 15-18: The LogEntry type is a mock-only exported struct (LogEntry)
that may imply a stable public API; to avoid this, make it unexported or move it
into a test file: rename LogEntry to logEntry (or move the entire definition
into a _test.go) and update all usages in pkg/logger/mock.go to use the
unexported name (all current usages are keyed so no literal breaks), or relocate
the type into the mock's _test file and adjust imports/usages accordingly to
keep it internal to tests.
---
Outside diff comments:
In `@example/broker.example.yaml`:
- Around line 5-8: Add the missing broker.log_config setting to the example by
enabling broker.log_config: true so the example reflects runtime logging
behavior (and mention you can alternatively set LOG_CONFIG=true); update the
broker stanza where broker.type is defined (the broker: / type: googlepubsub
block) and any other broker example blocks (the other occurrence around the same
config) to include broker.log_config: true so config is logged as JSON with
secrets masked at creation time.
In `@example/googlepubsub/broker.yaml`:
- Around line 25-45: Add the missing log_config flag to this broker.yaml example
by setting log_config: true at the top-level of the document (next to the
existing dead_letter/deadline/topic/receive settings), so the broker will log
the full config as JSON with secrets masked; update the top-level keys that
include max_outstanding_messages, max_outstanding_bytes, num_goroutines,
create_topic_if_missing, create_subscription_if_missing, and
dead_letter_max_attempts to ensure log_config appears alongside them in the same
root mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7461e6ae-7249-4004-bcf7-a7934d0e895b
📒 Files selected for processing (31)
.golangci.yml.pre-commit-config.yamlAGENTS.mdMakefileREADME.mdbroker/broker.gobroker/cloudevents_test.gobroker/config.gobroker/config_test.gobroker/error_test.gobroker/errors.gobroker/googlepubsub.gobroker/health_test.gobroker/metrics.gobroker/metrics_test.gobroker/options_test.gobroker/publisher.gobroker/rabbitmq.gobroker/subscriber.goexample/broker.example.yamlexample/cmd/publisher/main.goexample/cmd/subscriber/main.goexample/googlepubsub/broker.yamlpkg/logger/mock.gotest/integration/broker_leak_test.gotest/integration/broker_perf_test.gotest/integration/common/common.gotest/integration/googlepubsub/googlepubsub_test.gotest/integration/googlepubsub/setup.gotest/integration/rabbitmq/rabbitmq_test.gotest/integration/rabbitmq/setup.go
Contributor
Author
|
/test presubmits-integration |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/common/common.go`:
- Around line 629-631: The tests currently mutate the caller's configMap
in-place when cfg.BrokerType == broker.BrokerTypeRabbitMQ by setting
configMap["subscriber.parallelism"], which can leak state between tests;
instead, create a shallow copy of the map before modifying it (e.g., newMap :=
make(map[string]string, len(configMap)); copy all entries from configMap into
newMap) and set newMap["subscriber.parallelism"]="6", then pass newMap into the
downstream helper; apply the same non-mutating copy-and-modify fix for the other
occurrence handling subscriber.parallelism so the original configMap remains
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 37be098c-5454-4110-92c0-c7df424dfb5d
📒 Files selected for processing (1)
test/integration/common/common.go
Contributor
Author
|
/retest |
AGENTS.md contains agent-facing documentation for the broker library: verification targets, source of truth table, architecture context, project boundaries, and non-obvious gotchas. CLAUDE.md imports AGENTS.md via @AGENTS.md.
Google Pub/Sub's Receive() can only be called once per subscriber. Setting subscriber.parallelism > 1 creates multiple handlers that compete for the same Receive call, causing "Receive already in progress" errors and flaky package-level failures in CI. Only set parallelism > 1 for RabbitMQ in RunCloseWaitsForInFlightMessages and RunPanicHandler.
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)