feat: per-person App default + shell-aware export snippet#2
Conversation
Two ergonomic changes to setup: 1. Default to one-App-per-person. The setup flow now points at https://github.com/settings/apps/new (personal account) and suggests `<username>-claude` as the App name, so PR comments are attributed to e.g. `hinne-claude[bot]` per developer instead of a shared org bot. Org-owned Apps remain a documented alternative for teams that want a single shared identity. 2. Detect \$SHELL and emit the right rc-file path + export syntax: - bash → ~/.bashrc with `export FOO=bar` - zsh → ~/.zshrc with `export FOO=bar` - fish → ~/.config/fish/config.fish with `set -x FOO bar` and fish-style command substitution `(cmd)` instead of `\$(cmd)` - unknown → bash-style snippet with broader rc-file hint Also wraps command-substitution values in double quotes for bash/zsh so the substitution defers to read-time, not profile-load time. README updated to lead with the per-person pattern and document the trade-offs vs a shared org bot.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the documentation and setup process to recommend a per-person GitHub App pattern for improved attribution and security. It also introduces a shell detection mechanism to provide tailored environment variable export snippets for bash, zsh, and fish. The review feedback focuses on ensuring that generated shell export values are properly quoted to handle spaces or special characters and correcting technical descriptions regarding shell evaluation timing.
| func bashStyleExport(key, value string, isCommandSub bool) string { | ||
| if isCommandSub { | ||
| // Quote command substitutions so the shell defers evaluation | ||
| // until the variable is read, not at profile load. | ||
| return fmt.Sprintf(`export %s="%s"`, key, value) | ||
| } | ||
| return fmt.Sprintf("export %s=%s", key, value) | ||
| } |
There was a problem hiding this comment.
The values in the export snippet should be quoted to handle paths or values containing spaces or special characters. Additionally, the comment regarding "deferring evaluation" is inaccurate; in bash/zsh, export VAR="$(cmd)" executes the command at the time of assignment (e.g., when the shell profile is sourced), not when the variable is read. Quoting is primarily useful here to preserve newlines in PEM data and handle spaces in paths.
func bashStyleExport(key, value string, isCommandSub bool) string {
// We always quote the value to handle spaces and special characters.
// Note: for command substitutions, this prevents word splitting but
// evaluation still occurs at profile load (assignment) time.
return fmt.Sprintf("export %s=\"%s\"", key, value)
}| func fishStyleExport(key, value string, isCommandSub bool) string { | ||
| // fish uses `set -x` for exported vars. Command substitution in | ||
| // fish is `(cmd)` not `$(cmd)`, so we rewrite if we see one. | ||
| if isCommandSub { | ||
| v := strings.TrimPrefix(value, "$(") | ||
| v = strings.TrimSuffix(v, ")") | ||
| return fmt.Sprintf("set -x %s (%s)", key, v) | ||
| } | ||
| return fmt.Sprintf("set -x %s %s", key, value) | ||
| } |
There was a problem hiding this comment.
For literal values (where isCommandSub is false), the value should be quoted to handle spaces or special characters in paths. In Fish, set -x KEY value with spaces would treat the value as a list of multiple elements.
| func fishStyleExport(key, value string, isCommandSub bool) string { | |
| // fish uses `set -x` for exported vars. Command substitution in | |
| // fish is `(cmd)` not `$(cmd)`, so we rewrite if we see one. | |
| if isCommandSub { | |
| v := strings.TrimPrefix(value, "$(") | |
| v = strings.TrimSuffix(v, ")") | |
| return fmt.Sprintf("set -x %s (%s)", key, v) | |
| } | |
| return fmt.Sprintf("set -x %s %s", key, value) | |
| } | |
| func fishStyleExport(key, value string, isCommandSub bool) string { | |
| // fish uses `set -x` for exported vars. Command substitution in | |
| // fish is `(cmd)` not `$(cmd)`, so we rewrite if we see one. | |
| if isCommandSub { | |
| v := strings.TrimPrefix(value, "$(") | |
| v = strings.TrimSuffix(v, ")") | |
| return fmt.Sprintf("set -x %s (%s)", key, v) | |
| } | |
| return fmt.Sprintf("set -x %s \"%s\"", key, value) | |
| } |
| func TestExportLines_LiteralPathUnquoted(t *testing.T) { | ||
| t.Setenv("SHELL", "/bin/bash") | ||
| p := detectShell() | ||
| lines := p.exportLines("1", "2", "/abs/path/key.pem") | ||
| keyLine := lines[2] | ||
| if strings.Contains(keyLine, `"`) { | ||
| t.Errorf("literal path should not be quoted; got %q", keyLine) | ||
| } | ||
| } |
There was a problem hiding this comment.
This test explicitly checks that literal paths are unquoted. If the implementation is updated to quote values (to support paths with spaces), this test should be updated to expect quotes.
| func TestExportLines_LiteralPathUnquoted(t *testing.T) { | |
| t.Setenv("SHELL", "/bin/bash") | |
| p := detectShell() | |
| lines := p.exportLines("1", "2", "/abs/path/key.pem") | |
| keyLine := lines[2] | |
| if strings.Contains(keyLine, `"`) { | |
| t.Errorf("literal path should not be quoted; got %q", keyLine) | |
| } | |
| } | |
| func TestExportLines_LiteralPathQuoted(t *testing.T) { | |
| t.Setenv("SHELL", "/bin/bash") | |
| p := detectShell() | |
| lines := p.exportLines("1", "2", "/abs/path/key.pem") | |
| keyLine := lines[2] | |
| if !strings.Contains(keyLine, "\"") { | |
| t.Errorf("literal path should be quoted; got %q", keyLine) | |
| } | |
| } |
1 participant
Summary
Two ergonomic changes on top of the initial scaffold:
1. One App per person (recommended). Setup now defaults to
https://github.com/settings/apps/new(personal account) instead of the org URL, and suggests `-claude` as the App name. PR comments end up attributed to e.g. `hinne-claude[bot]` per developer — natural fit for the use case where bot reviews are shaped by each developer's project memory and prompts. Org-owned Apps are still a documented alternative.2. Shell-aware export snippet. `gh as-bot setup` now reads `$SHELL` and emits the right rc-file path and export syntax:
Bonus: command substitutions are now wrapped in double quotes for bash/zsh so the substitution defers to read-time, not profile-load time.
Test plan