Skip to content

task: Update generated README to document Codex CLI - #36

Merged
BjRo merged 5 commits into
mainfrom
deliver-agentbox-thmk
Apr 8, 2026
Merged

BjRo merged 5 commits into
mainfrom
deliver-agentbox-thmk

Conversation

@BjRo

@BjRo BjRo commented Apr 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • Updates the generated README template to document Codex CLI availability alongside Claude Code
  • Adds new "Coding Tools" section with auth options (OPENAI_API_KEY, ChatGPT login) and usage commands
  • Renames "Claude Code Permissions" to "Coding Tool Permissions" covering both tools
  • Updates Settings Sync and Customization sections to reference both tools
  • Adds 4 new tests and updates 2 existing tests for full coverage

Test plan

  • TDD: tests written before implementation
  • go test ./... passes
  • golangci-lint run ./... passes
  • Automated code review

Bean: agentbox-thmk

🤖 Generated with Claude Code

BjRo added 2 commits April 8, 2026 13:59
…Code

Add Coding Tools section documenting both Claude Code and Codex CLI,
including auth options (OPENAI_API_KEY, ChatGPT login) and usage
(interactive TUI, --full-auto). Rename permissions section to cover
both tools. Update Settings Sync and Customization sections.

@BjRo BjRo left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go Code Review

Summary

Clean, well-scoped documentation-only change. The template updates are factually accurate (cross-referenced against existing devcontainer.json, Dockerfile, and codex config templates). Test coverage is thorough -- 4 new tests plus 2 updated tests covering the new section, renamed section, settings sync, and the containerEnv forwarding correction. All tests pass.

Two minor observations, neither blocking.

Verdict

LGTM

Automated review by Go Review Agent

"OPENAI_API_KEY",
"ChatGPT",
"codex",
"codex --full-auto",

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: The bare "codex" check is a weak assertion -- it matches "Codex CLI" (via substring of the uppercase variant... actually no, strings.Contains is case-sensitive so "codex" does not match "Codex"). It does match "codex --full-auto" though, so it is fully redundant with line 336. Consider either removing this entry or anchoring it to the rendered backtick form (e.g., "`codex`") to verify the standalone command reference independently of the --full-auto variant.

This is cosmetic -- the test suite is correct as-is.

