Skip to content

[defer] fix embedded CLIProxyAPI config handling - #6877

Merged
diegosouzapw merged 3 commits into
cheaper-inference:release/v3.8.49from
professional-ALFIE:agent/fix-cliproxy-config-flag
Jul 21, 2026
Merged

diegosouzapw merged 3 commits into
cheaper-inference:release/v3.8.49from
professional-ALFIE:agent/fix-cliproxy-config-flag

Conversation

@professional-ALFIE

@professional-ALFIE professional-ALFIE commented Jul 11, 2026 •

Copy link
Copy Markdown
Contributor

What changed

  • Use CLIProxyAPI's supported --config flag when starting the embedded service.
  • Create the embedded config.yaml only when it does not already exist.

Why

OmniRoute currently starts CLIProxyAPI with -c, but CLIProxyAPI does not define that shorthand. The process exits immediately with code 2 before binding its configured port.

OmniRoute also rewrites the complete embedded config on every start. That discards user-managed settings such as remote management, API keys, and TLS configuration. Existing config files should be preserved; the minimal default is only needed for first-time initialization.

Impact

The embedded CLIProxyAPI service starts normally and keeps existing configuration across Start and Restart operations.

Validation

  • Reproduced with CLIProxyAPI 7.2.66: -c exits with code 2 and flag provided but not defined: -c.
  • Verified the same binary and config start on 127.0.0.1:8317 with --config and GET /healthz returns HTTP 200.
  • Ran resolveSpawnArgs() against an existing config and verified its contents remain unchanged.
  • Ran resolveSpawnArgs() with no config and verified the minimal default is created.
  • Ran node --import tsx/esm --test tests/unit/services/installers/cliproxy.test.ts: 6 tests passed.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the CLI proxy installer service to use the --config flag instead of -c when spawning the process. The review feedback correctly points out that, according to the repository style guide, changing production code requires corresponding unit test updates to prevent regressions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

