Skip to content

feat: support environment-based BYOK credentials - #307

Open
jzlikewei wants to merge 3 commits into
mainfrom
feat/byok-environment-credentials
Open

jzlikewei wants to merge 3 commits into
mainfrom
feat/byok-environment-credentials

Conversation

@jzlikewei

@jzlikewei jzlikewei commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Change

BYOK API keys can now stay in the launch environment while config.yaml stores 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

  • Reviewed revision: d4db8c68c05a798021d696a87e280662ce843498.
  • macOS arm64, Node.js 22.22.2: pnpm verify full 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.
  • The review follow-up passed 197 focused tests across five configuration/model-service files and 108 synthetic scalar round trips. Ten added regression cases cover numeric/scalar key updates and deletion, merge precedence, YAML 1.1 alias/header preservation, quoted numeric-looking strings and rejection of mismatched output. The two reported reproductions also pass through updateLocalByokConfig and updateLocalModelSelection.
  • Focused configuration, credential, logger and model-service tests passed; offline BYOK tests exercise the built CLI with environment references, key rotation, missing-variable rejection, retained references and authenticated requests to the local protocol fixture.
  • At final revision d4db8c6, a real interactive PTY run of the public dist/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.
  • Gitleaks 8.24.3: complete public Git history, reviewed source snapshot and built distribution passed. Public source inventory and standalone-boundary checks passed.
  • The final head's CodeQL summary still reports existing alert #68, js/insufficient-password-hash, at packages/local-runtime-v2/src/service/model-system/catalog/model-cache.ts:62. The alert API reports the same location on current main bcb1d96553e06bf2a0d44d06f40ac79350696465 and this PR head d4db8c68c05a798021d696a87e280662ce843498; 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.
  • Performance: basic. The 100-round run for final head passed with a PASS measurement for duration, CPU and RSS across all three comparison pairs. This changes credential/config handling and log redaction; no history processing, session storage, streaming or tool execution changes.
  • NOT RUN locally: Linux/Windows acceptance, live MiniMax API, and live ACP model requests. ACP shares the verified resolver and its existing adapter tests remain in the full profile. Desktop is outside this repository's scope.

Publication and contribution checks

  • The maintainer-requested port stays within the existing first-party package licenses, and preserves existing notices. New helpers and tests are reviewed shared-source additions.
  • No credentials, account data, real user content, internal source history or private review material is included.
  • Added source files were reviewed before regenerating release/public-source.json; new Vitest files are registered in test/vitest-suites.json.
  • English user documentation and bundled product guidance describe the supported syntax and launch environment. Offline fixtures and live-provider validation are distinguished.

Maintainer handoff

Publication scope or license changes: none. The dependency license inventory was regenerated from the locked dependencies; yaml was 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 d4db8c6 should be retained during subsequent source synchronization.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with 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 local pnpm verify profile 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.

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 1anZhang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines +85 to +87
Alias(_key, alias) {
const value: unknown = alias.toJS(document);
const node = createConfigNode(document, value);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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
@hetaoBackend hetaoBackend added enhancement New feature or request cli Standalone mcode: TUI, headless, ACP and source builds/tooling labels Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Standalone mcode: TUI, headless, ACP and source builds/tooling enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants