build(COMPASS-28): move to @oclif/core 4

[timdawborn]

Ticket

COMPASS-28 — Bump spot's NodeJS minimum from 18 to 22

PR 9 of 9 — the last. Staying in draft until the whole chain is verified.

Stacked on #2729 → #2724 → #2723 → #2722 → #2721 → #2720 → #2719. This PR's own commit is 932ec16.

What

Five deprecated packages out, three in:

Out	In
@oclif/command ^1.8.0	@oclif/core ^4.13.3
@oclif/config ^1.17.0
@oclif/errors ^1.3.6
@oclif/help ^1.0.15
@oclif/plugin-help ^3.2.3	@oclif/plugin-help ^6.2.58
@oclif/dev-cli ^1.26.0 (dev)	oclif ^4.23.30 (dev)

@oclif/core is held at 4.13.3, not 4.14.0 — that was published today and does not clear minimumReleaseAge. Supersedes Dependabot #2699.

Across the nine commands: flags → Flags, static args from an array of descriptors to an object of Args.string(...), this.parse awaited, and the flags.Input<flags.Output> annotations dropped (v4 has no equivalent and infers the same). bin/run moves to core's own run/flush/handle. prepack calls oclif manifest and oclif readme.

flags.enum is gone, and its successor is a trap

Flags.option looks like the replacement but returns a factory, not a flag. Assigning it where a flag belongs throws at startup:

TypeError: Cannot assign to read only property 'name' of function '(options = {}) => ({...})'

That is how spot lint failed first — the build was clean and the suite was green. The v1 shape maps to Flags.string({ options }), which restricts values the same way. Checked all three cases:

Invocation	Exit
lint <contract>	0
lint --no-trailing-forward-slash=error	0
lint --no-trailing-forward-slash=bogus	2 — Expected ... to be one of: error, warn, off

Two things only the image showed

1. Every command failed before doing anything.

SystemError: A system error occurred: uv_os_get_passwd returned ENOENT

@oclif/core reads $SHELL and falls back to os.userInfo().shell (lib/util/os.js:37). That throws for a uid with no /etc/passwd entry — exactly the uid --user "$(id -u):$(id -g)" supplies, which is how the Dockerfile header documents running the image. It worked without --user and failed with it, which is the wrong way round for the supported usage.

ENV SHELL=/bin/sh now sits beside ENV HOME=/tmp, which exists for the same class of reason and carries a comment saying so.

2. The two parity assertions I expected to fail actually hold — but only because I checked.

Both fail open, so a changed message would have passed silently:

• The image drops mock, docs and init by deleting their compiled files and shipping no manifest. That still works under v4's loader.
• v4 still says Error: command <x> not found, which is what check-image-parity greps for with grep -qi "not found".

this.log no longer goes through process.stdout

In v4 it goes through console.log, and jest substitutes its own console that never reaches the real stream — so two specs captured an empty string while the command was printing correctly. ts-lint.spec.ts and generate.spec.ts spy on console.log now. Notably these passed in isolation and failed in the full run, which is a good argument for not trusting a single-file run.

The README is regenerated, not left for the next publish

v4's help prints FLAGS instead of OPTIONS, adds a DESCRIPTION section, and shows flags in the usage line. prepack runs oclif readme, so leaving this out would mean the committed reference silently disagreeing with --help until a release rewrote it.

All 284 changed lines fall inside the <!-- commands --> and <!-- toc --> blocks — verified programmatically that zero changes land outside them, so no hand-written content moved.

Generated output does not change

Check	Result
318-artifact comparison vs the pre-oclif baseline	identical
parity, 7 generator × language combinations	byte-for-byte identical

The 318-artifact run matters here beyond generation: it drives generate 318 times through the rewritten Args/Flags parsing.

How this was verified

Check	Result
pnpm build	0
pnpm test	0 — 55 suites, 558 tests, 44 snapshots
pnpm lint:check	0
pnpm build-docs	0
docker build + check-image-parity	0 — all checks passed
oclif manifest	0 — 9 commands, validate args ["spot_contract"], flags ["help"]
oclif readme	0 — changes confined to the marker blocks
image with --user on a clean workspace	Contract is valid, Generated .../api.yml
--prod install + --help / validate / lint / ts-lint	all 0
oclif error path exit code	2, unchanged

The prepack steps are hand-verified because nothing in CI runs the pack lifecycle — the publish job would have been the first thing to execute them.

[timdawborn]

Automated review — /pr-review-toolkit:review-pr

Agents run: code-reviewer, pr-test-analyzer. Scoped to this PR's own commit (932ec16).

This PR ships a live regression. Both agents found it independently and I reproduced it against the built CLI.

🔴 Critical (1)

cli/src/commands/lint.ts:81 — warn: this.warn is an unbound method that dereferences this. Every warn-level lint violation crashes spot lint.

v1's Command.warn was warn(input) { Errors.warn(input) } — no this, so a bare reference was safe. v4's is warn(input) { if (!this.jsonEnabled()) Errors.warn(input); return input }. findLintViolations calls it as logger.warn(...), so this is the object literal.