"codex --full-auto",
"persist across container rebuilds",
}
for _, s := range checks {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SUGGESTION: "persist across container rebuilds" is a loose prose assertion that could silently pass or fail based on minor rewording. This is fine for a documentation template (the content is static and controlled), just noting the coupling is tighter than the structural assertions used elsewhere in this file (e.g., domain table checks). No action needed.

@BjRo

BjRo commented Apr 8, 2026

Copy link
Copy Markdown
Owner Author

Codex Code Review

Note: The Codex CLI review tool failed to connect to the OpenAI API (connection timeout after 5 retries). This review was performed manually instead.

Summary

This PR updates the generated README template (README.md.tmpl) to document Codex CLI alongside Claude Code. It is a documentation-only change — no Go logic, no GenerationConfig changes, no new FuncMap helpers. All tests pass and lint is clean.

Findings

SUGGESTION: Tests lack t.Parallel() calls

The four new test functions (TestREADME_CodingToolsSection, TestREADME_CodingToolPermissionsSection, TestREADME_SettingsSyncMentionsBothTools, TestREADME_CustomizationContainerEnvForwarding) do not call t.Parallel(). None of the existing tests in this file do either, so this is consistent with the existing style — but since these are pure render tests with no shared mutable state, they are safe to parallelize. Low priority.

Severity: SUGGESTION
Files: internal/render/readme_test.go

SUGGESTION: TestREADME_CodingToolsSection bare substring checks

Several checks in TestREADME_CodingToolsSection use bare strings.Contains for short tokens like "codex" and "ChatGPT". Per the project's testing rules, bare substring matches on short tokens can produce false positives. The check for "codex" (line 337) will match both "codex" and "codex --full-auto" and "Codex CLI", so it does not specifically validate the interactive TUI usage line. That said, the combination of all eight checks together gives reasonable confidence, and the template content is static prose rather than dynamic output. Low priority.

Severity: SUGGESTION
Files: internal/render/readme_test.go

SUGGESTION: Claude Code authentication not mentioned in Coding Tools section

The "Coding Tools" section documents Codex CLI authentication in detail (API key and ChatGPT login), but the Claude Code subsection does not mention how to authenticate. Users unfamiliar with Claude Code may wonder how to set it up. Consider adding a brief note (e.g., "Run claude and follow the authentication prompts" or mention that ANTHROPIC_API_KEY can be set). This is a documentation completeness point, not a code issue.

Severity: SUGGESTION
Files: internal/render/templates/README.md.tmpl (line 37-39)

Verdict

APPROVED — Clean, well-scoped documentation change. All new content is factually accurate against the existing devcontainer.json template (which already has OPENAI_API_KEY in containerEnv, the agentbox-codex-config volume mount, and sync-codex-settings.sh in postStartCommand), the Codex config template (approval_policy = "never", sandbox_mode = "danger-full-access"), and the Dockerfile template. Tests are thorough and follow established patterns. The challenge report's suggestion to add a containerEnv forwarding assertion was implemented as TestREADME_CustomizationContainerEnvForwarding. No blockers.


Automated review by Codex Review Agent

BjRo added 2 commits April 8, 2026 14:09
- Anchor bare "codex" substring check to backtick-rendered form "`codex`"
  to avoid redundancy with the "codex --full-auto" check
- Add t.Parallel() to four new pure-render test functions
- Add Claude Code authentication note to README template for parity
  with Codex CLI authentication docs
@BjRo

BjRo commented Apr 8, 2026

Copy link
Copy Markdown
Owner Author

Codex Code Review

Summary

Re-review of PR #36 after rework. This PR updates the generated README template to document Codex CLI alongside Claude Code. All three previous SUGGESTION-level findings have been addressed:

  1. t.Parallel() added -- All four new test functions now call t.Parallel().
  2. Bare "codex" substring anchored -- Changed to `codex` (backtick-wrapped form) so the assertion targets the rendered inline-code reference specifically, not the substring appearing in "codex --full-auto" or "Codex CLI".
  3. Claude Code authentication documented -- Added "Run claude inside the container and follow the interactive login prompts" to the Claude Code subsection in the Coding Tools section.

Verification

  • All README tests pass (go test ./internal/render/...).
  • Linter clean (golangci-lint run ./internal/render/... -- 0 issues).
  • Template content is factually accurate against existing artifacts (devcontainer.json, Codex config template, Dockerfile template).

Findings

No new findings. The rework is clean and complete.

Verdict

APPROVED -- All previous review feedback has been addressed. No remaining issues.


Automated review by Codex Review Agent

@BjRo BjRo left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go Code Review (Post-Rework)

Summary

Rework cleanly addresses all three findings from the first review:

  1. Anchored bare substring check: "codex" changed to "`codex`" in TestREADME_CodingToolsSection, matching the backtick-wrapped command reference in the template. This distinguishes the standalone command assertion from the codex --full-auto variant.
  2. Added t.Parallel(): All four new test functions now call t.Parallel().
  3. Added Claude Code authentication note: The Claude Code subsection in README.md.tmpl now has an **Authentication** line mirroring the Codex CLI auth documentation, improving symmetry.

No new issues introduced. Template content is factually accurate. Tests pass.

Verdict

LGTM

Automated review by Go Review Agent

@BjRo

BjRo commented Apr 8, 2026

Copy link
Copy Markdown
Owner Author

Codex Code Review

The change is small and the tests pass, but the new README now makes an unconditional claim about Codex's runtime permissions that is not true when the existing shared Codex config volume is reused. That documentation mismatch is the only blocking issue I found.

Review comment:

  • [P2] Qualify Codex permission claims when an existing config volume is reused — /workspace/.claude/worktrees/deliver-agentbox-thmk/internal/render/templates/README.md.tmpl:100-103
    If the user already has an agentbox-codex-config volume, sync-codex-settings.sh preserves the existing ~/.codex/config.toml instead of rewriting it, so Codex may keep a different sandbox_mode or approval_policy than the generated defaults. The new README states unconditionally that Codex runs with danger-full-access and approval_policy = "never", which is only guaranteed on first run; this should be phrased as the default generated config or note that existing Codex settings are preserved.

Automated review by Codex Review Agent

@BjRo
BjRo merged commit 56de798 into main Apr 8, 2026
3 checks passed
BjRo added a commit that referenced this pull request Apr 9, 2026
* feat: Update generated README to document Codex CLI alongside Claude Code

Add Coding Tools section documenting both Claude Code and Codex CLI,
including auth options (OPENAI_API_KEY, ChatGPT login) and usage
(interactive TUI, --full-auto). Rename permissions section to cover
both tools. Update Settings Sync and Customization sections.

* chore: Update bean agentbox-thmk pipeline state

* fix: Address PR review feedback for agentbox-thmk

- Anchor bare "codex" substring check to backtick-rendered form "`codex`"
  to avoid redundancy with the "codex --full-auto" check
- Add t.Parallel() to four new pure-render test functions
- Add Claude Code authentication note to README template for parity
  with Codex CLI authentication docs

* chore: Mark rework bean agentbox-hj35 as completed

* chore: Finalize bean agentbox-thmk pipeline state
@BjRo
BjRo deleted the deliver-agentbox-thmk branch April 9, 2026 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant