Skip to content

[ENG-2691] Language selection 5/5 — brv config set/get + release notes + ship gate#714

Merged
danhdoan merged 1 commit into
proj/language-selectionfrom
feat/ENG-2691-cli-release
May 26, 2026
Merged

[ENG-2691] Language selection 5/5 — brv config set/get + release notes + ship gate#714
danhdoan merged 1 commit into
proj/language-selectionfrom
feat/ENG-2691-cli-release

Conversation

@danhdoan
Copy link
Copy Markdown
Collaborator

Summary

Final commit of the language-selection initiative. Ships the user-facing CLI (brv config set / brv config get) and the release notes that close #616 end-to-end.

After this lands, the feature is shippable to any release that includes proj/language-selection. The four prior PRs (#710 / #711 / #712 / #713) plus this one form the complete delivery.

Plan: `features/language-selection/plan.md` in the research repo. Spec: `tasks/commit-05-cli-release.md`.

What landed

CLI — generic project-config infrastructure (not a one-off)

  • `brv config set ` — sets a project config field in `.brv/config.json`.
  • `brv config get ` — reads it back. Returns "(not set)" for absent fields.
  • Initial keys: `language.mode` (auto|fixed) and `language.code` (ISO 639-1 from LANGUAGE_NAMES). Future project-config keys plug into the `SETTERS` / `GETTERS` dispatcher with no new oclif surface.
  • Validation:
    • `language.mode fixed` requires `language.code` set first → redirect message: "Run: brv config set language.code ". Prevents writing an on-disk config that `isBrvConfigJson` would reject on next load.
    • Unknown ISO codes rejected with a sorted supported-list (24 codes).

Domain

  • `BrvConfig.withLanguage(language?)` following the existing `with*` pattern. Used by the set command's setters.

Architecture

  • Pure dispatcher functions (`applyConfigSet` / `applyConfigGet`) separated from oclif Command classes so the validation contract is testable in isolation.

Release notes

  • Entry under `[Unreleased]` in `CHANGELOG.md` crediting Dmitriy K and linking Language selecting #616. Includes the restoration recipe for users who prefer the prior implicit-English behavior:

```bash
brv config set language.code en
brv config set language.mode fixed
```

Test plan

  • Typecheck + lint clean (pre-existing complexity warning unchanged)
  • 22 new tests — 12 on `applyConfigSet`, 5 on `applyConfigGet`, 5 on `withLanguage`
  • 242 mocha tests green across the language-selection surfaces (BrvConfig + config CLI + prompt builders + clause module + curate-session + brv-curate-tool + CJK tokenizer + language roundtrip + validate-brv-config init hook)
  • Auto-test harness 13/13 green — 7 original English cases byte-identical (no structural drift) + 6 language-selection cases (clause emission + 4 cross-language curate→query roundtrips)
  • Manual CLI smoke test — restoration recipe end-to-end:
    $ brv config set language.code ru → \"Setting saved: language.code = ru.\"
    $ brv config set language.mode fixed → \"Setting saved: language.mode = fixed.\"
    $ brv config get language.mode → \"fixed\"
    $ brv config get language.code → \"ru\"
    $ cat .brv/config.json → {... \"language\": {\"code\": \"ru\", \"mode\": \"fixed\"} ...}
    $ brv config set language.code xx → \"Unknown ISO 639-1 code 'xx'. Supported codes: ar, de, el, en, es, fi, fr, he, hi, id, it, ja, ko, nl, no, pl, pt, ru, sv, th, tr, uk, vi, zh.\"
    

Post-merge actions

Manual step — once this PR is merged and `proj/language-selection` ships to a numbered release, post on #616 with:

  1. Feature summary ("ByteRover now preserves your input language by default")
  2. Link to release notes
  3. Pointer to `features/language-selection/backlog.md` so Dmitriy can comment on prioritization (per-curate `--lang` flag, TUI panel, per-domain overrides)
  4. Do not auto-close — let Dmitriy confirm the feature works for him

Reviewer notes

  • The CLI deliberately does NOT prompt interactively when `fixed` is set without `code`. Reasoning: the redirect message tells the user the exact two-command sequence (`brv config set language.code ` then `brv config set language.mode fixed`), and the on-disk config never reaches an invalid intermediate state. Interactive prompts complicate scripting and JSON-mode automation; a clear error message is the better trade.
  • The asymmetry between `language.code` (validated against `LANGUAGE_NAMES`) and the auto-mode runtime (accepts any input language the LLM detects) is deliberate. Explicit user input deserves strong validation; inferred behavior accepts whatever language the input happens to be in.
  • `CHANGELOG.md` entry is under `[Unreleased]` — let the next-release author rename to `[X.Y.Z]` at version-bump time.

What's done

After 5 PRs spanning ENG-2687 through ENG-2691:

Feature is complete. Closes #616.

Final commit of the language-selection initiative (#616). Ships the
user-facing CLI for language preference and the release notes that close
the issue end-to-end.

New `brv config set <key> <value>` and `brv config get <key>` commands —
generic project-config infrastructure (not a one-off `brv language`
command). Today's keys: `language.mode` and `language.code`; future
project-config keys plug into the `SETTERS` / `GETTERS` dispatch with no
new oclif surface area.

CLI behavior:

- `brv config set language.mode auto | fixed` — reject `fixed` when no
  `language.code` is set, with a redirect message pointing at
  `brv config set language.code <iso>`. Prevents writing a config that
  `isBrvConfigJson` would refuse on next load.
- `brv config set language.code <iso>` — validates against the
  `LANGUAGE_NAMES` map (24 entries, no `iso-639-1` dependency). Unknown
  codes rejected with a sorted supported-list error.
- `brv config get language.<key>` — symmetric reader; returns "(not set)"
  for absent fields, or the value (in text or JSON mode).

Other:

- New `BrvConfig.withLanguage(language?)` method following the existing
  `with*` pattern. Used by the set command's setters; previously a future
  caller would have to spread BrvConfig fields by hand.
- Pure dispatcher functions (`applyConfigSet`, `applyConfigGet`) separated
  from the oclif Command class so the validation contract is testable in
  isolation.

Release notes under [Unreleased] in CHANGELOG.md crediting Dmitriy K and
including the **restoration recipe** for users who prefer the prior
implicit-English behavior:

    brv config set language.code en
    brv config set language.mode fixed

Tests (22 new):

- 12 cases on `applyConfigSet` covering: auto / fixed / mode transitions,
  code update preserves mode, English restoration recipe, rejection paths
  (fixed-without-code, unknown mode value, unknown ISO code, unknown
  config key, totally unrelated key).
- 5 cases on `applyConfigGet`: unset → undefined, both modes, both keys,
  unknown-key rejection.
- 5 cases on `withLanguage`: replace, set-when-unset, clear via undefined,
  no mutation of original, all-other-fields-preserved.

Ship gate:

- Typecheck + lint clean (pre-existing complexity warning unchanged).
- 242 mocha tests green across the affected surfaces (BrvConfig + config
  CLI + prompt builders + clause module + curate-session + brv-curate-tool
  + CJK tokenizer + language roundtrip integration + validate-brv-config
  init hook).
- Auto-test harness 13/13 green after rebuild — including the 4
  cross-language curate→query roundtrips and the 7 original English
  cases (zero structural drift).
- Manual CLI smoke test: full restoration recipe roundtrip + unknown-
  code rejection.

Post-merge action: post on #616 with feature summary, link to release
notes, and pointer to backlog.md so Dmitriy can comment on what to
prioritize next (per-curate --lang flag, TUI panel, per-domain overrides).
Don't auto-close — let Dmitriy close after confirming the feature works
for him.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026

Claude finished @danhdoan's task in 3m 50s —— View job


Code Review — [ENG-2691] Language selection 5/5

  • Read changed files
  • Compare against base branch proj/language-selection
  • Analyze CLI commands (config set / config get)
  • Review BrvConfig.withLanguage
  • Review tests
  • Review CHANGELOG entry
  • Post inline comments for issues
  • Post summary with score

Overall score: 4.5 / 5

A clean, well-tested ship gate for the language-selection initiative. The dispatcher / oclif separation is the right call: applyConfigSet and applyConfigGet are pure functions, the Command classes carry only arg parsing + IO, and the test suite exercises the dispatcher contract directly. The with* family on BrvConfig is extended consistently with the existing pattern (withSpace, withReviewDisabled, withVersion), and language is now threaded through all four siblings so a future call to any of them can't accidentally drop the field.

The two-step codemode fixed recipe is the right shape — it keeps the on-disk config matched to isBrvConfigJson's invariant (no mode: 'fixed' without code), so the validator never has to recover from a half-written state. The restoration recipe (code en + mode fixed) is honored end-to-end and has a dedicated test (set.test.ts:144-156).

Findings below; none block ship.


issue: non-null assertions in setLanguageMode (src/oclif/commands/config/set.ts:145)

{code: config.language!.code!, mode: 'fixed'} uses two ! non-null assertions that the project's CLAUDE.md explicitly forbids. The guard at line 134 proves code is set, but TS narrowing is lost across the ?. chain when re-reading the field. Easy to refactor by binding existingCode once after the guard — inline comment has the suggestion.

nit: redirect message names only step 1 (set.ts:139)

language.mode 'fixed' requires language.code to be set first. Run: brv config set language.code <iso>

A user reading literally would set the code and stop. The CHANGELOG restoration recipe is two commands; mirroring that in the error message is one extra clause — see inline.

suggestion: test the auto-mode preservation branch (test/unit/oclif/commands/config/set.test.ts:92)

setLanguageCode does mode = config.language?.mode ?? 'auto'. The fixed-mode branch of that expression is tested; the symmetric auto-mode preservation branch isn't. One quick it() locks the contract — see inline.

nit: CHANGELOG duplicates LANGUAGE_NAMES (CHANGELOG.md:10)

The explicit ISO list will drift from language-clause.ts once a new code is added. Mark for the next-release author.

question: success JSON doesn't surface auto-defaulted mode (set.ts:83-89)

When language.code <iso> is set on a fresh config we silently default mode: 'auto'. Scripts using --format json see only the input value, not the resolved language object. Worth considering enriching the JSON envelope (text stays terse).


Strengths worth calling out

  • Comments explain why, not what — the {mode: 'auto', code} "vestigial but harmless" comment (set.ts:163-167) and the isBrvConfigJson-invariant comment (set.ts:131-134) are exactly the kind of non-obvious rationale that earn their keep.
  • brv-config round-trip coverage is now exhaustive — language is verified preserved through withSpace, withoutSpace, withReviewDisabled, and withVersion. Locks down the "language quietly drops on next write" failure mode.
  • Test isolation matches CLAUDE.md "pure-function dispatcher" model — no oclif harness needed, fast tests, clean separation.
  • Error envelope structure ({code, message}) is consistent with brv settings set precedent so scripts targeting both can share parsing.

Observation (informational only)

resolveProjectRoot() walks up to any .brv (file or directory). Inside a worktree pointer, that returns the worktree path and ProjectConfigStore.read() will try <worktree>/.brv/config.json where .brv is a file — read returns the early undefined path. Pre-existing limitation shared with all ProjectConfigStore-using commands, not introduced here. Worth a follow-up issue if worktree-aware project config is on the language-selection backlog.


Comment thread src/oclif/commands/config/set.ts
Comment thread src/oclif/commands/config/set.ts
Comment thread test/unit/oclif/commands/config/set.test.ts
Comment thread CHANGELOG.md
Comment thread src/oclif/commands/config/set.ts
@danhdoan danhdoan merged commit f52cc29 into proj/language-selection May 26, 2026
6 checks passed
@danhdoan danhdoan deleted the feat/ENG-2691-cli-release branch May 26, 2026 14:19
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.

2 participants