Skip to content

CODE_REVIEW_FEEDBACK.md

Latest commit

 

History

History
236 lines (157 loc) · 6.95 KB

CODE_REVIEW_FEEDBACK.md

File metadata and controls

236 lines (157 loc) · 6.95 KB
Raw

Code Review Feedback

Resolution Status

  • Fixed: ocp with no arguments prints usage without an internal zsh shift error.
  • Fixed: profile names are validated for ocp, ocp --set-default, ocp new, ocp-new, ocp copy, and ocp-copy.
  • Fixed: profile create/copy operations return non-zero instead of printing success after write failures.
  • Fixed: installer source lines are quoted and tested with install paths containing spaces.
  • Fixed: installer .zshrc edits are opt-in through --write-zshrc.
  • Fixed: pass-through argument behavior is tested without shell string evaluation for quoted arguments.
  • Fixed: ocp list, ocp current, ocp doctor, ocp new, and ocp copy are implemented and documented.
  • Fixed: the README uses the public jleifeld/opencode-profiles repository URL.

Findings

Medium: ocp Without Arguments Emits A Shell Error

src/ocp.zsh shifts before validating that a profile argument exists.

Current behavior:

ocp:shift:17: shift count must be <= $#
usage: ocp <profile> [opencode args...]

Expected behavior: show usage without an internal zsh error.

Recommended change:

  • Add a Bats test for ocp with no arguments.
  • Check (( $# == 0 )) before assigning profile and calling shift.

Medium: Profile Names Can Escape The Profiles Directory

Profile names are used directly as path segments in ocp, ocp-new, and ocp-copy.

Examples that should be rejected:

ocp-new ../outside
ocp ../../somewhere
ocp-copy work ../copy

Recommended change:

  • Add _ocp_valid_profile_name.
  • Reject empty names, ., .., names containing /, and names containing NUL-like unsafe input.
  • Add tests for rejected profile names in ocp, ocp-new, ocp-copy, and --set-default.

Medium: Copy And Create Commands Do Not Check All Write Failures

ocp-copy prints success even if cp -R fails. ocp-new also assumes directory and file writes succeeded.

Recommended change:

  • Use command cp -R "$src_root" "$dst_root" || return 1.
  • Check mkdir and config-file writes in ocp-new.
  • Add tests for failed writes where feasible.

Low: Installer Writes An Unquoted Source Line

The installer writes this style of line to .zshrc:

source /path/to/opencode-profiles/src/ocp.zsh

This breaks if the install path contains spaces.

Recommended change:

  • Write a quoted zsh source line.
  • Escape embedded double quotes and backslashes in the generated path.
  • Add an installer test using an OCP_INSTALL_DIR with spaces.

Low: Installer Modifies .zshrc By Default

For a public installer, automatically modifying .zshrc can surprise users.

Recommended change:

  • Consider making the default installer copy files and print the source line.
  • Add --write-zshrc or --modify-zshrc for automatic edits.
  • Keep --dry-run for previewing behavior.

Low: README Uses Placeholder Repository URL

The install docs still use:

git clone https://github.com/<owner>/opencode-profiles.git

Recommended change:

  • Replace <owner> once the GitHub repository owner is known.

Low: Tests Do Not Cover Argument Quoting

The tests cover simple argument forwarding but not arguments containing spaces, quotes, or shell-sensitive characters.

Recommended change:

  • Add tests for passing arguments like "hello world" through ocp.
  • Avoid helper patterns that rely on shell string evaluation when testing argument preservation.

User Review

What Is Understandable

  • The core idea is clear: ocp runs OpenCode with a selected profile.
  • ocp-new work is easy to understand as the first command.
  • ocp work is short and convenient for daily use.
  • ocp work --set-default makes sense once the user understands that plain opencode is wrapped.
  • ocp --reset-default is discoverable and consistent with the default-profile feature.

What Is Unclear

  • It is not obvious that this project works by sourcing a zsh file, not by installing a standalone executable.
  • It is not immediately clear that opencode itself becomes a shell function wrapper after installation.
  • ocp work --help is ambiguous: users may expect help for ocp, but it actually forwards --help to OpenCode because work is treated as the profile.
  • ocp-new and ocp-copy feel like separate commands, while users may expect subcommands such as ocp new work and ocp copy work personal.
  • The README explains the profile layout but does not show where users should put agents, commands, and plugins after creating a profile.
  • There is no command to list available profiles.
  • There is no command to show the current default profile.
  • There is no doctor/debug command to explain why a profile is invalid.

CLI Improvements To Consider

Add Discovery Commands

The most useful additions would be:

ocp list
ocp current
ocp doctor [profile]

Benefits:

  • ocp list helps users discover profile names.
  • ocp current shows which profile plain opencode will use.
  • ocp doctor work can explain missing files instead of only saying the profile is invalid.

Consider Subcommands For Management

Current management commands:

ocp-new work
ocp-copy work personal

Alternative:

ocp new work
ocp copy work personal

This makes the CLI feel more cohesive. The current command names can stay as aliases if backward compatibility matters after public release.

Make Help More Explicit

The help output should explain pass-through behavior:

usage:
  ocp <profile> [-- <opencode args...>]
  ocp <profile> --set-default
  ocp --reset-default
  ocp --help

Using -- as an optional separator would make this clearer:

ocp work -- --help

That avoids confusion between ocp flags and OpenCode flags.

Improve First-Run Feedback

After ocp-new work, print next steps:

created isolated profile: work
path: ~/.config/opencode-profiles/work

Next steps:
  ocp work
  ocp work --set-default

This would make onboarding smoother.

Improve Invalid Profile Errors

Current error:

profile not found or missing OpenCode config: <path>

Better error:

profile is invalid: work
missing config file: opencode.jsonc or opencode.json
expected config dir: .opencode/

This reduces the need to inspect the README when something goes wrong.

Clarify The Default Profile Model

The default profile feature is useful, but it is conceptually subtle because it changes plain opencode behavior.

README and help should state clearly:

  • ocp <profile> always runs one profile for one command.
  • ocp <profile> --set-default changes future plain opencode calls.
  • ocp --reset-default restores plain opencode behavior.

Recommended Next Pass

  1. Fix no-argument handling.
  2. Add profile-name validation.
  3. Make write operations fail safely.
  4. Add ocp list and ocp current.
  5. Improve help text and README around pass-through args and the opencode wrapper.
  6. Decide whether to keep ocp-new and ocp-copy or move to ocp new and ocp copy before the first public release.