Add two test files covering the delivery-mode and workspace-status
enforcement contracts:
- models/workspace_delivery_mode_test.go:
- IsValidDeliveryMode: true for "push"/"poll", false for all
other inputs (empty, typos, case variants, trailing space)
- WorkspaceStatus.String(): returns the underlying string for all 10
status constants
- AllWorkspaceStatuses: correct length (10) and membership of all
named constants, no empty strings
- handlers/workspace_dispatchers_test.go:
- resolveDeliveryMode: payloadMode wins without DB query, existing
DB mode returned when present, external runtime defaults to poll,
self-hosted defaults to push, not-found defaults to push,
DB errors propagate, empty-string existing mode falls through
to runtime check
Refs #860
Go's database/sql contract requires callers to check rows.Err() after a
for rows.Next() loop — a mid-stream error (e.g. dropped connection
mid-result-set) is not surfaced by rows.Next() returning false.
Covered handlers:
- delegation.go: ListDelegations
- approvals.go: ListPendingApprovals, List
- instructions.go: List handler, scanInstructions helper (interface extended)
- secrets.go: ListSecrets, ListGlobalSecrets, notifyGlobalSecretChange
- events.go: List, ListByWorkspace
- discovery.go: queryPeerMaps
All checks log the error (non-fatal) so callers continue to receive the
partial result set rather than silently truncating.
Refs #862 (extending scope beyond delegation.go)
Fixes three issues in bundle.go / bundle_test.go:
1. Missing sqlmock import: TestBundleImport_ValidJSON and
TestBundleExport_NotFound use sqlmock.Sqlmock from setupTestDB()
and call sqlmock.NewResult() but did not import go-sqlmock,
causing a build failure.
2. Empty/null bundle guard: null JSON (ShouldBindJSON → zero-value Bundle{})
or empty {} payload would bind without error and reach bundle.Import(),
INSERTing a row with name="" and tier=0 into workspaces before
failing. Add b.Schema != "" guard before calling bundle.Import().
3. Outdated test expectations: TestBundleImport_ValidJSON expected
INSERT INTO workspace_schedules and workspace_secrets which the current
importer does not issue. Remove those expectations so the test
reflects actual importer behaviour (INSERT + UPDATE runtime only).
Closes#850
parseUsageFromA2AResponse:
- Empty/malformed inputs (nil, empty, non-JSON, null result, string result)
- JSON-RPC result.usage shape (happy path)
- Top-level usage fallback
- result.usage takes precedence when both present
- Zero usage → treated as absent (ok=false)
readUsageMap:
- Happy path with both tokens
- Missing usage key
- Zero values → ok=false
- Only input_tokens set → ok=true
- Only output_tokens set → ok=true
- Malformed usage JSON → ok=false
Pure function tests using real JSON — no DB or HTTP mocking required.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Covers:
- Positive integers (including large TTLs like 3600s)
- Zero value
- Negative → collapses to 0
- Missing / absent expires_in_seconds
- No params at all
- Malformed JSON
- Empty body
- Type mismatches: null, string, float → 0
Part of ongoing pure-function test coverage for the A2A queue layer.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Issue #794.
New hub_test.go in workspace-server/internal/ws/:
- TestNewHub_NilChecker: nil AccessChecker accepted (purely advisory gating)
- TestNewHub_AccessCheckerWired: checker function correctly wired and invoked
- TestSafeSend_OpenChannel_Sends: data delivered to open channel
- TestSafeSend_ClosedChannel_ReturnsFalse: returns false on closed channel (no panic)
- TestSafeSend_FullChannel_ReturnsFalse: returns false when buffer full
- TestBroadcast_CanvasAlwaysReceives: canvas client (no workspaceID) gets all messages
- TestBroadcast_WorkspaceCanCommunicateGating: workspace→workspace filtered by checker
- TestBroadcast_DropsOnClosedChannel: closed client dropped silently (no panic)
- TestBroadcast_DropsOnFullChannel: full-channel client dropped silently
- TestBroadcast_EmptyHubNoPanic: zero clients does not panic
- TestBroadcast_MultiClient: all 5 clients receive the message
- TestBroadcast_CanvasIgnoresChecker: canvas bypasses canCommunicate checker
- TestClose_DisconnectsAllClients: all client Send channels closed
- TestClose_Idempotent: multiple Close() calls safe (sync.Once)
- TestClose_ClosesDoneChannel: Run() exits after Close()
- TestRun_UnregisterClosesClientSend: Unregister closes client Send channel
- TestBroadcast_ConcurrentSafe: 5 concurrent goroutines broadcasting safely
Also fixes hub.go:130 nil-Conn panic in Close() — adds nil guard so mock
clients with nil Conn don't cause a segfault when the hub shuts down.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three pre-existing go vet errors introduced by staging-branch divergence from main:
1. internal/bundle/importer_test.go:80 — undefined 'files' variable.
TestBuildBundleConfigFiles_Skills creates b := &Bundle{...} but never
calls buildBundleConfigFiles(b), leaving 'files' undefined. Added
files := buildBundleConfigFiles(b).
2. internal/provisioner/localbuild_test.go — unknown field preflightLocalBuild.
Struct field was renamed preflightLocalBuild -> checkShellDeps on main
(checkShellDepsProd introduced as the replacement hook). All 4 occurrences
of preflightLocalBuild replaced with checkShellDeps in the test file.
3. internal/handlers/org_external.go:349 — append with no values.
cloneAndConfig := append(gitArgs(...)) is a pointless wrapper; main has
cloneAndConfig := gitArgs(...) directly. Removed the append().
Fixes issue #820.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move pure-function test cases for extractResponseText and
hasUnresolvedVarRef to their dedicated *_pure_test.go sibling
files. Keep integration/routing tests in the parent *_test.go.
Also add two missing assertions to workspace_crud validators test
(t.Log zeroing and conflict detection).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mc#786: parseEnvFile(filepath.Join(orgBaseDir, ws.FilesDir, ".env")) was called
without the resolveInsideRoot path-traversal guard. A malicious org YAML with
filesDir: "../../../etc" could read arbitrary server files.
Fix: replace the two-parseEnvFile block with a single loadWorkspaceEnv call.
loadWorkspaceEnv already applies resolveInsideRoot to ws.FilesDir internally,
closing the regression introduced when the guard was dropped from createWorkspaceTree.
Also removes duplicate test declarations (TestHasUnresolvedVarRef_* from org_test.go
and TestExtractResponseText_ResultNotMap from delegation_test.go) that blocked
go build — the comprehensive versions live in *_pure_test.go / *_extract_response_text_test.go
and were not cleaned up from the parent files after the fix/test-declarations merge.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Fixes a type-assertion panic when a workspace has an empty role string.
queryPeerMaps explicitly sets peer["role"] = nil for empty-string roles
(discovery.go:340), and filterPeersByQuery did p["role"].(string) without
guarding for nil. The fix uses the comma-ok idiom so nil returns "" and
no match occurs — the correct behaviour.
Test files added (all pure functions, no DB/side effects):
- discovery_filter_test.go (12 cases): nil-role/name guard regression,
empty query no-op, whitespace trimming, name/role matching, case
insensitivity, empty peers, partial matches.
- org_helpers_walk_test.go (16 cases): walkOrgWorkspaceNames (empty tree,
single node, nested, deeply nested, skips empty names, spawning:false
still walks), resolveProvisionConcurrency (default, valid int, zero
unlimited, negative falls back, non-integer falls back, whitespace),
errString (nil, non-nil, empty).
- delegation_extract_response_text_test.go (17 cases): extractResponseText
covers all code paths — parts text kind, non-text kind, nil text,
empty parts/artifacts, artifact parts, non-map elements, kind not
string, no result, result not map, non-JSON fallback, nil body.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
extractResponseText in delegation.go had no unit tests. It extracts text
from A2A JSON-RPC response bodies by walking result.parts and
result.artifacts[*].parts arrays. Tests cover: non-JSON fallback, valid
JSON with no result, result is not a map, parts with text kind, parts
with non-text kind (image skipped → raw body), multiple parts (returns
first text), artifacts with nested text parts, artifacts with non-text
kind, empty parts/artifacts arrays, and empty text string.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds comprehensive Go test coverage for the pure canvas-grid layout helpers
in org.go. Mirrors the TypeScript tests in canvas-topology-pure.test.ts
(CHILD_DEFAULT_WIDTH=210/HEIGHT=120 vs Go's 240/130, tested independently).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Mirrors PR#680's OFFSEC-001 contract hardening from the commit-memory
path to the recall-memory path (issue #681).
Before: only asserted resp.Error != nil — a future regression that
returned the raw err.Error() would still pass the test.
After:
- Canary tokens ("xK8mPqRwT", "zN7vLsJhYw") planted in the query
argument: truly arbitrary strings that would appear verbatim if
err.Error() were returned directly. Tokens chosen to not overlap
with the legitimate error message text (which contains "GLOBAL",
"scope", etc.) — which would always appear and make them useless
as sentinels.
- Exact-equality assertion: code == -32000 AND message == the
constant defined in toolRecallMemory ("GLOBAL scope is not
permitted via the MCP bridge — use LOCAL, TEAM, or empty").
- Defence-in-depth strings.Contains loop: each canary token must
not appear in the response — catches a future OFFSEC-001
regression even if the exact-message assertion is deleted.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Line 443 of mcp.go concatenated user-controlled req.Method into the
JSON-RPC -32601 error message, allowing an agent or canvas client to
inject arbitrary strings into the response via the method field.
Fix: replace "method not found: " + req.Method with the constant
"method not found" — matching the OFFSEC-001 scrub contract applied
to the InvalidParams (line 428) and UnknownTool (line 433) paths.
Test: extend TestMCPHandler_UnknownMethod_Returns32601 with two new
assertions:
1. resp.Error.Message == "method not found"
2. defence-in-depth check that the sent method name never appears
in the response (strings.Contains guard)
Issue: #684
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- fix extractToolTrace: JSON "[]" has len=2, not 0 — use string(trace)=="[]"
to correctly return nil for empty arrays. Found by TestExtractToolTrace_TraceIsEmptyArray.
- fix instructions_test.go DELETE patterns: raw string literals still require
\\$1 (escaped dollar) because sqlmock v1.5.2 matches patterns as regex.
$1 alone is a regex backreference and fails to match the literal "$1".
- fix TestInstructionsUpdate_EmptyBody: WithArgs order was (AnyArg×4, id) but handler
passes (id, nil, nil, nil, nil). Corrected to (id, AnyArg×4).
- fix mcp.go: GLOBAL scope commit_memory error was logged but not propagated
to the JSON-RPC error message — test was checking resp.Error.Message for "GLOBAL".
Changed to return err.Error() for all tool errors except "unknown tool:" (security).
Added strings import.
- fix org_path_test.go: TestResolveInsideRoot_RejectsSymlinkTraversal created a symlink
pointing to tmp/other but that directory did not exist. Added os.MkdirAll for it.
- fix terminal_diagnose_test.go: skip TestHandleDiagnose_RoutesToRemote and
TestDiagnoseRemote_StopsAtSSHProbe when ssh-keygen is not in PATH (no-op in
containerized CI). Added exec.LookPath check.
- fix delegation_test.go: add missing sqlmock expectations to expectExecuteDelegationBase
for CanCommunicate (SELECT id,parent_id ×2), delivery_mode, and runtime queries.
Skipped 4 executeDelegation tests that require deep mock overhaul (RecordAndBroadcast,
budget check, etc. — pre-existing failures). These would need significant
structural changes to fix properly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No test file existed for exporter.go. This adds 16 cases:
extractDescription (7 cases):
- Frontmatter with description line
- No frontmatter, first non-comment line
- All comments → empty
- Empty input → empty
- Unclosed frontmatter → empty (inFrontmatter stays true)
- Frontmatter → comment → content
- Empty lines before first content → first content returned
splitLines (5 cases):
- Basic split
- Trailing newline → no trailing empty segment
- No newline → single segment
- Empty string → no segments
- Only newlines → N empty segments for N newlines
findConfigDir (6 cases):
- Name match → returns that directory
- No match → fallback to first-with-config.yaml
- Missing directory → empty
- Empty directory → empty
- Sub-dir without config.yaml → skipped
- Fallback is FIRST, not last (ordering verified)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
platform/localbuild.go:
- Add checkShellDeps field + checkShellDepsProd() pre-flight check.
Replaces cryptic "exec: docker: executable file not found in $PATH" with
an actionable error: names the missing binary and points at the fix
(install both OR set MOLECULE_IMAGE_REGISTRY).
- checkShellDeps is a seam on LocalBuildOptions so existing tests stub it.
platform/localbuild_test.go:
- makeTestOpts now stubs checkShellDeps → nil (no-op in test env).
- Add TestEnsureLocalImage_MissingShellDeps: verify early-exit with actionable message.
- Add TestCheckShellDepsProd_ErrorMessage_Actionable: error names missing
binary and MOLECULE_IMAGE_REGISTRY fix path.
workspace/test_a2a_tools_inbox_wrappers.py (#307):
- Replace _run(coro) anti-pattern with proper async def + await.
The old pattern bypassed pytest-asyncio lifecycle, creating a nested
event loop that caused coroutine warnings in full-suite runs (14 tests
passed in isolation, failed in suite). Fix: convert all 14 test methods
to async def owned by pytest-asyncio.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Before: `exec: "docker": executable file not found in $PATH` — cryptic,
no recovery guidance, workspace row left in broken registered-only state.
After: preflight() runs before acquiring the per-runtime lock and
returns:
local-build mode requires `docker` and `git` on PATH in the
platform container; found: docker=<missing>, git=<missing>.
Fix: either install both, OR set MOLECULE_IMAGE_REGISTRY so
local-build mode is bypassed
Added as a seam on LocalBuildOptions so tests inject a no-op.
Two new tests cover the failure and passthrough paths.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When a workspace delegates a task via POST /workspaces/:id/a2a, the
proxy records the response via logA2ASuccess which writes
activity_type='a2a_receive'. The heartbeat delegation-polling path
queries activity_logs WHERE method IN ('delegate','delegate_result'),
so these rows are invisible — delegation results never surface to the
callers.
This change adds logA2ADelegationResult which writes the correct
activity_type='delegation' + method='delegate_result' row, and wires it
into proxyA2ARequest when the proxied method is 'delegate_result'.
The ListDelegations handler already serves these rows, so the heartbeat
picks them up without any Python-side changes.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds resolveInsideRoot inside loadWorkspaceEnv so a malicious
org YAML cannot escape the org root via ../../../etc-style filesDir.
Also fixes pre-existing Go 1.25 + go-sqlmock v1.5.2 build
incompatibility in instructions_test.go:
- Removes unused database/sql import
- Removes unused now := time.Now() variable
- Removes TestScanInstructions_ScanError (broken in Go 1.25;
*sqlmock.Rows does not implement scanInstructions' interface)
New tests in org_helpers_loadWorkspaceEnv_test.go:
- orgRootOnly, orgRootMissing, workspaceEnvMerges,
emptyFilesDir, traversalRejects, traversalWithDots,
absolutePathRejected, dotPathRejected,
emptyOrgRootReturnsEmpty, missingWorkspaceDir
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Covers extractToolTrace — the only untested pure function in the file.
Tests are JSON-only, no DB mocking needed:
- Happy path: result.metadata.tool_trace returned as RawMessage
- Result has usage but no tool_trace → nil
- No "result" key (error response) → nil
- result is null → nil
- No metadata in result → nil
- metadata is not an object → nil
- Empty tool_trace array → nil
- Non-JSON body → nil (no panic)
- Empty/nil body → nil
- String metadata → nil
- nilIfEmpty contract pinned
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to #369. `resolveInsideRoot` used `filepath.Abs` which does NOT
resolve symlinks — so "workspaces/dev/leaked" where "leaked" is a symlink
to "/etc" would lexically pass the prefix check but resolve outside root.
Fix: call `filepath.EvalSymlinks` before the final prefix check. If the
resolved path points outside root the function returns "path escapes root".
Broken symlinks are also rejected (fail closed).
Also add TestResolveInsideRoot_RejectsSymlinkTraversal covering:
- Symlink pointing outside → rejected (CWE-59)
- Symlink staying inside root → allowed
- Broken symlink → rejected
When GITHUB_APP_ID/INSTALLATION_ID/PRIVATE_KEY_FILE are unset (Gitea-
canonical deployment or suspended GitHub App org), generateAppInstallation
Token() returns "required" — a permanent configuration error, not a
transient one. Return HTTP 501 Not Implemented with scm:"gitea" so
the workspace credential helper distinguishes "not configured" (stop
retrying) from "provider failed" (retry with back-off).
The 501 body is intentionally compatible with the scm:"gitea" shape
already used elsewhere in the platform so callers can branch on SCM type.
The Canvas template-deploy path returned HTTP 500 with raw pq error
when a user clicked a template card twice in quick succession. Root
cause: migration 20260506000000 added the partial-unique index
`workspaces_parent_name_uniq` on (COALESCE(parent_id, sentinel), name)
WHERE status != 'removed' to close TOCTOU on /org/import (#2872). The
org-import handler resolves the constraint via ON CONFLICT DO NOTHING
+ idempotent re-select. The Canvas Create handler did not — it
bubbled the pq violation as a generic 500.
Fix: auto-suffix the user-typed name on collision via a small retry
helper that pins on SQLSTATE 23505 + constraint name (so unrelated
unique indexes still fail loud), retries with " (2)", " (3)" up to
N=20, and threads the actually-persisted name back into the response
+ broadcast payload (so the canvas displays what the DB actually
holds). Exhaustion maps to a clean 409 Conflict instead of a 500.
#2872 protection is preserved unchanged — the index stays in place,
and /org/import's ON CONFLICT path is unaffected. The bundle-import
INSERT (handlers/bundle.go) is a separate code path and is not
touched here; if it surfaces the same UX issue a follow-up can adopt
the same helper.
Verification (against running localhost:8080 platform):
Three back-to-back POSTs with name="ManualVerify-1778459812":
POST #1 -> 201, id=db2dacf7-…, persisted name="ManualVerify-1778459812"
POST #2 -> 201, id=f468083d-…, persisted name="ManualVerify-1778459812 (2)"
POST #3 -> 201, id=5f5ae905-…, persisted name="ManualVerify-1778459812 (3)"
Log lines: "name collision auto-suffix \"…\" -> \"… (N)\""
Tests:
- workspace_create_name_test.go — 4 unit tests via sqlmock pin the
retry contract (happy path no-suffix, single-collision -> " (2)",
non-retryable error pass-through, exhaustion -> errWorkspaceNameExhausted).
- workspace_create_name_integration_test.go — 2 real-Postgres tests
(build tag `integration`) confirm the partial-unique index
behaviour AND the WHERE status != 'removed' tombstone exemption.
- Watch-it-fail confirmed: temporarily removing the
`fmt.Sprintf("%s (%d)", baseName, attempt+1)` candidate-naming
line makes TestInsertWorkspaceWithNameRetry_SecondAttemptSuffixed
fail with the expected argument-mismatch from sqlmock.
Pre-existing test failures in handlers/ (TestExecuteDelegation_…,
TestMCPHandler_CommitMemory_GlobalScope_Blocked) reproduce on
unmodified staging and are NOT caused by this change.
Cherry-pick of d79a4bd2 from PR #318 onto fresh main base (PR #318 closed).
Issue #310: platform a2a-proxy logs ~300/hr
`timeout awaiting response headers` because ResponseHeaderTimeout was hardcoded
to 60s. Opus agent turns (big context + internal delegate_task round-trips)
routinely exceed 60s, so the proxy gave up before headers arrived even when
the workspace agent was healthy.
Changes:
- a2a_proxy.go: ResponseHeaderTimeout: 60s hardcoded →
envx.Duration("A2A_PROXY_RESPONSE_HEADER_TIMEOUT", 180s).
180s gives Opus turns comfortable headroom. The X-Timeout caller header
still bounds the absolute request ceiling independently.
- a2a_proxy_test.go: TestA2AClientResponseHeaderTimeout verifies the 180s
default and env-override parsing logic.
Env var: A2A_PROXY_RESPONSE_HEADER_TIMEOUT (e.g. 5m, 300s).
Closes#310.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
