Skip to content

stack 7/7: NIM vision classification, service repair, and the qwen3.8-max rename - #980

Merged
lidge-jun merged 10 commits into
devfrom
codex/stack7-service-vision
Aug 4, 2026
Merged

stack 7/7: NIM vision classification, service repair, and the qwen3.8-max rename#980
lidge-jun merged 10 commits into
devfrom
codex/stack7-service-vision

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Stack

7/7 — the two real-but-off-theme contributor bugs, reconstructed

Base: codex/stack6-overnight-triage (#973)

Layer 1 (#951) is merged. The overnight triage left #964 and #970 open as "real, independent, own review track". This layer is that track, plus a Qwen model rename the maintainer asked for.

Plan and evidence: devlog/_plan/260804_stack7_service_vision/ — four adversarial audit rounds are recorded in 001003, because three of them found real defects in my own designs.

#956 — NIM vision classification, and why #964 could not be carried

The nvidia entry declared no noVisionModels, so planVisionSidecar never fired and the catalog never advertised image input. Text-only NIM models either received image parts they cannot read, or had attachments blocked client-side.

#964 proposed a ~64-id text-only list. Six of its entries are natively image-capable per NVIDIA's own documentation:

Id #964 NVIDIA
thinkingmachines/inkling text-only text, image, audio
minimaxai/minimax-m3 text-only Text, Image, Video
moonshotai/kimi-k2.6 text-only text, image, video
moonshotai/kimi-k2.5 text-only text, image, video
stepfun-ai/step-3.7-flash text-only text + image (VLM)
mistralai/mistral-medium-3.5-128b text-only text + image

Listing a vision model there is a silent defect: the model could read the image, but the proxy substitutes another model's text description. No error, worse answers, extra cost. Issue #956's own body carries two of the same errors, so reporter and author shared the premise — this is not a lapse by @Yuxin-Qiao, it is what an unaudited list does.

So every id was verified individually (011_nim_id_audit.md): 26 confirmed text-only ship, 6 moved to the vision list, 32 dropped for having no current NVIDIA page. The dropped set includes nvidia/nemotron-nano-3-30b-a3b, a reversed-name typo of a real id the same list also spells correctly.

The 16 vision ids also get explicit modelInputModalities. Removing them from noVisionModels is not enough — the catalog advertises image input only for list members, so they would be published as text-only and the app would block attachments before the native path ran.

What this does not fix. Unknown ids are left unclassified deliberately. NIM publishes no modality metadata and shouldExposeRoutedModel filters only media-generation names, so embeddings, rerankers, guards and OCR endpoints reach the same code path — an unknown id carries no signal separating them from a text-only chat model. Two earlier designs of mine tried to default unknown ids and were falsified at the audit gate; the third claimed knowledge the data does not contain. A model NVIDIA ships after this snapshot still needs classifying by hand.

#970 — repair the service instead of re-registering it

ocx update stops the proxy, then brought the service back with ocx service install. The Windows scheduler installer always reaches schtasks /create, which needs elevation the updater does not have — so a normal update stopped a working proxy and could not restore its service.

repairService() already existed here, so the fix is small. The safety question the PR did not answer: repair throws when the service is not installed, and the update runs after ocx stop. Verified across all three platforms — stop never deregisters (macOS unloads the plist, Windows calls /end, Linux calls systemctl stop; deletion lives only in the uninstall paths).

Two things a plain argv swap would have missed:

  • The Windows GUI worker skipped the refresh entirely, because its own comment said /create would UAC-fail. Repair never calls /create, so that reason is gone — the skip is now narrowed to callers still passing install argv. Without this the dashboard update, the most common Windows path, keeps the bug while the CLI gets fixed.
  • bin/ocx.mjs infers service presence from a possibly-stale service-state.json. Repair correctly refuses that case, but its thrown Error is indistinguishable from any other failure there, so message-matching was unimplementable and a blanket install-on-failure would resurrect the elevation prompt. It now reads startup.serviceInstalled from the status --json subprocess it already spawns — the file is plain Node ESM and cannot import diagnoseService().

qwen3.8-max-previewqwen3.8-max

Qwen3.8-Max is stable and Alibaba documents the preview endpoint as liable to be taken offline. No alias is added: a config naming the old id still routes, it just stops carrying capability metadata keyed to a retiring id.

Pricing moves off a Routeway reseller proxy to Qwen's published $2 / $6 — the exit condition the old overlay's own comment named. Two caveats stay in the source string: the figure is Qwen's release announcement rather than an Alibaba Model Studio billing row (which has no 3.8 entry yet), and no cache rate is published anywhere, so both cache fields are 0 rather than inheriting the reseller's 0.15.

Verification

  • bun x tsc --noEmit clean; bun run privacy:scan passed
  • bun run test: 7753 pass / 8 skip / 0 fail across 508 files
  • Red-green on every new guard: reintroducing fix(providers): activate vision sidecar for NVIDIA NIM text-only models #964's kimi-k2.5 entry fails 2 tests, dropping the NIM modalities map fails 3, removing noVisionModels fails 8, restoring the unconditional Windows skip fails the GUI-repair guard, dropping any Qwen metadata key fails the rename-survival guard.

One ablation caught a test of mine that passed for the wrong reason: the Windows guard ran on macOS, so the branch never executed and removing the fix changed nothing. It needed a platform seam before it could fail honestly.

Closing #964 and #970

Both close once this is green, with comments naming these commits. @Yuxin-Qiao found a real bug and @stephen-drew diagnosed the elevation problem correctly; in both cases the finding was right and the implementation needed reworking. Reopening either is one click.

Summary by CodeRabbit

  • New Features

    • Added verified NVIDIA NIM vision support, including native image handling and vision fallback behavior for text-only models.
    • Renamed Alibaba’s Qwen model to qwen3.8-max and updated its published pricing.
  • Improvements

    • Updates now refresh existing background services with ocx service repair, preserving configuration and improving Windows support.
    • Added clearer recovery guidance when service refreshes fail.
  • Tests

    • Expanded coverage for vision routing, service repair, model migration, and pricing.

…ice repair

Two overnight contributor PRs describe real defects the #951-#973 stack does
not touch. This unit plans layer 7 as their reconstruction.

#964 cannot be carried: five ids in its hand-written text-only list are
natively image-capable per NVIDIA's own docs (inkling, minimax-m3, kimi-k2.6,
step-3.7-flash, mistral-medium-3.5-128b). A false positive there is silent —
the model can read the image, but the proxy substitutes another model's text
description. Issue #956's own body carries two of the same errors, so reporter
and author shared the premise. 010 inverts the design: maintain the 15 verified
vision-capable ids and derive text-only as the complement, so an unclassified
new model defaults to sidecar-on rather than to the bug being fixed.

#970's premise is right but its diff is oversized: repairService() and
'ocx service repair' already exist here. 020 records the safety proof that
matters — repair throws when not installed and the update path runs after
'ocx stop', but stop never deregisters on any of the three platforms. It also
closes a hole #970 leaves: bin/ocx.mjs infers service presence from a
possibly-stale marker, where repair would throw and lose the managed service.

030 sequences the bottom-up merge and issue closure, including the #954
security-review gate that can legitimately stop the queue.
The A-gate reviewer returned FAIL. Every blocker was reproduced before being
accepted; none was rebutted. 001 records the synthesis.

B1 killed my own design. I proposed maintaining the 15 vision-capable ids and
deriving text-only as their complement, and claimed an unclassified model would
default to sidecar-on. It does not — a complement over a static chat-model list
leaves an unknown id in neither list, so modelInList returns false and #956
survives verbatim:

  deepseek-ai/deepseek-v4-flash       sidecarWouldRun=true
  moonshotai/kimi-k2.6                sidecarWouldRun=false
  brandnew/model-nobody-classified    sidecarWouldRun=false

I had inverted which list is maintained while keeping the closed world — the
same lesson as the three earlier allowlist failures, reproduced while writing
the document that cites them. 010 now changes the predicate instead: default-on
for the nvidia entry with the vision list as its exception set, so a stale
exception list costs one description hop rather than reproducing the bug.

B2: removing a native-vision id from noVisionModels is not sufficient. The
catalog advertises image input only for list members, so those models would be
blocked client-side instead. They need explicit modelInputModalities.

B3: src/update/job.ts:775 skips the service refresh entirely on non-elevated
Windows — the dashboard path. Its stated reason is that schtasks /create needs
UAC, which repair does not call, so the skip must be narrowed or the reporter's
own surface stays broken.

B4: repairService throws plain Errors and bin/ocx.mjs sees only an exit status,
so 'fall back on not-installed' was unimplementable. Re-run diagnoseService()
after a failed repair instead of parsing messages.

B5: retargeting emits 'edited', which ci.yml does not listen for, so a green
check on the same head sha proves nothing about the new merge base.

030 also moves the #964/#970 closure from 'when stack 7 opens' to 'open and
green'. The earlier text borrowed a policy from the six carried PRs, which had
verified replacement commits already on a branch; this replacement does not
exist yet and its first design just failed audit.
…ler honest design

Audit round 2 closed B2/B3/B5 and returned FAIL on two P0s. Two consecutive
failures on the same surface means root cause, not a third patch of the same
shape.

R2-B2 is the one that matters: 010 flagged 'non-chat endpoints never reach the
predicate' as a thing to confirm rather than assume, and I did not confirm it.
It is false. NVIDIA has no discovery filter and shouldExposeRoutedModel rejects
only media-generation names, so embeddings, rerankers, guards and OCR all reach
planVisionSidecar:

  nvidia/nv-embedqa-e5-v5                        filteredOut=false
  nvidia/llama-3.1-nemotron-safety-guard-8b-v3   filteredOut=false
  nvidia/nemotron-ocr-v2                         filteredOut=false

Under default-on every one of them would advertise image input and burn a
sidecar call before failing upstream.

Root cause: twelve of the thirteen registry entries declaring noVisionModels
pair it with a static models list. NVIDIA is the first asked to classify over an
unbounded set, with no modality and no model-kind metadata. An unknown NIM id
therefore carries no signal separating a text-only chat model from an embedding
endpoint, and no predicate over an id string can recover information the
provider does not publish. Draft 1 kept the closed world; draft 2 escaped it but
claimed knowledge that does not exist.

The design that follows: enumerate the known text-only ids (correcting #964's
five false positives), pin the 15 verified vision ids with explicit
modelInputModalities so they become usable, leave unknown ids untouched, and
record the open-world gap as a stated limitation. Confined to registry.ts with
no predicate change, so no consumer edits — the reason this draft is
implementable where draft 2 was not.

R2-B1 also caught two consumers earlier drafts missed: web-search/index.ts:165,
and cli/models.ts:44 which uses raw .includes() instead of modelInList.

R2-B3 (bin/ocx.mjs cannot import diagnoseService from TypeScript) was found and
fixed before the verdict arrived; 020 already reads startup.serviceInstalled
from the status --json subprocess it spawns.
Audit round 3 FAIL. Three failures on one document is LOOP-DOOM territory, so
this changes the verification method the design rests on rather than patching
the design again.

R3-B1: moonshotai/kimi-k2.5 is a sixth false positive in #964's list — NVIDIA
documents GIF/JPG/PNG input, four images per prompt, with hosted image_url
examples. This is fatal to draft 3's justification, not just a missing entry.
Draft 3 argued 'for a known id the classification is real and verifiable' while
inheriting ~54 unaudited entries from #964 and calling them known. Finding a
sixth immediately after correcting five proves I never verified the remainder.
Every carried id now gets verified against NVIDIA docs or dropped; dropping
costs today's behavior, assuming costs a silent regression.

R3-B2: my registry census was wrong. Counted directly there are 17 entries
declaring noVisionModels, not 13, and the two without a static models list are
opencode-go and opencode-free — opencode-zen declares none at all. The numbers
came from an ad-hoc regex whose entry boundaries were wrong, and I wrote its
output into two documents as fact. Same failure as R2-B2, one document later.
The information-constraint argument survives and the two real exceptions
strengthen it: both classify only known ids, and opencode-free has a -free
suffix filter NVIDIA lacks.

R3-B3: test 5 asserted that a user's noVisionModels 'wins' over the registry.
mergeStringArray unions them, so a user cannot remove a registry entry. Test
now asserts additions are preserved.

R3-B4: dropped the dated snapshot test. A local date assertion has no NVIDIA
input, so it detects elapsed time rather than drift, and its cheapest CI fix is
bumping the date without auditing anything.

Also: 030 now requires #956 to close with an explicit bounded-scope statement,
and 020 records that the status probe runs only on the success path today.
… dropped

003 made per-id verification a gating step. This is that audit, run against
build.nvidia.com model pages and the NIM LLM/Visual API indexes on 2026-08-04.

#964 submitted ~64 ids. Fewer than half survive:

  26  confirmed text-only (explicit 'Input Modalities: Text') — these ship
   6  confirmed image-capable — moved to the vision list
  32  unverified or absent from NVIDIA's catalog — dropped

The 26 include z-ai/glm-5.2, deepseek-v4-flash/pro and the nemotron-3 family,
so the models issue #956 actually names are all fixed.

No seventh false positive was found, which is the first evidence the correction
has converged rather than merely advanced.

The 32 dropped are mostly delisted models — harmless in isolation, since nobody
can route to a model NVIDIA no longer serves. But the set includes
nvidia/nemotron-nano-3-30b-a3b, a reversed-name typo of the real
nvidia/nemotron-3-nano-30b-a3b which the same list also spells correctly, and
mistralai/mixtral-8x22b-v0.1 where NVIDIA documents mixtral-8x22b-instruct-v0.1.
Half the list was assembled rather than verified; the six reversed entries were
the visible damage, this is the extent of it.

Kimi is now split correctly across two independent axes: k2.5 and k2.6 join the
vision list, k2-thinking and k2-instruct stay text-only, and all four remain in
NVIDIA_NIM_KIMI_MODELS for reasoning suppression.

google/codegemma-7b verifies while google/codegemma-1.1-7b does not — adjacent
names, opposite outcomes, which is why name-based classification was rejected.
…uals

Adds 040: qwen3.8-max-preview becomes qwen3.8-max, and the price overlay moves
from a Routeway reseller proxy to Alibaba's published rate.

Alibaba released Qwen3.8-Max as stable on 2026-08-03 and documents the preview
endpoint as liable to be taken offline. Model Studio lists both ids today, so the
rename touches 10 sites in registry.ts, 3 in expected-prices.ts, and 6 test
files across both alibaba-token-plan providers.

No -preview alias is added. A config naming the old id still routes, because
routeModel accepts an arbitrary namespaced id for a configured provider and the
upstream still serves it; what such a user loses is capability metadata keyed to
a retiring preview id, which is the correct outcome.

Price: Qwen publishes $2 input / $6 output. Two honesty constraints recorded —
the figure is Qwen's own announcement and Model Studio has no qwen3.8-max row
yet, and cache rates are unpublished so both cache fields go to 0 rather than
inheriting the Routeway numbers. Carrying a reseller cache rate under a vendor
price label would be a wrong number wearing a verified badge. The Routeway
constant and its overlay are removed entirely, which is exactly the exit
condition its own comment named.

Round-4 NEAR-PASS residuals closed:
- k2.5 raised the vision set to 16 and the reversed-entry count to six; both
  numbers were stale in 000, 010. The k2.5 regression test was missing from the
  no-sidecar and emitted-modality cases and is now required in both.
- 011 claimed a delisted id 'cannot be routed to'. False: routeModel accepts
  arbitrary namespaced ids and a stale cache can surface one. The disposition
  holds for a narrower reason — exclusion leaves them at today's unclassified
  behavior — and the text now says that instead.
- The Mistral Medium 3.5 hosted-endpoint recheck is resolved; it ships.
The nvidia registry entry declared no noVisionModels, so planVisionSidecar never
fired for any NIM model and the catalog never advertised image input. A text-only
NIM model therefore either received raw image parts it cannot read, or had
attachments blocked client-side. That is issue #956.

Two verified lists, both audited per-model against NVIDIA documentation on
2026-08-04 (evidence: devlog/_plan/260804_stack7_service_vision/011_nim_id_audit.md):

  noVisionModels          26 ids — text-only, sidecar describes their images
  modelInputModalities    16 ids — natively image-capable, native path, image
                                   input advertised explicitly

PR #964 proposed ~64 text-only ids. Six are natively image-capable per NVIDIA's
own docs — inkling, minimax-m3, kimi-k2.6, kimi-k2.5, step-3.7-flash and
mistral-medium-3.5-128b — and listing those is a silent defect: the model can
read the image, but the proxy substitutes another model's text description. No
error, worse answers, extra cost. Issue #956's body carries two of the same
errors. A further 32 of #964's ids have no current NVIDIA page and are dropped
rather than assumed text-only.

The vision list also needs explicit modelInputModalities. Removing an id from
noVisionModels is not enough: the catalog advertises image input only for list
members, so a natively-capable model would be published as text-only and the
Codex app would block attachments before the native path could run.

Unclassified ids are left alone deliberately. NIM publishes no modality metadata
and shouldExposeRoutedModel filters only media-generation names, so embeddings,
rerankers, guards and OCR endpoints reach this same path — an unknown id carries
no signal separating them from a text-only chat model. Defaulting in either
direction would be a claim the data does not support.

Vision and reasoning stay independent: k2.5/k2.6 join the vision list while
k2-thinking/k2-instruct stay text-only, and all four keep reasoning suppression.

Red-green: reintroducing #964's kimi-k2.5 entry fails 2 guards; dropping the
modalities map fails 3; removing noVisionModels entirely fails 8. Restored:
23 pass / 0 fail.
…it (#970)

`ocx update` stops the proxy before replacing package files, then brought the
service back with `ocx service install`. The Windows scheduler installer always
reaches `schtasks /create`, which requires elevation the updater does not have,
so an ordinary non-elevated update stopped a working proxy and could not restore
its managed service.

serviceReinstallArgs() now returns ["service", "repair"], which rewrites the
wrapper assets and restarts the EXISTING registration without /create. The
export name is kept for out-of-module callers; serviceInstallArgs() is split out
for the paths that genuinely need to register.

Safety of the substitution: repairService() throws when the service is not
installed, and the update path runs after `ocx stop` — but stop never
deregisters on any platform. macOS unloads the plist, Windows calls /end, Linux
calls systemctl stop; deletion lives only in the uninstall paths. Verified
across all three (evidence: devlog 020).

Two things a straight argv change would have missed:

The Windows GUI worker skipped the refresh entirely (update/job.ts) because its
own comment said /create would UAC-fail. That reason does not survive repair, so
the skip is narrowed to callers still passing install argv — otherwise the
dashboard-triggered update, the most common Windows path, keeps the bug while
the CLI gets fixed.

bin/ocx.mjs infers 'a service manages this proxy' from service-state.json
existing, which can be stale. Repair correctly refuses that case, but its thrown
Error is indistinguishable from any other failure there (plain Error, inherited
stdio, generic exit status), so message-matching was unimplementable and a
blanket install-on-failure would resurrect the elevation prompt. It now reads
startup.serviceInstalled from the `status --json` subprocess it already spawns —
the file is plain Node ESM and cannot import diagnoseService() directly.

Advice strings that fire only for an INSTALLED service now say repair: cli/status,
winsw missing-binary, stale baked paths, stale scheduler assets, the launchd
older-plist and not-loaded hints. First-install and missing-unit guidance stays
install.

Red-green: restoring the unconditional Windows skip fails the new guard. Six
existing tests pinned the install argv and were updated with reasons.

245 pass / 0 fail across the service, update, winsw, doctor, status, startup and
Windows-deploy suites.
…it from the vendor

Alibaba shipped Qwen3.8-Max as a stable model and documents the preview endpoint
as liable to be taken offline once preview concludes. Model Studio lists both ids
today, so this moves the registry to the supported one across both providers:
10 sites in registry.ts, the price overlays, and 11 test files.

No -preview alias is added. A config still naming the old id keeps routing —
routeModel accepts an arbitrary namespaced id for a configured provider and the
upstream still serves it. What such a user loses is capability metadata keyed to
a retiring preview id, which is where that metadata should no longer live.

Pricing moves from a Routeway reseller proxy (1.5/5/0.15) to Qwen's published
$2 input / $6 output, which is exactly the exit condition the old overlay's own
comment named. Two caveats stay in the source string rather than being dropped:

- the figure is Qwen's release announcement, not an Alibaba Model Studio billing
  row (Model Studio still lists qwen3.7-max and qwen3-max, with no 3.8 entry);
- no cache rate is published anywhere, so both cache fields are 0 rather than
  inheriting the reseller's 0.15. A reseller number under a vendor-price label
  would be a wrong value wearing a verified badge.

Status rises to 'verified' for input/output because the vendor published them.

The intl provider's defaultModel stays qwen3.7-max — that predates this change
and renaming an id is not a licence to change which model a provider selects.

Red-green: dropping any single metadata key during the rename fails the new
survival guard. Full suite 7753 pass / 8 skip / 0 fail across 508 files.
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b8782ac6-a0dc-4567-935f-d847b27c351c

📥 Commits

Reviewing files that changed from the base of the PR and between 880d2e6 and b16a7ae.

📒 Files selected for processing (34)
  • bin/ocx.mjs
  • devlog/_plan/260804_stack7_service_vision/000_scope.md
  • devlog/_plan/260804_stack7_service_vision/001_audit_response.md
  • devlog/_plan/260804_stack7_service_vision/002_audit_response_r2.md
  • devlog/_plan/260804_stack7_service_vision/003_audit_response_r3.md
  • devlog/_plan/260804_stack7_service_vision/010_nim_vision_classification.md
  • devlog/_plan/260804_stack7_service_vision/011_nim_id_audit.md
  • devlog/_plan/260804_stack7_service_vision/020_service_repair_path.md
  • devlog/_plan/260804_stack7_service_vision/030_merge_and_close_sequence.md
  • devlog/_plan/260804_stack7_service_vision/040_qwen38_max_rename_pricing.md
  • src/cli/status.ts
  • src/lib/winsw.ts
  • src/providers/registry.ts
  • src/service.ts
  • src/update/index.ts
  • src/update/job.ts
  • src/usage/expected-prices.ts
  • tests/alibaba-intl-token-plan.test.ts
  • tests/alibaba-region-migration.test.ts
  • tests/claude-desktop-1m.test.ts
  • tests/multi-agent-compat.test.ts
  • tests/nvidia-nim-hardening.test.ts
  • tests/provider-registry-parity.test.ts
  • tests/qwen38-preserve-reasoning.test.ts
  • tests/reasoning-effort.test.ts
  • tests/router-discarded-baseurl-warning.test.ts
  • tests/service.test.ts
  • tests/subagent-model-fallback-api.test.ts
  • tests/subagent-model-fallback.test.ts
  • tests/update-job.test.ts
  • tests/update-stop-first.test.ts
  • tests/usage-cost.test.ts
  • tests/windows-deploy-close-regressions.test.ts
  • tests/winsw.test.ts
 ________________________________________________________
< Code review is a dish best served cold. Like a carrot. >
 --------------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/stack7-service-vision

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Stack navigation — 7 layers, review and merge bottom-up

Layer PR Contents
1/7 #951 merged af3ddedb4 — 22 label corrections + the plan unit
2/7 #952 long-context pricing tiers (#908)
3/7 #953 six carried contributor bug fixes, authorship intact
4/7 #954 explicit thinking disable through translation (#545)
5/7 #955 cooldown early-recovery probe (#915)
6/7 #973 overnight PR triage + fixes to #955's own defects
7/7 #980 NIM vision classification (#956), service repair (#970), qwen3.8-max rename

Each layer targets the branch below it, so its diff only makes sense on that base — enforce-target skips the wrong-base gate for stacked children by design (AGENTS.md, Branch policy). Review bottom-up; a layer cannot merge before its parent lands.

Note for the merge sequence: retargeting a child after its parent merges emits an edited event, which ci.yml does not listen for. A green check on the same head sha therefore proves nothing about the new merge base — merge current dev into the child to force a synchronize run before merging it.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d40367c0cf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/update/job.ts
Comment on lines +797 to +798
const refreshRegisters = (svcArgs ?? []).includes("install");
if ((io.platform ?? process.platform) === "win32" && process.env.OCX_SERVICE === "1" && refreshRegisters) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep repair enabled after an empty reclaim scan

When port reclaim times out but listPids returns no live holders—for example because listener inspection failed or Windows retains a ghost LISTEN row—the later liveAfter.length === 0 branch still unconditionally sets skipServiceInstall. Consequently, the new non-registering service repair command is never attempted and the updater falls back to an unmanaged direct process, losing login startup and crash recovery. Apply the refreshRegisters condition to that later skip as well, so only legacy install argv bypasses the service operation.

Useful? React with 👍 / 👎.

Comment thread src/cli/status.ts
// rather than print registration as if it were service.
const serviceSummary = service.installed && !live
? `${service.summary} — registered but NOT serving; see ${serviceLogPath()} and re-run 'ocx service install'`
? `${service.summary} — registered but NOT serving; see ${serviceLogPath()} and re-run 'ocx service repair'`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Synchronize the documented service repair workflow

The CLI now directs registered-but-unhealthy services to ocx service repair, but docs-site/src/content/docs/reference/cli/lifecycle.md still omits repair from the subcommand table and its status example says Repair: ocx service install (lines 178–192 and 229–236), with the translated lifecycle pages repeating the old workflow. Users following those pages can unnecessarily re-register the service—requiring elevation for Windows Task Scheduler and potentially changing a native backend—rather than using the repair path introduced here. Update the canonical page and affected locales to match the new command.

AGENTS.md reference: AGENTS.md:L212-L213

Useful? React with 👍 / 👎.

Comment on lines +142 to +143
{ provider: "alibaba-token-plan", modelId: "qwen3.8-max", cost4: QWEN38_MAX, source: QWEN38_MAX_PRICING, verifiedAt: "2026-08-04", status: "verified" },
{ provider: "alibaba-token-plan-intl", modelId: "qwen3.8-max", cost4: QWEN38_MAX, source: QWEN38_MAX_PRICING, verifiedAt: "2026-08-04", status: "verified" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fail closed on incomplete Alibaba pricing

These rows mark a complete verified four-part price even though the adjacent source explicitly says there is no Alibaba Model Studio billing row and cache rates are unpublished. For Qwen requests reporting cached input, the estimator consequently charges those tokens at zero and leaves estimated=false, presenting an unsupported provider-specific total as verified. Do not register these Alibaba overlays as verified until the plan's complete pricing is published; otherwise use a justified derived policy that preserves the uncertainty instead of converting unknown cache prices to zero.

Useful? React with 👍 / 👎.

Comment thread src/update/job.ts
const svcArgs = serviceInstalled ? [launcher, ...(serviceArgs ?? ["service", "install"])] : startArgs;
// Default to the non-registering refresh: an update path reaching here has an already
// installed service, and `install` would demand elevation on Windows scheduler backends.
const svcArgs = serviceInstalled ? [launcher, ...(serviceArgs ?? ["service", "repair"])] : startArgs;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Point repair failures back to the repair command

With the service command changed to repair, a nonzero exit now reaches the existing failure branch at lines 825–844, which still reports a reinstall failure and tells Windows users to run ocx service install as administrator. That reintroduces the elevation path this change avoids and, for a WinSW installation, plain install can switch the backend to Task Scheduler. Update this worker log and remedy to name ocx service repair, matching the command that actually failed.

Useful? React with 👍 / 👎.

Comment thread src/lib/winsw.ts
// A stale SCM service can outlive a deleted exe; surface the repair path.
return existsSync(winswXmlPath()) && !existsSync(winswExePath())
? "native assets present but WinSW binary missing — run 'ocx service install --native' to repair"
? "native assets present but WinSW binary missing — run 'ocx service repair'"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep install guidance when WinSW registration is absent

This branch is reached only when statusWinswRaw() returns nonexistent, which on Windows means the SCM probe explicitly confirmed that no native service is registered. ocx service repair immediately rejects that state through diag.installed === false, so the newly printed remedy cannot restore the missing binary or registration. Keep the ocx service install --native guidance here, or explicitly make repair support this absent-registration case without weakening its current fail-closed behavior.

Useful? React with 👍 / 👎.

@lidge-jun
lidge-jun merged commit 7343f0b into dev Aug 4, 2026
28 of 29 checks passed
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.

1 participant