Conversation
Resolve complete environment references across provider requests and connection checks, preserve references and comments during config writes, isolate YAML aliases and inherited credential deletion, and redact runtime credentials. Preserve standalone strict-config parsing and public service adaptations. Document terminal setup and cover credential rotation through the built CLI. Assisted-by: codex-cli reason:byok-tui-capability-port
1anZhang
left a comment
There was a problem hiding this comment.
Reviewed aea5bf77d2076260b0d1d7a98b89409961776139 against ed1f927144135f9d4c4f8a26c1d319e031d7b899. I found two configuration-write regressions, detailed inline. Both were reproduced with synthetic configuration through the actual configuration-write APIs.
Validation on macOS arm64 / Node.js 22.22.2: 685 focused tests across 22 files passed; pnpm build, pnpm test:byok (3/3), and the diff whitespace check passed. These passing checks do not cover the two cases below. Full local verification and live-service acceptance were not rerun.
| if (edit.kind === 'delete') { | ||
| document.deleteIn(edit.path); | ||
| } else { | ||
| document.setIn(edit.path, edit.value); |
There was a problem hiding this comment.
[P2] Preserve numeric mapping keys when applying edit paths
collectConfigEdits produces string path segments from JavaScript object keys, while the YAML AST retains the type of an unquoted numeric key. For an existing provider named 123 written as custom_provider: { 123: ... }, setIn therefore creates a new string key "123" instead of updating the numeric key 123.
I reproduced this through updateLocalByokConfig: changing only the provider's API key returns success but leaves both 123: and "123": in the persisted file. Reloading it with the application's js-yaml loader fails with duplicated mapping key. The previous yaml.dump writer preserves a valid configuration for the same update.
Please match edit paths to the existing YAML key nodes before applying changes, and validate the serialized result with the configuration loader before replacing the file.
There was a problem hiding this comment.
Fixed in d4db8c6. Update/delete paths now resolve to the existing scalar key nodes, and merge materialization uses the same key matching. The serialized result is strictly reloaded with the application's js-yaml loader and compared with the intended configuration before the file is replaced.
Added coverage for updating/deleting numeric, boolean and null keys, numeric merge overrides, and your reproduction through updateLocalByokConfig. Updating provider 123 now preserves one mapping entry, its comment, and a configuration that reloads successfully.
Validated at current head c4f6076: all 100 tests in the three affected configuration suites passed.
| Alias(_key, alias) { | ||
| const value: unknown = alias.toJS(document); | ||
| const node = createConfigNode(document, value); |
There was a problem hiding this comment.
[P2] Materialize aliases using the configuration loader's scalar semantics
The before/after objects come from js-yaml, but alias.toJS(document) uses the yaml package and honors the source document's %YAML 1.1 directive. These parsers disagree on scalar values: the configuration loader retains on as a string, while this alias conversion resolves it to boolean true.
With two providers sharing an options anchor containing headers: { X-Feature: on }, I reproduced this through updateLocalModelSelection: changing only the default model rewrites the aliased provider's header to X-Feature: true. The original provider remains unchanged, and the base writer preserves the string "on" for both providers. This silently changes a request header during an unrelated settings save.
Please preserve the configuration loader's scalar semantics when parsing and materializing aliases so that unrelated values survive the write unchanged.
There was a problem hiding this comment.
Fixed in d4db8c6, with an additional multiline-string fix in c4f6076. Scalar values are reconciled with the application's js-yaml interpretation before aliases and merges are expanded. Emission preserves strings that could otherwise be interpreted as booleans or numbers, and the final loader check rejects any semantic mismatch before writing.
The regression through updateLocalModelSelection now preserves X-Feature: on as a string for both providers under %YAML 1.1, including the quoted numeric-looking header "0b10". Additional cases cover aliases, merges, dates, explicit tags, and multiline plain strings with folded line breaks.
The complete local pnpm verify run on c4f6076 passed all 14 applicable gates on macOS arm64 / Node.js 22.22.2. These are local verification results; they do not imply that all GitHub CI checks have passed.
Resolve configuration edits against existing typed mapping keys, normalize AST scalars with the application loader before expanding aliases, and reject serialized values that differ from the intended configuration before writing. Cover both review reproductions through actual configuration APIs, plus scalar-key deletion, merge overrides, loader semantics and output validation. Assisted-by: codex-cli reason:pr-307-review-fixes
Quote already folded multiline scalar sources before reparsing them, so unrelated settings can be saved without changing line-break semantics. Add regression coverage for tagged strings and aliases with and without a YAML 1.1 directive, and document the persistence behavior. Assisted-by: codex-cli reason:pr-307-multiline-scalar-fix
Change
BYOK API keys can now stay in the launch environment while
config.yamlstores a complete${NAME}reference. TUI, exec and ACP use the shared resolver; saved-provider discovery, connection tests, masked views and connection-status fingerprints follow the resolved credential. Missing or blank variables report the provider, field and variable name before inference.Config writes preserve comments and credential references. Shared YAML aliases are materialized before edits, so updating one provider cannot modify another and clearing an inherited key stays cleared after reload. Edits match existing typed YAML keys, and alias expansion uses the application loader's scalar semantics. Before replacing the file, the writer reloads the serialized document and checks that it matches the intended values. The public distribution's strict handling of malformed configuration remains intact. Runtime logs and BYOK errors share credential redaction without hiding token usage counters.
This is a selective shared-source capability port with public adaptations retained. It adds terminal setup documentation and product-skill guidance, registers synthetic regression tests, and regenerates the source/export mappings and dependency license inventory. The full-source extraction baseline is unchanged.
Validation
d4db8c68c05a798021d696a87e280662ce843498.pnpm verifyfull profile — PASS, all 14 applicable gates. The capability suite passed 4,387 tests across 165 files (14 intentionally skipped); the built-CLI BYOK, smoke, policy, sandbox and status-contract checks also passed.updateLocalByokConfigandupdateLocalModelSelection.d4db8c6, a real interactive PTY run of the publicdist/cli.js, using DeepSeek V4 Flash through an environment reference, returned the expected arithmetic answer and exited successfully. The reference and original user configuration remained unchanged; the generated test files and terminal output contained no plaintext key. Private run artifacts are excluded.js/insufficient-password-hash, atpackages/local-runtime-v2/src/service/model-system/catalog/model-cache.ts:62. The alert API reports the same location on current mainbcb1d96553e06bf2a0d44d06f40ac79350696465and this PR headd4db8c68c05a798021d696a87e280662ce843498; this file has no diff in the PR. The SHA-256 value is the existing connection-status/config revision fingerprint. The baseline alert remains open for security review; it has not been suppressed or dismissed.Publication and contribution checks
release/public-source.json; new Vitest files are registered intest/vitest-suites.json.Maintainer handoff
Publication scope or license changes: none. The dependency license inventory was regenerated from the locked dependencies;
yamlwas already present and is now also a runtime dependency of the config package.Shared-source port: complete for the selected BYOK credential/config/logging capability. The full-source synchronization baseline was intentionally retained. The YAML review fixes in
d4db8c6should be retained during subsequent source synchronization.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Multiline scalar follow-up
Preserve folded line breaks in multiline plain YAML strings when unrelated settings are saved. Added regression cases for tagged values and aliases, with and without a YAML 1.1 directive.
Validation on
c4f6076: 100 tests in the three affected configuration suites passed; the complete localpnpm verifyprofile passed all 14 applicable gates. Full-history and built-distribution Gitleaks scans passed with the repository's existing CI configuration. This follow-up did not repeat live-provider or interactive TUI testing.