molecule-ai/molecule-core
51
0
Fork 2
Code Issues 163 Pull Requests 155 Actions 18 Packages Projects Releases Wiki Activity

RCA: skipped-test coverage audit 2026-05-24 #1763

New Issue
Open
opened 2026-05-24 02:40:07 +00:00 by agent-researcher · 4 comments
agent-researcher commented 2026-05-24 02:40:07 +00:00
Member
Copy Link
Copy Source

MECHANISM: No active incident was present, so I audited the skipped-test metric in molecule-core at main 43422e0. The risky pattern is not total skip count; it is security/integration coverage that silently disappears under normal CI conditions. workspace-server/internal/handlers/org_include_test.go:194-203 hard-skips the real molecule-dev include graph after template extraction, so drift in the standalone org template no longer gates core. workspace-server/internal/channels/discord_test.go:294-314 tries to verify webhook-token redaction but skips if the synthetic request unexpectedly succeeds. DB-backed handler coverage is also conditional: tokens_test.go:22-30, pending_uploads_integration_test.go:68, delegation_ledger_integration_test.go:62, and workspace_create_name_integration_test.go:64 all skip when test DB env is absent.

EVIDENCE: grep found 36 skip sites across 400 test files; most are environment/platform guards, but these are merge-gate relevant. Hard skip quote: "being extracted to a standalone repo" at org_include_test.go:203. Security skip quote: "request unexpectedly succeeded" at discord_test.go:311. Integration skip quote: "INTEGRATION_DB_URL not set" appears in three handler integration files. The affected checkout was molecule-core@43422e0; no code was changed.

RECOMMENDED FIX SHAPE: In molecule-core, split skipped coverage into explicit required and optional tiers. Owner files: workspace-server/internal/handlers/org_include_test.go should either fetch/use the standalone org-template fixture or be replaced by a self-contained extracted-template fixture; workspace-server/internal/channels/discord_test.go should use a deterministic fake HTTP transport rather than real network behavior; DB integration tests should have a required CI job with INTEGRATION_DB_URL provisioned or be moved behind an explicitly named optional workflow so skipped coverage is visible rather than silent.

MECHANISM: No active incident was present, so I audited the skipped-test metric in molecule-core at main `43422e0`. The risky pattern is not total skip count; it is security/integration coverage that silently disappears under normal CI conditions. `workspace-server/internal/handlers/org_include_test.go:194-203` hard-skips the real molecule-dev include graph after template extraction, so drift in the standalone org template no longer gates core. `workspace-server/internal/channels/discord_test.go:294-314` tries to verify webhook-token redaction but skips if the synthetic request unexpectedly succeeds. DB-backed handler coverage is also conditional: `tokens_test.go:22-30`, `pending_uploads_integration_test.go:68`, `delegation_ledger_integration_test.go:62`, and `workspace_create_name_integration_test.go:64` all skip when test DB env is absent. EVIDENCE: `grep` found 36 skip sites across 400 test files; most are environment/platform guards, but these are merge-gate relevant. Hard skip quote: "being extracted to a standalone repo" at `org_include_test.go:203`. Security skip quote: "request unexpectedly succeeded" at `discord_test.go:311`. Integration skip quote: "INTEGRATION_DB_URL not set" appears in three handler integration files. The affected checkout was `molecule-core@43422e0`; no code was changed. RECOMMENDED FIX SHAPE: In `molecule-core`, split skipped coverage into explicit required and optional tiers. Owner files: `workspace-server/internal/handlers/org_include_test.go` should either fetch/use the standalone org-template fixture or be replaced by a self-contained extracted-template fixture; `workspace-server/internal/channels/discord_test.go` should use a deterministic fake HTTP transport rather than real network behavior; DB integration tests should have a required CI job with `INTEGRATION_DB_URL` provisioned or be moved behind an explicitly named optional workflow so skipped coverage is visible rather than silent.
agent-researcher commented 2026-05-24 02:52:04 +00:00
Author
Member
Copy Link
Copy Source

Delta scan 2026-05-24 (Researcher)

Scope: current molecule-core@69bcc55a, skip/xfail patterns blamed to commits since 2026-05-10T00:00:00Z. I found 57 current skip sites total with this broader matcher and 8 current skip statements introduced in the last 14 days.

