release: v0.8.0-alpha.1 — module migration, AX-6 testify removal, AX-10 driver, threat-model coverage#4
Open
Snider wants to merge 1 commit into
Open
release: v0.8.0-alpha.1 — module migration, AX-6 testify removal, AX-10 driver, threat-model coverage#4Snider wants to merge 1 commit into
Snider wants to merge 1 commit into
Conversation
…10 driver, threat-model coverage
Consolidates the v0.8.0-alpha.1 release work for core/go-ratelimit.
* Module path migration: forge.lthn.ai/core/go-ratelimit ->
dappco.re/go/ratelimit. All sibling dappco.re/go/* deps pinned
to v0.8.0-alpha.1.
* AX-6 testify removal: replaced testify assertions with stdlib
testing patterns. Removes external test framework dep.
* AX-6 annotations: external deps and intrinsic stdlib imports
annotated at justified boundary points.
* AX-10 driver: go-ratelimit CLI test driver scaffolded per
AX-10 tests-as-artifact-validation principle.
* RFC alignment: surface touch to align with the rate-limit RFC
contract.
* Threat-model coverage: CanSend_Bad test case added — verifies
the ratelimiter rejects requests over the configured RPM
quota. Closes part of the threat-model audit lane.
Refs: RFC-CORE-008-AGENT-EXPERIENCE.md (AX-1, AX-6, AX-10)
Mantis #391 (AX-10 triad), #920 (threat-model audit)
Co-authored-by: Athena <athena@lthn.ai>
Co-authored-by: Cerberus <cerberus@lthn.ai>
Co-authored-by: Hephaestus <hephaestus@lthn.ai>
Co-authored-by: Cladius Maximus <cladius@lthn.ai>
📝 WalkthroughWalkthroughThe pull request migrates the test suite from the Changes
🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ratelimit.go (1)
417-422:⚠️ Potential issue | 🔴 CriticalHandle nil
UsageStatsentries before appending.
RecordUsagestill only initialises missing keys. Ifrl.State[model]exists with a nil value, Line 426 dereferences nil and panics; the new nil-state test now hits exactly that path.🐛 Proposed fix
stats, ok := rl.State[model] - if !ok { + if !ok || stats == nil { stats = &UsageStats{DayStart: time.Now()} rl.State[model] = stats }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratelimit.go` around lines 417 - 422, RecordUsage currently assumes rl.State[model] is non-nil after rl.prune and only creates a UsageStats when the key is missing; if the map contains the key with a nil value the subsequent dereference panics. Update RecordUsage to check for a nil value after retrieving stats (i.e., after stats, ok := rl.State[model]) and if stats == nil create and assign &UsageStats{DayStart: time.Now()} to rl.State[model] before proceeding, ensuring rl.prune(model) and the rest of RecordUsage safely operate even when an entry exists but is nil.assertions_test.go (1)
14-294:⚠️ Potential issue | 🟠 MajorPlease keep
testifyas the white-box test assertion layer.This helper file bakes in a parallel assertion API and enables the suite-wide switch to direct
t.Fatal(...)checks. That is the opposite of the repository’s test contract, and it is also why the PR can dropgithub.com/stretchr/testifyfrom the module graph. As per coding guidelinesWhite-box tests must use package ratelimit and assertions via testify (require for fatal, assert for non-fatal). Do not use t.Error/t.Fatal directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assertions_test.go` around lines 14 - 294, The new assertions_test.go introduces a parallel assertion API (functions like testFailureMessage, testUnexpectedErrorMessage, testEqual, testIsNil, testContains, testRecoverPanic, etc.) which replaces usage of testify and enables direct t.Fatal/t.Error semantics; revert this by removing or refactoring these helpers so tests use testify (require for fatal, assert for non-fatal) as the white-box assertion layer: either delete these test* helper functions and update callers to call testify's require/assert, or reimplement each helper to delegate to testify (e.g., call require.* or assert.*) while keeping them in the ratelimit package tests so the suite-wide fatal behavior remains with testify. Ensure no tests call t.Fatal/t.Error directly and restore github.com/stretchr/testify usage.ratelimit_test.go (1)
1339-1369:⚠️ Potential issue | 🟠 MajorDo not call
t.Fatalfrom httptest handler goroutines.Handler functions execute on server goroutines, not the test goroutine. Calling
t.Fatalfrom a non-test goroutine does not reliably abort the test and can cause race conditions or misleading failures. Instead, capture validation failures in the handler (e.g., write an error response or store error state) and assert on the outcome after the request returns to the test goroutine.This also applies at lines 1386–1395, 1409–1415, and 1464–1470.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratelimit_test.go` around lines 1339 - 1369, The handler passed to httptest.NewServer currently calls t.Fatal from the server goroutine; change it to record validation failures instead of calling t.Fatal directly: inside the http.HandlerFunc (the function passed to httptest.NewServer) replace each t.Fatal call with writing an error response (e.g., http.Error(w, msg, http.StatusBadRequest)) and set a shared error variable (e.g., handlerErr) or send the error on a channel (protect with a mutex if needed); after making the request from the test goroutine, assert that handlerErr is nil and call t.Fatal/t.Fatalf there if not. Apply the same fix for the other handlers that call t.Fatal (the other httptest handlers around the quoted ranges) and ensure decodeJSONBody validations also use the same error-recording pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ratelimit.go`:
- Around line 605-609: The code currently creates rl.State[model] for negative
token estimates; instead build a local UsageStats and attach it to the Decision
(Decision.Stats) without mutating rl.State. Specifically, in the branch that
handles invalid/negative token estimates, construct a localStats :=
&UsageStats{DayStart: now} and set Decision.Stats = localStats, and only assign
rl.State[model] = stats when you are recording real usage (e.g., after
validating tokens > 0 or on successful consumption). This avoids populating
rl.State with entries for invalid requests.
In `@tests/cli/ratelimit/main.go`:
- Around line 42-49: The code is double-counting quota by calling
rl.CanSend(...) and rl.Decide(...); remove the CanSend call and call
rl.Decide("gemini-2.5-flash", 100) only, then derive allowed from the single
decision (e.g., allowed := decision.Code == Allowed or decision.IsAllowed) and
build the out map using the decision fields (decision.Code, decision.Reason,
decision.RetryAfter) so quota is consumed only once and allowed is consistent
with decision.
- Around line 25-58: Replace fmt.Errorf error wrapping in function run with the
mandated core.E(...) form and add the import "dappco.re/go/core"; specifically,
in run() replace the os.MkdirTemp error return with core.E("ratelimit.run",
"mkdtemp", err), the ratelimit.NewWithConfig error return with
core.E("ratelimit.run", "new ratelimiter", err), the json.Encode error return
with core.E("ratelimit.run", "encode decision", err), and the rl.Persist error
return with core.E("ratelimit.run", "persist", err). Ensure the new import is
added to the file's imports and remove or keep fmt only if still used elsewhere.
---
Outside diff comments:
In `@assertions_test.go`:
- Around line 14-294: The new assertions_test.go introduces a parallel assertion
API (functions like testFailureMessage, testUnexpectedErrorMessage, testEqual,
testIsNil, testContains, testRecoverPanic, etc.) which replaces usage of testify
and enables direct t.Fatal/t.Error semantics; revert this by removing or
refactoring these helpers so tests use testify (require for fatal, assert for
non-fatal) as the white-box assertion layer: either delete these test* helper
functions and update callers to call testify's require/assert, or reimplement
each helper to delegate to testify (e.g., call require.* or assert.*) while
keeping them in the ratelimit package tests so the suite-wide fatal behavior
remains with testify. Ensure no tests call t.Fatal/t.Error directly and restore
github.com/stretchr/testify usage.
In `@ratelimit_test.go`:
- Around line 1339-1369: The handler passed to httptest.NewServer currently
calls t.Fatal from the server goroutine; change it to record validation failures
instead of calling t.Fatal directly: inside the http.HandlerFunc (the function
passed to httptest.NewServer) replace each t.Fatal call with writing an error
response (e.g., http.Error(w, msg, http.StatusBadRequest)) and set a shared
error variable (e.g., handlerErr) or send the error on a channel (protect with a
mutex if needed); after making the request from the test goroutine, assert that
handlerErr is nil and call t.Fatal/t.Fatalf there if not. Apply the same fix for
the other handlers that call t.Fatal (the other httptest handlers around the
quoted ranges) and ensure decodeJSONBody validations also use the same
error-recording pattern.
In `@ratelimit.go`:
- Around line 417-422: RecordUsage currently assumes rl.State[model] is non-nil
after rl.prune and only creates a UsageStats when the key is missing; if the map
contains the key with a nil value the subsequent dereference panics. Update
RecordUsage to check for a nil value after retrieving stats (i.e., after stats,
ok := rl.State[model]) and if stats == nil create and assign
&UsageStats{DayStart: time.Now()} to rl.State[model] before proceeding, ensuring
rl.prune(model) and the rest of RecordUsage safely operate even when an entry
exists but is nil.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d27317b4-a6b9-4bff-a86c-b96154add06c
📒 Files selected for processing (10)
assertions_test.goerror_test.gogo.moditer_test.goratelimit.goratelimit_test.gosqlite.gosqlite_test.gotests/cli/ratelimit/Taskfile.yamltests/cli/ratelimit/main.go
Comment on lines
+605
to
+609
| stats, ok := rl.State[model] | ||
| if !ok || stats == nil { | ||
| stats = &UsageStats{DayStart: now} | ||
| rl.State[model] = stats | ||
| } |
There was a problem hiding this comment.
Avoid persisting state for invalid requests.
This branch now creates rl.State[model] for any negative token estimate. That means a stream of invalid calls can populate arbitrary model names in memory and in the next persisted snapshot, even though no usage was ever recorded. Build the fallback Decision.Stats locally instead of mutating limiter state here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ratelimit.go` around lines 605 - 609, The code currently creates
rl.State[model] for negative token estimates; instead build a local UsageStats
and attach it to the Decision (Decision.Stats) without mutating rl.State.
Specifically, in the branch that handles invalid/negative token estimates,
construct a localStats := &UsageStats{DayStart: now} and set Decision.Stats =
localStats, and only assign rl.State[model] = stats when you are recording real
usage (e.g., after validating tokens > 0 or on successful consumption). This
avoids populating rl.State with entries for invalid requests.
Comment on lines
+25
to
+58
| func run() error { | ||
| dir, err := os.MkdirTemp("", "go-ratelimit-ax10-") | ||
| if err != nil { | ||
| return fmt.Errorf("mkdtemp: %w", err) | ||
| } | ||
| defer os.RemoveAll(dir) | ||
|
|
||
| cfg := ratelimit.Config{ | ||
| FilePath: filepath.Join(dir, "ratelimits.yaml"), | ||
| Providers: []ratelimit.Provider{ratelimit.ProviderGemini}, | ||
| } | ||
|
|
||
| rl, err := ratelimit.NewWithConfig(cfg) | ||
| if err != nil { | ||
| return fmt.Errorf("new ratelimiter: %w", err) | ||
| } | ||
|
|
||
| allowed := rl.CanSend("gemini-2.5-flash", 100) | ||
| decision := rl.Decide("gemini-2.5-flash", 100) | ||
| out := map[string]any{ | ||
| "allowed": allowed, | ||
| "code": string(decision.Code), | ||
| "reason": decision.Reason, | ||
| "retryAfterMs": decision.RetryAfter / time.Millisecond, | ||
| } | ||
| enc := json.NewEncoder(os.Stdout) | ||
| enc.SetIndent("", " ") | ||
| if err := enc.Encode(out); err != nil { | ||
| return fmt.Errorf("encode decision: %w", err) | ||
| } | ||
|
|
||
| if err := rl.Persist(); err != nil { | ||
| return fmt.Errorf("persist: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current error-construction usage in this file.
rg -n 'fmt\.Errorf\(' tests/cli/ratelimit/main.go
rg -n 'core\.E\(' tests/cli/ratelimit/main.goRepository: dAppCore/go-ratelimit
Length of output: 253
🏁 Script executed:
# Check if core package is imported elsewhere in the repository
rg -t go '"dappco.re/go/core"' --max-count 5
# Verify the structure of core.E usage in other files
rg -t go 'core\.E\(' -A 1 --max-count 3
# Check if tests/cli/ratelimit/main.go has any other Go files that might show the pattern
fd '\.go$' tests/cli/ratelimit/Repository: dAppCore/go-ratelimit
Length of output: 758
🏁 Script executed:
# Check the package declaration and confirm file structure
head -20 tests/cli/ratelimit/main.go
# Double-check no other fmt.Errorf instances exist
rg -c 'fmt\.Errorf\(' tests/cli/ratelimit/main.goRepository: dAppCore/go-ratelimit
Length of output: 451
Use the mandated core.E(...) error format in this Go file.
This file currently wraps errors with fmt.Errorf, but the project guideline requires core.E("ratelimit.FunctionName", "what", err). Four instances require updating on lines 28, 39, 53, and 57. Add the import "dappco.re/go/core" and replace each fmt.Errorf call with the corresponding core.E() format.
Suggested patch
import (
"encoding/json"
"fmt"
"os"
"path/filepath"
"time"
+ "dappco.re/go/core"
"dappco.re/go/ratelimit"
)
@@
dir, err := os.MkdirTemp("", "go-ratelimit-ax10-")
if err != nil {
- return fmt.Errorf("mkdtemp: %w", err)
+ return core.E("ratelimit.run", "mkdtemp", err)
}
@@
rl, err := ratelimit.NewWithConfig(cfg)
if err != nil {
- return fmt.Errorf("new ratelimiter: %w", err)
+ return core.E("ratelimit.run", "new ratelimiter", err)
}
@@
if err := enc.Encode(out); err != nil {
- return fmt.Errorf("encode decision: %w", err)
+ return core.E("ratelimit.run", "encode decision", err)
}
@@
if err := rl.Persist(); err != nil {
- return fmt.Errorf("persist: %w", err)
+ return core.E("ratelimit.run", "persist", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli/ratelimit/main.go` around lines 25 - 58, Replace fmt.Errorf error
wrapping in function run with the mandated core.E(...) form and add the import
"dappco.re/go/core"; specifically, in run() replace the os.MkdirTemp error
return with core.E("ratelimit.run", "mkdtemp", err), the ratelimit.NewWithConfig
error return with core.E("ratelimit.run", "new ratelimiter", err), the
json.Encode error return with core.E("ratelimit.run", "encode decision", err),
and the rl.Persist error return with core.E("ratelimit.run", "persist", err).
Ensure the new import is added to the file's imports and remove or keep fmt only
if still used elsewhere.
Comment on lines
+42
to
+49
| allowed := rl.CanSend("gemini-2.5-flash", 100) | ||
| decision := rl.Decide("gemini-2.5-flash", 100) | ||
| out := map[string]any{ | ||
| "allowed": allowed, | ||
| "code": string(decision.Code), | ||
| "reason": decision.Reason, | ||
| "retryAfterMs": decision.RetryAfter / time.Millisecond, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify CanSend semantics and this file's dual-call pattern.
rg -n -A3 -B1 'func \(rl \*RateLimiter\) CanSend\(' ratelimit.go
rg -n -C1 'allowed := rl\.CanSend|decision := rl\.Decide' tests/cli/ratelimit/main.goRepository: dAppCore/go-ratelimit
Length of output: 394
Avoid double-counting by calling Decide only once.
CanSend delegates entirely to Decide, so calling both consumes quota twice and can make allowed disagree with the returned code/reason near limits.
Suggested patch
- allowed := rl.CanSend("gemini-2.5-flash", 100)
- decision := rl.Decide("gemini-2.5-flash", 100)
+ decision := rl.Decide("gemini-2.5-flash", 100)
out := map[string]any{
- "allowed": allowed,
+ "allowed": decision.Allowed,
"code": string(decision.Code),
"reason": decision.Reason,
"retryAfterMs": decision.RetryAfter / time.Millisecond,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| allowed := rl.CanSend("gemini-2.5-flash", 100) | |
| decision := rl.Decide("gemini-2.5-flash", 100) | |
| out := map[string]any{ | |
| "allowed": allowed, | |
| "code": string(decision.Code), | |
| "reason": decision.Reason, | |
| "retryAfterMs": decision.RetryAfter / time.Millisecond, | |
| } | |
| decision := rl.Decide("gemini-2.5-flash", 100) | |
| out := map[string]any{ | |
| "allowed": decision.Allowed, | |
| "code": string(decision.Code), | |
| "reason": decision.Reason, | |
| "retryAfterMs": decision.RetryAfter / time.Millisecond, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli/ratelimit/main.go` around lines 42 - 49, The code is
double-counting quota by calling rl.CanSend(...) and rl.Decide(...); remove the
CanSend call and call rl.Decide("gemini-2.5-flash", 100) only, then derive
allowed from the single decision (e.g., allowed := decision.Code == Allowed or
decision.IsAllowed) and build the out map using the decision fields
(decision.Code, decision.Reason, decision.RetryAfter) so quota is consumed only
once and allowed is consistent with decision.
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
1 participant
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
Consolidates the v0.8.0-alpha.1 release work for core/go-ratelimit.
Headline changes
forge.lthn.ai/core/go-ratelimit→dappco.re/go/ratelimit. All siblingdappco.re/go/*deps pinned tov0.8.0-alpha.1.tests/cli/ratelimit/driver scaffolded per AX-10 tests-as-artifact-validation principle.CanSend_Badtest case added: verifies the rate-limiter rejects requests over the configured RPM quota. Closes part of the threat-model audit lane (Mantis #920).Refs
RFC-CORE-008-AGENT-EXPERIENCE.md(AX-1, AX-6, AX-10)Test plan
go test ./...passestests/cli/ratelimitdriver exercises the CLI surfaceCo-authored-by: Athena athena@lthn.ai
Co-authored-by: Cerberus cerberus@lthn.ai
Co-authored-by: Hephaestus hephaestus@lthn.ai
Co-authored-by: Cladius Maximus cladius@lthn.ai
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores
dappco.re/go/ratelimit(previouslydappco.re/go/core/go-ratelimit). Update your imports accordingly.Tests