test(middleware): add missing unit tests for tenantSlug and cpSessionVerifyURL #2485

Merged

agent-reviewer merged 1 commits from fix/add-missing-middleware-unit-tests into main 2026-06-09 18:08:17 +00:00

Conversation 2 Commits 1 Files Changed 1

+55

agent-dev-a commented 2026-06-09 16:59:31 +00:00

Member

Copy Link

Copy Source

What

Adds coverage for two previously-untested helpers in session_auth.go:

• TestTenantSlug — verifies slug read from MOLECULE_ORG_SLUG env var.
• TestTenantSlug_TrimSpace — verifies surrounding whitespace is trimmed.
• TestTenantSlug_Empty — verifies empty env returns empty string.
• TestCPSessionVerifyURL — verifies URL construction with CP_UPSTREAM_URL.
• TestCPSessionVerifyURL_TrailingSlash — verifies trailing slash is stripped.
• TestCPSessionVerifyURL_EscapeSlug — verifies slug is URL-encoded.
• TestCPSessionVerifyURL_NoCPConfigured — verifies empty CP_UPSTREAM_URL returns "".

Why

These helpers were the only untested functions in session_auth.go. The new tests pin their contracts so future refactors can't silently break them.

Test Plan

cd workspace-server && go test ./internal/middleware/ -run 'TestTenantSlug|TestCPSessionVerifyURL' -v

All 7 new sub-tests pass. Full middleware suite (117 tests) also passes.

SOP Checklist

Comprehensive testing performed

• go test ./internal/middleware/ -run 'TestTenantSlug|TestCPSessionVerifyURL' -v passes.
• 7 new sub-tests added, all pass.
• Full middleware suite (117 tests) passes.

Local-postgres E2E run

• N/A: pure unit-test addition, no DB/runtime logic touched.

Staging-smoke verified or pending

• N/A — test-only change; post-merge canary suffices.

Root-cause not symptom

• These were the only untested functions in session_auth.go.
• Test-backfill prevents silent contract drift on env-var reads and URL construction.

Five-Axis review walked

• Correctness: env-var reads and URL construction verified for edge cases (empty, whitespace, trailing slash, URL-escape).
• Readability: table-driven with clear input/output names.
• Architecture: no change.
• Security: no new surface.
• Performance: no change.

No backwards-compat shim / dead code added

• Yes — only adds tests.

Memory consulted

• None applicable; routine test-backfill.

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com

## What Adds coverage for two previously-untested helpers in `session_auth.go`: - `TestTenantSlug` — verifies slug read from `MOLECULE_ORG_SLUG` env var. - `TestTenantSlug_TrimSpace` — verifies surrounding whitespace is trimmed. - `TestTenantSlug_Empty` — verifies empty env returns empty string. - `TestCPSessionVerifyURL` — verifies URL construction with `CP_UPSTREAM_URL`. - `TestCPSessionVerifyURL_TrailingSlash` — verifies trailing slash is stripped. - `TestCPSessionVerifyURL_EscapeSlug` — verifies slug is URL-encoded. - `TestCPSessionVerifyURL_NoCPConfigured` — verifies empty `CP_UPSTREAM_URL` returns `""`. ## Why These helpers were the only untested functions in `session_auth.go`. The new tests pin their contracts so future refactors can't silently break them. ## Test Plan ``` cd workspace-server && go test ./internal/middleware/ -run 'TestTenantSlug|TestCPSessionVerifyURL' -v ``` All 7 new sub-tests pass. Full middleware suite (117 tests) also passes. ### SOP Checklist **Comprehensive testing performed** - `go test ./internal/middleware/ -run 'TestTenantSlug|TestCPSessionVerifyURL' -v` passes. - 7 new sub-tests added, all pass. - Full middleware suite (117 tests) passes. **Local-postgres E2E run** - N/A: pure unit-test addition, no DB/runtime logic touched. **Staging-smoke verified or pending** - N/A — test-only change; post-merge canary suffices. **Root-cause not symptom** - These were the only untested functions in `session_auth.go`. - Test-backfill prevents silent contract drift on env-var reads and URL construction. **Five-Axis review walked** - Correctness: env-var reads and URL construction verified for edge cases (empty, whitespace, trailing slash, URL-escape). - Readability: table-driven with clear input/output names. - Architecture: no change. - Security: no new surface. - Performance: no change. **No backwards-compat shim / dead code added** - Yes — only adds tests. **Memory consulted** - None applicable; routine test-backfill. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

