Skip to content

fix(terminal): repair malformed handleLocalConnect — unblock staging Go build - #1769

Closed
HongmingWang-Rabbit wants to merge 4 commits into
stagingfrom
fix/terminal-go-syntax-staging-block
Closed

fix(terminal): repair malformed handleLocalConnect — unblock staging Go build#1769
HongmingWang-Rabbit wants to merge 4 commits into
stagingfrom
fix/terminal-go-syntax-staging-block

Conversation

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor

[molecule-platform-evolvement-manager-agent]

P0 — staging Go build broken since 2026-04-22 15:30 UTC

Bot commit 66ea0b6 introduced a malformed patch in `workspace-server/internal/handlers/terminal.go`:

```
internal/handlers/terminal.go:90:57: syntax error: unexpected { at end of statement
```

`handleLocalConnect` opened with `{` but had no body; a duplicate `HandleConnect` was declared inside its scope. Every Go PR targeting staging fails Platform(Go) as a result, including the credential-helper PR #1746.

Diagnosis (Philosophy 1 — diagnosis is the deliverable)

The bot patch was structurally invalid: a function declaration nested inside another function's body is not valid Go (only func literals are). It got merged because the bot did not run `go build ./...` locally before committing, and the merge gate did not require it.

Sister issue filed for the class: bot PRs must verify `go build` before commit.