$ node ./bin/run lint lib/src/linting/rules/__spec-examples__/no-trailing-forward-slash/trailing-forward-slash.ts
    TypeError: Cannot read properties of undefined (reading 'jsonEnabled')
  exit: 1

Not an edge case: lintConfig ships no-omittable-fields-within-response-bodies and no-trailing-forward-slash at "warn" by default, so a plain spot lint on a normal contract hits it with no flags.

It escaped both gates, and the reasons are worth recording:

• there is no lint.spec.ts at all;
• the crash exits 1, not the 2 that check-image-parity asserts for the oclif error path, and parity's lint cases use a clean contract plus --no-trailing-forward-slash=error — so both bypass the warn branch entirely.

Fix, mirroring the error arrow two lines above (verified: prints both warnings, exits 0):

warn: (msg: string) => {
  this.warn(msg);
}

This is the only command handing a this.* callback to library code, so it wants a lint.spec.ts covering the warning path, plus a warn-severity case in the parity script.

🟡 Important (3)

2. lint.ts:44 — Record<string, any> is a real narrowing loss, and ironically the type that would have caught this PR's other bug. v1's flags.Input<any> constrained values to flag descriptors. Record<string, any> accepts anything — including the Flags.option factory whose accidental assignment the description recounts as the first spot lint failure. Interfaces.FlagInput restores the constraint and lets the no-explicit-any disable go: verified const finalFlags: Interfaces.FlagInput = {...} typechecks clean.

3. Dockerfile:86-90 — ENV SHELL=/bin/sh fixes the image, not the CLI. The crash is in @oclif/core's util/os.js:37 (process.env.SHELL ?? userInfo().shell), so it fires for any uid absent from /etc/passwd — a Kubernetes runAsUser, or npx @airtasker/spot inside someone else's CI container. None of those read this Dockerfile, and a downstream image using this as a base can drop the ENV. Since ENTRYPOINT is already node /opt/spot/bin/run, process.env.SHELL ||= '/bin/sh' in bin/run covers every consumer, with the Dockerfile line as belt-and-braces. Defensible as-is, but the layer is narrower than the failure.

4. Six of nine commands have no spec — checksum, docs, init, lint, mock, validate. checksum appears zero times in check-image-parity despite shipping in the image, so its rewritten Args/this.parse/exit path is verified by nothing. One table-driven spec would cover it: per command, missing required arg → exit 2, valid invocation → success.

🔵 Suggestions (3)

• Flags.string({ options }) rejection is hand-verified only. Add to a lint.spec.ts: =bogus rejects, =off suppresses, =warn warns.
• The console.log spy cannot see warn/error — Errors.warn → ux.stderr → console.error. Correct for today's assertions, but naming the buffer out invites a future stderr assertion that silently captures nothing. Capture both streams separately.
• oclif sits at the old @oclif/dev-cli position in devDependencies, breaking the otherwise-alphabetical list.

✅ Verified sound

• All nine commands migrated correctly — no required, default, hidden or char dropped; mock keeps port default: 3010, required: true, ts-lint keeps default: ".".
• Computed arg keys are safe — ARG_API/ARG_DIR have literal types, so the keys survive into the mapped type; required: true → string and the default keeps string rather than string | undefined.
• Dropping flags.Input<flags.Output> loses no checking — it gains some. That annotation was an index signature erasing literal keys and widening values to any; v4's inference gives generate.ts flags.contract as string.
• bin/run omitting run()'s second argument is correct — v4 falls back to require.main?.filename, which is bin/run itself.
• The isolation-vs-full-run discrepancy was correctly diagnosed, not a residual order dependence: jest defaults verbose: true for a single file (CustomConsole → straight to process.stdout, old spy fired) and buffers for multi-file (BufferedConsole, flushed after the test, old spy saw nothing). Spying on console.log holds in both modes, and no process.stdout.write spies remain.
• README hunks all sit below the <!-- commands --> marker.

Recommended action

1. Fix lint.ts:81 and add a lint.spec.ts. This is a shipped crash on a default-config invocation.
2. Type finalFlags as Interfaces.FlagInput — it is the guard that would have caught the factory mistake.
3. Decide on the SHELL layer; at minimum note why the Dockerfile is the chosen one.
4. Add the missing-command specs, starting with checksum, which nothing covers.

[timdawborn]

Addressed in 7955588. All four findings were sound; I validated each before acting.

Blocker — warn: this.warn. Reproduced on the rebased tree (TypeError: Cannot read properties of undefined (reading 'jsonEnabled')), root cause confirmed at @oclif/core/lib/command.js:235. Wrapped in an arrow; spot lint now prints the warning and exits 0. Added lint.spec.ts — mutation-checked: reverting to the unbound method gives 0 type errors and fails the new test, which is exactly why it shipped.

One thing that spec deliberately does not cover: an error-severity violation. The command answers that with process.exit(1), which takes the jest worker with it. Noted in the spec rather than worked around.

Interfaces.FlagInput. Verified it typechecks clean and that it catches the original mistake — assigning Flags.option's factory is TS2322 under FlagInput and compiles silently under Record<string, any>. So this is the type that would have caught the other bug in this PR.

