fix(office): download-path fixes, drop the vendor name from copy, kill a suite flake - #235
Merged
Merged
Conversation
Two download-path bugs found while reading `codevhub skill office`, plus
the README section the command never got.
- download.ts: `Number(res.headers.get("content-length"))` is `Number(null)`
=== 0 when the header is absent, and 0 is finite — so the `opts.size`
fallback was unreachable and a chunked response reported `total = offset`
(0 on a fresh download), rendering as Infinity%. Read the header first and
only coerce when it is present.
- office.ts: the progress printer now treats a 0 total like an unknown one,
so an empty body can't reintroduce the NaN%/Infinity% line.
- office.ts: always-refetch the setup script now drops `<script>.partial`
as well as the finished file. downloadFile resumes from a partial via
Range, so a leftover from an interrupted run would splice stale bytes
onto a republished script — and no expected checksum is passed that
could catch it.
- README: document `skill office` — per-OS bundle sizes, the resumable
~/.codev-hub/office cache, the --download-only/--platform/--dir staging
workflow, and the pass-through installer flags.
Tests go next to the existing runSkillOffice coverage in
tests/lib/download.test.ts, against the local HTTP server it already
stands up: a new `omitContentLength` server flag drives the two
content-length cases, the stale-.partial case asserts the script matches
byte-for-byte with no Range header sent, and a `withPlatform` helper
pins the PowerShell argv shape (otherwise unreachable off Windows, since
a cross-platform --platform forces download-only) plus the unsupported-OS
and unknown-flag exits. Each new test was checked to fail with its fix
reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`skill office` named the bundled skills by their upstream package names in
both the help text and the README, and the 1M-model entry in model-limits.ts
put the vendor name in the shipped bundle as a plain table key.
- help.ts / README: describe the bundle by what it does ("DOCX and XLSX
authoring") rather than by package name. Same for the office.ts header.
- model-limits.ts: the table key is now `M3_ID`, atob-encoded the same way
FALLBACK_MODEL already is in const.ts. Deliberately a separate constant
rather than reusing FALLBACK_MODEL — they are the same string today, and
coupling them would let a retargeted fallback silently move the 1M window
onto another model.
`grep -i minimax dist/index.js` is now empty. Model ids in tests and on the
wire are untouched: those are contracts with the gateway, not copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…exit
`SkillPushApp`'s "without raw mode" test failed roughly 1 full-suite run in
8, never in isolation. Instrumenting it to dump Ink's frame history showed
the refusal message *does* render — and that the app then exits ~20ms later,
at which point Ink writes an **empty** final frame. `lastFrame()` returns ""
from then on, so a 20ms poll that misses that 20ms window can never succeed
and burns its whole budget reporting a hang.
- tests/helpers/raw-mode.tsx: new `lastNonEmptyFrame`, stable once the app
is gone. `SkillPushApp` and `SkillPullApp`'s no-raw-mode tests assert
against it instead of `lastFrame()`. Verified by delaying the first poll
past the exit: `lastFrame()` then fails 100%, the settled frame passes.
The whole frame history is deliberately NOT the unit — the picker renders
for one frame before the effect replaces it, so SkillPullApp's
`not.toContain("❯ ")` is an assertion about the settled frame, not history.
- tests/helpers/raw-mode.tsx: `renderWithoutRawMode` renders an inert tree
first so the flag is already false when the component mounts. Four tests
mounted the real component and flipped isTTY underneath it — a state no
terminal reaches, and one ink's `handleSetRawMode` throws from when
`useInput`'s cleanup runs after the flip. A fidelity fix rather than the
flake fix; it also de-duplicates the same snippet across four files.
14 consecutive full-suite runs clean, against a ~1-in-8 baseline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Three independent things, one commit each.
1.
codevhub skill officedownload pathopts.sizewas unreachable (src/lib/download.ts).Number(res.headers.get("content-length"))isNumber(null)=== 0 when the header is absent, and 0 is finite — so the documented fallback could never run and a chunked response reportedtotal = offset(0 on a fresh download), rendering asInfinity%. The header is now read first and only coerced when present. The progress printer also treats a0total like an unknown one."Always refetch the setup script" was defeated by a leftover
.partial(src/lib/office.ts). Deleting the finished file still leftdownloadFileresuming from<script>.partialvia Range, so an interrupted run could splice stale bytes onto a script that has since been republished — and no expected checksum is passed that could catch it.README gained a section for the command (documented only in
help.tsuntil now): per-OS bundle sizes, the resumable~/.codev-hub/officecache, the--download-only/--platform/--dirstaging workflow, and the pass-through installer flags.Tests join the existing
runSkillOfficecoverage intests/lib/download.test.ts, against the local HTTP server it already stands up — a newomitContentLengthserver flag for the two content-length cases, the stale-.partialcase, the PowerShell argv shape (unreachable off Windows, so the host is stubbed), and the unsupported-OS / unknown-flag exits. Each was verified to fail with its fix reverted.2. Vendor name out of user-facing copy
skill officenamed the bundled skills by their upstream package names in the help text and README;model-limits.tsput the vendor name in the shipped bundle as a plain table key. Help and README now describe the bundle by what it does, and the table key isatob-encoded the wayFALLBACK_MODELalready is inconst.ts— as its own constant, since coupling it toFALLBACK_MODELwould let a retargeted fallback silently move the 1M window onto another model.grep -i minimax dist/index.jsis now empty. Model ids in tests and on the wire are untouched; those are gateway contracts.3. The
SkillPushAppflakeFlagged in the first version of this PR as pre-existing and out of scope — it's fixed here.
It failed ~1 full-suite run in 8 and never in isolation. Instrumenting the test to dump Ink's frame history showed the refusal message does render, and that the app exits ~20ms later — at which point Ink writes an empty final frame.
lastFrame()returns""from then on, so a 20ms poll that misses that 20ms window can never succeed; it burns its budget and reports what looks like a hang.Fixed with
lastNonEmptyFrame, which is stable once the app is gone. Verified by delaying the first poll past the exit:lastFrame()then fails 100%, the settled frame passes. The whole frame history is deliberately not the unit — a prompt renders for one frame before the effect replaces it, soSkillPullApp'snot.toContain("❯ ")is an assertion about the settled frame.The same commit adds
renderWithoutRawMode, which mounts an inert tree first soisTTYis already false when the component under test mounts. Four tests mounted the real component and flipped the flag underneath it — a state no terminal reaches, and one ink'shandleSetRawModethrows from whenuseInput's cleanup runs after the flip. That is a fidelity fix rather than the flake fix, and it de-duplicates the same snippet across four files.14 consecutive full-suite runs clean, against the ~1-in-8 baseline measured on
main.Validation
pnpm fix,pnpm typecheck,pnpm build+node dist/index.js --versionclean;pnpm test1367 passed / 2 skipped across 76 files.🤖 Generated with Claude Code