Add setup skill for guided onboarding - #1187
Conversation
There was a problem hiding this comment.
Review: skills/setup/SKILL.md
Thorough review of this new /setup skill against the existing codebase (setup_flow.py, secrets.template.env, config.yaml.example, repositories.yaml.example, gateway code, and validators).
Blocking Issues
1. Wrong OAuth token variable name — will break Anthropic auth
Phase 3 Step 1 says to validate OAuth tokens start with sk-ant-oat but never specifies the actual env var name to write. The codebase has two names:
CLAUDE_CODE_OAUTH_TOKEN— preferred, used bysetup_flow.py,sandbox/egg_lib/auth.py, and checked first bygateway/anthropic_credentials.py:155ANTHROPIC_OAUTH_TOKEN— legacy fallback name insecrets.template.envand gateway
The skill must explicitly instruct the agent to write CLAUDE_CODE_OAUTH_TOKEN (not ANTHROPIC_OAUTH_TOKEN) to secrets.env. Without this, an LLM executing the skill will likely use the template file's variable name, which is the legacy one.
2. Missing GITHUB_APP_ID and GITHUB_APP_INSTALLATION_ID in secrets.env
Phase 3 Step 2 (GitHub App flow) collects the App ID and Installation ID but only says to "copy the .pem file." It never instructs writing GITHUB_APP_ID and GITHUB_APP_INSTALLATION_ID to secrets.env. The gateway's token_refresher.py:278-287 reads these from secrets to generate installation tokens. Without them, GitHub App auth silently fails. Compare setup_flow.py:233-238 which writes both.
3. secrets.env variable names not specified anywhere
The skill tells the agent to collect credentials and "Write all collected secrets to ~/.config/egg/secrets.env" but never defines the exact env var names. This is a skill executed by an LLM — it will guess variable names, and those guesses may not match what the gateway, sandbox, and orchestrator expect.
The skill must provide a mapping, e.g.:
- Anthropic OAuth →
CLAUDE_CODE_OAUTH_TOKEN - Anthropic API key →
ANTHROPIC_API_KEY - GitHub App ID →
GITHUB_APP_ID - GitHub App Installation ID →
GITHUB_APP_INSTALLATION_ID - GitHub user PAT →
GITHUB_USER_TOKEN - Gateway bot name →
GATEWAY_BOT_NAME - Gateway branch prefix →
GATEWAY_BOT_BRANCH_PREFIX - Gateway trusted users →
GATEWAY_TRUSTED_USERS - Slack bot token →
SLACK_TOKEN - Slack app token →
SLACK_APP_TOKEN - Confluence →
CONFLUENCE_BASE_URL,CONFLUENCE_USERNAME,CONFLUENCE_API_TOKEN,CONFLUENCE_SPACE_KEYS - JIRA →
JIRA_BASE_URL,JIRA_USERNAME,JIRA_API_TOKEN,JIRA_JQL_QUERY
Or reference config/secrets.template.env as the canonical source.
4. PAT flow doesn't set GITHUB_TOKEN — gateway auth broken for default repos
Phase 3 Step 2 PAT flow sets GITHUB_USER_TOKEN but not GITHUB_TOKEN. The gateway uses GITHUB_TOKEN for all writable repos by default (secrets.template.env:44-66). GITHUB_USER_TOKEN is only used for repos with auth_mode: user in repositories.yaml. A user who picks PAT-only auth and doesn't set every repo to auth_mode: user will have a non-functional gateway.
Fix: Either set GITHUB_TOKEN from the provided PAT, or set all configured repos to auth_mode: user and document this clearly.
5. Python minimum version is wrong — 3.13+ vs actual 3.11+
Phase 1 states Python 3.13+ is required. The actual requirement is >=3.11 (sandbox/pyproject.toml:5). Python 3.13 is what runs inside the sandbox Docker container, but the host only needs 3.11+ to run the egg CLI. This will cause users on Python 3.11/3.12 to be incorrectly told their setup is broken.
Non-Blocking Issues
6. Missing bot_username in repositories.yaml output
Phase 4 doesn't collect or write the bot_username field, which exists in repositories.yaml.example:24 and is used by repo_config.py for bot PR identification. The skill should either collect this (defaulting to GATEWAY_BOT_NAME value) or note it as auto-inferred.
7. Missing readable_repos configuration
Phase 4 Step 3 only offers "Writable" and "Read-only" for each repo, but the Phase 6 write step doesn't mention writing to readable_repos — only writable_repos and local_repos.paths are referenced in the validation section. The skill needs to handle writing read-only repos to the readable_repos list in the YAML.
8. Phase 4 Step 2 UX doesn't match AskUserQuestion interaction model
The skill says: "Type each path and press Enter. Type 'done' when finished." But this is a skill for Claude Code where user input goes through AskUserQuestion (multiple-choice) or free-text. The "type and press Enter" loop pattern comes from the terminal-based setup_flow.py. The skill should use AskUserQuestion to ask for a path, process it, then ask if the user wants to add another.
9. Phase 6 port check — ss not available on macOS
ss -tlnp is Linux-only. For macOS (a primary user platform), use lsof -i :<port> instead. The skill should detect the platform and use the appropriate command, or use a cross-platform check like python3 -c "import socket; s=socket.socket(); s.bind(('', PORT))".
10. Launcher secret generation is redundant with existing code
Phase 3 Step 4 generates the launcher secret with an inline Python one-liner, but setup_flow.py:_create_launcher_secret() already handles this with proper error handling. If the egg CLI is installed (which it should be after dependency check), consider referencing the existing function or at minimum matching its behavior exactly.
11. Phase 5 missing gateway_bot_name, gateway_bot_branch_prefix, gateway_trusted_users in config.yaml
config.yaml.example:84-86 shows these can optionally live in config.yaml as well as secrets.env. The skill doesn't mention this. Not blocking since secrets.env is sufficient, but worth noting for completeness.
12. No reference to secrets.template.env as source of truth
The skill reinvents the secrets schema rather than pointing the agent to config/secrets.template.env (the canonical template). Adding a line like "Reference config/secrets.template.env for the complete list of supported variables and their formats" would prevent drift between the skill and the template.
13. Phase 6 validation reads secrets.env but doesn't specify masking
The Critical Rules section says "mask tokens when displaying (show first 8 and last 4 chars only)" — good. But Phase 6 says to verify credentials "are set" without specifying how to read the file safely. An LLM might cat the file and expose secrets in its output. Phase 6 should explicitly say to parse the file and only check for non-empty values, never displaying the raw content.
Blocking issues 1-5 mean the skill as written will produce broken configurations — wrong variable names, missing required variables, and incorrect version requirements. These need to be fixed before merge.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fix all 5 blocking issues: - Python version: 3.13+ -> 3.11+ (matches pyproject.toml requires-python) - Specify CLAUDE_CODE_OAUTH_TOKEN as preferred variable name - Add GITHUB_APP_ID and GITHUB_APP_INSTALLATION_ID to App flow - PAT flow now sets both GITHUB_TOKEN and GITHUB_USER_TOKEN - Add complete secrets.env variable name mapping table Fix non-blocking issues: - Add bot_username step in repository configuration - Write read-only repos to readable_repos section - Use AskUserQuestion pattern for local repo collection - Cross-platform port check (ss/lsof/Python fallback) - Secrets validation: never display raw values - Reference config/secrets.template.env as source of truth
There was a problem hiding this comment.
All 5 blocking issues and 8 non-blocking issues addressed in f2040c3.
Blocking fixes:
- OAuth variable name — Explicitly specify
CLAUDE_CODE_OAUTH_TOKENas the preferred variable name in Phase 3 Step 1, with note about legacyANTHROPIC_OAUTH_TOKENfallback. - Missing GitHub App IDs — Phase 3 Step 2 App flow now writes
GITHUB_APP_IDandGITHUB_APP_INSTALLATION_IDto secrets.env. - Variable name mapping — Added a complete mapping table in Phase 3 Step 5 with all 19 variable names, plus a reference to
config/secrets.template.envas the canonical source. - PAT flow GITHUB_TOKEN — PAT flow now sets both
GITHUB_TOKEN(for gateway default auth) andGITHUB_USER_TOKEN(forauth_mode: userrepos), with explanation of when each is used. - Python version — Fixed from 3.13+ to 3.11+ to match
sandbox/pyproject.tomlrequires-python = ">=3.11".
Non-blocking fixes:
- bot_username — Added Step 5 in Phase 4 to set
bot_username(defaults toGATEWAY_BOT_NAME). - readable_repos — Phase 4 Step 7 now specifies writable repos go to
writable_repos, read-only toreadable_repos. - AskUserQuestion UX — Phase 4 Step 2 rewritten to use iterative
AskUserQuestionpattern instead of terminal-style input loop. - Cross-platform port check — Phase 6 now uses
ss(Linux),lsof(macOS), or Pythonsocket.bindfallback. - Launcher secret — Kept the inline generation as-is since the skill runs on the host where
eggCLI may not yet be installed (this is the setup skill). The behavior matchessetup_flow.py:_create_launcher_secret(). - Phase 5 gateway config — Not added to config.yaml since secrets.env is sufficient and the skill already writes these there. Adding duplication could cause confusion about which source takes precedence.
- secrets.template.env reference — Added explicit reference in Phase 3 Step 5.
- Secrets validation safety — Added explicit instruction to never display raw values, only check presence and validate prefixes.
— Authored by egg
There was a problem hiding this comment.
Re-review: skills/setup/SKILL.md (delta from f41a8ff → f2040c3)
All 5 previously blocking issues and 8 non-blocking issues have been addressed. Good work on the variable mapping table and the cross-platform port check. Two new blocking issues found in the updated code.
Blocking Issues
1. "Both" flow is under-specified — risks reproducing the original GITHUB_TOKEN bug
Line 159:
Both flow: Run GitHub App flow first, then PAT flow for
GITHUB_USER_TOKEN.
This single sentence is ambiguous for an LLM executor. The PAT flow (lines 153–157) has 4 steps, including step 2 which sets both GITHUB_TOKEN and GITHUB_USER_TOKEN. The phrase "PAT flow for GITHUB_USER_TOKEN" is unclear about whether to:
- Run the full PAT flow (which would set
GITHUB_TOKENto the PAT, overriding App-based auth — broken) - Run a modified PAT flow that only sets
GITHUB_USER_TOKEN(correct)
In the "Both" scenario, the App's token refresher manages GITHUB_TOKEN at runtime. Setting GITHUB_TOKEN from the PAT would override App-based auth for non-user repos — the same class of bug as original issue #4.
Fix: Replace the one-liner with explicit steps:
**Both flow**:
1. Run GitHub App flow steps 1–5.
2. Ask for a GitHub PAT. Validate it starts with `ghp_` or `github_pat_`.
3. Set `GITHUB_USER_TOKEN` to the provided PAT in secrets.env. Do NOT set `GITHUB_TOKEN` — the App handles default authentication via the token refresher.
4. Ask for the user's GitHub username for `GATEWAY_TRUSTED_USERS` if not already set.
2. Missing directory creation for fresh installs
No step creates ~/.config/egg/ before writing files. Phase 2 checks ls -la ~/.config/egg/ 2>/dev/null and proceeds to Phase 3 when the directory doesn't exist. Phase 3 Step 4 (line 180) writes to ~/.config/egg/launcher-secret and Step 5 writes ~/.config/egg/secrets.env, but neither creates the parent directory first.
Compare setup_flow.py:589-596 which does config_dir.mkdir(parents=True, exist_ok=True) as its first action. A fresh install will hit FileNotFoundError or shell redirection failure.
Fix: Add a step at the start of Phase 3 (or between Phase 2 and Phase 3):
mkdir -p ~/.config/egg/Non-Blocking Issues
3. Phase 4 Step 2 still has "done" language from old pattern
Line 224: "Enter a path to a local git repository to mount into the egg container (or 'done' if finished)."
Step 4 (line 227) uses AskUserQuestion with "Yes" / "No, done adding repos" options. The "(or 'done' if finished)" in step 1 is leftover from the pre-fix terminal-style pattern that the previous review (issue #8) flagged. Remove it — the user exits the loop via step 4's Yes/No question, not by typing "done".
4. Phase 3 Step 3 integrations are vaguely specified
Line 173: "For each selected integration, collect the required credentials (see config/secrets.template.env for the fields)."
The variable mapping table in Step 5 lists all 8 integration variables (Slack, Confluence, JIRA), but Step 3 doesn't tell the LLM what specific fields to collect for each integration. An LLM executing Step 3 would need to read secrets.template.env at runtime to figure out what to ask for. More reliable to specify inline — e.g., "For Slack: collect SLACK_TOKEN (bot token, starts with xoxb-) and SLACK_APP_TOKEN (app-level token, starts with xapp-)."
5. GitHub App flow doesn't collect GATEWAY_TRUSTED_USERS
The PAT flow (step 4, line 157) collects GATEWAY_TRUSTED_USERS, but the GitHub App flow (lines 146–151) doesn't. While the variable is optional, users with App-only auth may still want to restrict which GitHub users can interact with the bot. Consider adding a step to collect it in the App flow, or note it as configurable later.
6. GITHUB_READONLY_TOKEN absent from variable mapping table
The table in Step 5 (lines 190–210) references config/secrets.template.env as canonical and lists 19 variables, but omits GITHUB_READONLY_TOKEN which exists in the template (line 55 of secrets.template.env). The skill's flow doesn't collect it separately, but since the table is presented as a reference mapping, the omission may cause an LLM to miss it when a user needs separate read-only credentials. Consider adding it with a note that it's optional.
Verification of Previous Issues
All 13 previously raised issues confirmed addressed:
| # | Issue | Status |
|---|---|---|
| 1 | OAuth variable name | Fixed — CLAUDE_CODE_OAUTH_TOKEN explicit at line 131 |
| 2 | Missing GITHUB_APP_ID/INSTALLATION_ID |
Fixed — lines 147–148 |
| 3 | Variable name mapping | Fixed — full table at lines 190–210 |
| 4 | PAT flow GITHUB_TOKEN |
Fixed — line 155 sets both |
| 5 | Python 3.13+ → 3.11+ | Fixed — line 32 |
| 6 | bot_username |
Fixed — Step 5, line 264 |
| 7 | readable_repos |
Fixed — Step 7, line 274 |
| 8 | AskUserQuestion UX |
Partially fixed — Step 4 uses Yes/No, but Step 1 still has "done" language (see issue 3 above) |
| 9 | Cross-platform port check | Fixed — ss / lsof / Python fallback chain |
| 10 | Launcher secret inline | Kept — acceptable rationale (host may lack egg CLI) |
| 11 | Phase 5 gateway config | Declined — acceptable (secrets.env sufficient) |
| 12 | secrets.template.env reference |
Fixed — line 188 |
| 13 | Secrets validation safety | Fixed — lines 314–315 |
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: skills/egg-setup/SKILL.md (delta from f2040c3 → 0905a37)
The only change since the last review is a rename from skills/setup/ to skills/egg-setup/ with corresponding /setup → /egg-setup reference updates. The rename itself is fine.
Both blocking issues from the previous review remain unaddressed. None of the 4 non-blocking issues were addressed either.
Blocking Issues (carried forward, unresolved)
1. "Both" flow is still under-specified — risks setting GITHUB_TOKEN from PAT, breaking App auth
Line 159:
Both flow: Run GitHub App flow first, then PAT flow for
GITHUB_USER_TOKEN.
This is the exact same one-liner flagged in the previous review. The ambiguity is: the PAT flow (lines 153–157) has 4 steps, and step 2 sets both GITHUB_TOKEN and GITHUB_USER_TOKEN. The phrase "then PAT flow for GITHUB_USER_TOKEN" doesn't clearly say to skip setting GITHUB_TOKEN.
I verified the gateway code (repo_visibility.py:_get_tokens()). In the "Both" scenario, the App's token refresher generates the bot token at runtime — GITHUB_TOKEN in secrets.env should NOT be set from a PAT, or it will override App-based auth for non-user repos. The skill must expand this into explicit steps that only set GITHUB_USER_TOKEN, as recommended in the previous review.
2. Missing ~/.config/egg/ directory creation for fresh installs
No mkdir -p ~/.config/egg/ step exists anywhere in Phases 1–3. On a fresh install:
- Phase 2 checks
ls -la ~/.config/egg/ 2>/dev/null(returns nothing) - Phase 3 Step 4 (line 180) writes to
~/.config/egg/launcher-secret— fails withNo such file or directory - Phase 3 Step 5 writes
~/.config/egg/secrets.env— same failure
The Python setup_flow.py handles this on line 608 with Config.USER_CONFIG_DIR.mkdir(parents=True, exist_ok=True). The skill needs an equivalent step at the start of Phase 3, or between Phases 2 and 3.
Non-Blocking Issues (carried forward, unresolved)
3. Phase 4 Step 2 still has "done" language from pre-fix pattern
Line 224: "Enter a path to a local git repository to mount into the egg container (or 'done' if finished)."
Step 4 (line 227) uses AskUserQuestion with "Yes" / "No, done adding repos" options — the user exits via that question, not by typing "done". Remove (or 'done' if finished) from step 1.
4. Phase 3 Step 3 integrations remain vaguely specified
Line 173: "For each selected integration, collect the required credentials (see config/secrets.template.env for the fields)."
An LLM executing this skill needs to know what fields to collect for each integration. The mapping table in Step 5 lists the variable names but Step 3 doesn't specify which variables belong to which integration or what format to validate. More reliable to specify inline — e.g., "For Slack: collect SLACK_TOKEN (bot token, starts with xoxb-) and SLACK_APP_TOKEN (app-level token, starts with xapp-)."
5. GitHub App flow doesn't collect GATEWAY_TRUSTED_USERS
Lines 146–151 (App flow) still don't collect GATEWAY_TRUSTED_USERS. The PAT flow (line 157) does. Users with App-only auth may want to restrict which GitHub users can interact with the bot.
6. GITHUB_READONLY_TOKEN absent from variable mapping table
Lines 190–210 list 19 variables but omit GITHUB_READONLY_TOKEN, which exists in secrets.template.env (line 55). Since the table is presented as a reference mapping and cites secrets.template.env as canonical, the omission creates a gap.
New Observations (from this commit)
7. Naming inconsistency with existing skills (non-blocking)
The existing skill directory is run-workflow (name: run-workflow) without an egg- prefix. This new skill uses egg-setup. If the intent is to namespace all skills with egg-, run-workflow should be updated too. If not, the prefix is inconsistent. Minor — not blocking.
Summary
The rename is clean and the /setup → /egg-setup references are all updated consistently. But the commit does not address either of the 2 blocking issues from the previous review. Both are correctness issues: one will cause App+PAT auth to break, the other will cause fresh installs to fail with No such file or directory.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
All 2 blocking issues and 4 non-blocking issues from the latest review addressed in cc58d0a.
Blocking fixes:
-
"Both" flow expanded — Replaced the ambiguous one-liner with 4 explicit steps. Step 3 now clearly states to set only
GITHUB_USER_TOKENand explicitly says "Do NOT setGITHUB_TOKEN" since the App's token refresher handles default auth at runtime. -
Directory creation for fresh installs — Added
mkdir -p ~/.config/egg/at the start of Phase 3, before any file writes. Matchessetup_flow.py:589-596behavior.
Non-blocking fixes:
-
Removed "done" language — Phase 4 Step 2 no longer says
(or 'done' if finished). The loop exit is handled by Step 4'sAskUserQuestionYes/No. -
Integration fields specified inline — Phase 3 Step 3 now lists exact variable names, prefixes, and formats for each integration (Slack:
SLACK_TOKEN/SLACK_APP_TOKEN, Confluence: 4 fields, JIRA: 4 fields) instead of just referencingsecrets.template.env. -
GATEWAY_TRUSTED_USERSin App flow — Added step 6 to the GitHub App flow to collectGATEWAY_TRUSTED_USERS. Updated the "Both" flow references from "steps 1–5" to "steps 1–6". -
GITHUB_READONLY_TOKENadded — Added to the variable mapping table in Step 5 with note that it's optional for separate read-only credentials.
— Authored by egg
There was a problem hiding this comment.
Re-review: skills/egg-setup/SKILL.md (delta from 0905a37 → cc58d0a)
Both previously blocking issues have been properly fixed. The "Both" flow is now explicit with clear GITHUB_TOKEN prohibition, and the mkdir -p ensures fresh installs don't fail. All 4 previously non-blocking issues were also addressed. One new blocking issue and two non-blocking issues found.
Verification of Previous Issues
All 6 issues from the prior review confirmed resolved:
| # | Issue | Status |
|---|---|---|
| 1 | "Both" flow under-specified | Fixed — expanded to 4 steps with explicit "Do NOT set GITHUB_TOKEN" (line 168). Verified against token_refresher.py and git_client.py — App tokens are injected via GIT_ASKPASS credential helper at push time, not from GITHUB_TOKEN env var. The prohibition is correct. |
| 2 | Missing mkdir -p ~/.config/egg/ |
Fixed — added at start of Phase 3 (line 119). Matches setup_flow.py:608 behavior. |
| 3 | "done" language in Phase 4 Step 2 | Fixed — removed (or 'done' if finished) (line 242). |
| 4 | Integration fields vaguely specified | Fixed — Slack, Confluence, JIRA fields now listed inline with variable names and prefix validation (lines 186–189). |
| 5 | GATEWAY_TRUSTED_USERS in App flow |
Fixed — added as step 6 in App flow (line 157). "Both" flow references "steps 1–6" (line 166). |
| 6 | GITHUB_READONLY_TOKEN in mapping table |
Fixed — added to table (line 215) with "optional" annotation. Verified it exists in secrets.template.env:49-54 and repo_config.py:603. |
Blocking Issues
1. --check flag doesn't check config — feature's core purpose is non-functional
Line 19 describes --check as:
Health check only — verify dependencies and config, report status
Line 429 says:
/egg-setup --check # Verify your setup is healthy
But line 79 says:
If running
/egg-setup --check, stop here after reporting the results. Do not proceed to other phases.
Phase 1 only checks dependencies (Python, Docker, Git, etc.). It does NOT run Phase 6 (configuration/secrets/repo validation). A user who runs /egg-setup --check expects to learn whether their secrets.env, repositories.yaml, and config.yaml are valid. Instead, they only get a dependency checklist.
This means the most common health check entry point gives false confidence — a user with valid dependencies but a broken secrets.env (wrong variable names, missing GITHUB_APP_ID, etc.) will see all green checkmarks and think their setup is healthy.
Fix: --check should run Phase 1 (dependencies) AND Phase 6 (validation), skipping Phases 2–5. Change line 79 to:
If running `/egg-setup --check`, skip to Phase 6 (Validation) after reporting dependency results. Do not run Phases 2–5.
Non-Blocking Issues
2. Port conflict check — echo "Ports available" is dead code
Lines 356–370 (Phase 6, Docker Validation) use an || chain:
ss -tlnp 2>/dev/null | grep -E ':(9848|...) ' || \
lsof -i :9848 ... 2>/dev/null || \
python3 -c "..." 2>/dev/null || echo "Ports available"When all ports are free, the Python fallback runs, all bind() calls succeed, it prints nothing, and exits 0. Because exit 0, the || chain stops — echo "Ports available" never executes. Additionally, when all ports are free on Linux, ss | grep exits 1 (no match), which causes lsof to also run unnecessarily — the chain can't distinguish "tool not found" from "tool found nothing."
The echo "Ports available" only runs in the degenerate case where ss, lsof, AND python3 are all missing. In every normal case where ports are free, the user sees no output.
Suggested fix: modify the Python script to exit nonzero when all ports are free (sys.exit(1) if no conflicts found), or restructure to capture output into a variable and branch on whether it's empty.
3. --update repos and --update config skip directory creation
--update repos jumps to Phase 4 and --update config jumps to Phase 5 — neither includes mkdir -p ~/.config/egg/. Only Phase 3 has the directory creation. If the directory doesn't exist (admittedly unlikely for an "update" path, but possible if the user deleted it), the file writes will fail.
Suggestion: add the mkdir -p to Phase 4 and Phase 5 as well, or add a shared preamble step that all phases run first.
Summary
The two previously blocking issues are cleanly fixed and the "Both" flow is now correctly specified — I verified the GITHUB_TOKEN prohibition against the gateway's actual token injection path (token_refresher.py → get_bot_token() → create_credential_helper() → GIT_ASKPASS). The new blocking issue (#1) is a straightforward fix: extend --check to also run Phase 6 validation.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
All 3 issues from the latest review addressed in 6aaf514. Blocking fix:
Non-blocking fixes:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: skills/egg-setup/SKILL.md (delta from cc58d0a → 6aaf514)
All 3 issues from the previous review have been properly addressed. Verified each fix against the codebase.
Verification of Previous Issues
| # | Issue | Status | Verification |
|---|---|---|---|
| 1 | --check doesn't run Phase 6 validation |
Fixed — line 79 now says "skip to Phase 6 (Validation) after reporting dependency results. Do not run Phases 2–5." | Traced the --check flow end-to-end. Phase 6 reads entirely from the filesystem (ls, parsing secrets.env, repositories.yaml, config.yaml). No in-session state from Phases 2–5 is required. A fresh --check on a complete install will validate everything; on an incomplete install, the ls commands will correctly report missing files. |
| 2 | Port check echo "Ports available" is dead code / broken || chain |
Fixed — replaced with a single cross-platform Python script that tracks conflicts in a list, prints each conflict, exits 1 on conflicts, prints "All ports available" and exits 0 when clear. | Verified the script logic: socket.bind() succeeds → close and continue; fails → append to conflicts list. Exit codes are correct. The old ss || lsof || python3 || echo chain had multiple problems (couldn't distinguish "tool missing" from "no conflicts," dead echo, suppressed errors via 2>/dev/null). The new version also correctly removes 2>/dev/null from the Python invocation, so missing python3 surfaces an error rather than being silently ignored. |
| 3 | --update repos and --update config skip directory creation |
Fixed — mkdir -p ~/.config/egg/ added at the start of Phase 4 (line 235) and Phase 5 (line 305). |
Mapped all entry points: full setup hits Phase 3 mkdir first; --update secrets → Phase 3 (has mkdir); --update repos → Phase 4 (now has mkdir); --update config → Phase 5 (now has mkdir); --check → Phase 6 (read-only, no mkdir needed). All write paths are covered. |
Additional Verification: "Both" Flow Token Handling
Since this was a critical fix from a prior iteration, I re-verified the "Both" flow (lines 166–170) against the gateway source:
token_refresher.py: Theinitialize_token_refresher()function readsGITHUB_APP_ID,GITHUB_INSTALLATION_ID, andGITHUB_PRIVATE_KEY_PATH— it does not readGITHUB_TOKENfrom the environment.git_client.py:1465-1498: Token selection usesget_auth_mode(repo)—"bot"mode gets tokens from the refresher,"user"mode readsGITHUB_USER_TOKEN.github_client.py:927: TheghCLI receives tokens viaGH_TOKENenv var set per-invocation, not from a persistentGITHUB_TOKEN.
The prohibition on setting GITHUB_TOKEN in the "Both" flow is correct — GITHUB_TOKEN is not consumed anywhere in the gateway, and the App's token refresher manages default auth via GITHUB_APP_ID + .pem key.
Non-Blocking Observations
1. Port check: unclosed sockets on the error path (trivial)
In the except OSError branch, the socket s is created but never closed. This accumulates at most 4 unclosed file descriptors. Since this is a one-shot script that exits immediately after, the OS cleans them up. Not a real issue, but adding s.close() in the except block or using a context manager would be slightly cleaner:
for port in [9848, 9849, 9850, 3129]:
s = socket.socket()
try:
s.bind(('', port))
except OSError:
conflicts.append(port)
finally:
s.close()2. Naming inconsistency with run-workflow (carried from prior review, still open)
egg-setup uses an egg- prefix while the existing run-workflow skill does not. If the intent is to namespace all skills, run-workflow should be updated too. Not blocking for this PR.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: skills/egg-setup/SKILL.md (delta from 6aaf514 → 44c365a)
Single change since the last review: socket close moved from try block to finally block in the port conflict check. This was the non-blocking suggestion #1 from the previous review.
Verification
The fix is correct:
# Before (6aaf514): socket leaked on OSError path
try:
s.bind(('', port))
s.close() # only runs on success
except OSError:
conflicts.append(port) # socket never closed
# After (44c365a): socket always closed
try:
s.bind(('', port))
except OSError:
conflicts.append(port)
finally:
s.close() # runs in both pathsFull State Verification
Re-verified all 15 previously raised issues across the entire review history remain fixed:
| # | Issue | Status |
|---|---|---|
| 1 | OAuth → CLAUDE_CODE_OAUTH_TOKEN |
Fixed |
| 2 | GITHUB_APP_ID / INSTALLATION_ID in App flow |
Fixed |
| 3 | Variable name mapping table (19 vars + GITHUB_READONLY_TOKEN) |
Fixed |
| 4 | PAT flow sets both GITHUB_TOKEN and GITHUB_USER_TOKEN |
Fixed |
| 5 | Python 3.11+ (not 3.13+) | Fixed |
| 6 | bot_username in Phase 4 |
Fixed |
| 7 | readable_repos in Phase 4 Step 7 |
Fixed |
| 8 | AskUserQuestion pattern (no "done" language) |
Fixed |
| 9 | Port check: cross-platform Python with finally: s.close() |
Fixed |
| 10 | Integration fields specified inline | Fixed |
| 11 | GATEWAY_TRUSTED_USERS in App flow |
Fixed |
| 12 | --check runs Phase 6 validation |
Fixed |
| 13 | Secrets validation: never display raw values | Fixed |
| 14 | mkdir -p in Phases 3, 4, and 5 |
Fixed |
| 15 | "Both" flow: explicit steps, GITHUB_TOKEN prohibition |
Fixed |
Remaining Non-Blocking Observation (carried forward)
Naming inconsistency: egg-setup uses an egg- prefix while the existing run-workflow skill does not. Not blocking for this PR — can be addressed separately if the intent is to namespace all skills.
— Authored by egg
|
egg review completed. View run logs 11 previous review(s) hidden. |
Summary
/setupClaude skill for guided egg onboarding and configuration management--update secrets,--update repos,--update config)--checkflag for health verification of existing setupsContext
New users currently rely on
egg --setup(the Python-based interactive CLI wizard) combined with manual doc reading to get egg configured. This skill provides a conversational alternative that runs inside Claude Code, leveragingAskUserQuestionfor a guided experience.The skill covers 7 phases: dependency check, existing config detection, secrets configuration, repository configuration, general config, validation, and next steps guidance. It auto-detects platform, GitHub username, and repo remotes to minimize questions.
Changes
skills/setup/SKILL.md— New skill definition following the same format asskills/run-workflow/SKILL.mdIssue: none
Test plan:
/setupin a Claude Code session~/.config/egg/directory)--checkflag for validation-only mode