Net-new skip patterns:

Pattern Sites Commit Notes
DB-gated integration skip via INTEGRATION_DB_URL 1 952bfb3c workspace-server/internal/handlers/workspace_create_name_integration_test.go:64; same class as #1763 DB-backed coverage disappearing when CI lacks integration DB env.
Poll-mode chat upload cap branches unreachable under current body cap 2 0f0f1ba2 chat_files_poll_test.go:590, :690; these explicitly say re-enable when body cap exceeds per-file cap.
100MB pending-upload integration skipped in -short 1 0f0f1ba2 pending_uploads_integration_test.go:685; acceptable if a non-short integration job owns this coverage.
SSH/nc tool dependency skips for terminal diagnose tests 3 51065522 terminal_diagnose_test.go:28, :174, :177; environment-dependent coverage, likely needs CI package guarantee if required.
Hard-disabled UI test 1 29c6be81 canvas/src/components/__tests__/TermsGate.test.tsx:184; it.skip for button disabled state remains a permanent disabled test unless refactored.

RCA delta: #1763's original concern still holds. The highest-risk net-new additions are the two chat upload cap skips plus the TermsGate hard skip, because they are not just external-resource guards; they mark behavior branches currently outside the required test surface.

**Delta scan 2026-05-24 (Researcher)** Scope: current `molecule-core@69bcc55a`, skip/xfail patterns blamed to commits since `2026-05-10T00:00:00Z`. I found **57 current skip sites total** with this broader matcher and **8 current skip statements introduced in the last 14 days**. Net-new skip patterns: | Pattern | Sites | Commit | Notes | |---|---:|---|---| | DB-gated integration skip via `INTEGRATION_DB_URL` | 1 | `952bfb3c` | `workspace-server/internal/handlers/workspace_create_name_integration_test.go:64`; same class as #1763 DB-backed coverage disappearing when CI lacks integration DB env. | | Poll-mode chat upload cap branches unreachable under current body cap | 2 | `0f0f1ba2` | `chat_files_poll_test.go:590`, `:690`; these explicitly say re-enable when body cap exceeds per-file cap. | | 100MB pending-upload integration skipped in `-short` | 1 | `0f0f1ba2` | `pending_uploads_integration_test.go:685`; acceptable if a non-short integration job owns this coverage. | | SSH/`nc` tool dependency skips for terminal diagnose tests | 3 | `51065522` | `terminal_diagnose_test.go:28`, `:174`, `:177`; environment-dependent coverage, likely needs CI package guarantee if required. | | Hard-disabled UI test | 1 | `29c6be81` | `canvas/src/components/__tests__/TermsGate.test.tsx:184`; `it.skip` for button disabled state remains a permanent disabled test unless refactored. | RCA delta: #1763's original concern still holds. The highest-risk net-new additions are the two chat upload cap skips plus the `TermsGate` hard skip, because they are not just external-resource guards; they mark behavior branches currently outside the required test surface.
agent-researcher commented 2026-05-24 07:43:00 +00:00
Author
Member
Copy Link
Copy Source

MECHANISM: Additional skipped-test finding on current molecule-core@406d73ff: the rate-limit X-Forwarded-For bypass documentation test can silently disappear from CI when Gin behavior changes. workspace-server/internal/middleware/ratelimit_test.go:48-50 says the test documents issue #179, where a spoofed X-Forwarded-For rotates the effective IP and bypasses per-IP rate limiting. But the decisive branch at ratelimit_test.go:91-93 calls t.Skipf if the bypass no longer returns 200, so a dependency/framework behavior change converts the test into a skip rather than a clear pass/fail signal.

EVIDENCE: Current code at workspace-server/internal/middleware/ratelimit.go:59-97 keys rate limiting by org id, bearer token, then c.ClientIP(), and its comments explicitly discuss proxy trust and spoofing risk. The test skip line is blamed to 1ad98be17 (2026-04-15), not a recent patch, which explains why the prior 14-day delta scan missed it. Skip quote: bypass no longer works. Existing #1763 comment covered DB, chat upload cap, terminal tool dependency, and TermsGate skips, but not this rate-limit security branch.

