stack 7/7: NIM vision classification, service repair, and the qwen3.8-max rename - #980
Merged
Conversation
…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.
📝 WalkthroughWalkthroughThe PR changes update-time service handling from reinstall to repair, adds structured service-state probing, classifies NVIDIA NIM model modalities, and renames Alibaba’s Qwen model identifier. It also updates Qwen pricing, diagnostics, planning records, and regression tests. ChangesService repair flow
NVIDIA NIM vision classification
Qwen catalog and pricing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateWorker
participant ServiceRepair
participant StatusJson
participant Proxy
UpdateWorker->>ServiceRepair: execute service repair
ServiceRepair-->>UpdateWorker: success or failure
UpdateWorker->>StatusJson: inspect startup.serviceInstalled
StatusJson-->>UpdateWorker: registered or absent
UpdateWorker->>ServiceRepair: install only when absent
UpdateWorker->>Proxy: direct start after non-absence failure
sequenceDiagram
participant ModelCatalog
participant NIMRegistry
participant VisionRouter
participant VisionSidecar
ModelCatalog->>NIMRegistry: provide model identifier
NIMRegistry-->>ModelCatalog: provide modality metadata
ModelCatalog->>VisionRouter: route image request
VisionRouter->>VisionSidecar: use sidecar for verified text-only model
VisionRouter-->>ModelCatalog: bypass sidecar for native vision model
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 4, 2026
Owner
Author
Stack navigation — 7 layers, review and merge bottom-up
Each layer targets the branch below it, so its diff only makes sense on that base — Note for the merge sequence: retargeting a child after its parent merges emits an |
There was a problem hiding this comment.
💡 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 on lines
+797
to
+798
| const refreshRegisters = (svcArgs ?? []).includes("install"); | ||
| if ((io.platform ?? process.platform) === "win32" && process.env.OCX_SERVICE === "1" && refreshRegisters) { |
There was a problem hiding this comment.
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 👍 / 👎.
| // 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'` |
There was a problem hiding this comment.
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" }, |
There was a problem hiding this comment.
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 👍 / 👎.
| 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; |
There was a problem hiding this comment.
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 👍 / 👎.
| // 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'" |
There was a problem hiding this comment.
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 👍 / 👎.
2 tasks
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/ocx.mjs (1)
277-329: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWait for a serving proxy before accepting service repair success.
status --jsoncan reportstartup.serviceViable === truewhile bothproxy.runningandproxy.health.okare false. This branch setsneedDirectStartonly when both the proxy and the manager are non-viable. The update then exits with no proxy serving.
src/update/job.tstreats manager viability as insufficient and waits for the proxy health check. Apply the same bounded health wait here. If the proxy does not serve before the timeout, start the direct fallback.Proposed fix
- if (!proxyUp && !viable) needDirectStart = true; + if (!proxyUp) { + // Poll status/health for a bounded interval before direct fallback. + // A viable manager can still fail to launch a serving proxy. + needDirectStart = true; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/ocx.mjs` around lines 277 - 329, Update the service-refresh validation in the refresh flow around serviceRefreshArgs so manager viability alone cannot clear needDirectStart. Reuse the bounded proxy health-wait behavior from src/update/job.ts, requiring the proxy to become serving before accepting refresh success; if the wait times out or health remains false, preserve the direct detached proxy fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260804_stack7_service_vision/020_service_repair_path.md`:
- Around line 118-120: Add a shared inter-process lock covering the repair flow,
including the fallback status read and service install, and acquire the same
lock in ocx service uninstall. Hold the lock across the status-to-install
decision and installation so uninstall cannot remove the registration or marker
between those operations.
In `@devlog/_plan/260804_stack7_service_vision/030_merge_and_close_sequence.md`:
- Line 83: Update the prose references on lines 83, 110, and 114 so each issue
ID is wrapped in bold Markdown text, preserving the references while ensuring
the lines no longer begin with an unspaced hash.
In `@devlog/_plan/260804_stack7_service_vision/040_qwen38_max_rename_pricing.md`:
- Around line 35-48: Correct the rename audit inventory in the documented
section: change the src/providers/registry.ts occurrence count to match all
listed references, add tests/alibaba-region-migration.test.ts and
tests/multi-agent-compat.test.ts to the test list, and update
src/usage/expected-prices.ts locations to the source at line 68 and overlays at
lines 142-143.
In `@src/update/index.ts`:
- Around line 336-339: Update the CLI repair-success condition in runUpdate() to
also call the serving-health probe used by the GUI path, serviceRestartServed(),
against capturedListen.port before skipping the direct proxy fallback. Require
the service to be viable and actively serving on that captured port; otherwise
preserve the existing fallback behavior.
In `@src/update/job.ts`:
- Around line 789-799: Update the failure guidance in the relevant updateJob
flow to derive the displayed service operation from refreshRegisters: use
“install” only when explicit install arguments are present, and use “repair” for
the default path. Replace the hard-coded “Service reinstall failed” wording and
ocx service install guidance while preserving the existing behavior for legacy
install callers.
In `@tests/alibaba-intl-token-plan.test.ts`:
- Around line 72-95: Extend the rename regression test around the existing
PROVIDER_REGISTRY assertions to verify qwen3.8-max-preview is absent from
modelInputModalities, modelReasoningEfforts, and modelDefaultReasoningEfforts
for both Alibaba providers, while confirming the stable qwen3.8-max entries
retain their metadata. Also assert that alibaba-token-plan-intl.defaultModel
remains qwen3.7-max, preserving the existing Beijing default assertion.
In `@tests/update-job.test.ts`:
- Around line 388-437: Extend the test around restartAfterUpdateForTests to
assert that the direct-start fallback was not invoked: after the existing
ranService repair assertions, verify the spawned array is empty. Keep the test
focused on the non-elevated Windows service-repair path.
In `@tests/update-stop-first.test.ts`:
- Around line 58-72: Strengthen the regression coverage for service reinstall
behavior: add a focused assertion near the service-related tests in
service.test.ts that directly verifies serviceReinstallArgs() returns the repair
argument vector, rather than only checking that updateSource references the
helper. Preserve the existing update-stop-first assertions and ensure the test
would fail if the helper returned service install.
In `@tests/winsw.test.ts`:
- Around line 260-262: Add a separate isolated test for serviceInstallArgs()
that records an existing native service installation and asserts ["service",
"install", "--native"]. Keep the current scheduler-default assertion in its own
test, and use the existing test setup/state helpers to avoid cross-test
contamination.
---
Outside diff comments:
In `@bin/ocx.mjs`:
- Around line 277-329: Update the service-refresh validation in the refresh flow
around serviceRefreshArgs so manager viability alone cannot clear
needDirectStart. Reuse the bounded proxy health-wait behavior from
src/update/job.ts, requiring the proxy to become serving before accepting
refresh success; if the wait times out or health remains false, preserve the
direct detached proxy fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b8782ac6-a0dc-4567-935f-d847b27c351c
📒 Files selected for processing (34)
bin/ocx.mjsdevlog/_plan/260804_stack7_service_vision/000_scope.mddevlog/_plan/260804_stack7_service_vision/001_audit_response.mddevlog/_plan/260804_stack7_service_vision/002_audit_response_r2.mddevlog/_plan/260804_stack7_service_vision/003_audit_response_r3.mddevlog/_plan/260804_stack7_service_vision/010_nim_vision_classification.mddevlog/_plan/260804_stack7_service_vision/011_nim_id_audit.mddevlog/_plan/260804_stack7_service_vision/020_service_repair_path.mddevlog/_plan/260804_stack7_service_vision/030_merge_and_close_sequence.mddevlog/_plan/260804_stack7_service_vision/040_qwen38_max_rename_pricing.mdsrc/cli/status.tssrc/lib/winsw.tssrc/providers/registry.tssrc/service.tssrc/update/index.tssrc/update/job.tssrc/usage/expected-prices.tstests/alibaba-intl-token-plan.test.tstests/alibaba-region-migration.test.tstests/claude-desktop-1m.test.tstests/multi-agent-compat.test.tstests/nvidia-nim-hardening.test.tstests/provider-registry-parity.test.tstests/qwen38-preserve-reasoning.test.tstests/reasoning-effort.test.tstests/router-discarded-baseurl-warning.test.tstests/service.test.tstests/subagent-model-fallback-api.test.tstests/subagent-model-fallback.test.tstests/update-job.test.tstests/update-stop-first.test.tstests/usage-cost.test.tstests/windows-deploy-close-regressions.test.tstests/winsw.test.ts
Comment on lines
+118
to
+120
| Instead: after a failed repair, consult **structured state** and install only | ||
| when it reports the service genuinely absent while the managed-service marker | ||
| still expresses intent. State beats error-message parsing. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 \
'service-state\.json|diagnoseService|serviceInstalled|uninstall|service install' \
bin src/service.ts src/cli/status.tsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- bin/ocx.mjs update service fallback ---'
sed -n '130,205p' bin/ocx.mjs
printf '%s\n' '--- possible lock/serialization primitives ---'
rg -n -C 4 \
'lock|mutex|flock|serviceWasInstalled|serviceInstallArgs|diagnoseService|removeServiceInstallState|function installService|case "install"|ops\.install|serviceStatePath' \
bin/ocx.mjs src/service.ts
printf '%s\n' '--- service command install/uninstall ---'
sed -n '2515,2650p' src/service.ts
printf '%s\n' '--- plan text ---'
sed -n '100,130p' devlog/_plan/260804_stack7_service_vision/020_service_repair_path.mdRepository: lidge-jun/opencodex
Length of output: 37193
Add serialization around the status-to-install fallback.
bin/ocx.mjs:293-295 performs the status read and service install as separate operations. ocx service uninstall has no shared lock with this path. It can remove the registration and marker after the status read, so the fallback can reinstall a service that the user just removed.
Use a shared inter-process lock for repair, install, and uninstall. A second state read alone does not close the race between that read and service install.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260804_stack7_service_vision/020_service_repair_path.md` around
lines 118 - 120, Add a shared inter-process lock covering the repair flow,
including the fallback status read and service install, and acquire the same
lock in ocx service uninstall. Hold the lock across the status-to-install
decision and installation so uninstall cannot remove the registration or marker
between those operations.
|
|
||
| ### Contributor PRs to close as superseded | ||
|
|
||
| #964 and #970 close once stack 7 is open **and green** — not the moment it opens. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the malformed issue references.
Markdownlint reports MD018 because Lines 83, 110, and 114 start with #<number> without a space. These lines are prose, not headings. Wrap the issue IDs in bold text to preserve the issue references and remove the warnings.
Proposed fix
-#964 and `#970` close once stack 7 is open **and green** — not the moment it opens.
+**`#964`** and **`#970`** close once stack 7 is open **and green** — not the moment it opens.
-#961 (enhancement, provider headers), `#966` (two falsifications survive), `#969`
+**`#961`** (enhancement, provider headers), **`#966`** (two falsifications survive), **`#969`**
(CI governance policy), `#922`, `#928`, `#935`, `#936`, `#940`, `#557` — each already carries
-#907 stays blocked on `lidge-jun/jawcode`:
+**`#907`** stays blocked on `lidge-jun/jawcode`:Also applies to: 110-110, 114-114
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 83-83: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260804_stack7_service_vision/030_merge_and_close_sequence.md` at
line 83, Update the prose references on lines 83, 110, and 114 so each issue ID
is wrapped in bold Markdown text, preserving the references while ensuring the
lines no longer begin with an unspaced hash.
Source: Linters/SAST tools
Comment on lines
+35
to
+48
| `src/providers/registry.ts` — 10 occurrences: the two model lists (`:355`, | ||
| `:359`, `:375`, `:382`), input modalities (`:362`, `:453`), `defaultModel` | ||
| (`:1424`), context windows `983_616` (`:1430`, `:1459`), reasoning efforts | ||
| (`:1468`), `modelDefaultReasoningEfforts` (`:1481`), and | ||
| `preserveReasoningContentModels` (`:1440`, `:1478`). | ||
|
|
||
| `src/usage/expected-prices.ts` — the two overlay rows (`:137`, `:138`) plus the | ||
| source constant (`:62-63`). | ||
|
|
||
| Tests naming the old id: `tests/alibaba-intl-token-plan.test.ts`, | ||
| `tests/qwen38-preserve-reasoning.test.ts`, `tests/claude-desktop-1m.test.ts`, | ||
| `tests/subagent-model-fallback-api.test.ts`, | ||
| `tests/router-discarded-baseurl-warning.test.ts`, | ||
| `tests/provider-registry-parity.test.ts`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the audit call-site inventory.
The section says src/providers/registry.ts has 10 occurrences, but the listed locations total 13. The supplied changes also include tests/alibaba-region-migration.test.ts and tests/multi-agent-compat.test.ts, which are missing from the test list. The src/usage/expected-prices.ts references are stale: the supplied final file places the source at Line 68 and the overlays at Lines 142-143.
Update this index before using it as the rename audit checklist.
Proposed documentation fix
-`src/providers/registry.ts` — 10 occurrences
+`src/providers/registry.ts` — 13 occurrences
-`src/usage/expected-prices.ts` — the two overlay rows (`:137`, `:138`) plus
- the source constant (`:62-63`).
+`src/usage/expected-prices.ts` — the two overlay rows (`:142`, `:143`) plus
+ the source constant (`:68`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260804_stack7_service_vision/040_qwen38_max_rename_pricing.md`
around lines 35 - 48, Correct the rename audit inventory in the documented
section: change the src/providers/registry.ts occurrence count to match all
listed references, add tests/alibaba-region-migration.test.ts and
tests/multi-agent-compat.test.ts to the test list, and update
src/usage/expected-prices.ts locations to the source at line 68 and overlays at
lines 142-143.
Comment on lines
+336
to
339
| // A repair needs no elevation (it never calls `schtasks /create`), but it can | ||
| // still fail — or exit 0 while leaving stale/missing assets that never start | ||
| // the proxy. Fall back to a direct detached proxy start so the update | ||
| // never leaves the user without a running proxy — but only when the port is free. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Probe the repaired service before completing the CLI update.
The condition immediately above checks only svc.status === 0 and isServiceViable(). It does not confirm that the service answers on capturedListen.port.
The GUI path already treats exit status and viability as insufficient and calls serviceRestartServed() before skipping the fallback in src/update/job.ts Lines 850-867. Apply the same serving-health check here. Otherwise, a repair can exit successfully, leave no listener, and make runUpdate() return without starting the direct proxy.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/update/index.ts` around lines 336 - 339, Update the CLI repair-success
condition in runUpdate() to also call the serving-health probe used by the GUI
path, serviceRestartServed(), against capturedListen.port before skipping the
direct proxy fallback. Require the service to be viable and actively serving on
that captured port; otherwise preserve the existing fallback behavior.
Comment on lines
+789
to
+799
| // This skip existed because the refresh ran `ocx service install`, whose Windows | ||
| // scheduler path always reaches `schtasks /create` — elevation the GUI update worker | ||
| // (OCX_SERVICE=1) never has. `service repair` rewrites the wrapper assets and | ||
| // restarts the EXISTING task with no `/create`, so the reason no longer applies and | ||
| // skipping would leave the dashboard-triggered update — the most common Windows | ||
| // path — with a stale service it could have refreshed. | ||
| // | ||
| // Only a caller that still passes install argv keeps the old behavior. | ||
| const refreshRegisters = (svcArgs ?? []).includes("install"); | ||
| if ((io.platform ?? process.platform) === "win32" && process.env.OCX_SERVICE === "1" && refreshRegisters) { | ||
| updateJob(job, {}, "Skipping service re-registration from the non-elevated update worker; falling back to a direct proxy start."); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use repair in the failure guidance for the default path.
refreshRegisters is false for the default service repair path. However, this function still reports “Service reinstall failed” and tells users to run ocx service install at Lines 840-843. A failed repair therefore directs users to the registration path that this change is intended to avoid.
Derive the displayed operation from refreshRegisters. Use install only for explicit legacy install arguments.
Suggested adjustment
+const serviceAction = refreshRegisters ? "install" : "repair";
...
-`Service reinstall failed ...`
+`Service ${serviceAction} failed ...`
...
-"Run 'ocx service install' ..."
+`Run 'ocx service ${serviceAction}' ...`🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/update/job.ts` around lines 789 - 799, Update the failure guidance in the
relevant updateJob flow to derive the displayed service operation from
refreshRegisters: use “install” only when explicit install arguments are
present, and use “repair” for the default path. Replace the hard-coded “Service
reinstall failed” wording and ocx service install guidance while preserving the
existing behavior for legacy install callers.
Comment on lines
+72
to
95
| // 260804: Qwen3.8-Max left preview, and Alibaba documents the preview endpoint as | ||
| // liable to be taken offline. The rename must carry EVERY capability key across both | ||
| // Alibaba providers — a rename that silently drops one degrades the model without | ||
| // failing anything else. Ablate by removing any single key below and this goes red. | ||
| test("the preview id is fully retired and its metadata moved to the stable id", () => { | ||
| for (const id of ["alibaba-token-plan", "alibaba-token-plan-intl"]) { | ||
| const entry = PROVIDER_REGISTRY.find(e => e.id === id)!; | ||
| expect(entry.models).toContain("qwen3.8-max"); | ||
| expect(entry.models).not.toContain("qwen3.8-max-preview"); | ||
| expect(entry.modelContextWindows?.["qwen3.8-max"]).toBe(983_616); | ||
| expect(entry.modelContextWindows?.["qwen3.8-max-preview"]).toBeUndefined(); | ||
| expect(entry.modelInputModalities?.["qwen3.8-max"]).toEqual(["text", "image"]); | ||
| expect(entry.preserveReasoningContentModels).toContain("qwen3.8-max"); | ||
| expect(entry.preserveReasoningContentModels).not.toContain("qwen3.8-max-preview"); | ||
| } | ||
| // The intl entry additionally carries the effort ladder. | ||
| const intl = PROVIDER_REGISTRY.find(e => e.id === "alibaba-token-plan-intl")!; | ||
| expect(intl.modelReasoningEfforts?.["qwen3.8-max"]).toEqual(["low", "high", "xhigh"]); | ||
| expect(intl.modelDefaultReasoningEfforts?.["qwen3.8-max"]).toBe("xhigh"); | ||
| // Only the Beijing entry defaults to this model; intl deliberately defaults to | ||
| // qwen3.7-max. That predates this rename and is left alone — renaming an id is not | ||
| // a licence to change which model a provider selects by default. | ||
| expect(PROVIDER_REGISTRY.find(e => e.id === "alibaba-token-plan")!.defaultModel).toBe("qwen3.8-max"); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Complete the rename regression assertions.
This test claims that the preview ID is fully retired. It does not check qwen3.8-max-preview in modelInputModalities, modelReasoningEfforts, or modelDefaultReasoningEfforts. A stale key in one of these maps could pass the test.
The comment also states that only the Beijing provider defaults to qwen3.8-max. Assert that alibaba-token-plan-intl.defaultModel remains qwen3.7-max.
[details]
for (const id of ["alibaba-token-plan", "alibaba-token-plan-intl"]) {
const entry = PROVIDER_REGISTRY.find(e => e.id === id)!;
expect(entry.models).not.toContain("qwen3.8-max-preview");
expect(entry.modelContextWindows?.["qwen3.8-max-preview"]).toBeUndefined();
+ expect(entry.modelInputModalities?.["qwen3.8-max-preview"]).toBeUndefined();
+ expect(entry.modelReasoningEfforts?.["qwen3.8-max-preview"]).toBeUndefined();
+ expect(entry.modelDefaultReasoningEfforts?.["qwen3.8-max-preview"]).toBeUndefined();
expect(entry.preserveReasoningContentModels).not.toContain("qwen3.8-max-preview");
}
const intl = PROVIDER_REGISTRY.find(e => e.id === "alibaba-token-plan-intl")!;
+ expect(intl.defaultModel).toBe("qwen3.7-max");[/details]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 260804: Qwen3.8-Max left preview, and Alibaba documents the preview endpoint as | |
| // liable to be taken offline. The rename must carry EVERY capability key across both | |
| // Alibaba providers — a rename that silently drops one degrades the model without | |
| // failing anything else. Ablate by removing any single key below and this goes red. | |
| test("the preview id is fully retired and its metadata moved to the stable id", () => { | |
| for (const id of ["alibaba-token-plan", "alibaba-token-plan-intl"]) { | |
| const entry = PROVIDER_REGISTRY.find(e => e.id === id)!; | |
| expect(entry.models).toContain("qwen3.8-max"); | |
| expect(entry.models).not.toContain("qwen3.8-max-preview"); | |
| expect(entry.modelContextWindows?.["qwen3.8-max"]).toBe(983_616); | |
| expect(entry.modelContextWindows?.["qwen3.8-max-preview"]).toBeUndefined(); | |
| expect(entry.modelInputModalities?.["qwen3.8-max"]).toEqual(["text", "image"]); | |
| expect(entry.preserveReasoningContentModels).toContain("qwen3.8-max"); | |
| expect(entry.preserveReasoningContentModels).not.toContain("qwen3.8-max-preview"); | |
| } | |
| // The intl entry additionally carries the effort ladder. | |
| const intl = PROVIDER_REGISTRY.find(e => e.id === "alibaba-token-plan-intl")!; | |
| expect(intl.modelReasoningEfforts?.["qwen3.8-max"]).toEqual(["low", "high", "xhigh"]); | |
| expect(intl.modelDefaultReasoningEfforts?.["qwen3.8-max"]).toBe("xhigh"); | |
| // Only the Beijing entry defaults to this model; intl deliberately defaults to | |
| // qwen3.7-max. That predates this rename and is left alone — renaming an id is not | |
| // a licence to change which model a provider selects by default. | |
| expect(PROVIDER_REGISTRY.find(e => e.id === "alibaba-token-plan")!.defaultModel).toBe("qwen3.8-max"); | |
| }); | |
| // 260804: Qwen3.8-Max left preview, and Alibaba documents the preview endpoint as | |
| // liable to be taken offline. The rename must carry EVERY capability key across both | |
| // Alibaba providers — a rename that silently drops one degrades the model without | |
| // failing anything else. Ablate by removing any single key below and this goes red. | |
| test("the preview id is fully retired and its metadata moved to the stable id", () => { | |
| for (const id of ["alibaba-token-plan", "alibaba-token-plan-intl"]) { | |
| const entry = PROVIDER_REGISTRY.find(e => e.id === id)!; | |
| expect(entry.models).toContain("qwen3.8-max"); | |
| expect(entry.models).not.toContain("qwen3.8-max-preview"); | |
| expect(entry.modelContextWindows?.["qwen3.8-max"]).toBe(983_616); | |
| expect(entry.modelContextWindows?.["qwen3.8-max-preview"]).toBeUndefined(); | |
| expect(entry.modelInputModalities?.["qwen3.8-max"]).toEqual(["text", "image"]); | |
| expect(entry.modelInputModalities?.["qwen3.8-max-preview"]).toBeUndefined(); | |
| expect(entry.modelReasoningEfforts?.["qwen3.8-max-preview"]).toBeUndefined(); | |
| expect(entry.modelDefaultReasoningEfforts?.["qwen3.8-max-preview"]).toBeUndefined(); | |
| expect(entry.preserveReasoningContentModels).toContain("qwen3.8-max"); | |
| expect(entry.preserveReasoningContentModels).not.toContain("qwen3.8-max-preview"); | |
| } | |
| // The intl entry additionally carries the effort ladder. | |
| const intl = PROVIDER_REGISTRY.find(e => e.id === "alibaba-token-plan-intl")!; | |
| expect(intl.modelReasoningEfforts?.["qwen3.8-max"]).toEqual(["low", "high", "xhigh"]); | |
| expect(intl.modelDefaultReasoningEfforts?.["qwen3.8-max"]).toBe("xhigh"); | |
| expect(intl.defaultModel).toBe("qwen3.7-max"); | |
| // Only the Beijing entry defaults to this model; intl deliberately defaults to | |
| // qwen3.7-max. That predates this rename and is left alone — renaming an id is not | |
| // a licence to change which model a provider selects by default. | |
| expect(PROVIDER_REGISTRY.find(e => e.id === "alibaba-token-plan")!.defaultModel).toBe("qwen3.8-max"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/alibaba-intl-token-plan.test.ts` around lines 72 - 95, Extend the
rename regression test around the existing PROVIDER_REGISTRY assertions to
verify qwen3.8-max-preview is absent from modelInputModalities,
modelReasoningEfforts, and modelDefaultReasoningEfforts for both Alibaba
providers, while confirming the stable qwen3.8-max entries retain their
metadata. Also assert that alibaba-token-plan-intl.defaultModel remains
qwen3.7-max, preserving the existing Beijing default assertion.
Comment on lines
+388
to
+437
| // 260804 #970: the Windows GUI update worker (OCX_SERVICE=1, never elevated) used to | ||
| // skip the service refresh entirely, because it ran `service install` whose scheduler | ||
| // path always reaches `schtasks /create`. `repair` never calls /create, so the skip's | ||
| // reason is gone and the dashboard-triggered update — the most common Windows path — | ||
| // must actually refresh the service. Ablate by restoring the unconditional skip: | ||
| // runService is then never called and this goes red. | ||
| test("a non-elevated Windows update worker repairs the service instead of skipping it", async () => { | ||
| const ranService: string[][] = []; | ||
| const spawned: Array<{ port: number }> = []; | ||
| const job: UpdateJobState = { | ||
| id: "svc-win-repair", | ||
| status: "restarting", | ||
| startedAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| currentVersion: "2.7.42", | ||
| latestVersion: "2.7.43", | ||
| channel: "latest", | ||
| installer: "npm", | ||
| restart: true, | ||
| command: "", | ||
| log: [], | ||
| }; | ||
| writeFileSync(updateJobPath(job.id), JSON.stringify(job)); | ||
| const prevService = process.env.OCX_SERVICE; | ||
| process.env.OCX_SERVICE = "1"; | ||
| try { | ||
| await restartAfterUpdateForTests(job, { port: 19998, hostname: "127.0.0.1" }, { | ||
| platform: "win32", | ||
| serviceInstalledFn: () => true, | ||
| serviceViableFn: () => true, | ||
| waitForPort: async () => true, | ||
| probeProxy: async () => true, | ||
| runService: (_j, _bin, args) => { | ||
| ranService.push(args); | ||
| return { status: 0 }; | ||
| }, | ||
| spawnStart: (_job, _installer, port) => { | ||
| spawned.push({ port: port ?? 0 }); | ||
| }, | ||
| }); | ||
| // The refresh ran, and it ran the non-registering subcommand. | ||
| expect(ranService.length).toBe(1); | ||
| expect(ranService[0]).toContain("repair"); | ||
| expect(ranService[0]).not.toContain("install"); | ||
| } finally { | ||
| if (prevService === undefined) delete process.env.OCX_SERVICE; | ||
| else process.env.OCX_SERVICE = prevService; | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert that the direct-start fallback is not used.
The test records direct-start calls in spawned, but it never checks the array. A regression can run service repair and then invoke spawnStart while all current assertions still pass.
Add this assertion after the repair assertions:
+ expect(spawned).toHaveLength(0);As per path instructions, tests under tests/** must add focused regression coverage near changed src/** behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 260804 #970: the Windows GUI update worker (OCX_SERVICE=1, never elevated) used to | |
| // skip the service refresh entirely, because it ran `service install` whose scheduler | |
| // path always reaches `schtasks /create`. `repair` never calls /create, so the skip's | |
| // reason is gone and the dashboard-triggered update — the most common Windows path — | |
| // must actually refresh the service. Ablate by restoring the unconditional skip: | |
| // runService is then never called and this goes red. | |
| test("a non-elevated Windows update worker repairs the service instead of skipping it", async () => { | |
| const ranService: string[][] = []; | |
| const spawned: Array<{ port: number }> = []; | |
| const job: UpdateJobState = { | |
| id: "svc-win-repair", | |
| status: "restarting", | |
| startedAt: new Date().toISOString(), | |
| updatedAt: new Date().toISOString(), | |
| currentVersion: "2.7.42", | |
| latestVersion: "2.7.43", | |
| channel: "latest", | |
| installer: "npm", | |
| restart: true, | |
| command: "", | |
| log: [], | |
| }; | |
| writeFileSync(updateJobPath(job.id), JSON.stringify(job)); | |
| const prevService = process.env.OCX_SERVICE; | |
| process.env.OCX_SERVICE = "1"; | |
| try { | |
| await restartAfterUpdateForTests(job, { port: 19998, hostname: "127.0.0.1" }, { | |
| platform: "win32", | |
| serviceInstalledFn: () => true, | |
| serviceViableFn: () => true, | |
| waitForPort: async () => true, | |
| probeProxy: async () => true, | |
| runService: (_j, _bin, args) => { | |
| ranService.push(args); | |
| return { status: 0 }; | |
| }, | |
| spawnStart: (_job, _installer, port) => { | |
| spawned.push({ port: port ?? 0 }); | |
| }, | |
| }); | |
| // The refresh ran, and it ran the non-registering subcommand. | |
| expect(ranService.length).toBe(1); | |
| expect(ranService[0]).toContain("repair"); | |
| expect(ranService[0]).not.toContain("install"); | |
| } finally { | |
| if (prevService === undefined) delete process.env.OCX_SERVICE; | |
| else process.env.OCX_SERVICE = prevService; | |
| } | |
| }); | |
| // 260804 `#970`: the Windows GUI update worker (OCX_SERVICE=1, never elevated) used to | |
| // skip the service refresh entirely, because it ran `service install` whose scheduler | |
| // path always reaches `schtasks /create`. `repair` never calls /create, so the skip's | |
| // reason is gone and the dashboard-triggered update — the most common Windows path — | |
| // must actually refresh the service. Ablate by restoring the unconditional skip: | |
| // runService is then never called and this goes red. | |
| test("a non-elevated Windows update worker repairs the service instead of skipping it", async () => { | |
| const ranService: string[][] = []; | |
| const spawned: Array<{ port: number }> = []; | |
| const job: UpdateJobState = { | |
| id: "svc-win-repair", | |
| status: "restarting", | |
| startedAt: new Date().toISOString(), | |
| updatedAt: new Date().toISOString(), | |
| currentVersion: "2.7.42", | |
| latestVersion: "2.7.43", | |
| channel: "latest", | |
| installer: "npm", | |
| restart: true, | |
| command: "", | |
| log: [], | |
| }; | |
| writeFileSync(updateJobPath(job.id), JSON.stringify(job)); | |
| const prevService = process.env.OCX_SERVICE; | |
| process.env.OCX_SERVICE = "1"; | |
| try { | |
| await restartAfterUpdateForTests(job, { port: 19998, hostname: "127.0.0.1" }, { | |
| platform: "win32", | |
| serviceInstalledFn: () => true, | |
| serviceViableFn: () => true, | |
| waitForPort: async () => true, | |
| probeProxy: async () => true, | |
| runService: (_j, _bin, args) => { | |
| ranService.push(args); | |
| return { status: 0 }; | |
| }, | |
| spawnStart: (_job, _installer, port) => { | |
| spawned.push({ port: port ?? 0 }); | |
| }, | |
| }); | |
| // The refresh ran, and it ran the non-registering subcommand. | |
| expect(ranService.length).toBe(1); | |
| expect(ranService[0]).toContain("repair"); | |
| expect(ranService[0]).not.toContain("install"); | |
| expect(spawned).toHaveLength(0); | |
| } finally { | |
| if (prevService === undefined) delete process.env.OCX_SERVICE; | |
| else process.env.OCX_SERVICE = prevService; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/update-job.test.ts` around lines 388 - 437, Extend the test around
restartAfterUpdateForTests to assert that the direct-start fallback was not
invoked: after the existing ranService repair assertions, verify the spawned
array is empty. Keep the test focused on the non-elevated Windows service-repair
path.
Source: Path instructions
Comment on lines
+58
to
+72
| test("both paths abort when the stop fails, and REPAIR a managed service after success", () => { | ||
| expect(updateSource).toContain("aborting the update"); | ||
| // The update path now uses serviceReinstallArgs() to preserve the chosen backend. | ||
| // 260804 #970: the refresh must not re-register. `install` reaches `schtasks /create` | ||
| // on Windows scheduler backends, which a non-elevated updater cannot run — it would | ||
| // stop a working proxy and then fail to bring its service back. | ||
| expect(updateSource).toContain("serviceReinstallArgs()"); | ||
| expect(launcherSource).toContain("aborting the update"); | ||
| // The launcher reads service-state.json to preserve the backend choice on reinstall. | ||
| expect(launcherSource).toContain("serviceReinstallArgs"); | ||
| // The launcher reads the state path for both service-installed detection and backend choice. | ||
| expect(launcherSource).toContain('"service", "repair"'); | ||
| // The launcher still reads service-state.json for service-installed detection, and | ||
| // for the backend choice on the genuinely-absent install fallback. | ||
| expect(launcherSource).toContain('"service-state.json"'); | ||
| // That marker can be STALE, so the fallback asks for structured state rather than | ||
| // parsing a failure message; bin/ocx.mjs is plain Node and cannot import | ||
| // diagnoseService(), so it reads startup.serviceInstalled from `status --json`. | ||
| expect(launcherSource).toContain("startup?.serviceInstalled"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the repair result, not only the helper name.
expect(updateSource).toContain("serviceReinstallArgs()") proves only that the update path calls a helper. It does not prove that the helper returns ["service", "repair"]. The test can pass if the helper still returns ["service", "install"].
Add a direct assertion for serviceReinstallArgs() in tests/service.test.ts, or execute the update path with a mocked child process and assert the actual argv.
As per path instructions, tests under tests/** must add focused regression coverage near changed src/** behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/update-stop-first.test.ts` around lines 58 - 72, Strengthen the
regression coverage for service reinstall behavior: add a focused assertion near
the service-related tests in service.test.ts that directly verifies
serviceReinstallArgs() returns the repair argument vector, rather than only
checking that updateSource references the helper. Preserve the existing
update-stop-first assertions and ensure the test would fail if the helper
returned service install.
Source: Path instructions
Comment on lines
+260
to
+262
| test("explicit installs still preserve the recorded backend", () => { | ||
| // On a dev machine without a native install-state the accessor maps to scheduler. | ||
| expect(serviceReinstallArgs()).toEqual(["service", "install"]); | ||
| expect(serviceInstallArgs()).toEqual(["service", "install"]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a native-backend assertion for serviceInstallArgs().
This test only verifies the scheduler default. It does not verify the changed contract that an existing native installation returns ["service", "install", "--native"].
Set up recorded native service state in an isolated test case. Assert the native arguments. Keep the scheduler assertion as a separate case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/winsw.test.ts` around lines 260 - 262, Add a separate isolated test for
serviceInstallArgs() that records an existing native service installation and
asserts ["service", "install", "--native"]. Keep the current scheduler-default
assertion in its own test, and use the existing test setup/state helpers to
avoid cross-test contamination.
Source: Path instructions
lidge-jun
added a commit
that referenced
this pull request
Aug 4, 2026
…troubleshooting docs Third round of the same audit. The reviewer re-checked my first fix and found three more surfaces still advising re-registration for an installed service. update/job.ts: the refresh it runs is 'service repair' (serviceReinstallArgs), but the failure message told Windows users to run 'ocx service install' as administrator. That is the exact post-update path #980 changed — advising install there sends the user to a UAC prompt and a possible WinSW-to-scheduler switch to fix a service that is already registered. It now names the command that actually failed, on every platform, so its output explains why. doctor.ts proxyDownRestartHint(): took only serviceViable, which conflates 'no service at all' with 'registered but stale or stopped'. Only the first wants install. It now takes serviceInstalled/serviceConflict and points an installed service at repair; a conflict still gets install because repairService() refuses a two-manager conflict and the user must uninstall first. docs-site troubleshooting/windows-memory.md in all five locales: the paragraph explicitly discusses 'an already-installed service' and then said to re-run 'ocx service install' to re-bake OPENCODEX_BUN_PATH. repairService() rewrites exactly those scheduler/WinSW assets in place, so repair is both correct and cheaper. The reviewer also flagged autostart-health, which bee1cc7 already fixed — it audited the earlier commit. Its note that the existing stale-service test was vacuous (asserting status but never the resulting command) was accurate, and the guard added in that commit closes it. 196 pass / 0 fail across doctor, autostart-health, update-job, service, and update-stop-first.
chrisae9
pushed a commit
to chrisae9/opencodex
that referenced
this pull request
Aug 4, 2026
…hd hint Follow-up to lidge-jun#980, found by an adversarial audit of the merged dev state. `ocx service repair` ships and is what the update path now runs, but the CLI reference never documented it: the subcommand heading, the table and the example block all listed install/start/stop/status/uninstall only, across all five locales. Worse, the English status example literally read `Repair: ocx service install`. That advice is wrong in a way that costs the user something. `repair` refreshes the installed backend in place; plain `install` re-registers, which needs elevation on Windows and can switch a WinSW install to Task Scheduler (src/service.ts:519, :1760). A user hand-recovering a service after an update would hit a UAC prompt and possibly a backend switch, both avoidable. src/service.ts:2478 had the same stale text in the launchd older-plist hint — the sibling at :1676 was updated in lidge-jun#980 and this one was missed. Both are installed-service recovery paths, so both say repair now. First-install, absent-service and backend-switch messages still say install, which is correct: repair refuses a service that is not installed.
Open
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 in001–003, because three of them found real defects in my own designs.#956 — NIM vision classification, and why #964 could not be carried
The
nvidiaentry declared nonoVisionModels, soplanVisionSidecarnever 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:
thinkingmachines/inklingminimaxai/minimax-m3moonshotai/kimi-k2.6moonshotai/kimi-k2.5stepfun-ai/step-3.7-flashmistralai/mistral-medium-3.5-128bListing 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 includesnvidia/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 fromnoVisionModelsis 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
shouldExposeRoutedModelfilters 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 updatestops the proxy, then brought the service back withocx service install. The Windows scheduler installer always reachesschtasks /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 afterocx stop. Verified across all three platforms — stop never deregisters (macOS unloads the plist, Windows calls/end, Linux callssystemctl stop; deletion lives only in the uninstall paths).Two things a plain argv swap would have missed:
/createwould 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.mjsinfers service presence from a possibly-staleservice-state.json. Repair correctly refuses that case, but its thrownErroris indistinguishable from any other failure there, so message-matching was unimplementable and a blanket install-on-failure would resurrect the elevation prompt. It now readsstartup.serviceInstalledfrom thestatus --jsonsubprocess it already spawns — the file is plain Node ESM and cannot importdiagnoseService().qwen3.8-max-preview→qwen3.8-maxQwen3.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
0rather than inheriting the reseller's0.15.Verification
bun x tsc --noEmitclean;bun run privacy:scanpassedbun run test: 7753 pass / 8 skip / 0 fail across 508 filesnoVisionModelsfails 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
qwen3.8-maxand updated its published pricing.Improvements
ocx service repair, preserving configuration and improving Windows support.Tests