add nowsh reason to help debug failure

[sawka]

No description provided.

[coderabbitai]

Walkthrough

The changes introduce structured error codes for Wave Shell connection failures. Eight new NoWshCode constants are defined to categorize specific failure modes: disabled, permission error, user declined, domain socket error, connection server start error, install error, post-install start error, and install verify error. The WshCheckResult struct is extended with a NoWshCode field to carry these codes. The tryEnableWsh function is modified to populate NoWshCode across all error paths. Concurrently, telemetry infrastructure is updated to support reporting these error codes through a new "conn:nowsh" event type and a ConnWshErrorCode field in the TEventProps struct.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to evaluate whether the description relates to the changeset.	Add a pull request description explaining the purpose of the NoWshCode additions and how they improve debugging of Wave Shell failures.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'add nowsh reason to help debug failure' directly relates to the main changes: adding NoWshCode constants and fields to track specific Wave Shell failure reasons for debugging purposes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/remote/conncontroller/conncontroller.go (1)

682-733: NoWshCode is consistently populated across all error paths.

All failure scenarios in tryEnableWsh now correctly set the appropriate NoWshCode, enabling structured debugging of WSH connection issues.

Minor observation: Lines 730-732 have identical success returns that could be consolidated, but this is a stylistic nitpick and doesn't affect correctness.

🔎 Optional: Consolidate success returns

 	if needsInstall {
 		conn.Infof(ctx, "connserver needs to be (re)installed\n")
 		err = conn.InstallWsh(ctx, osArchStr)
 		if err != nil {
 			conn.Infof(ctx, "ERROR installing wsh: %v\n", err)
 			err = fmt.Errorf("error installing wsh: %w", err)
 			return WshCheckResult{NoWshReason: "error installing wsh/connserver", NoWshCode: NoWshCode_InstallError, WshError: err}
 		}
 		needsInstall, clientVersion, _, err = conn.StartConnServer(ctx, true)
 		if err != nil {
 			conn.Infof(ctx, "ERROR starting conn server (after install): %v\n", err)
 			err = fmt.Errorf("error starting conn server (after install): %w", err)
 			return WshCheckResult{NoWshReason: "error starting connserver", NoWshCode: NoWshCode_PostInstallStartError, WshError: err}
 		}
 		if needsInstall {
 			conn.Infof(ctx, "conn server not installed correctly (after install)\n")
 			err = fmt.Errorf("conn server not installed correctly (after install)")
 			return WshCheckResult{NoWshReason: "connserver not installed properly", NoWshCode: NoWshCode_InstallVerifyError, WshError: err}
 		}
-		return WshCheckResult{WshEnabled: true, ClientVersion: clientVersion}
-	} else {
-		return WshCheckResult{WshEnabled: true, ClientVersion: clientVersion}
 	}
+	return WshCheckResult{WshEnabled: true, ClientVersion: clientVersion}
 }

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26e6dde and df48ef0.

📒 Files selected for processing (2)

• pkg/remote/conncontroller/conncontroller.go
• pkg/telemetry/telemetrydata/telemetrydata.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: Analyze (go)
• GitHub Check: Analyze (javascript-typescript)
• GitHub Check: Build for TestDriver.ai

🔇 Additional comments (5)

pkg/telemetry/telemetrydata/telemetrydata.go (2)

36-38: LGTM!

The new "conn:nowsh" event name is correctly added to track WSH connection failures, aligning with the telemetry emission in conncontroller.go.

---

109-122: LGTM!

The new ConnWshErrorCode field is appropriately added to capture the error code for WSH connection failures. The formatting improvements with blank lines grouping related fields enhance readability.

pkg/remote/conncontroller/conncontroller.go (3)

50-59: LGTM!

The NoWshCode constants are well-organized and provide comprehensive coverage of all WSH failure modes. Using descriptive string values (rather than numeric iota) is appropriate for telemetry readability.

---

673-679: LGTM!

The NoWshCode field complements the existing NoWshReason by providing a structured, machine-readable error code alongside the human-readable reason.

---

795-801: LGTM!

The telemetry emission correctly captures the NoWshCode when WSH fails to enable, providing valuable debugging data. The event name and props align with the definitions in telemetrydata.go.