ENV SHELL is the wrong layer. Agreed and fixed in bin/run. Proved it independently: an image built with the ENV SHELL line removed still runs validate successfully under --user, so the entrypoint default alone covers npx and runAsUser. Kept the Dockerfile line as belt-and-braces.

checksum untested. Confirmed zero occurrences in the parity script. Added — OK: checksum (exit 0, identical output).

Left the remaining five uncovered commands to a ticket, as discussed: a table-driven spec across nine commands is a larger change than this migration should carry.

[timdawborn]

Automated review, round 2 — /pr-review-toolkit:review-pr

Agents: code-reviewer, pr-test-analyzer. Scoped to the revised commit 7955588.

All four round-1 fixes verified sound, including the blocker. Two new Important findings, both about the code the fix sits next to.

🔴 Critical (0)

None. The blocker is genuinely fixed and genuinely pinned — reverting lint.ts:83 to warn: this.warn fails lint.spec.ts with the original TypeError, confirmed independently by both agents.

🟡 Important (3)

1. lint.ts:13 / lint.spec.ts:54 — lintConfig is module-level mutable state, and my new spec passes only because of test declaration order.

run() writes flag values into the shared lintConfig, so the --no-trailing-forward-slash=off case permanently disables that rule for the rest of the jest module instance. A plain Lint.run declared after it reports Found 0 errors and 0 warnings. The warning test passes today only because it happens to be declared first — reordering it, or adding any later warning-path case, silently asserts against a disabled rule.

This is also a live bug for any in-process caller invoking Lint.run twice. Fix: build the config inside run() from a frozen default, which fixes both. At minimum reset it in beforeEach.

2. lint.spec.ts:16 / lint.ts:92 — the constraint I documented is self-imposed, and ts-lint.ts already has the right pattern.

My docstring says the error path can't be tested because process.exit(1) kills the jest worker. The kill is real (verified). But cli/src/commands/ts-lint.ts:53-55 in this same repo already documents the alternative and uses it:

"…truncates the report mid-stream. Setting the code lets Node flush and exit on its own." → process.exitCode = 1

lint.ts:92 has the same hazard — v4 routes this.log through console.log, and process.exit bypasses bin/run's flush, so Found N errors can be truncated on a CI pipe. Switching to process.exitCode = 1 removes the truncation and makes the error branch testable. The code is pre-existing; the comment framing it as immovable is new, and reads as external when it isn't.

Worth noting the error path currently has zero coverage anywhere: parity's lint --no-trailing-forward-slash=error runs against a clean contract, so escalating severity still yields no violation. Neither parity case reaches errorCount > 0.

3. lint.spec.ts:63 — the regex is loose in the wrong direction. /Expected .* to be one of/ matches oclif's FlagInvalidOptionError verbatim, so a reworded upstream message would fail loudly rather than pass silently — fine. But .* swallows both the flag name and the allowed set: change options at lint.ts:54 to ["error"] and this test still passes. Tighten to /--no-trailing-forward-slash=bogus to be one of: error, warn, off/ to pin the set.

🔵 Suggestions (2)

• The deferral note should be more explicit. mock, docs and init have no execution coverage of any kind in any harness — parity only asserts they are absent from the image. checksum and validate are covered at the exit-code level by parity. Worth saying which is which in the ticket.
• The parity checksum case does not pin the hash value — a change shifting it on both sides passes. Acceptable (hash.spec.ts covers the algorithm), though that spec is only relational: no golden constant exists anywhere in the repo.

✅ Verified sound

• Interfaces.FlagInput is the correct type, not just a replacement: its default parameter keeps the index signature buildFlags needs while rejecting factories. A narrower type isn't available (Flags.help is a BooleanFlag, the rules are OptionFlags), and Command.flags expects FlagInput anyway.
• No other unbound this.* handoff exists. grep -nE "this\.(log|warn|error|exit|debug)([^(a-zA-Z]|$)" across all nine commands and lib/src returns nothing, and findLintViolations is the only library function taking command methods as callbacks. So the round-1 fix was the whole of that class of bug.
• process.env.SHELL ||= … has no consumer impact. bin/run is not the library entrypoint — main is build/index.js ← index.ts → lib/src/lib, and grep -rn "@oclif" lib/src index.ts is empty. An importing consumer never loads it. bin/run.cmd shells to it, so Windows is covered.
• The console.error spy is load-bearing, not redundant — Command.warn → Errors.warn → ux/write.stderr → console.error. Without it the warning text lands on jest's own console. Note only the toContain("trailing forward slash") assertion kills the mutant.
• Command.error in v4 reads no this, so it dodged the round-1 bug by luck rather than by the arrow wrapper.

Recommended action

1. Fix lintConfig — an order-dependent spec is worse than no spec, and it's a real bug for in-process callers.
2. Switch lint.ts to process.exitCode = 1, matching ts-lint.ts, then add the error-path case. That removes both the truncation hazard and the coverage hole.
3. Tighten the options regex.