Skip to content
This repository was archived by the owner on Jun 11, 2026. It is now read-only.

Migrate activity and upload to pure command runners#92

Merged
patosullivan merged 2 commits into
masterfrom
po/testing-strategy-phase-3-command-handler-migration-and-coverage
May 27, 2026
Merged

Migrate activity and upload to pure command runners#92
patosullivan merged 2 commits into
masterfrom
po/testing-strategy-phase-3-command-handler-migration-and-coverage

Conversation

@patosullivan

@patosullivan patosullivan commented May 22, 2026

Copy link
Copy Markdown
Member

Summary

This PR introduces a testable command-runner pattern and migrates activity and upload as pilot commands.

The goal is to stop growing the legacy import-side-effect CLI shape, where command modules read globals, call process.exit(), and are difficult to test without subprocesses. The new pattern keeps command logic in pure run(args, deps): Promise<number> modules, with command-specific runtime deps wired through the top-level CLI.

This is PR 1 of 3 for this phase.

What Changed

  • Added shared command primitives for typed command errors, usage errors, output writers, help output, and unexpected-error formatting.
  • Added command-specific runtime dependency factories for activity and upload.
  • Migrated activity into scripts/commands/activity.ts with injected auth, activity API calls, and output formatting.
  • Migrated upload into scripts/commands/upload.ts with injected stdin, filesystem, fetch, Blob creation, and upload API operations.
  • Removed standalone activity/upload script entrypoints; these commands now run through scripts/main.ts.
  • Updated scripts/main.ts so top-level tlon activity ... and tlon upload ... dispatch through explicit command runners instead of import-time side effects.
  • Added a command-contract test to prevent migrated pure command modules from using adapter-only globals like process, console, fetch, or direct fs imports.
  • Expanded in-process unit coverage for activity and upload, including URL upload failure modes and stable command-error output.
  • Expanded hermetic source and built-binary coverage for migrated activity/upload dispatch through the top-level CLI.
  • Split activity and upload process-backed runtime wiring into command-specific runtime modules.

Scope Notes

  • This PR intentionally migrates only activity and upload.
  • Legacy command families still use the existing import-side-effect path until later follow-up work.
  • upload is included as the IO-heavy pilot because it proves the dependency-injection shape for stdin, filesystem, fetch, Blob construction, and upload API behavior.
  • activity is included as the read-only pilot because it proves side-effect import removal and API/output injection.

Tests

Validated locally:

bun test scripts/commands/activity.test.ts scripts/commands/upload.test.ts scripts/commands/command-contract.test.ts
npm run typecheck
npm run test:unit
npm run test:integration
npm run build:smoke
npm run check
git diff --check

@patosullivan patosullivan requested a review from arthyn May 22, 2026 21:48
@patosullivan patosullivan force-pushed the po/testing-strategy-phase-3-command-handler-migration-and-coverage branch from 65cfe4e to 2b8bbb3 Compare May 26, 2026 20:45
@patosullivan patosullivan changed the base branch from po/testing-strategy-phase-2-credential-resolver-auth-policy to master May 26, 2026 20:46

@arthyn arthyn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand the gist of this PR which is basically make all of this more testable. I don't understand why we'd keep the standalone scripts we convert though, since this is intended to always be run from main.ts. Also I'm not a huge fan of having to create a deps bundles in the runtime file (esp not if it means defining command specific helpers there). could we maybe just have something like import * as api from '@tloncorp/api' and then pass that api object to each command to use?

Comment thread scripts/activity.ts Outdated
Comment thread scripts/upload.ts Outdated
Comment thread scripts/commands/activity.ts Outdated
Comment thread scripts/activity-runtime.ts
@patosullivan patosullivan requested a review from arthyn May 27, 2026 13:08
@patosullivan

Copy link
Copy Markdown
Member Author

I understand the gist of this PR which is basically make all of this more testable. I don't understand why we'd keep the standalone scripts we convert though, since this is intended to always be run from main.ts. Also I'm not a huge fan of having to create a deps bundles in the runtime file (esp not if it means defining command specific helpers there). could we maybe just have something like import * as api from '@tloncorp/api' and then pass that api object to each command to use?

Yeah, I agree we need a better solution on imports. Maybe we should switch over to node-style moduleResolution, but that's probably best for another PR.

For now, the activity command gets its local types from the @tloncorp/api function return types instead of just copying the shapes.

@patosullivan patosullivan merged commit 7a195f5 into master May 27, 2026
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants