Skip to content

development.md

Latest commit

 

History

History
309 lines (252 loc) · 15.5 KB

development.md

File metadata and controls

309 lines (252 loc) · 15.5 KB
Raw

codereview-cli Development Guide

This is the repo-local guide for codereview-cli-specific facts. Shared Open CLI Collective standards and automation remain canonical in their own repositories.

Project Overview

codereview-cli is the Open CLI Collective code-review CLI and ships the cr binary. It provides configuration and credential commands, trusted-agent inspection, dry-run and live pull-request review orchestration, inline thread response handling through cr respond, named orchestrator sessions, automatic PR-scoped reviewer cohorts, and local data lifecycle commands.

The current Go code is a Cobra command tree in internal/cmd/* with a thin cmd/cr entrypoint, shared exit-code mapping in internal/cmd/exitcode, and version plumbing in internal/version. Production review and response runtime assembly lives in internal/app; internal/appruntime contains only small command-independent helpers such as retention conversion and invocation-root resolution. Command packages keep CLI composition, rendering, config-path selection, and error mapping. Review orchestration is split across internal/pipeline, internal/reviewrun, internal/threadrespond, internal/reviewplan, internal/outbox, internal/gate, and internal/gateio. Command packages construct typed runtime requests, while internal/app owns the reusable provider, ledger, adapter, pipeline, live-run, and response-run assembly.

Architecture guardrails for LLM execution, model resolution, Git provider writes, command and review harness boundaries, inline thread lifecycle, and retention live in docs/architecture.md. The canonical first-run, rerun, fresh-session, checkpoint, and recovery sequence lives in docs/review-lifecycle.md.

The temporary architecture refactor workstream context for issue #420 lives in docs/architecture-refactor-workstream.md. While that workstream is active, extend the named acceptance harness tests when a change could regress review composition or dependency boundaries rather than cloning broad review assertions. Retire or promote the workstream context document when issue #420 completes. That temporary doc tracks #420-specific sequencing and the current child-issue harness inventory under "Harness And Guardrails".

Within internal/pipeline, the public entry points are DryRun, Live, and SelectionOnly. DryRun and Live execute the full review pipeline, while SelectionOnly runs just the selection phase with caller-owned artifact paths and no ledger or posting side effects so benchmark tooling can reuse the real selector implementation.

Review workbench preparation is location-independent. Runtime composition constructs the authenticated Git command adapter from the repository/read credential and injects it into the pipeline; workbench code derives remotes from the validated PR identity and must not resolve or inspect the invocation checkout.

Structured LLM calls in the review pipeline are durable per-task units. See docs/llm-task-artifacts.md for the artifact schema, status taxonomy, and resume invariants.

The target checkout-native review design for the "avoid context stuffing" work is documented in docs/checkout-native-review-contract.md. Treat that file as the source of truth for runtime order, reviewer-facing dossier boundaries, workbench ownership, and artifact-digest resume rules while the follow-on pipeline issues land.

Progress Logging

Non-review CLI progress now flows through one reusable component in internal/progress. The contract for this slice is:

  • progress writes to stderr only
  • JSON and text result payloads stay on stdout
  • --quiet suppresses progress only
  • lines are single-line structured records: cr progress event=<start|finish|error|info> command="..." op="..." target="..." ...

Review progress includes instant catalog-resolution, final selection, and per-reviewer assignment records. These expose IDs, winning provenance, selection reasoning, and assigned paths without logging prompt bodies.

Current command coverage is benchmark run, benchmark select, benchmark compare, data prune, data purge, config clear, me, and sessions delete.

Command packages own the command= label and stderr sink. Lower-level reuse is intentionally narrow: internal/datalifecycle accepts a tiny start/end progress interface rather than importing Cobra or root options.

Quick Commands

make build   # compile the binary
make test    # go test ./...
make test-race # repeated race checks for ledger/dbmig
make lint    # golangci-lint run
make tidy    # go mod tidy and verify go.mod is unchanged
make deps    # download and verify Go modules
make check   # tidy + fmt + lint + test + build
make clean   # remove build artifacts
make test-pi-runtime  # installed Pi runtime gate (see Pi Runtime Gate)

make snapshot runs a local GoReleaser snapshot build without publishing.

Concurrency checks

make test-race runs all tests in internal/ledger and internal/dbmig under the Go race detector with 25 repetitions. This includes concurrent ledger startup and allocation, the immediate transaction-lock DSN contract, and migration version re-checks. Repeating the existing contention tests makes timing-dependent failures more likely to surface without applying race detection to the entire suite.

The target sets test GOMAXPROCS to four with -cpu=4 and caps tests using t.Parallel at four with -parallel=4. Contention-test worker goroutines remain bounded by their own fixture counts. Packages run serially, with a five-minute timeout per package. The target requires CGO_ENABLED=1 and a C compiler, even though the SQLite driver itself is pure Go. CI runs the same target in the separate test-race job on pull requests and pushes to main, with a 20-minute job timeout. The job uses the same pinned shared test action as test, including Go version selection from go.mod and the action's default dependency/build caching. Existing required check names and branch-protection settings are unchanged; requiring the new check is a separate maintainer decision after it has run successfully.

Pi Runtime Gate

make test-pi-runtime runs the pi_rpc adapter against the installed Pi on PATH instead of the fake Pi the ordinary adapter tests use. It sets CR_PI_RUNTIME_VERSION to the Makefile's PI_RUNTIME_VERSION (currently 1.0.0, printed by make -s pi-runtime-version). Both targets fail when PI_RUNTIME_VERSION is blank or holds more than one version. When CR_PI_RUNTIME_VERSION is set, the runtime tests fail if it is blank, if Pi is missing, or if pi --version reports any other version. When it is unset, as in make test, they skip. To install the pinned runtime:

pi_version="$(make -s pi-runtime-version)" && \
  npm install -g --ignore-scripts "@earendil-works/pi-coding-agent@${pi_version}" && \
  make test-pi-runtime

The tests make no paid or external calls:

  • Each test gives Pi a fresh PI_CODING_AGENT_DIR and HOME with PI_OFFLINE=1.
  • The only model is a scripted OpenAI-compatible endpoint on loopback, with a dummy key stored in the selected directory's auth.json.
  • Reviewer tool calls run the real __pi-review-tool helper, dispatched to the test binary by TestMain.
  • Hostile instructions, extensions, and MCP servers are planted in the actually selected agent directory as well as under the fixture HOME's default .pi/agent location and in the reviewer checkout. Hostile skills are also planted under the fixture home and checkout. The tests fail if any resource reaches a provider request or runs.
  • Both structured and reviewer cases plant each selected AGENTS.md, CLAUDE.md, AGENTS.override.md, SYSTEM.md, and APPEND_SYSTEM.md independently, so context-file precedence cannot hide an uncovered input.

The Pi 1.0 prompt-resource contract is explicit: --no-context-files disables context discovery, CR's --system-prompt suppresses SYSTEM.md discovery, and --append-system-prompt with an empty argument supplies an explicit CLI source list that suppresses APPEND_SYSTEM.md discovery while contributing no text. See Pi's pinned argument parser and resource loader.

CR does not redirect PI_CODING_AGENT_DIR, HOME, or Pi's credential store to achieve isolation. Authentication, model configuration, and OAuth refresh persistence continue to use the selected directory. The ordinary subprocess test verifies that a synthetic auth-state update remains in the selected store after scratch cleanup; it does not exercise a real OAuth provider or refresh. All installed-runtime evidence is local/mock-provider evidence, not deployed validation or an operating-system sandbox claim.

The CI pi-runtime job installs the Makefile version on the Node release it pins and runs the same target. It is not a required check. To move to a new Pi release, change PI_RUNTIME_VERSION and run the target locally first.

Repo-Local Shape

  • Module: github.com/open-cli-collective/codereview-cli
  • Intended binary: cr
  • Main branch: main
  • Support scripts are documented in scripts/README.md.
  • Local workflow files: .github/workflows/ci.yml, .github/workflows/auto-release.yml, and .github/workflows/release.yml
  • Packaging identity: packaging/identity.yml
  • Current distribution status: GitHub release archives plus standard package channels declared in packaging/identity.yml.
  • Application package layering follows the active codereview implementation: command glue in internal/cmd/*, production review/response composition in internal/app, small runtime helpers in internal/appruntime, presentation in internal/view, state/config adapters in internal/config, internal/ledger, and internal/statepaths, provider/LLM adapters in their owning packages, and review posting/gating in internal/outbox, internal/gate, and internal/gateio, and response-only inline discussion handling in internal/threadrespond.

Interactive Init Notes

cr init interactive mode keeps all writes draft-local until the user commits staged changes. Reviewer setup may collect PAT or GitHub App reviewer secrets inside the reviewer-entity flow, but those values are only written to the credential store during Commit staged changes and exit. When the selected LLM auth mode is api_key, the wizard saves the non-secret profile shape and prints a follow-up cr set-credential command instead of collecting the API key inline.

Interactive secret fields stay blank even when a credential key already exists. The helper text carries the state instead: missing secrets prompt for entry, known existing secrets say blank preserves the current value and a new value replaces it, and staged values distinguish new secrets from replacement of an exact known credential ref. Init only treats an inline secret as overwrite intent when the exact credential ref is known to exist; otherwise the normal conflict check still protects against accidental name collisions.

Interactive git.host edits now route through the repository-route stage when the target profile already participates in repository_profiles routing. The wizard shows the affected routes, lets the user reconcile or remove them, and rejects preserved routes whose host no longer matches the selected profile.

Default repo builds now compile 1Password keyring backend support in by keeping keyring_nopassage but dropping keyring_no1password from shipped build tags. CI still runs an explicit keyring_no1password opt-out test path plus a CGO_ENABLED=0 smoke test so unsupported build modes fail intentionally rather than drifting unnoticed.

Use the keyring_no1password build tag for distributions that must exclude 1Password integration entirely, such as FIPS-constrained, air-gapped, or otherwise compliance-scoped builds. In that mode the named secrets-management workflow hides all 1Password backends and validation rejects 1Password-backed config up front instead of failing later at runtime.

Release Secrets

auto-release.yml passes RELEASE_TAG_TOKEN to the shared auto-release workflow as its tag-token. That credential must be separate from GITHUB_TOKEN so tag pushes retrigger release.yml, and separate from package-channel credentials. The preferred long-term shape is the GitHub App installation-token path described in the shared release standard; until that is wired through, use a narrowly scoped RELEASE_TAG_TOKEN.

TAP_GITHUB_TOKEN is only for Homebrew tap pushes from the release workflow. Do not reuse it for auto-release tag pushes.

Package-channel releases also use CHOCOLATEY_API_KEY, WINGET_GITHUB_TOKEN, and LINUX_PACKAGES_DISPATCH_TOKEN according to the shared distribution secret table.

Package Channel Release Mechanics

release.yml delegates package publishing to the shared reusable release workflow. Repo-local files declare package identity and native metadata; the shared workflow owns publish mechanics.

  • Homebrew: GoReleaser renders dist/homebrew/Casks/codereview-cli.rb with skip_upload: true; the shared Homebrew job pushes that generated cask to open-cli-collective/homebrew-tap with TAP_GITHUB_TOKEN.
  • Chocolatey: the shared Chocolatey job rewrites packaging/chocolatey/codereview-cli.nuspec from version 0.0.0 to the release version and replaces CHECKSUM_AMD64_PLACEHOLDER / CHECKSUM_ARM64_PLACEHOLDER in chocolateyInstall.ps1 before packing and pushing.
  • winget: the shared winget job resolves the release asset URLs/checksums and submits OpenCLICollective.codereview-cli with WINGET_GITHUB_TOKEN; because this is a first-time package id, packaging/identity.yml keeps packages.winget.bootstrap: true until the package exists upstream.
  • Linux packages: GoReleaser renders .deb, .rpm, and Arch .pkg.tar.zst artifacts named cr; the shared workflow dispatches package-release to open-cli-collective/linux-packages with LINUX_PACKAGES_DISPATCH_TOKEN.

make snapshot runs a local GoReleaser snapshot and then scripts/verify-package-render.sh, which asserts the rendered cask, deb/rpm/Arch artifacts, native package templates, and release secret wiring match the declared package IDs.

Shared Standards

Use these sources for shared policy. Do not copy their mechanics into this file.

Source of truth: https://github.com/open-cli-collective/cli-common/blob/main/docs/repo-layout.md
Local convenience copy, if present: `../cli-common/docs/repo-layout.md`

Source of truth: https://github.com/open-cli-collective/cli-common/blob/main/docs/ci.md
Local convenience copy, if present: `../cli-common/docs/ci.md`

Source of truth: https://github.com/open-cli-collective/cli-common/blob/main/docs/release.md
Local convenience copy, if present: `../cli-common/docs/release.md`

Source of truth: https://github.com/open-cli-collective/cli-common/blob/main/docs/distribution.md
Local convenience copy, if present: `../cli-common/docs/distribution.md`

Shared Automation

Use open-cli-collective/.github for shared action and reusable workflow implementations.

Source of truth: https://github.com/open-cli-collective/.github
Local convenience copy, if present: `../.github`

Repo Policy Summary

Keep main protected, require reviewed pull requests, keep the shared checks green, use squash merge, and delete branches after merge. For the exact branch protection and check contracts, use the shared standards above.