fix(wizard): retry Kiro API key prompt on invalid format (do not merge yet) - #91
Merged
Conversation
…kipping Previous behavior: user typos ksk_... key, wizard warns 'API key doesn't match expected format', silently skips the whole step, and user has to authenticate manually after install. New behavior: wrap the prompt in a retry loop. - Valid ksk_... key -> accept, break - Empty input (Enter) -> skip (existing convention) - Literal 'skip' typed (case-insensitive) -> skip - Invalid format -> warn with remaining attempt count and re-prompt - After 5 attempts -> warn and skip (avoid infinite loop) Also: gum prompt cancellation (Esc / Ctrl-C) returns non-zero and prompt_secret falls back to the empty default -> treated as skip, so 'press Esc to move on' works out of the box without extra keyboard plumbing. UX text updated from 'Press Enter to skip' to 'Press Enter (empty input) or type skip to skip and authenticate via browser later.' Verified: - bash -n install.sh: OK - All existing paths (empty, valid, invalid+skip, invalid+retry+valid, invalid+retry-until-exhausted) mapped to distinct branches with clear warn messages.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f96d0f491
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _KIRO_INPUT="" | ||
| prompt_secret "Kiro API key" _KIRO_INPUT "" | ||
| # Skip: empty input, or user typed "skip" (case-insensitive) | ||
| if [[ -z "$_KIRO_INPUT" ]] || [[ "${_KIRO_INPUT,,}" == "skip" ]]; then |
There was a problem hiding this comment.
On macOS with the stock Bash 3.2, any nonempty response at this Kiro prompt reaches the Bash 4-only ${_KIRO_INPUT,,} expansion and terminates the installer with bad substitution; this includes valid API keys before they can be validated. Since the installer explicitly supports Darwin, use a portable case-insensitive comparison such as a case pattern or tr instead.
Useful? React with 👍 / 👎.
) Codex flagged: ${_KIRO_INPUT,,} is a Bash 4+ feature. On macOS's stock Bash 3.2 this expansion errors with 'bad substitution' — and because the installer runs under 'set -euo pipefail', ANY non-empty API key entry (including valid keys) would kill the whole installer before validation. Fix: swap the case-conversion for a portable tr pipeline, matching what the rest of the installer already uses for Darwin support. Verified: - bash -n install.sh: OK - Skip paths (Enter / 'skip' / 'SKIP' / 'Skip') still work - Valid keys no longer trip on the lowercasing
royosherove
added a commit
that referenced
this pull request
Aug 23, 2026
… merge yet) (#95) * feat(kirocrew): opt-in Telegram channel wiring during install Roy: 'add telegram auto-connection. ask user if they want to connect telegram. if yes, show them short guide. Get both info from them (allow retry if user pastes wrong formatted stuff). then during pack install, put both sets of info in [user home]/.kiro/crew/config.json: "telegram": { "enabled": true, "allowed_user_ids": [123456789] }'. Three-part change: 1. install.sh (wizard, pre-deploy): - New confirm block gated on PACK_NAME=kirocrew && !AUTO_YES, default_no (opt-in), before the existing Kiro API key prompt. - Short on-screen guide: @Botfather /newbot for token, @userinfobot for numeric user ID. - Bot token retry loop (5 attempts, regex ^[0-9]+:[A-Za-z0-9_-]+$). - User ID retry loop (5 attempts, regex ^[0-9]{5,15}$). - Enter or 'skip' at either prompt abandons both fields cleanly. - Bash 3.2-safe lowercasing via tr (Codex P1 pattern from #91). - Values written to shell vars KIROCREW_TG_BOT_TOKEN and KIROCREW_TG_USER_ID. 2. deploy/bootstrap.sh: - Plumb the two vars into PACK_CONFIG as new fields: 'telegram-bot-token' and 'telegram-user-id' (hyphenated keys to match the existing 'from-secret' / 'codex-model' style). 3. packs/kirocrew/install.sh (pack, on the instance): - Read the two values via pack_config_get (new PACK_ARG_TG_*). - New Step 15c writes both when present: * TELEGRAM_BOT_TOKEN=<t> appended (idempotently) to the existing ~/.kiro/crew/.env managed by the pack. Env var is KiroCrew's preferred way (per their docs/telegram-integration.md) since it keeps the token out of config.json. * telegram.enabled=true + allowed_user_ids=[<id>] jq-merged into ~/.kiro/crew/config.json (upgrade-safe: preserves other keys). * Fallback to a telegram-only overwrite if jq merge fails (invalid existing JSON), with warn. * Done BEFORE the gateway starts \u2014 no restart required. - Half-configured input (only one of the two) warns and skips both, so we never leave the channel in a broken state. Config path and key names verified against KiroCrew v0.3.0 docs: https://raw.githubusercontent.com/kirodotdev/KiroCrew/main/src/kiro_crew/docs/telegram-integration.md Verified: - bash -n install.sh: OK - bash -n deploy/bootstrap.sh: OK - bash -n packs/kirocrew/install.sh: OK - Diff: install.sh +87, deploy/bootstrap.sh +4, packs/kirocrew/install.sh +54 (net +145) * fix(kirocrew): plumb Telegram creds through CFN into bootstrap (Codex P1) Codex P1 on PR #95: the two KIROCREW_TG_* vars set by the install wizard never reached the remote bootstrap, so packs/kirocrew/install.sh always read them as empty and silently skipped Telegram wiring even when the wizard reported the credentials were captured. Full chain now: 1. install.sh - PARAM_CFN_NAMES: append KirocrewTgBotToken, KirocrewTgUserId. - PARAM_VALUES: append matching ${KIROCREW_TG_BOT_TOKEN:-} and ${KIROCREW_TG_USER_ID:-} so build_deploy_params surfaces them to both CFN CLI and CFN deploy paths. 2. deploy/cloudformation/template.yaml - New parameters KirocrewTgBotToken and KirocrewTgUserId (NoEcho: true, Default: ''). - Added to the Model Access parameter group after TelegramUser. - UserData exports KIROCREW_TG_BOT_TOKEN / KIROCREW_TG_USER_ID from the CFN params. - UserData forwards them to bootstrap.sh as --kirocrew-tg-bot-token / --kirocrew-tg-user-id. 3. deploy/bootstrap.sh - Init both vars from environment (defense in depth). - New CLI flags --kirocrew-tg-bot-token and --kirocrew-tg-user-id with the same 'requires a value' guard used by the rest of the flags. - Existing PACK_CONFIG jq block already reads them, so pack-side resolution needs no further change on this commit. Ties directly to Codex review comment #95 (comment) * fix(kirocrew): store TG bot token in Secrets Manager + rebuild params after wizard (Codex P1 x2) Two P1s posted by Codex on commit 4c9d1fd: P1 #A (install.sh:3378) — Rebuild parameters after collecting Telegram credentials build_deploy_params ran at line 3335 BEFORE the KiroCrew Telegram wizard block. When the operator opted in, KIROCREW_TG_* got populated in shell vars but PARAM_VALUES was already frozen with empty strings, so on the default CFN CLI path the pack still received empty values while the wizard reported success. Only the unrelated WebUI-auth CLI path happened to rebuild later. Fix: append a build_deploy_params call inside the success branch of the KiroCrew wizard (after the secret is created and KIROCREW_TG_BOT_TOKEN_SECRET is set), so PARAM_VALUES reflects the fresh state before the deploy step consumes it. P1 #B (template.yaml:1879) — Keep the bot token out of EC2 user data Passing the raw bot token as a CloudFormation parameter and !Sub-ing it into UserData embeds it in the instance's resolved user data attribute. NoEcho only masks the parameter DISPLAY; principals with ec2:DescribeInstanceAttribute --attribute userData can still recover the token in plaintext. Fix: store the token in Secrets Manager BEFORE stack deploy, pass only the secret arn/id through CFN + UserData, resolve on the instance via the instance role. Same pattern as the existing TelegramBotTokenSecret flow for roundhouse. Concrete changes: 1) install.sh - PARAM_CFN_NAMES: rename KirocrewTgBotToken -> KirocrewTgBotTokenSecret. - PARAM_VALUES: emit ${KIROCREW_TG_BOT_TOKEN_SECRET:-} instead of the plaintext token. - KiroCrew wizard success branch: create/update Secrets Manager secret /lowkey/${ENV_NAME}/kirocrew-telegram-bot-token with the captured token (tags: loki:managed, loki:pack=kirocrew, loki:env=${ENV_NAME}), set KIROCREW_TG_BOT_TOKEN_SECRET to the secret id, CLEAR the plaintext var so it can't leak downstream, then call build_deploy_params to refresh PARAM_VALUES. 2) deploy/cloudformation/template.yaml - Parameter KirocrewTgBotToken (NoEcho plaintext) REMOVED. - Parameter KirocrewTgBotTokenSecret (plaintext arn/id) ADDED. - UserData: export KIROCREW_TG_BOT_TOKEN_SECRET only; the token itself never appears in UserData. - New Condition HasKirocrewTgBotTokenSecret. - New Resource KirocrewTgSecretReadPolicy (AWS::IAM::Policy) granting the instance role secretsmanager:GetSecretValue + DescribeSecret, scoped by !Sub to arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:${KirocrewTgBotTokenSecret}* — created only when the parameter is non-empty. - CFN flag propagation to bootstrap.sh updated to --kirocrew-tg-bot-token-secret. 3) deploy/bootstrap.sh - New CLI flag --kirocrew-tg-bot-token-secret (kept --kirocrew-tg-bot-token flag for backward-compat / local invocation). - New resolver block before the PACK_CONFIG jq build: when KIROCREW_TG_BOT_TOKEN is empty but KIROCREW_TG_BOT_TOKEN_SECRET is set, resolve via aws secretsmanager get-secret-value (region from REGION or IMDS) and populate KIROCREW_TG_BOT_TOKEN. Non-fatal on failure — pack side already tolerates missing token. - Rest of the pipeline (jq --arg telegram_bot_token → PACK_CONFIG → packs/kirocrew/install.sh) unchanged, so the pack keeps a single plaintext code path regardless of how the token was supplied. Verification: bash -n clean on install.sh, deploy/bootstrap.sh, and packs/kirocrew/install.sh. * fix(kirocrew): inline TG secret policy, exempt account_assistant deny, defer secret write (Codex P1 x3 + P2) Three P1s and one P2 posted by Codex on commit 818c0f8: P1 #C (template.yaml:882) — Emit only a valid secret ARN in the IAM policy KirocrewTgSecretReadPolicy Resource listed BOTH !Sub arn:aws:secretsmanager:...:secret:${KirocrewTgBotTokenSecret}* !Ref KirocrewTgBotTokenSecret Plain name: the !Ref is not an ARN, IAM rejects the policy. Full ARN: the !Sub produces arn:...:secret:arn:aws:... (nested), also invalid. P1 #D (template.yaml:879) — Exempt the Telegram secret from the account_assistant deny AccountAssistantDenyPolicy explicitly denies secretsmanager:GetSecretValue on Resource '*'; explicit Deny always wins over the sibling Allow, so the bootstrap resolver silently fails under the account_assistant profile and Telegram wiring is skipped. P1 #E (template.yaml:867) — Race between Instance and the sibling IAM Policy Instance depended only on InstanceProfile, not on the new AWS::IAM::Policy. UserData could start before IAM propagated the permission; the resolver runs once and swallows failure, permanently skipping Telegram wiring. P2 (install.sh:3461) — Defer creating the secret until deployment confirmation The wizard used to write to Secrets Manager BEFORE show_summary. Canceling at the summary or choosing 'Change settings' left an orphaned secret. Roundhouse and kiro-cli already defer. Concrete changes: 1) deploy/cloudformation/template.yaml - Removed sibling resource KirocrewTgSecretReadPolicy and its Condition HasKirocrewTgBotTokenSecret entirely. - Added the equivalent statement INLINE on InstanceRole.Policies: PolicyName: ${EnvironmentName}-kirocrew-tg-secret-read Sid: ReadKirocrewTelegramBotToken Action: secretsmanager:GetSecretValue, secretsmanager:DescribeSecret Resource: !Sub arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:${KirocrewTgBotTokenSecret}* Attached at role creation - no race with UserData (P1 #E). Exactly ONE Resource form, well-formed for the installer's plain-name input - the wildcard suffix covers the Secrets Manager 6-char random suffix (P1 #C). - AccountAssistantDenyPolicy DenySecretValues statement: replaced Resource '*' with NotResource: [the same !Sub'd ARN pattern] so the deny still covers everything else but exempts this one bootstrap secret (P1 #D). 2) install.sh - Wizard success branch: no longer calls aws secretsmanager create-secret / put-secret-value. Only computes _KC_TG_SECRET_NAME, sets KIROCREW_TG_BOT_TOKEN_SECRET, keeps the plaintext token in-memory in KIROCREW_TG_BOT_TOKEN, and calls build_deploy_params to refresh PARAM_VALUES. - New deferred block placed immediately before the existing Roundhouse deferred secret write (post show_summary confirmation): materializes the secret via create-secret with put-secret-value fallback for updates, restore-secret guard, tags (loki:managed, loki:pack=kirocrew, loki:env=${ENV_NAME}), mktemp+chmod 600+file:// pattern to keep the token out of process args, then rm -f + unset to scrub state. Verification: bash -n clean on install.sh, deploy/bootstrap.sh, and packs/kirocrew/install.sh. Removed stale KirocrewTgSecretReadPolicy resource and HasKirocrewTgBotTokenSecret condition; verified inline policy + NotResource exemption + deferred write are all in place. * fix(kirocrew): resolve TG secret in stack region, not Bedrock region (Codex P1) Codex P1 on 79c93b6 (deploy/bootstrap.sh region lookup): Chain of failure for deploys outside the Bedrock allowlist (us-east-1 us-west-2 eu-west-1 eu-central-1 eu-north-1 ap-northeast-1 ap-southeast-1) e.g. ap-south-1: install.sh:2421-2425 BEDROCK_REGION = DEPLOY_REGION if allowlisted, else us-east-1 install.sh (deferred) creates the KiroCrew TG secret in DEPLOY_REGION template.yaml UserData bootstrap.sh --region "$BEDROCK_REGION" bootstrap.sh sets REGION from --region Old resolver read ${REGION:-...} -> lookup in Bedrock region, not stack region 2>/dev/null || true swallowed the failure Telegram wiring silently skipped despite operator opt-in. Fix: thread the CFN stack region into bootstrap independently of the Bedrock region. 1) deploy/cloudformation/template.yaml UserData now exports STACK_REGION=${AWS::Region} in addition to the existing REGION and BEDROCK_REGION. STACK_REGION is documented as the authoritative CFN stack / deployment region for downstream resolvers that must hit AWS APIs where the installer created resources (Secrets Manager here; more callers can follow the same pattern). REGION and BEDROCK_REGION semantics unchanged for back-compat. 2) deploy/bootstrap.sh KiroCrew TG resolver region source-of-truth, in order: 1. STACK_REGION exported by UserData (authoritative). 2. IMDSv2 placement/region (works on any EC2, exact stack region). 3. AWS_DEFAULT_REGION. 4. REGION (Bedrock region) only as a last resort, so single-region deployments in the Bedrock allowlist still work when UserData is skipped (dev/manual invocation). WARN message now includes the region actually queried so a future silent-skip is easier to diagnose from the bootstrap log. Verification: bash -n clean on install.sh and deploy/bootstrap.sh; UserData exports STACK_REGION; resolver picks STACK_REGION first with IMDSv2 fallback. --------- Co-authored-by: Roy Osherove <575051+royosherove@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Old behavior
User types
ksk_...with a typo → wizard printswarn: API key doesn't match expected format→ silently skips the whole step → user has to authenticate via browser after install. No second chance.New behavior
Wrap the prompt in a retry loop.
ksk_...keyskiptyped (case-insensitive)Also updated the on-screen guidance:
Press Enter (empty input) or type 'skip' to skip and authenticate via browser later.Not touched
Verification
bash -n install.sh: OKNot merging per Aug 22 20:34 rule.