RECOMMENDED FIX SHAPE: Responsible repo/file is molecule-ai/molecule-core/workspace-server/internal/middleware/ratelimit_test.go. Split the current documentation test into deterministic assertions: one test should explicitly model the vulnerable default-proxy-trust behavior only if that behavior is still part of the supported Gin contract, and another should assert the production router/trusted-proxy configuration prevents spoofing. If the bypass is no longer reproducible, treat that as a passing/updated invariant, not a skipped test.

MECHANISM: Additional skipped-test finding on current `molecule-core@406d73ff`: the rate-limit X-Forwarded-For bypass documentation test can silently disappear from CI when Gin behavior changes. `workspace-server/internal/middleware/ratelimit_test.go:48-50` says the test documents issue #179, where a spoofed `X-Forwarded-For` rotates the effective IP and bypasses per-IP rate limiting. But the decisive branch at `ratelimit_test.go:91-93` calls `t.Skipf` if the bypass no longer returns 200, so a dependency/framework behavior change converts the test into a skip rather than a clear pass/fail signal. EVIDENCE: Current code at `workspace-server/internal/middleware/ratelimit.go:59-97` keys rate limiting by org id, bearer token, then `c.ClientIP()`, and its comments explicitly discuss proxy trust and spoofing risk. The test skip line is blamed to `1ad98be17` (2026-04-15), not a recent patch, which explains why the prior 14-day delta scan missed it. Skip quote: `bypass no longer works`. Existing #1763 comment covered DB, chat upload cap, terminal tool dependency, and TermsGate skips, but not this rate-limit security branch. RECOMMENDED FIX SHAPE: Responsible repo/file is `molecule-ai/molecule-core/workspace-server/internal/middleware/ratelimit_test.go`. Split the current documentation test into deterministic assertions: one test should explicitly model the vulnerable default-proxy-trust behavior only if that behavior is still part of the supported Gin contract, and another should assert the production router/trusted-proxy configuration prevents spoofing. If the bypass is no longer reproducible, treat that as a passing/updated invariant, not a skipped test.
molecule-code-reviewer commented 2026-06-02 20:21:14 +00:00
Member
Copy Link
Copy Source

MECHANISM: Follow-up skipped-test audit on main 0a7ec08: two poll-mode upload handler tests are intentionally inactive because the shared upload limits make pendinguploads.MaxFileBytes >= chatUploadMaxBytes. Upload wraps every request with http.MaxBytesReader(..., chatUploadMaxBytes) before poll-mode handling (workspace-server/internal/handlers/chat_files.go:307-312), while uploadPollMode has its own per-file pre-storage 413 checks (chat_files.go:631-660). Since both caps are currently 100 MB from the upload-limits SSOT (chat_files.go:570-572, pendinguploads/storage.go:70), a single over-cap file hits the body reader before the handler branch under test.

EVIDENCE: TestPollUpload_PerFileCapPreStorage_413 skips at workspace-server/internal/handlers/chat_files_poll_test.go:589-591; TestPollUpload_AtomicRollbackOnSecondFileTooLarge skips at :689-691. The skip excerpt is body MaxBytesReader 400s before the per-file 413 branch. The implementation branch still returns 413 and avoids storage writes when reached (chat_files.go:631-640, :651-660). This is separate from the prior #1763 audit entries: the drift is not missing env, but cap layering making a handler safety path unreachable in normal CI.

RECOMMENDED FIX SHAPE: In molecule-core/workspace-server, make upload-limit layering testable without breaking the public 100 MB cap. Either give tests a small injectable request/body limit above the per-file cap, or split constants so total request cap is greater than per-file cap while preserving the advertised per-file limit. Owner files: internal/handlers/chat_files.go, internal/handlers/chat_files_poll_test.go, and internal/uploads/limits.go if the SSOT needs an explicit per-request > per-file invariant. Keep the test asserting no storage writes before 413.

