fix: align thread-safety docs and add CI test coverage#37
Conversation
Clarify non-thread-safe container behavior, expose top-level Try* APIs, and run full test suite in task/CI with added smoke tests for public wrappers. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 20 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughAdds three error-returning exported helpers ( ChangesTry* API, Thread-Safety Clarification, and Core Tests
Documentation Updates and Security Guidance
CI and Build Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request exposes new TryProvide, TryInject, and TryInjectContext helper functions, updates the test suite and Taskfile to cover all packages, and documents that Dix containers are not thread-safe. The reviewer suggests completing the new API suite by also adding TryInjectT and TryInjectTContext helper functions to allow struct-targeted dependency injection with error handling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func TryInject(di *Dix, data any, opts ...Option) error { | ||
| return di.TryInject(data, opts...) | ||
| } | ||
|
|
||
| func TryInjectContext(ctx context.Context, di *Dix, data any, opts ...Option) error { | ||
| return di.TryInjectContext(ctx, data, opts...) | ||
| } |
There was a problem hiding this comment.
To complete the newly exposed Try* API suite and match the existing InjectT and InjectTContext helpers, consider adding TryInjectT and TryInjectTContext functions.
Currently, there is no safe way to inject into a struct of type T and return it along with an error without writing verbose boilerplate. Adding these helpers provides a clean, safe, and consistent way to perform struct-targeted dependency injection.
func TryInject(di *Dix, data any, opts ...Option) error {
return di.TryInject(data, opts...)
}
func TryInjectContext(ctx context.Context, di *Dix, data any, opts ...Option) error {
return di.TryInjectContext(ctx, data, opts...)
}
func TryInjectT[T any](di *Dix, opts ...Option) (T, error) {
var data T
typ := reflect.TypeOf(&data).Elem()
if typ.Kind() != reflect.Struct {
return data, fmt.Errorf("<T> type kind is not struct")
}
err := di.TryInject(&data, opts...)
return data, err
}
func TryInjectTContext[T any](ctx context.Context, di *Dix, opts ...Option) (T, error) {
var data T
typ := reflect.TypeOf(&data).Elem()
if typ.Kind() != reflect.Struct {
return data, fmt.Errorf("<T> type kind is not struct")
}
err := di.TryInjectContext(ctx, &data, opts...)
return data, err
}Add unit tests for dixcontext panic/nil handling and dixglobal provide/inject flows to improve wrapper-module coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/lint.yml (3)
14-15: ⚖️ Poor tradeoffPin actions to commit hashes for supply-chain security.
Both
actions/checkout@v4andactions/setup-go@v5should be pinned to specific commit hashes rather than tags. Tag-based references can be moved by attackers who compromise the action repository, creating a supply-chain attack vector.🔒 Recommended fix to pin actions
- - uses: actions/checkout@v4 - - uses: actions/setup-go@v5 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/setup-go@3041bf56c941b39c61721a86cd11f3bb1338122a # v5.2.0🤖 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 @.github/workflows/lint.yml around lines 14 - 15, Pin the GitHub Actions to specific commit hashes instead of version tags for improved supply-chain security. Replace `actions/checkout@v4` with the full commit hash of that action version and replace `actions/setup-go@v5` with its corresponding full commit hash. This prevents potential attacks where tag references could be manipulated if the action repositories are compromised.Source: Linters/SAST tools
14-14: ⚡ Quick winDisable credential persistence to prevent leakage.
By default,
actions/checkoutpersists GitHub credentials in the local git config, which can be inadvertently exposed by subsequent steps. Settingpersist-credentials: falseremoves this risk.🛡️ Recommended fix
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + persist-credentials: false🤖 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 @.github/workflows/lint.yml at line 14, The actions/checkout@v4 action at the specified location persists GitHub credentials in the local git config by default, creating a potential security risk if these credentials are exposed by subsequent workflow steps. Add the `persist-credentials: false` option to the actions/checkout action configuration to disable this credential persistence and mitigate the leakage risk.Source: Linters/SAST tools
10-21: ⚡ Quick winConsider setting explicit minimal permissions for the test job.
The test job inherits default repository permissions, which may be broader than necessary. Explicitly setting
permissions: {}or minimal read permissions follows the principle of least privilege.🔐 Recommended permissions block
test: runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 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 @.github/workflows/lint.yml around lines 10 - 21, The test job in the lint.yml workflow inherits overly broad default repository permissions. Add an explicit permissions field to the test job definition with either permissions: {} (no permissions) or minimal read-only permissions such as permissions: { contents: read } to follow the principle of least privilege. This should be added at the job level under the test job, before the steps section.Source: Linters/SAST tools
🤖 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 `@dix_test.go`:
- Around line 27-47: The test suite is missing coverage for the TryInjectContext
API, which is one of the new public wrapper APIs introduced in this PR. Add a
new test function (such as TestTryInjectContext) that provides smoke-test
coverage for this API, following the same testing pattern as the existing
TestTryInject function. The new test should verify that TryInjectContext can
successfully inject dependencies with context, validate that callbacks are
invoked correctly, and confirm that appropriate errors are returned when
required dependencies are missing.
---
Nitpick comments:
In @.github/workflows/lint.yml:
- Around line 14-15: Pin the GitHub Actions to specific commit hashes instead of
version tags for improved supply-chain security. Replace `actions/checkout@v4`
with the full commit hash of that action version and replace
`actions/setup-go@v5` with its corresponding full commit hash. This prevents
potential attacks where tag references could be manipulated if the action
repositories are compromised.
- Line 14: The actions/checkout@v4 action at the specified location persists
GitHub credentials in the local git config by default, creating a potential
security risk if these credentials are exposed by subsequent workflow steps. Add
the `persist-credentials: false` option to the actions/checkout action
configuration to disable this credential persistence and mitigate the leakage
risk.
- Around line 10-21: The test job in the lint.yml workflow inherits overly broad
default repository permissions. Add an explicit permissions field to the test
job definition with either permissions: {} (no permissions) or minimal read-only
permissions such as permissions: { contents: read } to follow the principle of
least privilege. This should be added at the job level under the test job,
before the steps section.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c31dcf0-a1e1-4c95-8d42-7f713193e371
📒 Files selected for processing (7)
.github/workflows/lint.ymlREADME.mdREADME_zh.mdTaskfile.ymldix.godix_test.godixinternal/dix.go
Improve bilingual READMEs with clearer onboarding, API/option quick references, production-safe examples, and condensed diagnostics guidance linked to detailed dixhttp docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Add dix wrapper edge-case tests and a README local-link checker wired into task and CI docs job. Co-authored-by: Cursor <cursoragent@cursor.com>
Document reverse-proxy authentication practices for dixhttp and add a reusable PR description template for review-ready changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the optional readme-check script and its CI/task wiring since it is not needed for day-to-day development. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@dixglobal/global_test.go`:
- Around line 24-32: The TestInjectT test function relies on global state from
another test to have testGlobalDep already registered, causing order-dependent
and flaky behavior. Make TestInjectT self-contained by registering the
testGlobalDep dependency at the beginning of the test itself, before calling
InjectT[app](), so the test has no external dependencies on execution order or
prior test state.
In `@README.md`:
- Around line 112-121: Add a row for the exported function InjectTContext[T] to
the API table in README.md, documenting that it allocates a struct and injects
exported fields with trace context propagation, and indicate its panic behavior
(yes, similar to InjectT[T]). Additionally, review the parameter names used in
the table (fn and target) to verify if they should be updated to match the
actual implementation parameter names (data), and consider whether the other
exported functions SetLog, WithValuesNull, WithProviderTimeout, and
WithSlowProviderThreshold should also be documented in the API table for
completeness.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b46ebb4-bf3e-4c3b-b364-390b30ebc8b6
📒 Files selected for processing (8)
README.mdREADME_zh.mddix_test.godixcontext/context_test.godixglobal/global_test.godixhttp/README.mddixhttp/README_zh.mddocs/pr_description_template.md
✅ Files skipped from review due to trivial changes (3)
- dixhttp/README.md
- docs/pr_description_template.md
- README_zh.md
Rewrite example programs with clearer scenarios, dix.New usage, run instructions, and Try* error-handling patterns aligned with the public API docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Make dixglobal InjectT test self-contained to avoid order-dependent state and document InjectTContext in both README API tables. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Clarify non-thread-safe container behavior, expose top-level Try* APIs, and run full test suite in task/CI with added smoke tests for public wrappers.
Summary by CodeRabbit
TryProvide,TryInject, andTryInjectContexthelpers for dependency operations.dixhttpsecurity guidance (including a reverse-proxy auth example).dixcontexthelpers.testjob (runsgo test ./...with-race), and broadened thetesttask to all packages.