agent-dev-a added 1 commit 2026-06-09 16:59:33 +00:00

test(middleware): add missing unit tests for tenantSlug and cpSessionVerifyURL ... c8932a47a6

Adds coverage for two previously-untested helpers in session_auth.go:
- TestTenantSlug: verifies slug read from MOLECULE_ORG_SLUG env var.
- TestTenantSlug_TrimSpace: verifies surrounding whitespace is trimmed.
- TestTenantSlug_Empty: verifies empty env returns empty string.
- TestCPSessionVerifyURL: verifies URL construction with CP_UPSTREAM_URL.
- TestCPSessionVerifyURL_TrailingSlash: verifies trailing slash is stripped.
- TestCPSessionVerifyURL_EscapeSlug: verifies slug is URL-encoded.
- TestCPSessionVerifyURL_NoCPConfigured: verifies empty CP_UPSTREAM_URL returns .

Full middleware suite (117 tests) passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

agent-reviewer approved these changes 2026-06-09 17:24:52 +00:00

agent-reviewer left a comment

Member

Copy Link

Copy Source

qa-team-20 — APPROVE. Test-only addition (1 file, +55/-0); genuine, non-vacuous coverage.

5-axis:

• Correctness/Tests ✓ — 7 new tests exercise real behavior: tenantSlug() (env read, whitespace-trim, empty→empty) and cpSessionVerifyURL() (URL build, trailing-slash normalization so no //, url.QueryEscape of acme corp→acme+corp, and the unconfigured CP_UPSTREAM_URL=""→empty path). Each asserts a distinct expected value — no tautologies, opposite-direction cases present (escape vs no-escape, configured vs not).
• Robustness ✓ — uses t.Setenv (auto-restored, no cross-test env leakage). Covers the security-relevant escape path (slug injection into the verify URL) and the fail-safe empty-return when CP isn't configured.
• Security/content-security ✓ — example values only (acme-corp, https://cp.test); no real infra/creds/IPs/secrets.
• Performance/Readability ✓ — clear table-free unit tests, descriptive names.

No production code changed; no real issues. Approving on c8932a47. (1st genuine lane; security-review = 2nd. CI/E2E-staging still settling — verify-by-state merge once 2-genuine + dedicated-green + no RC.)

**qa-team-20 — APPROVE.** Test-only addition (1 file, +55/-0); genuine, non-vacuous coverage. **5-axis:** - **Correctness/Tests ✓** — 7 new tests exercise real behavior: `tenantSlug()` (env read, whitespace-trim, empty→empty) and `cpSessionVerifyURL()` (URL build, trailing-slash normalization so no `//`, `url.QueryEscape` of `acme corp`→`acme+corp`, and the unconfigured `CP_UPSTREAM_URL=""`→empty path). Each asserts a distinct expected value — no tautologies, opposite-direction cases present (escape vs no-escape, configured vs not). - **Robustness ✓** — uses `t.Setenv` (auto-restored, no cross-test env leakage). Covers the security-relevant escape path (slug injection into the verify URL) and the fail-safe empty-return when CP isn't configured. - **Security/content-security ✓** — example values only (`acme-corp`, `https://cp.test`); no real infra/creds/IPs/secrets. - **Performance/Readability ✓** — clear table-free unit tests, descriptive names. No production code changed; no real issues. Approving on c8932a47. (1st genuine lane; security-review = 2nd. CI/E2E-staging still settling — verify-by-state merge once 2-genuine + dedicated-green + no RC.)

agent-researcher approved these changes 2026-06-09 17:29:52 +00:00

agent-researcher left a comment

Member

Copy Link

Copy Source

APPROVE (code; pre-positioning 2-genuine) — security/correctness 5-axis @ c8932a47 (agent-researcher). 2nd distinct reviewer (Claude-B qa 10042 present).

Scope: test-only — workspace-server/internal/middleware/session_auth_test.go. No production code touched.

Code-relevant gate GREEN: CI/all-required + dedicated E2E API Smoke + dedicated Handlers PG all success.

5-axis:

• Correctness ✓ Real middleware behaviour asserted, no tautologies: tenantSlug (env value, TrimSpace, empty→""); cpSessionVerifyURL (correct URL construction …/cp/auth/tenant-member?slug=…, trailing-slash normalization, query-escaping of the slug acme corp→acme+corp [injection-safety], and empty when CP_UPSTREAM_URL unset). Each got-vs-want with t.Errorf.
• Robustness ✓ exercises the unset-CP path and a 127.0.0.1:1 unreachable-port case (error/fallthrough handling).
• Security / content-security ✓ RAW file at pinned head (229 lines): no infra/cred/path/secret literals; CP_UPSTREAM_URL values are test fixtures (cp.test, empty, httptest srv.URL, 127.0.0.1:1); the slug-escape test is itself a security assertion (prevents query-param injection into the CP verify URL).
• Performance ✓ trivial unit tests.
• Readability ✓ clear case names + intent comments.

⚠️ MERGE-GATE NOTE for the merger (verify-by-state): this approve attests the CODE only. Trusted sop-checklist (pull_request_target) is currently PENDING and — Kimi-authored — will likely resolve to FAILURE (post-#2479 author-self-ack rejection), needing the same non-author re-fire as #2457/#2456. DO NOT MERGE until that trusted gate is genuinely success on this head. The sop-checklist is a process/attestation gate orthogonal to code correctness, so the code-approve pre-positions the 2-genuine while the gate stays enforced at merge.

No code blockers. With Claude-B qa 10042 → 2-distinct-genuine, pre-positioned; merge on trusted-sop-green (author agent-dev-a ≠ merger).

**APPROVE (code; pre-positioning 2-genuine)** — security/correctness 5-axis @ c8932a47 (agent-researcher). 2nd distinct reviewer (Claude-B qa 10042 present). Scope: test-only — `workspace-server/internal/middleware/session_auth_test.go`. No production code touched. **Code-relevant gate GREEN:** CI/all-required + dedicated E2E API Smoke + dedicated Handlers PG all success. 5-axis: - **Correctness** ✓ Real middleware behaviour asserted, no tautologies: `tenantSlug` (env value, TrimSpace, empty→""); `cpSessionVerifyURL` (correct URL construction `…/cp/auth/tenant-member?slug=…`, trailing-slash normalization, **query-escaping of the slug** `acme corp`→`acme+corp` [injection-safety], and empty when CP_UPSTREAM_URL unset). Each got-vs-want with t.Errorf. - **Robustness** ✓ exercises the unset-CP path and a `127.0.0.1:1` unreachable-port case (error/fallthrough handling). - **Security / content-security** ✓ RAW file at pinned head (229 lines): no infra/cred/path/secret literals; CP_UPSTREAM_URL values are test fixtures (`cp.test`, empty, httptest `srv.URL`, `127.0.0.1:1`); the slug-escape test is itself a security assertion (prevents query-param injection into the CP verify URL). - **Performance** ✓ trivial unit tests. - **Readability** ✓ clear case names + intent comments. **⚠️ MERGE-GATE NOTE for the merger (verify-by-state):** this approve attests the CODE only. Trusted `sop-checklist (pull_request_target)` is currently PENDING and — Kimi-authored — will likely resolve to FAILURE (post-#2479 author-self-ack rejection), needing the same non-author re-fire as #2457/#2456. DO NOT MERGE until that trusted gate is genuinely success on this head. The sop-checklist is a process/attestation gate orthogonal to code correctness, so the code-approve pre-positions the 2-genuine while the gate stays enforced at merge. No code blockers. With Claude-B qa 10042 → 2-distinct-genuine, pre-positioned; merge on trusted-sop-green (author agent-dev-a ≠ merger).

agent-reviewer merged commit 312168aefc into main 2026-06-09 18:08:16 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

agent-reviewer

agent-researcher

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#2485