return {
command: symlinkPath,
args: ["-c", configPath],
args: ["--config", configPath],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

According to the Repository Style Guide (Rule 9: 'Always include tests when changing production code'), any changes to production code under src/ must include corresponding test updates. Please update the unit tests in tests/unit/services/installers/cliproxy.test.ts to verify that the --config flag is correctly passed instead of -c to prevent future regressions.

References
  1. Always include tests when changing production code (src/, open-sse/, electron/, bin/). (link)

@professional-ALFIE professional-ALFIE changed the title fix embedded CLIProxyAPI config flag fix embedded CLIProxyAPI config handling Jul 11, 2026
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks for tracking this down — verified against the vendored CLIProxyAPI source (_references/_sistemas_proxys/CLIProxyAPI/cmd/server/main.go:101, flag.StringVar(&configPath, "config", ...)): there's no -c alias registered, so the root cause and the --config fix are correct. The config-preservation change also makes sense — clobbering config.yaml on every start would indeed wipe user-managed remote-management/API-key/TLS settings.

One gap before this is ready: tests/unit/services/installers/cliproxy.test.ts doesn't actually exercise resolveSpawnArgs() against the filesystem — it re-asserts hardcoded string literals (including the old ["-c", configPath] shape, just in a comment) rather than calling the real function, so it would pass identically with or without this fix. Could you add a test that imports resolveSpawnArgs with DATA_DIR pointed at a temp dir and asserts (a) the args are ["--config", configPath], (b) an existing config.yaml is left untouched, and (c) a missing one gets the default written? That gives this fix a real regression guard and should be enough to flip it out of Draft.

Separately (non-blocking): since CLIPROXYAPI_PORT can change between restarts but the config.yaml is now only written once, a port change after first install won't propagate into the persisted config — worth a one-line note or follow-up if that's a real operator flow.

@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.8.47 July 12, 2026 11:56
@diegosouzapw diegosouzapw changed the title fix embedded CLIProxyAPI config handling [defer] fix embedded CLIProxyAPI config handling Jul 12, 2026
@diegosouzapw
diegosouzapw changed the base branch from release/v3.8.47 to release/v3.8.48 July 13, 2026 05:05
@diegosouzapw
diegosouzapw changed the base branch from release/v3.8.48 to release/v3.8.49 July 13, 2026 21:57
…wnArgs (cheaper-inference#6877)

The existing cliproxy.test.ts only re-asserted string literals and never
called the real resolveSpawnArgs() against a filesystem, so it could not
have caught the -c/--config flag mismatch or the config.yaml clobbering
bug this PR fixes. Add a test that imports the real function against a
temp DATA_DIR and asserts: the spawn args always use --config (never -c),
a missing config.yaml gets the default template, and an existing
operator-customized config.yaml is left byte-identical.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw
diegosouzapw marked this pull request as ready for review July 21, 2026 17:14
@diegosouzapw
diegosouzapw self-requested a review as a code owner July 21, 2026 17:14
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Validated in local merge-train on tomni-proxmox-113 @ db6718332763af295ec8374913f1f5eb7641734f (FAST gates green: static + changed tests + vitest)

@diegosouzapw
diegosouzapw merged commit 0844163 into cheaper-inference:release/v3.8.49 Jul 21, 2026
3 checks passed
seanford added a commit to seanford/OmniRoute that referenced this pull request Jul 22, 2026
bifrost.ts resolveSpawnArgs() set BIFROST_TRANSPORT_VERSION straight from
getInstalledVersionSync(), which reads the raw "version" field out of
node_modules/@maximhq/bifrost/package.json - always bare semver per npm
convention (e.g. "1.6.3").

@maximhq/bifrost's own bin.js validates that env var against
/^v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/ or the literal "latest" and rejects
anything else with "Invalid transport version format", exiting immediately.
Every embedded Bifrost instance failed to start as a result.

Adds formatTransportVersion() to normalize at the call site that owns the
env var, so bifrost.ts and the upstream @maximhq/bifrost package both stay
exactly as designed - no changes needed to bifrost itself.

Strengthens the existing resolveSpawnArgs test to assert the actual v-prefix
format (it previously only checked for a non-empty string, the same gap
cheaper-inference#6877 called out for cliproxy's pre-existing test), and adds a dedicated
regression test file covering the pure formatTransportVersion() helper plus
a real-filesystem resolveSpawnArgs() integration check.

Found and fixed while self-hosting OmniRoute and diagnosing why bifrost
kept crash-looping on startup.
@diegosouzapw diegosouzapw mentioned this pull request Jul 23, 2026
diegosouzapw added a commit that referenced this pull request Jul 23, 2026
* fix(bifrost): send v-prefixed transport version, not bare semver

bifrost.ts resolveSpawnArgs() set BIFROST_TRANSPORT_VERSION straight from
getInstalledVersionSync(), which reads the raw "version" field out of
node_modules/@maximhq/bifrost/package.json - always bare semver per npm
convention (e.g. "1.6.3").

@maximhq/bifrost's own bin.js validates that env var against
/^v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/ or the literal "latest" and rejects
anything else with "Invalid transport version format", exiting immediately.
Every embedded Bifrost instance failed to start as a result.

Adds formatTransportVersion() to normalize at the call site that owns the
env var, so bifrost.ts and the upstream @maximhq/bifrost package both stay
exactly as designed - no changes needed to bifrost itself.

Strengthens the existing resolveSpawnArgs test to assert the actual v-prefix
format (it previously only checked for a non-empty string, the same gap
#6877 called out for cliproxy's pre-existing test), and adds a dedicated
regression test file covering the pure formatTransportVersion() helper plus
a real-filesystem resolveSpawnArgs() integration check.

Found and fixed while self-hosting OmniRoute and diagnosing why bifrost
kept crash-looping on startup.

* fix(bifrost): resolve install dir lazily so version read honors runtime DATA_DIR

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Co-authored-by: seanford <seanford@users.noreply.github.com>
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
)

* fix embedded CLIProxyAPI config flag

* preserve embedded CLIProxyAPI config

* test(services): add fs-backed regression test for cliproxy resolveSpawnArgs (cheaper-inference#6877)

The existing cliproxy.test.ts only re-asserted string literals and never
called the real resolveSpawnArgs() against a filesystem, so it could not
have caught the -c/--config flag mismatch or the config.yaml clobbering
bug this PR fixes. Add a test that imports the real function against a
temp DATA_DIR and asserts: the spawn args always use --config (never -c),
a missing config.yaml gets the default template, and an existing
operator-customized config.yaml is left byte-identical.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
…aper-inference#8194)

* fix(bifrost): send v-prefixed transport version, not bare semver

bifrost.ts resolveSpawnArgs() set BIFROST_TRANSPORT_VERSION straight from
getInstalledVersionSync(), which reads the raw "version" field out of
node_modules/@maximhq/bifrost/package.json - always bare semver per npm
convention (e.g. "1.6.3").

@maximhq/bifrost's own bin.js validates that env var against
/^v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/ or the literal "latest" and rejects
anything else with "Invalid transport version format", exiting immediately.
Every embedded Bifrost instance failed to start as a result.

Adds formatTransportVersion() to normalize at the call site that owns the
env var, so bifrost.ts and the upstream @maximhq/bifrost package both stay
exactly as designed - no changes needed to bifrost itself.

Strengthens the existing resolveSpawnArgs test to assert the actual v-prefix
format (it previously only checked for a non-empty string, the same gap
cheaper-inference#6877 called out for cliproxy's pre-existing test), and adds a dedicated
regression test file covering the pure formatTransportVersion() helper plus
a real-filesystem resolveSpawnArgs() integration check.

Found and fixed while self-hosting OmniRoute and diagnosing why bifrost
kept crash-looping on startup.

* fix(bifrost): resolve install dir lazily so version read honors runtime DATA_DIR

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Co-authored-by: seanford <seanford@users.noreply.github.com>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
)

* fix embedded CLIProxyAPI config flag

* preserve embedded CLIProxyAPI config

* test(services): add fs-backed regression test for cliproxy resolveSpawnArgs (cheaper-inference#6877)

The existing cliproxy.test.ts only re-asserted string literals and never
called the real resolveSpawnArgs() against a filesystem, so it could not
have caught the -c/--config flag mismatch or the config.yaml clobbering
bug this PR fixes. Add a test that imports the real function against a
temp DATA_DIR and asserts: the spawn args always use --config (never -c),
a missing config.yaml gets the default template, and an existing
operator-customized config.yaml is left byte-identical.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…aper-inference#8194)

* fix(bifrost): send v-prefixed transport version, not bare semver

bifrost.ts resolveSpawnArgs() set BIFROST_TRANSPORT_VERSION straight from
getInstalledVersionSync(), which reads the raw "version" field out of
node_modules/@maximhq/bifrost/package.json - always bare semver per npm
convention (e.g. "1.6.3").

@maximhq/bifrost's own bin.js validates that env var against
/^v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/ or the literal "latest" and rejects
anything else with "Invalid transport version format", exiting immediately.
Every embedded Bifrost instance failed to start as a result.

Adds formatTransportVersion() to normalize at the call site that owns the
env var, so bifrost.ts and the upstream @maximhq/bifrost package both stay
exactly as designed - no changes needed to bifrost itself.

Strengthens the existing resolveSpawnArgs test to assert the actual v-prefix
format (it previously only checked for a non-empty string, the same gap
cheaper-inference#6877 called out for cliproxy's pre-existing test), and adds a dedicated
regression test file covering the pure formatTransportVersion() helper plus
a real-filesystem resolveSpawnArgs() integration check.

Found and fixed while self-hosting OmniRoute and diagnosing why bifrost
kept crash-looping on startup.

* fix(bifrost): resolve install dir lazily so version read honors runtime DATA_DIR

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Co-authored-by: seanford <seanford@users.noreply.github.com>
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