fix: address codex PR review feedback (PRs #14, #16, #18)#19
Merged
royosherove merged 3 commits intomainfrom Apr 17, 2026
Merged
fix: address codex PR review feedback (PRs #14, #16, #18)#19royosherove merged 3 commits intomainfrom
royosherove merged 3 commits intomainfrom
Conversation
Adds a full Mintlify docs structure under docs/ so we can publish https://docs.lowkey.run (or similar). Structure --------- docs/ ├── docs.json # Mintlify config (Guide + Reference tabs) ├── index.mdx # Landing ├── quickstart.mdx # ~10-min deploy walkthrough ├── concepts.mdx # Pack / profile / mode / deploy-method ├── profiles/ │ ├── overview.mdx │ ├── builder.mdx # AdministratorAccess profile │ ├── account-assistant.mdx # ReadOnlyAccess + targeted writes │ └── personal-assistant.mdx # Bedrock only, no AWS surface ├── agents/ │ ├── overview.mdx # Pick-a-pack comparison table │ ├── openclaw.mdx # stable, gateway + memory │ ├── claude-code.mdx # stable, Anthropic via Bedrock │ ├── codex-cli.mdx # experimental, OpenAI │ ├── kiro-cli.mdx # experimental, Kiro cloud + headless mode │ ├── nemoclaw.mdx # experimental, OpenShell-sandboxed OpenClaw │ ├── hermes.mdx # experimental, NousResearch via bedrockify │ ├── pi.mdx # experimental, minimal harness │ └── ironclaw.mdx # experimental, Rust, via bedrockify └── reference/ ├── cli.mdx # Full top-level flag reference ├── simple-mode-defaults.mdx # Everything auto-picked per (pack, profile) ├── environment-variables.mdx # Installer + instance + per-pack env ├── cloudformation.mdx # Direct template usage ├── terraform.mdx # Module usage ├── security-services.mdx # What the 5 security services do + cost └── secrets-manager.mdx # Pattern for secrets (--from-secret canonical) Each agent page covers: - When to use it - What the pack installs - Non-interactive install commands (tabbed per profile) - Pack parameters table (flag, default, description) - Resource requirements per profile - First-run / post-install steps - Tear-down Each profile page covers: - What it is (IAM policy summary) - Simple-mode defaults (instance size, volumes, security services) - Install examples (tabbed across top packs) - When to use / when NOT to use Validation ---------- All 23 pages referenced from docs.json exist. JSON schema validated. README.md in docs/ explains structure + local preview via 'mintlify dev'. Follow-ups (separate PRs welcome) -------------------------------- - Wire up a CNAME for docs.lowkey.run - Connect the repo to Mintlify's hosted service or self-host - Add screenshots where they'd help - Add a translations workflow if we want i18n (pattern from openclaw/openclaw)
5 findings flagged by chatgpt-codex-connector across 3 PRs. All addressed here. PR #14 — P1: Hermes default model was wrong --------------------------------------------- pack_default_model(hermes) returned 'NousResearch/Hermes-3-Llama-3.1-8B' which then flowed through CFN DefaultModel → bootstrap.sh --model → PACK_CONFIG.model. Because hermes depends on bedrockify and dependencies read the same PACK_CONFIG, bedrockify's install-daemon got the Hermes-specific ID as its --model — but bedrockify expects a Bedrock model ID. This would have broken hermes deploys (bedrockify fails with model-not-found). The correct split: 'model' = Bedrock id (for bedrockify proxy), 'hermes-model' = the OpenAI-style id bedrockify exposes to Hermes. The pack manifest's 'hermes-model' param was already correct; we only had the shared 'model' pointed at the wrong layer. Fix: pack_default_model(hermes) now returns 'us.anthropic.claude-opus-4-6-v1' (matches other bedrockify-dependent packs like pi, ironclaw, nemoclaw). PR #16 — P1: headless auth params unreachable via CFN/TF --------------------------------------------------------- Already fixed in PR #17 / v0.5.98. No action needed. PR #16 — P2: --kiro-api-key accepted flag-like values ------------------------------------------------------ Already fixed in PR #17 / v0.5.98. No action needed. PR #18 — P1: Terraform mode actually runs apply -auto-approve -------------------------------------------------------------- docs/reference/terraform.mdx and docs/reference/cli.mdx both claimed the installer 'prints terraform init/apply commands for manual execution'. That's wrong — deploy_terraform() calls terraform_init → validate → apply (with -auto-approve in terraform_apply()). Users expecting to review a plan would hit immediate infra changes. Fix: - terraform.mdx now describes two paths: installer-driven (auto-apply) and direct Terraform (plan-first loop). - cli.mdx paragraph corrected to match. PR #18 — P2: --help flag documented but not implemented -------------------------------------------------------- docs/reference/cli.mdx listed --help/-h but install.sh's top-level parser fell through to *) shift ;; — --help was silently swallowed and the installer proceeded to run. Fix: install.sh now has a real --help|-h case that prints a usage block and exits 0. The docs row was already accurate after this; kept as-is. PR #18 — P2: Terraform profile_name default was incorrect ---------------------------------------------------------- docs claimed profile_name defaults to 'builder'. It doesn't — variables.tf has no default ('# No default — must be explicitly specified'). Users following the doc would have hit interactive prompts or failed non-interactive applies. Fix: terraform.mdx table now marks profile_name as '(required)' (same treatment as environment_name, which also has no default). Verification ------------ - bash -n install.sh: OK - bash install.sh --help: prints usage + exits 0 - bash install.sh -h: prints usage + exits 0 - pack_default_model hermes → us.anthropic.claude-opus-4-6-v1 - tests/test-pack-contracts.sh: 177/0 - scripts/sync-registry --check: clean - packs/codex-cli/test.sh: 28/0 (no regression) - packs/kiro-cli/test.sh: 53/0 (no regression)
![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: 9548a3115a
ℹ️ 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".
| | Param | Flag | Default | Description | | ||
| |---|---|---|---| | ||
| | `region` | `--region` | `us-east-1` | AWS region for Bedrock | | ||
| | `model` | `--model` | `us.anthropic.claude-sonnet-4-6` | Main Bedrock model (`ANTHROPIC_MODEL`) | |
There was a problem hiding this comment.
This row documents us.anthropic.claude-sonnet-4-6 as the default, but the top-level installer currently sets Claude Code’s default via pack_default_model() to us.anthropic.claude-opus-4-6-v1 (install.sh, openclaw|claude-code case). In non-interactive/simple installs, users following this page will get a different model than documented, which can change cost/latency and fail in accounts where Opus access is not enabled.
Useful? React with 👍 / 👎.
| | `from-secret` | `--kiro-from-secret` | — | Secrets Manager id/arn whose `SecretString` is the Kiro API key | | ||
|
|
||
| <Warning> | ||
| There's a deprecated `--kiro-api-key KEY` flag still accepted for back-compat, but **don't use it** — passing secrets on argv leaks them to shell history and `/proc/<pid>/cmdline`. Always use `--kiro-from-secret`. |
There was a problem hiding this comment.
The warning says the top-level installer still accepts --kiro-api-key, but install.sh only parses --kiro-from-secret and silently drops unknown flags in the *) shift ;; branch. That means users who rely on this documented back-compat path won’t actually provision KIRO_API_KEY and can end up with broken headless installs.
Useful? React with 👍 / 👎.
Two P2 findings from chatgpt-codex-connector on PR #19. Finding 1: claude-code default model was wrong (P2) ---------------------------------------------------- pack_default_model() had 'openclaw|claude-code)' returning Opus 4.6. Claude Code's standard default is Sonnet (cheaper, faster, good for coding). Opus would triple costs and needs broader model access. Fix: split into separate cases: openclaw) → us.anthropic.claude-opus-4-6-v1 (unchanged) claude-code) → us.anthropic.claude-sonnet-4-6 (correct default) Docs (claude-code.mdx, simple-mode-defaults.mdx) already said Sonnet — now the code matches. Finding 2: --kiro-api-key advertised but not parsed (P2) --------------------------------------------------------- kiro-cli.mdx Warning block claimed the top-level installer accepts '--kiro-api-key KEY' for back-compat. It doesn't — install.sh's parser only has --kiro-from-secret; unknown flags fall through to *) shift ;;. Users following the docs would get silently broken headless installs. Fix: replaced the Warning with a Note that clarifies: only --kiro-from-secret is supported at the top level. The pack-level script (packs/kiro-cli/install.sh) still accepts --kiro-api-key with a deprecation warning, but it's not threaded through CFN/TF. Verification: bash -n install.sh: OK pack_default_model openclaw → us.anthropic.claude-opus-4-6-v1 pack_default_model claude-code → us.anthropic.claude-sonnet-4-6 test-pack-contracts.sh: 177/0 codex-cli/test.sh: 28/0 kiro-cli/test.sh: 53/0
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.
Addresses all 5 findings from chatgpt-codex-connector's automated reviews on PRs #14, #16, and #18.
Findings handled
PR #14 — P1 🐛 Hermes default model was the wrong layer
pack_default_model(hermes)returnedNousResearch/Hermes-3-Llama-3.1-8B. That flowed through CFNDefaultModel→bootstrap.sh --model→PACK_CONFIG.model. Since Hermes depends on bedrockify, bedrockify's install-daemon got the Hermes ID as its--model— but bedrockify expects a Bedrock ID. Hermes deploys would fail with model-not-found.The correct split:
model= Bedrock id (for bedrockify),hermes-model= OpenAI-style id bedrockify exposes to Hermes. The pack manifest had this right; only the top-level dispatcher was wrong.Fix:
pack_default_model(hermes)now returnsus.anthropic.claude-opus-4-6-v1(matches pi/ironclaw/nemoclaw which have the same dep chain).PR #16 — P1 & P2 (already fixed)
Both fixed in PR #17 / v0.5.98. No action here.
PR #18 — P1 🐛 Terraform docs lied about auto-apply
terraform.mdxandcli.mdxboth said 'the installer prints terraform commands for manual execution.' That's wrong —deploy_terraform()callsterraform_applywhich runsapply -auto-approve. Users expecting a review-first flow would have hit immediate infra changes.Fix: rewrote both terraform docs to describe two paths — installer-driven (auto-apply) and direct Terraform (plan-first loop).
PR #18 — P2 🐛
--helpwas documented but not implementedinstall.sh's top-level parser had no--helpcase; the flag was silently swallowed by*) shift ;;.Fix: install.sh now has a real
--help|-hcase that prints usage + exits 0. (Docs were already accurate, they're now implemented.)PR #18 — P2 🐛 Terraform
profile_namedefault was incorrectDoc claimed default
builder.variables.tfhas no default — it's required. Users following the doc would hit interactive prompts / failed unattended applies.Fix: marked as
(required)in the variables table.Verification
Cut v0.5.99 after merge.