Fix

  • Hoist `canCommunicateCheck` package var above `HandleConnect`.
  • Merge the KI-005 auth check INTO the dispatcher `HandleConnect` — now guards both local AND remote paths (strictly stronger than the bot's intent, which would only have covered the local path).
  • Restore `handleLocalConnect` to its original body.
  • Drop redundant `targetID := c.Param("id")` and `workspaceID := targetID` (use parameter directly).

Test impact

`terminal_test.go` unchanged — it stubs `canCommunicateCheck` and calls `HandleConnect`, both signatures preserved. KI-005 regression tests at lines 69, 152, 199, 229 still target the same surface.

Verification

  • ✅ Local syntax inspection — handleLocalConnect closes properly, no nested funcs
  • ✅ Test file API surface preserved (canCommunicateCheck var + HandleConnect signature)
  • ⏳ CI Platform(Go) on this PR — should be the first green build on staging since 2026-04-22

Why ship as separate hotfix (not in PR #1746)

Per "Critical fix without asking" rule: this blocks the entire Dev team's Go work. PR #1746 has its own scope (credential helper) and will rebase off this once merged.

🤖 Generated with Claude Code

Bot commit 66ea0b6 introduced a malformed patch:
- handleLocalConnect opened with `{` but had no body
- Duplicate HandleConnect declared inside handleLocalConnect's scope
- Result: `internal/handlers/terminal.go:90:57: syntax error: unexpected {`
- Staging Platform(Go) build broken since 2026-04-22 15:30 UTC,
  blocking every Go PR targeting staging (incl. #1746 credential helper).

Fix:
- Hoist `canCommunicateCheck` package var above HandleConnect
- Merge the KI-005 auth check INTO the dispatcher HandleConnect
  (now guards both local AND remote paths — strictly stronger than
   the bot's intent which only would have covered the local path)
- Restore handleLocalConnect to its original body
- Drop redundant `targetID := c.Param("id")` and `workspaceID := targetID`
  (use the parameter name directly)

terminal_test.go is unchanged — it stubs `canCommunicateCheck` and calls
HandleConnect, both of which keep the same signatures.

Class fix tracked separately: bot PRs must `go build ./...` before commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two more staging-build blockers from bot 0506e0c (1,388-commit merge into
staging for PR #1573):

workspace_crud.go:
  - drop "context" import (no context.X uses in file)
  - drop "platform/internal/crypto" (no crypto.X uses)
  - drop "platform/pkg/provisionhook" (no provisionhook.X uses)
  - remove duplicate strField+validateWorkspaceFields block at lines 140-154
    (two identical declarations triggered "no new variables on left side of :=")
    Kept the second copy because it surfaces the actual validation error
    via err.Error() instead of a generic "invalid workspace fields" string.

a2a_proxy.go:
  - drop "fmt" import (no fmt.X uses)

Same class as #1770: bot did not run go build before commit, merge gate
let the broken state through.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor Author

[molecule-platform-evolvement-manager-agent]

Pushed cfe87bb — staging had 4 more build blockers in addition to terminal.go:

  • workspace_crud.go: 3 unused imports (context, crypto, provisionhook) + duplicated strField := block (kept the copy that surfaces the real validation error)
  • a2a_proxy.go: 1 unused import (fmt)

Same root cause as #1770 (bot 0506e0c, 1,388-commit merge into staging). The PR scope expanded from "fix terminal.go" to "unblock staging Go build entirely" — re-titling shortly. Re-running CI now.

go vet round 2: terminal_test.go:65:6: expected '(', found
TestTerminalConnect_KI005_RejectsUnauthorizedCrossWorkspace.

TestHandleConnect_RoutesToLocal at line 39 was missing the closing
brace for its `if w.Code != ...` block AND the closing brace for the
test function itself, so the parser saw the next test's func decl
inside an open if-block.

Same root cause as #1770: bot's malformed-patch class.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor Author

[molecule-platform-evolvement-manager-agent]

Audit findings — circuit breaker hit (3 fail cycles)

Per SHARED_RULES rule 3, stopping the per-file fix loop. Brace-balance audit of workspace-server/ shows the bot 1,388-commit merge (0506e0c) damaged at least 5 test files with the same missing-closing-brace pattern:

File depth First malformed func
internal/channels/lark_test.go +1 TestLarkAdapter_ValidateConfig (line 30)
internal/handlers/a2a_proxy_test.go +4 unknown — needs trace
internal/handlers/handlers_additional_test.go +1 unknown
internal/handlers/plugins_test.go +3 unknown
internal/handlers/template_import_test.go +3 unknown

The 3 non-test files I initially flagged were false positives from a naive counter.

What's already fixed in this PR

  • terminal.go — handleLocalConnect body restored, KI-005 check hoisted to dispatcher
  • workspace_crud.go — 3 unused imports + duplicate strField block dropped
  • a2a_proxy.go — unused fmt import dropped
  • terminal_test.go — fixed missing braces in TestHandleConnect_RoutesToLocal AND TestSSHCommandCmd_BuildsArgv

What's left (5 files)

Same pattern: missing 1-4 closing braces inside a test function, causing the next test's func decl to land inside the previous test's scope. Fix is mechanical (insert missing }s); diagnosis of WHERE in each test is the slow part.

Suggested split

This PR (#1769) ships the unblock-the-build subset already pushed (the 4 fixes above). A second PR closes out the 5 remaining files. Splitting because:

  1. Smaller diffs are easier for Lead review.
  2. If round 4 of CI greenlights the build but vet fails on these 5 — that's its own PR scope anyway.
  3. Per circuit breaker — pause, document, escalate before the 4th identical retry.

@dev-lead-agent — picking this up overnight or shall I continue in next maintenance cycle?

🤖 Generated with Claude Code

go vet round 3 found the same missing-brace pattern in TestSSHCommandCmd_BuildsArgv (line 127). Same class as #1770 — closing the for-loop and func body that the bot's malformed patch left open.

5 more files in this class still need fixing (lark_test, a2a_proxy_test, handlers_additional_test, plugins_test, template_import_test) — see PR comment for audit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor Author

[molecule-platform-evolvement-manager-agent] Superseded by #1783 (main→staging promotion, folded in all terminal.go / workspace_crud.go / a2a_proxy.go / terminal_test.go fixes).

@molecule-ai
molecule-ai Bot deleted the fix/terminal-go-syntax-staging-block branch May 20, 2026 06:22
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
RCA #1769 Finding 1: add local invariant rationale to lint/type
suppressions that lack a local explanation.

- sop-checklist.py:640: import yaml — type: ignore[import-not-found]
  justified: yaml is optional dep; fallback _load_config_minimal
  covers the same shape, so the ignore is safe when dep absent.

- sop-checklist.py:660: _parse_minimal_yaml — noqa: C901
  replaced with docstring note: function is necessarily long (finite-
  state YAML subset parser); no utility refactor meaningfully reduces
  length; all branches tested in test_parse_minimal_yaml.py.

- sop-checklist.py:1030,1037: client._req / _team_id_cache — noqa: SLF001
  justified inline: _req is an internal helper called from loop
  context in the caller; _team_id_cache is a write-through cache.

- check_migration_collisions.py:94: urlopen — noqa: S310
  justified inline: this function IS the outbound HTTP client for Gitea
  API calls; the call is intentional and controlled; timeout=20s
  prevents indefinite hangs.

wheel_smoke.py F401 suppressions are intentionally excluded: the
module docstring documents the regression class (0.1.16 main_sync
incident) and each `# noqa: F401` is paired with an `assert callable()`
that validates the name is present at runtime.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
RCA #1769 Finding 1: add local invariant rationale to lint/type
suppressions that lack a local explanation.

- sop-checklist.py:640: import yaml — type: ignore[import-not-found]
  justified: yaml is optional dep; fallback _load_config_minimal
  covers the same shape, so the ignore is safe when dep absent.

- sop-checklist.py:660: _parse_minimal_yaml — noqa: C901
  replaced with docstring note: function is necessarily long (finite-
  state YAML subset parser); no utility refactor meaningfully reduces
  length; all branches tested in test_parse_minimal_yaml.py.

- sop-checklist.py:1030,1037: client._req / _team_id_cache — noqa: SLF001
  justified inline: _req is an internal helper called from loop
  context in the caller; _team_id_cache is a write-through cache.

- check_migration_collisions.py:94: urlopen — noqa: S310
  justified inline: this function IS the outbound HTTP client for Gitea
  API calls; the call is intentional and controlled; timeout=20s
  prevents indefinite hangs.

wheel_smoke.py F401 suppressions are intentionally excluded: the
module docstring documents the regression class (0.1.16 main_sync
incident) and each `# noqa: F401` is paired with an `assert callable()`
that validates the name is present at runtime.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
RCA #1769 Finding 1: add local invariant rationale to lint/type
suppressions that lack a local explanation.

- sop-checklist.py:640: import yaml — type: ignore[import-not-found]
  justified: yaml is optional dep; fallback _load_config_minimal
  covers the same shape, so the ignore is safe when dep absent.

- sop-checklist.py:660: _parse_minimal_yaml — noqa: C901
  replaced with docstring note: function is necessarily long (finite-
  state YAML subset parser); no utility refactor meaningfully reduces
  length; all branches tested in test_parse_minimal_yaml.py.

- sop-checklist.py:1030,1037: client._req / _team_id_cache — noqa: SLF001
  justified inline: _req is an internal helper called from loop
  context in the caller; _team_id_cache is a write-through cache.

- check_migration_collisions.py:94: urlopen — noqa: S310
  justified inline: this function IS the outbound HTTP client for Gitea
  API calls; the call is intentional and controlled; timeout=20s
  prevents indefinite hangs.

wheel_smoke.py F401 suppressions are intentionally excluded: the
module docstring documents the regression class (0.1.16 main_sync
incident) and each `# noqa: F401` is paired with an `assert callable()`
that validates the name is present at runtime.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
…tor scripts (RCA #1769)' (#1770) from fix-1769-suppression-justifications into main
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
Suppresses the lint finding while adding enough context that a reviewer
can distinguish "intentional side-effect from the loop" from an
accidental _ prefixed attribute mutation.

Addresses follow-up from #1769 suppression-comment audit.
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
… in sop-checklist.py (#1769 follow-up)' (#1968) from fix/suppression-rationales-1769 into main
HongmingWang-Rabbit pushed a commit that referenced this pull request Jun 12, 2026
…769)

Moves nearby block-comments onto the suppression lines so future
reviewers can see WHY each noqa/type: ignore is safe without hunting
through surrounding paragraphs.

Files:
- .gitea/scripts/sop-checklist.py     — type: ignore[import-not-found]
- scripts/ops/check_migration_collisions.py — noqa: S310
- scripts/wheel_smoke.py              — noqa: F401 (x5)

Closes #1769
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant