test(hub): fix integration suite contract drift (errcode/admin/fixtures) - #1489
Merged
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe change preserves typed OIDC errors, includes route rejection reasons, and updates local-edge health resolution. Tests now use centralized error codes, deterministic administrator setup, explicit guardrails, and isolated concurrent OIDC data. ChangesRuntime behavior and integration coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@hub-server/tests/extra_test.go`:
- Around line 187-191: Update the unreferenced attachment download assertion in
the test around the HTTP request to parse the response body and verify the
returned error code equals errcode.AttachNotFound.Code, while retaining the HTTP
404 status check. Follow the response parsing and comparison pattern used in
attachment_sharing_test.go.
In `@hub-server/tests/phase1_7_integration_test.go`:
- Around line 296-302: Update the response decoding block around the created
struct so json.Unmarshal errors fail the test immediately, and replace the
t.Logf call in the created.IsOnline branch with a test failure. Preserve the
expected false assertion for the initial target state.
In `@hub-server/tests/setup_test.go`:
- Around line 152-162: Update the TokenDanceID setup so RedirectURI and
AllowedRedirectURIs remain consistent: when one field is present and the other
is missing, derive the missing field from the configured value; apply the
deterministic test URI to both fields only when both are absent. Preserve the
existing values when both are configured.
- Around line 464-478: Update seedRefreshToken so device identity conflicts are
not suppressed: load the existing device by deviceID or use an id-targeted
conflict path, then validate that its UserID and DeviceType match the requested
values and fail the test on any mismatch. Preserve creating the device when no
row exists, but remove the unconditional DoNothing behavior that can allow
inconsistent refresh-token fixtures.
- Line 19: Resolve all unmerged merge-marker changes reported by git status,
then run hub-server/go test ./... -short -count=1 and fix any failures in the
short test suite until it passes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d412aedf-7837-40dc-8588-251d21842d33
📒 Files selected for processing (14)
hub-server/internal/handler/oidc.gohub-server/internal/service/agentteam/agent_team_routing.gohub-server/internal/service/dispatch/target_health.gohub-server/tests/attachment_sharing_test.gohub-server/tests/edge_integration_test.gohub-server/tests/extra_test.gohub-server/tests/message_pin_security_test.gohub-server/tests/phase1_7_integration_test.gohub-server/tests/rest_test.gohub-server/tests/seq_test.gohub-server/tests/setup_test.gohub-server/tests/skill_mcp_integration_test.gohub-server/tests/teamrun_error_paths_test.gohub-server/tests/tokendance_oidc_e2e_test.go
Comment on lines
+187
to
+191
| // #81: download requires an active session message reference. | ||
| // A bare upload without a session context must not be | ||
| // downloadable — the handler returns 404 attach_not_found. | ||
| if w.StatusCode != http.StatusNotFound { | ||
| t.Errorf("unreferenced attachment download status = %d, want 404", w.StatusCode) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert attach_not_found, not only HTTP 404.
A missing route or another not-found response can satisfy the current status check. Parse the response and compare errcode.AttachNotFound.Code, as hub-server/tests/attachment_sharing_test.go Lines [64-65] does.
Proposed assertion
if w.StatusCode != http.StatusNotFound {
t.Errorf("unreferenced attachment download status = %d, want 404", w.StatusCode)
}
+mustCode(t, parse(w), errcode.AttachNotFound.Code, "unreferenced attachment rejected")📝 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
| // #81: download requires an active session message reference. | |
| // A bare upload without a session context must not be | |
| // downloadable — the handler returns 404 attach_not_found. | |
| if w.StatusCode != http.StatusNotFound { | |
| t.Errorf("unreferenced attachment download status = %d, want 404", w.StatusCode) | |
| // `#81`: download requires an active session message reference. | |
| // A bare upload without a session context must not be | |
| // downloadable — the handler returns 404 attach_not_found. | |
| if w.StatusCode != http.StatusNotFound { | |
| t.Errorf("unreferenced attachment download status = %d, want 404", w.StatusCode) | |
| } | |
| mustCode(t, parse(w), errcode.AttachNotFound.Code, "unreferenced attachment rejected") |
🤖 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 `@hub-server/tests/extra_test.go` around lines 187 - 191, Update the
unreferenced attachment download assertion in the test around the HTTP request
to parse the response body and verify the returned error code equals
errcode.AttachNotFound.Code, while retaining the HTTP 404 status check. Follow
the response parsing and comparison pattern used in attachment_sharing_test.go.
Comment on lines
+296
to
+302
| // is_online is a JSON boolean; decode it as bool, not via extract(). | ||
| var created struct { | ||
| IsOnline bool `json:"is_online"` | ||
| } | ||
| json.Unmarshal(r.Data, &created) | ||
| if created.IsOnline { | ||
| t.Logf("initial is_online = true (expected false)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail when the initial target is online.
The test comment requires is_online to be false, but t.Logf records the mismatch and lets the test pass. Fail the test when created.IsOnline is true. Also fail when json.Unmarshal returns an error.
Proposed fix
- json.Unmarshal(r.Data, &created)
+ if err := json.Unmarshal(r.Data, &created); err != nil {
+ t.Fatalf("decode initial target: %v", err)
+ }
if created.IsOnline {
- t.Logf("initial is_online = true (expected false)")
+ t.Fatalf("initial is_online = true, expected false")
}📝 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
| // is_online is a JSON boolean; decode it as bool, not via extract(). | |
| var created struct { | |
| IsOnline bool `json:"is_online"` | |
| } | |
| json.Unmarshal(r.Data, &created) | |
| if created.IsOnline { | |
| t.Logf("initial is_online = true (expected false)") | |
| // is_online is a JSON boolean; decode it as bool, not via extract(). | |
| var created struct { | |
| IsOnline bool `json:"is_online"` | |
| } | |
| if err := json.Unmarshal(r.Data, &created); err != nil { | |
| t.Fatalf("decode initial target: %v", err) | |
| } | |
| if created.IsOnline { | |
| t.Fatalf("initial is_online = true, expected 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 `@hub-server/tests/phase1_7_integration_test.go` around lines 296 - 302, Update
the response decoding block around the created struct so json.Unmarshal errors
fail the test immediately, and replace the t.Logf call in the created.IsOnline
branch with a test failure. Preserve the expected false assertion for the
initial target state.
Comment on lines
+152
to
+162
| // config.yaml ships production-empty TokenDance ID values (client_id "", | ||
| // redirect_uri "", allowed_redirect_uris []), which makes the OIDC | ||
| // authorize success path unreachable in tests. Inject a deterministic | ||
| // test redirect URI so authorize/callback flows are exercisable. | ||
| if cfg.TokenDanceID.RedirectURI == "" { | ||
| cfg.TokenDanceID.RedirectURI = "http://127.0.0.1:54321/callback" | ||
| } | ||
| if len(cfg.TokenDanceID.AllowedRedirectURIs) == 0 { | ||
| cfg.TokenDanceID.AllowedRedirectURIs = []string{"http://127.0.0.1:54321/callback"} | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep RedirectURI and AllowedRedirectURIs consistent.
If only one field is configured, this code creates a mismatch. A configured RedirectURI with an empty allowlist is replaced by an allowlist containing the hard-coded URI. The OIDC validator then rejects the configured fallback. The inverse case has the same problem.
Fill a missing field from the configured field, or apply the test default to both fields only when both are absent.
Proposed fix
+ const testRedirectURI = "http://127.0.0.1:54321/callback"
+
if cfg.TokenDanceID.RedirectURI == "" {
- cfg.TokenDanceID.RedirectURI = "http://127.0.0.1:54321/callback"
+ if len(cfg.TokenDanceID.AllowedRedirectURIs) > 0 {
+ cfg.TokenDanceID.RedirectURI = cfg.TokenDanceID.AllowedRedirectURIs[0]
+ } else {
+ cfg.TokenDanceID.RedirectURI = testRedirectURI
+ }
}
if len(cfg.TokenDanceID.AllowedRedirectURIs) == 0 {
- cfg.TokenDanceID.AllowedRedirectURIs = []string{"http://127.0.0.1:54321/callback"}
+ cfg.TokenDanceID.AllowedRedirectURIs = []string{cfg.TokenDanceID.RedirectURI}
}📝 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
| // config.yaml ships production-empty TokenDance ID values (client_id "", | |
| // redirect_uri "", allowed_redirect_uris []), which makes the OIDC | |
| // authorize success path unreachable in tests. Inject a deterministic | |
| // test redirect URI so authorize/callback flows are exercisable. | |
| if cfg.TokenDanceID.RedirectURI == "" { | |
| cfg.TokenDanceID.RedirectURI = "http://127.0.0.1:54321/callback" | |
| } | |
| if len(cfg.TokenDanceID.AllowedRedirectURIs) == 0 { | |
| cfg.TokenDanceID.AllowedRedirectURIs = []string{"http://127.0.0.1:54321/callback"} | |
| } | |
| // config.yaml ships production-empty TokenDance ID values (client_id "", | |
| // redirect_uri "", allowed_redirect_uris []), which makes the OIDC | |
| // authorize success path unreachable in tests. Inject a deterministic | |
| // test redirect URI so authorize/callback flows are exercisable. | |
| const testRedirectURI = "http://127.0.0.1:54321/callback" | |
| if cfg.TokenDanceID.RedirectURI == "" { | |
| if len(cfg.TokenDanceID.AllowedRedirectURIs) > 0 { | |
| cfg.TokenDanceID.RedirectURI = cfg.TokenDanceID.AllowedRedirectURIs[0] | |
| } else { | |
| cfg.TokenDanceID.RedirectURI = testRedirectURI | |
| } | |
| } | |
| if len(cfg.TokenDanceID.AllowedRedirectURIs) == 0 { | |
| cfg.TokenDanceID.AllowedRedirectURIs = []string{cfg.TokenDanceID.RedirectURI} | |
| } |
🤖 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 `@hub-server/tests/setup_test.go` around lines 152 - 162, Update the
TokenDanceID setup so RedirectURI and AllowedRedirectURIs remain consistent:
when one field is present and the other is missing, derive the missing field
from the configured value; apply the deterministic test URI to both fields only
when both are absent. Preserve the existing values when both are configured.
Comment on lines
+464
to
+478
| // refresh_tokens.device_id has an FK to devices.id, so the device row is | ||
| // seeded first (a real login registers it via device registration). | ||
| func seedRefreshToken(t *testing.T, userID, deviceType, deviceID string) string { | ||
| t.Helper() | ||
|
|
||
| device := &model.Device{ | ||
| ID: deviceID, | ||
| UserID: userID, | ||
| DeviceType: deviceType, | ||
| Capabilities: "[]", | ||
| } | ||
| if err := db.Clauses(clause.OnConflict{DoNothing: true}).Create(device).Error; err != nil { | ||
| t.Fatalf("seed device %s for refresh token: %v", deviceID, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 'seedRefreshToken|testDeviceID|refresh_tokens|device_id' \
hub-server/tests --glob '*.go'Repository: TokenDanceLab/AgentHub
Length of output: 28485
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- setup_test.go seedRefreshToken ---\n'
sed -n '459,508p' hub-server/tests/setup_test.go
printf '\n--- device model/schema references ---\n'
rg -n -C 4 'type Device|Devices|refresh_tokens|refreshTokens|device_id|deviceID|Constraint: "unique|Unique|device|devices' hub-server --glob '*.go' | head -n 300
printf '\n--- repository device methods ---\n'
rg -n -C 5 'UpsertDevice|CreateDevice|FindDevice|Device_|Devices' hub-server --glob '*.go' | head -n 400Repository: TokenDanceLab/AgentHub
Length of output: 251
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- setup_test.go seedRefreshToken ---'
sed -n '459,508p' hub-server/tests/setup_test.go
printf '%s\n' ''
printf '%s\n' '--- device model/schema references ---'
rg -n -C 4 'type Device|Devices|refresh_tokens|refreshTokens|device_id|deviceID|Constraint: "unique|unique|device|devices' hub-server --glob '*.go' | head -n 300 || true
printf '%s\n' ''
printf '%s\n' '--- repository device methods ---'
rg -n -C 5 'UpsertDevice|CreateDevice|FindDevice|Device_|Devices' hub-server --glob '*.go' | head -n 400 || trueRepository: TokenDanceLab/AgentHub
Length of output: 48548
Do not hide device identity conflicts.
clause.OnConflict{DoNothing: true} suppresses conflicting device inserts. If deviceID already belongs to another user or has another DeviceType, the insert is skipped and the function seeds a refresh token with the requested userID and deviceID, creating inconsistent fixture state. Load the existing device and assert UserID and DeviceType, or target the conflict on id with ownership validation and fail on mismatch.
🤖 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 `@hub-server/tests/setup_test.go` around lines 464 - 478, Update
seedRefreshToken so device identity conflicts are not suppressed: load the
existing device by deviceID or use an id-targeted conflict path, then validate
that its UserID and DeviceType match the requested values and fail the test on
any mismatch. Preserve creating the device when no row exists, but remove the
unconditional DoNothing behavior that can allow inconsistent refresh-token
fixtures.
added 3 commits
August 2, 2026 01:09
…rojection - agentteam.rejectRouteDecision: return ErrBadRequest.WithMessage(reason) so the coordinator UI sees which limit was hit (repeat/budget/task/ active/instructions) instead of generic "invalid request" - dispatch health projection: a successful manual ping (is_online + health_state=online + fresh last_seen_at) counts as live evidence even before the edge device registers its heartbeat; device binding is still enforced independently at dispatch time via BoundDeviceID - oidc handler safeOIDCServiceError: pass through other errcode errors (e.g. ErrBadRequest) instead of flattening them to 500 internal_error
- assert lowercase snake_case errcode values via errcode.* constants instead of legacy UPPER_CASE strings (OK, msg_blocked_by_receiver, session_not_member, auth_device_mismatch, ...) - registerAsAdmin: pre-declared admin user (AGENTHUB_ADMIN_USERS) so admin-gated endpoints (publish/audit/market) are exercisable - TestMain: inject OIDC redirect_uri/allowed config so the authorize success path is reachable - seedRefreshToken: seed the devices row first — refresh_tokens.device_id has an FK to devices.id - OIDC E2E: real UUID device_ids, updated scope assertion (email added), devices user_type index aligned to production (non-unique), shared-cache SQLite for concurrent logins, distinct mock sub per concurrent login - TeamRun error-path guardrails: active sub-agent default raised to 8 so repeat/task/budget limits dominate; ActiveSubAgentLimit overrides its own - execution target ping: is_online=true after ping; market rate: publish via admin user
…eDecision WithMessage rejectRouteDecision now returns ErrBadRequest.WithMessage(specific reason) so the coordinator UI sees which guardrail was hit. Service unit tests compared sentinels with assert.Equal, which requires an exact message match; switch ErrBadRequest assertions to assert.ErrorIs (code match).
DeliciousBuding
force-pushed
the
fix/hub-integration-contract
branch
from
August 1, 2026 17:09
2841e13 to
07520e2
Compare
DeliciousBuding
pushed a commit
that referenced
this pull request
Aug 1, 2026
…wave) - Hub integration fixtures: merged in #1489 - Edge tests on Linux: merged in #1491 (incl. ACP request-sequence race fix) - TokenDance ID defaults: merged in #1480 (+ check-secrets.sh *_URL exemption) - Frontend coverage gate: merged #1490 (web 63.14→66.72) / #1488 (desktop) - Edge lint debt: merged in #1491 (102→0, zero exclusions, .gitattributes eol) Remaining: CI policy parser (#1481), Doc SSOT compaction, Desktop typecheck, shared frontend tests.
DeliciousBuding
added a commit
that referenced
this pull request
Aug 1, 2026
…wave) (#1492) - Hub integration fixtures: merged in #1489 - Edge tests on Linux: merged in #1491 (incl. ACP request-sequence race fix) - TokenDance ID defaults: merged in #1480 (+ check-secrets.sh *_URL exemption) - Frontend coverage gate: merged #1490 (web 63.14→66.72) / #1488 (desktop) - Edge lint debt: merged in #1491 (102→0, zero exclusions, .gitattributes eol) Remaining: CI policy parser (#1481), Doc SSOT compaction, Desktop typecheck, shared frontend tests. Co-authored-by: Codex (Delicious233) <codex@vectorcontrol.tech>
DeliciousBuding
added a commit
that referenced
this pull request
Aug 11, 2026
…es) (#1489) * fix(hub): surface guardrail reasons and honor manual ping in health projection - agentteam.rejectRouteDecision: return ErrBadRequest.WithMessage(reason) so the coordinator UI sees which limit was hit (repeat/budget/task/ active/instructions) instead of generic "invalid request" - dispatch health projection: a successful manual ping (is_online + health_state=online + fresh last_seen_at) counts as live evidence even before the edge device registers its heartbeat; device binding is still enforced independently at dispatch time via BoundDeviceID - oidc handler safeOIDCServiceError: pass through other errcode errors (e.g. ErrBadRequest) instead of flattening them to 500 internal_error * test(hub): fix integration suite contract drift (errcode/admin/fixtures) - assert lowercase snake_case errcode values via errcode.* constants instead of legacy UPPER_CASE strings (OK, msg_blocked_by_receiver, session_not_member, auth_device_mismatch, ...) - registerAsAdmin: pre-declared admin user (AGENTHUB_ADMIN_USERS) so admin-gated endpoints (publish/audit/market) are exercisable - TestMain: inject OIDC redirect_uri/allowed config so the authorize success path is reachable - seedRefreshToken: seed the devices row first — refresh_tokens.device_id has an FK to devices.id - OIDC E2E: real UUID device_ids, updated scope assertion (email added), devices user_type index aligned to production (non-unique), shared-cache SQLite for concurrent logins, distinct mock sub per concurrent login - TeamRun error-path guardrails: active sub-agent default raised to 8 so repeat/task/budget limits dominate; ActiveSubAgentLimit overrides its own - execution target ping: is_online=true after ping; market rate: publish via admin user * test(hub): agentteam sentinel assertions use ErrorIs after rejectRouteDecision WithMessage rejectRouteDecision now returns ErrBadRequest.WithMessage(specific reason) so the coordinator UI sees which guardrail was hit. Service unit tests compared sentinels with assert.Equal, which requires an exact message match; switch ErrBadRequest assertions to assert.ErrorIs (code match). --------- Co-authored-by: Codex <codex@vectorcontrol.tech>
DeliciousBuding
added a commit
that referenced
this pull request
Aug 11, 2026
…wave) (#1492) - Hub integration fixtures: merged in #1489 - Edge tests on Linux: merged in #1491 (incl. ACP request-sequence race fix) - TokenDance ID defaults: merged in #1480 (+ check-secrets.sh *_URL exemption) - Frontend coverage gate: merged #1490 (web 63.14→66.72) / #1488 (desktop) - Edge lint debt: merged in #1491 (102→0, zero exclusions, .gitattributes eol) Remaining: CI policy parser (#1481), Doc SSOT compaction, Desktop typecheck, shared frontend tests. Co-authored-by: Codex <codex@vectorcontrol.tech>
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
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.
背景
master 的 backend-integration CI（PostgreSQL + Redis）长期红：集成测试与实现契约漂移（errcode 大小写、admin 门禁、OIDC 夹具、FK 约束）。
修复内容
实现（3 文件）
agentteam.rejectRouteDecision：返回ErrBadRequest.WithMessage(reason)，协调器 UI 能看到具体限制（repeat/budget/task/active/instructions）而非 generic "invalid request"测试（11 文件）
验证
本地完整
go test ./tests/ -count=1（PG+Redis 容器）全绿：此前 41 个失败 → 0。Summary by CodeRabbit
Bug Fixes
Reliability