feat(kirocrew): opt-in Telegram channel wiring during install (do not merge yet) - #95
Conversation
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)
![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: 497dd01802
ℹ️ 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".
| --arg telegram_bot_token "${KIROCREW_TG_BOT_TOKEN:-}" \ | ||
| --arg telegram_user_id "${KIROCREW_TG_USER_ID:-}" \ |
There was a problem hiding this comment.
For both inspected CloudFormation CLI and Console deployment flows, these variables exist only in the local installer process: build_deploy_params has no corresponding parameters, and deploy/cloudformation/template.yaml neither exports them in UserData nor passes them to bootstrap.sh. Consequently, the remote bootstrap expands both values to empty strings here, so packs/kirocrew/install.sh always skips Telegram wiring even after the wizard reports that the credentials were captured.
Useful? React with 👍 / 👎.
|
To use Codex here, create an environment for this repo. |
… 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)
|
Fixed in 4c9d1fd — thanks Codex, that was a real gap. End-to-end plumbing now closed:
Traced both console-and-CLI deploy paths through the wizard → CFN → UserData → bootstrap. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c9d1fdc94
ℹ️ 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".
| KIROCREW_TG_BOT_TOKEN="" | ||
| KIROCREW_TG_USER_ID="" |
There was a problem hiding this comment.
When an interactive KiroCrew install opts into Telegram, the normal CloudFormation Console flow and the default CLI flow still deploy empty values because build_deploy_params ran at line 3335 before these assignments and is never called again for KiroCrew; only the unrelated WebUI-auth CLI path happens to rebuild later. Fresh evidence in this revision is that the KirocrewTg* CloudFormation parameters now exist, but PARAM_VALUES remains the stale array created before the prompt, so the wizard reports success while the pack receives no credentials.
Useful? React with 👍 / 👎.
| export KIRO_FROM_SECRET="${KiroFromSecret}" | ||
| export TELEGRAM_BOT_TOKEN_SECRET="${TelegramBotTokenSecret}" | ||
| export TELEGRAM_USER="${TelegramUser}" | ||
| export KIROCREW_TG_BOT_TOKEN="${KirocrewTgBotToken}" |
There was a problem hiding this comment.
When KirocrewTgBotToken is supplied, !Sub embeds its plaintext value permanently in the EC2 instance's resolved UserData; NoEcho only masks the CloudFormation parameter display and does not protect the resulting EC2 attribute. Consequently, principals with permission to retrieve EC2 user data can recover and reuse the Telegram bot credential, despite the later effort to keep it in a mode-0600 file. Pass a Secrets Manager identifier and resolve it during bootstrap, as the existing TelegramBotTokenSecret flow does, rather than interpolating the token here.
Useful? React with 👍 / 👎.
… 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 818c0f8585
ℹ️ 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".
| Resource: | ||
| - !Sub 'arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:${KirocrewTgBotTokenSecret}*' | ||
| - !Ref KirocrewTgBotTokenSecret |
There was a problem hiding this comment.
When the wizard supplies its normal secret name (/lowkey/... from install.sh), this !Ref produces a non-ARN IAM resource and causes creation of KirocrewTgSecretReadPolicy to fail with a malformed policy; when a caller instead supplies the documented full ARN, the preceding !Sub constructs arn:aws:secretsmanager:...:secret:arn:aws:..., which is likewise invalid. Normalize the parameter to one valid Secrets Manager ARN rather than emitting both representations.
Useful? React with 👍 / 👎.
| - Sid: ReadKirocrewTelegramBotToken | ||
| Effect: Allow | ||
| Action: | ||
| - secretsmanager:GetSecretValue | ||
| - secretsmanager:DescribeSecret |
There was a problem hiding this comment.
When KiroCrew is installed with the account_assistant profile, the existing AccountAssistantDenyPolicy explicitly denies secretsmanager:GetSecretValue on every resource (lines 906-922), so this new allow can never authorize the bootstrap lookup. The lookup failure is swallowed in deploy/bootstrap.sh, and the opted-in deployment completes without Telegram wiring; the deny must exclude this bootstrap secret or the secret must be resolved by a principal not subject to that deny.
Useful? React with 👍 / 👎.
| KirocrewTgSecretReadPolicy: | ||
| Type: AWS::IAM::Policy | ||
| Condition: HasKirocrewTgBotTokenSecret |
There was a problem hiding this comment.
On any opted-in deployment, Instance depends only on InstanceProfile, not on this sibling AWS::IAM::Policy, so CloudFormation may start UserData before the new permission has been attached. The bootstrap performs get-secret-value only once and suppresses an access failure, permanently skipping Telegram wiring if this race occurs; add an explicit dependency or place the conditional permission directly on the role.
Useful? React with 👍 / 👎.
| if aws secretsmanager describe-secret --secret-id "$_KC_TG_SECRET_NAME" --region "$DEPLOY_REGION" >/dev/null 2>&1; then | ||
| aws secretsmanager put-secret-value \ | ||
| --secret-id "$_KC_TG_SECRET_NAME" \ | ||
| --secret-string "$KIROCREW_TG_BOT_TOKEN" \ | ||
| --region "$DEPLOY_REGION" >/dev/null || fail "Failed to update Telegram bot token in Secrets Manager" |
There was a problem hiding this comment.
During an interactive KiroCrew setup, these AWS writes happen before show_summary asks whether to deploy or change settings. Canceling at the advanced-mode confirmation, or selecting “Change settings” in simple mode, therefore leaves a newly created secret (or an updated secret version) even though no deployment was approved; retain the token locally and perform the write after confirmation, as the adjacent Roundhouse and Kiro API-key flows already do.
Useful? React with 👍 / 👎.
…, 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79c93b6e97
ℹ️ 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".
| Action: | ||
| - secretsmanager:GetSecretValue | ||
| - secretsmanager:DescribeSecret | ||
| Resource: !Sub 'arn:aws:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:${KirocrewTgBotTokenSecret}*' |
There was a problem hiding this comment.
When KirocrewTgBotTokenSecret keeps its default empty value—which is every deployment that skips this opt-in—this renders as arn:aws:secretsmanager:<region>:<account>:secret:*, granting the instance role GetSecretValue for every secret. The matching empty substitution in AccountAssistantDenyPolicy at line 929 also exempts every secret from its explicit deny, so an account_assistant can read all account secrets despite that profile's isolation policy; condition both the allow and deny exemption on a nonempty parameter.
Useful? React with 👍 / 👎.
| _kc_tg_region="${REGION:-${AWS_DEFAULT_REGION:-us-east-1}}" | ||
| _kc_tg_resolved="$(aws secretsmanager get-secret-value \ | ||
| --secret-id "${KIROCREW_TG_BOT_TOKEN_SECRET}" \ | ||
| --query SecretString --output text \ | ||
| --region "${_kc_tg_region}" 2>/dev/null || true)" |
There was a problem hiding this comment.
For deployments outside the bedrock_allowed list in install.sh:2421-2425 (for example ap-south-1), the installer creates this secret in DEPLOY_REGION, but CloudFormation passes the fallback Bedrock region (us-east-1) to bootstrap as --region. This lookup therefore queries the wrong region, suppresses the resulting failure, and silently skips the Telegram wiring after the user opted in; preserve the stack/deployment region separately for Secrets Manager access.
Useful? React with 👍 / 👎.
…(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.
1 participant
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. Wizard prompts (
install.sh)confirmblock gated onPACK_NAME=kirocrew&&!AUTO_YES, default_no (opt-in), positioned right before the existing Kiro API key prompt.^[0-9]+:[A-Za-z0-9_-]+$.^[0-9]{5,15}$.skipat either prompt abandons both fields cleanly (no half-config).tr(matches the Codex-P1 pattern from fix(wizard): retry Kiro API key prompt on invalid format (do not merge yet) #91).2.
deploy/bootstrap.shPlumbs the two vars into
PACK_CONFIGas new fields:telegram-bot-tokenandtelegram-user-id(hyphenated keys matching existingfrom-secret/codex-modelstyle).3. Pack write (
packs/kirocrew/install.sh)Reads the two values via
pack_config_get. New Step 15c writes when both present:TELEGRAM_BOT_TOKEN=<t>appended idempotently to~/.kiro/crew/.env. Env var is KiroCrew's preferred path (per their docs) so the token stays out ofconfig.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 the existing file is malformed JSON, with a warn.Verified
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
bash -n install.sh: OKbash -n deploy/bootstrap.sh: OKbash -n packs/kirocrew/install.sh: OKNot merging per Aug 22 20:34 rule.