Fix SSH agent socket path tilde expansion#329
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCentralizes SSH path expansion into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
TablePro/Core/SSH/SSHPathUtilities.swift (1)
11-22: Consider handling~usernamesyntax for completeness.The implementation correctly handles
~and~/path, which covers the primary use cases for SSH socket paths. However, SSH config also supports~username/pathsyntax for other users' home directories.If this edge case isn't needed for the current use cases (1Password agent socket, private keys), this is fine to defer. Otherwise,
NSString.expandingTildeInPathhandles all tilde forms natively.Alternative using NSString
static func expandTilde(_ path: String) -> String { - if path.hasPrefix("~/") { - return FileManager.default.homeDirectoryForCurrentUser - .appendingPathComponent(String(path.dropFirst(2))) - .path(percentEncoded: false) - } - if path == "~" { - return FileManager.default.homeDirectoryForCurrentUser - .path(percentEncoded: false) - } - return path + return (path as NSString).expandingTildeInPath }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/SSH/SSHPathUtilities.swift` around lines 11 - 22, The expandTilde(_:) function only handles "~" and "~/..." but not "~username/..."; replace or augment its logic by using NSString(string: path).expandingTildeInPath (or manually detect "~" followed by non-slash and call getpwnam to resolve other users) so that SSHPathUtilities.expandTilde correctly expands "~username" and "~username/..." as well as the existing cases; update the function to return the expanded path from NSString.expandingTildeInPath (or the resolved home directory plus remaining path) while preserving current behavior for "~" and "~/...".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TablePro/Core/SSH/SSHPathUtilities.swift`:
- Line 8: The enum declaration for SSHPathUtilities lacks explicit access
control; update the declaration of enum SSHPathUtilities to include the
appropriate access level (e.g., internal or public depending on cross-module
usage) so the type follows the project's coding guidelines requiring explicit
access control.
---
Nitpick comments:
In `@TablePro/Core/SSH/SSHPathUtilities.swift`:
- Around line 11-22: The expandTilde(_:) function only handles "~" and "~/..."
but not "~username/..."; replace or augment its logic by using NSString(string:
path).expandingTildeInPath (or manually detect "~" followed by non-slash and
call getpwnam to resolve other users) so that SSHPathUtilities.expandTilde
correctly expands "~username" and "~username/..." as well as the existing cases;
update the function to return the expanded path from
NSString.expandingTildeInPath (or the resolved home directory plus remaining
path) while preserving current behavior for "~" and "~/...".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de85690e-0561-44b9-9866-d985242bb054
📒 Files selected for processing (5)
TablePro/Core/SSH/Auth/AgentAuthenticator.swiftTablePro/Core/SSH/Auth/PublicKeyAuthenticator.swiftTablePro/Core/SSH/SSHConfigParser.swiftTablePro/Core/SSH/SSHPathUtilities.swiftTableProTests/Core/SSH/SSHConfigurationTests.swift
|
|
||
| import Foundation | ||
|
|
||
| enum SSHPathUtilities { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit access control to the enum declaration.
The coding guidelines require explicit access control on types. Since this utility is used across multiple modules within the SSH subsystem, consider adding internal (or public if needed externally).
Proposed fix
-enum SSHPathUtilities {
+internal enum SSHPathUtilities {As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enum SSHPathUtilities { | |
| internal enum SSHPathUtilities { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TablePro/Core/SSH/SSHPathUtilities.swift` at line 8, The enum declaration for
SSHPathUtilities lacks explicit access control; update the declaration of enum
SSHPathUtilities to include the appropriate access level (e.g., internal or
public depending on cross-module usage) so the type follows the project's coding
guidelines requiring explicit access control.
1 participant
Summary
rc=-42when socket paths contain~(e.g., 1Password's~/Library/Group Containers/2BUA8C4S2C.com.1password/t/agent.sock)setenv()doesn't expand~like shell commands do — now applyingSSHPathUtilities.expandTilde()before passing tosetenv("SSH_AUTH_SOCK", ...)expandPath()fromPublicKeyAuthenticatorandSSHConfigParserinto sharedSSHPathUtilitiesutilityTest plan
rc=-42)~/.ssh/agent.sockpath — should expand correctly~/.ssh/id_rsa— should still work (uses shared utility)SSHConfigurationTests— 4 new tilde expansion tests passCloses #322
Summary by CodeRabbit
New Features
Bug Fixes
Tests