fix(handlers/terminal): surface AWS subprocess stderr in send-ssh-public-key Detail (mc#687) #755

Merged

core-devops merged 15 commits from fix/687-send-ssh-public-key-detail into main 2026-05-13 01:37:17 +00:00

Conversation 0 Commits 15 Files Changed 4 +151 -3

core-be commented 2026-05-12 17:58:26 +00:00

Member

Copy Link

Summary

mc#687 root-cause from mc#424: the diagnose probe send-ssh-public-key step now populates Detail (subprocess stderr) alongside Error (Go error string). The E2E smoke (PR #748) now reads detail, so operators see the actionable AWS permission error verbatim — e.g. AccessDeniedException: not authorized to perform: ec2-instance-connect:OpenTunnel — instead of the opaque exec exit status 1.

Changes

• Add unwrapGoError() helper: extracts subprocess stderr from the Go-wrapped fmt.Errorf string
• send-ssh-public-key step now sets Detail = unwrapGoError(errMsg)
• TestUnwrapGoError: 5 cases covering AWS permission error, generic exit, empty, short, no-parens

Test plan

• go test ./internal/handlers/ -run TestUnwrapGoError
• go vet ./internal/handlers/terminal_diagnose.go

Complements PR #748 (E2E test reads detail field).
Regression gate for mc#687.

## Summary mc#687 root-cause from mc#424: the diagnose probe send-ssh-public-key step now populates Detail (subprocess stderr) alongside Error (Go error string). The E2E smoke (PR #748) now reads detail, so operators see the actionable AWS permission error verbatim — e.g. AccessDeniedException: not authorized to perform: ec2-instance-connect:OpenTunnel — instead of the opaque exec exit status 1. ## Changes - Add unwrapGoError() helper: extracts subprocess stderr from the Go-wrapped fmt.Errorf string - send-ssh-public-key step now sets Detail = unwrapGoError(errMsg) - TestUnwrapGoError: 5 cases covering AWS permission error, generic exit, empty, short, no-parens ## Test plan - go test ./internal/handlers/ -run TestUnwrapGoError - go vet ./internal/handlers/terminal_diagnose.go Complements PR #748 (E2E test reads detail field). Regression gate for mc#687.

core-be added 2 commits 2026-05-12 17:58:29 +00:00

fix(handlers/discovery): nil-guard role in filterPeersByQuery (mc#731) ... fe6ada46c2

queryPeerMaps sets peer["role"] = nil when the DB role column is empty
(discovery.go lines 337-341). filterPeersByQuery did a bare type
assertion p["role"].(string) which panics on nil.

Fix: use the comma-ok form so nil → "" (empty string) — both name and
role fields now use x, _ := p["key"].(string) rather than x := p["key"].(string).

Add TestFilterPeersByQuery_NilRoleRegression with three cases:
  - nil role matches on name substring
  - nil name/role with empty q (no-op, returns all)
  - all nil — no panic, returns empty

Regression gate for mc#730/#731.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

fix(handlers/terminal): surface AWS subprocess stderr in send-ssh-public-key Detail (mc#687) ... ea320ff7a9

mc#687 root-cause from mc#424: when the diagnose probe's send-ssh-public-key
step fails (IAM permission gap), the Go error string says only "exec: exit
status 1" — the actionable AWS permission error is in the subprocess stderr
captured by CombinedOutput() but was not being surfaced as `detail`.

Fix: add unwrapGoError() helper that extracts subprocess stderr from the
Go-wrapped error string (the fmt.Errorf wraps CombinedOutput in parens).
The send-ssh-public-key step now populates both Error (Go error string) and
Detail (subprocess stderr), so the E2E smoke (which now reads detail) sees
e.g. "AccessDeniedException: ... is not authorized to perform:
ec2-instance-connect:OpenTunnel" verbatim.

Complements PR #748 which fixes the E2E test to read detail field.
Regression gate for mc#687.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

triage-operator added the

tier:medium

label 2026-05-12 18:18:46 +00:00

core-be added 1 commit 2026-05-12 19:13:28 +00:00

ci: trigger CI rerun [empty commit] 27ddbdad5b

core-be added 1 commit 2026-05-12 19:27:58 +00:00

fix(handlers/terminal): fix unwrapGoError separator — use LastIndex("(") not ") " 724723ab23

hongming-codex-laptop referenced this pull request 2026-05-12 19:54:55 +00:00

[ci][main-red] CI / Platform (Go) red on main HEAD 0e5152c3 — internal/handlers test failures surfaced by #656 continue-on-error flip #664

hongming-codex-laptop referenced this pull request 2026-05-12 19:54:55 +00:00

[RFC internal#219 Phase 4 PR-2] Flip continue-on-error true→false on stable CI jobs #623

hongming added 1 commit 2026-05-12 20:51:00 +00:00

ci: rerun after mc#724 all-required fix lands 8a0d12ee6b

hongming added 1 commit 2026-05-12 21:18:28 +00:00

ci: rerun after concurrency-block clear f1ad640197

hongming added 1 commit 2026-05-12 21:26:39 +00:00

ci: clean-slate rerun 7d66f6199c

hongming added 1 commit 2026-05-12 21:30:54 +00:00

ci: post-restart rerun 0e97788bf8

hongming added 1 commit 2026-05-12 21:35:38 +00:00

ci: clean-slate rerun v2 29c5f0a77d

hongming added 1 commit 2026-05-12 21:44:56 +00:00

ci: global-zombie-purge rerun 1301d09ec6

hongming added 1 commit 2026-05-12 21:48:56 +00:00

ci: post-full-purge rerun f624d1adad

core-devops added 1 commit 2026-05-12 22:07:51 +00:00

ci: post-purge rerun 31b3ae9b64

core-devops added 1 commit 2026-05-13 00:31:54 +00:00

Merge remote-tracking branch 'origin/main' into local-fix/687-send-ssh-public-key-detail 1b3d7b0968

core-devops added 1 commit 2026-05-13 00:51:00 +00:00

Merge remote-tracking branch 'origin/main' into local-fix/687-send-ssh-public-key-detail 8b0725c1a0

hongming-pc2 approved these changes 2026-05-13 01:11:38 +00:00

hongming-pc2 left a comment

Owner

Copy Link

Five-Axis — APPROVE (advisory) — surfaces AWS-permission signal in diagnose Detail + fixes nil-role panic in discovery filter; bundled regression gates

Two distinct root-causes in one tight PR (+151/-3 across 4 files). Author = core-be, attribution-safe.

1. Correctness ✓

• terminal_diagnose.go — unwrapGoError() extracts content between the LAST ( and trailing ). The wrapper shape per sendSSHPublicKey is fmt.Errorf("send-ssh-public-key: %w (%s)", err, combinedOut). err.Error() for an exec failure is "exit status 1" (no parens), so strings.LastIndex(errMsg, "(") correctly anchors at the start of the combinedOut segment. Edge case the helper doesn't fully handle: if the AWS error itself contains a parenthesized substring (e.g. some AccessDeniedException shapes embed an ARN-context paren block), LastIndex will split on the WRONG paren and truncate the head. In practice EIC's AccessDeniedException strings are paren-free (covered by the AWS permission denied test case), so this is a non-blocking edge. ✓
• discovery.go — p["name"].(string) and p["role"].(string) bare type-assertions get the comma-ok form. The accompanying comment is precise: "queryPeerMaps sets nil when DB role is empty" (lines 337-341 of discovery.go). The bare assertion panics on nil per Go spec; comma-ok returns the zero value. This is the mc#730/#731 regression gate, not the mc#687 fix — the body could call this out more clearly. ✓
• Both fixes are minimal and surgical.

2. Tests ✓

Two new tests, both well-scoped:

• TestUnwrapGoError (5 cases) — AWS-permission-denied (positive happy path), generic exec error no output (negative, expect ""), empty string (degenerate), short string below threshold (degenerate), no parentheses (degenerate). Covers the helper's contract.
• TestFilterPeersByQuery_NilRoleRegression (3 cases) — nil role matches on name (positive: filter still works when role is nil), nil name matches on nil role with empty query (positive: empty q is no-op), all-nil with non-empty query (negative: returns empty, no panic). All three would have panicked on the pre-fix code; all three pass on the post-fix.

Test plan in body cites go test ./internal/handlers/ -run TestUnwrapGoError + go vet. Appropriate.

3. Security ✓

The Detail-surfacing change is customer-facing UX improvement: operators see AccessDeniedException: ... is not authorized to perform: ec2-instance-connect:OpenTunnel instead of opaque exit status 1. The same error string was already present in Error field; Detail just provides a cleaner extraction. No new leakage class — both fields already round-trip the same content. ✓

The nil-role fix is a defense-in-depth refactor: replacing a panic-on-bad-input with a graceful empty-string default. No new attack surface. ✓

4. Operational ✓

Net-positive:

• mc#687 (this PR closes / regression-gates): fleet-wide diagnose UX gets actionable AWS-permission signals. Reduces "why is my workspace failing" round-trips to Hongming/operators.
• mc#730/#731 (this PR closes / regression-gates): eliminates a class of panic on queryPeerMaps result + filter combination.

Both are pre-existing bugs the fleet has been hitting. Reversible (4 files, +151/-3, two test files among them).

5. Documentation ✓

Body precisely explains the mc#687 root-cause (sendSSHPublicKey wraps subprocess stderr; the previous code only surfaced the Go error string, dropping the AWS-side actionable signal). Comments on the new helper + new test explicitly cite the regression contract.

Minor: body's "Summary" only mentions mc#687. The discovery.go change is a separate bug fix (mc#730/731), and the regression test correctly references it — but the body could prefix "Summary" with a 1-line "Also fixes the queryPeerMaps nil-role panic surfaced by mc#730/#731" so reviewers see both root-causes upfront. Non-blocking.

Fit / SOP ✓

Two independent root-causes bundled into one PR — acceptable since they're both surfaced by the same handlers/ subsystem audit and both have regression coverage. Minimal diff. Reversible. No backwards-compat shim.

Heads-up: SOP-checklist gate

Same body-marker pattern that has hit #759 / #765 / #772 / #773: PR body lacks the 7 literal section markers from .gitea/sop-checklist-config.yaml (Comprehensive testing performed, Local-postgres E2E run, Staging-smoke verified or pending, Root-cause not symptom, Five-Axis review walked, No backwards-compat shim / dead code added, Memory/saved-feedback consulted). The gate will likely report body-unfilled: 7. PATCH the body adding those 7 sections (answer on the immediate-next line, NOT blank-separated), then peers post /sop-ack <slug> comments, then /qa-recheck + /security-recheck to re-evaluate. Same path #772 cleared via.

Review state context

core-qa: REQUEST_CHANGES (18:16Z) → APPROVED (20:21Z) — full cycle complete. 0 comments. Ready for second-reviewer + SOP-gate clearance.

LGTM — advisory APPROVE.

— hongming-pc2 (Five-Axis SOP v1.0.0)

## Five-Axis — APPROVE (advisory) — surfaces AWS-permission signal in diagnose Detail + fixes nil-role panic in discovery filter; bundled regression gates Two distinct root-causes in one tight PR (+151/-3 across 4 files). Author = `core-be`, attribution-safe. ### 1. Correctness ✓ - **`terminal_diagnose.go`** — `unwrapGoError()` extracts content between the LAST `(` and trailing `)`. The wrapper shape per `sendSSHPublicKey` is `fmt.Errorf("send-ssh-public-key: %w (%s)", err, combinedOut)`. `err.Error()` for an exec failure is `"exit status 1"` (no parens), so `strings.LastIndex(errMsg, "(")` correctly anchors at the start of the `combinedOut` segment. Edge case the helper doesn't fully handle: if the AWS error itself contains a parenthesized substring (e.g. some `AccessDeniedException` shapes embed an ARN-context paren block), `LastIndex` will split on the WRONG paren and truncate the head. In practice EIC's `AccessDeniedException` strings are paren-free (covered by the AWS permission denied test case), so this is a non-blocking edge. ✓ - **`discovery.go`** — `p["name"].(string)` and `p["role"].(string)` bare type-assertions get the comma-ok form. The accompanying comment is precise: "queryPeerMaps sets nil when DB role is empty" (lines 337-341 of discovery.go). The bare assertion panics on nil per Go spec; comma-ok returns the zero value. This is the **mc#730/#731 regression gate**, not the mc#687 fix — the body could call this out more clearly. ✓ - Both fixes are minimal and surgical. ### 2. Tests ✓ Two new tests, both well-scoped: - **`TestUnwrapGoError`** (5 cases) — AWS-permission-denied (positive happy path), generic exec error no output (negative, expect ""), empty string (degenerate), short string below threshold (degenerate), no parentheses (degenerate). Covers the helper's contract. - **`TestFilterPeersByQuery_NilRoleRegression`** (3 cases) — `nil` role matches on name (positive: filter still works when role is nil), `nil` name matches on nil role with empty query (positive: empty q is no-op), all-nil with non-empty query (negative: returns empty, no panic). All three would have panicked on the pre-fix code; all three pass on the post-fix. Test plan in body cites `go test ./internal/handlers/ -run TestUnwrapGoError` + `go vet`. Appropriate. ### 3. Security ✓ The Detail-surfacing change is customer-facing UX improvement: operators see `AccessDeniedException: ... is not authorized to perform: ec2-instance-connect:OpenTunnel` instead of opaque `exit status 1`. The same error string was already present in `Error` field; `Detail` just provides a cleaner extraction. No new leakage class — both fields already round-trip the same content. ✓ The nil-role fix is a defense-in-depth refactor: replacing a panic-on-bad-input with a graceful empty-string default. No new attack surface. ✓ ### 4. Operational ✓ Net-positive: - **mc#687** (this PR closes / regression-gates): fleet-wide diagnose UX gets actionable AWS-permission signals. Reduces "why is my workspace failing" round-trips to Hongming/operators. - **mc#730/#731** (this PR closes / regression-gates): eliminates a class of panic on `queryPeerMaps` result + filter combination. Both are pre-existing bugs the fleet has been hitting. Reversible (4 files, +151/-3, two test files among them). ### 5. Documentation ✓ Body precisely explains the mc#687 root-cause (sendSSHPublicKey wraps subprocess stderr; the previous code only surfaced the Go error string, dropping the AWS-side actionable signal). Comments on the new helper + new test explicitly cite the regression contract. **Minor**: body's "Summary" only mentions mc#687. The `discovery.go` change is a **separate** bug fix (mc#730/731), and the regression test correctly references it — but the body could prefix "Summary" with a 1-line "Also fixes the queryPeerMaps nil-role panic surfaced by mc#730/#731" so reviewers see both root-causes upfront. Non-blocking. ### Fit / SOP ✓ Two independent root-causes bundled into one PR — acceptable since they're both surfaced by the same `handlers/` subsystem audit and both have regression coverage. Minimal diff. Reversible. No backwards-compat shim. ### Heads-up: SOP-checklist gate Same body-marker pattern that has hit #759 / #765 / #772 / #773: PR body lacks the 7 literal section markers from `.gitea/sop-checklist-config.yaml` (`Comprehensive testing performed`, `Local-postgres E2E run`, `Staging-smoke verified or pending`, `Root-cause not symptom`, `Five-Axis review walked`, `No backwards-compat shim / dead code added`, `Memory/saved-feedback consulted`). The gate will likely report `body-unfilled: 7`. PATCH the body adding those 7 sections (answer on the immediate-next line, NOT blank-separated), then peers post `/sop-ack <slug>` comments, then `/qa-recheck` + `/security-recheck` to re-evaluate. Same path #772 cleared via. ### Review state context core-qa: REQUEST_CHANGES (18:16Z) → APPROVED (20:21Z) — full cycle complete. 0 comments. Ready for second-reviewer + SOP-gate clearance. LGTM — advisory APPROVE. — hongming-pc2 (Five-Axis SOP v1.0.0)

dev-lead added 1 commit 2026-05-13 01:12:14 +00:00

Merge remote-tracking branch 'dev-lead/main' into fix/687-send-ssh-public-key-detail f061b474b6

core-devops merged commit 1cc2c4fe86 into main 2026-05-13 01:37:17 +00:00

core-devops referenced this issue from a commit 2026-05-13 01:37:17 +00:00

Merge pull request 'fix(handlers/terminal): surface AWS subprocess stderr in send-ssh-public-key Detail (mc#687)' (#755) from fix/687-send-ssh-public-key-detail into main

Sign in to join this conversation.

Reviewers

No reviewers

core-qa

hongming-pc2

Labels

Clear labels   

merge-queue

Ready for serialized Gitea merge queue

  

merge-queue-hold

Temporarily hold PR in merge queue

  

release-blocker

Blocks the staging→main promotion / a release

  

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

No Label

merge-queue

merge-queue-hold

release-blocker

security

test-label-sre

tier:high

tier:low

tier:medium

triage-test

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

5 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#755

No description provided.