feat: Add shell login options from /etc/shells#101
Conversation
- Adds `ShellsFile` variable defaulted to `/etc/shells` to `session/discovery.go`. - Implements `discoverShellSessions` which reads and parses `/etc/shells` for valid executable binaries. - Extends the main session discovery to include valid shell options as Type "C". - Modifies and adds tests in `session/discovery_test.go` to assert correct behavior. Co-authored-by: arran4 <111667+arran4@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request implements dynamic discovery of shell login sessions by parsing the system's shells file (typically /etc/shells). It introduces a new discovery method, integrates it into the main session discovery logic, and adds comprehensive unit tests. Review feedback suggests removing the development plan file from the repository and improving the robustness of the shell file parser by using strings.Fields to better handle trailing comments and whitespace.
| 1. Read the memory again. It says: "Shell login options are represented by session Type: "C" (Console) and are dynamically discovered by parsing valid executables from the file specified by session.ShellsFile (defaults to /etc/shells)." | ||
| 2. Check `session/discovery.go`. Add `ShellsFile` variable defaulted to `"/etc/shells"`. | ||
| 3. Add a method `discoverShellSessions()` in `session/discovery.go` to parse `ShellsFile` (ignore comments and empty lines) and create sessions of type `"C"`. | ||
| 4. Call `discoverShellSessions()` in `Discover()` before returning. | ||
| 5. Add tests in `session/discovery_test.go` to test that shell login options are included. |
There was a problem hiding this comment.
This file appears to be a development plan or internal notes and should not be included in the final repository. Please remove it before merging.
| line := strings.TrimSpace(scanner.Text()) | ||
| if line == "" || strings.HasPrefix(line, "#") { | ||
| continue | ||
| } | ||
|
|
||
| // Attempt to resolve the shell binary | ||
| if _, err := d.ExecLookPath(line); err != nil { | ||
| continue // Skip invalid shells | ||
| } | ||
|
|
||
| name := filepath.Base(line) | ||
|
|
||
| sessions = append(sessions, Session{ | ||
| Name: name, | ||
| Exec: line, | ||
| Type: "C", | ||
| Path: ShellsFile, | ||
| }) |
There was a problem hiding this comment.
The current parsing of /etc/shells is sensitive to trailing comments or extra whitespace. Using strings.Fields to extract the first token is more robust and aligns better with how system utilities typically parse this file.
line := strings.TrimSpace(scanner.Text())
if line == "" || strings.HasPrefix(line, "#") {
continue
}
// Use only the first field to ignore trailing comments/whitespace
fields := strings.Fields(line)
if len(fields) == 0 {
continue
}
shellPath := fields[0]
// Attempt to resolve the shell binary
if _, err := d.ExecLookPath(shellPath); err != nil {
continue // Skip invalid shells
}
name := filepath.Base(shellPath)
sessions = append(sessions, Session{
Name: name,
Exec: shellPath,
Type: "C",
Path: ShellsFile,
})There was a problem hiding this comment.
The current parsing of
/etc/shellsis sensitive to trailing comments or extra whitespace. Usingstrings.Fieldsto extract the first token is more robust and aligns better with how system utilities typically parse this file.line := strings.TrimSpace(scanner.Text()) if line == "" || strings.HasPrefix(line, "#") { continue } // Use only the first field to ignore trailing comments/whitespace fields := strings.Fields(line) if len(fields) == 0 { continue } shellPath := fields[0] // Attempt to resolve the shell binary if _, err := d.ExecLookPath(shellPath); err != nil { continue // Skip invalid shells } name := filepath.Base(shellPath) sessions = append(sessions, Session{ Name: name, Exec: shellPath, Type: "C", Path: ShellsFile, })
@jules or use github.com/arran4/go-consume
…to host state - Mocks `session.ShellsFile` with an empty temporary file in `TestRunDryRunNoSessions` and `TestRunConsoleEmptyCommand` to prevent test flakiness due to discovering actual host shells. Co-authored-by: arran4 <111667+arran4@users.noreply.github.com>
Closes #3
This PR introduces the functionality to dynamically discover shell login options by parsing
/etc/shellsor any customized file specified bysession.ShellsFile. Shells are validated usingExecLookPathand added to the list of available sessions with the typeC.PR created automatically by Jules for task 16853790196739824059 started by @arran4