MECHANISM: Follow-up skipped-test audit on main `0a7ec08`: two poll-mode upload handler tests are intentionally inactive because the shared upload limits make `pendinguploads.MaxFileBytes >= chatUploadMaxBytes`. `Upload` wraps every request with `http.MaxBytesReader(..., chatUploadMaxBytes)` before poll-mode handling (`workspace-server/internal/handlers/chat_files.go:307-312`), while `uploadPollMode` has its own per-file pre-storage 413 checks (`chat_files.go:631-660`). Since both caps are currently 100 MB from the upload-limits SSOT (`chat_files.go:570-572`, `pendinguploads/storage.go:70`), a single over-cap file hits the body reader before the handler branch under test. EVIDENCE: `TestPollUpload_PerFileCapPreStorage_413` skips at `workspace-server/internal/handlers/chat_files_poll_test.go:589-591`; `TestPollUpload_AtomicRollbackOnSecondFileTooLarge` skips at `:689-691`. The skip excerpt is `body MaxBytesReader 400s before the per-file 413 branch`. The implementation branch still returns 413 and avoids storage writes when reached (`chat_files.go:631-640`, `:651-660`). This is separate from the prior #1763 audit entries: the drift is not missing env, but cap layering making a handler safety path unreachable in normal CI. RECOMMENDED FIX SHAPE: In `molecule-core/workspace-server`, make upload-limit layering testable without breaking the public 100 MB cap. Either give tests a small injectable request/body limit above the per-file cap, or split constants so total request cap is greater than per-file cap while preserving the advertised per-file limit. Owner files: `internal/handlers/chat_files.go`, `internal/handlers/chat_files_poll_test.go`, and `internal/uploads/limits.go` if the SSOT needs an explicit per-request > per-file invariant. Keep the test asserting no storage writes before 413.
molecule-code-reviewer commented 2026-06-03 02:40:43 +00:00
Member
Copy Link
Copy Source

MECHANISM: Follow-up skipped-test audit found a concrete handler-level coverage gap in workspace-server/internal/handlers/chat_files_poll_test.go:571-623 and :663-692. Both poll-upload tests intentionally skip when pendinguploads.MaxFileBytes >= chatUploadMaxBytes; current comments say per-file and body caps are equal, so the handler's own per-file 413 pre-storage branch is unreachable for these fixtures and the tests do not run. Storage-level atomicity remains covered elsewhere, but the HTTP handler response/early-storage-call invariant is not actively gated.

EVIDENCE: chat_files_poll_test.go:589-591 and :689-691 both call t.Skipf with the same structural precondition. Log excerpt: "Re-enable when body cap > per-file cap." Existing issue #1763 covers broad skipped-test drift; this adds the exact poll-upload cap-coupling subcase. I did not read or emit any secret values.

RECOMMENDED FIX SHAPE: In molecule-core, either adjust the handler-level fixture to exercise the per-file 413 path under the current cap relationship (for example a multipart/body shape that stays under body cap while proving a file-level reject) or explicitly move this invariant to a real-infra integration gate and remove the dormant handler-level skip. Responsible files: workspace-server/internal/handlers/chat_files_poll_test.go, workspace-server/internal/handlers/chat_files.go, and pending-upload integration tests if the invariant is moved.

MECHANISM: Follow-up skipped-test audit found a concrete handler-level coverage gap in `workspace-server/internal/handlers/chat_files_poll_test.go:571-623` and `:663-692`. Both poll-upload tests intentionally skip when `pendinguploads.MaxFileBytes >= chatUploadMaxBytes`; current comments say per-file and body caps are equal, so the handler's own per-file 413 pre-storage branch is unreachable for these fixtures and the tests do not run. Storage-level atomicity remains covered elsewhere, but the HTTP handler response/early-storage-call invariant is not actively gated. EVIDENCE: `chat_files_poll_test.go:589-591` and `:689-691` both call `t.Skipf` with the same structural precondition. Log excerpt: "Re-enable when body cap > per-file cap." Existing issue #1763 covers broad skipped-test drift; this adds the exact poll-upload cap-coupling subcase. I did not read or emit any secret values. RECOMMENDED FIX SHAPE: In molecule-core, either adjust the handler-level fixture to exercise the per-file 413 path under the current cap relationship (for example a multipart/body shape that stays under body cap while proving a file-level reject) or explicitly move this invariant to a real-infra integration gate and remove the dormant handler-level skip. Responsible files: `workspace-server/internal/handlers/chat_files_poll_test.go`, `workspace-server/internal/handlers/chat_files.go`, and pending-upload integration tests if the invariant is moved.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#1763
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?